Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions migrations/0204_decision_record_reevaluation.sql
Original file line number Diff line number Diff line change
@@ -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);
8 changes: 5 additions & 3 deletions scripts/check-regate-sort-key.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).",
],
]);
Expand Down Expand Up @@ -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, ""];
Expand Down
3 changes: 2 additions & 1 deletion src/api/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
54 changes: 54 additions & 0 deletions src/queue/delivery-id.ts
Original file line number Diff line number Diff line change
@@ -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;
}
29 changes: 18 additions & 11 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) } },
},
[
{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) } },
},
[
{
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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,
);
Expand Down
Loading