Skip to content

Commit ccb76e7

Browse files
committed
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.
1 parent 68fadf5 commit ccb76e7

15 files changed

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

src/api/routes.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { verifyShotRenderToken } from "../review/visual/shot-render-token";
1212
import { isScreenshotsEnabled } from "../review/visual-wire";
1313
import { buildFindingTaxonomyDocument } from "../review/finding-taxonomy";
1414
import { buildEnrichmentAnalyzersTaxonomyDocument } from "../review/enrichment-analyzers-taxonomy";
15+
import { deliveryIdFor } from "../queue/delivery-id";
1516
import {
1617
GITHUB_OAUTH_STATE_COOKIE,
1718
authenticateInternalToken,
@@ -4423,7 +4424,7 @@ export function createApp() {
44234424
if (typeof repo?.installationId !== "number") return c.json({ error: "repo not installed" }, 404);
44244425
const message: JobMessage = {
44254426
type: "agent-regate-pr",
4426-
deliveryId: `manual-regate:${crypto.randomUUID()}`,
4427+
deliveryId: deliveryIdFor("manualRegate", crypto.randomUUID()),
44274428
repoFullName: repo.fullName,
44284429
prNumber,
44294430
installationId: repo.installationId,

src/queue/delivery-id.ts

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// The delivery-id vocabulary (#9742).
2+
//
3+
// Every `agent-regate-pr` job carries a `deliveryId`. Most are a REAL GitHub webhook delivery id -- the
4+
// event that caused the work. The rest are SYNTHETIC ids minted by a producer that has no webhook to
5+
// point at (a scheduled sweep, a repair fan-out, an operator's manual re-gate), and each of those tags
6+
// itself with a prefix naming its origin.
7+
//
8+
// The prefixes were string literals at each producer with matching `startsWith` reads scattered
9+
// elsewhere. They live here now because the re-evaluation reason a decision record must declare
10+
// (#9742) is DERIVED from them: `REEVALUATION_REASON_BY_ORIGIN` in review/decision-record.ts is typed
11+
// `Record<DeliveryIdOrigin, ReevaluationReason>`, so a new origin added here without a reason assigned
12+
// there fails the build. A verdict whose cause nobody assigned is a verdict nobody can explain.
13+
14+
export const DELIVERY_ID_PREFIXES = {
15+
/** The scheduled re-gate sweep's own per-PR fan-out (#audit-sweep-fanout). */
16+
regateSweep: "regate-sweep:",
17+
/** The sweep's outage-repair fan-out: a PR missing a current-head Gate check or surface publish. */
18+
regateRepair: "regate-repair:",
19+
/** An operator's explicit re-gate through the internal jobs endpoint. */
20+
manualRegate: "manual-regate:",
21+
/** Backlog convergence: an open PR whose surface was never published for its current head. */
22+
backlogConvergence: "backlog-convergence:",
23+
/** Recovery of a panel "Re-run review" click whose webhook delivery was lost. */
24+
panelRetriggerRecovery: "panel-retrigger-recovery:",
25+
/** Pass 2 of flag-then-close: re-verify the linked issue after the pending-closure label landed. */
26+
linkedIssueVerify: "linked-issue-verify:",
27+
/** PR reconciliation repair. */
28+
reconcile: "reconcile:",
29+
/** A published surface with no disposition marker for its head -- a crashed or lost pass. */
30+
surfaceWithoutDisposition: "surface-without-disposition:",
31+
} as const;
32+
33+
/** The producers that mint a synthetic delivery id. A raw GitHub delivery id has no origin. */
34+
export type DeliveryIdOrigin = keyof typeof DELIVERY_ID_PREFIXES;
35+
36+
export const DELIVERY_ID_ORIGINS = Object.keys(DELIVERY_ID_PREFIXES) as readonly DeliveryIdOrigin[];
37+
38+
/** Build a synthetic delivery id, so no producer repeats a prefix literal. */
39+
export function deliveryIdFor(origin: DeliveryIdOrigin, suffix: string): string {
40+
return `${DELIVERY_ID_PREFIXES[origin]}${suffix}`;
41+
}
42+
43+
/**
44+
* Which producer minted this delivery id, or null for a raw GitHub webhook delivery id.
45+
*
46+
* Longest prefix wins, so an origin whose prefix is a prefix of another's still resolves to the more
47+
* specific one rather than to whichever happens to be declared first.
48+
*/
49+
export function deliveryIdOrigin(deliveryId: string | null | undefined): DeliveryIdOrigin | null {
50+
if (typeof deliveryId !== "string") return null;
51+
let match: DeliveryIdOrigin | null = null;
52+
for (const origin of DELIVERY_ID_ORIGINS) {
53+
if (!deliveryId.startsWith(DELIVERY_ID_PREFIXES[origin])) continue;
54+
if (match === null || DELIVERY_ID_PREFIXES[origin].length > DELIVERY_ID_PREFIXES[match].length) {
55+
match = origin;
56+
}
57+
}
58+
return match;
59+
}

src/queue/processors.ts

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -676,7 +676,7 @@ import { isWithinFreshRebaseWindow, type DecisionClockCapture } from "../review/
676676
import { recordVerdictFlip } from "../review/verdict-flip-store";
677677
import { resolveAutomaticCloseConfidence } from "../review/risk-control-wire";
678678
import { maybeApplyCloseAuditHoldout } from "../review/close-audit-holdout";
679-
import { buildDecisionRecord, contentDigest, loadDecisionRecordCollapsible, persistDecisionRecord } from "../review/decision-record";
679+
import { buildDecisionRecord, contentDigest, deriveReevaluationReason, loadDecisionRecordCollapsible, persistDecisionRecord } from "../review/decision-record";
680680
import { neutralHoldReasonCode, nativeGateActionFromConclusion, recordNativeGateDecision } from "../review/parity-wire";
681681
import { recordContributorGateDecision } from "../review/contributor-calibration";
682682
import { recordGateBlockersAndCheckRepeatAlarm } from "../review/rule-repeat-alarm-wire";
@@ -703,6 +703,7 @@ import { sha256Hex } from "../utils/crypto";
703703
import { errorMessage, nowIso, repoParts } from "../utils/json";
704704
import { DISPOSITION_CONSIDERED_EVENT_TYPE } from "../review/surface-disposition-reconciler";
705705
import { maybeSuggestMilestoneMatchForPr } from "../integrations/project-tracker-adapter";
706+
import { deliveryIdFor } from "./delivery-id";
706707

707708
const OFFICIAL_MINER_DETECTION_TTL_MS = 5 * 60 * 1000;
708709
const OFFICIAL_MINER_DETECTION_UNAVAILABLE_TTL_MS = 60 * 1000;
@@ -1660,8 +1661,8 @@ export async function sweepRepoRegate(
16601661
const job: JobMessage = {
16611662
type: "agent-regate-pr",
16621663
deliveryId: isPriorityRepair
1663-
? `regate-repair:${repoFullName}#${pr.number}`
1664-
: `regate-sweep:${repoFullName}#${pr.number}`,
1664+
? deliveryIdFor("regateRepair", `${repoFullName}#${pr.number}`)
1665+
: deliveryIdFor("regateSweep", `${repoFullName}#${pr.number}`),
16651666
repoFullName,
16661667
prNumber: pr.number,
16671668
installationId: sweepInstallationId,
@@ -1952,7 +1953,7 @@ export async function sweepRepoBacklogConvergence(
19521953
candidates.map((pr, index) => {
19531954
const job: JobMessage = {
19541955
type: "agent-regate-pr",
1955-
deliveryId: `backlog-convergence:${repoFullName}#${pr.number}`,
1956+
deliveryId: deliveryIdFor("backlogConvergence", `${repoFullName}#${pr.number}`),
19561957
repoFullName,
19571958
prNumber: pr.number,
19581959
installationId: sweepInstallationId,
@@ -3197,7 +3198,7 @@ async function maybeCloseForContributorCapOnOpen(
31973198
// risk-control calibration set). The generic policy_close:contributor_cap reasonCode derivation
31983199
// (agent-action-executor.ts's defaultDecisionRecordReasonCode) is exactly right here — this path has
31993200
// no gate evaluation to derive a richer blockerClass-based code from.
3200-
decisionRecord: { configDigest: await contentDigest(settings) },
3201+
decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
32013202
},
32023203
planned,
32033204
);
@@ -3835,7 +3836,13 @@ async function runAgentMaintenancePlanAndExecute(
38353836
// #9135: legible on the record's own face — see maybeApplyCloseAuditHoldout's doc comment.
38363837
divertedByHoldout: closeAuditHoldout?.diverted ?? false,
38373838
});
3838-
const recordId = await persistDecisionRecord(env, record, recordDigest);
3839+
// #9742: a repeat verdict for the SAME head SHA must declare why. The cause is derived from this
3840+
// job's own delivery id -- the scheduled sweep, a repair fan-out, an operator's manual re-gate,
3841+
// or (no synthetic prefix) a real webhook event that moved something the verdict depends on. The
3842+
// writer ignores it entirely on a first evaluation, which is the overwhelming majority of writes.
3843+
const recordId = await persistDecisionRecord(env, record, recordDigest, 3, {
3844+
reason: deriveReevaluationReason(deliveryId),
3845+
});
38393846
// #8838: persist the evaluation's own exact inputs beside the record (PRIVATE sibling, migration 0182)
38403847
// so the replay harness can re-derive this decision bit-exactly. Best-effort, like the record itself;
38413848
// the no-replay no-op (synthetic content-lane/bridge evaluations) lives inside the helper. Keyed to the
@@ -4472,7 +4479,7 @@ async function prReadyForReview(
44724479
moderationSettings: null,
44734480
// #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close),
44744481
// so the decision-record path inside the executor is never actually exercised for it.
4475-
decisionRecord: { configDigest: await contentDigest(settings) },
4482+
decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
44764483
},
44774484
[
44784485
{
@@ -4890,7 +4897,7 @@ async function maybeRecoverLostPanelRetrigger(
48904897
await markPendingPrPanelRetrigger(env, args.repoFullName, args.prNumber, args.headSha);
48914898
const job: JobMessage = {
48924899
type: "agent-regate-pr",
4893-
deliveryId: `panel-retrigger-recovery:${args.repoFullName}#${args.prNumber}`,
4900+
deliveryId: deliveryIdFor("panelRetriggerRecovery", `${args.repoFullName}#${args.prNumber}`),
48944901
repoFullName: args.repoFullName,
48954902
prNumber: args.prNumber,
48964903
installationId: args.installationId,
@@ -5094,7 +5101,7 @@ async function maybeForceFreshRebase(
50945101
moderationSettings: null,
50955102
// #9134: required on every ctx, though this path only ever plans `update_branch` (never merge/close), so
50965103
// the decision-record path inside the executor is never actually exercised for it.
5097-
decisionRecord: { configDigest: await contentDigest(settings) },
5104+
decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
50985105
},
50995106
[
51005107
{
@@ -15229,7 +15236,7 @@ async function maybeThrottleReviewNagPing(
1522915236
expectedBaseRef: null,
1523015237
// #9134: this review-nag close previously wrote NO decision record at all -- exactly the kind of
1523115238
// contributor-disputable close the issue flagged as biasing the risk-control calibration join.
15232-
decisionRecord: { configDigest: await contentDigest(settings) },
15239+
decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
1523315240
},
1523415241
finalPlanned,
1523515242
);
@@ -15441,7 +15448,7 @@ async function maybeThrottleMonitoredMentions(
1544115448
expectedBaseRef: null,
1544215449
// #9134: this review-nag close (the @loopover-mention variant) previously wrote NO decision record at
1544315450
// all -- the same gap as its comment-thread-cooldown sibling immediately above.
15444-
decisionRecord: { configDigest: await contentDigest(settings) },
15451+
decisionRecord: { configDigest: await contentDigest(settings), reevaluation: { reason: deriveReevaluationReason(deliveryId) } },
1544515452
},
1544615453
finalPlanned,
1544715454
);

0 commit comments

Comments
 (0)