diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 397c50f511..27ae191ff5 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2627,6 +2627,41 @@ export async function findHottestReviewTargetForRepo( return { targetKey: row.targetKey, count: row.count }; } +/** + * #review-burst-blind-spot: findHottestReviewTargetForRepo (above) only counts SUCCESSFUL publish events, so a + * repeat-failure retry storm (every attempt SIGKILLed / zero output, never reaching a publish) is invisible to + * it -- the exact incident shape c7073949 (#3747) fixed. Every AI review call's `ai_usage_events` row already + * carries a structured `inconclusive` boolean in its metadata_json (set at src/services/ai-review.ts's `record` + * call site) regardless of whether the review ever published, so this is a genuine companion signal, not a + * guess: the hottest PR by INCONCLUSIVE review-call count in the window, across whichever repo's calls those + * are. Deliberately does not touch `status` (always "ok" for a completed call, inconclusive or not) so the + * daily neuron-budget sum (which filters status='ok') is never affected by this query. + */ +export async function findHottestInconclusiveReviewTargetForRepo( + env: Env, + repoFullName: string, + sinceIso: string, +): Promise<{ targetKey: string; count: number } | null> { + const db = getDb(env.DB); + const pullNumberExpr = sql`json_extract(${aiUsageEvents.metadataJson}, '$.pullNumber')`; + const [row] = await db + .select({ pullNumber: pullNumberExpr, count: sql`count(*)` }) + .from(aiUsageEvents) + .where( + and( + eq(aiUsageEvents.feature, "ai_review_pr"), + gte(aiUsageEvents.createdAt, sinceIso), + sql`json_extract(${aiUsageEvents.metadataJson}, '$.repoFullName') = ${repoFullName}`, + sql`json_extract(${aiUsageEvents.metadataJson}, '$.inconclusive') = 1`, + ), + ) + .groupBy(pullNumberExpr) + .orderBy(desc(sql`count(*)`)) + .limit(1); + if (!row || row.pullNumber === null) return null; + return { targetKey: `${repoFullName}#${row.pullNumber}`, count: row.count }; +} + /** Moderation-rules engine (#selfhost-mod-engine): the actor's TOTAL violation count across every rule type in * `eventTypes` and EVERY repo this install tracks (no targetKey/route scoping -- `audit_events` carries no * repo/installation column at all, so this is inherently install-wide, mirroring the install-wide contributor diff --git a/src/review/ops-wire.ts b/src/review/ops-wire.ts index 2f8e35c7c6..f218fecee2 100644 --- a/src/review/ops-wire.ts +++ b/src/review/ops-wire.ts @@ -30,7 +30,7 @@ // `override_audit` D1 tables (none of which exist in gittensory's migrations yet) plus a careful soak/promote // design. This module is READ-ONLY observability: it reports drift; it never changes what blocks a live PR. -import { findHottestReviewTargetForRepo, listRepositories } from "../db/repositories"; +import { findHottestInconclusiveReviewTargetForRepo, findHottestReviewTargetForRepo, listRepositories } from "../db/repositories"; import { isAgentConfigured } from "../settings/autonomy"; import { resolveRepositorySettings } from "../settings/repository-settings"; import { loadGatePrecisionReport, type GatePrecisionReport } from "../services/gate-precision"; @@ -63,14 +63,21 @@ const REVIEW_BURST_THRESHOLD = 6; * to prevent from recurring. */ const REVIEW_BURST_WINDOW_HOURS = 2; -/** One repo's outcome reports + the repo it covers — the input to the pure anomaly detector. `reviewBurst` is - * optional so existing snapshot-fixture tests need not be touched; absent/null means "not computed", not - * "healthy" -- the caller (runOpsAlerts/computeOpsStats) always populates it today. */ +/** #review-burst-blind-spot: reviewBurst (above) only sees SUCCESSFUL publishes, so a repeat-failure retry + * storm (every attempt inconclusive, never publishing) sails under it indefinitely -- the exact incident + * c7073949 (#3747) fixed. Lower than REVIEW_BURST_THRESHOLD on purpose: a failure burst is inherently rarer + * and more anomalous than a publish burst (normal iteration never produces repeated INCONCLUSIVE calls). */ +const REVIEW_FAILURE_BURST_THRESHOLD = 3; + +/** One repo's outcome reports + the repo it covers — the input to the pure anomaly detector. `reviewBurst` and + * `reviewFailureBurst` are optional so existing snapshot-fixture tests need not be touched; absent/null means + * "not computed", not "healthy" -- the caller (runOpsAlerts/computeOpsStats) always populates both today. */ export interface RepoOutcomeSnapshot { repoFullName: string; gatePrecision: GatePrecisionReport; calibration: OutcomeCalibration; reviewBurst?: { targetKey: string; count: number } | null | undefined; + reviewFailureBurst?: { targetKey: string; count: number } | null | undefined; } /** @@ -120,6 +127,14 @@ export function detectOutcomeAnomalies(snapshot: RepoOutcomeSnapshot): string[] ); } + // REVIEW FAILURE BURST (#review-burst-blind-spot): the publish-burst check above cannot see a repeat-failure + // retry storm -- every attempt produced no usable output and never reached a publish. Catch that shape too. + if (snapshot.reviewFailureBurst && snapshot.reviewFailureBurst.count >= REVIEW_FAILURE_BURST_THRESHOLD) { + out.push( + `review failure burst: ${snapshot.reviewFailureBurst.targetKey} produced ${snapshot.reviewFailureBurst.count} inconclusive (zero-output) AI review calls in the last ${REVIEW_BURST_WINDOW_HOURS}h with no successful publish — likely a stuck-CI finalize loop or retry storm burning tokens for no result. Investigate why this PR's reviews keep failing.`, + ); + } + return out; } @@ -159,12 +174,13 @@ export async function runOpsAlerts(env: Env): Promise> const repos = await opsScanRepos(env); for (const repoFullName of repos) { try { - const [gatePrecision, calibration, reviewBurst] = await Promise.all([ + const [gatePrecision, calibration, reviewBurst, reviewFailureBurst] = await Promise.all([ loadGatePrecisionReport(env, repoFullName), buildRepoOutcomeCalibration(env, repoFullName), findHottestReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), + findHottestInconclusiveReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), ]); - const anomalies = detectOutcomeAnomalies({ repoFullName, gatePrecision, calibration, reviewBurst }); + const anomalies = detectOutcomeAnomalies({ repoFullName, gatePrecision, calibration, reviewBurst, reviewFailureBurst }); if (anomalies.length === 0) continue; found[repoFullName] = anomalies; // Structured log = gittensory's notify path (no Discord/operator webhook exists) AND the Sentry path @@ -210,10 +226,11 @@ export async function computeOpsStats(env: Env): Promise { const reviewBurstSinceIso = new Date(Date.now() - REVIEW_BURST_WINDOW_HOURS * 60 * 60 * 1000).toISOString(); for (const repoFullName of repos) { try { - const [gatePrecision, calibration, reviewBurst] = await Promise.all([ + const [gatePrecision, calibration, reviewBurst, reviewFailureBurst] = await Promise.all([ loadGatePrecisionReport(env, repoFullName), buildRepoOutcomeCalibration(env, repoFullName), findHottestReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), + findHottestInconclusiveReviewTargetForRepo(env, repoFullName, reviewBurstSinceIso), ]); rows.push({ repoFullName, @@ -228,7 +245,7 @@ export async function computeOpsStats(env: Env): Promise { discriminates: calibration.slop.discriminates, }, recommendations: calibration.recommendations, - anomalies: detectOutcomeAnomalies({ repoFullName, gatePrecision, calibration, reviewBurst }), + anomalies: detectOutcomeAnomalies({ repoFullName, gatePrecision, calibration, reviewBurst, reviewFailureBurst }), }); } catch { /* a per-repo failure must not blank the whole feed */ diff --git a/test/unit/ops-wire.test.ts b/test/unit/ops-wire.test.ts index 3f47b1f9eb..282574867e 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 { recordAuditEvent, recordGateBlockOutcome, upsertPullRequestFromGitHub } from "../../src/db/repositories"; +import { recordAiUsageEvent, recordAuditEvent, recordGateBlockOutcome, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import { computeOpsStats, detectOutcomeAnomalies, @@ -139,6 +139,22 @@ describe("detectOutcomeAnomalies — over gittensory's own outcome data", () => expect(detectOutcomeAnomalies(snap).some((a) => /review burst/.test(a))).toBe(false); }); + it("flags a review FAILURE burst — repeated inconclusive AI-review calls for the same PR with no successful publish (#review-burst-blind-spot)", () => { + const snap: RepoOutcomeSnapshot = { ...healthySnapshot, reviewFailureBurst: { targetKey: "owner/repo#42", count: 4 } }; + const out = detectOutcomeAnomalies(snap); + expect(out.some((a) => /review failure burst/.test(a) && /owner\/repo#42/.test(a) && /4 inconclusive/.test(a))).toBe(true); + }); + + it("does NOT flag a review failure burst below the threshold", () => { + const snap: RepoOutcomeSnapshot = { ...healthySnapshot, reviewFailureBurst: { targetKey: "owner/repo#42", count: 2 } }; + expect(detectOutcomeAnomalies(snap).some((a) => /review failure burst/.test(a))).toBe(false); + }); + + it("does NOT flag a review failure burst when none was computed (absent/null)", () => { + expect(detectOutcomeAnomalies({ ...healthySnapshot, reviewFailureBurst: null }).some((a) => /review failure burst/.test(a))).toBe(false); + expect(detectOutcomeAnomalies(healthySnapshot).some((a) => /review failure burst/.test(a))).toBe(false); // field omitted entirely + }); + it("does NOT flag a review burst when none was computed (absent/null)", () => { expect(detectOutcomeAnomalies({ ...healthySnapshot, reviewBurst: null }).some((a) => /review burst/.test(a))).toBe(false); expect(detectOutcomeAnomalies(healthySnapshot).some((a) => /review burst/.test(a))).toBe(false); // field omitted entirely @@ -228,6 +244,56 @@ describe("runOpsAlerts — cron path over gittensory's outcome data", () => { expect(row?.anomalies.some((a) => /review burst/.test(a))).toBe(true); }); + it("detects and reports a review FAILURE burst end-to-end -- reproduces the #3747 incident shape (repeated inconclusive calls, zero publishes) (#review-burst-blind-spot)", async () => { + const env = createTestEnv(); + await seedRegisteredRepo(env, "owner/repo"); + // Mirrors the exact incident: every AI review call for this PR came back inconclusive (zero usable output), + // and NONE of them ever reached a successful publish -- so findHottestReviewTargetForRepo alone sees nothing. + for (let i = 0; i < 4; i += 1) { + await recordAiUsageEvent(env, { + feature: "ai_review_pr", + model: "self-host:claude-code", + status: "ok", + estimatedNeurons: 100, + metadata: { repoFullName: "owner/repo", pullNumber: 99, inconclusive: true }, + }); + } + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + + const found = await runOpsAlerts(env); + + expect(found["owner/repo"]?.some((a) => /review failure burst/.test(a) && /owner\/repo#99/.test(a) && /4 inconclusive/.test(a))).toBe(true); + expect(found["owner/repo"]?.some((a) => /review burst:/.test(a))).toBe(false); // the publish-only signal stays silent -- proves this is a genuinely new detector, not a duplicate. + const stats = await computeOpsStats(env); + const row = stats.repos.find((r) => r.repoFullName === "owner/repo"); + expect(row?.anomalies.some((a) => /review failure burst/.test(a))).toBe(true); + }); + + it("does NOT flag a review failure burst from a healthy mix of successful and merely-occasional inconclusive calls", async () => { + const env = createTestEnv(); + await seedRegisteredRepo(env, "owner/repo"); + await recordAiUsageEvent(env, { + feature: "ai_review_pr", + model: "self-host:claude-code", + status: "ok", + estimatedNeurons: 100, + metadata: { repoFullName: "owner/repo", pullNumber: 5, inconclusive: false }, + }); + await recordAiUsageEvent(env, { + feature: "ai_review_pr", + model: "self-host:claude-code", + status: "ok", + estimatedNeurons: 100, + metadata: { repoFullName: "owner/repo", pullNumber: 5, inconclusive: true }, + }); + const errors = vi.spyOn(console, "error").mockImplementation(() => {}); + + const found = await runOpsAlerts(env); + + expect(found["owner/repo"]).toBeUndefined(); + expect(errors.mock.calls.map((c) => String(c[0])).some((line) => line.includes("ops_anomaly\""))).toBe(false); + }); + it("fails safe per-repo: a load error on one repo is logged and the scan continues (ops_anomaly_repo_error)", async () => { const env = createTestEnv(); await seedRegisteredRepo(env, "owner/repo");