fix(orb): close-authority integrity across four review-engine gaps - #9206
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | df89791 | Commit Preview URL Branch Preview URL |
Jul 27 2026, 06:58 AM |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-27 06:55:18 UTC
Review summary Nits — 5 non-blocking
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.
|
Four related fixes to the review-engine's close/gate-execution path: - queue liveness sweep released ANY future-scheduled foreground row, not just rate-limit deferrals, collapsing the contributor grace window from 300s to ~60s. Added a `deferred_by` provenance column (pg + sqlite queues) set only at admission-gate defer sites, so an enqueue-time delay is now structurally excluded from the release candidate set regardless of age. - duplicate_pr_risk was derived entirely from a rival PR's own body text yet could one-shot-close a PR by default, exempt from the close-precision breaker. Corroboration (changed-file overlap, or the sibling being a non-trivial real change) now gates the concrete code from the always- advisory duplicate_pr_risk_unconfirmed code; the code-level default changed to advisory; and a gate that fails solely on a duplicate finding now HOLDS instead of closing, under any duplicatePrGateMode. - SELFHOST_DEPLOYMENT_MODE=dry-run was consulted only inside the octokit request hook, one layer below every decision that depends on it — the executor believed it was live and drove real outcome writes, CI cancellation, notifications, and moderation escalation off a suppressed mutation. Folded the instance-wide switch into resolveAgentActionMode as the first precedence term (threaded through every call site), routed the CI-cancel raw fetch through the same chokepoint, and made a suppressed merge structurally distinguishable from a real one. - review-nag closes never went through applyPrecisionBreakers or recordNativeGateDecision, so #9086's breaker coverage was inert and the close was invisible to merge-precision calibration. Extracted the shared precision-breaker + gate-decision-recording sequence into finalizePolicyCloseDisposition and wired both review-nag close sites through it. Mirrored the duplicate-detection fix into the engine package's gate-advisory twin (packages/loopover-engine) to keep the live-gate/predicted-gate parity contract green, and updated the golden fixtures it feeds. Closes #9127, Closes #9129, Closes #9130, Closes #9132
cc7d78a to
df89791
Compare
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9206 +/- ##
==========================================
- Coverage 93.94% 93.93% -0.02%
==========================================
Files 821 821
Lines 81601 81656 +55
Branches 24769 24787 +18
==========================================
+ Hits 76662 76705 +43
- Misses 3556 3558 +2
- Partials 1383 1393 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
Four fixes to the review-engine's close/gate-execution path, all touching the same close-authority surface:
orb(queue): foreground-liveness cancels every deliberate delay, not just rate-limit deferrals — the contributor's promised close grace window collapses from 300s to 60s #9127 — foreground-liveness sweep released deliberate delays, not just rate-limit deferrals.
releaseStaleForegroundDeferrals(pg-queue.ts / sqlite-queue.ts) selected any pending foreground row scheduled in the future with no provenance filter, andisRateLimitAdmissionNowClearreturnedtruefor any job with no rate-limit bucket — so a job enqueued with a deliberatedelaySeconds(e.g. the linked-issue flag-then-close 300s grace window) was released on the very next sweep tick (≤60s). Added adeferred_byprovenance column, set only at the three admission-gate defer sites (rate-limit, maintenance-admission, installation-concurrency); the candidateSELECTnow requiresdeferred_by IS NOT NULL, structurally excluding enqueue-time delays regardless of age. The rate-limit-clear recheck is now scoped todeferred_by='rate_limit'specifically (a maintenance-admission or installation-concurrency deferral has no rate-limit bucket either, and would have hit the same false-premise bug for a different admission kind). Corrected the module header's false premise inforeground-liveness.ts. Mirrored in both the Postgres and SQLite queue backends.orb(gate): any contributor can force-close a rival's PR by citing the same issue number — breaker-exempt, default-on, and it has already closed 2 PRs #9129 —
duplicate_pr_riskwas an offensive close primitive. The finding was derived entirely from a rival PR's own body text, blocked by default, and exempt from the close-precision breaker — so any contributor could force-close a rival's clean PR by citing the same issue number in a throwaway PR (no code required). Now requires corroboration (a changed-file-path overlap, or the sibling being a non-trivial real change) before the concreteduplicate_pr_riskcode fires at all; an uncorroborated citation gets the separate, always-non-blockingduplicate_pr_risk_unconfirmedcode. Dropped the duplicate-link check fromhasConcreteCloseEvidence(the breaker can now downgrade a duplicate-driven close). Changed the code-level default forduplicatePrGateModefromblocktoadvisory. Most importantly:evaluateGateCheckCorenow HOLDS (neutral) rather than closes when a gate fails solely onduplicate_pr_risk, under anyduplicatePrGateMode— there is no configuration left that lets this finding close a PR. Mirrored into the engine package'sgate-advisory.tstwin to keep the live-gate/predicted-gate parity contract accurate (predicted-gate would otherwise have kept predicting the old, wrong verdict).orb(safety): SELFHOST_DEPLOYMENT_MODE=dry-run does not reach the executor — a 'posts nothing' instance still cancels CI, globally blacklists contributors, and writes fabricated merge outcomes #9130 —
SELFHOST_DEPLOYMENT_MODE=dry-rundidn't reach the executor. The instance-wide kill switch was consulted only insidemakeInstallationOctokit's request hook — one layer below every decision that depends on it. The executor computed its own mode from per-repo settings alone, believed it was live, and drove real outcome writes, CI cancellation, Discord/Slack notifications, and moderation escalation off a mutation that was actually suppressed at the wire. FoldedforcedSelfhostMode(env)intoresolveAgentActionModeas the first ("most restrictive wins") precedence term, threaded through all 22 call sites acrossprocessors.ts,agent-action-executor.ts,review-evasion.ts,automation-state.ts, andclient.ts's ownresolveRepoActionMode. Routed the CI-cancel raw-fetch call (cancelInFlightWorkflowRunsForHeadSha, the last installation write that bypassed the octokit suppression hook) through the same chokepoint. GavemergePullRequestasuppressedfield so a synthetic shadow response can never look like a real merge to the executor, as defense in depth.orb(breaker): #9086's review_nag breaker coverage is inert — both nag close sites skip the breaker entirely, and each close is scored as a merge misprediction #9132 — review-nag breaker coverage was inert. orb(gate): the six anti-abuse close paths are all exempt from the precision breaker AND from live recheck #9086 added
review_nagto the breaker's content-inspection close kinds, but neither review-nag close site ever calledapplyPrecisionBreakersorrecordNativeGateDecision— they built a plan and executed it directly. The breaker could never engage, and (because nogate_decisionrow was written) every review-nag close was later scored as a merge misprediction against the repo's calibration corpus (the gate_decision can contradict the action taken — stale 'merge' verdicts recorded after CI-failure closes distort calibration #8825 bug class). Extracted the shared sequence (applyPrecisionBreakers→maybeApplyCloseAuditHoldout→recordNativeGateDecision(reasonCode: "policy_close:review_nag")→buildDecisionRecord/persistDecisionRecord→ contributor/predicted-gate calibration) intofinalizePolicyCloseDispositionand wired both review-nag close sites through it.Follow-up noted, not fixed here (per #9132's own scoping)
Audited all 7
executeAgentMaintenanceActionscall sites. Besides the main disposition path (already correct) and the two review-nag sites (fixed here):processors.ts'scontributor_capearly-close short-circuit (~line 3013) has the same gap-class as review-nag (a policy close bypassing the breaker/recording sequence) and is a good follow-up candidate forfinalizePolicyCloseDisposition.update_branch-only forced-rebase call sites are not a merge/close disposition at all (no gate decision concept applies).agent-approval-queue.ts's accept-time replay already recorded a gate decision once, at original staging time — needs a closer look before concluding it's a gap.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not touchsite/,CNAME, or**/lovable/**.Closes #9127
Closes #9129
Closes #9130
Closes #9132
Validation
Ran targeted checks, not the full
npm run test:ci— see the note below for why and what was substituted.git diff --check— clean.npm run actionlint— not run (no workflow files touched).npm run typecheck(tsc --noEmit) — ran repeatedly against the full changed set; zero new errors (pre-existing failures are allCannot find module '@loopover/engine'in files that neednpm run build --workspace @loopover/enginefirst, unrelated to this diff).npm run test:coverage— not run in unsharded form; substituted withnpx vitest run --changed=origin/main, which selected 555 test files / 12,702 tests across the real import graph of this diff (12,686 passed, 16 pre-existing integration skips requiring a real Postgres) after fixing every test this diff's behavior change legitimately invalidated (duplicate-detection golden fixtures, engine-parity goldens, and the review-nag/foreground-liveness/dry-run test suites this PR extends). Every new/changed line in the four issues' functions has a dedicated invariant or regression test (see below); I did not generate an unsharded coverage % number.npm run test:workers— not run.npm run build:mcp— ran, clean (needed to rebuild@loopover/miner/@loopover/mcpagainst the rebuilt@loopover/engineso their own compiled-dist/tests weren't stale).npm run test:mcp-pack— not run directly; MCP-affected tests were included in the--changedsweep above.npm run ui:openapi:check— clean (no API/schema changes; verified anyway).npm run ui:lint/ui:typecheck/ui:build— not run (no UI changes in this PR).npm audit --audit-level=moderate— 5 pre-existing high-severity findings, all ineslint's transitivebrace-expansion/minimatchchain on a cleanorigin/maincheckout before any of my changes (confirmed by running the same audit before starting). Unrelated to this diff; fixing them requires an unrelated breakingeslintmajor-version bump, out of scope here.Additional generated-artifact/drift checks run and confirmed clean:
npm run db:migrations:check,npm run db:schema-drift:check,npm run selfhost:env-reference:check,npm run cf-typegen:check,npm run manifest:drift-check,npm run engine-parity:drift-check(the last one specifically validates thepackages/loopover-enginetwin this PR also edits for #9129).If any required check was skipped, explain why: instructed not to run the full
npm run test:cigate for this PR (it will run in CI regardless); relied on targetedtsc --noEmitplus a fullvitest run --changed=origin/mainsweep instead, since that sweep exercises every test whose real import graph touches this diff — a stronger signal than a partial manual selection, even without an unsharded coverage percentage.Per-issue test coverage
test/unit/selfhost-pg-queue.test.ts/test/unit/selfhost-sqlite-queue.test.ts— new invariant tests (adeferred_by-less row is never released regardless of age-staleness; the linked-issue grace-window job type survives a liveness sweep even as the sole candidate) and a new test proving the rate-limit-clear recheck is scoped todeferred_by='rate_limit'only (other admission-gate tags only release via the age arm). All pre-existing tests in both files updated to seed the new column where their scenario needs a genuine admission-gate provenance tag.test/unit/rules.test.ts— new tests for corroborated vs. uncorroborated overlaps, an explicit adversarial-regression test (overlapping linked issue, no diff overlap,duplicatePrGateMode: "block"explicitly set → never closes, never even holds), and a corroborated-overlap test proving the new hold behavior. Updatedtest/unit/agent-actions.test.ts,test/unit/maintainer-activation.test.ts,test/unit/queue-5.test.ts(a live end-to-end webhook-driven regression),test/unit/predicted-gate.test.ts,test/unit/predicted-gate-engine.test.ts, andtest/unit/self-review-adapter.test.tsfor the corrected behavior. Golden-fixture/corpus updates:test/fixtures/engine-parity/predicted-gate/duplicate-pr-block.ts+ its golden JSON,test/golden-corpus/gate-corpus.json(version bumped to 2, with the version-change documented in its own$comment).test/unit/agent-execution.test.ts— full branch coverage ofresolveAgentActionMode's newinstanceModeprecedence.test/unit/github-client.test.ts—resolveRepoActionModenow folds inSELFHOST_DEPLOYMENT_MODE.test/unit/github-app.test.ts—cancelInFlightWorkflowRunsForHeadShais suppressed with zero network calls under dry-run/disabled.test/unit/github-pr-actions.test.ts—mergePullRequestmarks a suppressed writesuppressed: truewith zero calls to the real merge endpoint.test/unit/agent-action-executor.test.ts— the full end-to-end invariant: underSELFHOST_DEPLOYMENT_MODE=dry-runwith every per-repo setting saying live, a merge + a moderation-escalating close together produce zero GitHub writes, zeropr_outcomerows, zero notifications, and zero blacklist entries despite abanThreshold: 1that would otherwise auto-ban outright.test/unit/queue-5.test.ts— extended both review-nag close sites' existing live end-to-end tests to assert agate_decisionrow now exists withreasonCode: "policy_close:review_nag"and adecision_recordsrow naming the same reason, plus a new regression test proving a review-nag close is genuinely breaker-eligible (downgraded to a hold when the project's close-precision breaker is engaged, never reaching GitHub).Safety
ui:openapi:check.UI Evidencesection — N/A, no UI changes.CHANGELOG.mdedit (not a release-prep PR); no other public docs reference the internals this PR changes.Notes
@loopover/mcpgate-prediction preflight (loopover_predict_gate/loopover_check_slop_risk/loopover_lint_pr_text) — this is a non-interactive maintainer session and the tool's device-flow login isn't available here. Relied on thevitest --changedsweep, targetedtypecheck, and manual review of this description instead.packages/loopover-engine/src/advisory/gate-advisory.ts(the intentionally-divergent predicted-gate twin) — not something I'd assumed going in, buttest/contract/live-gate-parity.test.tscorrectly caught that the live gate and predicted gate would otherwise silently disagree on this exact scenario. Updated the one golden fixture this touches (duplicate-pr-block) and its golden JSON via the project's ownscripts/record-engine-parity-goldens.ts.