Skip to content

fix(review): stop charging a regate-repair attempt before readiness is confirmed#5482

Merged
JSONbored merged 2 commits into
mainfrom
fix/regate-repair-false-exhaustion
Jul 12, 2026
Merged

fix(review): stop charging a regate-repair attempt before readiness is confirmed#5482
JSONbored merged 2 commits into
mainfrom
fix/regate-repair-false-exhaustion

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Sentry issue GITTENSORY-1E (regate_repair_exhausted, 88 occurrences across many different PRs/head SHAs) is a false-positive alert, not a genuinely broken repair mechanism. regatePullRequest charged a repair "attempt" the instant a job cleared GitHub rate-limit admission — before ever calling reReviewStoredPullRequest, and therefore before the review pipeline's own readiness gate (prReadyForReview) had any chance to decide whether there was a real opportunity to review at all.

prReadyForReview legitimately, by design (#3947), defers unconditionally and indefinitely whenever a branch-protection-required status check hasn't posted yet — there's no finalize escape for that specific case (unlike the sibling "genuinely stuck, non-required pending" case, which does eventually finalize). So a perfectly healthy PR sitting on a queued-but-not-yet-run required check got charged a full repair attempt on every ~2-minute sweep tick, burning the fixed 5-attempt cap in ~10 minutes — an order of magnitude shorter than realistic CI queue latency — and firing a "repair exhausted" alert for a review that was never actually broken.

Confirmed concretely for the cited example: main's branch protection requires the validate status check, and PR #5429's cited head SHA never got a validate check-run at all during its ~33-minute life (only Workers-Builds/CodeQL/Security-Scan/Analyze ran) before being superseded by a force-push — every one of its 5 repair "attempts" was doomed from the start by design, not by a real failure.

Changes

  • reReviewStoredPullRequest (src/queue/processors.ts) now returns whether it actually got past prReadyForReview — there is no early return anywhere in the function after that point, so every exit before it returns false, and the function's natural end (once readiness passed) returns true.
  • regatePullRequest only records the REGATE_REPAIR_ATTEMPT_EVENT_TYPE audit event when that's true — moved from unconditionally-before-the-call to conditionally-after-it. A swallowed (non-retryable) exception past the readiness check still counts as a real executed attempt, same as before this fix; only the clean "declined before ever reaching readiness" path is now excluded.
  • This mirrors the exact same "count executions, not deferrals" reasoning the #orb-retry-storm fix already applied one layer out (rate-limit admission).

Test plan

  • npm run typecheck — clean
  • npm run test:ci (full local gate, unsharded coverage) — green
  • Added a regression test that runs 6 simulated sweep ticks against a PR stuck on a missing required-CI context and confirms zero repair attempts / zero exhaustion events are ever recorded
  • Manually confirmed (before writing the fix) that this new test fails against the old code with exactly 6 phantom attempts recorded — the same false-exhaustion pattern reported live
  • The two existing #orb-retry-storm regression tests (which exercise a PR that DOES reach readiness) still pass unchanged

Fixes GITTENSORY-1E

…s confirmed

regatePullRequest recorded a REGATE_REPAIR_ATTEMPT_EVENT_TYPE audit event as
soon as the job cleared GitHub rate-limit admission, before ever calling
reReviewStoredPullRequest -- so a PR legitimately still waiting on a missing
branch-protection-required check (which prReadyForReview defers
unconditionally and indefinitely by design, #3947) got charged a full repair
attempt on every ~2-minute sweep tick even though nothing was actually wrong.
The fixed 5-attempt cap exhausted in ~10 minutes, an order of magnitude
shorter than realistic required-CI latency, firing a false
"repair exhausted" alert for a review that was never broken.

reReviewStoredPullRequest now returns whether it actually got past
prReadyForReview (there is no early return anywhere after that point, so
every exit before it returns false and the function's natural end returns
true); regatePullRequest only records the repair attempt when that's true.
A swallowed (non-retryable) exception past that point still counts as a real
executed attempt, same as before -- only the clean "declined before ever
reaching readiness" path is now excluded.

Fixes GITTENSORY-1E
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.73%. Comparing base (849e11e) to head (bfee161).
⚠️ Report is 15 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5482   +/-   ##
=======================================
  Coverage   94.73%   94.73%           
=======================================
  Files         561      561           
  Lines       44794    44800    +6     
  Branches    14667    14667           
=======================================
+ Hits        42437    42443    +6     
  Misses       1622     1622           
  Partials      735      735           
Flag Coverage Δ
shard-1 43.79% <93.33%> (-0.50%) ⬇️
shard-2 35.39% <0.00%> (+0.10%) ⬆️
shard-3 32.11% <0.00%> (+0.08%) ⬆️
shard-4 31.30% <53.33%> (-0.09%) ⬇️
shard-5 33.10% <0.00%> (-0.12%) ⬇️
shard-6 43.87% <46.66%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/queue/processors.ts 95.64% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 12, 2026
@loopover-orb

loopover-orb Bot commented Jul 12, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-12 23:40:27 UTC

2 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This moves recording of the REGATE_REPAIR_ATTEMPT_EVENT_TYPE audit event from immediately after rate-limit admission to after reReviewStoredPullRequest confirms it actually passed its own readiness gate (prReadyForReview), by threading a new boolean return value through every early-exit path in that function. The change is well-targeted at the described root cause: prReadyForReview defers unconditionally for missing required-status-checks, so charging an attempt before that gate ran was exhausting the 5-attempt budget on healthy, still-pending PRs. The added regression test drives the real code path (mocking fetchRequiredStatusContexts/fetchLiveCiAggregatePreferGraphQl to simulate a pending required check) across 6 sweep ticks and asserts zero attempt/exhausted audit rows, which is a meaningful assertion rather than a fabricated one.

Blockers

  • src/queue/processors.ts:1842 records `REGATE_REPAIR_ATTEMPT_EVENT_TYPE` only after `await reReviewStoredPullRequest(...)` returns, so any retryable exception after `prReadyForReview` has passed is a real executed repair that is never charged and can be reselected indefinitely instead of consuming the repair budget.
Nits — 5 non-blocking
  • src/queue/processors.ts: the diff shows four `return;` sites converted to `return false;`, but the function body has other unchanged sections not shown in the diff (e.g. around the sweep/CI-wait and linked-issue checks) — worth double-checking no bare `return;` remains uncovered that would fall through to the final `return true;`, since a missed one would silently mis-charge an attempt for a decline.
  • src/queue/processors.ts: the `.catch` handler that swallows a non-retryable failure and returns `true` treats any thrown error past the readiness point as a 'genuine executed attempt' — worth confirming this catch can't also fire for errors thrown before readiness was reached (which would incorrectly charge an attempt for a case that should count as `false`).
  • Consider a short inline comment at each new `return false;` site explaining briefly why (already partially covered by the JSDoc above the function, but line-level breadcrumbs make future edits safer).
  • The JSDoc addition on reReviewStoredPullRequest is thorough and useful — good pattern to keep for other multi-early-return functions in this file.
  • src/queue/processors.ts:1829 adds a very long incident narrative inline in `regatePullRequest`; keep the operational context in the regression test or issue and leave the code comment focused on the invariant being enforced.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

2. Maintainer requires a linked issue — Link the relevant issue (for example `Closes #123`) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 44 registered-repo PR(s), 36 merged, 424 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 44 PR(s), 424 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Improvement ✅ Minor risk: clean · value: minor — Code changes are accompanied by test evidence.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 44 PR(s), 424 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
[BETA] Chat with Gittensory

Ask Gittensory a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @gittensory ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @gittensory chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @gittensory mention with a real question is routed to the closest matching read-only command automatically -- no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/gittensory-commands

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 12, 2026
… then throws

Gittensory review flagged that regatePullRequest only recorded
REGATE_REPAIR_ATTEMPT_EVENT_TYPE after reReviewStoredPullRequest
returns, so a retryable error (GitHub rate limit / actuation-lock
contention) surfacing from real post-readiness work skipped the
charge entirely and could let a stuck repair reselect indefinitely.

reReviewStoredPullRequest now exposes an onReachedReadiness callback
that fires the instant its own readiness gate passes, before any
further throwable work -- a side channel that survives a later throw,
unlike the boolean return value. regatePullRequest uses it in a
try/catch/finally: the retryable branch still rethrows for the
queue's own retry, but the finally block charges the attempt first
whenever readiness was genuinely reached, regardless of whether the
call returned or threw. This also closes the symmetric gap the
review's own nit raised: an error thrown BEFORE readiness (the
callback never fires) correctly still does NOT charge the budget.
@JSONbored
JSONbored merged commit f6e5d07 into main Jul 12, 2026
13 checks passed
@JSONbored
JSONbored deleted the fix/regate-repair-false-exhaustion branch July 12, 2026 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant