diff --git a/src/review/alerts.ts b/src/review/alerts.ts index 1cee22214a..7801d710e5 100644 --- a/src/review/alerts.ts +++ b/src/review/alerts.ts @@ -1,6 +1,6 @@ // Anomaly alerting (reviewbot→loopover convergence — ADDITIVE, NATIVE port of reviewbot // src/core/alerts.ts). On each cron tick, snapshot agent health and push a THROTTLED Discord alert when -// something drifts — a manual-rate spike, stuck/failed targets, a DLQ spike, calibration drift, disputed +// something drifts — a manual-rate spike, a DLQ spike, calibration drift, disputed // closes, or a config invariant violation — so drift is HEARD ABOUT instead of polled for. // // SELF-CONTAINED: every type + helper this module needs is defined HERE. No imports from reviewbot. The @@ -36,16 +36,15 @@ export interface ReversedTarget { /** Per-agent health snapshot from review_targets + config invariants. Shared by /status and alerting. * (Ported shape from reviewbot src/core/ops.ts AgentHealth — every field load-bearing here is kept.) */ export interface AgentHealth { - byStatus: Record; - byVerdict: Record; + /** #9136: the gate's own decision breakdown (merge / close / hold), replacing the former byStatus + + * byVerdict pair that read the orphaned review_targets. */ + byDecision: Record; /** Count of terminal decisions (merged/closed/commented/manual/error) — the manualRate denominator. */ terminalCount: number; nonTerminal: number; /** Fraction of terminal decisions punted to a human. */ manualRate: number; - stuckRetryable: number; - /** Permanently-failed (dead-lettered or attempt-exhausted) reviews in the recent window. */ - failed: number; + /** Dead-letter-queue events (event_type='dead_lettered') in the recent window — reviews the queue gave * up on. A spike means a systemic problem (rate-limit storm, AI-quota exhaustion) dropped a batch. */ dlqCount: number; @@ -55,9 +54,8 @@ export interface AgentHealth { reversals: number; /** reversals / (merged + closed) — the ground-truth accuracy signal. 0 when nothing auto-acted. */ reversalRate: number; - /** The specific recent failed / reversed PRs, for an actionable alert (capped). Absent on hand-built + /** The specific recent reversed PRs, for an actionable alert (capped). Absent on hand-built * health objects (e.g. tests) — render defensively. */ - failedTargets?: FailedTarget[]; reversedTargets?: ReversedTarget[]; configIssues: string[]; /** Kill-switch state: true when this agent's autonomous writes are frozen. Optional for hand-built objects. */ @@ -142,9 +140,6 @@ function runChanges(result: unknown): number { const MANUAL_RATE_THRESHOLD = 0.6; const MIN_TERMINAL_FOR_RATE = 10; // don't cry "manual-rate spike" off a handful of decisions -const STUCK_THRESHOLD = 5; -// DLQ should be ~0 in normal operation — even a few dropped reviews is a systemic signal (rate-limit/AI-quota -// storm). Alert low so we hear about it within a cron tick, not days later via manual audit. const DLQ_ALERT_THRESHOLD = 3; const MAX_LISTED = 8; // keep the embed readable; note the remainder @@ -193,14 +188,13 @@ export function detectAnomalies(h: AgentHealth, calibration?: Calibration): stri const list = listSuffix(h.dlqTargets, (t) => `${prLink(t)}${t.lastError ? ` · ${t.lastError}` : ""}`); out.push(`⚠️ ${h.dlqCount} review(s) DEAD-LETTERED in the window — the queue gave up (likely a rate-limit / AI-quota storm). These were dropped and need a re-queue${list}`); } - if (h.failed > 0) { - const list = listSuffix(h.failedTargets, (t) => `${prLink(t)} (${t.verdict ?? "no verdict"}${t.lastError ? ` · ${t.lastError}` : ""})`); - out.push(`${h.failed} review(s) permanently failed — attempt-exhausted/dead-lettered${list}`); - } + // #9136: the "permanently failed" and "stuck in error_retryable" alerts are GONE, not repointed. Both read + // review_targets' processing-status enum, which the 2026-06-22 convergence cutover removed as a concept -- + // so both had been structurally unfireable since, while still reading in the code as live coverage. The + // failure they were meant to catch is exactly what the DLQ alert directly above catches, from a live source. if (h.terminalCount >= MIN_TERMINAL_FOR_RATE && h.manualRate >= MANUAL_RATE_THRESHOLD) { out.push(`manual-rate ${Math.round(h.manualRate * 100)}% over ${h.terminalCount} decisions`); } - if (h.stuckRetryable >= STUCK_THRESHOLD) out.push(`${h.stuckRetryable} target(s) stuck in error_retryable`); if (h.reversals > 0) { const list = listSuffix(h.reversedTargets, (t) => prLink(t)); out.push(`${h.reversals} auto-action(s) reverted/reopened by humans in the last 7d (reversal-rate ${Math.round(h.reversalRate * 100)}%)${list}`); diff --git a/src/review/ops.ts b/src/review/ops.ts index 90deb0efcb..c1ac5e9c85 100644 --- a/src/review/ops.ts +++ b/src/review/ops.ts @@ -40,20 +40,19 @@ export interface ReversedTarget { /** Per-agent health snapshot from review_targets + config invariants. Shared by /status and alerting. */ export interface AgentHealth { - byStatus: Record; - byVerdict: Record; + /** #9136: the gate's own decision breakdown (merge / close / hold) from review_audit's gate_decision rows. + * Replaces the former `byStatus` + `byVerdict` pair, which were near-duplicates of each other sourced from + * the orphaned review_targets and had been frozen since the 2026-06-22 cutover. */ + byDecision: Record; terminalCount: number; nonTerminal: number; manualRate: number; - stuckRetryable: number; - failed: number; dlqCount: number; dlqTargets?: FailedTarget[]; reversals: number; reversalRate: number; /** Merged + closed auto-actions in the 7d anomaly window — the reversalRate denominator. */ recentAutoActions: number; - failedTargets?: FailedTarget[]; reversedTargets?: ReversedTarget[]; configIssues: string[]; frozen?: boolean; @@ -162,25 +161,12 @@ function rowId(project: string, kind: TargetKind, repo: string, number: number): return `${project}:${kind}:${repo}#${number}`; } -/** The minimal review_targets row the decision endpoint reads (inlined from reviewbot src/core/db.ts). */ -interface DecisionTargetRow { - id: string; - repo: string; - number: number; - kind: string; - status: string; - verdict: string | null; - head_sha: string | null; - decided_sha: string | null; - attempt_count: number | null; - terminal_at: string | null; - decision_json: string | null; -} +// #9136: DecisionTargetRow and NON_TERMINAL are gone with the last review_targets reads. NON_TERMINAL listed +// the processing states (queued / reviewing / error_retryable) the convergence cutover removed as a concept; +// non-terminal is now simply the gate's `hold` count. // ── Thresholds (byte-faithful from reviewbot src/core/ops.ts) ──────────────────────────────────── -const NON_TERMINAL = new Set(["queued", "reviewing", "error_retryable"]); - /** How far back the anomaly signals (failed / reversals) look. */ const ANOMALY_WINDOW = "-7 days"; // DLQ spike = a RECENT burst of dead-letters whose targets HAVEN'T recovered. @@ -199,6 +185,19 @@ const DLQ_WINDOW = "-6 hours"; // still live. byStatus/verdictRows/failedRows below are UNCHANGED (still review_targets-sourced, and so still // silently zero) -- deferred; see the PR description for the full scope decision and #9136 for the tracked // remainder (manualRate/stuckRetryable/failed/calibration bins, submitter-reputation.ts, ams-miner-cohort.ts). +/** + * #9136: a PR accumulates one decision_records row per head sha, and its own header states + * "latest-finalize-wins per id". Every calibration read wants the decision that actually stands, so this + * restricts to the newest record for each (repo, pull) — the direct analogue of review_targets' single + * terminal row per target, and the same shape as submitter-reputation.ts's LATEST_PR_OUTCOME_FILTER. + * + * Correlated on the alias `dr`, so every query using it must name its decision_records table `dr`. + */ +export const LATEST_DECISION_RECORD_FILTER = `dr.created_at = ( + SELECT MAX(newer.created_at) FROM decision_records newer + WHERE newer.repo_full_name = dr.repo_full_name AND newer.pull_number = dr.pull_number + )`; + function parseReviewAuditTargetId(targetId: string): { repo: string; number: number } | null { const hashIndex = targetId.lastIndexOf("#"); if (hashIndex <= 0) return null; @@ -235,14 +234,28 @@ export async function computeAgentHealth(env: Env, config: OpsAgentConfig, deps: // a few); recent failed/reversal counts + the rate denominator are all 7-day-windowed. `manualRate` is // all-time on purpose (a different, lifetime signal). const LIST_CAP = 100; - const [statusRows, verdictRows, failedRows, reversedRows, recentActionsRow, dlqRows, dlqCountRow] = await Promise.all([ - storage(env).prepare(`SELECT status, COUNT(*) AS n FROM review_targets WHERE project = ? GROUP BY status`).bind(slug).all<{ status: string; n: number }>(), - storage(env).prepare(`SELECT verdict, COUNT(*) AS n FROM review_targets WHERE project = ? AND verdict IS NOT NULL GROUP BY verdict`).bind(slug).all<{ verdict: string; n: number }>(), + const [decisionRows, reversedRows, recentActionsRow, dlqRows, dlqCountRow] = await Promise.all([ + // #9136, the last three review_targets reads in this file. byStatus/byVerdict/failedRows were the only + // survivors of the convergence cutover's orphaning, and so returned a frozen set that has been silently + // shrinking since 2026-06-22 -- taking manualRate, terminalCount and two Discord alerts down with them. + // + // Resolved by splitting them on whether a live analogue EXISTS, rather than repointing all three at + // something approximate: + // + // byStatus/byVerdict -> byDecision, sourced from review_audit's own gate_decision rows. The disposition + // half of the old status vocabulary (merged/closed/manual) maps exactly onto merge/close/hold, and it + // is what terminalCount/nonTerminal/manualRate were really measuring. The two fields also collapse + // into one: separate status and verdict breakdowns were near-duplicates of each other. + // + // stuckRetryable / failed / failedTargets -> DELETED, not repointed. `queued/reviewing/error/ + // error_retryable` were PROCESSING states, and the cutover removed the concept, not just the table -- + // nothing live records them (this file's own comment below the fold said as much). `failed` was + // additionally redundant: dlqCount/dlqTargets already cover "permanently failed", are live, and have + // their own alert. An alert that cannot fire is worse than no alert, because it reads as coverage. storage(env).prepare( - `SELECT number, repo, verdict, last_error FROM review_targets - WHERE project = ? AND status = 'error' AND updated_at > datetime('now', ?) - ORDER BY updated_at DESC LIMIT ?`, - ).bind(slug, ANOMALY_WINDOW, LIST_CAP).all<{ number: number; repo: string; verdict: string | null; last_error: string | null }>(), + `SELECT decision, COUNT(*) AS n FROM review_audit + WHERE project = ? AND event_type = 'gate_decision' AND decision IS NOT NULL GROUP BY decision`, + ).bind(slug).all<{ decision: string; n: number }>(), // #9136: repointed off review_targets (see this file's own header comment above). Recent human reversals // of a bot auto-action, read directly from review_audit's own reversal_* rows -- repo/number parsed from // target_id, no join. A reopened bot-close the gate SUBSEQUENTLY re-acted on (a LATER gate_decision row @@ -282,14 +295,13 @@ export async function computeAgentHealth(env: Env, config: OpsAgentConfig, deps: `SELECT COUNT(*) AS n FROM review_audit WHERE project = ? AND event_type = 'dead_lettered' AND created_at > datetime('now', ?)`, ).bind(slug, DLQ_WINDOW).first<{ n: number }>(), ]); - const byStatus: Record = {}; - for (const r of statusRows.results ?? []) byStatus[r.status] = r.n; - const byVerdict: Record = {}; - for (const r of verdictRows.results ?? []) byVerdict[r.verdict] = r.n; - const terminalCount = (byStatus.merged ?? 0) + (byStatus.closed ?? 0) + (byStatus.commented ?? 0) + (byStatus.manual ?? 0) + (byStatus.error ?? 0); - const nonTerminal = Object.entries(byStatus).reduce((sum, [s, n]) => (NON_TERMINAL.has(s) ? sum + n : sum), 0); + const byDecision: Record = {}; + for (const r of decisionRows.results ?? []) byDecision[r.decision] = r.n; + // merge/close are the gate acting; hold is it deferring to a human. That is exactly the terminal / + // non-terminal split the old status vocabulary encoded, minus the processing states that no longer exist. + const terminalCount = (byDecision.merge ?? 0) + (byDecision.close ?? 0); + const nonTerminal = byDecision.hold ?? 0; const recentAutoActions = recentActionsRow?.n ?? 0; - const failedTargets: FailedTarget[] = (failedRows.results ?? []).map((r) => ({ number: r.number, repo: r.repo, verdict: r.verdict, lastError: r.last_error })); // #9136: repo/number parsed from review_audit's own target_id (no review_targets join -- see this file's // header comment). A target_id this malformed to parse is skipped (filtered out) rather than crashing the // whole snapshot over one bad row -- defensive only; every writer of this column (outcomes-wire.ts, @@ -312,20 +324,20 @@ export async function computeAgentHealth(env: Env, config: OpsAgentConfig, deps: }) .filter((t): t is FailedTarget => t !== null); const reversals = reversedTargets.length; + // The share of decisions the gate handed to a human instead of acting on. Denominator is ALL decisions + // (including holds), not just the terminal ones -- the old `manual / terminalCount` divided by a + // denominator that excluded the very rows it was counting. + const allDecisions = terminalCount + nonTerminal; return { - byStatus, - byVerdict, + byDecision, terminalCount, nonTerminal, - manualRate: terminalCount ? Number(((byStatus.manual ?? 0) / terminalCount).toFixed(3)) : 0, - stuckRetryable: byStatus.error_retryable ?? 0, - failed: failedTargets.length, + manualRate: allDecisions ? Number((nonTerminal / allDecisions).toFixed(3)) : 0, dlqCount: dlqCountRow?.n ?? dlqTargets.length, // true window count (uncapped); dlqTargets is the display sample dlqTargets, reversals, reversalRate: recentAutoActions ? Number((reversals / recentAutoActions).toFixed(3)) : 0, recentAutoActions, - failedTargets, reversedTargets, configIssues: deps.validateAgentConfig(config), frozen: await deps.isFrozen(env, slug), @@ -340,20 +352,52 @@ export async function computeAgentHealth(env: Env, config: OpsAgentConfig, deps: */ export async function computeCalibration(env: Env, config: OpsAgentConfig): Promise { const slug = config.slug; + // #9136: all four reads were sourced from review_targets, which the 2026-06-22 convergence cutover orphaned + // (no writer exists anywhere), so every one of them returned a frozen and shrinking set. Repointed onto the + // live ledgers: decision_records (#8836, the acted disposition + its reasonCode + the record JSON carrying + // aiConfidence) and review_audit (the realized reversal signal). + // + // This also retires the target_id NAMESPACE MISMATCH that made two of them unfixable even in principle. + // review_audit.target_id is `owner/repo#123`; review_targets.id was `project:kind:owner/repo#123`. The + // disputed-closes JOIN below could therefore never match a row, and — less visibly — the `reverted` Set a + // few lines down was built from review_audit target_ids and tested against review_targets ids, so + // `!reverted.has(...)` was ALWAYS true: every merge read as kept, `rev` was always empty, and + // recommendedFloor was structurally always null. decision_records keys on + // `repo_full_name || '#' || pull_number`, the SAME namespace as review_audit, so both now genuinely match. + // + // No `project` filter on decision_records: it has no such column because it is written only by this + // deployment (one instance, one project), unlike review_audit which carries the slug for historical export. const [mergedRows, revRows, closesByReasonRows, disputedRows] = await Promise.all([ - storage(env).prepare(`SELECT id, decision_json FROM review_targets WHERE project = ? AND status = 'merged'`).bind(slug).all<{ id: string; decision_json: string | null }>(), + // Every acted MERGE and the confidence that justified it. Latest record per PR wins, mirroring + // decision_records' own "latest-finalize-wins per id" semantics across a PR's successive head shas. + storage(env).prepare( + `SELECT dr.repo_full_name || '#' || dr.pull_number AS target_id, dr.record_json AS decision_json + FROM decision_records dr + WHERE dr.action = 'merge' AND ${LATEST_DECISION_RECORD_FILTER}`, + ).all<{ target_id: string; decision_json: string | null }>(), storage(env).prepare(`SELECT DISTINCT target_id FROM review_audit WHERE project = ? AND event_type = 'reversal_reverted'`).bind(slug).all<{ target_id: string }>(), // Close distribution by reasonCode — the denominator for spotting an over-closing gate. storage(env).prepare( - `SELECT COALESCE(json_extract(decision_json, '$.reasonCode'), '(none)') AS rc, COUNT(*) AS n - FROM review_targets WHERE project = ? AND status = 'closed' GROUP BY rc`, - ).bind(slug).all<{ rc: string; n: number }>(), + `SELECT dr.reason_code AS rc, COUNT(*) AS n + FROM decision_records dr + WHERE dr.action = 'close' AND ${LATEST_DECISION_RECORD_FILTER} + GROUP BY rc`, + ).all<{ rc: string; n: number }>(), // Disputed closes: a bot-close a human REOPENED that the gate did NOT subsequently re-terminalize. + // "Re-terminalized" is now expressed against the live ledger as "a later decision record exists for the + // same target", which is what review_targets.terminal_at > a.created_at meant. storage(env).prepare( - `SELECT COALESCE(json_extract(t.decision_json, '$.reasonCode'), '(none)') AS rc, COUNT(DISTINCT t.id) AS n - FROM review_audit a JOIN review_targets t ON t.id = a.target_id - WHERE a.project = ? AND a.event_type = 'reversal_reopened' - AND NOT (t.terminal_at IS NOT NULL AND t.terminal_at > a.created_at) GROUP BY rc`, + `SELECT dr.reason_code AS rc, COUNT(DISTINCT dr.repo_full_name || '#' || dr.pull_number) AS n + FROM review_audit a + JOIN decision_records dr ON dr.repo_full_name || '#' || dr.pull_number = a.target_id + WHERE a.project = ? AND a.event_type = 'reversal_reopened' AND dr.action = 'close' + AND ${LATEST_DECISION_RECORD_FILTER} + AND NOT EXISTS ( + SELECT 1 FROM decision_records later + WHERE later.repo_full_name = dr.repo_full_name AND later.pull_number = dr.pull_number + AND later.created_at > a.created_at + ) + GROUP BY rc`, ).bind(slug).all<{ rc: string; n: number }>(), ]); const disputedByReason = new Map((disputedRows.results ?? []).map((r) => [r.rc, r.n])); @@ -361,11 +405,18 @@ export async function computeCalibration(env: Env, config: OpsAgentConfig): Prom .map((r) => ({ reasonCode: r.rc, closes: r.n, disputed: disputedByReason.get(r.rc) ?? 0 })) .sort((a, b) => b.closes - a.closes); const disputedCloseCount = [...disputedByReason.values()].reduce((a, b) => a + b, 0); + // Both sides are `owner/repo#123` now — see the namespace note on the queries above for why this + // membership test silently answered "kept" for every single merge before. const reverted = new Set((revRows.results ?? []).map((r) => r.target_id)); + // #9136: review_targets.decision_json spelled this `confidence`; decision_records' canonical record spells + // it `aiConfidence` (see risk-control-wire.ts, which reads the same field off the same JSON). Both are + // accepted — reading only the new name would silently yield null for every row and leave the calibration + // exactly as dead as the orphaned table left it, which is the failure mode this repoint exists to end. const confidenceOf = (j: string | null): number | null => { if (!j) return null; try { - const c = (JSON.parse(j) as { confidence?: unknown }).confidence; + const record = JSON.parse(j) as { aiConfidence?: unknown; confidence?: unknown }; + const c = typeof record.aiConfidence === "number" ? record.aiConfidence : record.confidence; return typeof c === "number" ? c : null; } catch { return null; @@ -377,7 +428,7 @@ export async function computeCalibration(env: Env, config: OpsAgentConfig): Prom for (const r of mergedRows.results ?? []) { const c = confidenceOf(r.decision_json); if (c == null) continue; - const isKept = !reverted.has(r.id); + const isKept = !reverted.has(r.target_id); binSamples.push({ confidence: c, kept: isKept }); (isKept ? kept : rev).push(c); } @@ -449,13 +500,11 @@ export async function handleInternalStatus( return Response.json({ project: slug, - counts: { byStatus: health.byStatus, byVerdict: health.byVerdict }, + counts: { byDecision: health.byDecision }, health: { frozen: health.frozen ?? false, holdOnly: health.holdOnly ?? false, nonTerminal: health.nonTerminal, - stuckRetryable: health.stuckRetryable, - failed: health.failed, dlqCount: health.dlqCount, aiErrors, manualRate: health.manualRate, @@ -484,40 +533,62 @@ export async function handleInternalDecision(request: Request, env: Env, config: return Response.json({ error: "provide ?repo=&number=" }, { status: 400 }); } + // #9136: this endpoint read review_targets, which the convergence cutover orphaned — so it returned 404 for + // EVERY pull request opened since 2026-06-22, silently, while still being routed and authenticated. Rebuilt + // on the live ledgers: pull_requests for the target's realized state, decision_records for the decision that + // explains it, review_audit for the trail. + // + // The audit lookup is also a namespace fix: it bound `rowId(...)` (`project:kind:owner/repo#n`) against + // review_audit.target_id (`owner/repo#n`), so the trail came back empty even when rows existed. const id = rowId(config.slug, kind, repo, number); - const target = await storage(env).prepare(`SELECT * FROM review_targets WHERE id = ?`).bind(id).first(); + const targetKey = `${repo}#${number}`; + const [target, record, audit] = await Promise.all([ + storage(env).prepare( + `SELECT repo_full_name, number, state, head_sha, merged_at, merge_attempt_count + FROM pull_requests WHERE repo_full_name = ? AND number = ?`, + ).bind(repo, number).first<{ repo_full_name: string; number: number; state: string; head_sha: string | null; merged_at: string | null; merge_attempt_count: number | null }>(), + // The decision that stands for this PR — the newest record across its successive head shas. + storage(env).prepare( + `SELECT head_sha, action, reason_code, record_json, created_at + FROM decision_records WHERE repo_full_name = ? AND pull_number = ? ORDER BY created_at DESC LIMIT 1`, + ).bind(repo, number).first<{ head_sha: string; action: string; reason_code: string; record_json: string | null; created_at: string }>(), + storage(env).prepare( + `SELECT event_type, decision, substr(summary, 1, 240) AS summary, created_at + FROM review_audit WHERE project = ? AND target_id = ? ORDER BY created_at DESC LIMIT 25`, + ).bind(config.slug, targetKey).all<{ event_type: string; decision: string | null; summary: string | null; created_at: string }>(), + ]); if (!target) return Response.json({ error: "no such target", id }, { status: 404 }); let decision: unknown = null; - if (target.decision_json) { + if (record?.record_json) { try { - decision = JSON.parse(target.decision_json); + decision = JSON.parse(record.record_json); } catch { decision = null; } } - const audit = await storage(env).prepare( - `SELECT event_type, decision, substr(summary, 1, 240) AS summary, created_at - FROM review_audit WHERE project = ? AND target_id = ? ORDER BY created_at DESC LIMIT 25`, - ) - .bind(config.slug, id) - .all<{ event_type: string; decision: string | null; summary: string | null; created_at: string }>(); return Response.json({ project: config.slug, target: { id, - repo: target.repo, + repo: target.repo_full_name, number: target.number, - kind: target.kind, - status: target.status, - verdict: target.verdict ?? null, + kind, + // review_targets' status enum (queued/reviewing/error/error_retryable/merged/closed/manual) has no live + // equivalent — the cutover kept the realized DISPOSITION and dropped the processing states. Reported as + // the disposition, which is the half every consumer of this endpoint actually reads. + status: target.merged_at ? "merged" : target.state === "closed" ? "closed" : "open", + verdict: record?.action ?? null, headSha: target.head_sha ?? null, - decidedSha: target.decided_sha ?? null, - attemptCount: target.attempt_count ?? 0, - terminalAt: target.terminal_at, + // The sha the standing decision was actually made on. review_targets' `decided_sha` was a cache of the + // same idea; this is the authoritative version rather than a reconstruction of it. + decidedSha: record?.head_sha ?? null, + attemptCount: target.merge_attempt_count ?? 0, + terminalAt: target.merged_at ?? (record && record.action !== "hold" ? record.created_at : null), + reasonCode: record?.reason_code ?? null, }, - decision, // the cached terminal GateDecision for decidedSha (null if none cached yet) + decision, // the canonical DecisionRecord that explains the standing verdict (null if none recorded yet) audit: (audit.results ?? []).map((r) => ({ event: r.event_type, decision: r.decision, summary: r.summary, at: r.created_at })), }); } @@ -564,8 +635,17 @@ export interface ReviewSourceFreshnessCheck { * again, exactly the shape review_targets' own orphaning took. */ const REVIEW_SOURCE_FRESHNESS_SOURCES: ReadonlyArray<{ table: string; timestampColumn: string; windowDays: number }> = [ - { table: "review_targets", timestampColumn: "terminal_at", windowDays: 90 }, + // #9136: review_targets is GONE from this list, with the last reader that consumed it. The table is + // permanently frozen (the 2026-06-22 convergence cutover left it with no writer), so a 90-day freshness + // probe against it reported `fresh: false` forever — a gauge pinned at 0 that no action could ever clear, + // which is alert noise rather than a signal. A staleness check only earns its place while something still + // reads the table. + // + // decision_records takes its slot: it is what the calibration and decision surfaces read now, so IT is the + // source whose silence would mean those surfaces have quietly gone dark. 7 days matches review_audit — both + // are written on every gate decision, so a week of silence on either is a real outage, not a quiet period. { table: "review_audit", timestampColumn: "created_at", windowDays: 7 }, + { table: "decision_records", timestampColumn: "created_at", windowDays: 7 }, ]; /** Check every tracked source table for a row inside its own consuming window. Read-only; a per-table diff --git a/src/review/stats.ts b/src/review/stats.ts index eefec76ae2..3bfe23331e 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -21,6 +21,7 @@ // ledger (same `github_app.pr_public_surface_published` rows + `reviewEffortMinutes` metadata public-stats.ts uses). // Bearer-gated here only — never folded into the public homepage counter. import { bandFromMinutes } from "./review-effort"; +import { LATEST_DECISION_RECORD_FILTER } from "./ops"; // ── Inlined report types (ported shapes from reviewbot src/core/{eval,tuning}.ts) ──────────────── @@ -388,7 +389,15 @@ export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggre }; } -/** Aggregate the decision ledger for the dashboard. Pure-ish (reads D1 only); no GitHub I/O. */ +/** + * Aggregate the decision ledger for the dashboard. Pure-ish (reads D1 only); no GitHub I/O. + * + * #9136 kept this rather than deleting it with the rest of the review_targets readers. It is not + * wired to a route today -- only a comment in src/api/routes.ts references handleStats -- but it is + * an exported, bearer-gated feed, and silently retiring a capability is a different change from + * retiring an orphaned table. Its one review_targets read is repointed below, exactly like every + * other reader in this PR. + */ export async function computeStats( env: Env, opts: { days: number; bucket: string; nowMs: number }, @@ -401,14 +410,32 @@ export async function computeStats( // `bucket` is now always a present BUCKET_SQL key (day/week/month), so `BUCKET_SQL[bucket]` is never // undefined; the `?? BUCKET_SQL.day` only satisfies noUncheckedIndexedAccess and is unreachable. /* v8 ignore next */ - const bucketExpr = BUCKET_SQL[bucket] ?? BUCKET_SQL.day; + const bucketExpr = BUCKET_SQL[bucket] ?? BUCKET_SQL.day!; + // decision_records is aliased `dr` in the decision query below, and `created_at` is ambiguous there + // because LATEST_DECISION_RECORD_FILTER's correlated subquery also names one. + const decisionBucketExpr = bucketExpr.replace(/created_at/g, "dr.created_at"); const fromIso = new Date(opts.nowMs - days * 86_400_000).toISOString().slice(0, 10); // YYYY-MM-DD const [decisionRows, reversalRows, effortRows, cycleTime, findingAcceptance] = await Promise.all([ + // #9136: repointed off the orphaned review_targets, the same way ops.ts's four reads were. Two + // shape changes fall out of the move and both are visible in the payload: + // + // - decision_records has no `project` column -- it is written only by this app for its own repos -- + // so the series is keyed by repo_full_name. The dashboard pivots on whatever `project` holds, so + // it keeps working; the label is now the repo rather than a project slug. + // - `COALESCE(verdict, status)` becomes `action` -- merge | close | hold, the acted disposition. + // review_targets' verdict/status pair has no analogue here, and `action` is what ops.ts reads + // for the same purpose. (Not `decision`: decision_records has no such column, which the + // real-D1 test in test/unit/stats.test.ts catches rather than the stubbed ones.) + // + // LATEST_DECISION_RECORD_FILTER for the same reason ops.ts needs it: a PR accumulates one row per + // head sha, and counting all of them would inflate every bucket by the number of times a PR was + // re-reviewed. storage(env).prepare( - `SELECT ${bucketExpr} AS bucket, project, COALESCE(verdict, status) AS verdict, COUNT(*) AS n - FROM review_targets - WHERE created_at >= ? + `SELECT ${decisionBucketExpr} AS bucket, + dr.repo_full_name AS project, dr.action AS verdict, COUNT(*) AS n + FROM decision_records dr + WHERE dr.created_at >= ? AND ${LATEST_DECISION_RECORD_FILTER} GROUP BY bucket, project, verdict ORDER BY bucket ASC`, ).bind(fromIso).all<{ bucket: string; project: string; verdict: string; n: number }>(), diff --git a/test/unit/alerts.test.ts b/test/unit/alerts.test.ts index 0cfaac0416..b49fe8ddeb 100644 --- a/test/unit/alerts.test.ts +++ b/test/unit/alerts.test.ts @@ -10,13 +10,10 @@ import { import { createTestEnv } from "../helpers/d1"; const healthy: AgentHealth = { - byStatus: {}, - byVerdict: {}, + byDecision: {}, terminalCount: 50, nonTerminal: 0, manualRate: 0.1, - stuckRetryable: 0, - failed: 0, dlqCount: 0, reversals: 0, reversalRate: 0, @@ -27,11 +24,23 @@ describe("detectAnomalies", () => { it("returns nothing for a healthy snapshot", () => { expect(detectAnomalies(healthy)).toEqual([]); }); - it("flags config issues, failures, manual-rate spikes, and stuck targets", () => { + it("flags config issues and manual-rate spikes", () => { expect(detectAnomalies({ ...healthy, configIssues: ["bad slug"] })[0]).toMatch(/config invariant/); - expect(detectAnomalies({ ...healthy, failed: 2 })[0]).toMatch(/permanently failed/); expect(detectAnomalies({ ...healthy, manualRate: 0.8 })[0]).toMatch(/manual-rate 80%/); - expect(detectAnomalies({ ...healthy, stuckRetryable: 7 })[0]).toMatch(/stuck in error_retryable/); + }); + + // #9136: the "permanently failed" and "stuck in error_retryable" alerts are gone, not merely quiet. Both + // read review_targets' processing-status enum, which the convergence cutover removed as a CONCEPT, so both + // had been structurally unfireable since 2026-06-22 while still reading as live coverage. The real failure + // they were meant to catch is what the DLQ alert catches, from a source that is actually written. + it("REGRESSION: no alert claims to watch processing states that no longer exist", () => { + const noisy = { ...healthy, configIssues: ["bad slug"], manualRate: 0.9, dlqCount: 9, reversals: 4, reversalRate: 0.5 }; + const all = detectAnomalies(noisy).join("\n"); + expect(all).not.toMatch(/permanently failed/); + expect(all).not.toMatch(/error_retryable/); + // ...while the live signals it replaced them with still fire. + expect(all).toMatch(/DEAD-LETTERED/); + expect(all).toMatch(/manual-rate/); }); it("does NOT flag a high manual-rate on too few decisions", () => { expect(detectAnomalies({ ...healthy, terminalCount: 4, manualRate: 1 })).toEqual([]); @@ -81,31 +90,31 @@ describe("detectAnomalies", () => { expect(out.some((a) => /reverted\/reopened/.test(a) && /reversal-rate 2%/.test(a))).toBe(true); }); it("surfaces MULTIPLE simultaneous anomalies together (so a compound regression isn't masked)", () => { - const out = detectAnomalies({ ...healthy, failed: 1, manualRate: 0.9, reversals: 3, reversalRate: 0.1 }); + const out = detectAnomalies({ ...healthy, dlqCount: 5, manualRate: 0.9, reversals: 3, reversalRate: 0.1 }); expect(out.length).toBe(3); }); it("NAMES the specific PRs (with links) so the alert is actionable, not a mystery count", () => { const out = detectAnomalies({ ...healthy, - failed: 2, - failedTargets: [ - { number: 2420, repo: "JSONbored/awesome-claude", verdict: "merge", lastError: "max_attempts_exceeded" }, + dlqCount: 3, + dlqTargets: [ + { number: 2420, repo: "JSONbored/awesome-claude", verdict: null, lastError: "max_attempts_exceeded" }, { number: 2318, repo: "JSONbored/awesome-claude", verdict: null, lastError: "max_attempts_exceeded" }, ], reversals: 1, reversalRate: 0.01, reversedTargets: [{ number: 2643, repo: "JSONbored/awesome-claude", status: "merged", eventType: "reversal_reopened" }], }); - const failedLine = out.find((a) => /permanently failed/.test(a)) ?? ""; - expect(failedLine).toContain("[#2420](https://github.com/JSONbored/awesome-claude/pull/2420)"); - expect(failedLine).toContain("merge · max_attempts_exceeded"); - expect(failedLine).toContain("#2318"); + const dlqLine = out.find((a) => /DEAD-LETTERED/.test(a)) ?? ""; + expect(dlqLine).toContain("[#2420](https://github.com/JSONbored/awesome-claude/pull/2420)"); + expect(dlqLine).toContain("max_attempts_exceeded"); + expect(dlqLine).toContain("#2318"); const reversalLine = out.find((a) => /reverted\/reopened/.test(a)) ?? ""; expect(reversalLine).toContain("[#2643](https://github.com/JSONbored/awesome-claude/pull/2643)"); }); it("caps the listed PRs and notes the remainder (keeps the embed readable)", () => { - const many = Array.from({ length: 12 }, (_, i) => ({ number: 3000 + i, repo: "o/r", verdict: "close", lastError: "x" })); - const line = detectAnomalies({ ...healthy, failed: 12, failedTargets: many }).find((a) => /permanently failed/.test(a)) ?? ""; + const many = Array.from({ length: 12 }, (_, i) => ({ number: 3000 + i, repo: "o/r", verdict: null, lastError: "x" })); + const line = detectAnomalies({ ...healthy, dlqCount: 12, dlqTargets: many }).find((a) => /DEAD-LETTERED/.test(a)) ?? ""; expect(line).toContain("(+4 more)"); // 12 - MAX_LISTED(8) }); it("flags the accuracy circuit-breaker (holdOnly) FIRST and on its own", () => { @@ -128,16 +137,8 @@ describe("detectAnomalies", () => { expect(line).toContain("[#700](https://github.com/o/r/pull/700)"); expect(line).not.toContain("·"); // lastError null → no " · " suffix }); - it("omits the '· error' suffix on a permanently-failed PR with no lastError", () => { - const out = detectAnomalies({ - ...healthy, - failed: 1, - failedTargets: [{ number: 800, repo: "o/r", verdict: "merge", lastError: null }], - }); - const line = out.find((a) => /permanently failed/.test(a)) ?? ""; - expect(line).toContain("(merge)"); // verdict present, but no " · " - expect(line).not.toContain("·"); - }); + // (The '· error' suffix branch is covered by its DLQ twin directly above; #9136 removed the + // permanently-failed alert that was the second caller of the same listSuffix helper.) it("treats an undefined dlqCount as 0 (no DLQ line on a hand-built health object)", () => { // dlqCount is the noUncheckedIndexedAccess-style defensive `?? 0` — a hand-built health snapshot may omit it. const partial = { ...healthy } as Partial; @@ -203,7 +204,7 @@ function claimEnv(extra: Record = {}): Env { const WEBHOOK = "https://discord.com/api/webhooks/123/abc"; -const anomalousHealth: AgentHealth = { ...healthy, failed: 1, dlqCount: 3, dlqTargets: [{ number: 9, repo: "o/r", verdict: null, lastError: "storm" }] }; +const anomalousHealth: AgentHealth = { ...healthy, dlqCount: 3, dlqTargets: [{ number: 9, repo: "o/r", verdict: null, lastError: "storm" }] }; const driftCal: Calibration = { currentFloor: 0.9, mergedCount: 50, revertedCount: 2, keptAvgConfidence: 0.95, revertedMaxConfidence: 0.93, recommendedFloor: 0.95, note: "raise", diff --git a/test/unit/ops.test.ts b/test/unit/ops.test.ts index 966520c188..79f198460f 100644 --- a/test/unit/ops.test.ts +++ b/test/unit/ops.test.ts @@ -57,20 +57,32 @@ describe("defaultOpsHealthDeps.isFrozen — DB-backed global freeze (#audit-§5. // ── computeCalibration (ported from reviewbot test/calibration.test.ts) ────────────────────────── +/** + * #9136: reads are sourced from decision_records + review_audit now, not the orphaned review_targets. + * + * The old stub keyed on `FROM review_targets` and handed BOTH queries the same bare ids ("a"/"b"/"c"), which + * quietly made the two id namespaces agree — so these tests passed for as long as production was broken by + * exactly that disagreement. Ids here are the real `owner/repo#n` target keys both live tables actually use, + * so a namespace regression fails the test instead of hiding in it. + * + * Also note the calibration queries are a mix of bound (`.bind(slug).all()`) and unbound (`.all()`) calls — + * decision_records carries no `project` column — so the stub supports both shapes. + */ function calibrationEnv(merged: Array<{ id: string; confidence: number }>, revertedIds: string[]): Env { + const results = (sql: string): { results: unknown[] } => { + if (sql.includes("FROM decision_records") && sql.includes("'merge'")) { + return { results: merged.map((m) => ({ target_id: m.id, decision_json: JSON.stringify({ action: "merge", aiConfidence: m.confidence }) })) }; + } + if (sql.includes("FROM review_audit") && sql.includes("reversal_reverted")) { + return { results: revertedIds.map((target_id) => ({ target_id })) }; + } + return { results: [] }; // closes-by-reason and disputed-closes: not under test here + }; return { DB: { prepare(sql: string) { - return { - bind() { - return { - all: async () => - sql.includes("FROM review_targets") - ? { results: merged.map((m) => ({ id: m.id, decision_json: JSON.stringify({ verdict: "merge", confidence: m.confidence }) })) } - : { results: revertedIds.map((target_id) => ({ target_id })) }, - }; - }, - }; + const all = async () => results(sql); + return { all, bind: () => ({ all }) }; }, }, } as unknown as Env; @@ -80,7 +92,7 @@ const calConfig: OpsAgentConfig = { slug: "metagraphed", confidenceFloor: 0.9, s describe("computeCalibration", () => { it("recommends raising the floor above the highest-confidence reverted merge", async () => { - const env = calibrationEnv([{ id: "a", confidence: 0.95 }, { id: "b", confidence: 0.92 }, { id: "c", confidence: 0.99 }], ["b"]); + const env = calibrationEnv([{ id: "o/r#1", confidence: 0.95 }, { id: "o/r#2", confidence: 0.92 }, { id: "o/r#3", confidence: 0.99 }], ["o/r#2"]); const cal = await computeCalibration(env, calConfig); expect(cal.revertedCount).toBe(1); expect(cal.revertedMaxConfidence).toBe(0.92); @@ -94,20 +106,20 @@ describe("computeCalibration", () => { }); it("recommends no change when nothing was reverted", async () => { - const env = calibrationEnv([{ id: "a", confidence: 0.95 }], []); + const env = calibrationEnv([{ id: "o/r#1", confidence: 0.95 }], []); const cal = await computeCalibration(env, calConfig); expect(cal.recommendedFloor).toBeNull(); expect(cal.note).toMatch(/adequate/); }); it("recommends no change when the floor already sits above the reverted merges", async () => { - const env = calibrationEnv([{ id: "a", confidence: 0.85 }], ["a"]); // reverted at 0.85, floor 0.9 already higher + const env = calibrationEnv([{ id: "o/r#1", confidence: 0.85 }], ["o/r#1"]); // reverted at 0.85, floor 0.9 already higher const cal = await computeCalibration(env, calConfig); expect(cal.recommendedFloor).toBeNull(); }); it("treats a missing confidenceFloor as 0 (config.confidenceFloor ?? 0)", async () => { - const env = calibrationEnv([{ id: "a", confidence: 0.5 }], ["a"]); // reverted at 0.5 → suggest 0.52 > floor 0 + const env = calibrationEnv([{ id: "o/r#1", confidence: 0.5 }], ["o/r#1"]); // reverted at 0.5 → suggest 0.52 > floor 0 const cal = await computeCalibration(env, { slug: "x", secrets: {} }); // no confidenceFloor expect(cal.currentFloor).toBe(0); expect(cal.recommendedFloor).toBe(0.52); @@ -174,7 +186,12 @@ describe("handleInternalCalibration", () => { // ── handleInternalDecision (ported from reviewbot test/decision-endpoint.test.ts) ──────────────── -function decisionEnv(targetRow: Record | null): Env { +/** + * #9136: the endpoint reads pull_requests (realized state) + decision_records (the standing decision) + + * review_audit (the trail), not the orphaned review_targets. `prRow` null models "no such PR" — which, before + * this fix, was the answer for EVERY pull request opened since the 2026-06-22 cutover. + */ +function decisionEnv(prRow: Record | null, recordRow: Record | null = null, auditRows?: unknown[]): Env { return { INTERNAL_SECRET: "s3cret", DB: { @@ -182,8 +199,12 @@ function decisionEnv(targetRow: Record | null): Env { return { bind() { return { - first: async () => (sql.includes("SELECT * FROM review_targets") ? targetRow : null), - all: async () => ({ results: sql.includes("review_audit") ? [{ event_type: "reviewed", decision: "manual", summary: "needs human", created_at: "2026-06-13T00:00:00Z" }] : [] }), + first: async () => (sql.includes("FROM pull_requests") ? prRow : sql.includes("FROM decision_records") ? recordRow : null), + all: async () => ({ + results: sql.includes("review_audit") + ? (auditRows ?? [{ event_type: "reviewed", decision: "manual", summary: "needs human", created_at: "2026-06-13T00:00:00Z" }]) + : [], + }), }; }, }; @@ -192,6 +213,11 @@ function decisionEnv(targetRow: Record | null): Env { } as unknown as Env; } +/** A merged PR row in the live pull_requests shape. */ +function livePr(over: Record = {}): Record { + return { repo_full_name: "o/r", number: 5, state: "open", head_sha: "head1", merged_at: null, merge_attempt_count: 1, ...over }; +} + const decisionConfig: OpsAgentConfig = { slug: "metagraphed", secrets: { internalSecret: "INTERNAL_SECRET" } }; const auth = { authorization: "Bearer s3cret" }; const url = "https://x/metagraphed/internal/decision?repo=o/r&number=5"; @@ -228,27 +254,39 @@ describe("handleInternalDecision", () => { expect(r.status).toBe(404); }); - it("returns the cached decision + audit trail for an existing target", async () => { - const row = { - id: "metagraphed:pull_request:o/r#5", - project: "metagraphed", - kind: "pull_request", - repo: "o/r", - number: 5, - status: "manual", - attempt_count: 1, - terminal_at: null, - decided_sha: "abc", - decision_json: JSON.stringify({ verdict: "manual", summary: "ownership-sensitive", confidence: 0.4 }), - }; - const r = await handleInternalDecision(new Request(url, { headers: auth }), decisionEnv(row), decisionConfig); + it("REGRESSION (#9136): returns the standing decision + audit trail for a live PR, which used to 404", async () => { + // Every PR since the 2026-06-22 cutover hit the `!target` 404 above, because the only lookup was against + // a table nothing writes. The endpoint stayed routed and authenticated the whole time. + const record = { head_sha: "abc", action: "hold", reason_code: "ownership_sensitive", record_json: JSON.stringify({ action: "hold", aiConfidence: 0.4 }), created_at: "2026-06-13T00:00:00Z" }; + const r = await handleInternalDecision(new Request(url, { headers: auth }), decisionEnv(livePr(), record), decisionConfig); expect(r.status).toBe(200); - const body = (await r.json()) as { target: { status: string; attemptCount: number }; decision: { verdict: string }; audit: unknown[] }; - expect(body.target.status).toBe("manual"); + const body = (await r.json()) as { target: { status: string; attemptCount: number; verdict: string; decidedSha: string; reasonCode: string; terminalAt: string | null }; decision: { action: string }; audit: unknown[] }; + expect(body.target.status).toBe("open"); expect(body.target.attemptCount).toBe(1); - expect(body.decision.verdict).toBe("manual"); + expect(body.target.verdict).toBe("hold"); + expect(body.target.decidedSha).toBe("abc"); // the sha the standing decision was actually made on + expect(body.target.reasonCode).toBe("ownership_sensitive"); + expect(body.target.terminalAt).toBeNull(); // a hold is not terminal + expect(body.decision.action).toBe("hold"); expect(body.audit).toHaveLength(1); }); + + it("reports the realized disposition: merged wins over state, and a close is terminal at its decision", async () => { + const merged = await handleInternalDecision(new Request(url, { headers: auth }), decisionEnv(livePr({ state: "closed", merged_at: "2026-06-14T00:00:00Z" })), decisionConfig); + expect((await merged.json() as { target: { status: string; terminalAt: string } }).target).toMatchObject({ status: "merged", terminalAt: "2026-06-14T00:00:00Z" }); + + const closeRecord = { head_sha: "abc", action: "close", reason_code: "duplicate", record_json: null, created_at: "2026-06-15T00:00:00Z" }; + const closed = await handleInternalDecision(new Request(url, { headers: auth }), decisionEnv(livePr({ state: "closed" }), closeRecord), decisionConfig); + expect((await closed.json() as { target: { status: string; terminalAt: string } }).target).toMatchObject({ status: "closed", terminalAt: "2026-06-15T00:00:00Z" }); + }); + + it("a PR with no decision record yet reports nulls rather than failing", async () => { + const r = await handleInternalDecision(new Request(url, { headers: auth }), decisionEnv(livePr()), decisionConfig); + expect(r.status).toBe(200); + const body = (await r.json()) as { target: { verdict: null; decidedSha: null; reasonCode: null }; decision: null }; + expect(body.target).toMatchObject({ verdict: null, decidedSha: null, reasonCode: null }); + expect(body.decision).toBeNull(); + }); }); // ── computeAgentHealth + handleInternalStatus (native D1 + injected gate deps) ──────────────────── @@ -268,8 +306,8 @@ function healthEnv(): Env { return { n: 0 }; }, all: async () => { - if (sql.includes("GROUP BY status")) return { results: [{ status: "merged", n: 8 }, { status: "manual", n: 2 }, { status: "queued", n: 1 }] }; - if (sql.includes("GROUP BY verdict")) return { results: [{ verdict: "merge", n: 8 }, { verdict: "manual", n: 2 }] }; + // #9136: byStatus/byVerdict -> byDecision, from review_audit's own gate_decision rows. + if (sql.includes("GROUP BY decision")) return { results: [{ decision: "merge", n: 8 }, { decision: "close", n: 2 }, { decision: "hold", n: 1 }] }; // #9136: repo/number parsed from target_id (owner/repo#n), no review_targets join. if (sql.includes("reversal_reverted")) return { results: [{ target_id: "o/r#99", event_type: "reversal_reverted" }] }; if (sql.includes("event_type IN ('reviewed', 'shadow_reviewed')")) return { results: [{ target_id: "t1", decision: "merge", summary: "ok", created_at: "2026-06-13T00:00:00Z" }] }; @@ -288,10 +326,12 @@ const healthConfig: OpsAgentConfig = { slug: "loopover", confidenceFloor: 0.9, s describe("computeAgentHealth (native D1, default gate deps)", () => { it("computes terminal/manual-rate/reversals from the ledger; defaults to no config issues / unfrozen", async () => { const h = await computeAgentHealth(healthEnv(), healthConfig); - expect(h.byStatus.merged).toBe(8); - expect(h.nonTerminal).toBe(1); // queued - expect(h.terminalCount).toBe(10); // merged 8 + manual 2 - expect(h.manualRate).toBe(0.2); + expect(h.byDecision.merge).toBe(8); + expect(h.nonTerminal).toBe(1); // hold — the gate deferring to a human + expect(h.terminalCount).toBe(10); // merge 8 + close 2 — the gate acting + // #9136: hold / ALL decisions (11), not the old hold / terminalCount, whose denominator excluded the + // very rows it was counting. + expect(h.manualRate).toBe(0.091); expect(h.reversals).toBe(1); expect(h.reversalRate).toBe(0.5); // 1 reversal / 2 recent auto-actions expect(h.recentAutoActions).toBe(2); @@ -326,7 +366,7 @@ describe("handleInternalStatus", () => { }); expect(r.status).toBe(200); const body = (await r.json()) as { health: { manualRate: number; aiErrors: number }; recent: unknown[] }; - expect(body.health.manualRate).toBe(0.2); + expect(body.health.manualRate).toBe(0.091); // hold 1 / all 11 decisions (#9136) expect(body.health.aiErrors).toBe(4); expect(body.recent).toHaveLength(1); }); @@ -355,12 +395,12 @@ describe("handleInternalStatus", () => { } as unknown as Env; const r = await handleInternalStatus(new Request("https://x/s", { headers: auth }), emptyEnv, healthConfig); expect(r.status).toBe(200); - const body = (await r.json()) as { health: { aiErrors: number; manualRate: number; reversalRate: number; frozen: boolean; holdOnly: boolean }; counts: { byStatus: Record }; recent: unknown[] }; + const body = (await r.json()) as { health: { aiErrors: number; manualRate: number; reversalRate: number; frozen: boolean; holdOnly: boolean }; counts: { byDecision: Record }; recent: unknown[] }; expect(body.health.aiErrors).toBe(0); // defaultRecentAiErrorCount expect(body.health.manualRate).toBe(0); // terminalCount 0 → ternary false branch expect(body.health.reversalRate).toBe(0); // recentAutoActions 0 → ternary false branch expect(body.health.frozen).toBe(false); // health.frozen ?? false (undefined → false not exercised, but default deps give false) - expect(body.counts.byStatus).toEqual({}); + expect(body.counts.byDecision).toEqual({}); expect(body.recent).toEqual([]); }); }); @@ -428,38 +468,38 @@ describe("computeCalibration confidenceOf branches", () => { const env = { DB: { prepare(sql: string) { - return { - bind() { + const all = async () => { + if (sql.includes("FROM decision_records") && sql.includes("'merge'")) { return { - all: async () => { - if (sql.includes("status = 'merged'")) { - return { - results: [ - { id: "a", decision_json: null }, // confidenceOf → null (if !j) - { id: "b", decision_json: "{not json" }, // JSON.parse throws → catch returns null (line 271) - { id: "c", decision_json: JSON.stringify({ confidence: "high" }) }, // non-number → null - { id: "d", decision_json: JSON.stringify({ confidence: 0.8 }) }, // counted - ], - }; - } - return { results: [] }; - }, + results: [ + { target_id: "o/r#1", decision_json: null }, // confidenceOf → null (if !j) + { target_id: "o/r#2", decision_json: "{not json" }, // JSON.parse throws → catch returns null + { target_id: "o/r#3", decision_json: JSON.stringify({ aiConfidence: "high" }) }, // non-number → null + { target_id: "o/r#4", decision_json: JSON.stringify({ aiConfidence: 0.8 }) }, // counted + // #9136: the LEGACY spelling still parses, so records written before decision_records + // renamed the field are not silently dropped from the curve. + { target_id: "o/r#5", decision_json: JSON.stringify({ confidence: 0.8 }) }, // counted + ], }; - }, + } + return { results: [] }; }; + return { all, bind: () => ({ all }) }; }, }, } as unknown as Env; const cal = await computeCalibration(env, calConfig); - // only "d" had a numeric confidence and was kept (none reverted) + // only the two numeric-confidence rows (new and legacy spelling) count, both kept (none reverted) expect(cal.keptAvgConfidence).toBe(0.8); + expect(cal.mergedCount).toBe(5); // every merge row counts; only the confidence CURVE skips the unusable ones expect(cal.recommendedFloor).toBeNull(); expect(cal.note).toMatch(/adequate/); }); it("defaults closesByReason + disputedByReason to [] when those queries return no results", async () => { const env = { - DB: { prepare() { return { bind() { return { all: async () => ({}) }; } }; } }, // every query: undefined results + // every query: undefined results. Both call shapes, since decision_records reads are unbound. + DB: { prepare() { const all = async () => ({}); return { all, bind: () => ({ all }) }; } }, } as unknown as Env; const cal = await computeCalibration(env, calConfig); expect(cal.closesByReason).toEqual([]); @@ -472,16 +512,18 @@ describe("computeCalibration confidenceOf branches", () => { const env = { DB: { prepare(sql: string) { + const all = async () => { + // The disputed-closes query ALSO reads decision_records with action 'close' and groups by rc, so + // it must be matched FIRST or the closes-by-reason branch swallows it. + if (sql.includes("reversal_reopened")) return { results: [{ rc: "duplicate", n: 1 }] }; + if (sql.includes("FROM decision_records") && sql.includes("'close'") && sql.includes("GROUP BY rc")) { + return { results: [{ rc: "duplicate", n: 5 }, { rc: "conflict", n: 2 }] }; + } + return {}; // merged + reverted: undefined results → `?? []` fallback + }; return { - bind() { - return { - all: async () => { - if (sql.includes("status = 'closed' GROUP BY rc")) return { results: [{ rc: "duplicate", n: 5 }, { rc: "conflict", n: 2 }] }; - if (sql.includes("reversal_reopened")) return { results: [{ rc: "duplicate", n: 1 }] }; - return {}; // merged + reverted: undefined results → `?? []` fallback - }, - }; - }, + all, + bind: () => ({ all }), }; }, }, @@ -521,39 +563,32 @@ describe("handleInternalDecision decision_json parse + nullish target fields", ( }); it("defaults the audit list to empty when review_audit returns no results", async () => { - const row = { - id: "metagraphed:pull_request:o/r#5", - repo: "o/r", - number: 5, - kind: "pull_request", - status: "merged", - verdict: "merge", - head_sha: "abc", - decided_sha: "abc", - attempt_count: 2, - terminal_at: "2026-06-13T00:00:00Z", - decision_json: null, // skips the parse block entirely (if target.decision_json false branch) - }; const env = { INTERNAL_SECRET: "s3cret", DB: { prepare(sql: string) { - return { bind() { return { first: async () => (sql.includes("SELECT * FROM review_targets") ? row : null), all: async () => ({}) }; } }; + return { + bind() { + return { + first: async () => (sql.includes("FROM pull_requests") ? livePr({ state: "closed", merged_at: "2026-06-13T00:00:00Z", merge_attempt_count: 2 }) : null), + all: async () => ({}), // undefined results -> the `?? []` fallback + }; + }, + }; }, }, } as unknown as Env; const r = await handleInternalDecision(new Request(url, { headers: auth }), env, decisionConfig); expect(r.status).toBe(200); const body = (await r.json()) as { decision: unknown; audit: unknown[]; target: { terminalAt: unknown } }; - expect(body.decision).toBeNull(); + expect(body.decision).toBeNull(); // no decision record for this PR expect(body.audit).toEqual([]); expect(body.target.terminalAt).toBe("2026-06-13T00:00:00Z"); }); it("defaults kind to pull_request when ?kind is an unknown value", async () => { // exercises the `params.get("kind") === "issue" ? "issue" : "pull_request"` false branch with a non-issue value - const row = { id: "metagraphed:pull_request:o/r#5", repo: "o/r", number: 5, kind: "pull_request", status: "merged", verdict: "merge", head_sha: "a", decided_sha: "a", attempt_count: 1, terminal_at: null, decision_json: null }; - const r = await handleInternalDecision(new Request("https://x/d?repo=o/r&number=5&kind=bogus", { headers: auth }), decisionEnv(row), decisionConfig); + const r = await handleInternalDecision(new Request("https://x/d?repo=o/r&number=5&kind=bogus", { headers: auth }), decisionEnv(livePr()), decisionConfig); expect(r.status).toBe(200); const body = (await r.json()) as { target: { kind: string } }; expect(body.target.kind).toBe("pull_request"); @@ -580,13 +615,10 @@ describe("computeAgentHealth empty-ledger fallbacks", () => { }, } as unknown as Env; const h = await computeAgentHealth(emptyEnv, healthConfig); - expect(h.byStatus).toEqual({}); - expect(h.byVerdict).toEqual({}); + expect(h.byDecision).toEqual({}); expect(h.terminalCount).toBe(0); expect(h.nonTerminal).toBe(0); - expect(h.manualRate).toBe(0); // terminalCount 0 → ternary false branch - expect(h.stuckRetryable).toBe(0); // byStatus.error_retryable ?? 0 - expect(h.failed).toBe(0); + expect(h.manualRate).toBe(0); // no decisions at all → ternary false branch expect(h.dlqCount).toBe(0); // dlqCountRow?.n ?? dlqTargets.length (both fall through) expect(h.dlqTargets).toEqual([]); expect(h.reversals).toBe(0); @@ -594,7 +626,7 @@ describe("computeAgentHealth empty-ledger fallbacks", () => { expect(h.recentAutoActions).toBe(0); }); - it("computes manualRate with a present terminalCount but no manual rows (byStatus.manual ?? 0 fallback)", async () => { + it("computes manualRate with decisions but no holds (the `?? 0` fallback)", async () => { const env = { DB: { prepare(sql: string) { @@ -603,7 +635,7 @@ describe("computeAgentHealth empty-ledger fallbacks", () => { return { first: async () => ({}), all: async () => { - if (sql.includes("GROUP BY status")) return { results: [{ status: "merged", n: 4 }] }; // terminal but no `manual` + if (sql.includes("GROUP BY decision")) return { results: [{ decision: "merge", n: 4 }] }; // acted, never held return {}; }, }; @@ -614,31 +646,13 @@ describe("computeAgentHealth empty-ledger fallbacks", () => { } as unknown as Env; const h = await computeAgentHealth(env, healthConfig); expect(h.terminalCount).toBe(4); - expect(h.manualRate).toBe(0); // (byStatus.manual ?? 0) / 4 + expect(h.manualRate).toBe(0); // (byDecision.hold ?? 0) / 4 }); - it("maps recent failed (status='error') rows into failedTargets", async () => { - const env = { - DB: { - prepare(sql: string) { - return { - bind() { - return { - first: async () => ({ n: 0 }), - all: async () => { - if (sql.includes("status = 'error' AND updated_at")) return { results: [{ number: 42, repo: "o/r", verdict: null, last_error: "boom" }] }; - return {}; - }, - }; - }, - }; - }, - }, - } as unknown as Env; - const h = await computeAgentHealth(env, healthConfig); - expect(h.failed).toBe(1); - expect(h.failedTargets?.[0]).toEqual({ number: 42, repo: "o/r", verdict: null, lastError: "boom" }); - }); + // #9136: the "maps recent failed (status='error') rows into failedTargets" test is GONE with the signal it + // covered. `status='error'` was a review_targets processing state the convergence cutover removed as a + // concept, so that query could only ever return nothing; dlqTargets (tested directly below) is the live + // signal for the same operator question. it("uses dlqTargets.length as the dlqCount fallback when the COUNT row lacks n", async () => { const env = { @@ -676,15 +690,23 @@ describe("computeAgentHealth empty-ledger fallbacks", () => { // orphaning was for months. Real D1 (createTestEnv applies every migration), not a hand-rolled mock, so // the actual `datetime('now', ?)` window arithmetic is exercised for real. ───────────────────────────── describe("checkReviewSourceFreshness (#9136)", () => { - it("reads review_targets and review_audit as STALE when both are empty (the ground state right now)", async () => { + it("reads both live sources as STALE when both are empty", async () => { const env = createTestEnv(); const checks = await checkReviewSourceFreshness(env); expect(checks).toEqual([ - { table: "review_targets", windowDays: 90, fresh: false }, { table: "review_audit", windowDays: 7, fresh: false }, + { table: "decision_records", windowDays: 7, fresh: false }, ]); }); + it("REGRESSION: review_targets is no longer probed at all — a frozen table cannot be 'fresh' again", async () => { + // It was checked on a 90-day window against a table with no writer since 2026-06-22, so the gauge read 0 + // forever: an alert nothing could ever clear, which is noise rather than signal. A staleness probe only + // earns its place while something still READS the table, and nothing does now. + const env = createTestEnv(); + expect((await checkReviewSourceFreshness(env)).map((c) => c.table)).not.toContain("review_targets"); + }); + it("reads review_audit as FRESH when it has a row inside its 7-day window (the live, steady-state case)", async () => { const env = createTestEnv(); await env.DB.prepare( @@ -707,26 +729,20 @@ describe("checkReviewSourceFreshness (#9136)", () => { expect(checks.find((c) => c.table === "review_audit")).toEqual({ table: "review_audit", windowDays: 7, fresh: false }); }); - it("reads review_targets as FRESH when it has a row inside its 90-day window", async () => { - const env = createTestEnv(); - await env.DB.prepare( - `INSERT INTO review_targets (id, project, kind, repo, number, terminal_at) VALUES (?, ?, 'pull_request', ?, 1, datetime('now'))`, - ) - .bind("loopover:pull_request:o/r#1", "loopover", "o/r") - .run(); - const checks = await checkReviewSourceFreshness(env); - expect(checks.find((c) => c.table === "review_targets")).toEqual({ table: "review_targets", windowDays: 90, fresh: true }); - }); - - it("reads review_targets as STALE once its newest row falls outside the 90-day window (the 2026-09-20 cliff this exists to catch)", async () => { - const env = createTestEnv(); - await env.DB.prepare( - `INSERT INTO review_targets (id, project, kind, repo, number, terminal_at) VALUES (?, ?, 'pull_request', ?, 1, datetime('now', '-91 days'))`, - ) - .bind("loopover:pull_request:o/r#1", "loopover", "o/r") - .run(); - const checks = await checkReviewSourceFreshness(env); - expect(checks.find((c) => c.table === "review_targets")).toEqual({ table: "review_targets", windowDays: 90, fresh: false }); + it("reads decision_records as FRESH inside its window and STALE outside it — the source the calibration and decision surfaces now read", async () => { + const fresh = createTestEnv(); + await fresh.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, 1, 'sha', 'merge', 'gate_pass', 'd', '{}', datetime('now'))`, + ).bind("record:o/r#1@sha", "o/r").run(); + expect((await checkReviewSourceFreshness(fresh)).find((c) => c.table === "decision_records")).toEqual({ table: "decision_records", windowDays: 7, fresh: true }); + + const stale = createTestEnv(); + await stale.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, 1, 'sha', 'merge', 'gate_pass', 'd', '{}', datetime('now', '-8 days'))`, + ).bind("record:o/r#1@sha", "o/r").run(); + expect((await checkReviewSourceFreshness(stale)).find((c) => c.table === "decision_records")).toEqual({ table: "decision_records", windowDays: 7, fresh: false }); }); it("fails CLOSED (stale) on a read error, e.g. a dropped/missing table, rather than throwing", async () => { diff --git a/test/unit/routes-internal-decision-calibration.test.ts b/test/unit/routes-internal-decision-calibration.test.ts index 15672eb550..fcd821c60d 100644 --- a/test/unit/routes-internal-decision-calibration.test.ts +++ b/test/unit/routes-internal-decision-calibration.test.ts @@ -32,19 +32,30 @@ describe("GET /v1/internal/decision — operator decision-trail endpoint", () => it("200s with the decision trail for a seeded target, scoped to the app slug", async () => { const app = createApp(); const env = createTestEnv(); // GITHUB_APP_SLUG defaults to "loopover-orb" - // review_targets is raw-SQL-only (migration 0050) — seed the row the endpoint reads back. Its id is the - // project-namespaced natural key `${slug}:${kind}:${repo}#${number}` (rowId). + // #9136: seeds the LIVE ledgers the endpoint now reads — pull_requests for the realized state and + // decision_records for the standing decision. It previously seeded review_targets, which nothing has + // written since the 2026-06-22 cutover. await env.DB.prepare( - `INSERT INTO review_targets (id, project, kind, repo, number, status, verdict, head_sha, decided_sha, attempt_count, terminal_at, decision_json) - VALUES (?, ?, 'pull_request', ?, ?, 'merged', 'merge', 'abc123', 'abc123', 1, '2026-07-01T00:00:00Z', ?)`, + `INSERT INTO pull_requests (repo_full_name, number, title, state, head_sha, merged_at, merge_attempt_count) + VALUES (?, ?, 'seeded', 'closed', 'abc123', '2026-07-01T00:00:00Z', 1)`, ) - .bind("loopover-orb:pull_request:owner/repo#5", "loopover-orb", "owner/repo", 5, JSON.stringify({ action: "merge", confidence: 0.9 })) + .bind("owner/repo", 5) .run(); + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) + VALUES (?, ?, ?, 'abc123', 'merge', 'gate_pass', 'digest', ?, '2026-07-01T00:00:00Z')`, + ) + .bind("record:owner/repo#5@abc123", "owner/repo", 5, JSON.stringify({ action: "merge", aiConfidence: 0.9 })) + .run(); + // CRITICAL (#9136): review_audit.target_id is `owner/repo#5`, NOT the project-namespaced rowId. This test + // used to seed the rowId shape, which made the audit trail appear to work while production — which writes + // the bare `owner/repo#n` — got an empty trail every time. Seeding the real shape is what makes this a + // regression test rather than a restatement of the bug. await env.DB.prepare( `INSERT INTO review_audit (id, project, target_id, event_type, decision, summary, created_at) VALUES ('a1', 'loopover-orb', ?, 'reviewed', 'merge', 'looks good', '2026-07-01T00:00:00Z')`, ) - .bind("loopover-orb:pull_request:owner/repo#5") + .bind("owner/repo#5") .run(); const res = await app.request("/v1/internal/decision?repo=owner/repo&number=5", { headers: bearer(env) }, env); expect(res.status).toBe(200); @@ -52,7 +63,7 @@ describe("GET /v1/internal/decision — operator decision-trail endpoint", () => expect(body.project).toBe("loopover-orb"); expect(body.target.number).toBe(5); expect(body.target.status).toBe("merged"); - expect(body.decision).toEqual({ action: "merge", confidence: 0.9 }); + expect(body.decision).toEqual({ action: "merge", aiConfidence: 0.9 }); expect(body.audit.map((a) => a.event)).toContain("reviewed"); // Privacy: aggregate review state only — never actor logins / trust internals. expect(JSON.stringify(body)).not.toMatch(/login|actor|reward|payout|trust|wallet|hotkey/i); @@ -103,12 +114,13 @@ describe("GET /v1/internal/status — operator agent-health endpoint (#8904)", ( expect(res.status).toBe(200); const body = (await res.json()) as { project: string; - counts: { byStatus: unknown; byVerdict: unknown }; + counts: { byDecision: unknown }; health: { manualRate: number }; }; expect(body.project).toBe("loopover-orb"); expect(body.counts).toBeTruthy(); - expect(body.counts.byStatus).toBeTruthy(); + // #9136: byStatus + byVerdict collapsed into byDecision, sourced from review_audit's gate_decision rows. + expect(body.counts.byDecision).toBeTruthy(); expect(typeof body.health.manualRate).toBe("number"); // Privacy: aggregate review state only — never actor logins / trust internals. expect(JSON.stringify(body)).not.toMatch(/login|actor|reward|payout|trust|wallet|hotkey/i); diff --git a/test/unit/stats.test.ts b/test/unit/stats.test.ts index aeaaca31dd..1f8dc6aead 100644 --- a/test/unit/stats.test.ts +++ b/test/unit/stats.test.ts @@ -179,6 +179,35 @@ describe("aggregateReviewEffort — maintainer complexity fold (#2155)", () => { }); }); +describe("computeStats — the decision series reads decision_records (#9136)", () => { + // Against a REAL D1, not the stub above. The stubbed cases route by SQL substring and would pass + // with any column name at all -- which is exactly how the first repoint of this query shipped + // `dr.decision`, a column decision_records does not have. + it("groups the acted disposition per repo, counting each PR's newest record only", async () => { + const env = createTestEnv(); + await env.DB.prepare( + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) VALUES + ('record:acme/api#1@sha1', 'acme/api', 1, 'sha1', 'hold', 'needs_tests', 'd1', '{}', '2026-06-01T00:00:00Z'), + ('record:acme/api#1@sha2', 'acme/api', 1, 'sha2', 'merge', 'clean', 'd2', '{}', '2026-06-02T00:00:00Z'), + ('record:acme/api#2@sha1', 'acme/api', 2, 'sha1', 'close', 'duplicate', 'd3', '{}', '2026-06-02T00:00:00Z'), + ('record:acme/web#3@sha1', 'acme/web', 3, 'sha1', 'merge', 'clean', 'd4', '{}', '2026-06-02T00:00:00Z')`, + ).run(); + + const out = await computeStats(env, { days: 90, bucket: "day", nowMs: Date.parse("2026-06-14T00:00:00Z") }); + + // PR #1 was re-reviewed: its `hold` on sha1 is superseded by `merge` on sha2 and must not be + // counted, or every bucket inflates by the number of times a PR was re-reviewed. + expect(out.rows).toEqual([ + { bucket: "2026-06-02", project: "acme/api", verdict: "close", n: 1 }, + { bucket: "2026-06-02", project: "acme/api", verdict: "merge", n: 1 }, + { bucket: "2026-06-02", project: "acme/web", verdict: "merge", n: 1 }, + ]); + // Keyed by repo now, not a project slug: decision_records has no `project` column. + expect(out.projects).toEqual(["acme/api", "acme/web"]); + expect(out.verdicts).toEqual(["close", "merge"]); + }); +}); + describe("computeStats — review-effort read is fail-safe", () => { function effortThrowingEnv(): Env { let lastSql = "";