fix(queue): stop the verdict backoff from eating a manual retrigger or an operator force - #10229
Merged
Merged
Conversation
…r an operator force #10204 placed the skip at the entry of the publish-and-maintain pass, past two things it must not run past. Readiness fires options.onReachedReadiness -- which charges regatePullRequest's bounded repair budget -- and then consumes the ONE-SHOT panel-retrigger marker (#7626). A guard sitting after both meant a backed-off pass had already spent a user's "Re-run LoopOver review" click with nothing left to re-trigger it, and had charged a repair attempt for work it never did. The guard now sits between the readiness gate and onReachedReadiness. It must not move EARLIER than readiness either: readiness legitimately defers a pass, and the screenshot-table recapture chain (#10061) depends on those deferrals to bound its retry budget -- a pre-readiness guard truncated it from 5 attempts to 3, caught by that test. options.force is now honoured. An operator's manual re-gate passes force: true and was being silently suppressed; a poll tick (previewPollAttempt) likewise. Backoff exists to stop the machine re-asking itself a settled question and must never suppress a pass a human asked for. The guard is extracted so both publish-and-maintain sites can share it, and its !headSha half is documented as a TSC-enforced early-out rather than a safety guard -- mutation testing confirms no runtime test can distinguish its absence, and an unverifiable guard is a claim, not a safeguard (verdict-stability.ts's own removed exponent clamp made the same point). #10204 shipped this wiring with NO test; test/unit/verdict-stability-wire.test.ts is the first, and pins each defect above as a regression. The webhook path is deliberately still unguarded. Adding it there truncates #10061's recapture budget, and the fix is not another exemption but moving the skip to the verdict-derivation choke point the record half already uses -- scoped in #10227. Closes #10222
Contributor
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-31 13:41:04 UTC
Review summary Nits — 6 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionPartially addressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
4 tasks
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10229 +/- ##
==========================================
- Coverage 92.22% 91.36% -0.87%
==========================================
Files 937 937
Lines 114413 114416 +3
Branches 27662 27664 +2
==========================================
- Hits 105520 104533 -987
- Misses 7588 8772 +1184
+ Partials 1305 1111 -194
Flags with carried forward coverage won't be shown. Click here to find out more.
|
20 tasks
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
JSONbored
added a commit
that referenced
this pull request
Jul 31, 2026
…webhook path is covered (#10227) (#10276) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#10204 placed the verdict-stability skip (#10184) at the entry of the publish-and-maintain pass, past two things it must not run past.
Readiness fires
options.onReachedReadiness— which chargesregatePullRequest's bounded repair budget — and then consumes the one-shot panel-retrigger marker (#7626). A guard sitting after both meant a backed-off pass had already spent a user's "Re-run LoopOver review" click, with nothing left to re-trigger it, and had charged a repair attempt for work it never did.Three defects, all live on
main:onReachedReadinesscharges the repair budget for a pass that never reviewedoptions.force— an operator's manual re-gate — is silently suppressedThe guard now sits between the readiness gate and
onReachedReadiness, and honoursforce/previewPollAttempt.It must not move earlier than readiness either
Readiness legitimately defers a pass, and the screenshot-table recapture chain (#10061) depends on those deferrals to bound its retry budget. Moving the guard before readiness truncated it from 5 attempts to 3 — caught by that test, not by me. Between the two is the only correct place. That constraint is now recorded in the code.
The webhook path is deliberately still unguarded
Adding the guard to
handlePullRequestWebhookEventtruncates #10061's recapture budget the same way, and the only way to make it fit here would be another entry in anexplicitlyRequestedallowlist — or weakening that test, which I won't do.That allowlist is the design flaw: the default for anything not listed is "suppress me", so every future bounded retry chain is silently truncated the day it ships. The fix is to move the skip to the verdict-derivation choke point that the record half of #10204 already occupies ("the single ledger write every verdict passes through, so no caller can bypass it"). Scoped in #10227.
So this PR fixes the three live defects and explicitly leaves the dominant churn path — 293 of 344 repeat evaluations in a 24h Orb window are
upstream_state_change, i.e. raw webhook deliveries — for that redesign rather than forcing it through here.Testing
#10204 shipped this wiring with no test at all —
grep -rn "review_skipped_stable_verdict" test/returned nothing.test/unit/verdict-stability-wire.test.tsis the first, and pins each defect above as a named regression.Mutation testing: 5 mutants applied to the guard, 4 killed. The survivor (
!headSharemoved) is an equivalent mutant at runtime — with no head SHA the lookup misses andshouldSkipStableVerdictreturns false anyway. It is enforced by TSC instead (verdictStabilityKeytakes astring), and is now documented as an early-out rather than a safety guard, matchingverdict-stability.ts's own reasoning about its removed exponent clamp.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragenpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateFull
npx vitest run: 1359 files, 26517 tests, 0 failures. Green:dead-source-files,dead-exports,import-specifiers,engine-parity:drift,coverage-boltons,typecheck-coverage,cf-typegen.If any required check was skipped, explain why:
actionlint,test:workers,mcp,ui:*— this PR touches one file undersrc/queue/plus one new test. CI runs them all.npm auditreports pre-existing advisories onmain; no dependency changes here.Safety
UI Evidencesection below.This makes the engine do more work, never less: every change here removes a case where a pass was wrongly suppressed.
UI Evidence
Not applicable — no UI, frontend, docs, or extension files are touched.
Closes #10222