fix(queue): guard the verdict derivation instead of the pass, so the webhook path is covered (#10227) - #10276
Merged
Merged
Conversation
Contributor
|
Important 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏳ LoopOver is waiting…LoopOver has seen this pull request and is waiting on CI checks to finish before reviewing it. This comment will update once the review runs. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟨 Waiting |
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
loopover-ui | 27fad9e | Jul 31 2026, 11:43 PM |
Bundle ReportBundle size has no change ✅ |
JSONbored
force-pushed
the
fix/verdict-backoff-chokepoint
branch
from
July 31, 2026 21:56
a623142 to
01c50aa
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10276 +/- ##
=======================================
Coverage 92.28% 92.29%
=======================================
Files 939 939
Lines 114775 114781 +6
Branches 27715 27715
=======================================
+ Hits 105920 105935 +15
+ Misses 7553 7546 -7
+ Partials 1302 1300 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JSONbored
force-pushed
the
fix/verdict-backoff-chokepoint
branch
from
July 31, 2026 22:21
01c50aa to
69f5769
Compare
Merged
17 tasks
JSONbored
force-pushed
the
fix/verdict-backoff-chokepoint
branch
2 times, most recently
from
July 31, 2026 22:55
36b9e1c to
0e71f7c
Compare
…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
force-pushed
the
fix/verdict-backoff-chokepoint
branch
from
July 31, 2026 23:43
0e71f7c to
27fad9e
Compare
JSONbored
pushed a commit
that referenced
this pull request
Aug 1, 2026
…t block #10276 added it to the local gate after this branch's last rebase; the branch's own checker (correctly) fails validate-code until it is wired.
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 split the verdict-stability backoff (#10184) across two kinds of place and only got one 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 turned the escape hatch into 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) toVERDICT_BACKOFF_MIN_REPEATS(3). That truncation was reproduced first, before anything was changed.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.Why the webhook path is the whole point (measured, not asserted)
Decision records on the Orb over 24h:
records_per_headupstream_state_change(raw GitHub delivery — the webhook path)pipeline_errorThe entire repeat signal is the one path that was never guarded.
github_app.review_skipped_stable_verdictfired 11 times in the same window. 548 of 723 records arehold, which is the only classshouldSkipStableVerdictacts on — so the guard is pointed at the right population.The structural note worth recording
The issue expected the guard to sit "after the visual-capture scheduling and before the AI review / gate evaluation." That ordering does not exist in the file: the recapture chain is scheduled from inside the comment-publish block, roughly 800 lines below the gate evaluation, not above it. No single line in the current source both runs the chain and skips the derivation.
So the chain is extracted (
runVisualCaptureObligation) and run explicitly on the backed-off path. The normal publish path calls it in the same place, in the same order, with the same inputs — behaviour there is unchanged, verified by the twoscreenshot-table gate (#10061)cases passing untouched both before and after the extraction. Its file 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.explicitlyRequestedshrank to one entrywebhook.forceAiReview, meaning exactly one thing: a human asked for this pass — an operator's manual re-gate (reReviewStoredPullRequest'soptions.force) or a consumed "Re-run LoopOver review" click (#7626).previewPollAttemptneeded an exemption only because the old guard sat above the recapture chain.Two #10229 behaviours deliberately reversed
Both are pinned by name in
verdict-stability-wire.test.tsrather than left to be rediscovered.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 on every sweep tick forever without ever exhausting.The drift check
scripts/check-verdict-backoff-chokepoint.ts(wired intotest:ci) enforces four rules — one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth 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.Verification of the guards themselves
Every guard added was mutation-tested, and each mutant is killed by a named test:
explicitlyRequested→false#10061cases (5 → 0 recapture jobs) and the new full-budget property testCloses #10227
Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #10227).Validation
git diff --checknpm run actionlintnpm run typecheck(run again after the test files were added)npm run test:coveragelocally, unshardednpx vitest run: 26,703 passed. The single failure wasminer-cli --versionagainst a staledist/;npm run build:minerthen 35/35 green.npm run build:mcpnpm audit --audit-level=moderate— findings are pre-existing onmain; this PR changes no dependency (package.jsondiff is two script lines,package-lock.jsonuntouched).checkers-wired,dead-exports,dead-source-files,test-wiring,coverage-boltons,import-specifiersNo UI, OpenAPI, wrangler binding,
src/selfhost/**env read, or DB change, so no regenerated artifact applies.Safety
UI Evidencesection.Notes
The acceptance criterion this PR cannot close by itself is the last one:
records_per_headfalling for theupstream_state_changepopulation. The pre-deploy baseline is recorded above (3.52 over 99 heads in 24h) so the post-deploy comparison is against a measured number rather than a remembered one.