fix(orb): enforce the model/fact boundary — CI claims demoted at parse, silence is not certainty - #8845
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8845 +/- ##
==========================================
- Coverage 93.84% 92.30% -1.54%
==========================================
Files 803 803
Lines 80065 80080 +15
Branches 24267 24274 +7
==========================================
- Hits 75138 73920 -1218
- Misses 3562 5096 +1534
+ Partials 1365 1064 -301
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-26 12:41:55 UTC
Review summary Nits — 5 non-blocking
Why this is blocked
📋 Copy for AI agents — paste into your coding agentCI checks failing
Decision drivers
Context & advisory signals — never blocks the verdict
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. 🟩 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.
|
…e, silence is not certainty A full inventory of the AI-review prompt (posted on #8833) found the two highest-risk gaps between what the prompt REQUESTS and what the parser ENFORCES: 1. CI adjudication. The system prompt forbids the model from judging CI state twice in prose, then hands it the CI table for context -- with no parser enforcement. A blocker string naming CI still became the gate's public 'why blocked' text and drove willClose, even though buildCheckAggregate already owns that fact deterministically (the exact contradiction class behind the 59 corrected verdicts on #8825). demoteCiClaimBlockers now demotes run-state claims to annotated nits at BOTH parse sites (Workers + BYOK provider), with a bounded-vocabulary pattern that never touches code-content judgment ('this breaks the build contract' is a prediction about the diff and stays a blocker; 'the build is failing' is a report about CI and does not). Demotions log ai_review_ci_claim_demoted -- a rising rate is a prompt-regression signal. 2. Unstated confidence read as maximum certainty. parseReviewConfidence defaulted absent/garbage to 1.0, so a review that STATED no confidence skipped every low-confidence safeguard and drove a straight close. It now defaults to CONFIDENCE_WHEN_UNSTATED (0.5, under every sane close floor), routing to the low-confidence disposition -- still blocks, a human decides the close. Advances #8833. Remaining extraction items from the inventory (structured verifiable blocker schema, moving test-evidence/size/ staleness out of the prompt, non-negotiable enrichment facts, explicit unknown in the finding schema) are recorded on the issue.
7a836c3 to
ead3eed
Compare
…r contradicts (#8833) Whether a PR carries test-path evidence is a deterministic fact — signals/test-evidence.ts's isTestPath owns it, and slop.ts's missing_test_evidence finding already decides its severity. The reviewer prompt nonetheless fed the same fact as free text and let the model re-decide blocker-vs-nit, so a model could close a PR on a claim the engine had already answered the other way. Extends the existing parse-time fact-authority pattern (demoteCiClaimBlockers #8845, demoteEvidenceAbsenceBlockers #8961) to this criterion: a whole-PR "no tests were added" blocker is demoted to a nit when the classifier confirms the PR does change test paths. Fires only in the arm where the claim is provably a fact error — a genuine test-absence blocker on a genuinely test-free PR is untouched — and demotes rather than drops, so the observation still reaches the human. Coverage-DEPTH claims that narrow to a specific target ("no tests for the nullish branch") are deliberately excluded: the classifier cannot check those, so they remain the model's judgment to make and keep blocking.
…eement (#8834) Verbalized confidence alone is poorly calibrated — #8845 already had to stop reading an absent confidence as certainty. Sampling-based consistency is the better-behaved signal, and the risk-control literature this epic builds on finds two samples capture most of the benefit. Scores inter-run agreement across the reviewer stances the engine ALREADY produces (#8229's reviewerVotes) and folds it into the verbalized confidence, at zero additional AI spend. The combined score multiplies the two so it is monotonically below either input — a judgment is only as trustworthy as both how sure the judge said it was and how reproducibly the judges reached it, which is the property an abstention threshold depends on. A lone run is recorded as UNCORROBORATED at a 0.5 agreement floor rather than fabricated unanimity, so a single-reviewer or budget-degraded review records a strictly lower confidence than a genuinely corroborated one. Zero samples refuses to invent a score at all. The signal rides to DecisionRecord.aiAgreement (schema v5) so every decision joins the risk-control calibration set with its reproducibility attached. Deliberately ADDITIVE: it does not re-route the gate. Disagreement already routes to a hold today — differing stances ARE the ai_review_split finding, which blocks or holds via the existing confidence floor — so a second parallel route would double-count the same evidence instead of measuring it.
…sion confidence, and replayable time (#8833, #8834, #9028) (#9256) * fix(review): demote whole-PR test-absence blockers the path classifier contradicts (#8833) Whether a PR carries test-path evidence is a deterministic fact — signals/test-evidence.ts's isTestPath owns it, and slop.ts's missing_test_evidence finding already decides its severity. The reviewer prompt nonetheless fed the same fact as free text and let the model re-decide blocker-vs-nit, so a model could close a PR on a claim the engine had already answered the other way. Extends the existing parse-time fact-authority pattern (demoteCiClaimBlockers #8845, demoteEvidenceAbsenceBlockers #8961) to this criterion: a whole-PR "no tests were added" blocker is demoted to a nit when the classifier confirms the PR does change test paths. Fires only in the arm where the claim is provably a fact error — a genuine test-absence blocker on a genuinely test-free PR is untouched — and demotes rather than drops, so the observation still reaches the human. Coverage-DEPTH claims that narrow to a specific target ("no tests for the nullish branch") are deliberately excluded: the classifier cannot check those, so they remain the model's judgment to make and keep blocking. * feat(orb): capture the decision-time wall clock so staleness rules are replayable (#9028) gate.requireFreshRebaseWindow compared the base branch's tip against an inline Date.now() read inside maybeForceFreshRebase. Time is a decision INPUT, and nothing recorded which instant the comparison used — so re-deriving such a decision later could silently reach the opposite answer purely because the wall clock had moved, and report it as a match. The decision pass now takes ONE Date.now() reading, records it into decision_replay_inputs.replay_json as `clock`, and passes it to every clock-dependent rule instead of each calling the clock itself. Both staleness rules move to a pure, clock-injected module: isWithinFreshRebaseWindow takes the instant explicitly, and isBaseStaleByAheadBy is stated as the commit-count comparison it is — provably instant-independent, not merely assumed so. replayDecision gains a stage-0 `clock` check: replaying at the recorded instant (the CLI default) is bit-exact, while naming a different instant reports a `clock` divergence rather than silently certifying a re-derivation that never reproduced the original evaluation. Records written before this change carry no instant, so the stage is skipped rather than guessed. The CLI exposes it as `--at <epoch ms>`. Also corrects the decision_replay_inputs migration reference in two doc comments (0181 is alert_dedup_claims; the table is created in 0182). * feat(orb): record a per-decision confidence signal from inter-run agreement (#8834) Verbalized confidence alone is poorly calibrated — #8845 already had to stop reading an absent confidence as certainty. Sampling-based consistency is the better-behaved signal, and the risk-control literature this epic builds on finds two samples capture most of the benefit. Scores inter-run agreement across the reviewer stances the engine ALREADY produces (#8229's reviewerVotes) and folds it into the verbalized confidence, at zero additional AI spend. The combined score multiplies the two so it is monotonically below either input — a judgment is only as trustworthy as both how sure the judge said it was and how reproducibly the judges reached it, which is the property an abstention threshold depends on. A lone run is recorded as UNCORROBORATED at a 0.5 agreement floor rather than fabricated unanimity, so a single-reviewer or budget-degraded review records a strictly lower confidence than a genuinely corroborated one. Zero samples refuses to invent a score at all. The signal rides to DecisionRecord.aiAgreement (schema v5) so every decision joins the risk-control calibration set with its reproducibility attached. Deliberately ADDITIVE: it does not re-route the gate. Disagreement already routes to a hold today — differing stances ARE the ai_review_split finding, which blocks or holds via the existing confidence floor — so a second parallel route would double-count the same evidence instead of measuring it. * test(orb): update decision-record schema assertions for the v5 agreement field (#8834) The DECISION_RECORD_SCHEMA_VERSION bump to v5 moves the pinned version assertions in the backfill bundle and queue decision-record suites. The #9124 record test additionally carries the agreement on its cached finding and asserts it threads through to the record, mirroring exactly how that test already proves modelIds/promptDigest pass through rather than being re-derived. --------- Co-authored-by: JSONbored <airdroptopian@gmail.com>
…d attribute votes to the producing model (#9477, #9478) (#9513) * fix(ai): make prompt drift a cache miss, bound the Ollama context, and attribute votes to the producing model (#9477, #9478) getCachedAiReview reuses a cacheable row with NO maxAgeMs and replays its findings verbatim, including a severity:critical ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and last moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 all changed prompt text or verdict logic without one -- so a PR closed or held under the pre-#9074 false-consensus rule re-gated at the same head to the SAME stale finding and the corrected logic never ran. The fingerprint now folds in REVIEW_PROMPT_VERSION and a digest of the canonical judge prompt, both pure and available before the call, so drift is an automatic miss rather than something a human must remember. Bumped to v7 to kill rows written under the incomplete fingerprint. Ollama silently left-truncates past num_ctx (typically 4k-8k by default) and then answers confidently over whatever survived -- the TAIL of the prompt, i.e. the context sections rather than the diff. The review path sends up to 120k chars of diff plus a 240k-char context budget, so on the fallback provider a review could come from a fraction of the change while carrying the same blocker authority and confidence semantics as the primary. An explicit num_ctx is now sent for the ollama provider only, since other OpenAI-compatible servers may reject an unknown options key, and an explicit caller providerOptions still wins. runWorkersOpinion iterates [primary, fallback] internally and its outcome carried no model identity, so a fallback-produced review was recorded as a PRIMARY vote -- poisoning the reviewer_vote audit events, #8229's routing track records, and scoreJudgmentAgreement's contribution to decision-record confidence. The doc's claim that slot-to-model is unambiguous by construction holds for the tie-break swap, not for in-slot fallback. * chore(selfhost): regenerate the env reference for the new env reads
… the explicit unknown abstention (#8833) (#9592) Two more objectively-checkable facts move behind the deterministic boundary, enforced the same way the CI-state ban (#8845) and the test-evidence ban already are -- demoted at parse time on BOTH the Workers and BYOK paths, so no route escapes: - SIZE_CLAIM_PATTERN / demoteSizeClaimBlockers: whole-PR size claims ('too large', 'should be split') are owned by sizeGateMode -> oversized_pr. Anchored on a PR/changeset noun so code-magnitude judgments ('this buffer is too large') never match. - STALE_BASE_CLAIM_PATTERN / demoteStaleBaseClaimBlockers: staleness / rebase / merge-conflict claims are owned by stale_base_ref and GitHub's own mergeable_state -- none of them is even visible in a diff. Claim-shaped so code ABOUT rebasing/conflicts never matches. The rubric now also OFFERS an explicit abstention: a model that genuinely cannot judge its blockers sets confidence to the string "unknown", which parseReviewConfidence maps to CONFIDENCE_WHEN_UNSTATED (0.5) -- under every sane close floor, so an honest abstain routes to the low-confidence hold disposition instead of a guessed verdict. REVIEW_PROMPT_VERSION bumps to v2 (both edits shape the judge's verdict surface), which the #9477 cache fingerprint and the #8222 counterfactual replay both key on. Part of #8833; the structured verifiable-blocker schema lands separately.
What
The first (highest-risk-reduction) tranche of #8833: two places where the prompt requests a boundary between model judgment and deterministic fact, now enforced by the parser. Advances #8833 — the full input inventory and the remaining extraction items are posted on the issue.
Why — from the inventory
A complete audit of everything flowing into the dual-model review prompt classified every input as objectively-checkable vs genuinely-subjective. Two enforcement gaps stood out:
willClose, even thoughbuildCheckAggregatealready owns that fact deterministically. This is the same contradiction class behind the 59 corrected verdicts on gate_decision can contradict the action taken — stale 'merge' verdicts recorded after CI-failure closes distort calibration #8825 ("CI is failing" close +mergeverdict).parseReviewConfidencedefaulted absent/garbage to 1.0 — a review that stated no confidence skipped the close-confidence floor and Resolve dead aiReviewCloseConfidence floor (decision + fix) #4603's low-confidence disposition entirely, driving a straight close on an unquantified claim.How
demoteCiClaimBlockers(pure, exported): run-state CI claims ("CI is failing","tests are red","build failed", pending/in-progress variants) are demoted from blockers to annotated nits at both parse sites — Workers and BYOK provider — so no route escapes. The pattern is deliberately narrow: it requires the run-state verb shape, so "this change breaks the build contract" (a prediction about the diff — the model's actual job) is never touched. Each demotion logsai_review_ci_claim_demoted; a rising rate is a prompt-regression signal.CONFIDENCE_WHEN_UNSTATED = 0.5: absent/garbage confidence now parses below every sane close floor (default 0.93), routing to the low-confidence disposition — defaulthold_for_review, which still blocks but puts a human on the close. A stated confidence is untouched, including explicit0.Behavior change, stated plainly
A consensus defect whose reviews omit
confidencepreviously closed outright; it now blocks-and-holds for a human. That is the intended direction: the model never claimed the certainty the old default invented.Verification