Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>`json_extract(${aiUsageEvents.metadataJson}, '$.pullNumber')`;
const [row] = await db
.select({ pullNumber: pullNumberExpr, count: sql<number>`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
Expand Down
33 changes: 25 additions & 8 deletions src/review/ops-wire.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -159,12 +174,13 @@ export async function runOpsAlerts(env: Env): Promise<Record<string, string[]>>
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
Expand Down Expand Up @@ -210,10 +226,11 @@ export async function computeOpsStats(env: Env): Promise<OpsStatsPayload> {
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,
Expand All @@ -228,7 +245,7 @@ export async function computeOpsStats(env: Env): Promise<OpsStatsPayload> {
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 */
Expand Down
68 changes: 67 additions & 1 deletion test/unit/ops-wire.test.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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");
Expand Down
Loading