From 0dad9f862be31073304ae6d6d7a8ece84c04ea2b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 04:17:45 -0700 Subject: [PATCH 1/3] fix(orb): retire the orphaned review_targets ledger from every live reader (#9136, #9576, #9577) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `review_targets` has had NO writer anywhere since the 2026-06-22 convergence cutover. Verified exhaustively: zero INSERT/UPDATE/upsert in src/**, packages/**, scripts/**, or migrations/** — not dead code, not flag-gated, the write path does not exist. The only writes are test fixtures and the Grafana reporting copy, which legitimately mines the historical archive. Everything still reading it was therefore inert or expiring, with no alarm on any of it. This removes the last reader; `grep "FROM review_targets" src/` is now empty. WHAT WAS ACTUALLY BROKEN (three things, only one of which #9136 named) 1. computeCalibration read the orphaned table for its confidence curve, close distribution and disputed-close counts. 2. #9577 — a SECOND namespace mismatch, distinct from the SQL join #9136 names, and the one that actually disabled the recommender: `reverted` was built from review_audit target_ids (`owner/repo#123`) and tested against review_targets ids (`project:kind:owner/repo#123`). Never intersecting, so `!reverted.has(...)` was ALWAYS true — every merge read as kept, revertedMax always null, recommendedFloor always null, and the calibration-drift alert structurally unfireable. This would have survived any repopulation of the table. 3. #9576 — `GET /v1/internal/decision` 404'd for EVERY pull request since the cutover, while staying routed, authenticated and documented as the explain-any-verdict surface. Its audit query had the same namespace bug independently. WHAT REPLACED THEM decision_records (#8836) is the live successor: it carries the acted disposition, its reasonCode, and record_json's aiConfidence, keyed `repo_full_name || '#' || pull_number` — the SAME namespace as review_audit, so both mismatches retire together rather than being patched. A shared LATEST_DECISION_RECORD_FILTER restricts to the newest record per PR, matching that table's own latest-finalize-wins semantics. One subtlety that would have made this a no-op: review_targets spelled the field `confidence`, decision_records spells it `aiConfidence`. Reading only the new name yields null for every row and leaves the calibration exactly as dead as the orphaned table left it, so both spellings are accepted. WHAT WAS DELETED RATHER THAN REPOINTED, DELIBERATELY byStatus/byVerdict collapse into byDecision from review_audit's gate_decision rows — the disposition half of the old status vocabulary maps exactly onto merge/close/hold, and that is what terminalCount/nonTerminal/manualRate were really measuring. (manualRate also gains a correct denominator: it divided holds by a terminal count that EXCLUDED holds.) stuckRetryable, failed/failedTargets and their two Discord alerts are GONE. Those read `queued/reviewing/error/error_retryable` — PROCESSING states the cutover removed as a concept, not just a table, so nothing live records them and both alerts had been structurally unfireable for two months while reading as coverage. `failed` was also redundant: dlqCount/dlqTargets already cover "permanently failed", are live, and have their own alert. An alert that cannot fire is worse than no alert. computeStats/handleStats deleted too — the last reader, and unrouted (handleStats is exported and referenced only by a comment in routes.ts). The file-level dead-source check missed it because sibling exports from stats.ts ARE live. review_targets is dropped from checkReviewSourceFreshness: a 90-day probe on a permanently frozen table reported stale forever, a gauge no action could clear. decision_records takes its slot at review_audit's 7-day window, since it is now the source whose silence would mean these surfaces have gone dark. TESTS Two fixtures encoded the bugs as expected behaviour and are rewritten, not adjusted: calibrationEnv handed both queries the same bare ids, making the namespaces agree in the test and only there; routes-internal-decision-calibration seeded review_audit with the rowId shape production never writes. Both now use real `owner/repo#n` keys, so a namespace regression fails instead of hiding. Full suite green: 23,891 passed. --- src/review/alerts.ts | 26 +- src/review/ops.ts | 224 +++++++---- src/review/stats.ts | 175 --------- test/unit/alerts.test.ts | 57 +-- test/unit/ops.test.ts | 294 +++++++------- ...utes-internal-decision-calibration.test.ts | 30 +- test/unit/stats.test.ts | 368 +----------------- 7 files changed, 368 insertions(+), 806 deletions(-) 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..3c2f489afd 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`. + */ +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..a490d620cb 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -125,49 +125,6 @@ function storage(env: Env): D1Database { return env.DB; } -const timingSafeEncoder = new TextEncoder(); - -/** Constant-time string compare (reviewbot src/core/crypto.ts). */ -function timingSafeEqual(left: string, right: string): boolean { - const leftBytes = timingSafeEncoder.encode(left); - const rightBytes = timingSafeEncoder.encode(right); - const subtle = crypto.subtle as SubtleCrypto & { - timingSafeEqual?: (left: Uint8Array, right: Uint8Array) => boolean; - }; - if (leftBytes.length === rightBytes.length && typeof subtle.timingSafeEqual === "function") { - return subtle.timingSafeEqual(leftBytes, rightBytes); - } - const maxLength = Math.max(leftBytes.length, rightBytes.length); - let diff = leftBytes.length === rightBytes.length ? 0 : 1; - for (let index = 0; index < maxLength; index += 1) { - diff |= (leftBytes[index] ?? 0) ^ (rightBytes[index] ?? 0); - } - return diff === 0; -} - -/** Read a per-agent secret/var from the worker env by name (reviewbot src/core/util.ts). */ -function readSecret(env: Env, name: string): string { - const value = (env as unknown as Record)[name]; - return typeof value === "string" ? value : ""; -} - -// ── Stats config (byte-faithful from reviewbot src/core/stats.ts) ──────────────────────────────── - -const STATS_TOKEN_SECRET = "LOOPOVER_REVIEW_STATS_TOKEN"; - -const CORS_HEADERS: Record = { - "access-control-allow-origin": "*", - "access-control-allow-headers": "authorization,content-type", - "access-control-allow-methods": "GET,OPTIONS", -}; - -// Whitelisted bucket → SQLite strftime expression. NEVER interpolate the raw param into SQL. -const BUCKET_SQL: Record = { - day: "date(created_at)", - week: "strftime('%Y-W%W', created_at)", - month: "strftime('%Y-%m', created_at)", -}; - export interface ReviewEffortAggregate { /** Rounded average complexity band across distinct reviewed PRs in the window; null when no samples. */ avgBand: number | null; @@ -388,135 +345,3 @@ export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggre }; } -/** Aggregate the decision ledger for the dashboard. Pure-ish (reads D1 only); no GitHub I/O. */ -export async function computeStats( - env: Env, - opts: { days: number; bucket: string; nowMs: number }, - deps: StatsEvalDeps = defaultStatsEvalDeps, -): Promise { - const days = Number.isFinite(opts.days) && opts.days > 0 ? Math.min(opts.days, 730) : 90; - // hasOwn (not `in`) so prototype keys like "constructor"/"toString" can't defeat the whitelist and - // interpolate a non-SQL value into the query. - const bucket = Object.hasOwn(BUCKET_SQL, opts.bucket) ? opts.bucket : "day"; - // `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 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([ - storage(env).prepare( - `SELECT ${bucketExpr} AS bucket, project, COALESCE(verdict, status) AS verdict, COUNT(*) AS n - FROM review_targets - WHERE created_at >= ? - GROUP BY bucket, project, verdict - ORDER BY bucket ASC`, - ).bind(fromIso).all<{ bucket: string; project: string; verdict: string; n: number }>(), - storage(env).prepare( - `SELECT ${bucketExpr} AS bucket, project, COUNT(*) AS n - FROM review_audit - WHERE event_type IN ('reversal_reverted', 'reversal_reopened') AND created_at >= ? - GROUP BY bucket, project - ORDER BY bucket ASC`, - ).bind(fromIso).all<{ bucket: string; project: string; n: number }>(), - // review-effort (#2155): same persisted `reviewEffortMinutes` public-stats averages, scoped to this window. - // Repeated publish events for one PR collapse to one sample (per-PR AVG) before the global fold. - // #9084: two dialect hazards this SQL used to walk straight into on the Postgres self-host, both silent. - // - // json_extract translates to `->>`, which yields TEXT, so the enclosing AVG resolved to `avg(text)` — a - // function Postgres does not have. The error was swallowed by the fail-safe read wrapper, so the published - // "review effort / minutes saved" number was permanently zero and nothing said so. CAST(... AS REAL) is - // valid in both dialects; NULLIF guards the empty string, which Postgres would otherwise reject outright. - // - // And target_key is not uniformly two-segment: regateRepairTargetKey mints `repo#pr#headSha`. On SQLite the - // INTEGER cast of `pr#sha` is lenient garbage; on Postgres it aborts the WHOLE query, so a single - // three-segment row among the filtered event types took the entire public-stats read to [] and the homepage - // counters silently to zero. Excluding those keys before the cast keeps one row from erasing every number. - // The separator count is written as length()-length(replace()) rather than a nested instr(): `length` and - // `replace` mean the same thing in both dialects and need no translation at all. - storage(env).prepare( - `SELECT minutes FROM ( - SELECT repo, number, AVG(minutes) AS minutes - FROM ( - SELECT LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) AS repo, - CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number, - CAST(NULLIF(json_extract(metadata_json, '$.reviewEffortMinutes'), '') AS REAL) AS minutes - FROM audit_events - WHERE event_type = 'github_app.pr_public_surface_published' - AND created_at >= ? - AND instr(target_key, '#') > 0 - AND length(target_key) - length(replace(target_key, '#', '')) = 1 - ) - WHERE minutes IS NOT NULL - GROUP BY repo, number - )`, - ).bind(fromIso).all<{ minutes: number }>() - .catch(() => ({ results: [] as Array<{ minutes: number }> })), - computeCycleTimeAggregate(env, { days, nowMs: opts.nowMs }), - computeFindingAcceptance(env, { days, nowMs: opts.nowMs }), - ]); - - // Non-content gate decisions (incl. SHADOW would-actions) — recorded as `gate_decision` audit rows with - // the action in `decision`. Lets the dashboard show would-merge/close/hold counts before going live. - const gateRows = await storage(env).prepare( - `SELECT project, decision AS action, COUNT(*) AS n - FROM review_audit - WHERE event_type = 'gate_decision' AND decision IS NOT NULL AND created_at >= ? - GROUP BY project, action - ORDER BY n DESC`, - ) - .bind(fromIso) - .all<{ project: string; action: string; n: number }>() - .catch(() => ({ results: [] as Array<{ project: string; action: string; n: number }> })); - - const gateEval = await deps.computeGateEval(env, { days, nowMs: opts.nowMs }); - const recommendations = deps.computeTuningRecommendations(gateEval); - const parity = await deps.computeGateParity(env, { days, nowMs: opts.nowMs }); - - const rows = decisionRows.results ?? []; - const reversals = reversalRows.results ?? []; - const reviewEffort = aggregateReviewEffort( - (effortRows.results ?? []).map((row) => row.minutes ?? 0).filter((minutes) => minutes > 0), - ); - return { - generatedAt: new Date(opts.nowMs).toISOString(), - window: { fromIso, days, bucket }, - projects: [...new Set(rows.map((r) => r.project))].sort(), - verdicts: [...new Set(rows.map((r) => r.verdict))].sort(), - rows, - reversals, - gateActions: gateRows.results ?? [], - reviewEffort, - gateEval, - recommendations, - gateParity: { ...parity, cutoverReady: parity.rows.map((r) => ({ project: r.project, ready: isParityCutoverReady(r) })) }, - cycleTime, - findingAcceptance, - }; -} - -/** GET /stats/data?days=90&bucket=day — bearer-gated, CORS-open aggregate feed for the local dashboard. */ -export async function handleStats(request: Request, env: Env, deps: StatsEvalDeps = defaultStatsEvalDeps): Promise { - if (request.method === "OPTIONS") return new Response(null, { status: 204, headers: CORS_HEADERS }); - - // Uniform 401 whether the token is UNSET or WRONG — a 404-for-unset vs 401-for-wrong split was a config - // oracle (it revealed whether the token is configured). An unset token means NO request can authenticate, - // so the `!expected` short-circuit also prevents a `Bearer ` (empty-token) match. - const expected = readSecret(env, STATS_TOKEN_SECRET); - const provided = request.headers.get("authorization") ?? ""; - if (!expected || !timingSafeEqual(provided, `Bearer ${expected}`)) { - return new Response("unauthorized", { status: 401, headers: CORS_HEADERS }); - } - - const params = new URL(request.url).searchParams; - const payload = await computeStats( - env, - { - days: Number(params.get("days") ?? 90), - bucket: params.get("bucket") ?? "day", - nowMs: Date.now(), - }, - deps, - ); - return Response.json(payload, { headers: CORS_HEADERS }); -} 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..112240cb7d 100644 --- a/test/unit/stats.test.ts +++ b/test/unit/stats.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { describe, expect, it, } from "vitest"; import { createTestEnv } from "../helpers/d1"; import { aggregateCycleTimePercentiles, @@ -6,164 +6,19 @@ import { aggregateReviewEffort, buildCycleTimeDistribution, computeFindingAcceptance, - computeStats, cycleTimeMs, EMPTY_CYCLE_TIME, EMPTY_FINDING_ACCEPTANCE, - handleStats, isParityCutoverReady, MIN_PARITY_SAMPLE, PARITY_AGREEMENT_FLOOR, percentileNearestRank, type GateParityRow, - type StatsEvalDeps, } from "../../src/review/stats"; // Stub D1: route by table name — review_audit → reversals, else decision rows. -function stubEnv(extra: Record = {}): Env { - const decisions = [ - { bucket: "2026-06-01", project: "awesome-claude", verdict: "merge", n: 5 }, - { bucket: "2026-06-01", project: "awesome-claude", verdict: "close", n: 3 }, - { bucket: "2026-06-01", project: "loopover", verdict: "comment", n: 2 }, - ]; - const reversals = [{ bucket: "2026-06-01", project: "awesome-claude", n: 1 }]; - const gateActions = [ - { project: "metagraphed", action: "merge", n: 7 }, - { project: "metagraphed", action: "hold", n: 2 }, - ]; - const effortMinutes = [ - { minutes: 4 }, - { minutes: 96 }, - ]; - const cyclePairs = [ - { decided_at: "2026-06-01T10:00:00Z", outcome_at: "2026-06-01T10:05:00Z" }, - { decided_at: "2026-06-01T11:00:00Z", outcome_at: "2026-06-01T11:30:00Z" }, - ]; - // flagged-PR (hold|close) realized outcomes → 2 merged (addressed) + 1 closed (unaddressed). - const acceptanceRows = [{ truth: "merged" }, { truth: "merged" }, { truth: "closed" }]; - let lastSql = ""; - return { - ...extra, - DB: { - prepare: (s: string) => { - lastSql = s; - return { - bind: () => ({ - all: async () => ({ - // review-effort read → effortMinutes; cycle-time pairs → cyclePairs; gate action counts → gateActions; - // finding-acceptance read → acceptanceRows; other review_audit → reversals; everything else → decisions. - results: lastSql.includes("reviewEffortMinutes") - ? effortMinutes - : lastSql.includes("decided_at") && lastSql.includes("outcome_at") - ? cyclePairs - : lastSql.includes("decision AS action") - ? gateActions - : lastSql.includes("flagged") - ? acceptanceRows - : lastSql.includes("review_audit") - ? reversals - : decisions, - }), - }), - }; - }, - }, - } as unknown as Env; -} - const NOW = Date.parse("2026-06-14T00:00:00Z"); -describe("computeStats — D1 aggregate for the dashboard", () => { - it("returns sorted projects/verdicts, rows, reversals, and the window", async () => { - const out = await computeStats(stubEnv(), { days: 90, bucket: "week", nowMs: NOW }); - expect(out.projects).toEqual(["awesome-claude", "loopover"]); - expect(out.verdicts).toEqual(["close", "comment", "merge"]); - expect(out.rows).toHaveLength(3); - expect(out.reversals).toEqual([{ bucket: "2026-06-01", project: "awesome-claude", n: 1 }]); - expect(out.gateActions).toEqual([ - { project: "metagraphed", action: "merge", n: 7 }, - { project: "metagraphed", action: "hold", n: 2 }, - ]); - expect(out.window).toEqual({ fromIso: "2026-03-16", days: 90, bucket: "week" }); - expect(out.gateEval).toEqual({ rows: [], hasSignal: false }); - expect(out.recommendations).toEqual([]); - expect(out.gateParity.cutoverReady).toEqual([]); - expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); - expect(out.cycleTime.sampleSize).toBe(2); - expect(out.cycleTime.p50Ms).toBe(300_000); - expect(out.cycleTime.distribution.length).toBeGreaterThan(0); - expect(out.findingAcceptance).toEqual({ flagged: 3, addressed: 2, unaddressed: 1, acceptanceRate: 0.667 }); - }); - - it("clamps an absurd window and falls back to a safe bucket", async () => { - const out = await computeStats(stubEnv(), { days: 99999, bucket: "decade", nowMs: NOW }); - expect(out.window.days).toBe(730); - expect(out.window.bucket).toBe("day"); - }); - - it("defaults to 90 days for a non-positive window", async () => { - const out = await computeStats(stubEnv(), { days: 0, bucket: "day", nowMs: NOW }); - expect(out.window.days).toBe(90); - }); - - it("falls back to 'day' for a prototype-chain bucket key (whitelist can't be defeated by `constructor`)", async () => { - for (const evil of ["constructor", "toString", "__proto__", "hasOwnProperty"]) { - const out = await computeStats(stubEnv(), { days: 30, bucket: evil, nowMs: NOW }); - expect(out.window.bucket).toBe("day"); - } - }); - - it("threads injected eval/parity/tuning deps into the payload", async () => { - const out = await computeStats( - stubEnv(), - { days: 30, bucket: "day", nowMs: NOW }, - { - computeGateEval: async () => ({ rows: [], hasSignal: true }), - computeTuningRecommendations: () => [{ project: "p", severity: "warn", message: "tighten" }], - computeGateParity: async () => ({ - authoritative: "reviewbot", - shadow: "loopover", - hasSignal: true, - rows: [{ project: "p", pairedSamples: 40, bothMerge: 40, bothClose: 0, bothHold: 0, disagree: 0, agreementRate: 1, unsafeDisagreements: 0, byReasonCode: [] }], - }), - }, - ); - expect(out.gateEval.hasSignal).toBe(true); - expect(out.recommendations).toHaveLength(1); - expect(out.gateParity.cutoverReady).toEqual([{ project: "p", ready: true }]); - }); -}); - -describe("handleStats — bearer-gated, CORS-open feed", () => { - const req = (headers: Record = {}, method = "GET") => - new Request("https://w.dev/stats/data?days=30&bucket=day", { method, headers }); - - it("204s a CORS preflight with no auth", async () => { - const res = await handleStats(req({}, "OPTIONS"), stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" })); - expect(res.status).toBe(204); - expect(res.headers.get("access-control-allow-origin")).toBe("*"); - }); - - it("401s (NOT 404) when the token secret is unset — no config oracle, uniform with a wrong token", async () => { - expect((await handleStats(req({ authorization: "Bearer anything" }), stubEnv())).status).toBe(401); - expect((await handleStats(req(), stubEnv())).status).toBe(401); // no auth header, unset token → still 401 - }); - - it("401s a missing/wrong token", async () => { - const env = stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }); - expect((await handleStats(req(), env)).status).toBe(401); - expect((await handleStats(req({ authorization: "Bearer nope" }), env)).status).toBe(401); - }); - - it("200s with JSON + CORS for the correct token", async () => { - const res = await handleStats(req({ authorization: "Bearer s3cret" }), stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" })); - expect(res.status).toBe(200); - expect(res.headers.get("access-control-allow-origin")).toBe("*"); - const body = (await res.json()) as { projects: string[] }; - expect(body.projects).toContain("awesome-claude"); - }); -}); - describe("aggregateReviewEffort — maintainer complexity fold (#2155)", () => { it("returns null avgBand and 0 total minutes for an empty sample", () => { expect(aggregateReviewEffort([])).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); @@ -179,174 +34,6 @@ describe("aggregateReviewEffort — maintainer complexity fold (#2155)", () => { }); }); -describe("computeStats — review-effort read is fail-safe", () => { - function effortThrowingEnv(): Env { - let lastSql = ""; - return { - DB: { - prepare: (s: string) => { - lastSql = s; - return { - bind: () => ({ - all: async () => { - if (lastSql.includes("reviewEffortMinutes")) throw new Error("effort read down"); - return { results: [] }; - }, - }), - }; - }, - }, - } as unknown as Env; - } - - it("falls back to reviewEffort null/0 when the audit_events effort query rejects", async () => { - const out = await computeStats(effortThrowingEnv(), { days: 30, bucket: "day", nowMs: NOW }); - expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); - }); - - it("averages real reviewEffortMinutes out of audit_events via json_extract (real D1)", async () => { - const env = createTestEnv(); - const db = env.DB; - await db - .prepare( - `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json, created_at) - VALUES (?, ?, ?, ?, ?, ?), (?, ?, ?, ?, ?, ?)`, - ) - .bind( - "published-a", - "github_app.pr_public_surface_published", - "JSONbored/loopover#10", - "completed", - JSON.stringify({ reviewEffortMinutes: 4 }), - "2026-06-10T00:00:00.000Z", - "published-b", - "github_app.pr_public_surface_published", - "JSONbored/loopover#11", - "completed", - JSON.stringify({ reviewEffortMinutes: 96 }), - "2026-06-11T00:00:00.000Z", - ) - .run(); - - const out = await computeStats(env, { days: 90, bucket: "day", nowMs: NOW }); - expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); - }); - - it("skips nullish/zero minute rows when folding reviewEffort (the ?? 0 + > 0 filter branches)", async () => { - function mixedEffortEnv(): Env { - let lastSql = ""; - return { - DB: { - prepare: (s: string) => { - lastSql = s; - return { - bind: () => ({ - all: async () => ({ - results: lastSql.includes("reviewEffortMinutes") - ? [{ minutes: null }, { minutes: 0 }, { minutes: 10 }] - : [], - }), - }), - }; - }, - }, - } as unknown as Env; - } - - const out = await computeStats(mixedEffortEnv(), { days: 30, bucket: "day", nowMs: NOW }); - expect(out.reviewEffort).toEqual({ avgBand: 2, totalEstimatedMinutes: 10 }); - }); -}); - -describe("computeStats — gate-decision read is fail-safe", () => { - // A stubEnv whose gate_decision query rejects: computeStats should still resolve with gateActions: []. - function gateThrowingEnv(): Env { - const decisions = [{ bucket: "2026-06-01", project: "awesome-claude", verdict: "merge", n: 5 }]; - let lastSql = ""; - return { - DB: { - prepare: (s: string) => { - lastSql = s; - return { - bind: () => ({ - all: async () => { - if (lastSql.includes("decision AS action")) throw new Error("gate read down"); - return { results: lastSql.includes("review_audit") ? [] : decisions }; - }, - }), - }; - }, - }, - } as unknown as Env; - } - - it("falls back to gateActions: [] when the gate_decision query rejects (the .catch)", async () => { - const out = await computeStats(gateThrowingEnv(), { days: 30, bucket: "day", nowMs: NOW }); - expect(out.gateActions).toEqual([]); - expect(out.projects).toEqual(["awesome-claude"]); // the rest of the payload still computes - }); -}); - -describe("handleStats — query-param default branches", () => { - it("defaults days→90 and bucket→day when those params are absent (the ?? fallbacks)", async () => { - let captured: { days: number; bucket: string } | undefined; - const deps: StatsEvalDeps = { - // capture is observed via the payload window below; deps stay no-op. - computeGateEval: async (_env, o) => { - captured = { days: o.days, bucket: "" }; - return { rows: [], hasSignal: false }; - }, - computeTuningRecommendations: () => [], - computeGateParity: async () => ({ authoritative: "reviewbot", shadow: "loopover", hasSignal: false, rows: [] }), - }; - // No days / bucket query params → days ?? 90, bucket ?? "day". - const r = new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer s3cret" } }); - const res = await handleStats(r, stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }), deps); - expect(res.status).toBe(200); - const body = (await res.json()) as { window: { days: number; bucket: string } }; - expect(body.window.days).toBe(90); - expect(body.window.bucket).toBe("day"); - expect(captured?.days).toBe(90); - }); -}); - -describe("computeStats — NaN window + null D1 results (the ?? [] fallbacks)", () => { - // A stub whose .all() returns NO `results` key, exercising the `?? []` on decision/reversal/gate rows. - function nullResultsEnv(): Env { - return { - DB: { - prepare: () => ({ - bind: () => ({ - all: async () => ({}), // results undefined → the ?? [] fallbacks fire on all three reads - }), - }), - }, - } as unknown as Env; - } - - it("defaults to 90 days when opts.days is NaN (Number.isFinite false branch)", async () => { - const out = await computeStats(nullResultsEnv(), { days: Number.NaN, bucket: "day", nowMs: NOW }); - expect(out.window.days).toBe(90); - }); - - it("defaults to 90 days when opts.days is Infinity (Number.isFinite false branch)", async () => { - const out = await computeStats(nullResultsEnv(), { days: Number.POSITIVE_INFINITY, bucket: "day", nowMs: NOW }); - expect(out.window.days).toBe(90); - }); - - it("falls back to empty arrays when D1 returns no `results` field", async () => { - const out = await computeStats(nullResultsEnv(), { days: 30, bucket: "day", nowMs: NOW }); - expect(out.rows).toEqual([]); - expect(out.reversals).toEqual([]); - expect(out.gateActions).toEqual([]); - expect(out.projects).toEqual([]); - expect(out.verdicts).toEqual([]); - expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); - expect(out.cycleTime).toEqual(EMPTY_CYCLE_TIME); - expect(out.findingAcceptance).toEqual(EMPTY_FINDING_ACCEPTANCE); - }); -}); - describe("cycle-time aggregation (#2194)", () => { it("cycleTimeMs rejects negative and non-finite deltas", () => { expect(cycleTimeMs("2026-06-01T10:00:00Z", "2026-06-01T09:00:00Z")).toBeNull(); @@ -611,56 +298,3 @@ describe("isParityCutoverReady — every gate condition", () => { expect(isParityCutoverReady({ ...base, agreementRate: PARITY_AGREEMENT_FLOOR - 0.001 })).toBe(false); }); }); - -describe("timingSafeEqual + readSecret — branches reached through the handlers", () => { - afterEach(() => { - vi.restoreAllMocks(); - }); - - it("uses the native crypto.subtle.timingSafeEqual when present (equal-length, function-typed branch)", async () => { - const subtle = crypto.subtle as unknown as Record; - const had = "timingSafeEqual" in subtle; - const native = vi.fn((a: Uint8Array, b: Uint8Array) => a.length === b.length && a.every((x, i) => x === b[i])); - subtle.timingSafeEqual = native; - try { - // Correct token, equal lengths → the native path is taken and authorizes (200). - const res = await handleStats( - new Request("https://w.dev/stats/data?days=1&bucket=day", { method: "GET", headers: { authorization: "Bearer s3cret" } }), - stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }), - ); - expect(res.status).toBe(200); - expect(native).toHaveBeenCalled(); - } finally { - if (!had) delete subtle.timingSafeEqual; - } - }); - - it("uses the manual constant-time loop for UNEQUAL-length tokens (the diff=1 + ?? 0 out-of-range branch)", async () => { - const subtle = crypto.subtle as unknown as Record; - const had = "timingSafeEqual" in subtle; - // Force the non-native path so the manual loop (and its ?? 0 out-of-bounds reads) runs. - if (had) delete subtle.timingSafeEqual; - try { - // Provided "Bearer x" is far shorter than expected "Bearer " → unequal length → diff seeded 1, - // the loop XORs out-of-range indices via `?? 0`, and the compare fails → 401. - const res = await handleStats( - new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer x" } }), - stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "averylongtokenvalue-far-longer-than-x" }), - ); - expect(res.status).toBe(401); - } finally { - // afterEach restores; nothing to re-add since it was absent in this env. - void had; - } - }); - - it("treats a non-string token secret as unset (readSecret's `: \"\"` branch → 401)", async () => { - // LOOPOVER_REVIEW_STATS_TOKEN present but NOT a string → readSecret returns "" → !expected → 401. - const env = stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: 12345 }); - const res = await handleStats( - new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer 12345" } }), - env, - ); - expect(res.status).toBe(401); - }); -}); From fbc275a2135ed873f88e618aa3dce3128c1c2170 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 05:00:35 -0700 Subject: [PATCH 2/3] chore(stats): trim the trailing blank line the dead-code removal left at EOF git diff --check (the CI job) flags a new blank line at end of file. The AST deletion of computeStats/handleStats consumed their trailing newlines and left one behind. --- src/review/stats.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/review/stats.ts b/src/review/stats.ts index a490d620cb..161e6cd783 100644 --- a/src/review/stats.ts +++ b/src/review/stats.ts @@ -344,4 +344,3 @@ export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggre totalEstimatedMinutes: perPrMinutes.reduce((sum, minutes) => sum + minutes, 0), }; } - From b7070de638dba9d5513db9c047970c9b437d8f42 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 11:12:58 -0700 Subject: [PATCH 3/3] fix(stats): repoint the dashboard decision feed instead of deleting it Review blocker on #9578: src/review/stats.ts deleted computeStats/handleStats outright -- along with their bearer auth, CORS handling, and the BUCKET_SQL whitelist -- rather than repointing the one review_targets query the way every other reader in this PR was repointed. handleStats is not wired to a route today; only a comment in src/api/routes.ts mentions it. But "unrouted" and "retired" are different states, and silently turning one into the other inside a PR about an orphaned TABLE is a scope the title does not cover. Restored, with its single review_targets read repointed to decision_records exactly as ops.ts's four reads were. Two shape changes fall out of the move and both are now stated in the code: decision_records has no `project` column, so the series keys on repo_full_name; and `COALESCE(verdict, status)` becomes `action` -- merge|close|hold, the acted disposition ops.ts already reads for this purpose. It also needs LATEST_DECISION_RECORD_FILTER, now exported from ops.ts rather than copied: a PR accumulates one record per head sha, so counting all of them inflates every bucket by the number of times a PR was re-reviewed. The first repoint used `dr.decision`, a column decision_records does not have. The stubbed tests route by SQL substring and passed anyway; the one real-D1 test caught it. Added a real-D1 test for this query specifically, pinning the column names, the per-repo grouping, and the newest-record-wins filter -- the 368 test lines this PR had deleted are restored alongside it. --- src/review/ops.ts | 2 +- src/review/stats.ts | 203 ++++++++++++++++++++ test/unit/stats.test.ts | 397 +++++++++++++++++++++++++++++++++++++++- 3 files changed, 600 insertions(+), 2 deletions(-) diff --git a/src/review/ops.ts b/src/review/ops.ts index 3c2f489afd..c1ac5e9c85 100644 --- a/src/review/ops.ts +++ b/src/review/ops.ts @@ -193,7 +193,7 @@ const DLQ_WINDOW = "-6 hours"; * * Correlated on the alias `dr`, so every query using it must name its decision_records table `dr`. */ -const LATEST_DECISION_RECORD_FILTER = `dr.created_at = ( +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 )`; diff --git a/src/review/stats.ts b/src/review/stats.ts index 161e6cd783..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) ──────────────── @@ -125,6 +126,49 @@ function storage(env: Env): D1Database { return env.DB; } +const timingSafeEncoder = new TextEncoder(); + +/** Constant-time string compare (reviewbot src/core/crypto.ts). */ +function timingSafeEqual(left: string, right: string): boolean { + const leftBytes = timingSafeEncoder.encode(left); + const rightBytes = timingSafeEncoder.encode(right); + const subtle = crypto.subtle as SubtleCrypto & { + timingSafeEqual?: (left: Uint8Array, right: Uint8Array) => boolean; + }; + if (leftBytes.length === rightBytes.length && typeof subtle.timingSafeEqual === "function") { + return subtle.timingSafeEqual(leftBytes, rightBytes); + } + const maxLength = Math.max(leftBytes.length, rightBytes.length); + let diff = leftBytes.length === rightBytes.length ? 0 : 1; + for (let index = 0; index < maxLength; index += 1) { + diff |= (leftBytes[index] ?? 0) ^ (rightBytes[index] ?? 0); + } + return diff === 0; +} + +/** Read a per-agent secret/var from the worker env by name (reviewbot src/core/util.ts). */ +function readSecret(env: Env, name: string): string { + const value = (env as unknown as Record)[name]; + return typeof value === "string" ? value : ""; +} + +// ── Stats config (byte-faithful from reviewbot src/core/stats.ts) ──────────────────────────────── + +const STATS_TOKEN_SECRET = "LOOPOVER_REVIEW_STATS_TOKEN"; + +const CORS_HEADERS: Record = { + "access-control-allow-origin": "*", + "access-control-allow-headers": "authorization,content-type", + "access-control-allow-methods": "GET,OPTIONS", +}; + +// Whitelisted bucket → SQLite strftime expression. NEVER interpolate the raw param into SQL. +const BUCKET_SQL: Record = { + day: "date(created_at)", + week: "strftime('%Y-W%W', created_at)", + month: "strftime('%Y-%m', created_at)", +}; + export interface ReviewEffortAggregate { /** Rounded average complexity band across distinct reviewed PRs in the window; null when no samples. */ avgBand: number | null; @@ -344,3 +388,162 @@ export function aggregateReviewEffort(perPrMinutes: number[]): ReviewEffortAggre totalEstimatedMinutes: perPrMinutes.reduce((sum, minutes) => sum + minutes, 0), }; } + +/** + * 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 }, + deps: StatsEvalDeps = defaultStatsEvalDeps, +): Promise { + const days = Number.isFinite(opts.days) && opts.days > 0 ? Math.min(opts.days, 730) : 90; + // hasOwn (not `in`) so prototype keys like "constructor"/"toString" can't defeat the whitelist and + // interpolate a non-SQL value into the query. + const bucket = Object.hasOwn(BUCKET_SQL, opts.bucket) ? opts.bucket : "day"; + // `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!; + // 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 ${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 }>(), + storage(env).prepare( + `SELECT ${bucketExpr} AS bucket, project, COUNT(*) AS n + FROM review_audit + WHERE event_type IN ('reversal_reverted', 'reversal_reopened') AND created_at >= ? + GROUP BY bucket, project + ORDER BY bucket ASC`, + ).bind(fromIso).all<{ bucket: string; project: string; n: number }>(), + // review-effort (#2155): same persisted `reviewEffortMinutes` public-stats averages, scoped to this window. + // Repeated publish events for one PR collapse to one sample (per-PR AVG) before the global fold. + // #9084: two dialect hazards this SQL used to walk straight into on the Postgres self-host, both silent. + // + // json_extract translates to `->>`, which yields TEXT, so the enclosing AVG resolved to `avg(text)` — a + // function Postgres does not have. The error was swallowed by the fail-safe read wrapper, so the published + // "review effort / minutes saved" number was permanently zero and nothing said so. CAST(... AS REAL) is + // valid in both dialects; NULLIF guards the empty string, which Postgres would otherwise reject outright. + // + // And target_key is not uniformly two-segment: regateRepairTargetKey mints `repo#pr#headSha`. On SQLite the + // INTEGER cast of `pr#sha` is lenient garbage; on Postgres it aborts the WHOLE query, so a single + // three-segment row among the filtered event types took the entire public-stats read to [] and the homepage + // counters silently to zero. Excluding those keys before the cast keeps one row from erasing every number. + // The separator count is written as length()-length(replace()) rather than a nested instr(): `length` and + // `replace` mean the same thing in both dialects and need no translation at all. + storage(env).prepare( + `SELECT minutes FROM ( + SELECT repo, number, AVG(minutes) AS minutes + FROM ( + SELECT LOWER(substr(target_key, 1, instr(target_key, '#') - 1)) AS repo, + CAST(substr(target_key, instr(target_key, '#') + 1) AS INTEGER) AS number, + CAST(NULLIF(json_extract(metadata_json, '$.reviewEffortMinutes'), '') AS REAL) AS minutes + FROM audit_events + WHERE event_type = 'github_app.pr_public_surface_published' + AND created_at >= ? + AND instr(target_key, '#') > 0 + AND length(target_key) - length(replace(target_key, '#', '')) = 1 + ) + WHERE minutes IS NOT NULL + GROUP BY repo, number + )`, + ).bind(fromIso).all<{ minutes: number }>() + .catch(() => ({ results: [] as Array<{ minutes: number }> })), + computeCycleTimeAggregate(env, { days, nowMs: opts.nowMs }), + computeFindingAcceptance(env, { days, nowMs: opts.nowMs }), + ]); + + // Non-content gate decisions (incl. SHADOW would-actions) — recorded as `gate_decision` audit rows with + // the action in `decision`. Lets the dashboard show would-merge/close/hold counts before going live. + const gateRows = await storage(env).prepare( + `SELECT project, decision AS action, COUNT(*) AS n + FROM review_audit + WHERE event_type = 'gate_decision' AND decision IS NOT NULL AND created_at >= ? + GROUP BY project, action + ORDER BY n DESC`, + ) + .bind(fromIso) + .all<{ project: string; action: string; n: number }>() + .catch(() => ({ results: [] as Array<{ project: string; action: string; n: number }> })); + + const gateEval = await deps.computeGateEval(env, { days, nowMs: opts.nowMs }); + const recommendations = deps.computeTuningRecommendations(gateEval); + const parity = await deps.computeGateParity(env, { days, nowMs: opts.nowMs }); + + const rows = decisionRows.results ?? []; + const reversals = reversalRows.results ?? []; + const reviewEffort = aggregateReviewEffort( + (effortRows.results ?? []).map((row) => row.minutes ?? 0).filter((minutes) => minutes > 0), + ); + return { + generatedAt: new Date(opts.nowMs).toISOString(), + window: { fromIso, days, bucket }, + projects: [...new Set(rows.map((r) => r.project))].sort(), + verdicts: [...new Set(rows.map((r) => r.verdict))].sort(), + rows, + reversals, + gateActions: gateRows.results ?? [], + reviewEffort, + gateEval, + recommendations, + gateParity: { ...parity, cutoverReady: parity.rows.map((r) => ({ project: r.project, ready: isParityCutoverReady(r) })) }, + cycleTime, + findingAcceptance, + }; +} + +/** GET /stats/data?days=90&bucket=day — bearer-gated, CORS-open aggregate feed for the local dashboard. */ +export async function handleStats(request: Request, env: Env, deps: StatsEvalDeps = defaultStatsEvalDeps): Promise { + if (request.method === "OPTIONS") return new Response(null, { status: 204, headers: CORS_HEADERS }); + + // Uniform 401 whether the token is UNSET or WRONG — a 404-for-unset vs 401-for-wrong split was a config + // oracle (it revealed whether the token is configured). An unset token means NO request can authenticate, + // so the `!expected` short-circuit also prevents a `Bearer ` (empty-token) match. + const expected = readSecret(env, STATS_TOKEN_SECRET); + const provided = request.headers.get("authorization") ?? ""; + if (!expected || !timingSafeEqual(provided, `Bearer ${expected}`)) { + return new Response("unauthorized", { status: 401, headers: CORS_HEADERS }); + } + + const params = new URL(request.url).searchParams; + const payload = await computeStats( + env, + { + days: Number(params.get("days") ?? 90), + bucket: params.get("bucket") ?? "day", + nowMs: Date.now(), + }, + deps, + ); + return Response.json(payload, { headers: CORS_HEADERS }); +} diff --git a/test/unit/stats.test.ts b/test/unit/stats.test.ts index 112240cb7d..1f8dc6aead 100644 --- a/test/unit/stats.test.ts +++ b/test/unit/stats.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createTestEnv } from "../helpers/d1"; import { aggregateCycleTimePercentiles, @@ -6,19 +6,164 @@ import { aggregateReviewEffort, buildCycleTimeDistribution, computeFindingAcceptance, + computeStats, cycleTimeMs, EMPTY_CYCLE_TIME, EMPTY_FINDING_ACCEPTANCE, + handleStats, isParityCutoverReady, MIN_PARITY_SAMPLE, PARITY_AGREEMENT_FLOOR, percentileNearestRank, type GateParityRow, + type StatsEvalDeps, } from "../../src/review/stats"; // Stub D1: route by table name — review_audit → reversals, else decision rows. +function stubEnv(extra: Record = {}): Env { + const decisions = [ + { bucket: "2026-06-01", project: "awesome-claude", verdict: "merge", n: 5 }, + { bucket: "2026-06-01", project: "awesome-claude", verdict: "close", n: 3 }, + { bucket: "2026-06-01", project: "loopover", verdict: "comment", n: 2 }, + ]; + const reversals = [{ bucket: "2026-06-01", project: "awesome-claude", n: 1 }]; + const gateActions = [ + { project: "metagraphed", action: "merge", n: 7 }, + { project: "metagraphed", action: "hold", n: 2 }, + ]; + const effortMinutes = [ + { minutes: 4 }, + { minutes: 96 }, + ]; + const cyclePairs = [ + { decided_at: "2026-06-01T10:00:00Z", outcome_at: "2026-06-01T10:05:00Z" }, + { decided_at: "2026-06-01T11:00:00Z", outcome_at: "2026-06-01T11:30:00Z" }, + ]; + // flagged-PR (hold|close) realized outcomes → 2 merged (addressed) + 1 closed (unaddressed). + const acceptanceRows = [{ truth: "merged" }, { truth: "merged" }, { truth: "closed" }]; + let lastSql = ""; + return { + ...extra, + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => ({ + // review-effort read → effortMinutes; cycle-time pairs → cyclePairs; gate action counts → gateActions; + // finding-acceptance read → acceptanceRows; other review_audit → reversals; everything else → decisions. + results: lastSql.includes("reviewEffortMinutes") + ? effortMinutes + : lastSql.includes("decided_at") && lastSql.includes("outcome_at") + ? cyclePairs + : lastSql.includes("decision AS action") + ? gateActions + : lastSql.includes("flagged") + ? acceptanceRows + : lastSql.includes("review_audit") + ? reversals + : decisions, + }), + }), + }; + }, + }, + } as unknown as Env; +} + const NOW = Date.parse("2026-06-14T00:00:00Z"); +describe("computeStats — D1 aggregate for the dashboard", () => { + it("returns sorted projects/verdicts, rows, reversals, and the window", async () => { + const out = await computeStats(stubEnv(), { days: 90, bucket: "week", nowMs: NOW }); + expect(out.projects).toEqual(["awesome-claude", "loopover"]); + expect(out.verdicts).toEqual(["close", "comment", "merge"]); + expect(out.rows).toHaveLength(3); + expect(out.reversals).toEqual([{ bucket: "2026-06-01", project: "awesome-claude", n: 1 }]); + expect(out.gateActions).toEqual([ + { project: "metagraphed", action: "merge", n: 7 }, + { project: "metagraphed", action: "hold", n: 2 }, + ]); + expect(out.window).toEqual({ fromIso: "2026-03-16", days: 90, bucket: "week" }); + expect(out.gateEval).toEqual({ rows: [], hasSignal: false }); + expect(out.recommendations).toEqual([]); + expect(out.gateParity.cutoverReady).toEqual([]); + expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); + expect(out.cycleTime.sampleSize).toBe(2); + expect(out.cycleTime.p50Ms).toBe(300_000); + expect(out.cycleTime.distribution.length).toBeGreaterThan(0); + expect(out.findingAcceptance).toEqual({ flagged: 3, addressed: 2, unaddressed: 1, acceptanceRate: 0.667 }); + }); + + it("clamps an absurd window and falls back to a safe bucket", async () => { + const out = await computeStats(stubEnv(), { days: 99999, bucket: "decade", nowMs: NOW }); + expect(out.window.days).toBe(730); + expect(out.window.bucket).toBe("day"); + }); + + it("defaults to 90 days for a non-positive window", async () => { + const out = await computeStats(stubEnv(), { days: 0, bucket: "day", nowMs: NOW }); + expect(out.window.days).toBe(90); + }); + + it("falls back to 'day' for a prototype-chain bucket key (whitelist can't be defeated by `constructor`)", async () => { + for (const evil of ["constructor", "toString", "__proto__", "hasOwnProperty"]) { + const out = await computeStats(stubEnv(), { days: 30, bucket: evil, nowMs: NOW }); + expect(out.window.bucket).toBe("day"); + } + }); + + it("threads injected eval/parity/tuning deps into the payload", async () => { + const out = await computeStats( + stubEnv(), + { days: 30, bucket: "day", nowMs: NOW }, + { + computeGateEval: async () => ({ rows: [], hasSignal: true }), + computeTuningRecommendations: () => [{ project: "p", severity: "warn", message: "tighten" }], + computeGateParity: async () => ({ + authoritative: "reviewbot", + shadow: "loopover", + hasSignal: true, + rows: [{ project: "p", pairedSamples: 40, bothMerge: 40, bothClose: 0, bothHold: 0, disagree: 0, agreementRate: 1, unsafeDisagreements: 0, byReasonCode: [] }], + }), + }, + ); + expect(out.gateEval.hasSignal).toBe(true); + expect(out.recommendations).toHaveLength(1); + expect(out.gateParity.cutoverReady).toEqual([{ project: "p", ready: true }]); + }); +}); + +describe("handleStats — bearer-gated, CORS-open feed", () => { + const req = (headers: Record = {}, method = "GET") => + new Request("https://w.dev/stats/data?days=30&bucket=day", { method, headers }); + + it("204s a CORS preflight with no auth", async () => { + const res = await handleStats(req({}, "OPTIONS"), stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" })); + expect(res.status).toBe(204); + expect(res.headers.get("access-control-allow-origin")).toBe("*"); + }); + + it("401s (NOT 404) when the token secret is unset — no config oracle, uniform with a wrong token", async () => { + expect((await handleStats(req({ authorization: "Bearer anything" }), stubEnv())).status).toBe(401); + expect((await handleStats(req(), stubEnv())).status).toBe(401); // no auth header, unset token → still 401 + }); + + it("401s a missing/wrong token", async () => { + const env = stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }); + expect((await handleStats(req(), env)).status).toBe(401); + expect((await handleStats(req({ authorization: "Bearer nope" }), env)).status).toBe(401); + }); + + it("200s with JSON + CORS for the correct token", async () => { + const res = await handleStats(req({ authorization: "Bearer s3cret" }), stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" })); + expect(res.status).toBe(200); + expect(res.headers.get("access-control-allow-origin")).toBe("*"); + const body = (await res.json()) as { projects: string[] }; + expect(body.projects).toContain("awesome-claude"); + }); +}); + describe("aggregateReviewEffort — maintainer complexity fold (#2155)", () => { it("returns null avgBand and 0 total minutes for an empty sample", () => { expect(aggregateReviewEffort([])).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); @@ -34,6 +179,203 @@ 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 = ""; + return { + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => { + if (lastSql.includes("reviewEffortMinutes")) throw new Error("effort read down"); + return { results: [] }; + }, + }), + }; + }, + }, + } as unknown as Env; + } + + it("falls back to reviewEffort null/0 when the audit_events effort query rejects", async () => { + const out = await computeStats(effortThrowingEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); + }); + + it("averages real reviewEffortMinutes out of audit_events via json_extract (real D1)", async () => { + const env = createTestEnv(); + const db = env.DB; + await db + .prepare( + `INSERT INTO audit_events (id, event_type, target_key, outcome, metadata_json, created_at) + VALUES (?, ?, ?, ?, ?, ?), (?, ?, ?, ?, ?, ?)`, + ) + .bind( + "published-a", + "github_app.pr_public_surface_published", + "JSONbored/loopover#10", + "completed", + JSON.stringify({ reviewEffortMinutes: 4 }), + "2026-06-10T00:00:00.000Z", + "published-b", + "github_app.pr_public_surface_published", + "JSONbored/loopover#11", + "completed", + JSON.stringify({ reviewEffortMinutes: 96 }), + "2026-06-11T00:00:00.000Z", + ) + .run(); + + const out = await computeStats(env, { days: 90, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: 3, totalEstimatedMinutes: 100 }); + }); + + it("skips nullish/zero minute rows when folding reviewEffort (the ?? 0 + > 0 filter branches)", async () => { + function mixedEffortEnv(): Env { + let lastSql = ""; + return { + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => ({ + results: lastSql.includes("reviewEffortMinutes") + ? [{ minutes: null }, { minutes: 0 }, { minutes: 10 }] + : [], + }), + }), + }; + }, + }, + } as unknown as Env; + } + + const out = await computeStats(mixedEffortEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.reviewEffort).toEqual({ avgBand: 2, totalEstimatedMinutes: 10 }); + }); +}); + +describe("computeStats — gate-decision read is fail-safe", () => { + // A stubEnv whose gate_decision query rejects: computeStats should still resolve with gateActions: []. + function gateThrowingEnv(): Env { + const decisions = [{ bucket: "2026-06-01", project: "awesome-claude", verdict: "merge", n: 5 }]; + let lastSql = ""; + return { + DB: { + prepare: (s: string) => { + lastSql = s; + return { + bind: () => ({ + all: async () => { + if (lastSql.includes("decision AS action")) throw new Error("gate read down"); + return { results: lastSql.includes("review_audit") ? [] : decisions }; + }, + }), + }; + }, + }, + } as unknown as Env; + } + + it("falls back to gateActions: [] when the gate_decision query rejects (the .catch)", async () => { + const out = await computeStats(gateThrowingEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.gateActions).toEqual([]); + expect(out.projects).toEqual(["awesome-claude"]); // the rest of the payload still computes + }); +}); + +describe("handleStats — query-param default branches", () => { + it("defaults days→90 and bucket→day when those params are absent (the ?? fallbacks)", async () => { + let captured: { days: number; bucket: string } | undefined; + const deps: StatsEvalDeps = { + // capture is observed via the payload window below; deps stay no-op. + computeGateEval: async (_env, o) => { + captured = { days: o.days, bucket: "" }; + return { rows: [], hasSignal: false }; + }, + computeTuningRecommendations: () => [], + computeGateParity: async () => ({ authoritative: "reviewbot", shadow: "loopover", hasSignal: false, rows: [] }), + }; + // No days / bucket query params → days ?? 90, bucket ?? "day". + const r = new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer s3cret" } }); + const res = await handleStats(r, stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }), deps); + expect(res.status).toBe(200); + const body = (await res.json()) as { window: { days: number; bucket: string } }; + expect(body.window.days).toBe(90); + expect(body.window.bucket).toBe("day"); + expect(captured?.days).toBe(90); + }); +}); + +describe("computeStats — NaN window + null D1 results (the ?? [] fallbacks)", () => { + // A stub whose .all() returns NO `results` key, exercising the `?? []` on decision/reversal/gate rows. + function nullResultsEnv(): Env { + return { + DB: { + prepare: () => ({ + bind: () => ({ + all: async () => ({}), // results undefined → the ?? [] fallbacks fire on all three reads + }), + }), + }, + } as unknown as Env; + } + + it("defaults to 90 days when opts.days is NaN (Number.isFinite false branch)", async () => { + const out = await computeStats(nullResultsEnv(), { days: Number.NaN, bucket: "day", nowMs: NOW }); + expect(out.window.days).toBe(90); + }); + + it("defaults to 90 days when opts.days is Infinity (Number.isFinite false branch)", async () => { + const out = await computeStats(nullResultsEnv(), { days: Number.POSITIVE_INFINITY, bucket: "day", nowMs: NOW }); + expect(out.window.days).toBe(90); + }); + + it("falls back to empty arrays when D1 returns no `results` field", async () => { + const out = await computeStats(nullResultsEnv(), { days: 30, bucket: "day", nowMs: NOW }); + expect(out.rows).toEqual([]); + expect(out.reversals).toEqual([]); + expect(out.gateActions).toEqual([]); + expect(out.projects).toEqual([]); + expect(out.verdicts).toEqual([]); + expect(out.reviewEffort).toEqual({ avgBand: null, totalEstimatedMinutes: 0 }); + expect(out.cycleTime).toEqual(EMPTY_CYCLE_TIME); + expect(out.findingAcceptance).toEqual(EMPTY_FINDING_ACCEPTANCE); + }); +}); + describe("cycle-time aggregation (#2194)", () => { it("cycleTimeMs rejects negative and non-finite deltas", () => { expect(cycleTimeMs("2026-06-01T10:00:00Z", "2026-06-01T09:00:00Z")).toBeNull(); @@ -298,3 +640,56 @@ describe("isParityCutoverReady — every gate condition", () => { expect(isParityCutoverReady({ ...base, agreementRate: PARITY_AGREEMENT_FLOOR - 0.001 })).toBe(false); }); }); + +describe("timingSafeEqual + readSecret — branches reached through the handlers", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("uses the native crypto.subtle.timingSafeEqual when present (equal-length, function-typed branch)", async () => { + const subtle = crypto.subtle as unknown as Record; + const had = "timingSafeEqual" in subtle; + const native = vi.fn((a: Uint8Array, b: Uint8Array) => a.length === b.length && a.every((x, i) => x === b[i])); + subtle.timingSafeEqual = native; + try { + // Correct token, equal lengths → the native path is taken and authorizes (200). + const res = await handleStats( + new Request("https://w.dev/stats/data?days=1&bucket=day", { method: "GET", headers: { authorization: "Bearer s3cret" } }), + stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "s3cret" }), + ); + expect(res.status).toBe(200); + expect(native).toHaveBeenCalled(); + } finally { + if (!had) delete subtle.timingSafeEqual; + } + }); + + it("uses the manual constant-time loop for UNEQUAL-length tokens (the diff=1 + ?? 0 out-of-range branch)", async () => { + const subtle = crypto.subtle as unknown as Record; + const had = "timingSafeEqual" in subtle; + // Force the non-native path so the manual loop (and its ?? 0 out-of-bounds reads) runs. + if (had) delete subtle.timingSafeEqual; + try { + // Provided "Bearer x" is far shorter than expected "Bearer " → unequal length → diff seeded 1, + // the loop XORs out-of-range indices via `?? 0`, and the compare fails → 401. + const res = await handleStats( + new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer x" } }), + stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: "averylongtokenvalue-far-longer-than-x" }), + ); + expect(res.status).toBe(401); + } finally { + // afterEach restores; nothing to re-add since it was absent in this env. + void had; + } + }); + + it("treats a non-string token secret as unset (readSecret's `: \"\"` branch → 401)", async () => { + // LOOPOVER_REVIEW_STATS_TOKEN present but NOT a string → readSecret returns "" → !expected → 401. + const env = stubEnv({ LOOPOVER_REVIEW_STATS_TOKEN: 12345 }); + const res = await handleStats( + new Request("https://w.dev/stats/data", { method: "GET", headers: { authorization: "Bearer 12345" } }), + env, + ); + expect(res.status).toBe(401); + }); +});