fix(selfhost): disambiguate red-CI hold reasons and enrich hold audit metadata

src/queue/processors.ts

@@ -2043,6 +2043,27 @@ function boundAgentHoldAuditReason(reason: string): string {
     : reason;
 }
 
+/** Shared disambiguation for "the PR isn't review-good/mergeable and a close should be considered, but no
+ *  close ended up in the final plan" -- used by BOTH the CI-failed branch and the gate-blocker-codes branch in
+ *  {@link agentHoldAuditDetail} below, so a protected author or a not-yet-"auto" close autonomy is surfaced
+ *  with the SAME specific reason regardless of which signal (red CI vs. a gate blocker) triggered the hold.
+ *  Before this helper existed, the ciState==="failed" branch returned a bare, unexplained
+ *  "no close action was planned" message unconditionally -- it never checked protectedAuthor/closeAutonomy the
+ *  way the gate-blocker-codes branch already did just a few lines below it, so the single MOST common real-world
+ *  hold reason (a protected author, or close autonomy not yet set to auto) was invisible for a red-CI hold even
+ *  though the identical check already worked correctly for a gate-blocker hold (#selfhost-holdplan-audit). Returns
+ *  null when neither condition explains the hold -- a genuine residual case the caller falls back to its own
+ *  more specific generic message for. */
+function closeWithheldReason(args: { protectedAuthor: boolean; closeOwnerAuthors: boolean; closeAutonomy: string; blockerCode?: string | undefined }): string | null {
+  if (args.protectedAuthor && args.closeOwnerAuthors !== true) {
+    return args.blockerCode ? boundAgentHoldAuditReason(`close withheld for protected author on gate blocker ${args.blockerCode}`) : "close withheld for protected author";
+  }
+  if (args.closeAutonomy !== "auto" && args.closeAutonomy !== "auto_with_approval") {
+    return boundAgentHoldAuditReason(`close withheld because close autonomy is ${args.closeAutonomy}`);
+  }
+  return null;
+}
+
 export function agentHoldAuditDetail(args: {
   planned: PlannedAgentAction[];
   breakerOnPlan: PlannedAgentAction[];
@@ -2065,8 +2086,13 @@ export function agentHoldAuditDetail(args: {
     return "auto-action held by precision circuit breaker";
   if (args.ciHasPending || args.ciState === "pending")
     return "auto-action held because CI is still pending";
-  if (args.ciState === "failed")
-    return "auto-action held because CI is failing but no close action was planned";
+  const protectedAuthor = args.authorIsAutomationBot || args.authorIsOwner || args.authorIsAdmin;
+  if (args.ciState === "failed") {
+    return (
+      closeWithheldReason({ protectedAuthor, closeOwnerAuthors: args.closeOwnerAuthors, closeAutonomy: args.closeAutonomy }) ??
+      "auto-action held because CI is failing but no close action was planned"
+    );
+  }
   if (args.gateConclusion === "success") {
     if (args.mergeableState === "dirty")
       return "merge withheld because the PR conflicts with the base branch";
@@ -2078,13 +2104,11 @@ export function agentHoldAuditDetail(args: {
       return boundAgentHoldAuditReason(`merge withheld because merge autonomy is ${args.mergeAutonomy}`);
     return "merge withheld because no merge action was planned";
   }
-  const protectedAuthor = args.authorIsAutomationBot || args.authorIsOwner || args.authorIsAdmin;
   if (args.gateBlockerCodes.length > 0) {
-    if (protectedAuthor && args.closeOwnerAuthors !== true)
-      return boundAgentHoldAuditReason(`close withheld for protected author on gate blocker ${args.gateBlockerCodes[0]}`);
-    if (args.closeAutonomy !== "auto" && args.closeAutonomy !== "auto_with_approval")
-      return boundAgentHoldAuditReason(`close withheld because close autonomy is ${args.closeAutonomy}`);
-    return boundAgentHoldAuditReason(`held on gate blocker ${args.gateBlockerCodes[0]}`);
+    return (
+      closeWithheldReason({ protectedAuthor, closeOwnerAuthors: args.closeOwnerAuthors, closeAutonomy: args.closeAutonomy, blockerCode: args.gateBlockerCodes[0] }) ??
+      boundAgentHoldAuditReason(`held on gate blocker ${args.gateBlockerCodes[0]}`)
+    );
   }
   if (protectedAuthor && args.closeOwnerAuthors !== true)
     return "auto-action held for protected author";
@@ -2640,7 +2664,10 @@ async function runAgentMaintenancePlanAndExecute(
   // text) so an operator can see, at a glance, how much of the plan a breaker is currently rewriting, without
   // re-deriving it from individual PR audit rows. Fires only when the breaker actually changed something —
   // the common (not-engaged) path increments nothing, matching every other breaker log in this codebase.
-  for (const direction of precisionBreakerDowngradeDirections(planned, breakerOnPlan)) {
+  // Captured into a variable (not just consumed by the loop below) so the hold-audit metadata below can report
+  // whether/which direction the breaker engaged without recomputing it a second time (#selfhost-holdplan-audit).
+  const precisionBreakerDirections = precisionBreakerDowngradeDirections(planned, breakerOnPlan);
+  for (const direction of precisionBreakerDirections) {
     incr("gittensory_precision_breaker_downgrades_total", { direction });
   }
   // Observability (#terminal-outcome-audit): the final per-pass disposition, ALWAYS recorded -- including the
@@ -2674,6 +2701,14 @@ async function runAgentMaintenancePlanAndExecute(
   });
   if (disposition.actionClass === "hold") {
     const gateBlockerCodes = gate.blockers.map((blocker) => blocker.code);
+    const mergeAutonomy = resolveAutonomy(settings.autonomy, "merge");
+    const closeAutonomy = resolveAutonomy(settings.autonomy, "close");
+    // Same isContributor/closeEligible formula planAgentMaintenanceActions itself uses (agent-actions.ts) --
+    // duplicated here (not imported) because the planner computes it as a private local, never returns it.
+    // Persisted so a hold can be debugged without re-deriving eligibility from the three author-flag booleans
+    // by hand (#selfhost-holdplan-audit).
+    const isContributorAuthor = !authorIsOwner && !authorIsAdmin && !authorIsAutomationBot;
+    const closeEligible = isContributorAuthor || ((authorIsOwner || authorIsAdmin) && settings.closeOwnerAuthors === true);
     const holdDetail = agentHoldAuditDetail({
       planned,
       breakerOnPlan,
@@ -2687,8 +2722,8 @@ async function runAgentMaintenancePlanAndExecute(
       authorIsAdmin,
       authorIsAutomationBot,
       closeOwnerAuthors: settings.closeOwnerAuthors,
-      mergeAutonomy: resolveAutonomy(settings.autonomy, "merge"),
-      closeAutonomy: resolveAutonomy(settings.autonomy, "close"),
+      mergeAutonomy,
+      closeAutonomy,
     });
     await recordAuditEvent(env, {
       eventType: "agent.action.hold",
@@ -2700,12 +2735,25 @@ async function runAgentMaintenancePlanAndExecute(
         deliveryId,
         repoFullName,
         pullNumber: pr.number,
+        /* v8 ignore next -- defensive: a real GitHub PR always carries a head sha by the time it reaches this
+         * planning/audit path (it was upserted from the API earlier in this same webhook); the null fallback
+         * only keeps the JsonValue metadata type honest for the field's declared optionality. */
+        headSha: pr.headSha ?? null,
         gateConclusion: gate.conclusion,
         gateBlockerCodes,
+        gateBlockerTitles: gate.blockers.map((blocker) => blocker.title),
         ciState: ciAggregate.ciState,
         ciHasPending: ciAggregate.hasPending,
+        ciFailingCheckNames: ciAggregate.failingDetails.map((detail) => detail.name),
         mergeableState: liveMergeState ?? pr.mergeableState ?? null,
         reviewDecision: liveReviewDecision ?? pr.reviewDecision ?? null,
+        closeEligible,
+        closeAutonomy,
+        mergeAutonomy,
+        protectedAuthor: { owner: authorIsOwner, admin: authorIsAdmin, automation: authorIsAutomationBot },
+        closeOwnerAuthors: settings.closeOwnerAuthors,
+        precisionBreakerEngaged: precisionBreakerDirections.length > 0,
+        precisionBreakerDirections,
         disposition,
         plannedActionClasses: planned.map((action) => action.actionClass),
         finalActionClasses: breakerOnPlan.map((action) => action.actionClass),

test/unit/precision-breakers-chain.test.ts

@@ -174,6 +174,26 @@ describe("agentHoldAuditDetail — durable why-no-action audit reason", () => {
     expect(agentHoldAuditDetail({ ...base, ciState: "failed" })).toBe("auto-action held because CI is failing but no close action was planned");
   });
 
+  // REGRESSION (#selfhost-holdplan-audit): before this fix, a red-CI hold NEVER disambiguated protected-author
+  // or close-autonomy-not-auto -- it fell straight to the generic "no close action was planned" message even
+  // when the REAL reason (identical to the already-correct gate-blocker-codes branch below) was fully knowable.
+  // This is the single most common real-world cause of an opaque "CI is failing but no close action was
+  // planned" hold, so it must be surfaced with the SAME specificity red-CI gets via the gate-blocker path.
+  it("disambiguates a red-CI hold exactly like a gate-blocker hold: protected author, then close autonomy, before falling back to the generic reason", () => {
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", authorIsOwner: true })).toBe("close withheld for protected author");
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", authorIsAdmin: true })).toBe("close withheld for protected author");
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", authorIsAutomationBot: true })).toBe("close withheld for protected author");
+    // closeOwnerAuthors: true means the owner opted IN to being closeable like a contributor -- no longer
+    // "protected" for this purpose, so it falls through to the close-autonomy check (still "auto" here, so it
+    // reaches the generic fallback, proving the protected-author check is actually gated on closeOwnerAuthors).
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", authorIsOwner: true, closeOwnerAuthors: true })).toBe(
+      "auto-action held because CI is failing but no close action was planned",
+    );
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", closeAutonomy: "observe" })).toBe("close withheld because close autonomy is observe");
+    // Protected-author is checked BEFORE close-autonomy (matches the gate-blocker branch's own precedence).
+    expect(agentHoldAuditDetail({ ...base, ciState: "failed", authorIsOwner: true, closeAutonomy: "observe" })).toBe("close withheld for protected author");
+  });
+
   it("records the common green-review/no-merge reasons", () => {
     expect(agentHoldAuditDetail({ ...base, mergeableState: "dirty" })).toBe("merge withheld because the PR conflicts with the base branch");
     expect(agentHoldAuditDetail({ ...base, mergeableState: "blocked" })).toBe("merge withheld because mergeable_state is blocked");

test/unit/queue.test.ts

@@ -20919,6 +20919,26 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri
     expect(seen.closed).toBe(false);
     const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>();
     expect(closeAudit?.n).toBe(0);
+    // Enriched hold-audit fields (#selfhost-holdplan-audit): this scenario's gate blocker (missing linked issue)
+    // already produced a specific "protected author" detail before this change -- what's new here is that
+    // `metadata` now ALSO carries the structured closeEligible/closeAutonomy/protectedAuthor fields, so a hold
+    // is debuggable from the audit table alone. The actual bug fix -- a RED-CI hold (no gate blocker at all)
+    // gaining the same protected-author/close-autonomy disambiguation the gate-blocker branch already had --
+    // is unit-tested directly against agentHoldAuditDetail in precision-breakers-chain.test.ts, where the two
+    // branches can be exercised independently without needing a webhook fixture that produces CI-failed with
+    // zero gate blockers.
+    const holdAudit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = 'agent.action.hold' order by created_at desc limit 1").first<{ detail: string; metadata_json: string }>();
+    expect(holdAudit?.detail).toBe("close withheld for protected author on gate blocker missing_linked_issue");
+    expect(JSON.parse(holdAudit?.metadata_json ?? "{}")).toMatchObject({
+      repoFullName: "JSONbored/gittensory",
+      pullNumber: 63,
+      closeEligible: false,
+      closeAutonomy: "auto",
+      // The repo owner is also treated as an admin (GitHub's own collaborator-permission model), so both flags
+      // are true for this fixture -- only `automation` is meaningfully independent of `owner` here.
+      protectedAuthor: { owner: true, admin: true, automation: false },
+      closeOwnerAuthors: false,
+    });
   });
 
   it("REGRESSION: closeOwnerAuthors=true allows the general heuristic-close path to close a blocked owner-authored PR", async () => {