diff --git a/scripts/replay-decision.ts b/scripts/replay-decision.ts index aebc552a26..a8246173f0 100644 --- a/scripts/replay-decision.ts +++ b/scripts/replay-decision.ts @@ -4,6 +4,12 @@ // // node --experimental-strip-types scripts/replay-decision.ts // ... | node --experimental-strip-types scripts/replay-decision.ts - +// node --experimental-strip-types scripts/replay-decision.ts --at +// +// #9028: `--at` names the instant to replay AT. Omit it to replay at the instant the decision itself recorded +// (`replayInput.clock.nowMs`) — the bit-exact case. Passing a DIFFERENT instant exits 1 with a `clock` +// divergence rather than reporting a match: time is a decision INPUT, and a clock-dependent rule +// (`gate.requireFreshRebaseWindow`) can flip purely because the wall clock moved. // // The bundle is one JSON object: { record: {...decision_records row}, replayInput: {...replay_json} }. // EXTRACT (operator, against the instance DB): @@ -24,8 +30,13 @@ import { readFileSync } from "node:fs"; import { replayDecision, type DecisionReplayInput, type ReplayableRecord } from "../src/review/decision-replay"; -/** Parse + normalize a bundle (snake_case SQL rows accepted) and replay it. Exported for tests. */ -export function runReplayBundle(raw: string): { outcome: ReturnType | null; error?: string } { +/** Parse + normalize a bundle (snake_case SQL rows accepted) and replay it. Exported for tests. + * + * #9028: `atMs` names the instant to replay AT. Omitted (the default) replays at the instant the decision + * recorded, which is the bit-exact case. Supplying a DIFFERENT instant is reported as a `clock` divergence, + * never silently accepted — a clock-dependent rule can legitimately flip its answer as the wall clock moves, + * so "it still matches at a different instant" is not a re-derivation of the original decision. */ +export function runReplayBundle(raw: string, atMs?: number): { outcome: ReturnType | null; error?: string } { let bundle: { record?: Record; replayInput?: unknown }; try { bundle = JSON.parse(raw) as never; @@ -47,18 +58,25 @@ export function runReplayBundle(raw: string): { outcome: ReturnType index !== atIndex && index !== atIndex + 1)[0]; if (!source) { - console.error("usage: replay-decision.ts "); + console.error("usage: replay-decision.ts [--at ]"); process.exit(2); } const raw = source === "-" ? readFileSync(0, "utf8") : readFileSync(source, "utf8"); - const { outcome, error } = runReplayBundle(raw); + const { outcome, error } = runReplayBundle(raw, atRaw === undefined ? undefined : Number(atRaw)); if (!outcome) { console.error(`replay-decision: ${error}`); process.exit(2); diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index f35797e8f3..2aa96d7fdb 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -23,6 +23,7 @@ import { buildPullRequestAdvisory } from "../rules/advisory"; import { recordAuditEvent, getDecryptedRepositoryAiKey, getRepository, listCheckSummaries, listPullRequestFiles } from "../db/repositories"; import { registerHeldLock, unregisterHeldLock } from "./held-lock-registry"; import { recordRoutingShadow } from "../services/reviewer-routing"; +import { scoreJudgmentAgreement } from "../review/judgment-agreement"; import { createInstallationToken } from "../github/app"; import type { AgentActionMode } from "../settings/agent-execution"; import { buildAiReviewDiff } from "../review/review-diff"; @@ -889,6 +890,10 @@ export async function runAiReviewForAdvisory( // the REAL reviewer identities and the REAL system prompt into DecisionRecord instead of hardcoding null. const aiJudgmentModelIds = parsedReviewModelIds(result.reviewDiagnostics ?? []); const aiJudgmentPromptDigest = result.systemPromptDigest; + // #8834: inter-run agreement over the stances this review ALREADY produced (#8229's reviewerVotes) — + // zero additional AI spend. Computed once and attached to whichever AI-judgment finding is built below, + // so the decision record carries a per-decision confidence signal for the calibration set (#8835). + const aiJudgmentAgreement = (verbalizedConfidence: number) => scoreJudgmentAgreement(result.reviewerVotes, verbalizedConfidence); if (result.consensusDefect) { findings.push({ code: "ai_consensus_defect", @@ -913,6 +918,7 @@ export async function runAiReviewForAdvisory( confidence: result.consensusDefect.confidence, modelIds: aiJudgmentModelIds, promptDigest: aiJudgmentPromptDigest, + agreement: aiJudgmentAgreement(result.consensusDefect.confidence), }); } else if (result.split) { // The reviewers DISAGREED — exactly one flagged a blocking defect. reviewbot's quorum treats any reviewer @@ -940,6 +946,10 @@ export async function runAiReviewForAdvisory( : {}), modelIds: aiJudgmentModelIds, promptDigest: aiJudgmentPromptDigest, + // A split IS the disagreement case: the stances differ, so agreement scores strictly below unanimity + // and the recorded confidence falls with it. #8834's "disagreement routes to hold" is already this + // finding's existing behavior via the confidence floor; this measures it rather than re-routing it. + agreement: aiJudgmentAgreement(result.splitConfidence ?? 1), }); } else if (result.inconclusive) { // Fail-CLOSED (#ai-fail-closed): block-mode AI could not return a usable verdict. Hold the PR for a human diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1e4002e3f7..897e0f7a15 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -663,6 +663,7 @@ import { import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory"; import { computeSalvageabilityForTarget } from "../review/salvageability-wire"; import { deriveDecisionReasonCode, persistDecisionReplayInputForGate } from "../review/decision-replay"; +import { isWithinFreshRebaseWindow, type DecisionClockCapture } from "../review/staleness-clock"; import { recordVerdictFlip } from "../review/verdict-flip-store"; import { resolveAutomaticCloseConfidence } from "../review/risk-control-wire"; import { maybeApplyCloseAuditHoldout } from "../review/close-audit-holdout"; @@ -3175,6 +3176,12 @@ async function runAgentMaintenancePlanAndExecute( }, ): Promise { const { installationId, repoFullName, pr, settings, otherOpenPullRequests, deliveryId, gate } = args; + // #9028: ONE wall-clock read for this whole decision pass, recorded into the replay input below and passed + // to every clock-dependent gate rule (today `gate.requireFreshRebaseWindow` via maybeForceFreshRebase) + // instead of each of them calling Date.now() independently. Two rules reading the clock at two different + // moments cannot both be replayed from one recorded instant — and an unrecorded instant is an unrecorded + // decision INPUT, which is exactly what makes a time-dependent decision unreplayable. + const decisionClock: DecisionClockCapture = { nowMs: Date.now() }; // Convergence safety: feed the planner the PR's changed paths + the repo's hard-guardrail globs so guarded // paths force manual review, and flag owner-authored PRs so they are never auto-closed (standing rule). @@ -3756,18 +3763,22 @@ async function runAgentMaintenancePlanAndExecute( modelIds: aiJudgment?.modelIds ?? null, promptDigest: aiJudgment?.promptDigest ?? null, aiConfidence: aiJudgment?.confidence ?? null, + // #8834: the inter-run agreement signal computed at review time (ai-review-orchestration.ts) and + // carried on the finding, so the record captures HOW REPRODUCIBLY the judgment was reached, not just + // what the model claimed. null for a rule-only decision, exactly like the fields above. + aiAgreement: aiJudgment?.agreement ?? null, salvageability, // #9135: legible on the record's own face — see maybeApplyCloseAuditHoldout's doc comment. divertedByHoldout: closeAuditHoldout?.diverted ?? false, }); const recordId = await persistDecisionRecord(env, record, recordDigest); - // #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0181) + // #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0182) // so the replay harness can re-derive this decision bit-exactly. Best-effort, like the record itself; // the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. Keyed to the // id persistDecisionRecord actually wrote (#9123: a supersession at the same head gets a revisioned id, // not the base one this used to always recompute independently). #9135: the holdout outcome rides along // so `holdout_consistency` has something to check the public record against. - if (recordId !== null) await persistDecisionReplayInputForGate(env, recordId, gate, policyCloseKind ?? null, closeAuditHoldout ?? null); + if (recordId !== null) await persistDecisionReplayInputForGate(env, recordId, gate, policyCloseKind ?? null, closeAuditHoldout ?? null, decisionClock); } // #2349 (PR 1): additive per-contributor calibration data, gated identically to recordNativeGateDecision // above -- see src/review/contributor-calibration.ts's doc comment. Currently write-only; nothing reads @@ -3897,6 +3908,7 @@ async function runAgentMaintenancePlanAndExecute( token, admissionKey, deliveryId, + nowMs: decisionClock.nowMs, })) ) { return; @@ -4806,9 +4818,12 @@ async function maybeForceFreshRebase( token: string | undefined; admissionKey: GitHubRateLimitAdmissionKey | undefined; deliveryId: string; + // #9028: the decision pass's single captured instant — this rule reads it instead of the clock, so the + // window comparison is replayable from `decision_replay_inputs.replay_json`. + nowMs: number; }, ): Promise { - const { installationId, repoFullName, pr, settings, windowMinutes, baseRef, token, admissionKey, deliveryId } = args; + const { installationId, repoFullName, pr, settings, windowMinutes, baseRef, token, admissionKey, deliveryId, nowMs } = args; /* v8 ignore next -- structurally unreachable: the caller only invokes this after confirming * (liveMergeState ?? pr.mergeableState) === "clean", which GitHub can never compute for a PR with no * head commit; the null check is belt-and-suspenders against the field's optional TS type. */ @@ -4816,7 +4831,7 @@ async function maybeForceFreshRebase( const advancedAt = await fetchLiveBaseBranchAdvancedAt(env, repoFullName, baseRef, token, admissionKey); if (!advancedAt) return false; // fail-open: unreadable base commit -> no forced rebase const advancedAtMs = Date.parse(advancedAt); - if (!Number.isFinite(advancedAtMs) || Date.now() - advancedAtMs >= windowMinutes * 60_000) return false; + if (!isWithinFreshRebaseWindow({ baseAdvancedAtMs: advancedAtMs, windowMinutes, nowMs })) return false; const countKey = freshRebaseForceCountKey(repoFullName, pr.number); const storedCount = Number(await getTransientKey(env, countKey)); diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index 78bb9bf367..da4ccbbd27 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -39,7 +39,7 @@ import { errorMessage, nowIso } from "../utils/json"; /** Bump when the record's FIELD SET changes meaning — consumers compare records only within a version. */ -export const DECISION_RECORD_SCHEMA_VERSION = "4"; // v4 (#9124/#9135): configDigest digests the resolved policy (+ settingsDigest split out), promptDigest digests the actual sent prompt, modelId -> modelIds (real identities), ciState populated, + divertedByHoldout; v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments +export const DECISION_RECORD_SCHEMA_VERSION = "5"; // v5 (#8834): + aiAgreement (inter-run agreement folded with the verbalized confidence); v4 (#9124/#9135): configDigest digests the resolved policy (+ settingsDigest split out), promptDigest digests the actual sent prompt, modelId -> modelIds (real identities), ciState populated, + divertedByHoldout; v3 (#8962): + salvageability {score, factors}; v2 (#8834): + aiConfidence, model/prompt commitments /** * Canonical JSON: recursively key-sorted, no insignificant whitespace — the ONE serialization every digest @@ -118,6 +118,12 @@ export type DecisionRecord = { * defect / split), null when no AI judgment contributed. Persisted so every decision joins the * risk-control calibration set (#8835) with its confidence attached. */ aiConfidence: number | null; + /** #8834: the per-decision confidence signal — inter-run agreement across the reviewer stances that + * produced the AI judgment, folded together with that judgment's verbalized confidence (see + * src/review/judgment-agreement.ts). `aiConfidence` above records what the model SAID; this records how + * reproducibly the reviewers reached it, which is the input a calibrated abstention threshold (#8835) + * needs. null when no AI judgment contributed, and for every record predating v5. */ + aiAgreement: { agreement: number; confidence: number; sampleCount: number; uncorroborated: boolean } | null; /** #8962: the deterministic salvageability score + its named factors when an AI judgment shaped the * decision — the second-axis evidence for auditing the close/hold boundary. null for rule-only decisions * (and for reconstructed/backfilled records predating v3). */ @@ -135,12 +141,13 @@ export type DecisionRecord = { /** Assemble the record and its own content digest. PURE given pre-computed digests. Normalizes the * optional-shaped caller fields (undefined -> null) HERE so call sites carry no fallback arms of their own. */ export async function buildDecisionRecord( - input: Omit & { + input: Omit & { decidedAt?: string; gatePack?: string | null | undefined; ciState?: string | null | undefined; baseSha?: string | null | undefined; aiConfidence?: number | null | undefined; + aiAgreement?: { agreement: number; confidence: number; sampleCount: number; uncorroborated: boolean } | null | undefined; salvageability?: { score: number; factors: string[] } | null | undefined; settingsDigest?: string | null | undefined; divertedByHoldout?: boolean | undefined; @@ -154,6 +161,7 @@ export async function buildDecisionRecord( ciState: input.ciState ?? null, baseSha: input.baseSha ?? null, aiConfidence: input.aiConfidence ?? null, + aiAgreement: input.aiAgreement ?? null, salvageability: input.salvageability ?? null, settingsDigest: input.settingsDigest ?? null, divertedByHoldout: input.divertedByHoldout ?? false, diff --git a/src/review/decision-replay.ts b/src/review/decision-replay.ts index 859e6b43c5..d8ac9f0652 100644 --- a/src/review/decision-replay.ts +++ b/src/review/decision-replay.ts @@ -8,6 +8,10 @@ // guarantee is by construction, not by flag. // // Stages, compared in pipeline order (the first mismatch wins — everything after it is downstream noise): +// 0. `clock` — #9028: the instant the caller asked to replay AT vs the instant the live decision +// recorded. Time is a decision input, so replaying a clock-dependent rule at a +// DIFFERENT instant is never silently accepted as a match. Skipped when the caller +// names no instant (replay at the recorded one) or the record predates #9028. // 1. `conclusion` — re-evaluated gate conclusion vs the decision-time snapshot. // 2. `blocker_codes` — ordered blocker code list (order IS meaning: blockerClass is the first code). // 3. `reason_code` — re-derived exactly as the finalize site derives it (blockerClass → @@ -32,6 +36,7 @@ // and file it; there is no "close enough" outcome. import { evaluateGateCheck, type GateCheckPolicy } from "../rules/advisory"; import { neutralHoldReasonCode } from "./parity-wire"; +import type { DecisionClockCapture } from "./staleness-clock"; import type { Advisory, AdvisoryFinding } from "../types"; import { errorMessage, nowIso } from "../utils/json"; @@ -60,6 +65,12 @@ export type DecisionReplayInput = { * this decision (ε absent/0, close autonomy not auto, or no eligible close) — the overwhelmingly common * case, and the SAME zero-I/O common path `maybeApplyCloseAuditHoldout` already guarantees. */ holdout?: DecisionReplayHoldout | null | undefined; + /** #9028: the decision-time wall clock — ONE `Date.now()` read per pass, which every clock-dependent gate + * rule (today: `gate.requireFreshRebaseWindow`) reads instead of calling the clock itself. Recorded here so + * a replay evaluates time-dependent rules at the instant the LIVE decision used, not at replay time. + * undefined/null for a pre-#9028 record — such a record simply has no recorded instant, so the `clock` + * stage cannot check anything and is skipped rather than guessed. */ + clock?: DecisionClockCapture | null | undefined; }; /** The slice of the PUBLIC decision record replay verifies against. */ @@ -79,11 +90,18 @@ export type ReplayOutcome = verdict: "divergence"; recordId: string; /** The FIRST divergent stage — later stages are downstream of it and not reported. */ - stage: "conclusion" | "blocker_codes" | "reason_code" | "holdout_consistency"; + stage: "clock" | "conclusion" | "blocker_codes" | "reason_code" | "holdout_consistency"; expected: string; actual: string; }; +/** #9028: options for a replay run. `nowMs` names the instant the caller wants to replay AT — supply it only + * to assert the recorded instant, or to deliberately probe a different one (which must FAIL, never silently + * pass). Omitted (the default) means "replay at the recorded instant", which is the bit-exact case. */ +export type ReplayOptions = { + nowMs?: number | undefined; +}; + /** The finalize site's reasonCode derivation, extracted verbatim so replay and live can never disagree * about the mapping itself (single source of truth — processors.ts finalize imports this too). */ export function deriveDecisionReasonCode(blockerClass: string, policyCloseKind: string | null | undefined, conclusion: string): string { @@ -97,7 +115,19 @@ export function deriveBlockerClass(evaluation: { blockers: Array<{ code: string } /** PURE bit-exact replay. See the module doc for the stage contract. */ -export function replayDecision(record: ReplayableRecord, input: DecisionReplayInput): ReplayOutcome { +export function replayDecision(record: ReplayableRecord, input: DecisionReplayInput, options: ReplayOptions = {}): ReplayOutcome { + // #9028 stage 0: TIME IS AN INPUT. A caller that names the instant it is replaying at must be told when + // that instant is not the one the live decision used — a clock-dependent rule (today + // `gate.requireFreshRebaseWindow`) can legitimately flip its answer purely because the wall clock moved, so + // silently replaying at "now" and reporting `match` would certify a re-derivation that never actually + // reproduced the original evaluation. Checked FIRST: every later stage is evaluated as of this instant. + // A caller that supplies no instant is replaying at the recorded one by definition, which is the bit-exact + // path and the CLI's default. A pre-#9028 record has no recorded instant, so there is nothing to contradict + // and the stage is skipped rather than guessed. + const recordedNowMs = input.clock?.nowMs; + if (typeof recordedNowMs === "number" && typeof options.nowMs === "number" && options.nowMs !== recordedNowMs) { + return { verdict: "divergence", recordId: record.id, stage: "clock", expected: String(recordedNowMs), actual: String(options.nowMs) }; + } const advisory: Advisory = { id: `replay-${record.id}`, targetType: "pull_request", @@ -141,7 +171,7 @@ export function replayDecision(record: ReplayableRecord, input: DecisionReplayIn return { verdict: "match", recordId: record.id, conclusion: evaluation.conclusion, blockerCodes, reasonCode, pinnedAction: record.action }; } -/** Persist the replay input beside its record (PRIVATE sibling — see migration 0181). Best-effort: replay +/** Persist the replay input beside its record (PRIVATE sibling — see migration 0182). Best-effort: replay * legibility must never break finalization, mirroring persistDecisionRecord's posture. Accepts the gate * EVALUATION and owns the no-replay no-op: content-lane/bridge evaluations are synthetic (their verdicts * come from their own deterministic pipelines, not the advisory evaluator) and carry no replay input — @@ -154,6 +184,8 @@ export async function persistDecisionReplayInputForGate( // #9135: the close-audit holdout's decision-time outcome for this same decision, when it drew — threaded // straight through to the persisted replay input so `holdout_consistency` has something to check against. holdout?: DecisionReplayHoldout | null, + // #9028: the decision pass's single captured wall-clock instant, so time-dependent rules are replayable. + clock?: DecisionClockCapture | null, ): Promise { if (!gate.replay) return; await persistDecisionReplayInput(env, recordId, { @@ -162,6 +194,7 @@ export async function persistDecisionReplayInputForGate( policyCloseKind, evaluated: { conclusion: gate.conclusion, blockerCodes: gate.blockers.map((blocker) => blocker.code) }, holdout: holdout ?? null, + clock: clock ?? null, }); } diff --git a/src/review/judgment-agreement.ts b/src/review/judgment-agreement.ts new file mode 100644 index 0000000000..8b1dbaa7a4 --- /dev/null +++ b/src/review/judgment-agreement.ts @@ -0,0 +1,73 @@ +// Per-decision confidence via inter-run agreement (#8834, epic #8828 Phase 3) — PURE. +// +// Verbalized confidence alone is poorly calibrated: a model asked "how sure are you" answers on a scale it +// has no grounding in, and #8845 already had to stop treating an ABSENT confidence as certainty. Sampling-based +// consistency is the better-behaved signal, and the risk-control literature this epic builds on (Trust or +// Escalate, ICLR 2025) finds two samples capture most of the available benefit. +// +// This module scores the samples the engine ALREADY runs. A dual-model review produces one independent stance +// per reviewer (`LoopOverAiReviewResult.reviewerVotes`, #8229) at zero additional AI spend, so inter-run +// agreement is computable today without multiplying the per-review bill. See the "not implemented here" note +// at the bottom for the paid rotated-exemplar extension and why it is deliberately absent. +// +// The score is ADDITIVE: it is recorded with the decision (`DecisionRecord.aiAgreement`) so it can join the +// risk-control calibration set (#8835) as the input an abstention threshold needs. It deliberately does NOT +// re-route the gate. Disagreement already routes to a hold today — one reviewer flagging and the other not +// IS the `ai_review_split` finding, which blocks or holds via the existing confidence floor — so adding a +// second, parallel disagreement route would double-count the same evidence rather than measure it. + +/** One reviewer's stance on a single evaluation run. */ +export type JudgmentSample = { + /** Whether this run flagged a blocking defect. The agreement axis. */ + votedFail: boolean; +}; + +export type JudgmentAgreement = { + /** Share of runs holding the MODAL stance, in [0,1]. Unanimity across N≥2 runs is 1; an even split is 0.5. */ + agreement: number; + /** Inter-run agreement combined with the verbalized confidence — the calibrated per-decision signal. */ + confidence: number; + /** How many runs actually produced a stance. */ + sampleCount: number; + /** True when fewer than two runs were available, so agreement was never actually observed. */ + uncorroborated: boolean; +}; + +/** The agreement credited to a lone run. A single sample cannot corroborate itself: scoring it 1.0 would + * fabricate unanimity out of one opinion, which is exactly the failure #8845 fixed for absent confidence. + * 0.5 — "no evidence either way" — keeps a single-run decision strictly below any genuinely corroborated + * one, so a budget-degraded run records a LOWER confidence rather than a flattering one. */ +export const UNCORROBORATED_AGREEMENT = 0.5; + +/** + * Score inter-run agreement and fold it into the verbalized confidence. PURE and total. + * + * `verbalizedConfidence` is the model's own calibrated confidence in its blocker (`AdvisoryFinding.confidence`). + * The combined score multiplies the two: a claim is only as trustworthy as BOTH how sure the judge said it was + * AND how reproducibly the judges reached it. Multiplication (rather than an average) keeps the result + * monotonically below either input, so the combined signal can never read as more certain than its weakest + * component — the property an abstention threshold depends on. + * + * ZERO samples (every reviewer failed to produce a usable opinion) is not a fabricated score: it reports + * `uncorroborated`, the uncorroborated agreement floor, and whatever confidence was actually stated. That + * decision is already held as `ai_review_inconclusive` upstream; this just refuses to invent a number for it. + */ +export function scoreJudgmentAgreement(samples: readonly JudgmentSample[], verbalizedConfidence: number): JudgmentAgreement { + const sampleCount = samples.length; + const failCount = samples.filter((sample) => sample.votedFail).length; + const modal = Math.max(failCount, sampleCount - failCount); + // Below two runs there is nothing to agree WITH — fall back to the uncorroborated floor rather than + // dividing by a sample count that cannot express disagreement. + const uncorroborated = sampleCount < 2; + const agreement = uncorroborated ? UNCORROBORATED_AGREEMENT : modal / sampleCount; + const stated = Number.isFinite(verbalizedConfidence) ? Math.min(1, Math.max(0, verbalizedConfidence)) : 0; + return { agreement, confidence: agreement * stated, sampleCount, uncorroborated }; +} + +// NOT IMPLEMENTED HERE, deliberately (#8834): the issue also describes running N=2-3 evaluations of the SAME +// judge with few-shot exemplars rotated out of the golden corpus ("simulated annotators"). That is a strictly +// better agreement signal than two different models voting once each — it isolates the judge's own +// reproducibility instead of confounding it with the two models' differing priors — but every extra run is a +// real, per-review AI charge on every reviewed PR, and this engine pays that bill in production. The scoring +// above is the half that costs nothing; the extra-sampling half needs a budget decision (and a flag defaulting +// OFF) before it can ship, so it is not stubbed here rather than landing as unreachable code. diff --git a/src/review/staleness-clock.ts b/src/review/staleness-clock.ts new file mode 100644 index 0000000000..6ddead4af1 --- /dev/null +++ b/src/review/staleness-clock.ts @@ -0,0 +1,56 @@ +// Clock-injected staleness rule evaluation (#9028, epic #8828 Phase 4) — the two `gate.*` staleness rules +// as PURE functions of their inputs plus an explicitly supplied instant. +// +// Why this module exists: `requireFreshRebaseWindowMinutes` was evaluated by reading `Date.now()` inline +// inside an IO helper (processors.ts `maybeForceFreshRebase`). That made the rule structurally unreplayable +// — nothing recorded WHICH instant the comparison used, so re-deriving the decision later could silently +// reach the opposite answer purely because the wall clock had moved. Time is a decision INPUT; an input that +// is not recorded is not replayable. The engine now reads one decision-time instant per pass, records it into +// `decision_replay_inputs.replay_json`, and every clock-dependent rule reads THAT instant rather than the +// clock (see `DecisionReplayInput.clock`). +// +// `staleBaseAheadByThreshold` is deliberately included here even though it reads NO clock: it is a commit-COUNT +// comparison (`aheadBy >= threshold`), so it is instant-independent by construction. Stating that as a pure, +// tested function is what makes the property provable rather than assumed — the replay suite pins it by +// evaluating the same inputs at wildly different instants and asserting an identical answer. + +/** Minutes → milliseconds, named so the fresh-rebase window's unit conversion has exactly one definition. */ +export const MS_PER_MINUTE = 60_000; + +/** The decision-time wall clock, captured ONCE per evaluation pass and recorded with the replay input. Every + * clock-dependent gate rule reads this instead of calling `Date.now()` itself, so a replay re-derives the + * same answer the live pass reached instead of whatever the clock happens to say at replay time. */ +export type DecisionClockCapture = { + /** Unix epoch milliseconds, from a single `Date.now()` read at the top of the decision pass. */ + nowMs: number; +}; + +/** + * `gate.requireFreshRebaseWindow` (#2552): true when the base branch's tip commit landed WITHIN + * `windowMinutes` of `nowMs` — i.e. the base moved so recently that a `mergeable_state: clean` read may + * predate it, so a merge should be preceded by a forced rebase + CI recheck. + * + * PURE and total: an unparseable/non-finite `baseAdvancedAtMs` returns false (fail-open to today's behavior — + * an unreadable base commit must never manufacture a rebase), matching the live call site's own guard. A base + * commit dated in the FUTURE relative to the captured instant (clock skew between GitHub and this engine) + * yields a negative age, which is inside any positive window and therefore correctly reads as "just moved". + */ +export function isWithinFreshRebaseWindow(args: { baseAdvancedAtMs: number; windowMinutes: number; nowMs: number }): boolean { + const { baseAdvancedAtMs, windowMinutes, nowMs } = args; + if (!Number.isFinite(baseAdvancedAtMs)) return false; + return nowMs - baseAdvancedAtMs < windowMinutes * MS_PER_MINUTE; +} + +/** + * `gate.staleBaseAheadByThreshold` (#review-grounding stale-base fact): true when the repo's default branch + * has advanced at least `threshold` commits beyond this PR's head, so the PR should be updated before review. + * + * PURE, total, and deliberately CLOCK-FREE — a commit count carries no notion of "now", so this rule's answer + * is identical at every instant. A non-finite `aheadBy` (an unreadable compare API response) returns false, + * fail-open, matching the live call site's `typeof aheadBy === "number"` guard. + */ +export function isBaseStaleByAheadBy(args: { aheadBy: number; threshold: number }): boolean { + const { aheadBy, threshold } = args; + if (!Number.isFinite(aheadBy)) return false; + return aheadBy >= threshold; +} diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index d4422989e3..a9e0514837 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -862,6 +862,47 @@ export function demoteEvidenceAbsenceBlockers(review: ModelReview, bodyTruncated }; } +/** #8833: a WHOLE-PR "this change ships no tests" claim, in either direction ("no tests were added" / + * "tests are missing" / "untested"). Whether a PR carries test-path evidence is decided by the engine's own + * path classifier (signals/test-evidence.ts `isTestPath`) and already owned by the deterministic + * `missing_test_evidence` finding — it is not a judgment call, so a model must never be the one to answer it. + * + * Deliberately narrow, and narrower than it first looks: the negative lookahead drops any claim that NARROWS + * to a specific target ("no tests for the nullish branch", "no test covers the error path"). That is a real, + * still-legitimate judgment about coverage DEPTH — which the classifier cannot check and this demotion must + * therefore leave standing — as opposed to a claim about test-file EXISTENCE, which the classifier owns + * outright. */ +export const TEST_EVIDENCE_ABSENCE_PATTERN = + /\b(?:no|zero|none|missing|lacks?|lacking|without)\s*(?:any\s+|new\s+|added\s+|accompanying\s+|corresponding\s+|associated\s+)?(?:unit\s+|integration\s+|regression\s+|automated\s+)?tests?\b(?!\s+(?:for|covering|covers?|around|on|that|which|exercis\w+|in|of|against|verif\w+|assert\w+)\b)|\buntested\b|\bnot\s+tested\b|\btests?\s+(?:are|is|were|was)\s+(?:missing|absent|not\s+(?:included|provided|added))\b/i; + +/** #8833: demote a whole-PR test-absence blocker when the deterministic classifier CONTRADICTS it — i.e. this + * PR demonstrably DOES change test paths. Mirrors demoteEvidenceAbsenceBlockers exactly: it fires only in the + * arm where the model's claim is provably a FACT ERROR, never where the claim might still be true, so a + * genuine "this PR ships no tests" blocker on a genuinely test-free PR is untouched and keeps its severity. + * Demotes to a nit rather than dropping, so the observation survives for the human — it just can no longer + * close a PR on a fact the engine already decided the other way. PURE. */ +export function demoteTestEvidenceAbsenceBlockers(review: ModelReview, prHasTestEvidence: boolean): { review: ModelReview; demoted: string[] } { + if (!prHasTestEvidence) return { review, demoted: [] }; + const demoted = review.blockers.filter((blocker) => TEST_EVIDENCE_ABSENCE_PATTERN.test(blocker)); + if (demoted.length === 0) return { review, demoted }; + return { + review: { + ...review, + blockers: review.blockers.filter((blocker) => !TEST_EVIDENCE_ABSENCE_PATTERN.test(blocker)), + nits: [...review.nits, ...demoted.map((claim) => `${claim} (demoted: this PR changes test paths — whether test evidence exists is decided by the deterministic test-path classifier, not by review)`)], + }, + demoted, + }; +} + +/** #8833: whether a PR carries ANY test-path evidence, from the SAME whole-PR semantics + * buildTestEvidencePromptSection and slop.ts's buildMissingTestEvidenceFinding already use — one changed + * test path is evidence for the PR. Exported so the demotion's arming condition and the prompt's own + * test-evidence section can never disagree about the fact. PURE. */ +export function prHasTestPathEvidence(files: ReadonlyArray<{ path: string }> | null | undefined): boolean { + return (files ?? []).some((file) => Boolean(file.path) && isTestPath(file.path)); +} + /** Parse a model's JSON review into a normalized {@link ModelReview}, or null when unparseable. */ export function parseModelReview(text: string): ModelReview | null { const jsonText = extractLastJsonObject(text); @@ -1351,6 +1392,8 @@ async function runWorkersOpinion( images?: readonly AiContentBlock[] | undefined, // #8961: true when the PR description exceeded the prompt window — arms the evidence-absence demotion. bodyTruncated = false, + // #8833: true when the PR changes at least one test path — arms the test-absence demotion. + prHasTestEvidence = false, ): Promise { const ai = env.AI as unknown as AiRunner | undefined; if (!ai || typeof ai.run !== "function") return { review: null }; @@ -1458,13 +1501,18 @@ async function runWorkersOpinion( const demotion = parsedRaw ? demoteCiClaimBlockers(parsedRaw) : null; // #8961: same guarantee for evidence-absence claims against a truncated description. const evidenceDemotion = demotion ? demoteEvidenceAbsenceBlockers(demotion.review, bodyTruncated) : null; - const parsed = evidenceDemotion?.review ?? null; + // #8833: same guarantee for whole-PR test-absence claims the path classifier already contradicts. + const testEvidenceDemotion = evidenceDemotion ? demoteTestEvidenceAbsenceBlockers(evidenceDemotion.review, prHasTestEvidence) : null; + const parsed = testEvidenceDemotion?.review ?? null; if (demotion && demotion.demoted.length > 0) { console.warn(JSON.stringify({ level: "warn", event: "ai_review_ci_claim_demoted", model, count: demotion.demoted.length })); } if (evidenceDemotion && evidenceDemotion.demoted.length > 0) { console.warn(JSON.stringify({ level: "warn", event: "ai_review_evidence_absence_demoted", model, count: evidenceDemotion.demoted.length })); } + if (testEvidenceDemotion && testEvidenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_test_evidence_absence_demoted", model, count: testEvidenceDemotion.demoted.length })); + } if (parsed && parsed.assessment.trim() !== "") { diagnostics.push({ model, attempt, status: "parsed", responseChars: text.length, hasJsonObject: Boolean(extractLastJsonObject(text)), ...usageFields }); return { review: parsed }; @@ -1826,6 +1874,7 @@ async function runProviderReview( maxTokens: number, images?: readonly AiContentBlock[] | undefined, bodyTruncated = false, // #8961: arms the evidence-absence demotion, same contract as runWorkersOpinion + prHasTestEvidence = false, // #8833: arms the test-absence demotion, same contract as runWorkersOpinion ): Promise { const { text, usage, failure } = await callAiProvider( providerKey, @@ -1849,7 +1898,12 @@ async function runProviderReview( if (providerEvidenceDemotion && providerEvidenceDemotion.demoted.length > 0) { console.warn(JSON.stringify({ level: "warn", event: "ai_review_evidence_absence_demoted", provider: providerKey.provider, count: providerEvidenceDemotion.demoted.length })); } - const review = providerEvidenceDemotion?.review ?? null; + // #8833: same test-absence enforcement as the Workers path — no parse route escapes it. + const providerTestEvidenceDemotion = providerEvidenceDemotion ? demoteTestEvidenceAbsenceBlockers(providerEvidenceDemotion.review, prHasTestEvidence) : null; + if (providerTestEvidenceDemotion && providerTestEvidenceDemotion.demoted.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "ai_review_test_evidence_absence_demoted", provider: providerKey.provider, count: providerTestEvidenceDemotion.demoted.length })); + } + const review = providerTestEvidenceDemotion?.review ?? null; return { review, diagnostic: { @@ -2607,6 +2661,9 @@ export async function runLoopOverAiReview( // #8961: pinned to the SAME body buildUserPrompt just sliced, so the demotion can never disagree with // the prompt about whether the description was cut. const bodyTruncated = (promptInput.body?.length ?? 0) > PR_BODY_PROMPT_LIMIT; + // #8833: read from the SAME changedFiles buildUserPrompt's test-evidence section is built from, so the + // demotion and the prompt can never disagree about whether this PR carries test-path evidence. + const prHasTestEvidence = prHasTestPathEvidence(promptInput.changedFiles); // Grounding-discipline SYSTEM suffix (convergence, flag-gated). When the caller supplied grounding, the // reviewers are told to verify claims against the attached CI/files; otherwise this is REVIEW_SYSTEM_PROMPT // unchanged (byte-identical). Computed from `promptInput` so it travels with the (possibly defanged) input. @@ -2757,6 +2814,7 @@ export async function runLoopOverAiReview( maxTokens, undefined, bodyTruncated, + prHasTestEvidence, ); advisoryReview = outcome.review; byokFailure = outcome.failure; @@ -2775,6 +2833,7 @@ export async function runLoopOverAiReview( aiRunCorrelation, undefined, bodyTruncated, + prHasTestEvidence, ); advisoryReview = outcome.review; if (outcome.fallbackNote) fallbackNotes.push(outcome.fallbackNote); @@ -2805,6 +2864,7 @@ export async function runLoopOverAiReview( aiRunCorrelation, undefined, bodyTruncated, + prHasTestEvidence, ) : Promise.resolve({ review: advisoryReview }), runWorkersOpinion( @@ -2819,6 +2879,7 @@ export async function runLoopOverAiReview( aiRunCorrelation, undefined, bodyTruncated, + prHasTestEvidence, ), ]); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); @@ -2885,6 +2946,7 @@ export async function runLoopOverAiReview( aiRunCorrelation, undefined, bodyTruncated, + prHasTestEvidence, ) : ({ review: advisoryReview } as ReviewerOpinionOutcome); if (a.fallbackNote) fallbackNotes.push(a.fallbackNote); diff --git a/src/types.ts b/src/types.ts index 8a6a6b6c4f..2288230873 100644 --- a/src/types.ts +++ b/src/types.ts @@ -611,6 +611,11 @@ export type AdvisoryFinding = { * inline, category, improvement-signal), NOT a digest of the base constant alone. Two repos with different * `review.instructions` therefore publish different values here. Absent for every deterministic finding. */ promptDigest?: string; + /** #8834: inter-run agreement across the reviewer stances that produced this AI judgment, folded together + * with the verbalized confidence (see src/review/judgment-agreement.ts). Carried on the finding exactly + * like `confidence`/`modelIds` above so the decision-record call site can thread it into + * `DecisionRecord.aiAgreement` without re-deriving the votes. Absent for every deterministic finding. */ + agreement?: { agreement: number; confidence: number; sampleCount: number; uncorroborated: boolean }; /** Public-safe screenshot evidence for a `visual_regression_finding` / `visual_unrelated_issue_finding` * (`review.visual.bugAnalysis`) — the SAME shot URLs already rendered in the "Visual preview" collapsible, * carried alongside the finding (not just referenced by route path) so a later consumer — the PR-closed diff --git a/test/unit/ai-review-advisory.test.ts b/test/unit/ai-review-advisory.test.ts index f05a69b010..f508e611aa 100644 --- a/test/unit/ai-review-advisory.test.ts +++ b/test/unit/ai-review-advisory.test.ts @@ -390,6 +390,29 @@ describe("runAiReviewForAdvisory", () => { expect(result?.notes).toContain("Likely crash."); }); + it("#8834: the consensus finding carries the inter-run agreement signal, derived from the stances already run", async () => { + const adv = advisory(); + await runAiReviewForAdvisory(aiEnv(async () => ({ response: defectJson() })), { + mode: "live", + settings: { aiReviewMode: "block" } as RepositorySettings, + advisory: adv, + repoFullName: "acme/widgets", + pr, + author: "alice", + confirmedContributor: true, + }); + const finding = adv.findings.find((f) => f.code === "ai_consensus_defect"); + // Present, structurally complete, and consistent with the finding's own verbalized confidence — the + // record's per-decision confidence signal (#8835's calibration input) is populated at review time, not + // reconstructed later from votes nothing persisted. + expect(finding?.agreement).toBeDefined(); + expect(finding?.agreement?.sampleCount).toBeGreaterThanOrEqual(1); + expect(finding?.agreement?.agreement).toBeGreaterThan(0); + expect(finding?.agreement?.agreement).toBeLessThanOrEqual(1); + // The combined score is never more certain than the verbalized confidence it folds in. + expect(finding?.agreement?.confidence).toBeLessThanOrEqual((finding?.confidence ?? 1) + 1e-12); + }); + it("threads settings.aiReviewCombine/aiReviewOnMerge/aiReviewReviewers (#2567) into the AI review call", async () => { // settings.aiReviewCombine/OnMerge/Reviewers are resolved from `.loopover.yml gate.aiReview.*` upstream by // resolveEffectiveSettings; runAiReviewForAdvisory must forward them into runLoopOverAiReview's input so a diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index b95b0de8b4..2d354c4198 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -5035,6 +5035,50 @@ describe("#8833: enforced boundaries between model judgment and deterministic fa expect(EVIDENCE_ABSENCE_PATTERN.test("Missing test coverage for the error branch")).toBe(false); }); + it("#8833: whole-PR test-absence blockers demote ONLY when the path classifier contradicts them", async () => { + const { demoteTestEvidenceAbsenceBlockers, prHasTestPathEvidence } = await import("../../src/services/ai-review"); + const claims = ["No tests were added for this change", "The new helper is untested", "Null deref in src/a.ts"]; + // Arm 1 — the PR really ships no test paths: the claim may well be TRUE, so it keeps its severity and the + // zero-demotion path returns the SAME object (no reallocation). + const untouched = review(claims); + expect(demoteTestEvidenceAbsenceBlockers(untouched, false).review).toBe(untouched); + // Arm 2 — the PR DOES change a test path: the claim is a fact error, so it demotes to a nit and survives + // there for the human; the code-content blocker is untouched. + const { review: out, demoted } = demoteTestEvidenceAbsenceBlockers(review(claims), true); + expect(demoted).toHaveLength(2); + expect(out.blockers).toEqual(["Null deref in src/a.ts"]); + expect(out.nits.filter((nit: string) => nit.includes("decided by the deterministic test-path classifier"))).toHaveLength(2); + // Armed but nothing to demote: same object back. + const clean = review(["Null deref in src/a.ts"]); + expect(demoteTestEvidenceAbsenceBlockers(clean, true).review).toBe(clean); + }); + + it("#8833: a coverage-DEPTH claim narrowed to a specific target is NOT demoted — only existence claims are", async () => { + const { TEST_EVIDENCE_ABSENCE_PATTERN, demoteTestEvidenceAbsenceBlockers } = await import("../../src/services/ai-review"); + // Existence claims the classifier owns outright. + for (const positive of ["no tests", "No new tests were added", "zero unit tests", "tests are missing", "This is untested", "lacks regression tests", "without any automated tests", "not tested"]) { + expect(TEST_EVIDENCE_ABSENCE_PATTERN.test(positive)).toBe(true); + } + // Depth claims the classifier CANNOT check — a real judgment that must keep blocking. + for (const negative of ["no tests for the nullish branch", "no test covers the error path", "no tests exercising the retry loop", "no tests against the 403 arm"]) { + expect(TEST_EVIDENCE_ABSENCE_PATTERN.test(negative)).toBe(false); + } + const narrowed = review(["no tests for the nullish branch"]); + expect(demoteTestEvidenceAbsenceBlockers(narrowed, true).review.blockers).toEqual(["no tests for the nullish branch"]); + }); + + it("#8833: prHasTestPathEvidence is whole-PR and total over null/empty/pathless input", async () => { + const { prHasTestPathEvidence } = await import("../../src/services/ai-review"); + expect(prHasTestPathEvidence(null)).toBe(false); + expect(prHasTestPathEvidence(undefined)).toBe(false); + expect(prHasTestPathEvidence([])).toBe(false); + expect(prHasTestPathEvidence([{ path: "" }])).toBe(false); + expect(prHasTestPathEvidence([{ path: "src/a.ts" }])).toBe(false); + // ONE changed test path is evidence for the whole PR — the same semantics slop.ts's + // buildMissingTestEvidenceFinding and buildTestEvidencePromptSection already use. + expect(prHasTestPathEvidence([{ path: "src/a.ts" }, { path: "test/unit/a.test.ts" }])).toBe(true); + }); + it("#8961: the prompt carries the truncation + attachment FACT for a long body, and stays plain otherwise", async () => { const { PR_BODY_PROMPT_LIMIT } = await import("../../src/services/ai-review"); const images = "![a](https://x.io/1.png) ![b](https://x.io/2.png)"; diff --git a/test/unit/backfill-decision-labels-core.test.ts b/test/unit/backfill-decision-labels-core.test.ts index 22a272c554..83520a8639 100644 --- a/test/unit/backfill-decision-labels-core.test.ts +++ b/test/unit/backfill-decision-labels-core.test.ts @@ -115,7 +115,7 @@ describe("buildBundle", () => { expect(await contentDigest(record)).toBe(row.record_digest); expect(canonicalJson(record)).toBe(row.record_json); expect(record.configDigest).toBe("backfill:unavailable"); - expect(record.schemaVersion).toBe("4"); // v4 (#9124/#9135) — bumped past the v3 salvageability shape this test targets + expect(record.schemaVersion).toBe("5"); // v5 (#8834) — bumped past the v3 salvageability shape this test targets } expect(records[0]).toMatchObject({ id: "record:o/r#1@sha1", action: "close", created_at: "2026-07-01T00:00:00.000Z" }); expect(records[1]).toMatchObject({ action: "hold" }); diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index d8e42666d6..4d85ba9bb9 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -52,6 +52,7 @@ function recordInput(over: Partial = {}): Omit { expect(runReplayBundle(JSON.stringify({ replayInput: bundle.replayInput })).outcome).toBeNull(); }); }); + +// #9028: wall-clock capture. Time is a decision INPUT — `gate.requireFreshRebaseWindow` compares the base +// branch's tip against "now", so a decision shaped by it is only replayable if the instant it used was +// recorded. These are the seeded per-rule regressions the issue requires: replay at the recorded instant is +// bit-exact, replay at a DIFFERENT instant is never silently accepted. +describe("staleness rules are clock-injected and replayable (#9028)", () => { + // A fixed seed instant, so every assertion below is a pure function of literals — no ambient Date.now(). + const SEEDED_NOW_MS = 1_800_000_000_000; + + it("requireFreshRebaseWindow: PURE and clock-injected — both arms, at a seeded instant", async () => { + const { isWithinFreshRebaseWindow, MS_PER_MINUTE } = await import("../../src/review/staleness-clock"); + const windowMinutes = 30; + // Inside the window: the base moved 29m before the captured instant. + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs: SEEDED_NOW_MS - 29 * MS_PER_MINUTE, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(true); + // Exactly at the boundary is OUTSIDE (`>=` in the original guard) — pinned so the edge cannot drift. + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs: SEEDED_NOW_MS - 30 * MS_PER_MINUTE, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(false); + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs: SEEDED_NOW_MS - 31 * MS_PER_MINUTE, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(false); + // Clock skew: a base commit dated AFTER the captured instant reads as "just moved", never as stale. + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs: SEEDED_NOW_MS + MS_PER_MINUTE, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(true); + // Unreadable base commit → fail-open (no manufactured rebase), matching the live call site's guard. + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs: Number.NaN, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(false); + }); + + it("requireFreshRebaseWindow: the SAME inputs flip answer as the instant moves — which is why it must be recorded", async () => { + const { isWithinFreshRebaseWindow, MS_PER_MINUTE } = await import("../../src/review/staleness-clock"); + const baseAdvancedAtMs = SEEDED_NOW_MS - 10 * MS_PER_MINUTE; + const windowMinutes = 30; + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs, windowMinutes, nowMs: SEEDED_NOW_MS })).toBe(true); + // Replaying the identical rule inputs an hour later reaches the OPPOSITE answer. This is the whole + // hazard #9028 closes: without the recorded instant, that flip is invisible and reads as a match. + expect(isWithinFreshRebaseWindow({ baseAdvancedAtMs, windowMinutes, nowMs: SEEDED_NOW_MS + 60 * MS_PER_MINUTE })).toBe(false); + }); + + it("staleBaseAheadByThreshold: PURE and provably instant-INDEPENDENT (a commit count carries no 'now')", async () => { + const { isBaseStaleByAheadBy } = await import("../../src/review/staleness-clock"); + expect(isBaseStaleByAheadBy({ aheadBy: 20, threshold: 20 })).toBe(true); // boundary is inclusive (`>=`) + expect(isBaseStaleByAheadBy({ aheadBy: 19, threshold: 20 })).toBe(false); + expect(isBaseStaleByAheadBy({ aheadBy: 21, threshold: 20 })).toBe(true); + // Unreadable compare-API response → fail-open, matching the live `typeof aheadBy === "number"` guard. + expect(isBaseStaleByAheadBy({ aheadBy: Number.NaN, threshold: 20 })).toBe(false); + // The instant-independence property itself: the signature admits no clock, so no reachable instant can + // change the answer. Pinned against the same seeded instants the fresh-rebase rule flips across above. + const args = { aheadBy: 20, threshold: 20 }; + vi.useFakeTimers(); + vi.setSystemTime(SEEDED_NOW_MS); + const atSeed = isBaseStaleByAheadBy(args); + vi.setSystemTime(SEEDED_NOW_MS + 365 * 24 * 60 * 60 * 1000); + expect(isBaseStaleByAheadBy(args)).toBe(atSeed); + vi.useRealTimers(); + }); + + it("stage 0 — replay at the RECORDED instant is bit-exact; a DIFFERENT instant diverges at `clock`", () => { + const [, bundle] = bundles.find(([name]) => name === "clean-merge.json")!; + const withClock: DecisionReplayInput = { ...bundle.replayInput, clock: { nowMs: SEEDED_NOW_MS } }; + // No instant named → replay at the recorded one → bit-exact match (the CLI default). + expect(replayDecision(replayable(bundle), withClock).verdict).toBe("match"); + // The recorded instant, named explicitly → still a match. + expect(replayDecision(replayable(bundle), withClock, { nowMs: SEEDED_NOW_MS }).verdict).toBe("match"); + // A different instant → NOT silently accepted. `clock` is checked before every other stage. + expect(replayDecision(replayable(bundle), withClock, { nowMs: SEEDED_NOW_MS + 1 })).toMatchObject({ + verdict: "divergence", + stage: "clock", + expected: String(SEEDED_NOW_MS), + actual: String(SEEDED_NOW_MS + 1), + }); + }); + + it("stage 0 — a pre-#9028 record has no recorded instant, so the clock stage is skipped, not guessed", () => { + const [, bundle] = bundles.find(([name]) => name === "clean-merge.json")!; + // No `clock` at all (every record written before this change): nothing to contradict, so the remaining + // stages still replay normally rather than the harness inventing an instant and failing every old record. + expect(replayDecision(replayable(bundle), bundle.replayInput, { nowMs: SEEDED_NOW_MS }).verdict).toBe("match"); + expect(replayDecision(replayable(bundle), { ...bundle.replayInput, clock: null }, { nowMs: SEEDED_NOW_MS }).verdict).toBe("match"); + }); + + it("the captured instant round-trips through persistence into replay_json", async () => { + const env = createTestEnv(); + const input = bundles[0]![1].replayInput; + await persistDecisionReplayInputForGate( + env, + "record:o/r#42@s", + { conclusion: "success", blockers: [], replay: { findings: input.findings, policy: input.policy } }, + null, + null, + { nowMs: SEEDED_NOW_MS }, + ); + const row = await env.DB.prepare("SELECT replay_json FROM decision_replay_inputs WHERE record_id = 'record:o/r#42@s'").first<{ replay_json: string }>(); + expect((JSON.parse(row!.replay_json) as DecisionReplayInput).clock).toEqual({ nowMs: SEEDED_NOW_MS }); + // Omitting the capture stores an explicit null (a pre-#9028-shaped row), never an invented instant. + await persistDecisionReplayInputForGate(env, "record:o/r#43@s", { conclusion: "success", blockers: [], replay: { findings: input.findings, policy: input.policy } }, null); + const bare = await env.DB.prepare("SELECT replay_json FROM decision_replay_inputs WHERE record_id = 'record:o/r#43@s'").first<{ replay_json: string }>(); + expect((JSON.parse(bare!.replay_json) as DecisionReplayInput).clock).toBeNull(); + }); + + it("CLI --at: the recorded instant matches, a different one exits with a clock divergence", () => { + const [, bundle] = bundles.find(([name]) => name === "clean-merge.json")!; + const withClock = { ...bundle, replayInput: { ...bundle.replayInput, clock: { nowMs: SEEDED_NOW_MS } } }; + const raw = JSON.stringify(withClock); + expect(runReplayBundle(raw).outcome?.verdict).toBe("match"); + expect(runReplayBundle(raw, SEEDED_NOW_MS).outcome?.verdict).toBe("match"); + expect(runReplayBundle(raw, SEEDED_NOW_MS + 86_400_000).outcome).toMatchObject({ verdict: "divergence", stage: "clock" }); + }); +}); diff --git a/test/unit/judgment-agreement.test.ts b/test/unit/judgment-agreement.test.ts new file mode 100644 index 0000000000..984a5d9aad --- /dev/null +++ b/test/unit/judgment-agreement.test.ts @@ -0,0 +1,66 @@ +import { describe, expect, it } from "vitest"; +import { scoreJudgmentAgreement, UNCORROBORATED_AGREEMENT } from "../../src/review/judgment-agreement"; + +// #8834: the per-decision confidence signal. The contract worth pinning is that the score can never read as +// MORE certain than its weakest input, and that a run with nothing to corroborate it is never scored as +// unanimous — the same "silence is not certainty" failure #8845 fixed for absent verbalized confidence. + +describe("scoreJudgmentAgreement (#8834)", () => { + const fail = { votedFail: true }; + const pass = { votedFail: false }; + + it("unanimous reviewers score full agreement and keep the verbalized confidence intact", () => { + expect(scoreJudgmentAgreement([fail, fail], 0.9)).toEqual({ agreement: 1, confidence: 0.9, sampleCount: 2, uncorroborated: false }); + // Unanimity on the NON-fail stance is equally unanimous — agreement is about reproducibility, not verdict. + expect(scoreJudgmentAgreement([pass, pass], 0.8)).toMatchObject({ agreement: 1, confidence: 0.8 }); + }); + + it("a split scores below unanimity and drags the combined confidence down with it", () => { + const split = scoreJudgmentAgreement([fail, pass], 0.9); + expect(split).toMatchObject({ agreement: 0.5, sampleCount: 2, uncorroborated: false }); + // 0.5 agreement x 0.9 stated = 0.45: a disagreed-upon judgment is recorded as materially less certain + // than the same judgment reached unanimously (0.9 above), which is the whole point of the signal. + expect(split.confidence).toBeCloseTo(0.45, 10); + expect(split.confidence).toBeLessThan(scoreJudgmentAgreement([fail, fail], 0.9).confidence); + }); + + it("2-of-3 is the modal stance regardless of which side holds it", () => { + expect(scoreJudgmentAgreement([fail, fail, pass], 1).agreement).toBeCloseTo(2 / 3, 10); + expect(scoreJudgmentAgreement([pass, pass, fail], 1).agreement).toBeCloseTo(2 / 3, 10); + expect(scoreJudgmentAgreement([fail, fail, fail], 1).agreement).toBe(1); + }); + + it("a lone run is UNCORROBORATED, never fabricated unanimity — the budget-degraded arm", () => { + // A single reviewer (single-reviewer plan, or a dual plan whose second leg failed / was budget-cut) + // cannot corroborate itself. It must record a LOWER confidence than a genuinely agreed judgment, not a + // flattering 1.0. + const lone = scoreJudgmentAgreement([fail], 0.9); + expect(lone).toMatchObject({ agreement: UNCORROBORATED_AGREEMENT, sampleCount: 1, uncorroborated: true }); + expect(lone.confidence).toBeCloseTo(0.45, 10); + expect(lone.confidence).toBeLessThan(scoreJudgmentAgreement([fail, fail], 0.9).confidence); + }); + + it("zero samples still refuses to invent a score", () => { + expect(scoreJudgmentAgreement([], 0.9)).toMatchObject({ agreement: UNCORROBORATED_AGREEMENT, sampleCount: 0, uncorroborated: true }); + }); + + it("clamps and totalizes a hostile verbalized confidence instead of propagating it", () => { + expect(scoreJudgmentAgreement([fail, fail], 5).confidence).toBe(1); + expect(scoreJudgmentAgreement([fail, fail], -3).confidence).toBe(0); + expect(scoreJudgmentAgreement([fail, fail], Number.NaN).confidence).toBe(0); + expect(scoreJudgmentAgreement([fail, fail], Number.POSITIVE_INFINITY).confidence).toBe(0); + }); + + it("INVARIANT: the combined score never exceeds either input, at any sample shape", () => { + const shapes = [[fail], [fail, fail], [fail, pass], [pass, pass], [fail, fail, pass], [pass, fail, fail], [fail, fail, fail], []]; + for (const stated of [0, 0.13, 0.5, 0.93, 1]) { + for (const shape of shapes) { + const scored = scoreJudgmentAgreement(shape, stated); + expect(scored.confidence).toBeLessThanOrEqual(scored.agreement + 1e-12); + expect(scored.confidence).toBeLessThanOrEqual(stated + 1e-12); + expect(scored.agreement).toBeGreaterThanOrEqual(0); + expect(scored.agreement).toBeLessThanOrEqual(1); + } + } + }); +}); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 05d026a56c..6ac81fced3 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -369,7 +369,7 @@ describe("queue processors", () => { // The decision record carries the boundary evidence. const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = ? and pull_number = ?").bind("owner/agent-repo", 18).first<{ record_json: string }>(); const parsedRecord = JSON.parse(record!.record_json) as { schemaVersion: string; salvageability: { score: number; factors: string[] } | null }; - expect(parsedRecord.schemaVersion).toBe("4"); // v4 (#9124/#9135) — bumped past the v3 salvageability shape this test targets + expect(parsedRecord.schemaVersion).toBe("5"); // v5 (#8834) — bumped past the v3 salvageability shape this test targets expect(parsedRecord.salvageability?.score).toBe(70); expect(parsedRecord.salvageability?.factors.join(" ")).toContain("mechanical defect class"); // #8838: the replay input persisted beside the record, and the decision re-derives bit-exactly from it. @@ -404,7 +404,7 @@ describe("queue processors", () => { notes: "cached review", reviewerCount: 2, // 0.95 sits ABOVE the default 0.93 floor: no salvageability hold, so this one-shot-closes cleanly. - findings: [{ code: "ai_consensus_defect", severity: "critical", title: "Unused import", detail: "unused import join from node:path is dead code.", confidence: 0.95, modelIds: ["claude-code", "codex"], promptDigest: "p".repeat(64) }], + findings: [{ code: "ai_consensus_defect", severity: "critical", title: "Unused import", detail: "unused import join from node:path is dead code.", confidence: 0.95, modelIds: ["claude-code", "codex"], promptDigest: "p".repeat(64), agreement: { agreement: 1, confidence: 0.95, sampleCount: 2, uncorroborated: false } }], metadata: { inputFingerprint }, }); vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { @@ -436,7 +436,7 @@ describe("queue processors", () => { settingsDigest: string | null; divertedByHoldout: boolean; }; - expect(parsed.schemaVersion).toBe("4"); + expect(parsed.schemaVersion).toBe("5"); expect(parsed.action).toBe("close"); // above-floor confidence, no salvageability config -> a one-shot close DECISION // The exact requirement: modelId is non-null whenever an AI judgment shaped the decision. expect(parsed.modelIds).toEqual(["claude-code", "codex"]); @@ -451,6 +451,11 @@ describe("queue processors", () => { expect(parsed.settingsDigest).toMatch(/^[0-9a-f]{64}$/); // No closeAuditHoldoutPct configured -> the holdout never draws -> never diverted. expect(parsed.divertedByHoldout).toBe(false); + // #8834: the per-decision confidence signal threads through to the record exactly like modelIds/ + // promptDigest above — read straight off the finding, never re-derived or dropped, so every AI-judgment + // decision joins the calibration set with its reproducibility attached. + const withAgreement = parsed as unknown as { aiAgreement: { agreement: number; confidence: number; sampleCount: number; uncorroborated: boolean } | null }; + expect(withAgreement.aiAgreement).toEqual({ agreement: 1, confidence: 0.95, sampleCount: 2, uncorroborated: false }); }); it("#9135: with gate.closeAuditHoldoutPct set, a would-close is diverted to a hold and the decision record + replay input both say so", async () => { @@ -768,7 +773,7 @@ describe("queue processors", () => { // decision record — the row every future calibration read keys on. const record = await env.DB.prepare("select record_json from decision_records where repo_full_name = 'owner/agent-repo' and pull_number = 9 order by created_at desc limit 1").first<{ record_json: string }>(); const parsedRecord = JSON.parse(record!.record_json) as { aiConfidence: number | null; promptDigest: string | null; modelIds: string[] | null; schemaVersion: string }; - expect(parsedRecord.schemaVersion).toBe("4"); // v4 (#9124/#9135): configDigest/promptDigest/modelIds/ciState + divertedByHoldout + expect(parsedRecord.schemaVersion).toBe("5"); // v4 (#9124/#9135): configDigest/promptDigest/modelIds/ciState + divertedByHoldout expect(parsedRecord.aiConfidence).toBe(0.3); // the cached sub-floor defect's calibrated confidence expect(parsedRecord.promptDigest).toBe("f".repeat(64)); expect(parsedRecord.modelIds).toEqual(["claude-code"]);