diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index ddee2296a8..e3ef053815 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -23,7 +23,8 @@ // applyAutoTune engages nothing → isHoldOnly is false → the merge path is unchanged. The breaker only engages // once a repo's merge precision actually drops below the floor over a real sample. -import { recordAuditEvent } from "../db/repositories"; +import { getPullRequest, listPullRequestFiles, recordAuditEvent } from "../db/repositories"; +import { evaluateSuccessorMatch, REVERSAL_SUPERSEDED_EVENT_TYPE, SUPERSEDED_LOOKBACK_MS } from "./reversal-superseded"; import { createSignalStore } from "./signal-tracking-wire"; import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; import { tryEnqueueDecisionPackRebuild } from "../services/decision-pack"; @@ -675,6 +676,10 @@ export async function recordReversalSignals( await recordConfiguredGateBlockerOverrides(env, targetId).catch(() => undefined); // #8104 await recordLinkedIssueScopeMismatchOverride(env, targetId).catch(() => undefined); // #8101 } + // #8166: the one-shot culture's reversal shape — this merge may supersede a bot-CLOSED sibling PR + // (same linked issue, or same author reworking the same files). Best-effort, like every signal here. + await recordSupersededReversals(env, repoFullName, pr.number, payload.pull_request?.user?.login ?? null).catch(() => undefined); + const reverted = parseRevertedPrNumber(pr.body); if (!reverted) return; const revertedTargetKey = reviewAuditTargetId(repoFullName, reverted); @@ -926,3 +931,77 @@ export async function runSelfTuneBreaker(env: Env): Promise { ); } } + +/** + * #8166: scan the window for bot-CLOSED PRs this merge supersedes, and record the culture-correct reversal + * signal for each match: a `reversal_superseded` row in BOTH stores (like its reopen/revert siblings, with + * the matched heuristics in the audit metadata so borderline calls stay reviewable), plus the SAME per-rule + * "the firing was wrong" overrides the reopen path records (#8101/#8104) — which is what finally feeds the + * calibration corpus its positive class. Conservative + idempotent: evaluateSuccessorMatch's own bar + * decides, a target with an existing superseded row is never re-recorded, and every step fails safe. + */ +export async function recordSupersededReversals( + env: Env, + repoFullName: string, + mergedPrNumber: number, + mergedAuthorLogin: string | null, +): Promise { + try { + const project = repoFullName.slice(0, 200); + const mergedRecord = await getPullRequest(env, repoFullName, mergedPrNumber); + if (!mergedRecord) return; + const mergedFiles = (await listPullRequestFiles(env, repoFullName, mergedPrNumber)).map((file) => file.path); + const merged = { + authorLogin: mergedAuthorLogin ?? mergedRecord.authorLogin, + linkedIssues: mergedRecord.linkedIssues, + files: mergedFiles, + }; + + const sinceIso = new Date(Date.now() - SUPERSEDED_LOOKBACK_MS).toISOString(); + const candidates = await env.DB.prepare( + // Same bot-close definition as lastBotActionWasClose: real (non-dry-run) executed closes only. + `SELECT DISTINCT target_key FROM audit_events + WHERE event_type = 'agent.action.close' AND outcome IN ('success', 'completed') + AND COALESCE(json_extract(metadata_json, '$.mode'), 'live') <> 'dry_run' + AND target_key LIKE ? AND created_at >= ?`, + ) + .bind(`${project}#%`, sinceIso) + .all<{ target_key: string }>(); + + for (const row of candidates.results ?? []) { + const targetKey = row.target_key; + const closedNumber = Number(targetKey.slice(targetKey.lastIndexOf("#") + 1)); + if (!Number.isFinite(closedNumber) || closedNumber === mergedPrNumber) continue; + // Idempotent per closed target: one superseded record ever, however many successors merge later. + const already = await env.DB.prepare("SELECT 1 AS x FROM audit_events WHERE event_type = ? AND target_key = ? LIMIT 1") + .bind(REVERSAL_SUPERSEDED_EVENT_TYPE, targetKey) + .first<{ x: number }>(); + if (already) continue; + + const closedRecord = await getPullRequest(env, repoFullName, closedNumber); + if (!closedRecord) continue; + const closedFiles = (await listPullRequestFiles(env, repoFullName, closedNumber)).map((file) => file.path); + const heuristics = evaluateSuccessorMatch(merged, { + authorLogin: closedRecord.authorLogin, + linkedIssues: closedRecord.linkedIssues, + files: closedFiles, + }); + if (!heuristics) continue; + + const summary = `Bot-closed PR #${closedNumber} superseded by merged PR #${mergedPrNumber}.`; + await appendReviewAudit(env, { project, targetId: targetKey, eventType: REVERSAL_SUPERSEDED_EVENT_TYPE, summary }); + await recordAuditEvent(env, { + eventType: REVERSAL_SUPERSEDED_EVENT_TYPE, + actor: mergedAuthorLogin, + targetKey, + outcome: "completed", + detail: summary, + metadata: { repoFullName, pullNumber: closedNumber, supersededBy: mergedPrNumber, heuristics }, + }).catch(() => undefined); + await recordConfiguredGateBlockerOverrides(env, targetKey).catch(() => undefined); // #8104 + await recordLinkedIssueScopeMismatchOverride(env, targetKey).catch(() => undefined); // #8101 + } + } catch (error) { + console.warn(JSON.stringify({ event: "reversal_superseded_error", repo: repoFullName, message: errorMessage(error).slice(0, 200) })); + } +} diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index 3918112586..cc3fbf8e4d 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -310,7 +310,7 @@ export async function getPublicStats( SELECT substr(target_key, 1, instr(target_key, '#') - 1) AS project, CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS pr_number FROM audit_events - WHERE event_type IN ('reversal_reopened', 'reversal_reverted') + WHERE event_type IN ('reversal_reopened', 'reversal_reverted', 'reversal_superseded') AND outcome = 'completed' AND instr(target_key, '#') > 0 ) ev WHERE LOWER(ev.project) IN (${inList}) diff --git a/src/review/reversal-superseded.ts b/src/review/reversal-superseded.ts new file mode 100644 index 0000000000..d5282f4cb2 --- /dev/null +++ b/src/review/reversal-superseded.ts @@ -0,0 +1,61 @@ +// Successor-based reversal heuristics (#8166, feeds epic #8082's positive class). This gate's own one-shot +// design tells a wronged contributor "recovery = open a fresh PR", so the reopen-shaped reversal signal +// (`reversal_reopened`) is structurally near-impossible here — verified in production: zero reversal events +// ever, zero bot-closed PRs later merged. The culture's ACTUAL "the bot was wrong" shape is: bot CLOSES +// PR #N, and a SUCCESSOR PR — same linked issue, or same author reworking the same files — later MERGES. +// +// PURE MODULE: the match decision only. Conservative by design (the issue's own bar): a false "the bot was +// wrong" poisons calibration worse than a miss, so a match requires either a shared linked issue (the +// strongest intent signal this repo has — the same set-intersection `duplicate_pr_risk` trusts) or the same +// author reworking a majority of the closed PR's files. Borderline records NOTHING. The wire +// (outcomes-wire.ts's recordSupersededReversals) supplies the data and writes the events. + +export const REVERSAL_SUPERSEDED_EVENT_TYPE = "reversal_superseded"; + +/** A successor must re-touch at least this fraction of the CLOSED PR's files for the same-author path. */ +export const SUPERSEDED_FILE_OVERLAP_MIN = 0.5; + +/** How far back a merge scans for bot-closed PRs it might supersede. Mirrors the calibration lookbacks' + * order of magnitude — a months-later rework is a new effort, not a supersession signal. */ +export const SUPERSEDED_LOOKBACK_MS = 30 * 24 * 60 * 60 * 1000; + +export type SupersededSide = { + authorLogin: string | null | undefined; + linkedIssues: readonly number[]; + files: readonly string[]; +}; + +export type SupersededHeuristics = { + sameLinkedIssue: boolean; + sameAuthorFileOverlap: boolean; + /** |shared files| / |closed PR's files|; null when the closed PR has no recorded files. */ + fileOverlapRatio: number | null; +}; + +/** + * Decide whether `merged` supersedes the bot-closed `closed` PR. Returns the matched heuristics (for the + * audit trail — every recorded event carries WHY it matched) or null when neither conservative path holds: + * • sameLinkedIssue — both sides link at least one common issue number; + * • sameAuthorFileOverlap — same author (case-insensitive; unknown authors never match) AND the merged PR + * re-touches ≥ {@link SUPERSEDED_FILE_OVERLAP_MIN} of the closed PR's recorded files (a closed PR with + * no recorded files can never match this path — fail-open to a miss, never a guess). + * PURE and deterministic. + */ +export function evaluateSuccessorMatch(merged: SupersededSide, closed: SupersededSide): SupersededHeuristics | null { + const sameLinkedIssue = closed.linkedIssues.length > 0 && closed.linkedIssues.some((issue) => merged.linkedIssues.includes(issue)); + + const mergedAuthor = merged.authorLogin?.trim().toLowerCase() ?? ""; + const closedAuthor = closed.authorLogin?.trim().toLowerCase() ?? ""; + const sameAuthor = mergedAuthor !== "" && mergedAuthor === closedAuthor; + + let fileOverlapRatio: number | null = null; + if (closed.files.length > 0) { + const mergedFiles = new Set(merged.files); + const shared = closed.files.filter((file) => mergedFiles.has(file)).length; + fileOverlapRatio = shared / closed.files.length; + } + const sameAuthorFileOverlap = sameAuthor && fileOverlapRatio !== null && fileOverlapRatio >= SUPERSEDED_FILE_OVERLAP_MIN; + + if (!sameLinkedIssue && !sameAuthorFileOverlap) return null; + return { sameLinkedIssue, sameAuthorFileOverlap, fileOverlapRatio }; +} diff --git a/src/services/public-accuracy-trend.ts b/src/services/public-accuracy-trend.ts index d6f09b1f50..0b11937f01 100644 --- a/src/services/public-accuracy-trend.ts +++ b/src/services/public-accuracy-trend.ts @@ -126,7 +126,7 @@ async function loadReversalDayRows(env: Env, projects: string[], sinceIso: strin ) orig JOIN ( SELECT DISTINCT target_key FROM audit_events - WHERE event_type IN ('reversal_reopened', 'reversal_reverted') AND outcome = 'completed' + WHERE event_type IN ('reversal_reopened', 'reversal_reverted', 'reversal_superseded') AND outcome = 'completed' ) rev ON rev.target_key = orig.target_key WHERE LOWER(orig.project) IN (${inList}) GROUP BY day`, diff --git a/test/unit/reversal-superseded.test.ts b/test/unit/reversal-superseded.test.ts new file mode 100644 index 0000000000..28e1a88506 --- /dev/null +++ b/test/unit/reversal-superseded.test.ts @@ -0,0 +1,215 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { evaluateSuccessorMatch, REVERSAL_SUPERSEDED_EVENT_TYPE, SUPERSEDED_FILE_OVERLAP_MIN } from "../../src/review/reversal-superseded"; +import { recordSupersededReversals } from "../../src/review/outcomes-wire"; +import { recordAuditEvent, upsertPullRequestFile, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import * as repositories from "../../src/db/repositories"; +import * as signalTrackingWire from "../../src/review/signal-tracking-wire"; +import { createSignalStore } from "../../src/review/signal-tracking-wire"; +import { createTestEnv } from "../helpers/d1"; + +// #8166: the one-shot culture's reversal shape — a bot-closed PR superseded by a merged successor. The pure +// matcher's conservatism and the wire's recording/idempotency are the two load-bearing properties. + +const side = (over: Partial[0]> = {}) => ({ + authorLogin: "alice", + linkedIssues: [42], + files: ["src/a.ts", "src/b.ts"], + ...over, +}); + +describe("evaluateSuccessorMatch (#8166)", () => { + it("matches on a shared linked issue — the strongest intent signal — regardless of author/files", () => { + const heuristics = evaluateSuccessorMatch(side({ authorLogin: "bob", files: [] }), side()); + expect(heuristics).toMatchObject({ sameLinkedIssue: true, sameAuthorFileOverlap: false }); + }); + + it("matches on same author + majority file overlap when no linked issue is shared", () => { + const heuristics = evaluateSuccessorMatch( + side({ linkedIssues: [], files: ["src/a.ts", "src/b.ts", "src/new.ts"] }), + side({ linkedIssues: [] }), + ); + expect(heuristics).toMatchObject({ sameLinkedIssue: false, sameAuthorFileOverlap: true, fileOverlapRatio: 1 }); + }); + + it("is conservative: below-threshold overlap, different/unknown authors, and empty closed-file lists all record NOTHING", () => { + // Overlap below the floor (1 of 3 files). + expect( + evaluateSuccessorMatch(side({ linkedIssues: [], files: ["src/a.ts"] }), side({ linkedIssues: [], files: ["src/a.ts", "src/b.ts", "src/c.ts"] })), + ).toBeNull(); + // Different author. + expect(evaluateSuccessorMatch(side({ linkedIssues: [], authorLogin: "bob" }), side({ linkedIssues: [] }))).toBeNull(); + // Unknown authors never match the author path. + expect(evaluateSuccessorMatch(side({ linkedIssues: [], authorLogin: null }), side({ linkedIssues: [], authorLogin: null }))).toBeNull(); + expect(evaluateSuccessorMatch(side({ linkedIssues: [], authorLogin: " " }), side({ linkedIssues: [], authorLogin: " " }))).toBeNull(); + // Closed PR with no recorded files: ratio is null, the author path can never fire. + const noFiles = evaluateSuccessorMatch(side({ linkedIssues: [] }), side({ linkedIssues: [], files: [] })); + expect(noFiles).toBeNull(); + // Neither side links issues and files half-overlap exactly at the floor: matches (boundary is inclusive). + expect( + evaluateSuccessorMatch(side({ linkedIssues: [], files: ["src/a.ts"] }), side({ linkedIssues: [], files: ["src/a.ts", "src/b.ts"] })), + ).toMatchObject({ sameAuthorFileOverlap: true, fileOverlapRatio: SUPERSEDED_FILE_OVERLAP_MIN }); + }); + + it("author comparison is case-insensitive and trimmed", () => { + expect( + evaluateSuccessorMatch(side({ linkedIssues: [], authorLogin: " Alice " }), side({ linkedIssues: [], authorLogin: "alice" })), + ).not.toBeNull(); + }); +}); + +describe("recordSupersededReversals (#8166 wire)", () => { + const REPO = "owner/repo"; + + afterEach(() => { + vi.restoreAllMocks(); + }); + + async function seedClosedPr(env: Env, number: number, options: { author?: string; body?: string; files?: string[]; dryRun?: boolean } = {}) { + await upsertPullRequestFromGitHub(env, REPO, { + number, + title: `PR ${number}`, + state: "closed", + body: options.body ?? "Closes #42", + user: { login: options.author ?? "alice" }, + head: { sha: `sha${number}` }, + labels: [], + }); + for (const path of options.files ?? ["src/a.ts", "src/b.ts"]) { + await upsertPullRequestFile(env, { repoFullName: REPO, pullNumber: number, path, status: "modified", additions: 1, deletions: 1, changes: 2, payload: {} }); + } + await recordAuditEvent(env, { + eventType: "agent.action.close", + targetKey: `${REPO}#${number}`, + outcome: "completed", + ...(options.dryRun ? { metadata: { mode: "dry_run" } } : {}), + createdAt: new Date(Date.now() - 60_000).toISOString(), + }); + } + + async function seedMergedPr(env: Env, number: number, options: { author?: string; body?: string; files?: string[] } = {}) { + await upsertPullRequestFromGitHub(env, REPO, { + number, + title: `PR ${number}`, + state: "closed", + merged_at: new Date().toISOString(), + body: options.body ?? "Closes #42", + user: { login: options.author ?? "alice" }, + head: { sha: `sha${number}` }, + labels: [], + }); + for (const path of options.files ?? ["src/a.ts", "src/b.ts"]) { + await upsertPullRequestFile(env, { repoFullName: REPO, pullNumber: number, path, status: "modified", additions: 1, deletions: 1, changes: 2, payload: {} }); + } + } + + async function supersededRows(env: Env) { + const rows = await env.DB.prepare("SELECT target_key, metadata_json FROM audit_events WHERE event_type = ?") + .bind(REVERSAL_SUPERSEDED_EVENT_TYPE) + .all<{ target_key: string; metadata_json: string }>(); + return rows.results ?? []; + } + + it("records the superseded reversal + the per-rule reversed overrides that feed the corpus's positive class", async () => { + const env = createTestEnv(); + await seedClosedPr(env, 7); + // The closed PR's own fired signal — the thing the override must mark reversed. + await createSignalStore(env).recordRuleFired({ + ruleId: "linked_issue_scope_mismatch", + targetKey: `${REPO}#7`, + outcome: "unaddressed", + occurredAt: new Date(Date.now() - 120_000).toISOString(), + metadata: { confidence: 0.8 }, + }); + await seedMergedPr(env, 9); + + await recordSupersededReversals(env, REPO, 9, "alice"); + + const rows = await supersededRows(env); + expect(rows).toHaveLength(1); + expect(rows[0]!.target_key).toBe(`${REPO}#7`); + const metadata = JSON.parse(rows[0]!.metadata_json) as { supersededBy: number; heuristics: { sameLinkedIssue: boolean } }; + expect(metadata.supersededBy).toBe(9); + expect(metadata.heuristics.sameLinkedIssue).toBe(true); + + const overrides = await env.DB.prepare("SELECT metadata_json FROM audit_events WHERE event_type = 'signal.human_override:linked_issue_scope_mismatch' AND target_key = ?") + .bind(`${REPO}#7`) + .all<{ metadata_json: string }>(); + expect(overrides.results).toHaveLength(1); + expect(JSON.parse(overrides.results![0]!.metadata_json).verdict).toBe("reversed"); + }); + + it("is idempotent per closed target and skips: itself, dry-run closes, and non-matching candidates", async () => { + const env = createTestEnv(); + await seedClosedPr(env, 7); // matches (shared issue #42) + await seedClosedPr(env, 8, { body: "Different work entirely", author: "someone-else", files: ["docs/x.md"] }); // no match + await seedClosedPr(env, 11, { dryRun: true }); // dry-run close is not a bot close + await seedMergedPr(env, 9); + + await recordSupersededReversals(env, REPO, 9, "alice"); + await recordSupersededReversals(env, REPO, 9, "alice"); // second pass must be a no-op + + const rows = await supersededRows(env); + expect(rows).toHaveLength(1); + expect(rows[0]!.target_key).toBe(`${REPO}#7`); + }); + + it("skips a self-close (the merged PR was itself bot-closed once), garbage target keys, and closes with no stored record", async () => { + const env = createTestEnv(); + await seedMergedPr(env, 9); + // The merged PR itself carries a historical bot-close row -- must never supersede itself. + await recordAuditEvent(env, { eventType: "agent.action.close", targetKey: `${REPO}#9`, outcome: "completed", createdAt: new Date(Date.now() - 90_000).toISOString() }); + // A garbled target key whose number cannot parse. + await recordAuditEvent(env, { eventType: "agent.action.close", targetKey: `${REPO}#junk`, outcome: "completed", createdAt: new Date(Date.now() - 90_000).toISOString() }); + // A real-looking close with NO stored pull_requests record behind it. + await recordAuditEvent(env, { eventType: "agent.action.close", targetKey: `${REPO}#55`, outcome: "completed", createdAt: new Date(Date.now() - 90_000).toISOString() }); + + await recordSupersededReversals(env, REPO, 9, "alice"); + expect(await supersededRows(env)).toHaveLength(0); + }); + + it("stays best-effort per write: a rejecting audit-event write and rejecting override writes are swallowed, the review_audit record survives", async () => { + const env = createTestEnv(); + await seedClosedPr(env, 7); + await seedMergedPr(env, 9); + vi.spyOn(repositories, "recordAuditEvent").mockRejectedValue(new Error("D1 write error")); + // A synchronous construction failure is the only shape that rejects BOTH override recorders + // (the #8104 one catches per-code internally, so a queryRuleHistory throw never escapes it). + vi.spyOn(signalTrackingWire, "createSignalStore").mockImplementation(() => { + throw new Error("signal store down"); + }); + + await expect(recordSupersededReversals(env, REPO, 9, "alice")).resolves.toBeUndefined(); + + const audit = await env.DB.prepare("SELECT event_type FROM review_audit WHERE event_type = ?") + .bind(REVERSAL_SUPERSEDED_EVENT_TYPE) + .all<{ event_type: string }>(); + expect(audit.results).toHaveLength(1); + // The mocked write rejected, so no audit_events superseded row can exist — proves the spy actually intercepted. + expect(await supersededRows(env)).toHaveLength(0); + }); + + it("treats an undefined D1 result set as no candidates and falls back to the stored author when the payload has none", async () => { + const env = createTestEnv(); + await seedMergedPr(env, 9); + const origPrepare = env.DB.prepare.bind(env.DB); + env.DB.prepare = ((sql: string) => + sql.includes("SELECT DISTINCT target_key") + ? ({ bind: () => ({ all: async () => ({ results: undefined }) }) } as never) + : origPrepare(sql)) as typeof env.DB.prepare; + + // Null payload author exercises the mergedRecord.authorLogin fallback. + await expect(recordSupersededReversals(env, REPO, 9, null)).resolves.toBeUndefined(); + env.DB.prepare = origPrepare; + expect(await supersededRows(env)).toHaveLength(0); + }); + + it("records nothing when the merged PR has no stored record, and fails safe (never throws) on a broken DB", async () => { + const env = createTestEnv(); + await recordSupersededReversals(env, REPO, 999, "alice"); + expect(await supersededRows(env)).toHaveLength(0); + + const broken = createTestEnv(); + broken.DB = { prepare: () => { throw new Error("boom"); } } as never; + await expect(recordSupersededReversals(broken, REPO, 1, "alice")).resolves.toBeUndefined(); + }); +});