ledger(policy): require a declared reason for any repeat verdict at one head SHA (#9865)

* ledger(policy): require a declared reason for any repeat verdict at one head SHA (#9742)

decision_records already kept every evaluation of a (repo, pull, head_sha) -- a repeat
lands as <id>:revN, so nothing was ever silently replaced. What the record could not say
is WHY a second evaluation happened, which verdict it supersedes, or who asked for it:
"evaluated once" and "evaluated three times and one result was kept" were
indistinguishable in the public record.

The write layer now refuses a repeat verdict that does not declare a reason, so no caller
can bypass it by writing the row itself. A new head SHA is untouched -- that is a fresh
verdict by definition and needs no reason.

The reason is DERIVED, not judged per call site. The delivery-id prefixes that already
partitioned job producers were scattered string literals with matching startsWith reads
elsewhere; they now live in one module, and the reason map is typed
Record<DeliveryIdOrigin, ReevaluationReason> -- adding a producer without assigning a
reason fails the build rather than writing an unexplained verdict.

scheduled_recheck names the routine sweep. It is by far the highest-volume cause, and
without it every sweep tick past the first would have had to borrow one of the
incident-shaped codes -- drowning them, and (since both call sites swallow a throw) doing
it silently while aborting the rest of the maintenance pass.

DecisionRecordContext.reevaluation is required rather than defaulted: a repeat verdict
whose cause nobody declared is exactly what this refuses, so a future call site must
answer it at the type level instead of inheriting a plausible-looking guess.

* fix(ci): point the re-gate sort-key allowlist at the manual-regate ORIGIN key

The allowlist matches a producer by a marker string inside its object literal. The
manual re-gate route no longer repeats the `manual-regate:` prefix literal -- it calls
deliveryIdFor("manualRegate", ...) -- so the marker moved with it.

* refactor(queue): assert the delivery-id prefixes are distinguishable, not longest-match

The longest-prefix tie-break could only run if one prefix were a prefix of another,
which none is -- untestable defensive code standing in for a guarantee nobody stated.
The guarantee is now an invariant test, so adding an ambiguous prefix fails with the
real problem named rather than being silently absorbed by a scan.

Author: JSONbored

migrations/0204_decision_record_reevaluation.sql

@@ -0,0 +1,25 @@
+-- Verdict immutability per head SHA (#9742).
+--
+-- decision_records (0179) already keeps every evaluation of a (repo, pull, head_sha) -- a repeat lands as
+-- `<id>:revN` and #8837 gives each its own ledger chain row, so nothing is ever silently replaced. What the
+-- record could not say is WHY a second evaluation of the same head SHA happened, or WHICH verdict it
+-- supersedes. Without those, "this PR was evaluated once" and "this PR was evaluated three times and one
+-- result was kept" are indistinguishable in the public record.
+--
+-- Both columns are NULLABLE and stay NULL for a first evaluation, which is the overwhelming majority of rows
+-- and needs no reason: a new head SHA (force-push, new commits) legitimately starts a fresh verdict.
+-- Populated ONLY on a re-evaluation of a head SHA already carrying a verdict, where the writer must supply
+-- them -- enforced at the ledger-write layer so no caller can bypass it.
+ALTER TABLE decision_records ADD COLUMN reevaluation_reason TEXT;
+ALTER TABLE decision_records ADD COLUMN supersedes_record_id TEXT;
+
+-- WHO caused the re-evaluation, when that is a person rather than a schedule: the operator behind a
+-- manual re-gate, the maintainer who ran `@loopover review`. NULL for the machine-paced causes, which
+-- is most of them -- an actor column that invented "system" for those would make the two
+-- indistinguishable from a real named actor.
+ALTER TABLE decision_records ADD COLUMN reevaluation_actor TEXT;
+
+-- Answers "how many evaluations did this head SHA receive, and why" without scanning: the re-evaluation
+-- rollups (#9743) read exactly this shape, per window and per repo.
+CREATE INDEX IF NOT EXISTS decision_records_reevaluation
+  ON decision_records (reevaluation_reason, created_at);

scripts/check-regate-sort-key.ts

@@ -30,7 +30,9 @@ const PRODUCER_SCAN_CEILING_LINES = 60;
  */
 const ALLOWED_OMISSIONS: ReadonlyMap<string, string> = new Map([
   [
-    "src/api/routes.ts:manual-regate:",
+    // #9742: the delivery-id prefixes moved into queue/delivery-id.ts, so the producer names its ORIGIN
+    // key rather than repeating the `manual-regate:` literal. The marker tracks that name.
+    "src/api/routes.ts:deliveryIdFor(\"manualRegate\"",
     "The maintainer-triggered manual re-gate route enqueues at priority 99 to jump the queue ON PURPOSE — an operator asking for one PR now is exactly the case oldest-first should not apply to. It also has no PR record in hand (the body carries only repoFullName + prNumber).",
   ],
 ]);
@@ -114,8 +116,8 @@ function producerObjectText(lines: readonly string[], startIndex: number): strin
   return collected.join("\n");
 }
 
-/** Split `path/to/file.ts:marker-text` on the LAST colon that precedes the marker — a marker may itself
- *  contain colons (`manual-regate:`), so a naive split on the first or last colon gets it wrong. */
+/** Split `path/to/file.ts:marker-text` at the `.ts:` boundary rather than on the first or last colon — a
+ *  marker may itself contain colons, so either naive split gets it wrong. */
 function splitAllowKey(key: string): [string, string] {
   const boundary = key.indexOf(".ts:");
   if (boundary === -1) return [key, ""];

src/api/routes.ts

@@ -12,6 +12,7 @@ import { verifyShotRenderToken } from "../review/visual/shot-render-token";
 import { isScreenshotsEnabled } from "../review/visual-wire";
 import { buildFindingTaxonomyDocument } from "../review/finding-taxonomy";
 import { buildEnrichmentAnalyzersTaxonomyDocument } from "../review/enrichment-analyzers-taxonomy";
+import { deliveryIdFor } from "../queue/delivery-id";
 import {
   GITHUB_OAUTH_STATE_COOKIE,
   authenticateInternalToken,
@@ -4423,7 +4424,7 @@ export function createApp() {
     if (typeof repo?.installationId !== "number") return c.json({ error: "repo not installed" }, 404);
     const message: JobMessage = {
       type: "agent-regate-pr",
-      deliveryId: `manual-regate:${crypto.randomUUID()}`,
+      deliveryId: deliveryIdFor("manualRegate", crypto.randomUUID()),
       repoFullName: repo.fullName,
       prNumber,
       installationId: repo.installationId,

src/queue/delivery-id.ts

@@ -0,0 +1,54 @@
+// The delivery-id vocabulary (#9742).
+//
+// Every `agent-regate-pr` job carries a `deliveryId`. Most are a REAL GitHub webhook delivery id -- the
+// event that caused the work. The rest are SYNTHETIC ids minted by a producer that has no webhook to
+// point at (a scheduled sweep, a repair fan-out, an operator's manual re-gate), and each of those tags
+// itself with a prefix naming its origin.
+//
+// The prefixes were string literals at each producer with matching `startsWith` reads scattered
+// elsewhere. They live here now because the re-evaluation reason a decision record must declare
+// (#9742) is DERIVED from them: `REEVALUATION_REASON_BY_ORIGIN` in review/decision-record.ts is typed
+// `Record<DeliveryIdOrigin, ReevaluationReason>`, so a new origin added here without a reason assigned
+// there fails the build. A verdict whose cause nobody assigned is a verdict nobody can explain.
+
+export const DELIVERY_ID_PREFIXES = {
+  /** The scheduled re-gate sweep's own per-PR fan-out (#audit-sweep-fanout). */
+  regateSweep: "regate-sweep:",
+  /** The sweep's outage-repair fan-out: a PR missing a current-head Gate check or surface publish. */
+  regateRepair: "regate-repair:",
+  /** An operator's explicit re-gate through the internal jobs endpoint. */
+  manualRegate: "manual-regate:",
+  /** Backlog convergence: an open PR whose surface was never published for its current head. */
+  backlogConvergence: "backlog-convergence:",
+  /** Recovery of a panel "Re-run review" click whose webhook delivery was lost. */
+  panelRetriggerRecovery: "panel-retrigger-recovery:",
+  /** Pass 2 of flag-then-close: re-verify the linked issue after the pending-closure label landed. */
+  linkedIssueVerify: "linked-issue-verify:",
+  /** PR reconciliation repair. */
+  reconcile: "reconcile:",
+  /** A published surface with no disposition marker for its head -- a crashed or lost pass. */
+  surfaceWithoutDisposition: "surface-without-disposition:",
+} as const;
+
+/** The producers that mint a synthetic delivery id. A raw GitHub delivery id has no origin. */
+export type DeliveryIdOrigin = keyof typeof DELIVERY_ID_PREFIXES;
+
+export const DELIVERY_ID_ORIGINS = Object.keys(DELIVERY_ID_PREFIXES) as readonly DeliveryIdOrigin[];
+
+/** Build a synthetic delivery id, so no producer repeats a prefix literal. */
+export function deliveryIdFor(origin: DeliveryIdOrigin, suffix: string): string {
+  return `${DELIVERY_ID_PREFIXES[origin]}${suffix}`;
+}
+
+/**
+ * Which producer minted this delivery id, or null for a raw GitHub webhook delivery id.
+ *
+ * First match wins, which is unambiguous ONLY because no prefix here is a prefix of another -- an
+ * invariant asserted directly in decision-record.test.ts rather than worked around with a
+ * longest-match scan. A tie-break that can never run is untestable defensive code; a failing
+ * invariant test names the real problem (two origins that cannot be told apart) to whoever adds one.
+ */
+export function deliveryIdOrigin(deliveryId: string | null | undefined): DeliveryIdOrigin | null {
+  if (typeof deliveryId !== "string") return null;
+  return DELIVERY_ID_ORIGINS.find((origin) => deliveryId.startsWith(DELIVERY_ID_PREFIXES[origin])) ?? null;
+}

src/queue/processors.ts

@@ -676,7 +676,7 @@ import { isWithinFreshRebaseWindow, type DecisionClockCapture } from "../review/
 import { recordVerdictFlip } from "../review/verdict-flip-store";
 import { resolveAutomaticCloseConfidence } from "../review/risk-control-wire";
 import { maybeApplyCloseAuditHoldout } from "../review/close-audit-holdout";
-import { buildDecisionRecord, contentDigest, loadDecisionRecordCollapsible, persistDecisionRecord } from "../review/decision-record";
+import { buildDecisionRecord, contentDigest, deriveReevaluationReason, loadDecisionRecordCollapsible, persistDecisionRecord } from "../review/decision-record";
 import { neutralHoldReasonCode, nativeGateActionFromConclusion, recordNativeGateDecision } from "../review/parity-wire";
 import { recordContributorGateDecision } from "../review/contributor-calibration";
 import { recordGateBlockersAndCheckRepeatAlarm } from "../review/rule-repeat-alarm-wire";
@@ -703,6 +703,7 @@ import { sha256Hex } from "../utils/crypto";
 import { errorMessage, nowIso, repoParts } from "../utils/json";
 import { DISPOSITION_CONSIDERED_EVENT_TYPE } from "../review/surface-disposition-reconciler";
 import { maybeSuggestMilestoneMatchForPr } from "../integrations/project-tracker-adapter";
+import { deliveryIdFor } from "./delivery-id";
 
 const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000;
 const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000;
@@ -1660,8 +1661,8 @@ export async function sweepRepoRegate(
       const job: JobMessage = {
         type: "agent-regate-pr",
         deliveryId: isPriorityRepair
-          ? `regate-repair:${repoFullName}#${pr.number}`
-          : `regate-sweep:${repoFullName}#${pr.number}`,
+          ? deliveryIdFor("regateRepair", `${repoFullName}#${pr.number}`)
+          : deliveryIdFor("regateSweep", `${repoFullName}#${pr.number}`),
         repoFullName,
         prNumber: pr.number,
         installationId: sweepInstallationId,
@@ -1952,7 +1953,7 @@ export async function sweepRepoBacklogConvergence(
     candidates.map((pr, index) => {
       const job: JobMessage = {
         type: "agent-regate-pr",
-        deliveryId: `backlog-convergence:${repoFullName}#${pr.number}`,
+        deliveryId: deliveryIdFor("backlogConvergence", `${repoFullName}#${pr.number}`),
         repoFullName,
         prNumber: pr.number,
         installationId: sweepInstallationId,
@@ -3197,7 +3198,7 @@ async function maybeCloseForContributorCapOnOpen(
         // risk-control calibration set). The generic policy_close:contributor_cap reasonCode derivation
         // (agent-action-executor.ts's defaultDecisionRecordReasonCode) is exactly right here — this path has
         // no gate evaluation to derive a richer blockerClass-based code from.
-        decisionRecord: { configDigest: await contentDigest(settings) },
+        decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
       },
       planned,
     );
@@ -3835,7 +3836,13 @@ async function runAgentMaintenancePlanAndExecute(
       // #9135: legible on the record's own face — see maybeApplyCloseAuditHoldout's doc comment.
       divertedByHoldout: closeAuditHoldout?.diverted ?? false,
     });
-    const recordId = await persistDecisionRecord(env, record, recordDigest);
+    // #9742: a repeat verdict for the SAME head SHA must declare why. The cause is derived from this
+    // job's own delivery id -- the scheduled sweep, a repair fan-out, an operator's manual re-gate,
+    // or (no synthetic prefix) a real webhook event that moved something the verdict depends on. The
+    // writer ignores it entirely on a first evaluation, which is the overwhelming majority of writes.
+    const recordId = await persistDecisionRecord(env, record, recordDigest, 3, {
+      reason: deriveReevaluationReason(deliveryId),
+    });
     // #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
@@ -4472,7 +4479,7 @@ async function prReadyForReview(
         moderationSettings: null,
         // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close),
         // so the decision-record path inside the executor is never actually exercised for it.
-        decisionRecord: { configDigest: await contentDigest(settings) },
+        decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
       },
       [
         {
@@ -4890,7 +4897,7 @@ async function maybeRecoverLostPanelRetrigger(
   await markPendingPrPanelRetrigger(env, args.repoFullName, args.prNumber, args.headSha);
   const job: JobMessage = {
     type: "agent-regate-pr",
-    deliveryId: `panel-retrigger-recovery:${args.repoFullName}#${args.prNumber}`,
+    deliveryId: deliveryIdFor("panelRetriggerRecovery", `${args.repoFullName}#${args.prNumber}`),
     repoFullName: args.repoFullName,
     prNumber: args.prNumber,
     installationId: args.installationId,
@@ -5094,7 +5101,7 @@ async function maybeForceFreshRebase(
       moderationSettings: null,
       // #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), so
       // the decision-record path inside the executor is never actually exercised for it.
-      decisionRecord: { configDigest: await contentDigest(settings) },
+      decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
     },
     [
       {
@@ -15229,7 +15236,7 @@ async function maybeThrottleReviewNagPing(
       expectedBaseRef: null,
       // #9134: this review-nag close previously wrote NO decision record at all -- exactly the kind of
       // contributor-disputable close the issue flagged as biasing the risk-control calibration join.
-      decisionRecord: { configDigest: await contentDigest(settings) },
+      decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
     },
     finalPlanned,
   );
@@ -15441,7 +15448,7 @@ async function maybeThrottleMonitoredMentions(
       expectedBaseRef: null,
       // #9134: this review-nag close (the @loopover-mention variant) previously wrote NO decision record at
       // all -- the same gap as its comment-thread-cooldown sibling immediately above.
-      decisionRecord: { configDigest: await contentDigest(settings) },
+      decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
     },
     finalPlanned,
   );