diff --git a/migrations/0204_decision_record_reevaluation.sql b/migrations/0204_decision_record_reevaluation.sql new file mode 100644 index 0000000000..94200f6dce --- /dev/null +++ b/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 +-- `: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); diff --git a/scripts/check-regate-sort-key.ts b/scripts/check-regate-sort-key.ts index 67587b1967..265e89a61c 100644 --- a/scripts/check-regate-sort-key.ts +++ b/scripts/check-regate-sort-key.ts @@ -30,7 +30,9 @@ const PRODUCER_SCAN_CEILING_LINES = 60; */ const ALLOWED_OMISSIONS: ReadonlyMap = 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, ""]; diff --git a/src/api/routes.ts b/src/api/routes.ts index bfb843d314..c67839c292 100644 --- a/src/api/routes.ts +++ b/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, diff --git a/src/queue/delivery-id.ts b/src/queue/delivery-id.ts new file mode 100644 index 0000000000..0547f675bf --- /dev/null +++ b/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`, 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; +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index bf2cc16b40..46cfd4453c 100644 --- a/src/queue/processors.ts +++ b/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, ); diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index 21e7997ea8..6855079a48 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -45,6 +45,7 @@ // - `ciState` is populated from the live CI aggregate already in scope at the call site (was hardcoded null). // #9135 (also v4): `divertedByHoldout` records when the randomized close-audit holdout (#8831) converted this // decision's plan from a heuristic close to a hold — see that field's own doc comment. +import { deliveryIdOrigin, type DeliveryIdOrigin } from "../queue/delivery-id"; import { errorMessage, nowIso } from "../utils/json"; import { retentionCutoffIsoForTable } from "../db/retention"; @@ -193,7 +194,124 @@ export async function buildDecisionRecord( * swallowed failure) so a caller needing to key a private sibling row (e.g. decision-replay.ts's replay * input) targets the SAME row this call produced, including a supersession's revisioned id. */ -export async function persistDecisionRecord(env: Env, record: DecisionRecord, recordDigest: string, attempts = 3): Promise { +/** + * Why a head SHA was evaluated more than once (#9742). + * + * A CLOSED set, because the point of recording it is that an outsider can count re-evaluations by cause + * without interpreting free text. A new head SHA (force-push, new commits) is NOT a re-evaluation and needs + * no code -- that path is a fresh verdict by definition. + */ +export const REEVALUATION_REASONS = [ + /** Routine scheduled re-gate: the periodic sweep, or a sibling PR's churn waking this one. No new + * information is claimed -- this is by far the highest-volume cause, and naming it is what keeps + * the other four meaningful rather than making every repeat verdict look like an incident. */ + "scheduled_recheck", + /** The prior evaluation did not complete or produced an unusable result. */ + "pipeline_error", + /** Repo configuration or calibrated policy changed, so the prior verdict was computed under rules that no + * longer apply. */ + "config_change", + /** A maintainer explicitly asked for the PR to be re-gated. */ + "maintainer_request", + /** External state the verdict depended on (CI, upstream) settled differently after the fact. */ + "upstream_state_change", +] as const; + +export type ReevaluationReason = (typeof REEVALUATION_REASONS)[number]; + +export function isReevaluationReason(value: unknown): value is ReevaluationReason { + return typeof value === "string" && (REEVALUATION_REASONS as readonly string[]).includes(value); +} + +/** + * Which cause each synthetic delivery-id origin represents (#9742). + * + * `Record` deliberately: adding a producer to DELIVERY_ID_PREFIXES without + * saying why its re-evaluations happen is a build failure, not a silently-wrong verdict record. This + * is the whole reason the prefixes were consolidated into one module. + */ +export const REEVALUATION_REASON_BY_ORIGIN: Record = { + regateSweep: "scheduled_recheck", + regateRepair: "pipeline_error", + manualRegate: "maintainer_request", + backlogConvergence: "pipeline_error", + panelRetriggerRecovery: "maintainer_request", + linkedIssueVerify: "upstream_state_change", + reconcile: "pipeline_error", + surfaceWithoutDisposition: "pipeline_error", +}; + +/** + * The re-evaluation reason a job's delivery id implies. + * + * A raw GitHub delivery id (no synthetic prefix) means a real event on the PR moved something the + * verdict depends on -- CI settling, a label changing, a sibling merging -- which is + * `upstream_state_change`. Mechanical, so no call site has to judge its own cause. + */ +export function deriveReevaluationReason(deliveryId: string | null | undefined): ReevaluationReason { + const origin = deliveryIdOrigin(deliveryId); + return origin === null ? "upstream_state_change" : REEVALUATION_REASON_BY_ORIGIN[origin]; +} + +/** A re-evaluation's provenance: why it happened, and which verdict it supersedes. */ +export type ReevaluationContext = { + reason: ReevaluationReason; + /** The `decision_records.id` this supersedes. Resolved by the writer when absent. */ + supersedesRecordId?: string | undefined; + /** WHO caused it, when that is a person: the operator behind a manual re-gate, the maintainer who + * ran the review command. Absent for the machine-paced causes, which is most of them. */ + actor?: string | null | undefined; +}; + +/** Thrown when a repeat evaluation of a head SHA arrives without declaring why (#9742). */ +export class UndeclaredReevaluationError extends Error { + constructor(readonly target: string, readonly priorCount: number) { + super( + `Refusing to write a repeat verdict for ${target}: this head SHA already has ${priorCount} verdict(s), so the write must declare a re-evaluation reason (one of ${REEVALUATION_REASONS.join(", ")}). A new head SHA needs no reason.`, + ); + this.name = "UndeclaredReevaluationError"; + } +} + +/** A resolved re-evaluation: the declared reason and the record it supersedes. Null for a first evaluation. */ +type ResolvedReevaluation = { reason: ReevaluationReason; supersedesRecordId: string; actor: string | null }; + +/** + * The whole re-evaluation decision in one place (#9742): a first evaluation resolves to null and needs no + * reason, a repeat one must declare a valid reason or the write is refused. + * + * Separate from the write so the invariant is stated once and `isReevaluationReason` narrows the reason to + * `ReevaluationReason` for the caller -- the row's two columns are then either both set or both null, with + * no third state to represent or test for. + */ +function resolveReevaluation( + priorCount: number, + baseId: string, + context: ReevaluationContext | undefined, + target: string, +): ResolvedReevaluation | null { + if (priorCount === 0) return null; + const reason = context?.reason; + if (!isReevaluationReason(reason)) throw new UndeclaredReevaluationError(target, priorCount); + return { + reason, + // Whatever the caller names, else the immediately-prior revision, which `:revN` numbering makes derivable. + supersedesRecordId: context?.supersedesRecordId ?? (priorCount === 1 ? baseId : `${baseId}:rev${priorCount}`), + // Trimmed to null rather than stored as "" so "no actor" is one value, not two. + actor: context?.actor?.trim() ? context.actor.trim() : null, + }; +} + +export async function persistDecisionRecord( + env: Env, + record: DecisionRecord, + recordDigest: string, + attempts = 3, + /** #9742: required when this head SHA already carries a verdict. Absent on a first evaluation, which is + * every ordinary write. The check lives HERE, at the ledger-write layer, so no caller can bypass it by + * writing the row itself. */ + reevaluation?: ReevaluationContext | undefined, +): Promise { const baseId = `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250); try { for (let attempt = 1; ; attempt += 1) { @@ -203,13 +321,17 @@ export async function persistDecisionRecord(env: Env, record: DecisionRecord, re /* v8 ignore next -- defensive: a bare COUNT(*) always returns exactly one row (even {n: 0} against an * empty table); the `?? 0` only satisfies .first()'s optional-by-signature TS return type. */ const priorCount = prior?.n ?? 0; + // #9742: a repeat evaluation of the SAME head SHA must say why. Without this, "evaluated once" and + // "evaluated three times and one result was kept" are indistinguishable in the public record. A new + // head SHA never reaches here with priorCount > 0, so the ordinary path is untouched. + const reevaluated = resolveReevaluation(priorCount, baseId, reevaluation, `${record.repoFullName}#${record.pullNumber}@${record.headSha}`); const id = priorCount === 0 ? baseId : `${baseId}:rev${priorCount + 1}`; try { await env.DB.prepare( - `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)`, + `INSERT INTO decision_records (id, repo_full_name, pull_number, head_sha, action, reason_code, record_digest, record_json, created_at, reevaluation_reason, supersedes_record_id, reevaluation_actor) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) - .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt) + .bind(id, record.repoFullName.slice(0, 200), record.pullNumber, record.headSha, record.action, record.reasonCode.slice(0, 200), recordDigest, canonicalJson(record), record.decidedAt, reevaluated?.reason ?? null, reevaluated?.supersedesRecordId ?? null, reevaluated?.actor ?? null) .run(); } catch (error) { if (attempt >= attempts) throw error; @@ -243,6 +365,11 @@ export async function persistDecisionRecord(env: Env, record: DecisionRecord, re return id; } } catch (error) { + // #9742: a REFUSED re-evaluation is a policy answer, not a persistence failure, and must not be + // flattened into the same `null` every transient D1 fault returns -- a caller that cannot tell them + // apart cannot honour the refusal. Everything else keeps the historical swallow: a decision record + // failing to persist must never break the review that produced it. + if (error instanceof UndeclaredReevaluationError) throw error; console.warn(JSON.stringify({ event: "decision_record_persist_error", target: `${record.repoFullName}#${record.pullNumber}`, message: errorMessage(error).slice(0, 160) })); return null; } diff --git a/src/review/pending-closure-watchdog.ts b/src/review/pending-closure-watchdog.ts index 32f03dcbf5..9ac7cd4820 100644 --- a/src/review/pending-closure-watchdog.ts +++ b/src/review/pending-closure-watchdog.ts @@ -1,5 +1,6 @@ import { countRecentAuditEventsForActorAndTarget, getPullRequest, listAuditEventsByType, recordAuditEvent } from "../db/repositories"; import { errorMessage } from "../utils/json"; +import { deliveryIdFor } from "../queue/delivery-id"; /** * #9031 — rescue a PR stranded mid-way through the flag-then-close double-check. @@ -95,7 +96,7 @@ export async function sweepStrandedPendingClosures(env: Env, nowMs: number = Dat const verifyJob = { type: "recapture-preview" as const, - deliveryId: `linked-issue-verify:${repoFullName}#${pullNumber}`, + deliveryId: deliveryIdFor("linkedIssueVerify", `${repoFullName}#${pullNumber}`), repoFullName, prNumber: pullNumber, installationId, diff --git a/src/review/pr-reconciliation.ts b/src/review/pr-reconciliation.ts index d2050cc32c..e18c277214 100644 --- a/src/review/pr-reconciliation.ts +++ b/src/review/pr-reconciliation.ts @@ -21,6 +21,7 @@ import { incr } from "../selfhost/metrics"; import type { JobMessage } from "../types"; import { errorMessage } from "../utils/json"; import { isConvergenceRepoAllowed, listConvergenceRepos } from "./cutover-gate"; +import { deliveryIdFor } from "../queue/delivery-id"; /** A manifest-sourced enable override (#6558 / #6275) -- the top-level `prReconciliation` block of the * loopover self-repo's `.loopover.yml` (see FocusManifestPrReconciliationConfig). Distinct from the @@ -132,7 +133,7 @@ async function catchUpMissingPullRequest(env: Env, repoFullName: string, install // #9499: prCreatedAt is what jobClaimSortKey uses to drain contributor work oldest-first. Omitting it // falls back to LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber (~9.5e11), which sorts AHEAD of every real // 2026 PR (~1.78e12) -- so a reconciliation job silently jumped the whole queue. - const message: JobMessage = { type: "agent-regate-pr", deliveryId: `reconcile:${repoFullName}#${prNumber}`, repoFullName, prNumber, installationId, prCreatedAt: live.created_at }; + const message: JobMessage = { type: "agent-regate-pr", deliveryId: deliveryIdFor("reconcile", `${repoFullName}#${prNumber}`), repoFullName, prNumber, installationId, prCreatedAt: live.created_at }; await env.JOBS.send(message); } catch (error) { console.error(JSON.stringify({ level: "error", event: "open_pr_reconciliation_catch_up_failed", repository: repoFullName, prNumber, message: errorMessage(error).slice(0, 200) })); diff --git a/src/review/surface-disposition-reconciler.ts b/src/review/surface-disposition-reconciler.ts index b82ea15a7d..cd69c0ffa1 100644 --- a/src/review/surface-disposition-reconciler.ts +++ b/src/review/surface-disposition-reconciler.ts @@ -1,5 +1,6 @@ import { recordAuditEvent } from "../db/repositories"; import { errorMessage } from "../utils/json"; +import { deliveryIdFor } from "../queue/delivery-id"; /** * #8997 — rescue a PR left wearing a decisive panel with no matching disposition. @@ -79,7 +80,7 @@ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number for (const row of rows) { const sent = await env.JOBS.send({ type: "agent-regate-pr", - deliveryId: `surface-without-disposition:${row.repoFullName}#${row.number}#${row.headSha}`, + deliveryId: deliveryIdFor("surfaceWithoutDisposition", `${row.repoFullName}#${row.number}#${row.headSha}`), repoFullName: row.repoFullName, prNumber: row.number, installationId: row.installationId, diff --git a/src/selfhost/queue-common.ts b/src/selfhost/queue-common.ts index d5acfc62b7..fd734ec9ad 100644 --- a/src/selfhost/queue-common.ts +++ b/src/selfhost/queue-common.ts @@ -16,6 +16,7 @@ import { import { githubWebhookCoalesceKey } from "../github/webhook-coalesce"; import type { GitHubWebhookPayload, JobMessage } from "../types"; import { extractPayloadType } from "./audit"; +import { deliveryIdOrigin } from "../queue/delivery-id"; const DEFAULT_RATE_LIMIT_JITTER_MS = 5 * 60_000; const DEFAULT_STARTUP_JITTER_MS = 3 * 60_000; @@ -196,7 +197,7 @@ export function isGitHubBudgetBackgroundJob(message: JobMessage): boolean { // as background admission and parked behind a conservative maintenance floor even though they were reconciling // a live contributor PR someone was waiting on). export function isScheduledRegateSweepJob(deliveryId: string | null | undefined): boolean { - return typeof deliveryId === "string" && deliveryId.startsWith("regate-sweep:"); + return deliveryIdOrigin(deliveryId) === "regateSweep"; } export function buildSelfHostQueueSnapshot( diff --git a/src/selfhost/queue-fairness.ts b/src/selfhost/queue-fairness.ts index 0e21b16fc7..505914e17a 100644 --- a/src/selfhost/queue-fairness.ts +++ b/src/selfhost/queue-fairness.ts @@ -7,6 +7,8 @@ // which repo to serve next — the queue backends (sqlite-queue.ts / pg-queue.ts) consult them before falling // back to the existing unscoped foreground claim. Pure + deterministic; no wall-clock, no hidden state. +import { DELIVERY_ID_PREFIXES } from "../queue/delivery-id"; + export type ForegroundLane = "backlog" | "fresh" | null; const FRESH_PULL_REQUEST_ACTIONS = new Set(["opened", "reopened", "synchronize", "ready_for_review"]); @@ -15,7 +17,7 @@ const FRESH_PULL_REQUEST_ACTIONS = new Set(["opened", "reopened", "synchronize", // selfhost/backlog-convergence.ts) — distinct from sweep-originated (`regate-sweep:`) or manual // (`manual-regate:`) origins, which are intentionally left unclassified (lane `null`): their relative // ordering against everything else is unaffected by this fairness mechanism. -const BACKLOG_CONVERGENCE_DELIVERY_PREFIX = "backlog-convergence:"; +const BACKLOG_CONVERGENCE_DELIVERY_PREFIX = DELIVERY_ID_PREFIXES.backlogConvergence; /** * Classify a job's foreground fairness lane from its type + raw payload — `"fresh"` for a webhook-driven PR diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 6d46c72cee..af7e862fa9 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -45,7 +45,7 @@ import { incr } from "../selfhost/metrics"; import { MERGE_TRAIN_MAX_WAIT_MS, shouldWaitForOlderSiblings } from "../review/merge-train"; import { capturePostHogError } from "../selfhost/posthog"; import { claimContributorCapLock, releaseContributorCapLock } from "../queue/transient-locks"; -import { buildDecisionRecord, persistDecisionRecord, type DecisionRecord } from "../review/decision-record"; +import { buildDecisionRecord, persistDecisionRecord, type DecisionRecord, type ReevaluationContext } from "../review/decision-record"; // The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured // autonomy (the config IS the authorization; there is no human commenter to authorize, unlike #824). @@ -289,6 +289,13 @@ export type DecisionRecordContext = * below when a caller has nothing richer to say. Compute via `contentDigest(settings)` * (decision-record.ts) the SAME way the gate-evaluation call site already does. */ configDigest: string; + /** #9742: WHY this evaluation is running, for the case where the head SHA already carries a + * verdict. REQUIRED, and deliberately not defaulted: a repeat verdict whose cause nobody + * declared is exactly what this invariant exists to refuse, so a future call site must answer + * it at the type level rather than inherit a plausible-looking guess. Callers driven by a job + * derive it with `deriveReevaluationReason(deliveryId)`; a human-driven one names its own + * cause. Ignored entirely on a first evaluation, which is most writes. */ + reevaluation: ReevaluationContext; gatePack?: string | null | undefined; ciState?: string | null | undefined; baseSha?: string | null | undefined; @@ -358,7 +365,7 @@ async function recordCompletedDecision(env: Env, ctx: AgentActionExecutionContex aiConfidence: dr.aiConfidence ?? null, salvageability: dr.salvageability ?? null, }); - const recordId = await persistDecisionRecord(env, record, recordDigest); + const recordId = await persistDecisionRecord(env, record, recordDigest, 3, dr.reevaluation); if (recordId !== null && dr.afterPersist) await dr.afterPersist(recordId, record); } diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 521d46d5f3..593f604395 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -551,7 +551,13 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // generic policy_close: reasonCode derivation still applies here — coarser than the immediate // gate-evaluation path's blockerClass-based reasonCode for a replayed HEURISTIC close (this path has no // `gate` object to re-derive that from), an accepted, documented gap rather than a re-run evaluation. - decisionRecord: { configDigest: await contentDigest(settings) }, + // #9742: the one path whose re-evaluation cause is a named person rather than a derived one. A + // staged action executes when a human accepts it, at a head SHA that may already carry the + // verdict staged against -- so the repeat verdict is attributed to whoever accepted. + decisionRecord: { + configDigest: await contentDigest(settings), + reevaluation: { reason: "maintainer_request", actor: input.decidedBy }, + }, }, plan, ); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index a49cb9997e..e7ad47e0cf 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -108,7 +108,7 @@ function ctx(over: Partial = {}): AgentActionExecut installationPermissions: { pull_requests: "write", contents: "write", issues: "write" }, // #9134: required on every context — default keeps every pre-existing test byte-identical; tests that // care about the decision-record side effect itself override this explicitly. - decisionRecord: { configDigest: "test-config-digest" }, + decisionRecord: { configDigest: "test-config-digest", reevaluation: { reason: "scheduled_recheck" } }, ...over, }; } @@ -2669,7 +2669,7 @@ describe("decision record emission is structural, not per-call-site (#9134)", () // "assign" has no entry in ctx()'s default autonomy map (unlike the other six classes) -- every OTHER // "LIVE assign" test in this file grants it explicitly the same way. const autonomyOverride = actionClass === "assign" ? { assign: "auto" as const } : undefined; - const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" }, ...(autonomyOverride ? { autonomy: autonomyOverride } : {}) }), [action]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", reevaluation: { reason: "scheduled_recheck" } }, ...(autonomyOverride ? { autonomy: autonomyOverride } : {}) }), [action]); expect(outcomes[0]?.outcome).toBe("completed"); const { records, ledgerRows } = await decisionRecordCount(env); const shouldRecord = actionClass === "merge" || actionClass === "close"; @@ -2680,7 +2680,7 @@ describe("decision record emission is structural, not per-call-site (#9134)", () it("a completed merge persists a record whose action/configDigest/reasonCode reflect the ctx, chained into the ledger", async () => { const env = createTestEnv({}); - await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-abc", gatePack: "oss-anti-slop", reasonCode: "custom_reason" } }), [merge]); + await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-abc", reevaluation: { reason: "scheduled_recheck" }, gatePack: "oss-anti-slop", reasonCode: "custom_reason" } }), [merge]); const row = await env.DB.prepare("select action, reason_code, record_json from decision_records").first<{ action: string; reason_code: string; record_json: string }>(); expect(row).toMatchObject({ action: "merge", reason_code: "custom_reason" }); const parsed = JSON.parse(row!.record_json) as { configDigest: string; gatePack: string | null }; @@ -2693,7 +2693,7 @@ describe("decision record emission is structural, not per-call-site (#9134)", () it("afterPersist is called once with the persisted record's id + body — the hook a richer caller (e.g. a decision-replay input) uses to key its own sibling row", async () => { const env = createTestEnv({}); const afterPersist = vi.fn(async (_recordId: string, _record: DecisionRecord) => undefined); - await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", afterPersist } }), [close]); + await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", reevaluation: { reason: "scheduled_recheck" }, afterPersist } }), [close]); expect(afterPersist).toHaveBeenCalledTimes(1); const [recordId, record] = afterPersist.mock.calls[0]!; expect(recordId).toMatch(/^record:owner\/repo#7@/); @@ -2702,7 +2702,7 @@ describe("decision record emission is structural, not per-call-site (#9134)", () it("a NON-completed merge/close (denied) emits NO decision record — only a real, completed mutation counts", async () => { const env = createTestEnv({}); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" }, agentPaused: true }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", reevaluation: { reason: "scheduled_recheck" } }, agentPaused: true }), [merge]); expect(outcomes[0]?.outcome).toBe("denied"); expect((await decisionRecordCount(env)).records).toBe(0); }); @@ -2722,7 +2722,7 @@ describe("decision record emission is structural, not per-call-site (#9134)", () if (sql.includes("INSERT INTO decision_records")) throw new Error("db down"); return realPrepare(sql); }); - const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest" } }), [merge]); + const outcomes = await executeAgentMaintenanceActions(env, ctx({ decisionRecord: { configDigest: "cfg-digest", reevaluation: { reason: "scheduled_recheck" } } }), [merge]); expect(outcomes[0]?.outcome).toBe("completed"); expect(mergePullRequest).toHaveBeenCalled(); vi.restoreAllMocks(); diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 82be71643f..d7940998f9 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -101,7 +101,7 @@ function ctx(over: Partial = {}): AgentActionExecut agentDryRun: false, installationPermissions: { contents: "write", pull_requests: "write", issues: "write" }, // #9134: required on every context — default keeps every pre-existing test byte-identical. - decisionRecord: { configDigest: "test-config-digest" }, + decisionRecord: { configDigest: "test-config-digest", reevaluation: { reason: "scheduled_recheck" } }, ...over, }; } diff --git a/test/unit/decision-record.test.ts b/test/unit/decision-record.test.ts index b1f0fd9b27..654e7d9564 100644 --- a/test/unit/decision-record.test.ts +++ b/test/unit/decision-record.test.ts @@ -6,10 +6,16 @@ import { DECISION_RECORD_SCHEMA_VERSION, parseLedgerContentWaiver, persistDecisionRecord, + deriveReevaluationReason, + REEVALUATION_REASON_BY_ORIGIN, + REEVALUATION_REASONS, + UndeclaredReevaluationError, + isReevaluationReason, renderDecisionRecordSection, sha256Hex, type DecisionRecord, } from "../../src/review/decision-record"; +import { DELIVERY_ID_ORIGINS, DELIVERY_ID_PREFIXES, deliveryIdFor, deliveryIdOrigin } from "../../src/queue/delivery-id"; import { appendDecisionLedger, LEDGER_GENESIS_HASH, loadDecisionLedgerTip, loadDecisionRecordCollapsible, loadPublicDecisionRecord, verifyDecisionLedger } from "../../src/review/decision-record"; import { createTestEnv } from "../helpers/d1"; @@ -109,7 +115,9 @@ describe("buildDecisionRecord / persistDecisionRecord", () => { // documented extract query) already expects. expect(firstId).toBe(`record:o/r#7@abc1234def`); const second = await buildDecisionRecord(recordInput({ action: "close", reasonCode: "policy_close:contributor_cap", decidedAt: "2026-07-26T01:00:00Z" } as never)); - const secondId = await persistDecisionRecord(env, second.record, second.recordDigest); + // #9742: a repeat evaluation of the same head SHA must now DECLARE why. The revisioning this test pins + // is unchanged; only the requirement to say why is new. + const secondId = await persistDecisionRecord(env, second.record, second.recordDigest, 3, { reason: "config_change" }); expect(secondId).toBe(`record:o/r#7@abc1234def:rev2`); const rows = ( await env.DB.prepare("SELECT id, action, reason_code, record_digest, record_json FROM decision_records ORDER BY id").all<{ id: string; action: string; reason_code: string; record_digest: string; record_json: string }>() @@ -133,8 +141,8 @@ describe("buildDecisionRecord / persistDecisionRecord", () => { const two = await buildDecisionRecord(recordInput({ decidedAt: "2026-07-26T01:00:00Z" } as never)); const three = await buildDecisionRecord(recordInput({ decidedAt: "2026-07-26T02:00:00Z" } as never)); const idOne = await persistDecisionRecord(env, one.record, one.recordDigest); - const idTwo = await persistDecisionRecord(env, two.record, two.recordDigest); - const idThree = await persistDecisionRecord(env, three.record, three.recordDigest); + const idTwo = await persistDecisionRecord(env, two.record, two.recordDigest, 3, { reason: "config_change" }); + const idThree = await persistDecisionRecord(env, three.record, three.recordDigest, 3, { reason: "config_change" }); expect([idOne, idTwo, idThree]).toEqual([`record:o/r#7@abc1234def`, `record:o/r#7@abc1234def:rev2`, `record:o/r#7@abc1234def:rev3`]); }); @@ -316,7 +324,7 @@ describe("loadPublicDecisionRecord (#9123)", () => { const first = await buildDecisionRecord(recordInput({ action: "merge", decidedAt: "2026-07-26T00:00:00Z" } as never)); await persistDecisionRecord(env, first.record, first.recordDigest); const second = await buildDecisionRecord(recordInput({ action: "close", decidedAt: "2026-07-26T01:00:00Z" } as never)); - await persistDecisionRecord(env, second.record, second.recordDigest); + await persistDecisionRecord(env, second.record, second.recordDigest, 3, { reason: "config_change" }); const published = await loadPublicDecisionRecord(env, "o/r", 7); expect(published!.recordDigest).toBe(second.recordDigest); expect(published!.record.action).toBe("close"); @@ -357,9 +365,11 @@ describe("loadDecisionLedgerTip (#9274)", () => { }); describe("decision ledger (#8837)", () => { - const persist = async (env: Env, pull: number, action = "close") => { + /** #9742: `reevaluation` is threaded so a helper that deliberately re-persists the SAME target can say + * why, exactly as a real re-evaluation must. A first persist passes none, like every ordinary write. */ + const persist = async (env: Env, pull: number, action = "close", reevaluation?: { reason: "config_change" }) => { const { record, recordDigest } = await buildDecisionRecord(recordInput({ pullNumber: pull, action })); - await persistDecisionRecord(env, record, recordDigest); + await persistDecisionRecord(env, record, recordDigest, 3, reevaluation); return recordDigest; }; @@ -368,7 +378,7 @@ describe("decision ledger (#8837)", () => { await persist(env, 1); await persist(env, 2); // A rewrite of the SAME record id appends a THIRD row — supersessions are visible history. - await persist(env, 1, "merge"); + await persist(env, 1, "merge", { reason: "config_change" }); const rows = (await env.DB.prepare("SELECT seq, prev_hash AS prevHash, row_hash AS rowHash FROM decision_ledger ORDER BY seq").all<{ seq: number; prevHash: string; rowHash: string }>()).results!; expect(rows.map((r) => r.seq)).toEqual([1, 2, 3]); expect(rows[0]!.prevHash).toBe(LEDGER_GENESIS_HASH); @@ -823,3 +833,201 @@ describe("content waiver applied by verifyDecisionLedger (#9850)", () => { expect(verified).toMatchObject({ ok: true, waivedContentMismatches: 0, contentWaiver: { fromSeq: 2, toSeq: 3 } }); }); }); + +// #9742: a verdict's integrity was provable, its UNIQUENESS was not -- nothing distinguished "evaluated +// once" from "evaluated three times and one result was kept". The enforcement lives at the ledger-write +// layer so no caller can bypass it by writing the row itself. +describe("verdict immutability per head SHA (#9742)", () => { + const target = (over: Record = {}) => recordInput({ action: "merge", reasonCode: "success", ...over } as never); + + it("REFUSES a repeat evaluation of the same head SHA that does not say why", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + + const repeat = await buildDecisionRecord(target({ action: "close", decidedAt: "2026-07-29T01:00:00Z" })); + await expect(persistDecisionRecord(env, repeat.record, repeat.recordDigest)).rejects.toThrow(UndeclaredReevaluationError); + // And nothing was written: a refused re-evaluation must not leave a partial row behind. + const rows = (await env.DB.prepare("SELECT id FROM decision_records").all<{ id: string }>()).results!; + expect(rows).toHaveLength(1); + }); + + it("rejects a reason outside the closed set, so the dimension cannot be widened by free text", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + const repeat = await buildDecisionRecord(target({ action: "close", decidedAt: "2026-07-29T01:00:00Z" })); + await expect( + persistDecisionRecord(env, repeat.record, repeat.recordDigest, 3, { reason: "because i said so" as never }), + ).rejects.toThrow(UndeclaredReevaluationError); + }); + + it("records a reason-coded re-run as a LINKED chain: both retained, the later naming what it supersedes", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + const firstId = await persistDecisionRecord(env, first.record, first.recordDigest); + const second = await buildDecisionRecord(target({ action: "close", decidedAt: "2026-07-29T01:00:00Z" })); + const secondId = await persistDecisionRecord(env, second.record, second.recordDigest, 3, { reason: "pipeline_error" }); + + const rows = ( + await env.DB.prepare( + "SELECT id, reevaluation_reason AS reason, supersedes_record_id AS supersedes FROM decision_records ORDER BY id", + ).all<{ id: string; reason: string | null; supersedes: string | null }>() + ).results!; + expect(rows).toHaveLength(2); + // The FIRST evaluation carries neither -- it superseded nothing and needed no reason. + expect(rows.find((row) => row.id === firstId)).toMatchObject({ reason: null, supersedes: null }); + expect(rows.find((row) => row.id === secondId)).toMatchObject({ reason: "pipeline_error", supersedes: firstId }); + }); + + it("chains a THIRD evaluation to the second, not back to the first", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + const second = await buildDecisionRecord(target({ action: "close", decidedAt: "2026-07-29T01:00:00Z" })); + const secondId = await persistDecisionRecord(env, second.record, second.recordDigest, 3, { reason: "pipeline_error" }); + const third = await buildDecisionRecord(target({ action: "hold", decidedAt: "2026-07-29T02:00:00Z" })); + const thirdId = await persistDecisionRecord(env, third.record, third.recordDigest, 3, { reason: "config_change" }); + + const row = await env.DB.prepare("SELECT supersedes_record_id AS supersedes FROM decision_records WHERE id = ?") + .bind(thirdId) + .first<{ supersedes: string }>(); + expect(row?.supersedes).toBe(secondId); + }); + + it("honours an explicitly-named superseded record over the derived one", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + const firstId = await persistDecisionRecord(env, first.record, first.recordDigest); + const second = await buildDecisionRecord(target({ action: "close", decidedAt: "2026-07-29T01:00:00Z" })); + await persistDecisionRecord(env, second.record, second.recordDigest, 3, { reason: "maintainer_request" }); + const third = await buildDecisionRecord(target({ action: "hold", decidedAt: "2026-07-29T02:00:00Z" })); + const thirdId = await persistDecisionRecord(env, third.record, third.recordDigest, 3, { + reason: "upstream_state_change", + supersedesRecordId: firstId!, + }); + + const row = await env.DB.prepare("SELECT supersedes_record_id AS supersedes FROM decision_records WHERE id = ?") + .bind(thirdId) + .first<{ supersedes: string }>(); + expect(row?.supersedes).toBe(firstId); + }); + + it("leaves a NEW head SHA completely unaffected — that is a fresh verdict, not a re-evaluation", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + // A force-push moves the head; no reason code is required or recorded. + const pushed = await buildDecisionRecord(target({ headSha: "def5678abc", decidedAt: "2026-07-29T01:00:00Z" })); + const pushedId = await persistDecisionRecord(env, pushed.record, pushed.recordDigest); + expect(pushedId).toBe("record:o/r#7@def5678abc"); + + const row = await env.DB.prepare("SELECT reevaluation_reason AS reason, supersedes_record_id AS supersedes FROM decision_records WHERE id = ?") + .bind(pushedId) + .first<{ reason: string | null; supersedes: string | null }>(); + expect(row).toMatchObject({ reason: null, supersedes: null }); + }); + + it("keeps the reason vocabulary closed and machine-readable", async () => { + // The point of recording it is that an outsider can count re-evaluations BY CAUSE without interpreting + // prose, so the set is closed and each member is a code rather than a sentence. + expect(REEVALUATION_REASONS).toContain("pipeline_error"); + expect(REEVALUATION_REASONS).toContain("config_change"); + for (const reason of REEVALUATION_REASONS) expect(reason).toMatch(/^[a-z][a-z_]*$/); + expect(isReevaluationReason("pipeline_error")).toBe(true); + expect(isReevaluationReason("nope")).toBe(false); + expect(isReevaluationReason(undefined)).toBe(false); + }); + + it("records WHO asked, when a person did, and nothing when a schedule did", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + + const second = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:05:00Z" })); + await persistDecisionRecord(env, second.record, second.recordDigest, 3, { + reason: "maintainer_request", + actor: " JSONbored ", + }); + const third = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:10:00Z" })); + await persistDecisionRecord(env, third.record, third.recordDigest, 3, { reason: "scheduled_recheck" }); + + const rows = await env.DB.prepare( + "SELECT reevaluation_actor AS actor FROM decision_records WHERE reevaluation_reason IS NOT NULL ORDER BY created_at", + ).all<{ actor: string | null }>(); + // Trimmed, and a machine-paced cause never invents a "system" actor to sit beside a real name. + expect(rows.results.map((row) => row.actor)).toEqual(["JSONbored", null]); + }); + + it("treats a blank or absent actor as no actor rather than as an empty name", async () => { + const env = createTestEnv(); + const first = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:00:00Z" })); + await persistDecisionRecord(env, first.record, first.recordDigest); + const second = await buildDecisionRecord(target({ decidedAt: "2026-07-29T00:05:00Z" })); + const id = await persistDecisionRecord(env, second.record, second.recordDigest, 3, { + reason: "maintainer_request", + actor: " ", + }); + + const row = await env.DB.prepare("SELECT reevaluation_actor AS actor FROM decision_records WHERE id = ?") + .bind(id) + .first<{ actor: string | null }>(); + expect(row?.actor).toBeNull(); + }); +}); + +// The cause is DERIVED from the job's delivery id rather than judged at each call site (#9742) -- these +// pin the mapping that makes that mechanical. +describe("deriveReevaluationReason", () => { + it("names the routine scheduled sweep as such", () => { + // By volume this is the dominant cause. If it were not its own reason, every sweep tick past the + // first would have to borrow one of the incident-shaped codes and drown them. + expect(deriveReevaluationReason(deliveryIdFor("regateSweep", "o/r#7"))).toBe("scheduled_recheck"); + }); + + it("treats a repair fan-out as a pipeline error, not routine", () => { + for (const origin of ["regateRepair", "backlogConvergence", "reconcile", "surfaceWithoutDisposition"] as const) { + expect(deriveReevaluationReason(deliveryIdFor(origin, "o/r#7")), origin).toBe("pipeline_error"); + } + }); + + it("attributes an operator-driven re-gate to a maintainer request", () => { + expect(deriveReevaluationReason(deliveryIdFor("manualRegate", "uuid"))).toBe("maintainer_request"); + expect(deriveReevaluationReason(deliveryIdFor("panelRetriggerRecovery", "o/r#7"))).toBe("maintainer_request"); + }); + + it("reads a RAW GitHub delivery id as external state having moved", () => { + // No synthetic prefix means a real event on the PR -- CI settling, a label changing, a sibling merging. + expect(deriveReevaluationReason("f7a1c4e0-1234-4321-9876-0badc0ffee00")).toBe("upstream_state_change"); + expect(deriveReevaluationReason(null)).toBe("upstream_state_change"); + expect(deriveReevaluationReason(undefined)).toBe("upstream_state_change"); + expect(deriveReevaluationReason("")).toBe("upstream_state_change"); + }); + + it("assigns a reason to EVERY origin, and only valid ones", () => { + // The map is `Record`, so a new prefix without a reason is a + // build failure -- this asserts the runtime side of that same guarantee. + for (const origin of DELIVERY_ID_ORIGINS) { + const reason = REEVALUATION_REASON_BY_ORIGIN[origin]; + expect(isReevaluationReason(reason), origin).toBe(true); + expect(deriveReevaluationReason(deliveryIdFor(origin, "o/r#7")), origin).toBe(reason); + } + expect(DELIVERY_ID_ORIGINS.length).toBeGreaterThan(0); + }); + + it("keeps every origin distinguishable: no prefix may be a prefix of another", () => { + // This is what makes a first-match scan unambiguous. If it ever fails, two producers have become + // indistinguishable and one would silently inherit the other's reason -- `regate-sweep:` read as + // `regate-repair:` would file a repair as routine maintenance, the exact distinction this preserves. + for (const a of DELIVERY_ID_ORIGINS) { + for (const b of DELIVERY_ID_ORIGINS) { + if (a === b) continue; + expect(DELIVERY_ID_PREFIXES[a].startsWith(DELIVERY_ID_PREFIXES[b]), `${a} vs ${b}`).toBe(false); + } + } + expect(deliveryIdOrigin(deliveryIdFor("regateRepair", "o/r#7"))).toBe("regateRepair"); + expect(deliveryIdOrigin(deliveryIdFor("regateSweep", "o/r#7"))).toBe("regateSweep"); + expect(deliveryIdOrigin("not-a-known-prefix:o/r#7")).toBeNull(); + expect(deliveryIdOrigin(null)).toBeNull(); + }); +}); diff --git a/test/unit/priority-label-webhook.test.ts b/test/unit/priority-label-webhook.test.ts index 5042fb7327..68833b0aa4 100644 --- a/test/unit/priority-label-webhook.test.ts +++ b/test/unit/priority-label-webhook.test.ts @@ -240,6 +240,77 @@ describe("issues.labeled priority enforcement (#9737)", () => { expect(calls.some((call) => call.method === "DELETE"), "the real label is still found and removed").toBe(true); }); + // Every I/O call on this path is wrapped in a `.catch` that degrades rather than failing the delivery. + // Those handlers are the whole reason a label-mutating rule is safe to run on a webhook -- an escaping + // throw would fail the delivery and take every handler after it on the same event down with it. They are + // tested by making the call actually THROW: a 404 response exercises the ordinary return, not the catch. + it("falls back to the DEFAULT label name when the settings resolver throws", async () => { + const env = await seed(); + vi.spyOn(repositorySettingsModule, "resolveRepositorySettings").mockRejectedValue(new Error("D1 unavailable")); + const calls: Call[] = []; + stubGitHub("read", calls); + + await run(env, labeledPayload(), "d-settings-throw"); + + const removed = calls.find((call) => call.method === "DELETE" && call.url.includes("/labels/")); + expect(decodeURIComponent(removed?.url ?? ""), "an unreadable config still enforces the built-in label").toContain("gittensor:priority"); + }); + + it("FAILS OPEN when the permission read throws rather than answering", async () => { + const env = await seed(); + const calls: Call[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + calls.push({ method: (init?.method ?? "GET").toUpperCase(), url }); + if (url.includes("/access_tokens")) return Response.json({ token: "ghs_test", expires_at: "2099-01-01T00:00:00Z" }); + if (url.includes("/permission")) throw new Error("socket hang up"); + return new Response("not found", { status: 404 }); + }); + + await run(env, labeledPayload(), "d-permission-throws"); + + expect(calls.some((call) => call.method === "DELETE"), "a thrown permission read is not evidence of ineligibility").toBe(false); + }); + + it("still records the enforcement when the label removal and the comment both throw", async () => { + // A GitHub outage mid-enforcement must not fail the whole delivery. + const env = await seed(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + const method = (init?.method ?? "GET").toUpperCase(); + if (url.includes("/access_tokens")) return Response.json({ token: "ghs_test", expires_at: "2099-01-01T00:00:00Z" }); + if (url.includes("/collaborators/") && url.endsWith("/permission")) return Response.json({ permission: "read" }); + if (method === "DELETE" || url.includes("/comments")) throw new Error("GitHub 502"); + if (url.includes("/installation")) return Response.json({ id: 123, account: { login: "JSONbored" } }); + return new Response("not found", { status: 404 }); + }); + + await expect(run(env, labeledPayload(), "d-mutations-throw")).resolves.not.toThrow(); + + const audit = await env.DB.prepare("select outcome from audit_events where event_type = ?") + .bind(PRIORITY_LABEL_ENFORCEMENT_EVENT) + .first<{ outcome: string }>(); + expect(audit?.outcome, "the decision is still on the record even when the mutation failed").toBe("success"); + }); + + it("does not fail the delivery when the ledger writes themselves throw", async () => { + const env = await seed(); + const calls: Call[] = []; + stubGitHub("read", calls); + // The last two `.catch`es: the audit row and the webhook-event row. Both are recording, not acting -- + // losing them must not undo an enforcement that already happened on GitHub. + const realPrepare = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((sql: string) => { + // Drizzle quotes its table names, the raw-SQL writers do not -- match both. + if (/insert\s+(or\s+\w+\s+)?into\s+"?(audit_events|webhook_events)"?/i.test(sql)) throw new Error("D1 write failed"); + return realPrepare(sql); + }); + + await expect(run(env, labeledPayload(), "d-ledger-throw")).resolves.not.toThrow(); + + expect(calls.some((call) => call.method === "DELETE"), "the label was still removed").toBe(true); + }); + it("tolerates an author object with no login", async () => { const env = await seed(); const calls: Call[] = [];