From 878e8a0355c4c745e7fb323707cea0fff99d5a44 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 17 Jul 2026 10:38:53 -0700 Subject: [PATCH] fix(review): make review.selftune: false opt-out absolute for the breaker too The accuracy circuit-breaker (runSelfTuneBreaker) had no per-repo opt-out at all, unlike its sibling selfTuneRepos() (the routine tuning pass), which already correctly excludes a repo whose .loopover.yml sets review.selftune: false. A repo could opt out of routine tuning yet still have its gate mode forced into holdonly/closehold by the breaker. Per that flag's own documented intent ("excludes this repo from the tuning pass"), the opt-out is now absolute: an opted-out repo is excluded from every part of self-tune, both the merge and close breakers, in both the plain and miner-scoped passes, and in both directions -- the breaker can neither newly engage a hold for an opted-out repo nor auto-clear an already-engaged one. A manifest-load error fails open (repo stays included), matching selfTuneRepos()'s same fail-safe precedent. Closes #6803 --- src/review/outcomes-wire.ts | 64 +++++++++++++++++++++--- test/unit/outcomes-wire.test.ts | 86 +++++++++++++++++++++++++++++++++ 2 files changed, 142 insertions(+), 8 deletions(-) diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 13845f4302..b7fd72828f 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -26,6 +26,7 @@ import { recordAuditEvent } from "../db/repositories"; import { tryEnqueueDecisionPackRebuild } from "../services/decision-pack"; import { incr } from "../selfhost/metrics"; +import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; import type { GitHubWebhookPayload } from "../types"; import { errorMessage, nowIso } from "../utils/json"; import { @@ -537,6 +538,52 @@ function minerBreakerScope(project: string): string { return `${project}${MINER_BREAKER_SCOPE_SUFFIX}`; } +/** Strip the `:miner` scope suffix a project key MAY carry, so both `listEngagedProjectScopes`'s scoped keys + * and `GateEvalReport.rows[].project` resolve to the same real repo full name the opt-out check needs. */ +function baseProjectName(project: string): string { + return project.endsWith(MINER_BREAKER_SCOPE_SUFFIX) ? project.slice(0, -MINER_BREAKER_SCOPE_SUFFIX.length) : project; +} + +/** #6803: the accuracy circuit-breaker (this whole pass) previously had no per-repo opt-out at all, unlike its + * sibling `selfTuneRepos()` (selftune-wire.ts), which already correctly excludes a repo whose `.loopover.yml` + * sets `review.selftune: false` from the routine tuning pass. Per that flag's own documented intent + * ("excludes this repo from the tuning pass"), the opt-out is ABSOLUTE: it excludes a repo from every part of + * self-tune, not just the routine pass, so the breaker must never engage OR auto-clear holdonly/closehold for + * an opted-out repo either -- a manifest-load error fails OPEN (repo stays included), matching + * `selfTuneRepos()`'s own same fail-safe precedent, since a settings-read blip must never silently widen what + * the breaker acts on. */ +async function isSelfTuneOptedOut(env: Env, repoFullName: string): Promise { + const manifest = await loadRepoFocusManifest(env, repoFullName).catch(() => null); + return manifest?.review.selftune === false; +} + +/** Filter a {@link GateEvalReport}'s rows AND a scope's already-engaged flag list down to the projects that are + * NOT self-tune-opted-out, resolving each project key's real repo name first (`:miner`-suffixed keys included) + * -- the two lists this module's every downstream computation (engage candidates, clear candidates) derives + * from, so filtering both here is sufficient for the opt-out to be absolute. */ +async function excludeSelfTuneOptedOut( + env: Env, + report: GateEvalReport, + engagedHoldonly: readonly string[], + engagedClosehold: readonly string[], +): Promise<{ report: GateEvalReport; engagedHoldonly: string[]; engagedClosehold: string[] }> { + const candidateProjects = new Set([ + ...report.rows.map((row) => baseProjectName(row.project)), + ...engagedHoldonly.map(baseProjectName), + ...engagedClosehold.map(baseProjectName), + ]); + const optedOut = new Set(); + for (const repoFullName of candidateProjects) { + if (await isSelfTuneOptedOut(env, repoFullName)) optedOut.add(repoFullName); + } + if (optedOut.size === 0) return { report, engagedHoldonly: [...engagedHoldonly], engagedClosehold: [...engagedClosehold] }; + return { + report: { ...report, rows: report.rows.filter((row) => !optedOut.has(baseProjectName(row.project))) }, + engagedHoldonly: engagedHoldonly.filter((project) => !optedOut.has(baseProjectName(project))), + engagedClosehold: engagedClosehold.filter((project) => !optedOut.has(baseProjectName(project))), + }; +} + /** Run the full engage + auto-clear sequence for one {@link GateEvalReport} (either the plain project-keyed * report or a miner-rescoped one). `eventPrefix` namespaces the emitted log events (`""` for the existing * human/mixed pass, `"miner_"` for the #2352 miner-scoped pass) so an operator can tell which population @@ -638,22 +685,23 @@ export async function runSelfTuneBreaker(env: Env): Promise { const engagedScopes = await listEngagedProjectScopes(env); const isMinerScope = (project: string): boolean => project.endsWith(MINER_BREAKER_SCOPE_SUFFIX); - await runBreakerPassForReport( - flags, + // #6803: exclude every self-tune-opted-out repo from both passes -- see excludeSelfTuneOptedOut's own doc + // comment for why this must happen before engage/clear candidates are computed, not filtered after. + const plainPass = await excludeSelfTuneOptedOut( + env, report, engagedScopes.holdonly.filter((project) => !isMinerScope(project)), engagedScopes.closehold.filter((project) => !isMinerScope(project)), - nowMs, - "", ); - await runBreakerPassForReport( - flags, + const minerPass = await excludeSelfTuneOptedOut( + env, minerReport, engagedScopes.holdonly.filter(isMinerScope), engagedScopes.closehold.filter(isMinerScope), - nowMs, - "miner_", ); + + await runBreakerPassForReport(flags, plainPass.report, plainPass.engagedHoldonly, plainPass.engagedClosehold, nowMs, ""); + await runBreakerPassForReport(flags, minerPass.report, minerPass.engagedHoldonly, minerPass.engagedClosehold, nowMs, "miner_"); } catch (error) { console.warn( JSON.stringify({ diff --git a/test/unit/outcomes-wire.test.ts b/test/unit/outcomes-wire.test.ts index 83d2ef31ac..88a3e42d13 100644 --- a/test/unit/outcomes-wire.test.ts +++ b/test/unit/outcomes-wire.test.ts @@ -18,6 +18,7 @@ import { type PlannedAgentAction, } from "../../src/settings/agent-actions"; import { recordAuditEvent } from "../../src/db/repositories"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import type { GitHubPullRequestPayload } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -774,6 +775,91 @@ describe("runSelfTuneBreaker — reads recorded pr_outcome ground truth + engage expect(await isCloseHoldOnly(env, "owner/repo")).toBe(false); // close breaker auto-cleared }); + describe("#6803: review.selftune: false opt-out is absolute for the breaker too, not just the routine tuning pass", () => { + it("does NOT engage the merge or close breaker for an opted-out repo, even with data that would otherwise trip both", async () => { + const env = createTestEnv(); + await upsertRepoFocusManifest(env, "owner/opted-out", { review: { selftune: false } }); + // Same shape as the plain ENGAGES tests above -- would trip both breakers if this repo weren't opted out. + for (let i = 0; i < 4; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "merge", "merged"); + for (let i = 4; i < 12; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "merge", "closed"); + for (let i = 12; i < 16; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "close", "closed"); + for (let i = 16; i < 24; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "close", "merged"); + + await runSelfTuneBreaker(env); + + expect(await isHoldOnly(env, "owner/opted-out")).toBe(false); + expect(await isCloseHoldOnly(env, "owner/opted-out")).toBe(false); + }); + + it("does NOT auto-clear an already-engaged flag for an opted-out repo, even with fully recovered precision -- the opt-out is absolute, not one-directional", async () => { + const env = createTestEnv(); + const flags = createFlagStore(env); + await flags.setFlag("holdonly:owner/opted-out", true); + await flags.setFlag("closehold:owner/opted-out", true); + await env.DB.prepare( + "UPDATE system_flags SET updated_at = datetime('now', '-2 days') WHERE key IN ('holdonly:owner/opted-out', 'closehold:owner/opted-out')", + ).run(); + await upsertRepoFocusManifest(env, "owner/opted-out", { review: { selftune: false } }); + // Fully recovered precision -- would auto-clear both breakers (per the AUTO-CLEARS test above) if this + // repo weren't opted out. + for (let i = 0; i < 12; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "merge", "merged"); + for (let i = 12; i < 24; i += 1) await seedDecisionAndOutcome(env, "owner/opted-out", i, "close", "closed"); + + const log = vi.spyOn(console, "log").mockImplementation(() => {}); + await runSelfTuneBreaker(env); + log.mockRestore(); + + expect(await isHoldOnly(env, "owner/opted-out")).toBe(true); + expect(await isCloseHoldOnly(env, "owner/opted-out")).toBe(true); + }); + + it("also excludes an opted-out repo from the miner-scoped pass (#2352)", async () => { + const env = createTestEnv(); + await upsertRepoFocusManifest(env, "owner/opted-out", { review: { selftune: false } }); + // Miner-authored rows (miner_authored=1), same shape as the #2352 ENGAGES test: 33% precision, would + // otherwise trip the holdonly:owner/opted-out:miner flag. + for (let i = 0; i < 4; i += 1) { + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, miner_authored, created_at) VALUES (?, ?, ?, 'gate_decision', 'merge', 'gittensory-native', ?, NULL, 1, CURRENT_TIMESTAMP)", + ) + .bind(`gd:m:owner/opted-out#${i}`, "owner/opted-out", `owner/opted-out#${i}`, `sha${i}`) + .run(); + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at) VALUES (?, ?, ?, 'pr_outcome', 'merged', 'gittensory-native', NULL, NULL, CURRENT_TIMESTAMP)", + ) + .bind(`po:m:owner/opted-out#${i}`, "owner/opted-out", `owner/opted-out#${i}`) + .run(); + } + for (let i = 4; i < 12; i += 1) { + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, miner_authored, created_at) VALUES (?, ?, ?, 'gate_decision', 'merge', 'gittensory-native', ?, NULL, 1, CURRENT_TIMESTAMP)", + ) + .bind(`gd:m:owner/opted-out#${i}`, "owner/opted-out", `owner/opted-out#${i}`, `sha${i}`) + .run(); + await env.DB.prepare( + "INSERT INTO review_audit (id, project, target_id, event_type, decision, source, head_sha, summary, created_at) VALUES (?, ?, ?, 'pr_outcome', 'closed', 'gittensory-native', NULL, NULL, CURRENT_TIMESTAMP)", + ) + .bind(`po:m:owner/opted-out#${i}`, "owner/opted-out", `owner/opted-out#${i}`) + .run(); + } + + await runSelfTuneBreaker(env); + + expect(await isHoldOnly(env, "owner/opted-out:miner")).toBe(false); + }); + + it("does not opt out an unrelated repo (the exclusion is per-repo, not global)", async () => { + const env = createTestEnv(); + await upsertRepoFocusManifest(env, "owner/opted-out", { review: { selftune: false } }); + for (let i = 0; i < 4; i += 1) await seedDecisionAndOutcome(env, "owner/still-tuned", i, "merge", "merged"); + for (let i = 4; i < 12; i += 1) await seedDecisionAndOutcome(env, "owner/still-tuned", i, "merge", "closed"); + + await runSelfTuneBreaker(env); + + expect(await isHoldOnly(env, "owner/still-tuned")).toBe(true); + }); + }); + it("never throws (fails safe) even when review_audit reads blow up", async () => { const env = createTestEnv(); const realPrepare = env.DB.prepare.bind(env.DB);