fix(orb): webhook redelivery, reputation cadence, DB retention, and gate severity fidelity#9237
Conversation
…pruned caches (#9083) No RETENTION_POLICY table had a leading index on its retention timestamp, so pruneExpiredRecords's ctid-keyed batched delete forced a full sequential scan of the whole table on Postgres per batch -- large tables blew the prune job's 30-minute timeout and retention fell permanently behind (signal_snapshots alone reached 787MB at ~44KB/row). Rewrite the delete to range over a real, indexable primary key ordered by the retention column instead of rowid/ctid, and add the matching leading index for every policy table via migration 0191. Also close the "no delete path at all" gap for four read-side caches (grounding_file_content_cache, ai_review_cache, ai_slop_cache, linked_issue_satisfaction_cache) that only enforced their TTL at the read site, and add retention for three previously-untracked append-only logs (review_audit, decision_records, orb_webhook_events). Deliberately deferred: pull_request_files, check_summaries, gate_outcomes, agent_runs, and advisories are current-state tables (upserted per natural key, read as live state for open PRs/checks) rather than append-only logs, so they are excluded from this pass per RETENTION_POLICY's own documented scope -- pruning them needs a read-pattern review this PR doesn't do.
…manent duplicates (#9054) The dedup guard in enqueueWebhookByEnv only ever exempted 'error' rows from suppression. A row left at 'queued' (the insert happened but WEBHOOKS.send() was lost) or 'superseded' (overwritten by a later coalesced delivery) could never be redelivered: every GitHub retry and every operator "Redeliver" click carries the same delivery_id + payload hash, hits the guard, and is silently discarded as a no-op duplicate forever -- including check_suite.completed, the maybeReReviewOnCiCompletion auto-merge trigger. getWebhookEvent now also returns receivedAt, and a 'queued'/'superseded' row past a 10-minute staleness window is treated the same as an 'error' row: never suppressed. A migration purges the historical backlog of rows this bug already stuck permanently (dead by definition once a full day old), since replaying them is not useful and the code fix means no future delivery can get stuck this way again. Deferred: an active cron sweep that proactively re-triggers GitHub's own redelivery API for still-stuck rows was not implemented -- webhook_events does not persist the raw payload (by design, to avoid growing the exact blob-retention problem #9083 addresses), and this fix already ensures any future stuck delivery is redeliverable well inside GitHub's redelivery window without needing an active poke.
…ead of hard-denying (#9062) selfReputationThrottle's own doc comment promises a soft cadence throttle that "a recovering ratio restores it -- never a hard permanent ban", but the chokepoint ignored cadenceFactor entirely and hard-denied on ANY throttled verdict. Since submissions are the only source of new decided outcomes, and governor_reputation_history never decayed, a miner that hit the throttle band could never submit again to dilute its ratio back down -- an absorbing, permanent self-ban. evaluateGovernorChokepoint now consumes a non-floored cadenceFactor by scaling the per-repo write-rate-limit window instead of denying; the hard deny is reserved for the extreme "floored" ratio only. The miner-lib wrapper advances its rate-limit bucket against the SAME scaled policy the decision was evaluated with, so state stays consistent with the verdict it recorded. governor_reputation_history also gains a 14-day half-life decay (applied at both read and increment time), so even a floored ratio ages back below the sample-size floor over calendar time with zero new submissions, delivering the "recovering ratio" contract the module already documented.
… tighten confidence default, resolve near-miss code names (#9085) Finding severity was purely decorative: a warning-labeled finding (missing_linked_issue, slop_risk_above_threshold) can be the exact reason the gate one-shot-closes a PR, while a critical-labeled one (ai_consensus_defect, under the advisory-only default aiReviewGateMode) can have no gate effect at all. A contributor reading a plain warning icon on the finding that is about to close their PR was being actively misinformed. formatCheckRunOutput/buildCheckRunAnnotations now accept the real blocker codes from the SAME advisory's GateCheckEvaluation (when the caller has one) and use them to correct the displayed severity: an actual blocker always renders at the most alarming level regardless of its authored severity, and a non-blocking finding is capped at warning even if authored critical, so it never cries wolf louder than a genuine one. The parameter is optional and purely additive -- every existing caller that omits it keeps today's exact rendering. Wired the one caller with the evaluation already in scope (processors.ts's check-run publish). Also: - An absent AdvisoryFinding.confidence degraded to 1.0 (maximum certainty) at three gate-side consumption sites, the same "silence is not certainty" anti-pattern CONFIDENCE_WHEN_UNSTATED already fixed at the model-parsing layer. These sites read confidence off a finding object via a path the parser doesn't cover (a producer that omitted it, or the unvalidated cached-advisory JSON parse), so they now share the same 0.5 fallback instead of a second, wrong hardcoded default. - Renamed the near-miss repo_not_registered -> repo_not_cached (mirrors pr_not_cached/issue_not_cached's naming for the identical "not yet synced" shape) to stop it being confused with repo_unregistered, one character away and with the opposite gate consequence (repo_not_cached holds the gate for a human; repo_unregistered is a non-blocking advisory warning). The engine's gate-advisory.ts twin gets the matching confidence-default and rename fixes to stay in lock-step with the host copy.
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | eae7358 | Commit Preview URL Branch Preview URL |
Jul 27 2026, 08:31 AM |
|
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-27 09:07:24 UTC
Review summary Blockers
Nits — 6 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
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.
|
|
This finding is a false positive: `webhook_events_status_idx` isn't new to this PR — it was created back in `migrations/0001_initial.sql:143` (`CREATE INDEX IF NOT EXISTS webhook_events_status_idx ON webhook_events (status);`). This PR only adds the `received` index (`idx_webhook_events_received_at`, migration 0191), which the comment right above it in `schema.ts` says explicitly ("only the status index above existed before"). Drizzle requires declaring all of a table's indexes in one block, so the reviewer saw both listed together in the same schema.ts hunk and understandably assumed both were new. `npm run db:schema-drift:check` confirms schema.ts and migrations/ already agree with no drift. |
Summary
Four independent operational-hygiene fixes, bundled per repo convention:
queued/supersededwebhook deliveries (5,547 rows, 4,900 of themcheck_suite.completed— the auto-merge trigger) were permanently un-redeliverable: the dedup guard only ever exemptederrorrows. A row now escapes suppression once it's sat unprocessed past a 10-minute staleness window, and a migration purges the dead historical backlog.cadenceFactorwas computed and discarded, and anythrottledverdict hard-deniedopen_proutright. Since submissions are the only source of new decided outcomes, a miner that hit the throttle band could never dilute its ratio back down.ctid/rowidsemi-join) and four caches with a read-side TTL but no delete path at all.warning-labeled finding can be the exact reason the gate one-shot-closes a PR, while acritical-labeled one can have zero gate effect (advisory-only by default). An absent AI-judgment confidence also degraded to 1.0 (maximum certainty) at several gate-side sites, andrepo_not_registered/repo_unregisteredwere a one-character, opposite-consequence near-miss pair.Fixes and judgment calls
#9054 (
src/github/webhook.ts,src/db/repositories.ts,migrations/0192_*.sql)queued/supersededrow pastSTALE_QUEUED_WEBHOOK_MS(10 min) is treated like anerrorrow: never suppressed, so a GitHub retry or a manual "Redeliver" click re-enqueues it.webhook_eventsdoesn't persist the raw payload by design (to avoid growing the exact blob-retention problem orb(db): 3.4 GB in one month — no retention target has a usable index, and four caches have no delete path at all #9083 addresses), and the staleness-window fix already makes any future stuck delivery redeliverable well inside GitHub's redelivery window without an active poke.#9083 (
src/db/retention.ts,src/db/schema.ts,migrations/0191_*.sql)rowid/ctid, and added the matching leading index for every policy table.grounding_file_content_cacheat 2 days — its own read-side TTL is 24h — the other three at 30 days) and three previously-untracked append-only logs (review_audit,decision_recordsat 180 days for dispute evidence,orb_webhook_events).pull_request_files,check_summaries,gate_outcomes,agent_runs,advisoriesare current-state tables (upserted per natural key, read as live state), not append-only logs — pruning them needs a read-pattern review this PR doesn't do.#9062 (
packages/loopover-engine/src/governor/chokepoint.ts,packages/loopover-miner/lib/governor-{chokepoint,state}.ts)cadenceFactornow scales the per-repo write-rate-limit window instead of denying; the hard deny is reserved for the extremereason === "floored"case only. The miner-lib wrapper advances its bucket against the same scaled policy the decision was evaluated with, so recorded state never disagrees with the verdict it produced.governor_reputation_historygains a 14-day half-life decay (applied at both read and increment time), so even a floored ratio ages back below the sample-size floor purely from elapsed calendar time, with zero new submissions required.#9085 (
src/rules/advisory.ts,packages/loopover-engine/src/advisory/gate-advisory.ts,src/github/app.ts,src/queue/processors.ts,src/review/{parity-wire,unified-comment-bridge}.ts)formatCheckRunOutput/buildCheckRunAnnotationsnow accept the real blocker codes from the same advisory'sGateCheckEvaluation(optional, purely additive — every existing caller that omits it keeps byte-identical rendering). When provided, an actual blocker always renders at the most alarming level regardless of its authored severity, and a non-blocking finding is capped atwarningeven if authoredcritical. Wired the one caller that already has the evaluation in scope (the check-run publish inprocessors.ts).evaluateGateCheckCore) — that logic is already correctly driven byisConfiguredGateBlocker/policy modes, completely independent of theseverityfield, and rewiring severity itself to drive blocking would be a much larger, riskier behavioral change to a production auto-merge/close path than this PR should attempt. The issue's own complaint — "a contributor reading a warning icon on the finding that is about to close their PR is being actively misinformed" — is specifically about the displayed icon not matching the real consequence, which this fixes without touching the decision path at all.slop_risk_above_threshold/surface_lane_reject(item 2 of the issue) was already fixed onmainby fix(ai-review): verify reviewers actually agree; strip blocker authority from a bailed review #9114 prior to this branch.AdvisoryFinding.confidencenow degrades toCONFIDENCE_WHEN_UNSTATED(0.5) instead of 1.0 at the three gate-side consumption sites inadvisory.ts(and the mirrored site in the engine twin) — matching the "silence is not certainty" fixCONFIDENCE_WHEN_UNSTATEDalready applied at the model-parsing layer.repo_not_registered→repo_not_cached(mirrorspr_not_cached/issue_not_cached's naming for the identical "not yet synced" shape) to resolve the near-miss withrepo_unregistered, which fires when a repo record exists but isn't in the Gittensor registry snapshot — a plain non-blocking advisory warning, the opposite ofrepo_not_cached's app-state hold. Renamed consistently across bothsrc/rules/advisory.tsand the engine twin (gate-advisory.ts), plus their consumers (parity-wire.ts,unified-comment-bridge.ts) and tests.Validation
npx tsc --noEmitclean on the host,@loopover/engine, and@loopover/minerpackages.npm run build --workspace @loopover/engineclean.npx tsx scripts/check-engine-parity.ts— ok (7 hand-duplicated pairs agree).@loopover/enginetest suite (npm run test --workspace @loopover/engine): 739/739 pass.npx vitest run test/contract/engine-parity.test.ts: 14/14 pass.npm run test:coverage/test:cigate per the scope of this pass — happy to run it on request.Closes #9054
Closes #9062
Closes #9083
Closes #9085