diff --git a/migrations/0185_merge_block_expiry.sql b/migrations/0185_merge_block_expiry.sql new file mode 100644 index 0000000000..728d7929ba --- /dev/null +++ b/migrations/0185_merge_block_expiry.sql @@ -0,0 +1,8 @@ +-- #9012: an infra-scoped terminal merge failure (401 installation-token rejection, exhausted secondary +-- rate-limit window) is a property of the installation, not of the commit -- it fails every in-flight merge in +-- the fleet at once and heals for all of them at once. Before this column, every terminal class wrote a +-- head-scoped block whose ONLY escape was the contributor pushing a new commit, so one token rotation +-- permanently stranded every green, approved PR it caught, invisibly. An infra block now carries an expiry and +-- is re-probed once the window passes; a commit-scoped block (real conflict, repo merge policy) leaves this +-- NULL and keeps the original until-a-new-commit semantics. +ALTER TABLE pull_requests ADD COLUMN merge_blocked_until TEXT; diff --git a/migrations/0186_low_confidence_hold_counter.sql b/migrations/0186_low_confidence_hold_counter.sql new file mode 100644 index 0000000000..ec466f73da --- /dev/null +++ b/migrations/0186_low_confidence_hold_counter.sql @@ -0,0 +1,9 @@ +-- #9034: confidence-parking used to be an unbounded absorbing state. A blocker BELOW the close-confidence floor +-- still blocks, but under the default `hold_for_review` disposition it converts a one-shot close into an OPEN +-- hold -- with no cap on how many times the same PR may re-enter that hold. A PR shaped to keep drawing +-- low-confidence blockers therefore survives indefinitely, consuming the manual queue on every roll, and (with +-- the re-roll surface) can be walked toward a clean merge from there. These columns count the holds so the +-- Nth one closes instead. The head SHA makes the count per-ROLL rather than per-pass: a re-gate of the same +-- commit is the same hold, while each new commit that draws a fresh low-confidence blocker is a new one. +ALTER TABLE pull_requests ADD COLUMN low_confidence_hold_count INTEGER NOT NULL DEFAULT 0; +ALTER TABLE pull_requests ADD COLUMN low_confidence_hold_head_sha TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index c50921623d..a0c112c04e 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -525,6 +525,14 @@ export async function upsertPullRequestFromGitHub( lastSeenOpenAt, payloadJson: jsonString(payload), githubUpdatedAt: resolvedGithubUpdatedAt, + // #9012: a new commit starts the failed-merge budget fresh. This is what mergeAttemptCount's own schema + // and function docs have always PROMISED ("a new commit's attempts start fresh once the row's head + // advances") -- bumpPullRequestMergeAttempt scopes the *increment* to the head SHA, which stops a stale + // head from bumping the counter but never resets the counter itself, so the value survived every push. + // The consequence was cumulative: once one head exhausted MERGE_RETRY_CAP, every later head was + // one-strike-terminal on the first transient failure it met. Only reset on a REAL head change, so an + // ordinary resync (same head) cannot hand a genuinely failing merge an unlimited retry budget. + ...(headShaChanged ? { mergeAttemptCount: 0 } : {}), updatedAt: syncedAt, }, }); @@ -4246,15 +4254,67 @@ export async function bumpPullRequestDraftConversionCount(env: Env, fullName: st /** Mark a PR terminally merge-blocked for its current head SHA: the planner skips the `merge` disposition while * merge_blocked_sha == headSha. Scoped to headSha so a later commit (a pushed fix) auto-clears the block (the - * guard compares it to the live head). Records the human-readable terminal reason. */ -export async function markPullRequestMergeBlocked(env: Env, fullName: string, number: number, headSha: string, reason: string): Promise { + * guard compares it to the live head). Records the human-readable terminal reason. + * + * `expiresAt` (#9012) additionally lapses the block at an instant, for an INFRA-scoped cause — a rejected + * installation token or an exhausted secondary-rate-limit window, which is a property of the installation and + * not of the code, and which therefore cannot be cleared by the only escape a commit-scoped block offers. It + * also zeroes merge_attempt_count so the post-expiry re-probe starts from a full retry budget rather than + * being one-strike-terminal on the next hiccup. Omitted (undefined) = commit-scoped: unchanged behavior. */ +export async function markPullRequestMergeBlocked( + env: Env, + fullName: string, + number: number, + headSha: string, + reason: string, + expiresAt?: string | undefined, +): Promise { const db = getDb(env.DB); await db .update(pullRequests) - .set({ mergeBlockedSha: headSha, mergeBlockedReason: reason.slice(0, 280), updatedAt: nowIso() }) + .set({ + mergeBlockedSha: headSha, + mergeBlockedReason: reason.slice(0, 280), + mergeBlockedUntil: expiresAt ?? null, + ...(expiresAt !== undefined ? { mergeAttemptCount: 0 } : {}), + updatedAt: nowIso(), + }) .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); } +/** #9034: count this PR into the AI-review low-confidence hold tally and return the new total. + * + * Idempotent per HEAD, which is what makes the number mean "rolls", not "passes": the re-gate sweep, a CI + * event, and a label webhook can all re-evaluate the same commit within minutes, and every one of them would + * otherwise bump the counter and burn the cap against a single genuine hold. Only a head the counter has not + * already seen advances it. + * + * Deliberately NOT reset when the head changes (contrast bumpPullRequestMergeAttempt, whose whole point is + * that a new commit earns a fresh budget): a PR that keeps drawing low-confidence blockers across successive + * pushes is exactly the shape being capped, so a push must not buy another life. Mirrors + * bumpPullRequestDraftConversionCount's same reasoning for the same reason. + * + * Returns the pre-existing total unchanged when the head was already counted, so callers can compare against + * the cap on every pass without needing to know whether this particular pass advanced anything. */ +export async function bumpPullRequestLowConfidenceHold(env: Env, fullName: string, number: number, headSha: string | null | undefined): Promise { + const db = getDb(env.DB); + const where = and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number)); + const [existing] = await db + .select({ count: pullRequests.lowConfidenceHoldCount, countedHead: pullRequests.lowConfidenceHoldHeadSha }) + .from(pullRequests) + .where(where) + .limit(1); + if (!existing) return 0; + // `low_confidence_hold_count` is NOT NULL DEFAULT 0, so the row always carries a number here. + const current = Number(existing.count); + // An absent head SHA cannot be deduped against, so it must not count -- otherwise a stretch of sparse + // payloads would silently exhaust the cap and close a PR that was only ever held once. + if (headSha == null || existing.countedHead === headSha) return current; + const next = current + 1; + await db.update(pullRequests).set({ lowConfidenceHoldCount: next, lowConfidenceHoldHeadSha: headSha, updatedAt: nowIso() }).where(where); + return next; +} + // Linked-issue hard-rule violation memory (#linked-issue-hard-rule-persistence). /** Record the FIRST confirmed linked-issue hard-rule violation for a PR. Deliberately NOT scoped to headSha @@ -6903,6 +6963,9 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull mergeAttemptCount: row.mergeAttemptCount, mergeBlockedSha: row.mergeBlockedSha, mergeBlockedReason: row.mergeBlockedReason, + mergeBlockedUntil: row.mergeBlockedUntil, + lowConfidenceHoldCount: row.lowConfidenceHoldCount, + lowConfidenceHoldHeadSha: row.lowConfidenceHoldHeadSha, approvedHeadSha: row.approvedHeadSha, // Read straight from the row, NEVER the GitHub payload — this is a loopover-internal sweep marker. lastRegatedAt: row.lastRegatedAt, diff --git a/src/db/schema.ts b/src/db/schema.ts index 46e156ba2b..6ff68aa72f 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -377,10 +377,24 @@ export const pullRequests = sqliteTable( copycatMatchedPullNumber: integer("copycat_matched_pull_number"), // RC3 terminal-fail merges: failed-merge attempt count + the head SHA at which the merge is terminally // blocked (perms/required-check/conflict) so the planner stops planning a merge. Keyed to head SHA → a new - // commit auto-clears it. loopover-computed (executor-written), omitted from the GitHub-sync SET clause. + // commit auto-clears it. loopover-computed (executor-written), omitted from the GitHub-sync SET clause -- + // except merge_attempt_count, which the sync clause DOES reset when the head advances (#9012), because + // "a new commit's attempts start fresh" was documented here from the start but never actually implemented, + // leaving every head after the first exhaustion one-strike-terminal on any transient failure. mergeAttemptCount: integer("merge_attempt_count").notNull().default(0), mergeBlockedSha: text("merge_blocked_sha"), mergeBlockedReason: text("merge_blocked_reason"), + // #9012: expiry for an INFRA-scoped block (rejected installation token, exhausted rate-limit window) -- + // causes that belong to the installation rather than to the commit, so waiting for a commit that will never + // come is the wrong recovery. NULL = commit-scoped: blocked until the head advances, as before. + mergeBlockedUntil: text("merge_blocked_until"), + // #9034: how many distinct heads of this PR have been parked in the AI-review low-confidence hold, plus the + // head the last one was counted for (so a re-gate of the SAME commit is the same hold, not a new one). + // Deliberately NOT reset by a new commit -- unlike merge_attempt_count above, repeated low-confidence holds + // are the pattern being capped, so letting a push zero the counter would hand back exactly the unbounded + // survival this exists to end. loopover-computed, omitted from the GitHub-sync SET clause. + lowConfidenceHoldCount: integer("low_confidence_hold_count").notNull().default(0), + lowConfidenceHoldHeadSha: text("low_confidence_hold_head_sha"), // Review-evasion: repeated ready<->draft cycling (#gaming-tactic-draft-cycle). Counts every converted_to_draft // webhook ever processed for this PR NUMBER -- deliberately NOT scoped to head SHA like mergeAttemptCount, // since cycling back to draft after a fresh push is exactly the same evasion shape a new commit must not diff --git a/src/queue/job-dispatch.ts b/src/queue/job-dispatch.ts index bea3639208..37a3b6eb2f 100644 --- a/src/queue/job-dispatch.ts +++ b/src/queue/job-dispatch.ts @@ -50,6 +50,7 @@ import { generateSignalSnapshots } from "./signal-snapshot"; import { isDecisionAuditEnabled, runDecisionAuditSample } from "../review/decision-audit"; import { isRiskControlEnabled, runRiskControlRecalibration } from "../review/risk-control-wire"; import { runRetentionPrune } from "./retention"; +import { sweepStaleApprovalQueue } from "../services/agent-approval-queue"; // The 15 handlers below have no reason to move -- each is only reachable via this dispatcher (or, for // mapWithConcurrency, ALSO used by other still-in-processors.ts code), so they stay put and are exported // there purely for this one-directional import-back (processors.ts itself never calls processJob). @@ -272,6 +273,13 @@ export async function processJob(env: Env, message: JobMessage): Promise { } case "agent-regate-sweep": if (!message.repoFullName && message.requestedBy !== "test") { + // #9032: piggyback the approval-queue staleness pass on the sweep's own fan-out tick rather than adding + // a job type and a cron entry for a bounded DB scan. Best-effort and deliberately BEFORE the fan-out: + // a failure here must not cost the tick its re-gate work, which is the sweep's actual job. + const staleness = await sweepStaleApprovalQueue(env).catch(() => null); + if (staleness && (staleness.reminded > 0 || staleness.expired > 0)) { + console.log(JSON.stringify({ event: "approval_queue_staleness_swept", ...staleness })); + } await fanOutAgentRegateSweepJobs(env, message.requestedBy); return; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 3dbfaff11f..3254d6bf67 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -299,6 +299,8 @@ import { executeIssueMaintenanceActions, pendingClosureLabelApplied, } from "../services/agent-action-executor"; +import { activeMergeBlockedSha } from "../services/merge-failure"; +import { applyLowConfidenceHoldCap } from "../review/low-confidence-hold-cap"; import { loadIssueQualityReportMap } from "../services/issue-quality"; import { generateAndSendReviewRecap } from "../services/review-recap"; import { @@ -2660,7 +2662,12 @@ function buildAgentMaintenancePlanInput(args: { pr.createdAt, ), headSha: pr.headSha, - mergeBlockedSha: pr.mergeBlockedSha, + // #9012: pass through only a block that is STILL IN EFFECT. An infra-scoped block (rejected installation + // token, exhausted rate-limit window) carries an expiry; once it lapses the planner must see no block at + // all and re-probe the merge, so a fleet-wide token blip stops stranding green, approved PRs forever. + // Resolved here rather than in the planner so the planner stays a pure function of its inputs, clock-free. + mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, Date.now()), + mergeBlockedReason: pr.mergeBlockedReason, approvedHeadSha: pr.approvedHeadSha, authorLogin: pr.authorLogin, linkedIssues: pr.linkedIssues, @@ -3248,7 +3255,14 @@ async function runAgentMaintenancePlanAndExecute( // (no extra network/DB call, unlike migrationCollisionHold/unlinkedIssueMatchHold above) -- undefined unless the // gate failed SOLELY on a sub-aiReviewCloseConfidence-floor ai_consensus_defect/ai_review_split finding under // the (default) hold_for_review disposition. See resolveAiReviewLowConfidenceHold's own doc comment. - const aiReviewLowConfidenceHold = resolveAiReviewLowConfidenceHold(gate, settings); + const aiReviewLowConfidenceHoldCandidate = resolveAiReviewLowConfidenceHold(gate, settings); + // #9034: the hold is bounded. Confidence-parking a close is the right call while the verdict is genuinely + // uncertain, but repeated across independent rolls of the SAME PR it becomes an indefinite open hold that + // never escalates -- the PR survives, each roll costs a maintainer, and nothing counts. Past the cap the + // sub-floor finding has been reproduced enough times that the close is no longer the uncertain call the hold + // protects against, so it fires. The counter only advances on a head it has not already seen, so this budget + // is spent by real rolls rather than by the several re-gate passes a single commit attracts. + const aiReviewLowConfidenceHold = await applyLowConfidenceHoldCap(env, { repoFullName, pullNumber: pr.number, headSha: pr.headSha }, aiReviewLowConfidenceHoldCandidate); // #8962 salvageability hold — the OTHER side of the floor: an at/above-floor AI-judgment close routed to // hold-with-guidance when the deterministic salvageability score clears gate.aiReview.salvageabilityMinScore. // Knob unset (the default) short-circuits before any IO; the low-confidence hold keeps precedence. diff --git a/src/review/low-confidence-hold-cap.ts b/src/review/low-confidence-hold-cap.ts new file mode 100644 index 0000000000..951bc0672a --- /dev/null +++ b/src/review/low-confidence-hold-cap.ts @@ -0,0 +1,72 @@ +import { bumpPullRequestLowConfidenceHold, recordAuditEvent } from "../db/repositories"; + +/** + * #9034 — the bound on AI-review confidence parking. + * + * `resolveAiReviewLowConfidenceHold` (src/rules/advisory.ts) converts a would-be one-shot close into an OPEN + * hold when the blocking AI finding sits below the repo's close-confidence floor. That is the right call while + * the verdict is genuinely uncertain: an uncertain close is the expensive kind of mistake, and a human should + * see it. What was missing is any notion of "again": nothing counted how many times the SAME PR re-entered the + * hold, so a PR shaped to keep drawing sub-floor blockers survived indefinitely, cost a maintainer on every + * roll, and could be walked toward a clean merge from there. It was an absorbing state with no escape, the same + * shape as a permanently merge-blocked PR (#9012) or an unattended approval row (#9032). + * + * Deliberately its own module rather than a constant in advisory.ts: advisory.ts is one half of the + * hand-maintained gate-decision twin pair enforced by scripts/check-engine-parity.ts, and this cap has no engine + * counterpart to mirror — the engine's gate-advisory.ts carries no low-confidence hold resolver at all. Putting + * it here keeps the twin untouched instead of forcing a no-op engine release to satisfy the parity guard, and + * matches how MERGE_RETRY_CAP already lives beside its own policy (src/services/merge-failure.ts) rather than in + * the shared advisory module. + */ + +/** + * How many times one PR may be parked in the low-confidence hold before the hold stops applying and the close it + * was suppressing fires. + * + * Past the cap the sub-floor finding has been reproduced by several independent passes, which is itself the + * corroboration a single pass's confidence number lacked — so the close is no longer the uncertain call the hold + * exists to protect against. Three is deliberately generous next to MERGE_RETRY_CAP: this budget is spent by + * human-visible holds a maintainer could resolve at any point, not by silent retries. + */ +export const AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP = 3; + +/** Whether a PR has exhausted its low-confidence hold budget. `holds` is the running per-PR count from + * bumpPullRequestLowConfidenceHold, which advances once per distinct head — so this counts ROLLS, not the + * several re-gate passes a single commit attracts. Pure. */ +export function isLowConfidenceHoldCapped(holds: number): boolean { + return holds > AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP; +} + +/** + * Apply the cap to a low-confidence hold the gate just resolved (#9034). Returns the hold unchanged while the + * PR still has budget, or `undefined` once it does not — which lets the close the hold was suppressing fire. + * + * The counting lives here rather than at the re-gate call site so the "how many rolls has this PR spent" + * question has exactly one answer in the codebase, and so the capped path is directly testable: reaching that + * point through the pipeline needs a live gate evaluation, settings, GitHub state and a planner run — far too + * much machinery to stand up just to observe one boolean. + * + * Generic in the hold's shape because it neither reads nor changes it beyond quoting the reason into the audit + * trail — the hold is advisory.ts's to define. + */ +export async function applyLowConfidenceHoldCap( + env: Env, + target: { repoFullName: string; pullNumber: number; headSha: string | null | undefined }, + hold: T | undefined, +): Promise { + if (hold === undefined) return undefined; + const holds = await bumpPullRequestLowConfidenceHold(env, target.repoFullName, target.pullNumber, target.headSha); + if (!isLowConfidenceHoldCapped(holds)) return hold; + await recordAuditEvent(env, { + eventType: "agent.low_confidence_hold.capped", + actor: "loopover", + targetKey: `${target.repoFullName}#${target.pullNumber}`, + outcome: "denied", + detail: `low-confidence hold cap reached (${holds} > ${AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP}) — the suppressed close now proceeds`, + metadata: { repoFullName: target.repoFullName, pullNumber: target.pullNumber, holds, cap: AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP, reason: hold.reason }, + }).catch( + /* v8 ignore next -- best-effort: losing the audit row must never resurrect the hold the cap just lifted. */ + () => undefined, + ); + return undefined; +} diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 1ce034cfd4..89bdba411e 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -15,7 +15,7 @@ import { upsertGlobalContributorBlacklist, } from "../db/repositories"; import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; -import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "./merge-failure"; +import { classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeConflictMessage, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { recordTerminalActionOutcome, resolveDispositionReason } from "../review/outcomes-wire"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; @@ -974,7 +974,7 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er /* v8 ignore next -- guarded at the call site; defensive. */ if (!headSha) return; const message = errorMessage(error); - const { terminal: classifiedTerminal, reason: classifiedReason } = classifyMergeFailure(error); + const { terminal: classifiedTerminal, reason: classifiedReason, scope } = classifyMergeFailure(error); let terminal = classifiedTerminal; let reason = classifiedReason; if (!terminal) { @@ -986,7 +986,12 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er } } if (!terminal) return; - await markPullRequestMergeBlocked(env, ctx.repoFullName, ctx.pullNumber, headSha, reason); + // #9012: an infra-scoped cause (rejected token, exhausted rate-limit window) is fleet-wide and self-healing, + // so its block expires and is re-probed; a commit-scoped one still lasts until the contributor pushes. The + // scope carries through the retry-cap path above deliberately — a sustained secondary-rate-limit window is + // exactly the case that used to burn MERGE_RETRY_CAP and then permanently strand every PR it caught. + const expiresAt = scope === "infra" ? new Date(Date.now() + INFRA_MERGE_BLOCK_TTL_MS).toISOString() : undefined; + await markPullRequestMergeBlocked(env, ctx.repoFullName, ctx.pullNumber, headSha, reason, expiresAt); // A merge held for a human is the terminal outcome of this whole retry sequence -- exactly the "a real // failure the maintainer must see" case capturePostHogReviewFailure already covers for an exhausted AI // review pass. Fires once per hold (not per retry attempt), so a transient failure that resolves within @@ -1001,7 +1006,7 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er targetKey: `${ctx.repoFullName}#${ctx.pullNumber}`, outcome: "denied", detail: `merge held for human — ${reason}`, - metadata: { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, headSha, reason: reason.slice(0, 280) }, + metadata: { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, headSha, reason: reason.slice(0, 280), scope, ...(expiresAt !== undefined ? { expiresAt } : {}) }, }).catch(() => undefined); } diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 5bcd6287ac..d9ebe45cfb 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -1,4 +1,14 @@ -import { claimPendingAgentActionDecision, getInstallation, getPullRequest, getPendingAgentAction, recordAuditEvent, setPendingAgentActionStatus } from "../db/repositories"; +import { + claimPendingAgentActionDecision, + getInstallation, + getPullRequest, + getPendingAgentAction, + insertNotificationDeliveryIfAbsent, + listPendingAgentActions, + recordAuditEvent, + setPendingAgentActionStatus, +} from "../db/repositories"; +import { APPROVAL_EXPIRY_MS, planApprovalQueueMaintenance } from "./agent-approval-staleness"; import { resolveRepositorySettings } from "../settings/repository-settings"; import { createInstallationToken } from "../github/app"; import { loadLinkedIssueHardRules, resolveLinkedIssueHardRule } from "../review/linked-issue-hard-rules"; @@ -467,3 +477,64 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de }); return { status: finalStatus, action: { ...pending, status: finalStatus, decidedBy: input.decidedBy }, executionOutcome: execOutcome }; } + +/** + * #9032 — sweep the approval queue for rows nobody has acted on. + * + * Staging notifies the maintainer exactly once, forever (stageForApproval's dedup key is per (PR, actionClass)), + * so a missed badge meant a staged action waited indefinitely with nothing anywhere saying so. This pass runs on + * the same cron tick as the re-gate fan-out and gives every pending row two escapes it never had: a periodic + * reminder badge, and a hard expiry once reminders have plainly not worked. + * + * Expiry is deliberately NOT a rejection. Rejection is a maintainer's judgment that the action is wrong and it + * feeds the trust loop as such; expiry only records that consent was never given. Nothing executes either way, + * but a later pass that re-plans the same action stages a fresh row with a fresh notification, because a + * genuinely still-correct action should not be silenced by the fact that a human was on vacation once. + * + * Every step is best-effort and independent: one repo's notification failure must not stop another row from + * expiring. Returns per-outcome counts for the caller's structured log. + */ +export async function sweepStaleApprovalQueue(env: Env, nowMs: number = Date.now()): Promise<{ reminded: number; expired: number }> { + const pending = await listPendingAgentActions(env, { status: "pending", limit: 500 }); + let reminded = 0; + let expired = 0; + for (const row of pending) { + const plan = planApprovalQueueMaintenance(row.createdAt, nowMs); + if (plan.kind === "none") continue; + /* v8 ignore next -- a repo full name always has an owner segment; mirrors stageForApproval's own fallback. */ + const recipientLogin = row.repoFullName.split("/")[0] ?? ""; + if (plan.kind === "remind") { + const ageDays = plan.bucket; + const { created } = await insertNotificationDeliveryIfAbsent(env, { + // The bucket index is what makes this fire again at all: the dedup key changes once per interval, so + // the ~2-minute sweep cadence collapses to exactly one badge per interval with no extra persisted state. + dedupKey: `agent.pending_action.reminder:${row.repoFullName}#${row.pullNumber}:${row.actionClass}:${plan.bucket}`, + channel: "badge", + recipientLogin, + eventType: "agent.pending_action", + repoFullName: row.repoFullName, + pullNumber: row.pullNumber, + title: `Still waiting: a ${row.actionClass.replace(/_/g, " ")} staged ${ageDays} day${ageDays === 1 ? "" : "s"} ago needs your approval`, + body: `${row.reason ?? "A staged action"} — accept to execute it, or reject to cancel. It expires after ${Math.round(APPROVAL_EXPIRY_MS / (24 * 60 * 60 * 1000))} days.`, + deeplink: `https://github.com/${row.repoFullName}/pull/${row.pullNumber}`, + actorLogin: "loopover", + }).catch(() => ({ created: false })); + if (created) reminded += 1; + continue; + } + // Atomic pending→expired, so a maintainer accepting at the exact moment the sweep expires the row still + // wins: their claim and this one contend on the same conditional UPDATE and only one lands (#2423-concurrent). + const claimed = await claimPendingAgentActionDecision(env, row.id, { status: "expired", decidedBy: "loopover" }).catch(() => false); + if (!claimed) continue; + expired += 1; + await recordAuditEvent(env, { + eventType: "agent.pending_action.expired", + actor: "loopover", + targetKey: `${row.repoFullName}#${row.pullNumber}`, + outcome: "denied", + detail: `staged ${row.actionClass} expired unapproved after ${Math.round(APPROVAL_EXPIRY_MS / (24 * 60 * 60 * 1000))} days`, + metadata: { repoFullName: row.repoFullName, pullNumber: row.pullNumber, actionClass: row.actionClass, stagedAt: row.createdAt }, + }).catch(() => undefined); + } + return { reminded, expired }; +} diff --git a/src/services/agent-approval-staleness.ts b/src/services/agent-approval-staleness.ts new file mode 100644 index 0000000000..ebc266e6e5 --- /dev/null +++ b/src/services/agent-approval-staleness.ts @@ -0,0 +1,49 @@ +/** + * #9032 — the approval queue's own escape hatch. + * + * A staged `auto_with_approval` action notifies the maintainer exactly ONCE, on first staging: stageForApproval + * returns early on `!created`, and the badge delivery is dedup-keyed per (PR, actionClass) forever. A maintainer + * who misses that single badge gets no further prompt, and the row waits indefinitely — the same absorbing-state + * shape as a permanently merge-blocked PR (#9012) or a stranded pending-closure flag (#9031): the system stops + * making progress and nothing says so. + * + * This module is the pure decision half. Given when a row was staged and the current clock, it says whether the + * row is due for a reminder, due to expire, or should be left alone. The reminder is BUCKETED (one per interval) + * rather than "notify every pass", because the sweep that drives this runs every couple of minutes: the bucket + * index goes into the notification dedup key, so the existing insert-if-absent dedup turns hundreds of passes + * into exactly one badge per interval, with no extra state to persist. + * + * Reminders are bounded on purpose. An action nobody has accepted after a week is not a notification problem — + * the PR has almost certainly moved on — so the row expires instead of nagging forever. Expiry is NOT a rejection: + * it records that consent was never given, so nothing executes, and re-planning the same action on a later pass + * stages a fresh row with a fresh notification. + */ + +/** How long a pending row waits between reminder badges. A maintainer queue is checked in days, not minutes. */ +export const APPROVAL_REMINDER_INTERVAL_MS = 24 * 60 * 60 * 1000; + +/** How long a pending row may sit before it expires unexecuted. Six reminders precede it (days 1–6). */ +export const APPROVAL_EXPIRY_MS = 7 * 24 * 60 * 60 * 1000; + +export type ApprovalQueueMaintenance = + | { kind: "none" } + /** `bucket` is the reminder's 1-based index (day 1, day 2, …). It goes into the notification dedup key, so the + * same bucket re-derived on a later pass within the same interval dedups to a single delivery. */ + | { kind: "remind"; bucket: number } + | { kind: "expire" }; + +/** + * Decide what a pending approval row is due for. Pure. + * + * An unparseable or future `createdAt` yields `none`: a bad timestamp must never be the thing that expires a + * real staged action a maintainer is still expecting to accept. Failing quiet is correct here — the row simply + * keeps its pre-#9032 "waits forever" behavior rather than being destroyed by a clock artifact. + */ +export function planApprovalQueueMaintenance(createdAt: string, nowMs: number): ApprovalQueueMaintenance { + const staged = Date.parse(createdAt); + if (!Number.isFinite(staged)) return { kind: "none" }; + const age = nowMs - staged; + if (age >= APPROVAL_EXPIRY_MS) return { kind: "expire" }; + const bucket = Math.floor(age / APPROVAL_REMINDER_INTERVAL_MS); + return bucket >= 1 ? { kind: "remind", bucket } : { kind: "none" }; +} diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index 950440be96..ecb9703cc0 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -64,21 +64,84 @@ function httpStatus(error: unknown): number | undefined { return typeof status === "number" ? status : undefined; } -/** Classify a failed merge. `terminal: true` → never re-plan this merge for the current commit (hold for a - * human). `terminal: false` → possibly transient; the caller retries up to MERGE_RETRY_CAP. `reason` is a - * short human-readable summary persisted on the PR + audit record. */ -export function classifyMergeFailure(error: unknown): { terminal: boolean; reason: string } { +/** + * How long a merge held for an `infra`-scoped cause stays suppressed before the planner is allowed to try the + * merge again for the SAME commit (#9012). Long enough that a re-probe cannot hot-loop against a still-broken + * installation (each expiry costs at most one merge call per PR per window), short enough that a token rotation + * or a secondary-rate-limit window does not strand a green, approved PR for the rest of its life. + */ +export const INFRA_MERGE_BLOCK_TTL_MS = 30 * 60 * 1000; + +/** + * Which *thing* a terminal merge failure is terminal ABOUT (#9012). + * + * • `"commit"` — the failure is a property of this commit and only a new commit can change it: a real base + * conflict, a repo merge policy that forbids an App merge, an absent required check. Re-probing cannot + * help, so the block persists until the head advances. This is the pre-#9012 behavior for every class. + * • `"infra"` — the failure is a property of the INSTALLATION or of GitHub's current state, not of the code: + * a rejected token (App suspended, key rotated) or an exhausted secondary-rate-limit window. These are + * fleet-wide and self-healing — every in-flight merge fails at once and every one of them recovers at once. + * Persisting a head-scoped block for such a cause strands green, approved PRs permanently, since the only + * documented escape is a contributor pushing a commit they have no reason to push. So an infra block gets + * an expiry (INFRA_MERGE_BLOCK_TTL_MS) instead: still terminal for THIS pass — no hot-looping against a + * known-bad credential, which is the whole point of failing fast on a 401 — but re-probed once the window + * passes, so recovery is autonomous. + */ +export type MergeFailureScope = "commit" | "infra"; + +/** Classify a failed merge. `terminal: true` → do not re-plan this merge now (hold for a human, subject to + * `scope`). `terminal: false` → possibly transient; the caller retries up to MERGE_RETRY_CAP. `reason` is a + * short human-readable summary persisted on the PR + audit record. `scope` says whether a terminal block is + * commit-scoped (clears only on a new commit) or infra-scoped (also clears on a TTL re-probe) — see + * MergeFailureScope. It is meaningful on the non-terminal classes too: the executor carries it through the + * retry-cap exhaustion path, so a sustained rate-limit window that burns MERGE_RETRY_CAP still recovers on + * its own rather than terminally stranding everything that passed through it. */ +export function classifyMergeFailure(error: unknown): { terminal: boolean; reason: string; scope: MergeFailureScope } { const message = errorMessage(error); const status = httpStatus(error); - if (status === 401) return { terminal: true, reason: `installation token rejected: App suspended or key rotated (401): ${message}` }; - if (status === 403 && isConvergenceForbiddenMessage(message)) return { terminal: false, reason: `merge forbidden for now (403 — branch protection or GitHub permission visibility may still be converging): ${message}` }; - if (status === 403) return { terminal: true, reason: `merge forbidden (403): ${message}` }; + if (status === 401) return { terminal: true, scope: "infra", reason: `installation token rejected: App suspended or key rotated (401): ${message}` }; + if (status === 403 && isConvergenceForbiddenMessage(message)) + return { terminal: false, scope: "infra", reason: `merge forbidden for now (403 — branch protection or GitHub permission visibility may still be converging): ${message}` }; + if (status === 403) return { terminal: true, scope: "commit", reason: `merge forbidden (403): ${message}` }; // A 405 "Base branch was modified" is a benign TOCTOU race, not a policy rejection — retry against the new base // (the executor caps retries at MERGE_RETRY_CAP before escalating to the same terminal hold). - if (status === 405 && isBaseBranchMovedMessage(message)) return { terminal: false, reason: `base branch moved during merge — retrying: ${message}` }; - if (status === 405 && isMergeAlreadyInProgressMessage(message)) return { terminal: false, reason: `a merge for this PR was already in progress — retrying: ${message}` }; - if (status === 405) return { terminal: true, reason: `merge not allowed (405 — repo merge policy forbids an automated merge): ${message}` }; - if (status === 409) return { terminal: true, reason: `merge conflict / required check absent (409): ${message}` }; - if (isMergeConflictMessage(message)) return { terminal: true, reason: `branch conflicts with base — contributor must rebase: ${message}` }; - return { terminal: false, reason: message }; + if (status === 405 && isBaseBranchMovedMessage(message)) return { terminal: false, scope: "commit", reason: `base branch moved during merge — retrying: ${message}` }; + if (status === 405 && isMergeAlreadyInProgressMessage(message)) return { terminal: false, scope: "commit", reason: `a merge for this PR was already in progress — retrying: ${message}` }; + if (status === 405) return { terminal: true, scope: "commit", reason: `merge not allowed (405 — repo merge policy forbids an automated merge): ${message}` }; + if (status === 409) return { terminal: true, scope: "commit", reason: `merge conflict / required check absent (409): ${message}` }; + if (isMergeConflictMessage(message)) return { terminal: true, scope: "commit", reason: `branch conflicts with base — contributor must rebase: ${message}` }; + return { terminal: false, scope: "commit", reason: message }; +} + +/** Whether a persisted merge block still suppresses the `merge` disposition, given the live head and clock + * (#9012). Pure, so the planner stays clock-free: the caller resolves this and passes through only a block + * that is still in effect. A block with no expiry is commit-scoped and lasts until the head advances; one with + * an expiry is infra-scoped and additionally lapses at that instant. An unparseable expiry is treated as + * expired: a malformed timestamp must not be the thing that strands a green PR forever, which is the exact + * failure this fix exists to remove. */ +export function isMergeBlockInEffect( + block: { mergeBlockedSha?: string | null | undefined; mergeBlockedUntil?: string | null | undefined }, + headSha: string | null | undefined, + nowMs: number, +): boolean { + if (block.mergeBlockedSha == null || headSha == null || block.mergeBlockedSha !== headSha) return false; + if (block.mergeBlockedUntil == null) return true; + const expiry = Date.parse(block.mergeBlockedUntil); + return Number.isFinite(expiry) && nowMs < expiry; +} + +/** The merge-block head SHA the planner should see: the stored one while the block is still in effect, else + * `null` (#9012). The planner compares this to the live head and is deliberately clock-free, so resolving the + * infra-scoped expiry has to happen out here, on the way in. Returning `null` rather than omitting the field + * matches how an unblocked PR already looks to the planner, so a lapsed block is indistinguishable from never + * having been blocked — which is exactly the intent: re-probe it like any other mergeable PR. */ +export function activeMergeBlockedSha( + block: { mergeBlockedSha?: string | null | undefined; mergeBlockedUntil?: string | null | undefined }, + headSha: string | null | undefined, + nowMs: number, +): string | null { + // Normalized before the check, not inside the true arm: an absent block is `undefined` on the record but + // `null` on the wire the planner reads, and folding that in here keeps this a total function over both. + const blockedSha = block.mergeBlockedSha ?? null; + return isMergeBlockInEffect(block, headSha, nowMs) ? blockedSha : null; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 9d8a6facad..5eb8577728 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -476,6 +476,10 @@ export type AgentActionPlanInput = { // (perms/required-check/conflict). When they match, the merge can't complete for this commit → suppress it. headSha?: string | null | undefined; mergeBlockedSha?: string | null | undefined; + // #9012: the human-readable reason the merge is blocked. Purely for surfacing — with review_state_label on, + // a terminally-blocked PR used to keep a ready-to-merge label and no other signal anywhere, so a contributor + // looking at a green, approved, "ready" PR had no way to learn it would never merge and no reason to push. + mergeBlockedReason?: string | null | undefined; // Re-approval idempotency: the head SHA the bot last auto-approved. When it equals the live headSha this // exact commit is already bot-approved → suppress the `approve` disposition (a GitHub App's own approval // does NOT reliably flip reviewDecision to APPROVED, so without this the bot re-approves every sweep). A new @@ -1077,6 +1081,8 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne const ciUnverified = input.ciState === "unverified"; // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan // the merge; it can't complete for this commit. A new commit makes the live head differ from mergeBlockedSha. + // The caller passes through only a block that is still IN EFFECT (#9012 — isMergeBlockInEffect resolves the + // infra-scoped expiry against the clock), so this stays a plain head comparison and the planner stays pure. const mergeTerminallyBlocked = input.pr.mergeBlockedSha != null && input.pr.headSha != null && input.pr.mergeBlockedSha === input.pr.headSha; // Re-approval idempotency: this exact commit is already bot-approved when the stored approved-head SHA equals // the live head SHA → never re-post an approval for it (a GitHub App's own approval does not reliably flip @@ -1257,7 +1263,7 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? labels.changesRequested : input.migrationCollisionHold !== undefined ? labels.migrationCollision - : heldForManualReview + : heldForManualReview || mergeTerminallyBlocked ? labels.manualReview : labels.readyToMerge; const reason = linkedIssueCloseInFlight @@ -1278,7 +1284,14 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne ? `verdict=${conclusion}; ${mergeUnstableHoldReason(input.nonRequiredCheckFailures)}` : heldForManualReview ? `verdict=${conclusion}; ${guardrailReason}` - : `verdict=${conclusion}; CI green`; + : mergeTerminallyBlocked + ? // #9012: the ONE place a terminal merge block becomes visible to a human. Everything + // else about this PR reads healthy — gate passing, CI green, bot-approved — so + // without naming the block here the label says "needs review" with no reason and the + // maintainer has nothing to act on. mergeBlockedReason is executor-written and + // already length-capped at persist time (280 chars). + `verdict=${conclusion}; CI green, but a prior merge attempt failed and is held: ${input.pr.mergeBlockedReason ?? "reason unrecorded"}` + : `verdict=${conclusion}; CI green`; if (label !== null && !hasLabelOrPlanned(input.pr.labels, actions, label)) { actions.push({ actionClass: "label", diff --git a/src/types.ts b/src/types.ts index 16db91fe20..689bd9d294 100644 --- a/src/types.ts +++ b/src/types.ts @@ -679,6 +679,17 @@ export type PullRequestRecord = { mergeAttemptCount?: number | null | undefined; mergeBlockedSha?: string | null | undefined; mergeBlockedReason?: string | null | undefined; + /** #9012: expiry of an INFRA-scoped merge block (rejected installation token, exhausted rate-limit window). + * Those causes belong to the installation, not the commit, so "push a new commit" is not a real escape — + * the block lapses at this instant and the merge is re-probed. `null` = commit-scoped, blocked until the + * head advances. Read through isMergeBlockInEffect (src/services/merge-failure.ts), never compared raw. */ + mergeBlockedUntil?: string | null | undefined; + /** #9034: count of distinct heads parked in the AI-review low-confidence hold, and the head the last one was + * counted for. Once the count passes AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP the hold stops converting a close + * into an indefinite open hold and the close fires. Never reset by a new commit — repeated holds ARE the + * pattern being capped. */ + lowConfidenceHoldCount?: number | null | undefined; + lowConfidenceHoldHeadSha?: string | null | undefined; /** Re-approval idempotency: the head SHA the bot last auto-approved. The planner skips the `approve` * disposition while approvedHeadSha === headSha (this commit is already approved by the bot); a new commit * clears the match so the bot may re-approve the new code. Mirrors mergeBlockedSha. */ @@ -1769,7 +1780,10 @@ export type AgentPendingActionParams = { // executor, but the mutation itself threw (a real GitHub-call failure), as opposed to a clean "accepted" outcome // where the executor's own gates (autonomy/dry-run/freshness) declined to act -- that's an intentional policy // result, not a failure, and stays "accepted" (#2423). -export type AgentPendingActionStatus = "pending" | "accepted" | "rejected" | "errored"; +/** #9032: `expired` = staged, reminded, and never decided within APPROVAL_EXPIRY_MS. Distinct from `rejected` + * on purpose — a rejection is a maintainer's judgment that the action was wrong and feeds the trust loop as + * such, while an expiry only records that consent was never given. Neither executes anything. */ +export type AgentPendingActionStatus = "pending" | "accepted" | "rejected" | "errored" | "expired"; /** Approval-queue row (#779): an `auto_with_approval` action the write-actions layer staged for a one-tap * maintainer accept (→ execute) or reject (→ cancel). */ diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 31978bec33..946ffbee90 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -1455,6 +1455,47 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { captureSpy.mockRestore(); }); + // #9012: a 401 is terminal for the pass (the token really is bad), but it belongs to the INSTALLATION, not to + // the commit — every in-flight merge in the fleet fails at once and every one recovers at once. Writing a + // head-scoped block with no expiry meant the only escape was a commit the contributor had no reason to push, + // so one key rotation stranded every green, approved PR it caught, permanently and silently. + it("gives a 401-blocked merge an expiry so it recovers without a new commit (#9012)", async () => { + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "" }); + vi.mocked(mergePullRequest).mockRejectedValueOnce(Object.assign(new Error("Bad credentials"), { status: 401 })); + + await executeAgentMaintenanceActions(env, ctx(), [merge]); + + const row = await env.DB.prepare( + "select merge_blocked_sha as sha, merge_blocked_until as until, merge_attempt_count as attempts from pull_requests where repo_full_name = ? and number = ?", + ) + .bind("owner/repo", 7) + .first<{ sha: string | null; until: string | null; attempts: number }>(); + expect(row?.sha).toBe("sha7"); + expect(Date.parse(row?.until ?? "")).toBeGreaterThan(Date.now()); + // Zeroed so the post-expiry re-probe gets a full retry budget rather than being one-strike-terminal. + expect(row?.attempts).toBe(0); + const audit = await env.DB.prepare("select metadata_json as metadata from audit_events where event_type = ?").bind("agent.action.merge_blocked").first<{ metadata: string }>(); + expect(JSON.parse(audit?.metadata ?? "{}")).toMatchObject({ scope: "infra" }); + }); + + it("leaves a genuinely commit-scoped block with no expiry, so it lasts until the contributor rebases (#9012)", async () => { + const env = createTestEnv({}); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "" }); + vi.mocked(mergePullRequest).mockRejectedValueOnce(Object.assign(new Error("Pull Request is not mergeable"), { status: 405 })); + + await executeAgentMaintenanceActions(env, ctx(), [merge]); + + const row = await env.DB.prepare("select merge_blocked_sha as sha, merge_blocked_until as until from pull_requests where repo_full_name = ? and number = ?") + .bind("owner/repo", 7) + .first<{ sha: string | null; until: string | null }>(); + expect({ sha: row?.sha, until: row?.until }).toEqual({ sha: "sha7", until: null }); + const audit = await env.DB.prepare("select metadata_json as metadata from audit_events where event_type = ?").bind("agent.action.merge_blocked").first<{ metadata: string }>(); + const metadata = JSON.parse(audit?.metadata ?? "{}") as Record; + expect(metadata.scope).toBe("commit"); + expect(metadata).not.toHaveProperty("expiresAt"); + }); + it("opportunistically refreshes installation health when a PR-write mutation fails with a 403 (#2265)", async () => { const env = createTestEnv({}); vi.mocked(closePullRequest).mockRejectedValueOnce(Object.assign(new Error("Resource not accessible by integration"), { status: 403 })); diff --git a/test/unit/approval-queue-staleness.test.ts b/test/unit/approval-queue-staleness.test.ts new file mode 100644 index 0000000000..de46b9b79b --- /dev/null +++ b/test/unit/approval-queue-staleness.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it, vi } from "vitest"; +import { + createPendingAgentActionIfAbsent, + getPendingAgentAction, + listAuditEventsForTarget, + listNotificationDeliveriesForRecipient, + listPendingAgentActions, + setPendingAgentActionStatus, +} from "../../src/db/repositories"; +import { sweepStaleApprovalQueue } from "../../src/services/agent-approval-queue"; +import { APPROVAL_EXPIRY_MS, APPROVAL_REMINDER_INTERVAL_MS, planApprovalQueueMaintenance } from "../../src/services/agent-approval-staleness"; +import { createTestEnv } from "../helpers/d1"; + +const DAY = 24 * 60 * 60 * 1000; + +// #9032: stageForApproval returns early on `!created` and its badge dedup key is per (PR, actionClass) with no +// time component, so the maintainer was notified exactly ONCE, ever. A missed badge meant the staged action +// waited indefinitely with nothing anywhere saying so — the same absorbing-state shape as #9012's permanently +// merge-blocked PR. The decision half is pure and lives here. +describe("planApprovalQueueMaintenance (#9032)", () => { + const staged = "2026-07-01T00:00:00.000Z"; + const stagedMs = Date.parse(staged); + + it("leaves a freshly staged row alone — the staging notification is still the only one needed", () => { + expect(planApprovalQueueMaintenance(staged, stagedMs)).toEqual({ kind: "none" }); + expect(planApprovalQueueMaintenance(staged, stagedMs + APPROVAL_REMINDER_INTERVAL_MS - 1)).toEqual({ kind: "none" }); + }); + + it("buckets reminders by interval, so the ~2-minute sweep collapses to one badge per interval", () => { + expect(planApprovalQueueMaintenance(staged, stagedMs + APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ kind: "remind", bucket: 1 }); + // Anywhere inside the same interval derives the SAME bucket → the same dedup key → one delivery. + expect(planApprovalQueueMaintenance(staged, stagedMs + APPROVAL_REMINDER_INTERVAL_MS + 1000)).toEqual({ kind: "remind", bucket: 1 }); + expect(planApprovalQueueMaintenance(staged, stagedMs + 3 * APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ kind: "remind", bucket: 3 }); + }); + + it("expires rather than nagging forever once reminders have plainly not worked", () => { + expect(planApprovalQueueMaintenance(staged, stagedMs + APPROVAL_EXPIRY_MS - 1)).toEqual({ kind: "remind", bucket: 6 }); + expect(planApprovalQueueMaintenance(staged, stagedMs + APPROVAL_EXPIRY_MS)).toEqual({ kind: "expire" }); + expect(planApprovalQueueMaintenance(staged, stagedMs + 90 * DAY)).toEqual({ kind: "expire" }); + }); + + it("does nothing on an unparseable or future timestamp — a clock artifact must not destroy a real staged action", () => { + expect(planApprovalQueueMaintenance("not-a-date", stagedMs)).toEqual({ kind: "none" }); + expect(planApprovalQueueMaintenance(staged, stagedMs - DAY)).toEqual({ kind: "none" }); + }); + + it("orders the two thresholds so at least one reminder always precedes an expiry", () => { + expect(APPROVAL_EXPIRY_MS).toBeGreaterThan(APPROVAL_REMINDER_INTERVAL_MS); + }); +}); + +describe("sweepStaleApprovalQueue (#9032)", () => { + async function stage(env: Env, pullNumber: number): Promise { + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "alice/repo", + pullNumber, + installationId: 42, + actionClass: "merge", + autonomyLevel: "auto_with_approval", + params: { mergeMethod: "squash" }, + reason: "clean and approved", + }); + return action.id; + } + + it("does nothing while every pending row is fresh", async () => { + const env = createTestEnv(); + await stage(env, 1); + expect(await sweepStaleApprovalQueue(env)).toEqual({ reminded: 0, expired: 0 }); + expect(await listNotificationDeliveriesForRecipient(env, "alice", { limit: 50 })).toHaveLength(0); + }); + + it("re-notifies a row the maintainer has left waiting, and only once per interval", async () => { + const env = createTestEnv(); + const id = await stage(env, 2); + const stagedAt = Date.parse((await getPendingAgentAction(env, id))!.createdAt); + + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ reminded: 1, expired: 0 }); + // The sweep runs every couple of minutes; a second pass inside the same interval must not re-badge. + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_REMINDER_INTERVAL_MS + 60_000)).toEqual({ reminded: 0, expired: 0 }); + // The next interval is a new bucket → a new badge, which is the point: one prompt was never enough. + expect(await sweepStaleApprovalQueue(env, stagedAt + 2 * APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ reminded: 1, expired: 0 }); + + const deliveries = await listNotificationDeliveriesForRecipient(env, "alice", { limit: 50 }); + expect(deliveries).toHaveLength(2); + expect(deliveries[0]?.title).toContain("Still waiting"); + expect(deliveries.every((delivery) => delivery.recipientLogin === "alice")).toBe(true); + }); + + it("still writes a readable reminder for a row staged without a reason", async () => { + const env = createTestEnv(); + const { action } = await createPendingAgentActionIfAbsent(env, { + repoFullName: "alice/repo", + pullNumber: 20, + installationId: 42, + actionClass: "close", + autonomyLevel: "auto_with_approval", + params: {}, + reason: null, + }); + const stagedAt = Date.parse(action.createdAt); + + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ reminded: 1, expired: 0 }); + const [delivery] = await listNotificationDeliveriesForRecipient(env, "alice", { limit: 5 }); + expect(delivery?.body).toContain("A staged action"); + expect(delivery?.title).toContain("1 day ago"); + }); + + it("records the audit trail even when the audit write itself fails", async () => { + const env = createTestEnv(); + const id = await stage(env, 21); + const stagedAt = Date.parse((await getPendingAgentAction(env, id))!.createdAt); + const original = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((query: string) => { + if (query.includes("audit_events")) throw new Error("audit write failed"); + return original(query); + }); + + // The expiry itself must still stand — the audit row is a record of it, not a precondition for it. + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_EXPIRY_MS)).toEqual({ reminded: 0, expired: 1 }); + vi.restoreAllMocks(); + expect((await getPendingAgentAction(env, id))?.status).toBe("expired"); + }); + + it("expires a row nobody ever decided, records it, and executes nothing", async () => { + const env = createTestEnv(); + const id = await stage(env, 3); + const stagedAt = Date.parse((await getPendingAgentAction(env, id))!.createdAt); + + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_EXPIRY_MS)).toEqual({ reminded: 0, expired: 1 }); + + const row = await getPendingAgentAction(env, id); + // Expiry is NOT a rejection: a rejection is a maintainer's judgment and feeds the trust loop as such. + expect({ status: row?.status, decidedBy: row?.decidedBy }).toEqual({ status: "expired", decidedBy: "loopover" }); + const audits = await listAuditEventsForTarget(env, { repoFullName: "alice/repo", pullNumber: 3, limit: 50 }); + expect(audits.some((event) => event.eventType === "agent.pending_action.expired")).toBe(true); + }); + + it("is idempotent — an already-expired row is not swept again", async () => { + const env = createTestEnv(); + const id = await stage(env, 4); + const stagedAt = Date.parse((await getPendingAgentAction(env, id))!.createdAt); + await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_EXPIRY_MS); + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_EXPIRY_MS + DAY)).toEqual({ reminded: 0, expired: 0 }); + }); + + it("never touches a row a maintainer already decided", async () => { + const env = createTestEnv(); + const id = await stage(env, 5); + await setPendingAgentActionStatus(env, id, { status: "accepted", decidedBy: "alice" }); + expect(await sweepStaleApprovalQueue(env, Date.now() + 90 * DAY)).toEqual({ reminded: 0, expired: 0 }); + expect((await getPendingAgentAction(env, id))?.status).toBe("accepted"); + }); + + it("keeps going when one row's notification write fails — one repo must not stall the queue", async () => { + const env = createTestEnv(); + await stage(env, 6); + await stage(env, 7); + const stagedAt = Date.parse((await listPendingAgentActions(env, { status: "pending" }))[0]!.createdAt); + const original = env.DB.prepare.bind(env.DB); + let failuresLeft = 1; + vi.spyOn(env.DB, "prepare").mockImplementation((query: string) => { + if (failuresLeft > 0 && query.includes("notification_deliveries")) { + failuresLeft -= 1; + throw new Error("write failed"); + } + return original(query); + }); + + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_REMINDER_INTERVAL_MS)).toEqual({ reminded: 1, expired: 0 }); + vi.restoreAllMocks(); + }); + + it("survives a failed expiry claim without counting it", async () => { + const env = createTestEnv(); + const id = await stage(env, 8); + const stagedAt = Date.parse((await getPendingAgentAction(env, id))!.createdAt); + const original = env.DB.prepare.bind(env.DB); + vi.spyOn(env.DB, "prepare").mockImplementation((query: string) => { + if (query.startsWith("update") && query.includes("agent_pending_actions")) throw new Error("claim failed"); + return original(query); + }); + + expect(await sweepStaleApprovalQueue(env, stagedAt + APPROVAL_EXPIRY_MS)).toEqual({ reminded: 0, expired: 0 }); + vi.restoreAllMocks(); + expect((await getPendingAgentAction(env, id))?.status).toBe("pending"); + }); +}); diff --git a/test/unit/job-dispatch.test.ts b/test/unit/job-dispatch.test.ts index 27b6962c10..125ae0e9cd 100644 --- a/test/unit/job-dispatch.test.ts +++ b/test/unit/job-dispatch.test.ts @@ -104,3 +104,43 @@ describe("processJob backfill-registered-repos fan-out isolation (#8355)", () => expect(errorLogs.some((line) => line.includes("backfill_registered_repos_fanout_send_failed"))).toBe(false); }); }); + +// #9032: the approval-queue staleness pass rides the re-gate sweep's own fan-out tick rather than adding a job +// type and a cron entry for a bounded DB scan. It must be best-effort and must run BEFORE the fan-out — a +// failure sweeping the queue cannot be allowed to cost the tick its actual re-gate work. +describe("agent-regate-sweep also sweeps the stale approval queue (#9032)", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("logs what it swept when a pending row was reminded or expired", async () => { + const env = createTestEnv(); + const logs: string[] = []; + vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => void logs.push(String(args[0]))); + const approvalQueue = await import("../../src/services/agent-approval-queue"); + vi.spyOn(approvalQueue, "sweepStaleApprovalQueue").mockResolvedValue({ reminded: 2, expired: 1 }); + + await processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule" }); + + const swept = logs.map((line) => JSON.parse(line) as Record).find((log) => log.event === "approval_queue_staleness_swept"); + expect(swept).toMatchObject({ reminded: 2, expired: 1 }); + }); + + it("stays quiet when there was nothing to sweep", async () => { + const env = createTestEnv(); + const logs: string[] = []; + vi.spyOn(console, "log").mockImplementation((...args: unknown[]) => void logs.push(String(args[0]))); + + await processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule" }); + + expect(logs.map((line) => JSON.parse(line) as Record).some((log) => log.event === "approval_queue_staleness_swept")).toBe(false); + }); + + it("still fans out the re-gate work when the approval sweep throws", async () => { + const env = createTestEnv(); + const approvalQueue = await import("../../src/services/agent-approval-queue"); + vi.spyOn(approvalQueue, "sweepStaleApprovalQueue").mockRejectedValue(new Error("db down")); + + await expect(processJob(env, { type: "agent-regate-sweep", requestedBy: "schedule" })).resolves.toBeUndefined(); + }); +}); diff --git a/test/unit/low-confidence-hold-cap.test.ts b/test/unit/low-confidence-hold-cap.test.ts new file mode 100644 index 0000000000..f0e1b4dfea --- /dev/null +++ b/test/unit/low-confidence-hold-cap.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it } from "vitest"; +import { bumpPullRequestLowConfidenceHold, getPullRequest, listAuditEventsForTarget, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; +import { AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP, applyLowConfidenceHoldCap, isLowConfidenceHoldCapped } from "../../src/review/low-confidence-hold-cap"; +import { createTestEnv } from "../helpers/d1"; + +async function seedPr(env: Env, headSha: string): Promise { + await upsertInstallation(env, { + installation: { id: 31, account: { login: "alice", id: 31, type: "User" }, repository_selection: "selected", permissions: { metadata: "read" }, events: ["pull_request"] }, + }); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "alice/repo", private: false, owner: { login: "alice" } }, 31); + await upsertPullRequestFromGitHub(env, "alice/repo", { number: 9, title: "PR", state: "open", user: { login: "bob" }, head: { sha: headSha }, labels: [], body: "b" }); +} + +async function push(env: Env, headSha: string): Promise { + await upsertPullRequestFromGitHub(env, "alice/repo", { number: 9, title: "PR", state: "open", user: { login: "bob" }, head: { sha: headSha }, labels: [], body: "b" }); +} + +// #9034: a blocker below the close-confidence floor still blocks, but under the default `hold_for_review` +// disposition it converts a one-shot close into an OPEN hold — with nothing counting how many times the same PR +// re-entered that hold. So a PR shaped to keep drawing low-confidence blockers survived indefinitely, cost a +// maintainer on every roll, and (with the re-roll surface) could be walked toward a clean merge from there. +describe("bumpPullRequestLowConfidenceHold (#9034)", () => { + it("counts each distinct head exactly once, however many passes re-evaluate it", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + + // The re-gate sweep, a CI event and a label webhook can all re-evaluate one commit within minutes; every + // one of them would otherwise burn the cap against a single genuine hold. + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1")).toBe(1); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1")).toBe(1); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1")).toBe(1); + + const stored = await getPullRequest(env, "alice/repo", 9); + expect({ count: stored?.lowConfidenceHoldCount, head: stored?.lowConfidenceHoldHeadSha }).toEqual({ count: 1, head: "sha-1" }); + }); + + it("advances on each new roll and never resets on a push — repeated holds ARE the pattern being capped", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1")).toBe(1); + await push(env, "sha-2"); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-2")).toBe(2); + await push(env, "sha-3"); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-3")).toBe(3); + await push(env, "sha-4"); + // The (CAP + 1)th roll is the one that exceeds the cap, so the close the hold was suppressing fires. + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-4")).toBe(AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP + 1); + }); + + it("does not count a pass with no head SHA — a sparse payload must not silently exhaust the cap", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1"); + + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, null)).toBe(1); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, undefined)).toBe(1); + expect((await getPullRequest(env, "alice/repo", 9))?.lowConfidenceHoldHeadSha).toBe("sha-1"); + }); + + it("returns zero for a PR that has no row rather than creating one", async () => { + const env = createTestEnv(); + expect(await bumpPullRequestLowConfidenceHold(env, "alice/repo", 404, "sha-1")).toBe(0); + expect(await getPullRequest(env, "alice/repo", 404)).toBeNull(); + }); + + it("leaves the counter alone across an ordinary GitHub resync", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestLowConfidenceHold(env, "alice/repo", 9, "sha-1"); + await push(env, "sha-2"); + await push(env, "sha-3"); + // Pushes alone never advance it — only an actual low-confidence hold does. + expect((await getPullRequest(env, "alice/repo", 9))?.lowConfidenceHoldCount).toBe(1); + }); + + it("caps at a number that still allows a real uncertain verdict to reach a human more than once", () => { + expect(AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP).toBeGreaterThanOrEqual(2); + }); + + it("trips only once the budget is exceeded, not on the last hold inside it", () => { + expect(isLowConfidenceHoldCapped(0)).toBe(false); + expect(isLowConfidenceHoldCapped(AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP)).toBe(false); + expect(isLowConfidenceHoldCapped(AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP + 1)).toBe(true); + }); +}); + +describe("applyLowConfidenceHoldCap (#9034)", () => { + const hold = { reason: "an AI-reviewer defect finding's confidence is below the configured close-confidence floor (0.93)" }; + const target = (headSha: string | null) => ({ repoFullName: "alice/repo", pullNumber: 9, headSha }); + + it("passes through when there is no hold to cap", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + expect(await applyLowConfidenceHoldCap(env, target("sha-1"), undefined)).toBeUndefined(); + // Nothing was counted — a pass with no hold must not spend budget. + expect((await getPullRequest(env, "alice/repo", 9))?.lowConfidenceHoldCount).toBe(0); + }); + + it("keeps holding while the PR still has budget", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + expect(await applyLowConfidenceHoldCap(env, target("sha-1"), hold)).toBe(hold); + await push(env, "sha-2"); + expect(await applyLowConfidenceHoldCap(env, target("sha-2"), hold)).toBe(hold); + }); + + it("lifts the hold once the budget is spent, and says so in the audit trail", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + for (let roll = 1; roll <= AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP; roll += 1) { + await push(env, `sha-${roll}`); + expect(await applyLowConfidenceHoldCap(env, target(`sha-${roll}`), hold)).toBe(hold); + } + await push(env, "sha-over"); + + // The roll past the cap: the close this hold was suppressing now fires. + expect(await applyLowConfidenceHoldCap(env, target("sha-over"), hold)).toBeUndefined(); + + const audits = await listAuditEventsForTarget(env, { repoFullName: "alice/repo", pullNumber: 9, limit: 20 }); + const capped = audits.find((event) => event.eventType === "agent.low_confidence_hold.capped"); + expect(capped?.detail).toContain("the suppressed close now proceeds"); + }); + + it("stays lifted on every later pass, not just the one that crossed the cap", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + for (let roll = 1; roll <= AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP + 1; roll += 1) { + await push(env, `sha-${roll}`); + await applyLowConfidenceHoldCap(env, target(`sha-${roll}`), hold); + } + await push(env, "sha-later"); + expect(await applyLowConfidenceHoldCap(env, target("sha-later"), hold)).toBeUndefined(); + }); +}); diff --git a/test/unit/merge-block-recovery.test.ts b/test/unit/merge-block-recovery.test.ts new file mode 100644 index 0000000000..6128f8f318 --- /dev/null +++ b/test/unit/merge-block-recovery.test.ts @@ -0,0 +1,188 @@ +import { describe, expect, it } from "vitest"; +import { getPullRequest, markPullRequestMergeBlocked, bumpPullRequestMergeAttempt, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertInstallation } from "../../src/db/repositories"; +import { activeMergeBlockedSha, classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeBlockInEffect } from "../../src/services/merge-failure"; +import { AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, planAgentMaintenanceActions } from "../../src/settings/agent-actions"; +import { createTestEnv } from "../helpers/d1"; + +function httpError(status: number, message: string): Error & { status: number } { + return Object.assign(new Error(message), { status }); +} + +async function seedPr(env: Env, headSha: string): Promise { + await upsertInstallation(env, { + installation: { id: 77, account: { login: "alice", id: 77, type: "User" }, repository_selection: "selected", permissions: { metadata: "read" }, events: ["pull_request"] }, + }); + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "alice/repo", private: false, owner: { login: "alice" } }, 77); + await upsertPullRequestFromGitHub(env, "alice/repo", { number: 5, title: "PR", state: "open", user: { login: "bob" }, head: { sha: headSha }, labels: [], body: "b" }); +} + +// #9012: `merge_blocked_sha` was written for every terminal class and cleared by nothing — its only documented +// escape was the contributor pushing a new commit. But a 401 (App suspended / key rotated) and an exhausted +// secondary-rate-limit window are properties of the INSTALLATION, not of the commit: they fail every in-flight +// merge in the fleet at once, and no contributor has any reason to push, because the PR looks green, approved, +// and (with review_state_label on) ready-to-merge. One token rotation therefore stranded every merge it caught, +// permanently and invisibly. Infra-scoped blocks now carry an expiry and are re-probed. +describe("terminal merge failures distinguish infra causes from commit causes (#9012)", () => { + it("scopes a rejected installation token and an exhausted rate-limit window to infra, and real policy/conflict causes to the commit", () => { + expect(classifyMergeFailure(httpError(401, "Bad credentials")).scope).toBe("infra"); + expect(classifyMergeFailure(httpError(403, "You have exceeded a secondary rate limit")).scope).toBe("infra"); + expect(classifyMergeFailure(httpError(403, "Must have admin rights")).scope).toBe("commit"); + expect(classifyMergeFailure(httpError(405, "Pull Request is not mergeable")).scope).toBe("commit"); + expect(classifyMergeFailure(httpError(405, "Base branch was modified")).scope).toBe("commit"); + expect(classifyMergeFailure(httpError(405, "A merge for this pull request is already in progress")).scope).toBe("commit"); + expect(classifyMergeFailure(httpError(409, "required status check is expected")).scope).toBe("commit"); + expect(classifyMergeFailure(new Error("merge conflict between base and head")).scope).toBe("commit"); + expect(classifyMergeFailure(new Error("something else entirely"))).toEqual({ terminal: false, scope: "commit", reason: "something else entirely" }); + }); + + it("keeps a 401 terminal for the pass — failing fast against a known-bad credential is still the point", () => { + expect(classifyMergeFailure(httpError(401, "Bad credentials")).terminal).toBe(true); + }); +}); + +describe("a merge block only suppresses the merge while it is actually in effect (#9012)", () => { + const NOW = Date.parse("2026-07-26T12:00:00.000Z"); + + it("holds a commit-scoped block (no expiry) for as long as the head is unchanged", () => { + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: null }, "abc", NOW)).toBe(true); + }); + + it("lets go once the head advances, for either scope", () => { + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: null }, "def", NOW)).toBe(false); + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: new Date(NOW + 60_000).toISOString() }, "def", NOW)).toBe(false); + }); + + it("holds an infra-scoped block until its expiry and releases it after — the whole recovery path", () => { + const until = new Date(NOW + 60_000).toISOString(); + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: until }, "abc", NOW)).toBe(true); + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: until }, "abc", NOW + 61_000)).toBe(false); + }); + + it("treats an absent block, an absent head, and an unparseable expiry as not-blocked", () => { + expect(isMergeBlockInEffect({ mergeBlockedSha: null, mergeBlockedUntil: null }, "abc", NOW)).toBe(false); + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: null }, null, NOW)).toBe(false); + expect(isMergeBlockInEffect({}, "abc", NOW)).toBe(false); + // A malformed timestamp must never be the thing that strands a green PR forever — that is the exact + // failure this fix removes, so it must not be reintroduced by a bad write. + expect(isMergeBlockInEffect({ mergeBlockedSha: "abc", mergeBlockedUntil: "not-a-date" }, "abc", NOW)).toBe(false); + }); + + it("hands the planner the stored SHA while blocked and null once the block lapses", () => { + const until = new Date(NOW + 60_000).toISOString(); + expect(activeMergeBlockedSha({ mergeBlockedSha: "abc", mergeBlockedUntil: until }, "abc", NOW)).toBe("abc"); + // A lapsed block must be indistinguishable from never having been blocked — that is what makes the planner + // re-probe the merge instead of waiting for a commit nobody has any reason to push. + expect(activeMergeBlockedSha({ mergeBlockedSha: "abc", mergeBlockedUntil: until }, "abc", NOW + 61_000)).toBeNull(); + expect(activeMergeBlockedSha({ mergeBlockedSha: null, mergeBlockedUntil: null }, "abc", NOW)).toBeNull(); + expect(activeMergeBlockedSha({ mergeBlockedSha: "abc", mergeBlockedUntil: null }, "abc", NOW)).toBe("abc"); + }); + + it("is a real TTL, not a token constant", () => { + expect(INFRA_MERGE_BLOCK_TTL_MS).toBeGreaterThan(60_000); + }); +}); + +describe("markPullRequestMergeBlocked persists the scope (#9012)", () => { + it("writes no expiry for a commit-scoped block and leaves the attempt counter alone", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "branch conflicts with base"); + + const stored = await getPullRequest(env, "alice/repo", 5); + expect({ until: stored?.mergeBlockedUntil ?? null, attempts: stored?.mergeAttemptCount }).toEqual({ until: null, attempts: 1 }); + expect(stored?.mergeBlockedReason).toContain("conflicts"); + }); + + it("writes an expiry for an infra-scoped block and zeroes the attempt counter so the re-probe starts fresh", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + const expiresAt = new Date(Date.now() + INFRA_MERGE_BLOCK_TTL_MS).toISOString(); + await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "installation token rejected (401)", expiresAt); + + const stored = await getPullRequest(env, "alice/repo", 5); + expect({ until: stored?.mergeBlockedUntil, attempts: stored?.mergeAttemptCount }).toEqual({ until: expiresAt, attempts: 0 }); + // And the block genuinely lapses: this is the "merges autonomously with no new commit" acceptance criterion. + expect(isMergeBlockInEffect(stored!, "sha-1", Date.parse(expiresAt) + 1)).toBe(false); + }); + + it("truncates an overlong reason at the persisted 280-char cap", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "x".repeat(400)); + expect((await getPullRequest(env, "alice/repo", 5))?.mergeBlockedReason).toHaveLength(280); + }); +}); + +// #9012 compounding bug: mergeAttemptCount's own schema and function docs promised "a new commit's attempts +// start fresh once the row's head advances", but nothing reset it — bumpPullRequestMergeAttempt only scoped the +// INCREMENT to the head. So once one head exhausted MERGE_RETRY_CAP, every later head was one-strike-terminal. +describe("the failed-merge attempt counter resets when the head advances (#9012)", () => { + it("zeroes on a real new commit", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + expect((await getPullRequest(env, "alice/repo", 5))?.mergeAttemptCount).toBe(2); + + await upsertPullRequestFromGitHub(env, "alice/repo", { number: 5, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "sha-2" }, labels: [], body: "b" }); + expect((await getPullRequest(env, "alice/repo", 5))?.mergeAttemptCount).toBe(0); + }); + + it("does NOT zero on an ordinary resync of the same head — a genuinely failing merge keeps its bounded budget", async () => { + const env = createTestEnv(); + await seedPr(env, "sha-1"); + await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1"); + await upsertPullRequestFromGitHub(env, "alice/repo", { number: 5, title: "PR retitled", state: "open", user: { login: "bob" }, head: { sha: "sha-1" }, labels: [], body: "b" }); + expect((await getPullRequest(env, "alice/repo", 5))?.mergeAttemptCount).toBe(1); + }); +}); + +// #9012's "why it's silent": with review_state_label enabled, a terminally merge-blocked PR kept the +// ready-to-merge label and the block appeared on no human-visible surface at all — planner, audit and PostHog +// were the only readers of mergeBlockedReason. +describe("a terminally merge-blocked PR is labelled for review and names the reason (#9012)", () => { + const baseInput = { + conclusion: "success" as const, + blockerTitles: [] as string[], + autoMaintain: { requireApprovals: 1, mergeMethod: "squash" as const }, + slopGateMinScore: 60, + changedPaths: [] as string[], + hardGuardrailGlobs: [] as string[], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "passed" as const, + autonomy: { merge: "auto" as const, review_state_label: "auto" as const }, + pr: { labels: [] as string[], mergeableState: "clean", reviewDecision: "APPROVED", headSha: "sha-1" }, + }; + + function planWith(pr: Record): ReturnType { + return planAgentMaintenanceActions({ ...baseInput, pr: { ...baseInput.pr, ...pr } } as Parameters[0]); + } + + it("labels ready-to-merge when nothing is blocked", () => { + const labels = planWith({}).filter((action) => action.actionClass === "label"); + expect(labels.some((action) => action.label === AGENT_LABEL_READY && action.labelOp !== "remove")).toBe(true); + }); + + it("swaps the ready-to-merge promise for the manual-review label once the merge is terminally blocked", () => { + const labels = planWith({ mergeBlockedSha: "sha-1", mergeBlockedReason: "merge not allowed (405)" }).filter((action) => action.actionClass === "label"); + const added = labels.filter((action) => action.labelOp !== "remove"); + expect(added.some((action) => action.label === AGENT_LABEL_READY)).toBe(false); + expect(added.some((action) => action.label === AGENT_LABEL_NEEDS_REVIEW)).toBe(true); + // The reason must actually reach the human — a "needs review" label with no cause is what made this silent. + expect(added.find((action) => action.label === AGENT_LABEL_NEEDS_REVIEW)?.reason).toContain("merge not allowed (405)"); + }); + + it("still names the label when the stored reason is missing, rather than emitting 'undefined'", () => { + const labels = planWith({ mergeBlockedSha: "sha-1" }).filter((action) => action.actionClass === "label" && action.labelOp !== "remove"); + expect(labels.find((action) => action.label === AGENT_LABEL_NEEDS_REVIEW)?.reason).toContain("reason unrecorded"); + }); + + it("plans no merge while blocked, and plans one once the block is gone", () => { + expect(planWith({ mergeBlockedSha: "sha-1" }).some((action) => action.actionClass === "merge")).toBe(false); + expect(planWith({ mergeBlockedSha: null }).some((action) => action.actionClass === "merge")).toBe(true); + }); +});