fix(ledger): record WHY a pull request was held, not the gate's conclusion

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;

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:<scope>): when set, convert a would-MERGE into a human HOLD before executing.
   //   • CLOSE breaker (closehold:<scope>): 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

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<string | null> {
   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;

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);

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<Record<MergeHoldInput, boolean>> = {
     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