fix(gate): stop two subsystem failures from becoming wrong auto-verdicts (#9460, #9462) - #9501
Conversation
…cts (#9460, #9462) An unpublishable AI blocker and a stuck visual-capture marker each let a subsystem failure resolve as a verdict rather than as "cannot determine". pre-merge-checks already models the correct shape (an unresolvable enforced check -> NEUTRAL -> hold, never a silent auto-merge bypass); neither of these followed it. combineReviews (#9460): synthesizeDefect returns null both when nobody flagged anything and when a real blocker's title is unpublishable, and single/synthesis collapsed both to a clean pass -- so a blocker phrased with ordinary review vocabulary (reward/ranking/cohort, or a bare score term) auto-MERGED the change it blocked. single is this deployment's live strategy. Both strategies now hold; consensus already failed closed and is untouched. Note synthesizeDefect calls toPublicSafe with no options, so the gate-bearing title is filtered even on a repo in LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS -- the allowlist never rescued this path. Visual capture (#9462): scheduleVisualCaptureRetry's budget-exhausted early return only skipped re-writing visualCaptureRetryPendingSha, leaving the previous attempt's marker standing forever (the sole other clear needs a successful capture, which by definition never comes). A permanently-marked head silently disabled the screenshot gate's close -- a permanent bypass out of what was designed as a temporary deferral. The marker is now cleared on exhaustion, and the unresolved state is threaded into the planner so the deferral holds the PR rather than falling through to a merge.
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-28 00:36:48 UTC
Review summary Nits — 7 non-blocking
CI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed 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.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9501 +/- ##
==========================================
+ Coverage 89.55% 93.62% +4.06%
==========================================
Files 843 741 -102
Lines 110073 60428 -49645
Branches 26194 21300 -4894
==========================================
- Hits 98573 56574 -41999
+ Misses 10238 2901 -7337
+ Partials 1262 953 -309
Flags with carried forward coverage won't be shown. Click here to find out more.
|
loopover-orb
Bot
added
gittensor:bug
…ETRIC_META (#9460) The drift guard in test/unit/selfhost-metrics.test.ts requires every literal metric name emitted from src/ to carry a registered meta entry; the counter added alongside the combineReviews hold had none, failing the backend coverage suite.
…#9462) The existing #9030 exhaustion test starts from a PR that never had a marker written, so it exercises the early return but not the defect. This drives the real failing sequence -- attempt 0 errors and writes the marker, then the final attempt exhausts the budget with the pipeline still failing -- and asserts the marker does not outlive the retries that justified it. Verified to fail against the unfixed code (expected 'vis62' to be null), so it pins the fix rather than merely passing alongside it. Closes the codecov/patch gap flagged on the PR.
|
Coverage gap closed. The marker-clearing path now has a direct regression test (`screenshot-table gate (#9462): a marker written by an earlier attempt is CLEARED once the budget is exhausted`). It drives the real failing sequence the existing #9030 test could not reach — attempt 0 errors and writes the marker, then the final attempt exhausts the budget with the pipeline still failing — rather than starting from a PR that never had a marker written. Verified it actually pins the fix: with the clear neutralised it fails with |
…9504/#9505 merges (#9509) * test(gate): cover the fail-safe marker-clear path (#9462) Mirrors the sibling visual-capture-satisfied fail-safe test: a rejecting clear must not throw out of the pass, since a failed clear only means the gate stays deferred until the head moves. (cherry picked from commit efa6472) * refactor(gate): drop an unreachable spread arm and annotate the defensive head-SHA guard (#9462) screenshotTableEvidenceUnresolved is computed as a plain boolean, so the undefined arm of its conditional spread was dead code -- pass it directly. The head-SHA guard in the exhausted branch is genuinely defensive (a recapture-preview job is only minted for a PR that had one), matching the sibling mark write, so it carries a reason rather than a contrived test. (cherry picked from commit 4f59499) * test(ai): exclude two unreachable best-effort cleanup arms from coverage (#9479) rm with force:true does not throw for a missing path, so the catch needs a filesystem-level failure no unit test can portably induce; the unset-cwd guard is only reachable when mkdtemp itself threw, i.e. no directory was ever created. Both annotated with their reason, matching the file's existing convention. (cherry picked from commit bf3efe6) * test(ai): drive the real subprocess EPIPE path and the non-Error stall guard (#9476, #9479) The EPIPE regression uses the existing real-subprocess harness rather than a stubbed spawn, so the stdin error listener is genuinely exercised: a fake CLI exits immediately without draining stdin, and the test asserts no uncaughtException reaches the process. Verified to fail without the listener (it captures an uncaught EPIPE), which is precisely the crash that took down every in-flight queue job in the container. Also covers isStalledNoOutput rejecting a non-Error throw -- a provider adapter can reject with a string, and misclassifying that as a deadline signal would silently skip the retry budget. (cherry picked from commit c770cc1) * test(rate-limit): cover the inline-retry break for an over-budget Retry-After (#9494) Asserts the attempt COUNT, not just the classification: a 60s Retry-After yields exactly one attempt instead of four, and a short one still uses the full inline budget so the break stays narrow. (cherry picked from commit 92813bf)
1 participant
Summary
Two verified paths by which a subsystem failure became a verdict instead of a "cannot determine". Both are from the 2026-07-27 adversarial audit, and both are live on this deployment's current configuration.
The codebase already models the correct shape in
pre-merge-checks.ts:9-13,38-49— an unresolvable enforced check yieldsPRE_MERGE_CHECK_UNRESOLVED_CODE→ NEUTRAL → held, documented explicitly as "never silently skipping a hard requirement (auto-merge bypass)". Neither of these two followed it.Closes #9460
Closes #9462
#9460 — an AI blocker silently collapsed to a clean pass (false merge)
synthesizeDefectreturnsnullfor two situations that must not share an outcome: nobody named a real blocker (a genuine clean pass), and a real blocker whose titletoPublicSaferefuses to publish. Thesingleandsynthesisstrategies collapsed both to{defect: null, split: false, inconclusive: false}— indistinguishable from "the reviewer found nothing" — so the PR auto-merged with the defect unreported, and the row was cached durably.singleis this deployment's live strategy (AI_PROVIDER=claude-code,ollama→resolveAiReviewerPlan,src/selfhost/ai.ts:1596-1613).consensusalready failed closed via its ownsplitarm and is untouched.Both strategies now resolve to
inconclusive— deliberately, rather thansplit: the situation genuinely is "no usable public verdict for this head", which isai_review_inconclusive's own semantics and copy, and routes to a neutral gate → human hold.ai_review_split's copy instead asserts a disagreement between two reviewers that never happened, which would be actively misleading in single-reviewer mode.A blank-only blockers array is still not a flag (
realBlockersOffilters it) — that stays a clean pass, unchanged.One correction to the issue as filed: its example (
computeScore divides by zero) would not actually be filtered —BARE_SCORE_TERM_PATTERNis/\bscore\w*\b/iandcomputeScorehas no word boundary beforeScore. What genuinely trips it isreward/rewards/payout/farming/ranking/cohort/reviewability(plain.includes()) and a barescore/scores/scored/scorer. All are now pinned as regression tests, and the camelCase non-match is pinned too so any future widening of the pattern is a deliberate, visible decision. Also worth noting:synthesizeDefectcallstoPublicSafewith no options, so the gate-bearing title is filtered even on a repo inLOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS— the allowlist never rescued this path.#9462 — a stuck capture marker silently disabled the screenshot gate (false merge)
Two compounding defects:
The marker outlived its retries.
scheduleVisualCaptureRetry's budget-exhausted early return only skipped writingvisualCaptureRetryPendingShaagain — the previous attempt's marker stayed. The only other clear (markPullRequestVisualCaptureSatisfied) requires a successful capture, which is by definition the thing not happening. Verified by exhaustive grep: exactly one clear and one setter acrosssrc/. So a head whose capture never succeeded stayed marked forever, and the doc comment claiming the gate "falls through to its normal (accurate) evaluation" was false in code.The deferral skipped instead of holding. With the marker set,
screenshotTableMatchwas leftundefinedand the plan fell through to ordinary merit/CI evaluation — so a green PR planned a merge, with no finding, hold, or blocker representing "screenshot evidence unresolved". That is a disposition inversion: a PR the gate would have closed merged instead.The gate is configured
action: closefor all three repos in the private server-side config, and was re-enabled two commits before the audit (73d4e94, #9455).Scope note — #9464 deliberately excluded
The audit also found a false-close sibling (browserless render failures being swallowed). I implemented it, discovered it was wrong, and backed it out rather than ship it.
Inferring the failure downstream from "routes resolved but zero rendered pairs" cannot distinguish a browserless blip from a capture that legitimately found nothing renderable — and it broke the existing
#4110test covering exactly that legitimate case, where the gate should close. #9464's real deliverable is to surface an explicitrenderFailedsignal fromcapture.tswhere the failure is actually known. That needs its own change and its own tests, so it stays open for a follow-up.Validation
npx tsc --noEmit -p tsconfig.json— cleantest/unit/ai-review.test.ts,test/unit/agent-actions.test.ts,test/unit/queue-3.test.ts— 740 passedNew regression tests:
combineReviews: unpublishable blocker holds insingle,synthesis/either, andsynthesis/both; four parameterised real-vocabulary titles; publishable blocker still yields a defect; camelCase identifier still publishable; blank-only blockers still pass.falseor absent flag leaves the close path untouched.One test changed rather than added:
"synthesized defect drops a blocker whose only finding is blank or unsafe (fail-safe...)"assertedinconclusive: falsefor the unsafe case — i.e. it pinned the bug as correct behaviour, using the fixture literal"Boost your reward payout". It has been split so the blank case keeps its old assertion and the unsafe case asserts the hold.Coverage gap I want to flag honestly
The marker-clearing path (
clearPullRequestVisualCaptureRetryPendingand its call site) has no direct test. The existing#9030exhaustion test starts from a PR that never had a marker written, so it exercises the early return but not the clear. The test that would cover it is the marker-SET-then-budget-EXHAUSTED sequence; I drafted it, found it needed a fetch-stub helper that does not exist (the neighbouring tests inline their stubs), and removed it rather than commit a broken file. Happy to add it in this PR before merge — flagging rather than lettingcodecov/patchsurface it.