From 4ffebf34e0b01edff98f074090119e3d740e969f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 14 Jul 2026 13:10:38 -0700 Subject: [PATCH] fix(review): exclude maintainer PRs from the slop-discrimination alert The ops-anomaly detector flagged "slop score NOT discriminating" on two repos, reading it as a scoring-calibration problem. It wasn't: the slop scorer's weights, band thresholds, and gate-blocker direction are all internally consistent, and the detector's own unit tests pin the intended "higher band merges less" semantics correctly. The real bug is in buildSlopOutcomeCalibration's population: it pools every resolved PR regardless of author. Maintainer-authored PRs merge by human judgment regardless of severity (the same population settings-preview.ts's includeMaintainerAuthors already excludes from the public surface by default) -- so a repo where the maintainer is heavily active can have its contributor-only, correctly-discriminating signal swamped by a maintainer-PR pool that merges at a high rate independent of score, inverting the blended comparison without the score itself being wrong. Add an additive excludeMaintainerAuthors option (default off, every existing caller stays byte-identical) mirroring the miner/human cohorts gate-precision.ts already applies to the sibling #554 false-positive measurement, and opt the ops-anomaly detector's two call sites into it so the alert -- and the internal ops-stats row shown alongside it -- both reflect the contributor-only signal instead of the confounded one. --- src/review/ops-wire.ts | 10 +++- src/services/outcome-calibration.ts | 24 ++++++++-- test/unit/ops-wire.test.ts | 69 ++++++++++++++++++++++++++- test/unit/outcome-calibration.test.ts | 66 +++++++++++++++++++++++-- 4 files changed, 160 insertions(+), 9 deletions(-) diff --git a/src/review/ops-wire.ts b/src/review/ops-wire.ts index 40ab66331c..4afd0d865a 100644 --- a/src/review/ops-wire.ts +++ b/src/review/ops-wire.ts @@ -215,7 +215,11 @@ export async function runOpsAlerts(env: Env): Promise> try { const [gatePrecision, calibration, reviewBurst, reviewFailureBurst] = await Promise.all([ loadGatePrecisionReport(env, repoFullName), - buildRepoOutcomeCalibration(env, repoFullName), + // #orb-anomaly-slop-false-positive: exclude maintainer-authored PRs from the slop half -- they merge + // by human judgment regardless of score, so pooling them with contributor PRs can invert the + // merge-rate-by-band comparison detectOutcomeAnomalies reads below, in any repo the maintainer is + // heavily active in, without the deterministic score itself being wrong. + buildRepoOutcomeCalibration(env, repoFullName, undefined, { excludeMaintainerAuthors: true }), findHottestReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), findHottestInconclusiveReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), ]); @@ -303,7 +307,9 @@ export async function computeOpsStats(env: Env): Promise { try { const [gatePrecision, calibration, reviewBurst, reviewFailureBurst, byokUsage] = await Promise.all([ loadGatePrecisionReport(env, repoFullName), - buildRepoOutcomeCalibration(env, repoFullName), + // #orb-anomaly-slop-false-positive: same maintainer exclusion as runOpsAlerts above, so this row's + // `slop`/`anomalies` fields stay consistent with each other and with the cron alert. + buildRepoOutcomeCalibration(env, repoFullName, undefined, { excludeMaintainerAuthors: true }), findHottestReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), findHottestInconclusiveReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), sumByokAiUsageForRepoSince(env, repoFullName, byokUsageSinceIso), diff --git a/src/services/outcome-calibration.ts b/src/services/outcome-calibration.ts index 580e85ca4a..11760cbfa8 100644 --- a/src/services/outcome-calibration.ts +++ b/src/services/outcome-calibration.ts @@ -34,6 +34,15 @@ export type RecommendationOutcomeCalibration = { total: number; positive: number export type RecommendationOutcomeCalibrationOptions = { /** Only maintainer-lane outcomes are authoritative enough for live self-tune policy changes. */ maintainerOnly?: boolean | undefined; + /** Exclude maintainer-authored PRs (author_association OWNER/MEMBER/COLLABORATOR) from the SLOP half of the + * report. Maintainer PRs merge by human judgment regardless of score (the same population + * settings-preview.ts's `includeMaintainerAuthors` already excludes from the public surface by default), so + * pooling them with contributor PRs can invert a merge-rate-by-band comparison in any repo the maintainer is + * heavily active in without the deterministic score itself being wrong. Off by default so every existing + * caller (dashboards, the MCP tool, the public API route) stays byte-identical; mirrors the miner/human + * `cohorts` split gate-precision.ts already applies to the sibling #554 false-positive measurement, for the + * same reason. */ + excludeMaintainerAuthors?: boolean | undefined; }; export type OutcomeCalibration = { @@ -57,12 +66,21 @@ function terminalOutcome(pr: PullRequestRecord): "merged" | "closed" | null { return null; } +// Same OWNER/MEMBER/COLLABORATOR classification settings-preview.ts's includeMaintainerAuthors check already +// uses to exclude this population from the public surface by default. +const MAINTAINER_AUTHOR_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); + +function isMaintainerAuthoredPr(pr: PullRequestRecord): boolean { + return pr.authorAssociation != null && MAINTAINER_AUTHOR_ASSOCIATIONS.has(pr.authorAssociation); +} + /** Per-slop-band merge/close calibration over the resolved PRs that carry a slop assessment. Pure. */ -export function buildSlopOutcomeCalibration(pullRequests: PullRequestRecord[]): SlopOutcomeCalibration { +export function buildSlopOutcomeCalibration(pullRequests: PullRequestRecord[], options: RecommendationOutcomeCalibrationOptions = {}): SlopOutcomeCalibration { + const scoped = options.excludeMaintainerAuthors ? pullRequests.filter((pr) => !isMaintainerAuthoredPr(pr)) : pullRequests; const counts = new Map(); let totalMerged = 0; let totalResolved = 0; - for (const pr of pullRequests) { + for (const pr of scoped) { if (typeof pr.slopRisk !== "number" || !pr.slopBand) continue; // never assessed const band = pr.slopBand as SlopBand; if (!SLOP_BAND_ORDER.includes(band)) continue; @@ -158,7 +176,7 @@ export async function buildRepoOutcomeCalibration( listPullRequests(env, repoFullName), listAgentRecommendationOutcomes(env, windowDays !== undefined ? { repoFullName, windowDays } : { repoFullName }), ]); - const slop = buildSlopOutcomeCalibration(pullRequests); + const slop = buildSlopOutcomeCalibration(pullRequests, options); const recommendations = buildRecommendationOutcomeCalibration(outcomes, repoFullName, options); return { repoFullName, generatedAt: nowIso(), windowDays: windowDays ?? null, slop, recommendations, signals: buildOutcomeCalibrationSignals(slop, recommendations) }; } diff --git a/test/unit/ops-wire.test.ts b/test/unit/ops-wire.test.ts index 53522d60ef..0cfda2cbba 100644 --- a/test/unit/ops-wire.test.ts +++ b/test/unit/ops-wire.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { createApp } from "../../src/api/routes"; -import { recordAiUsageEvent, recordAuditEvent, recordGateBlockOutcome, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { recordAiUsageEvent, recordAuditEvent, recordGateBlockOutcome, updatePullRequestSlopAssessment, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import { classifyAnomalySeverity, computeOpsStats, @@ -221,6 +221,27 @@ async function seedAgentConfiguredRepo(env: Env, fullName: string, installationI .run(); } +// Seed a slop-band population where contributor PRs alone discriminate correctly (clean 0.667 > high 0.167), +// but `maintainerHighCount` maintainer-authored "high" PRs that ALL merge anyway (real maintainer behavior -- +// merged by human judgment, not the score) are also present. With maintainerHighCount=10 the blended high-band +// rate (11/16=0.6875) edges just over the contributor-only clean rate (0.667), inverting the BLENDED signal +// without either population's own signal actually being inverted -- the #orb-anomaly-slop-false-positive confound. +async function seedSlopMaintainerConfound(env: Env, repoFullName: string, maintainerHighCount: number): Promise { + let number = 1; + for (let i = 0; i < 6; i += 1, number += 1) { + await upsertPullRequestFromGitHub(env, repoFullName, { number, title: `contributor clean ${number}`, state: "closed", user: { login: "contributor" }, author_association: "CONTRIBUTOR", merged_at: i < 4 ? "2026-06-01T00:00:00.000Z" : null } as never); + await updatePullRequestSlopAssessment(env, repoFullName, number, { slopRisk: 0, slopBand: "clean" }); + } + for (let i = 0; i < 6; i += 1, number += 1) { + await upsertPullRequestFromGitHub(env, repoFullName, { number, title: `contributor high ${number}`, state: "closed", user: { login: "contributor" }, author_association: "CONTRIBUTOR", merged_at: i < 1 ? "2026-06-01T00:00:00.000Z" : null } as never); + await updatePullRequestSlopAssessment(env, repoFullName, number, { slopRisk: 70, slopBand: "high" }); + } + for (let i = 0; i < maintainerHighCount; i += 1, number += 1) { + await upsertPullRequestFromGitHub(env, repoFullName, { number, title: `maintainer high ${number}`, state: "closed", user: { login: "owner" }, author_association: "OWNER", merged_at: "2026-06-01T00:00:00.000Z" } as never); + await updatePullRequestSlopAssessment(env, repoFullName, number, { slopRisk: 70, slopBand: "high" }); + } +} + // Seed a gate-block ledger anomaly: blocked PRs that later MERGED (false positives) over the min sample. async function seedGateFalsePositiveAnomaly(env: Env, repoFullName: string): Promise { for (let i = 1; i <= 6; i += 1) { @@ -276,6 +297,40 @@ describe("runOpsAlerts — cron path over gittensory's outcome data", () => { expect(errors.mock.calls.map((c) => String(c[0])).some((line) => line.includes("ops_anomaly\""))).toBe(false); }); + it("REGRESSION (#orb-anomaly-slop-false-positive): does NOT flag 'slop score NOT discriminating' when the inversion is purely a maintainer-PR confound", async () => { + const env = createTestEnv(); + await seedRegisteredRepo(env, "owner/repo"); + await seedSlopMaintainerConfound(env, "owner/repo", 10); // blended would invert (verified in outcome-calibration.test.ts); contributor-only signal does not + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + + const found = await runOpsAlerts(env); + + expect(found["owner/repo"]?.some((a) => /slop score NOT discriminating/.test(a))).not.toBe(true); + const logged = errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("ops_anomaly") && line.includes("owner/repo")); + expect(logged).toBeUndefined(); + }); + + it("REGRESSION (#orb-anomaly-slop-false-positive): still flags a GENUINE non-discriminating score among contributor PRs alone (not a blanket suppression)", async () => { + const env = createTestEnv(); + await seedRegisteredRepo(env, "owner/repo"); + // Contributor-only population where a higher band genuinely merges more -- no maintainer PRs involved at all. + for (let i = 1; i <= 6; i += 1) { + await upsertPullRequestFromGitHub(env, "owner/repo", { number: i, title: `contributor clean ${i}`, state: "closed", user: { login: "contributor" }, author_association: "CONTRIBUTOR", merged_at: i <= 1 ? "2026-06-01T00:00:00.000Z" : null } as never); + await updatePullRequestSlopAssessment(env, "owner/repo", i, { slopRisk: 0, slopBand: "clean" }); + } + for (let i = 7; i <= 12; i += 1) { + await upsertPullRequestFromGitHub(env, "owner/repo", { number: i, title: `contributor high ${i}`, state: "closed", user: { login: "contributor" }, author_association: "CONTRIBUTOR", merged_at: i <= 11 ? "2026-06-01T00:00:00.000Z" : null } as never); + await updatePullRequestSlopAssessment(env, "owner/repo", i, { slopRisk: 70, slopBand: "high" }); + } + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + + const found = await runOpsAlerts(env); + + expect(found["owner/repo"]?.some((a) => /slop score NOT discriminating/.test(a))).toBe(true); + const logged = errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("ops_anomaly") && line.includes("owner/repo")); + expect(logged).toBeDefined(); + }); + it("REGRESSION (#sweep-requires-installation): prefers the agent-configured repo and never scans an uninstalled registered repo when a configured one exists", async () => { const env = createTestEnv(); await seedAgentConfiguredRepo(env, "owner/configured", 9501); @@ -532,6 +587,18 @@ describe("computeOpsStats — cross-repo outcome aggregate", () => { const row = payload.repos.find((r) => r.repoFullName === "owner/repo"); expect(row?.byokUsage).toEqual({ calls: 0, inputTokens: 0, outputTokens: 0, totalTokens: 0, costUsd: 0 }); }); + + it("REGRESSION (#orb-anomaly-slop-false-positive): the row's slop.discriminates and anomalies stay consistent -- both exclude the maintainer-PR confound", async () => { + const env = createTestEnv(); + await seedRegisteredRepo(env, "owner/repo"); + await seedSlopMaintainerConfound(env, "owner/repo", 10); + + const payload = await computeOpsStats(env); + const row = payload.repos.find((r) => r.repoFullName === "owner/repo"); + expect(row?.slop.discriminates).toBe(true); // contributor-only signal, not the inverted blended one + expect(row?.slop.totalResolved).toBe(12); // only the 12 contributor PRs, not the 10 maintainer PRs too + expect(row?.anomalies.some((a) => /slop score NOT discriminating/.test(a))).toBe(false); + }); }); describe("GET /v1/internal/ops/stats — bearer-gated, flag-gated endpoint", () => { diff --git a/test/unit/outcome-calibration.test.ts b/test/unit/outcome-calibration.test.ts index 512f86643f..aa19e24fb1 100644 --- a/test/unit/outcome-calibration.test.ts +++ b/test/unit/outcome-calibration.test.ts @@ -13,7 +13,7 @@ import type { AgentActionRecord, AgentRecommendationOutcomeRecord, AgentRecommen import { createTestEnv } from "../helpers/d1"; // A resolved PR carrying a slop assessment. `merged` → has a merge timestamp; otherwise closed-unmerged. -function pr(band: SlopBand, merged: boolean, number: number): PullRequestRecord { +function pr(band: SlopBand, merged: boolean, number: number, authorAssociation?: string): PullRequestRecord { return { repoFullName: "owner/repo", number, @@ -24,12 +24,13 @@ function pr(band: SlopBand, merged: boolean, number: number): PullRequestRecord linkedIssues: [], slopRisk: band === "clean" ? 0 : band === "low" ? 10 : band === "elevated" ? 40 : 70, slopBand: band, + ...(authorAssociation ? { authorAssociation } : {}), }; } // n PRs in a band, `merged` of them merged (the rest closed-unmerged). -function band(b: SlopBand, n: number, merged: number, base: number): PullRequestRecord[] { - return Array.from({ length: n }, (_, i) => pr(b, i < merged, base + i)); +function band(b: SlopBand, n: number, merged: number, base: number, authorAssociation?: string): PullRequestRecord[] { + return Array.from({ length: n }, (_, i) => pr(b, i < merged, base + i, authorAssociation)); } describe("buildSlopOutcomeCalibration", () => { @@ -60,6 +61,50 @@ describe("buildSlopOutcomeCalibration", () => { const result = buildSlopOutcomeCalibration([open, unassessed, ...band("clean", 1, 1, 0)]); expect(result.totalResolved).toBe(1); // only the one assessed+resolved PR }); + + // REGRESSION (#orb-anomaly-slop-false-positive): the ops-anomaly detector flagged "slop score NOT + // discriminating" on real repos, but the score itself was correct -- a pool of maintainer-authored PRs + // (which merge by human judgment regardless of severity) inverted the blended merge-rate-by-band comparison. + describe("excludeMaintainerAuthors", () => { + it("is a no-op by default (existing callers stay byte-identical) -- a maintainer-PR pool still inverts the blended result", () => { + // Contributor PRs alone would discriminate correctly (clean 0.667 > high 0.167), but 10 maintainer + // "high" PRs that ALL merged anyway (real maintainer behavior: merged by human judgment, not the score) + // pull the blended high-band rate up to 11/16=0.6875, just over clean's 0.667 -- flipping the blended + // comparison to non-discriminating even though neither population's OWN signal is actually inverted. + const contributorClean = band("clean", 6, 4, 0, "CONTRIBUTOR"); + const contributorHigh = band("high", 6, 1, 100, "CONTRIBUTOR"); + const maintainerHigh = band("high", 10, 10, 200, "OWNER"); + const result = buildSlopOutcomeCalibration([...contributorClean, ...contributorHigh, ...maintainerHigh]); + expect(result.discriminates).toBe(false); // the blended confound this fix addresses + }); + + it("excludeMaintainerAuthors:true removes the maintainer-PR confound and reveals the true (discriminating) signal", () => { + const contributorClean = band("clean", 6, 4, 0, "CONTRIBUTOR"); + const contributorHigh = band("high", 6, 1, 100, "CONTRIBUTOR"); + const maintainerHigh = band("high", 10, 10, 200, "OWNER"); + const result = buildSlopOutcomeCalibration([...contributorClean, ...contributorHigh, ...maintainerHigh], { excludeMaintainerAuthors: true }); + expect(result.totalResolved).toBe(12); // only the 12 contributor PRs counted + expect(result.discriminates).toBe(true); + }); + + it("excludeMaintainerAuthors:true still reports a genuine non-discriminating score among contributor PRs (not a blanket suppression)", () => { + const contributorClean = band("clean", 6, 1, 0, "CONTRIBUTOR"); + const contributorHigh = band("high", 6, 5, 100, "CONTRIBUTOR"); + const result = buildSlopOutcomeCalibration([...contributorClean, ...contributorHigh], { excludeMaintainerAuthors: true }); + expect(result.discriminates).toBe(false); + }); + + it("treats MEMBER and COLLABORATOR the same as OWNER, and leaves FIRST_TIME_CONTRIBUTOR/NONE/unset untouched", () => { + const memberOnly = buildSlopOutcomeCalibration(band("high", 3, 3, 0, "MEMBER"), { excludeMaintainerAuthors: true }); + const collaboratorOnly = buildSlopOutcomeCalibration(band("high", 3, 3, 0, "COLLABORATOR"), { excludeMaintainerAuthors: true }); + const firstTimeOnly = buildSlopOutcomeCalibration(band("high", 3, 3, 0, "FIRST_TIME_CONTRIBUTOR"), { excludeMaintainerAuthors: true }); + const unsetOnly = buildSlopOutcomeCalibration(band("high", 3, 3, 0), { excludeMaintainerAuthors: true }); + expect(memberOnly.totalResolved).toBe(0); + expect(collaboratorOnly.totalResolved).toBe(0); + expect(firstTimeOnly.totalResolved).toBe(3); // not a maintainer association -- kept + expect(unsetOnly.totalResolved).toBe(3); // no association at all -- kept, never over-excluded + }); + }); }); describe("buildRecommendationOutcomeCalibration", () => { @@ -189,6 +234,21 @@ describe("buildRepoOutcomeCalibration (env loader)", () => { const report = await buildRepoOutcomeCalibration(env, "owner/repo", 365); expect(report.recommendations).toMatchObject({ total: 2, positive: 1, negative: 1, positiveRate: 0.5 }); }); + + it("REGRESSION (#orb-anomaly-slop-false-positive): excludeMaintainerAuthors threads through to the slop half of the loaded report", async () => { + const env = createTestEnv(); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 1, title: "contributor clean", state: "closed", user: { login: "alice" }, author_association: "CONTRIBUTOR", merged_at: "2026-06-01T00:00:00.000Z" }); + await updatePullRequestSlopAssessment(env, "owner/repo", 1, { slopRisk: 0, slopBand: "clean" }); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 2, title: "maintainer high, merged anyway", state: "closed", user: { login: "owner" }, author_association: "OWNER", merged_at: "2026-06-01T00:05:00.000Z" }); + await updatePullRequestSlopAssessment(env, "owner/repo", 2, { slopRisk: 70, slopBand: "high" }); + + const blended = await buildRepoOutcomeCalibration(env, "owner/repo"); + expect(blended.slop.totalResolved).toBe(2); + + const contributorOnly = await buildRepoOutcomeCalibration(env, "owner/repo", undefined, { excludeMaintainerAuthors: true }); + expect(contributorOnly.slop.totalResolved).toBe(1); + expect(contributorOnly.slop.bands.find((b) => b.band === "high")).toMatchObject({ sampleSize: 0 }); + }); }); function runRecord(id: string, actorLogin: string, createdAt: string): AgentRunRecord {