From cf93ef690b0b62d7081a3b48f6755fe170486618 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:22:16 -0700 Subject: [PATCH] fix(ledger): record WHY a pull request was held, not the gate's conclusion The ledger filed 518 holds across 146 pull requests under `reason_code = "success"`. That is not a reason: deriveDecisionReasonCode ends with the gate conclusion when there is no blocker and no policy close, so a held PR whose gate passed records the conclusion as its cause. A reader of the public proof page's sample records sees the same thing. It also blocks #9729, which must loosen only what clears a backtest, PER PATH. `success` is not a path -- it is at least the seven inputs in MERGE_HOLD_INPUTS, and a clearance run against a bucket conflating them would mean nothing. reason_code is deliberately UNTOUCHED. replayDecision recomputes it and reports a divergence on mismatch, so changing the derivation would make all 518 existing records replay as unreproducible -- a false "this decision cannot be re-derived" about records that are perfectly fine -- and would change a value the proof page publishes. The cause goes in its own column, outside record_json, so digests and the replay contract are untouched. Historical rows stay NULL: we genuinely do not know which input held those 518, which is the whole finding. NO SIGNATURE CHANGE. The obvious route -- widening planAgentMaintenanceActions to return the cause -- churns five production call sites and roughly thirty test ones, and does not fit the data: a hold's defining shape is that NOTHING was planned, so there is no action to carry the reason. Instead the disposition-input construction is extracted into one builder that the planner and the new `resolveHoldCause` both call, so the cause recorded in the ledger is the same expression that suppressed the merge rather than a second opinion about it. Every value that builder needs is a function of the plan input, which is what makes recomputing it outside the planner sound rather than approximate. `derivePrDisposition` now returns `heldBy` -- which inputs held, in the table's declaration order so the recorded cause is deterministic. It was already a `.some(...)` over exactly that set, throwing the answer away; `heldForManualReview` is now derived from it, so the two cannot disagree. Released holds (#9808's cleared guardrail) are excluded, and the unstable mergeable state stays out because it is GitHub's computation rather than one of our declared inputs. Mutation-tested: ignoring released holds fails 1, forcing heldBy empty fails 3. The extraction is behaviour-neutral -- agent-actions' 347 tests pass unchanged. Closes #9991 --- .../0210_decision_records_hold_cause.sql | 21 ++++ src/queue/processors.ts | 25 ++-- src/review/decision-record.ts | 11 +- src/settings/agent-actions.ts | 89 +++++++++++--- src/settings/pr-disposition.ts | 17 ++- test/unit/hold-cause.test.ts | 116 ++++++++++++++++++ 6 files changed, 249 insertions(+), 30 deletions(-) create mode 100644 migrations/0210_decision_records_hold_cause.sql create mode 100644 test/unit/hold-cause.test.ts diff --git a/migrations/0210_decision_records_hold_cause.sql b/migrations/0210_decision_records_hold_cause.sql new file mode 100644 index 0000000000..9668ea62a1 --- /dev/null +++ b/migrations/0210_decision_records_hold_cause.sql @@ -0,0 +1,21 @@ +-- #9991: WHY a pull request was held, so the manual-review surface can be split by cause. +-- +-- 1,679 of 2,429 verdicts on the production Orb are holds, and the largest single bucket -- 518 of them, +-- across 146 pull requests -- carries `reason_code = 'success'`. That is not a reason, it is a fall-through: +-- deriveDecisionReasonCode ends with the gate conclusion when there is no blocker and no policy close, so a +-- held PR whose gate passed records the conclusion as its cause. #9729 must loosen only what clears a +-- backtest, per path, and cannot do that against a bucket conflating the seven inputs in MERGE_HOLD_INPUTS. +-- +-- A SEPARATE COLUMN, NOT reason_code. `replayDecision` recomputes reason_code through the same derivation and +-- reports a DIVERGENCE when it does not match the recorded value (decision-replay.ts). Changing that mapping +-- would make every one of those 518 existing records replay as unreproducible -- a false "this decision +-- cannot be re-derived" about records that are perfectly fine -- and would also change a value the public +-- proof page publishes in its sample records. +-- +-- OUTSIDE record_json for the same reason: that JSON is what recordDigest commits to, so adding a field to it +-- would move the digest of every future record and put an operational analysis field inside a published +-- commitment. This column is written alongside and read only by internal analysis. +-- +-- NULL on historical rows, and left that way deliberately. Backfilling a guess would be worse than the gap: +-- we genuinely do not know which input held those 518, which is the entire finding. +ALTER TABLE decision_records ADD COLUMN hold_cause TEXT; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 1456477d2f..b4bdec09eb 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -290,6 +290,7 @@ import { MAX_REVIEW_NAG_COOLDOWN_DAYS, isProtectedAutomationAuthor, planAgentMaintenanceActions, + resolveHoldCause, planContributorCapClose, resolveAgentDispositionLabels, type AgentActionPlanInput, @@ -3752,8 +3753,9 @@ async function runAgentMaintenancePlanAndExecute( duplicateWinnerEnabled && openDuplicateSiblings.length > 0 ? await resolveScopedLinkedIssueClaimedAt(env, repoFullName, pr, openDuplicateSiblings) : pr.linkedIssueClaimedAt; - const planned = planAgentMaintenanceActions( - buildAgentMaintenancePlanInput({ + // #9991: named, so the finalize site below can recompute the hold cause from the SAME input the planner + // consumed rather than from a second construction that could drift from it. + const agentPlanInput = buildAgentMaintenancePlanInput({ gate, settings, changedPaths, @@ -3783,8 +3785,8 @@ async function runAgentMaintenancePlanAndExecute( scopedLinkedIssueClaimedAt, manualReviewLockContentionResolved: hadPriorLockContentionHold && !aiReviewLockContentionThisPass, decisionNowMs: decisionClock.nowMs, - }), - ); + }); + const planned = planAgentMaintenanceActions(agentPlanInput); // Accuracy circuit-breakers (#self-improve / GAP-4): two INDEPENDENT, fail-open precision breakers, chained. // • MERGE breaker (holdonly:): when set, convert a would-MERGE into a human HOLD before executing. // • CLOSE breaker (closehold:): when set, convert a HEURISTIC would-CLOSE into a human HOLD (the @@ -3956,9 +3958,18 @@ async function runAgentMaintenancePlanAndExecute( // 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), - }); + // #9991: the hold cause, recomputed from the SAME plan input the planner consumed. A hold's defining + // shape is that nothing was planned, so there is no action carrying the reason -- and without this the + // record falls back to the gate conclusion and files a held PR under reason "success". + const holdCause = disposition.actionClass === "hold" ? resolveHoldCause(agentPlanInput).join(",") || null : null; + const recordId = await persistDecisionRecord( + env, + record, + recordDigest, + 3, + { reason: deriveReevaluationReason(deliveryId) }, + holdCause, + ); // #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 diff --git a/src/review/decision-record.ts b/src/review/decision-record.ts index cfd7fdb5cd..5e0b910648 100644 --- a/src/review/decision-record.ts +++ b/src/review/decision-record.ts @@ -297,6 +297,11 @@ export async function persistDecisionRecord( * 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, + /** #9991: which MERGE_HOLD_INPUTS suppressed this PR's merge, comma-joined, or null when none did. + * Deliberately a COLUMN and not a field on `record`: `record` is what `recordDigest` commits to and what + * `replayDecision` re-derives, so putting an operational analysis value in there would move every future + * digest and drag a private field into a published commitment. */ + holdCause?: string | null | undefined, ): Promise { const baseId = `record:${record.repoFullName}#${record.pullNumber}@${record.headSha}`.slice(0, 250); try { @@ -314,10 +319,10 @@ export async function persistDecisionRecord( 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, reevaluation_reason, supersedes_record_id, reevaluation_actor, findings_count) - 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, findings_count, hold_cause) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, ) - .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, record.findingsCount ?? null) + .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, record.findingsCount ?? null, holdCause ?? null) .run(); } catch (error) { if (attempt >= attempts) throw error; diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 0756580bbd..59d3e4b0ff 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -1,7 +1,7 @@ import type { AgentActionClass, AutoMaintainPolicy, AutoMergeMethod, AutonomyPolicy } from "../types"; import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckConclusion } from "../rules/advisory"; import { DEFAULT_AUTO_MAINTAIN_POLICY, autonomyRequiresApproval, isActingAutonomyLevel, resolveAutonomy } from "./autonomy"; -import { assessMergeableState, derivePrDisposition } from "./pr-disposition"; +import { assessMergeableState, derivePrDisposition, type MergeHoldInput, type PrDispositionInput } from "./pr-disposition"; import { changedPathsHittingGuardrail, isGuardrailHit } from "../signals/change-guardrail"; import { AGENT_LABEL_PENDING_CLOSURE } from "../review/linked-issue-hard-rules"; import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; @@ -897,6 +897,75 @@ function maybePlanCopycatLabel(actions: PlannedAgentAction[], input: AgentAction * request-changes; never both merge and close), each entry already filtered to an acting autonomy class. * Ordered least → most irreversible: label, then the review, then the disposition. */ + +/** + * PURE. The {@link PrDispositionInput} the planner feeds {@link derivePrDisposition}, extracted so a caller + * that needs the HOLD CAUSE can recompute it from the same plan input (#9991). + * + * Extracted rather than threaded out of `planAgentMaintenanceActions`: that function returns + * `PlannedAgentAction[]`, and a hold's defining shape is that NOTHING was planned -- so there is no action to + * hang the cause on, and widening the return type would churn five production call sites and roughly thirty + * test ones for a field only the ledger write needs. One shared builder, called twice, is cheaper and keeps + * the two answers derived from the same expression rather than from two constructions that can drift. + * + * `derived` carries the four values the planner computes on its way here; every one is itself a function of + * `input`, which is what makes recomputing this outside the planner sound rather than approximate. + */ +function buildPrDispositionInput( + input: AgentActionPlanInput, + derived: { reviewGood: boolean; guardrailHit: boolean; guardrailEscalationCleared: boolean; closeActing: boolean }, +): PrDispositionInput { + return { + mergeableState: input.pr.mergeableState, + reviewGood: derived.reviewGood, + guardrailHit: derived.guardrailHit, + guardrailEscalationCleared: derived.guardrailEscalationCleared, + migrationCollisionHold: input.migrationCollisionHold !== undefined, + unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined, + priorityEligibilityHold: input.priorityEligibilityHold !== undefined, + screenshotEvidenceHold: input.screenshotEvidenceHold !== undefined, + advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0, + // Deliberately conjoined with "nothing else adverse": an unstable state is only attributed to the ignore + // list when our own aggregate found NO failing check of any kind. If some other non-required check is + // also red, failingDetails is non-empty and the hold stands -- an ignore must never mask a real failure. + unstableExplainedByIgnoredChecks: + input.ignoredCheckNonPassing !== undefined && input.ignoredCheckNonPassing.length > 0 && (input.nonRequiredCheckFailures ?? []).length === 0 && input.ciState !== "failed", + unlinkedIssueMatchCloseWithoutCloseActing: input.unlinkedIssueMatchClose !== undefined && !derived.closeActing, + }; +} + +/** + * PURE. Which declared hold inputs are suppressing this PR's merge, in MERGE_HOLD_INPUTS declaration order. + * + * #9991: the ledger recorded `reason_code: "success"` for a held PR, because `deriveDecisionReasonCode` falls + * through to the gate conclusion when there is no blocker and no policy close. On the production Orb that is + * 518 holds across 146 pull requests filed under a value that is not a reason at all -- and #9729 cannot run a + * per-path backtest clearance against a bucket conflating seven mechanisms. + * + * Recomputed from the plan input rather than returned by the planner, for the reason in + * {@link buildPrDispositionInput}: a hold's defining shape is that no action was planned, so there is nothing + * in `PlannedAgentAction[]` to carry it. Both call sites go through the same builder, so the cause recorded in + * the ledger is the same expression that suppressed the merge, not a second opinion about it. + * + * Empty when nothing declared held it -- including when the only suppressor is the unstable mergeable state, + * which is GitHub's computation rather than one of our declared inputs and is reported separately by + * `heldForUnstableMergeState`. + */ +export function resolveHoldCause(input: AgentActionPlanInput): MergeHoldInput[] { + const level = (actionClass: AgentActionClass) => resolveAutonomy(input.autonomy, actionClass); + const guardrailHit = isGuardrailHit(input.changedPaths, input.hardGuardrailGlobs); + const reviewGood = input.conclusion === "success" && input.ciState === "passed"; + const escalationConfigured = + input.guardrailEscalationEffort != null || + input.guardrailEscalationSelfConsistencyRuns != null || + input.guardrailEscalationModel != null || + input.guardrailEscalationProvider != null; + const guardrailEscalationCleared = guardrailHit && reviewGood && escalationConfigured && input.guardrailEscalationOnCleanReview === "proceed"; + return derivePrDisposition( + buildPrDispositionInput(input, { reviewGood, guardrailHit, guardrailEscalationCleared, closeActing: isActingAutonomyLevel(level("close")) }), + ).heldBy; +} + export function planAgentMaintenanceActions(input: AgentActionPlanInput): PlannedAgentAction[] { const actions: PlannedAgentAction[] = []; const autoMaintain = input.autoMaintain ?? DEFAULT_AUTO_MAINTAIN_POLICY; @@ -1151,23 +1220,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne input.guardrailEscalationProvider != null; const guardrailEscalationCleared = guardrailHit && reviewGood && escalationConfigured && input.guardrailEscalationOnCleanReview === "proceed"; - const disposition = derivePrDisposition({ - mergeableState: input.pr.mergeableState, - reviewGood, - guardrailHit, - guardrailEscalationCleared, - migrationCollisionHold: input.migrationCollisionHold !== undefined, - unlinkedIssueMatchHold: input.unlinkedIssueMatchHold !== undefined, - priorityEligibilityHold: input.priorityEligibilityHold !== undefined, - screenshotEvidenceHold: input.screenshotEvidenceHold !== undefined, - advisoryCheckHold: input.advisoryCheckHold !== undefined && input.advisoryCheckHold.length > 0, - // Deliberately conjoined with "nothing else adverse": an unstable state is only attributed to the ignore - // list when our own aggregate found NO failing check of any kind. If some other non-required check is - // also red, failingDetails is non-empty and the hold stands -- an ignore must never mask a real failure. - unstableExplainedByIgnoredChecks: - input.ignoredCheckNonPassing !== undefined && input.ignoredCheckNonPassing.length > 0 && (input.nonRequiredCheckFailures ?? []).length === 0 && input.ciState !== "failed", - unlinkedIssueMatchCloseWithoutCloseActing: input.unlinkedIssueMatchClose !== undefined && !acting("close"), - }); + const disposition = derivePrDisposition(buildPrDispositionInput(input, { reviewGood, guardrailHit, guardrailEscalationCleared, closeActing: acting("close") })); const heldForManualReview = disposition.heldForManualReview; const mergeableStateUnstable = disposition.heldForUnstableMergeState; const labels = resolveAgentDispositionLabels(input); diff --git a/src/settings/pr-disposition.ts b/src/settings/pr-disposition.ts index dfb11add42..c4a0c35423 100644 --- a/src/settings/pr-disposition.ts +++ b/src/settings/pr-disposition.ts @@ -104,6 +104,10 @@ export type PrDisposition = { mergeable: MergeableAssessment; /** The SAME formula agent-actions.ts's heldForManualReview computes — one definition, two readers. */ heldForManualReview: boolean; + /** #9991: which declared inputs actually held, in MERGE_HOLD_INPUTS declaration order. Empty when the only + * suppressor is the unstable mergeable state — that is GitHub's computation, not one of our declared hold + * inputs, and it is reported by `heldForUnstableMergeState` instead. */ + heldBy: MergeHoldInput[]; /** True when the ONLY thing suppressing a would-merge is the unstable mergeable state (#8758's loud * hold): the planner uses it to attach the check-naming comment; the comment surface uses it to * downgrade "safe to merge". */ @@ -136,7 +140,16 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { const releasedHolds: Partial> = { guardrailHit: input.guardrailEscalationCleared === true, }; - const heldForManualReview = MERGE_HOLD_INPUT_KEYS.some((key) => input[key] === true && releasedHolds[key] !== true) || unstableHolds; + // #9991: WHICH inputs held, not merely whether any did. This was a `.some(...)` over exactly this set that + // threw the answer away, which is why the ledger could only fall back to the gate conclusion -- leaving 518 + // holds on the production Orb filed under reason "success", a bucket conflating seven distinct mechanisms + // that #9729 cannot run a per-path clearance against. + // + // Filtered from MERGE_HOLD_INPUT_KEYS rather than restated, so a hold added to the table is enumerated here + // automatically -- restatement is the thing that table exists to remove. Order is the table's own + // declaration order, which makes the recorded cause deterministic for identical inputs. + const heldBy = MERGE_HOLD_INPUT_KEYS.filter((key) => input[key] === true && releasedHolds[key] !== true); + const heldForManualReview = heldBy.length > 0 || unstableHolds; const heldForUnstableMergeState = unstableHolds; const wouldApprove = input.reviewGood && !heldForManualReview && mergeable !== "conflict"; const wouldMerge = input.reviewGood && !heldForManualReview && mergeable === "clean"; @@ -146,7 +159,7 @@ export function derivePrDisposition(input: PrDispositionInput): PrDisposition { // holds the PLANNER (the rebase rail acts) — a deliberate, documented asymmetry, not drift: the two // surfaces answer different questions ("is it safe to claim mergeable NOW" vs "should a human step in"). const commentMergeStateHeld = mergeable === "conflict" || mergeable === "behind" || unstableHolds; - return { mergeable, heldForManualReview, heldForUnstableMergeState, wouldApprove, wouldMerge, commentMergeStateHeld }; + return { mergeable, heldForManualReview, heldBy, heldForUnstableMergeState, wouldApprove, wouldMerge, commentMergeStateHeld }; } /** The comment surface's merge-state downgrade as a standalone predicate (#8759): the bridge diff --git a/test/unit/hold-cause.test.ts b/test/unit/hold-cause.test.ts new file mode 100644 index 0000000000..db0ce6d797 --- /dev/null +++ b/test/unit/hold-cause.test.ts @@ -0,0 +1,116 @@ +import { describe, expect, it } from "vitest"; + +import { MERGE_HOLD_INPUT_KEYS, derivePrDisposition, type PrDispositionInput } from "../../src/settings/pr-disposition"; +import { persistDecisionRecord } from "../../src/review/decision-record"; +import { createTestEnv } from "../helpers/d1"; + +// #9991: WHY a pull request was held, recorded. +// +// The ledger filed 518 holds across 146 pull requests under `reason_code = "success"` -- not a reason, but +// deriveDecisionReasonCode falling through to the gate conclusion when there is no blocker and no policy +// close. #9729 cannot run a per-path backtest clearance against a bucket conflating seven mechanisms. +// +// The constraint that shaped the fix: `reason_code` MUST NOT change. replayDecision recomputes it and reports +// a divergence on mismatch, so a new derivation would make all 518 existing records replay as unreproducible +// -- a false "cannot be re-derived" about records that are fine. The cause therefore lands in its own column, +// outside record_json, leaving digests and the replay contract untouched. + +const baseInput = (over: Partial = {}): PrDispositionInput => ({ + mergeableState: "clean", + reviewGood: true, + guardrailHit: false, + migrationCollisionHold: false, + unlinkedIssueMatchHold: false, + advisoryCheckHold: false, + priorityEligibilityHold: false, + screenshotEvidenceHold: false, + unlinkedIssueMatchCloseWithoutCloseActing: false, + ...over, +}); + +describe("derivePrDisposition heldBy (#9991)", () => { + it("names the input that held, not merely that something did", () => { + expect(derivePrDisposition(baseInput({ guardrailHit: true })).heldBy).toEqual(["guardrailHit"]); + }); + + it("names every input that held, in the table's declaration order", () => { + // Declaration order, not input order, so the recorded cause is deterministic for identical inputs. + const held = derivePrDisposition(baseInput({ screenshotEvidenceHold: true, guardrailHit: true })).heldBy; + expect(held).toEqual(["guardrailHit", "screenshotEvidenceHold"]); + expect(held.indexOf("guardrailHit")).toBeLessThan(held.indexOf("screenshotEvidenceHold")); + }); + + it("is empty when nothing held", () => { + const disposition = derivePrDisposition(baseInput()); + expect(disposition.heldBy).toEqual([]); + expect(disposition.heldForManualReview).toBe(false); + }); + + it("REGRESSION: excludes a RELEASED hold, matching heldForManualReview exactly", () => { + // #9808: a guardrail hit cleared by a clean escalated review no longer holds. Recording it as the cause + // would name a hold that did not happen. + const disposition = derivePrDisposition(baseInput({ guardrailHit: true, guardrailEscalationCleared: true })); + expect(disposition.heldBy).toEqual([]); + expect(disposition.heldForManualReview).toBe(false); + }); + + it("INVARIANT: heldForManualReview is true whenever heldBy is non-empty, for every declared input", () => { + // The two must never disagree -- heldForManualReview is now derived FROM heldBy, and this pins that for + // each key in the table rather than for one sampled case. + for (const key of MERGE_HOLD_INPUT_KEYS) { + const disposition = derivePrDisposition(baseInput({ [key]: true } as Partial)); + expect(disposition.heldBy, key).toEqual([key]); + expect(disposition.heldForManualReview, key).toBe(true); + } + }); + + it("stays empty when only the unstable mergeable state suppresses the merge", () => { + // That is GitHub's computation, not one of our declared inputs, and is reported separately. + const disposition = derivePrDisposition(baseInput({ mergeableState: "unstable" })); + expect(disposition.heldBy).toEqual([]); + expect(disposition.heldForUnstableMergeState).toBe(true); + expect(disposition.heldForManualReview).toBe(true); + }); +}); + +describe("persistDecisionRecord hold_cause (#9991)", () => { + const record = { + schemaVersion: 1 as const, + repoFullName: "acme/widgets", + pullNumber: 7, + headSha: "a".repeat(40), + baseSha: null, + action: "hold", + reasonCode: "success", + decidedAt: "2026-07-31T12:00:00.000Z", + }; + + async function holdCauseFor(env: Env, cause?: string | null): Promise { + await persistDecisionRecord(env, record as never, "d".repeat(64), 3, undefined, cause); + const row = await env.DB.prepare(`SELECT hold_cause FROM decision_records WHERE pull_number = 7`).first<{ hold_cause: string | null }>(); + return row?.hold_cause ?? null; + } + + it("writes the cause alongside the record", async () => { + expect(await holdCauseFor(createTestEnv(), "guardrailHit,screenshotEvidenceHold")).toBe("guardrailHit,screenshotEvidenceHold"); + }); + + it("writes NULL when no declared input held, rather than an empty string", async () => { + // Null and "" would both read as falsy in code but differently in SQL; one value for "no cause". + expect(await holdCauseFor(createTestEnv(), null)).toBeNull(); + expect(await holdCauseFor(createTestEnv())).toBeNull(); + }); + + it("REGRESSION: the cause does NOT enter record_json, so digests and replay are untouched", async () => { + // The whole reason this is a column. record_json is what recordDigest commits to and what replayDecision + // re-derives; a field in there would move every future digest and drag a private value into a published + // commitment. + const env = createTestEnv(); + await persistDecisionRecord(env, record as never, "d".repeat(64), 3, undefined, "guardrailHit"); + const row = await env.DB.prepare(`SELECT record_json, reason_code FROM decision_records WHERE pull_number = 7`).first<{ record_json: string; reason_code: string }>(); + expect(row?.record_json).not.toContain("guardrailHit"); + expect(row?.record_json).not.toContain("hold_cause"); + // And reason_code is untouched, which is what keeps the 518 historical records replayable. + expect(row?.reason_code).toBe("success"); + }); +});