fix(review): production reliability pass (budget/loop/merge/notify)

migrations/0052_pr_merge_attempt_terminal.sql

@@ -0,0 +1,21 @@
+-- RC3 (terminal-fail merges): stop the auto-maintain merge retry-forever loop.
+--
+-- BEFORE: executeAgentMaintenanceActions() calls mergePullRequest(); a 403 (Resource not accessible) / 405
+-- (method not allowed) / 409 (required check absent) / conflict throws, the action is recorded as an `error`
+-- audit row, but the pull_requests row stays plannable — so EVERY webhook + every scheduled re-gate sweep
+-- re-plans the same merge and it fails again, with no cap and no backoff. (reviewbot parity: review_targets'
+-- attempt_count + terminal_at, which gittensory's normalized planner path never had.)
+--
+-- AFTER: a non-transient merge failure marks the PR terminally merge-blocked FOR THE CURRENT HEAD SHA. The
+-- planner skips planning a merge while merge_blocked_sha == headSha, and the executor caps retries via
+-- merge_attempt_count so even a misclassified transient failure escalates to a human instead of looping.
+--
+-- merge_blocked_sha is keyed to the head SHA so a NEW commit (which upsertPullRequestFromGitHub writes) clears
+-- the block automatically — a pushed fix gets a fresh merge attempt without any manual reset.
+--
+-- pull_requests IS a Drizzle table (src/db/schema.ts), so these columns are added to the Drizzle schema too;
+-- this raw migration is the production DDL applied by `wrangler d1 migrations apply` (drizzle-kit is not the
+-- runtime migrator here).
+ALTER TABLE pull_requests ADD COLUMN merge_attempt_count INTEGER NOT NULL DEFAULT 0;
+ALTER TABLE pull_requests ADD COLUMN merge_blocked_sha TEXT;
+ALTER TABLE pull_requests ADD COLUMN merge_blocked_reason TEXT;

src/db/repositories.ts

@@ -2492,9 +2492,39 @@
     .from(pullRequests)
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number)))
     .limit(1);
   return row ? toPullRequestRecordFromRow(row) : null;
 }
 
+// RC3 terminal-fail merges. The auto-maintain executor calls these when a merge mutation fails so the planner
+// stops planning a merge it can never complete (403/405/409/conflict), instead of retrying every sweep forever.
+
+/** Increment the failed-merge attempt counter for a PR, scoped to the head SHA that failed. Returns the new
+ *  count. Scoping to headSha means a new commit's attempts start fresh once the row's head advances. */
+export async function bumpPullRequestMergeAttempt(env: Env, fullName: string, number: number, headSha: string): Promise<number> {
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ mergeAttemptCount: sql`${pullRequests.mergeAttemptCount} + 1`, updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
+  const [row] = await db
+    .select({ count: pullRequests.mergeAttemptCount })
+    .from(pullRequests)
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number)))
+    .limit(1);
+  return Number(row?.count ?? 0);
+}
+
+/** 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<void> {
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ mergeBlockedSha: headSha, mergeBlockedReason: reason.slice(0, 280), updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
+}
+
 export async function getIssue(env: Env, fullName: string, number: number): Promise<IssueRecord | null> {
   const db = getDb(env.DB);
   const [row] = await db.select().from(issues).where(and(eq(issues.repoFullName, fullName), eq(issues.number, number))).limit(1);
@@ -3897,6 +3927,9 @@
     linkedIssues: parseJson<number[]>(row.linkedIssuesJson, []),
     slopRisk: row.slopRisk,
     slopBand: row.slopBand,
+    mergeAttemptCount: row.mergeAttemptCount,
+    mergeBlockedSha: row.mergeBlockedSha,
+    mergeBlockedReason: row.mergeBlockedReason,
   };
 }
 

src/db/schema.ts

@@ -273,9 +273,15 @@
     linkedIssuesJson: text("linked_issues_json").notNull().default("[]"),
     lastSeenOpenAt: text("last_seen_open_at"),
     payloadJson: text("payload_json").notNull().default("{}"),
     // Latest deterministic slop assessment (gittensory-computed; written separately from the GitHub sync).
     slopRisk: integer("slop_risk"),
     slopBand: text("slop_band"),
+    // 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. gittensory-computed (executor-written), omitted from the GitHub-sync SET clause.
+    mergeAttemptCount: integer("merge_attempt_count").notNull().default(0),
+    mergeBlockedSha: text("merge_blocked_sha"),
+    mergeBlockedReason: text("merge_blocked_reason"),
     createdAt: text("created_at").notNull().$defaultFn(() => nowIso()),
     updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()),
   },

src/github/backfill.ts

@@ -1907,12 +1907,45 @@
 
 const CI_FAILING_CONCLUSIONS = new Set(["failure", "timed_out", "cancelled", "action_required", "startup_failure"]);
 const CI_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]);
 
 export type LiveCiAggregate = {
   ciState: "passed" | "failed" | "pending" | "unverified";
+  // Checks that FAIL the gate: every failing check when required contexts are unknown, else only the failing
+  // REQUIRED contexts. These drive ciState === "failed" and the disposition (no-merge / close / request-changes).
   failingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>;
+  // RC2: checks that are RED but NOT in branch-protection's required set (e.g. codecov/patch, codecov/project).
+  // Surfaced to the contributor but they do NOT fail the gate, block merge/approve, or force request_changes.
+  // Empty when required contexts are unknown (best-effort fetch failed / no protection) — then every red check
+  // stays in failingDetails (byte-identical to pre-RC2).
+  nonRequiredFailingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>;
 };
 
+/**
+ * RC2 best-effort fetch of the base branch's branch-protection REQUIRED status-check contexts. Returns the set
+ * of required context names (covering both the legacy `contexts` array and the newer `checks[].context` shape),
+ * or `null` when none can be determined — a 404 (no protection / no required checks), a 403 (token lacks
+ * admin:repo, common for installations/forks), or any other error. `null`/empty makes fetchLiveCiAggregate fall
+ * back to folding ALL red checks into the gate, so a fetch failure can never silently pass a required red check.
+ */
+export async function fetchRequiredStatusContexts(env: Env, repoFullName: string, baseRef: string | null | undefined, token: string | undefined): Promise<Set<string> | null> {
+  if (!baseRef) return null;
+  const result = await githubJsonWithHeaders<{ contexts?: Array<string | null> | null; checks?: Array<{ context?: string | null }> | null }>(
+    env,
+    repoFullName,
+    `/branches/${encodeURIComponent(baseRef)}/protection/required_status_checks`,
+    token,
+  ).catch(() => undefined);
+  if (!result) return null; // 404 (no protection) / 403 (no admin) — treat as "unknown".
+  const names = new Set<string>();
+  for (const ctx of result.data.contexts ?? []) {
+    if (typeof ctx === "string" && ctx.trim().length > 0) names.add(ctx);
+  }
+  for (const check of result.data.checks ?? []) {
+    if (typeof check?.context === "string" && check.context.trim().length > 0) names.add(check.context);
+  }
+  return names;
+}
+
 /**
  * Fetch the head SHA's LIVE CI aggregate over BOTH GitHub Check-runs AND classic commit-statuses. This is the
  * reviewbot `getAllChecksState` parity that the converged auto-maintain path needs: codecov (codecov/patch,
@@ -1922,9 +1955,22 @@
  * present → "passed"; none at all → "unverified". The disposition layer NEVER approves/merges unless "passed",
  * and closes (non-owner) / holds (owner) on "failed". Best-effort: a fetch error degrades that source to empty.
  */
-export async function fetchLiveCiAggregate(env: Env, repoFullName: string, headSha: string | null | undefined, token: string | undefined): Promise<LiveCiAggregate> {
-  if (!headSha) return { ciState: "unverified", failingDetails: [] };
+export async function fetchLiveCiAggregate(
+  env: Env,
+  repoFullName: string,
+  headSha: string | null | undefined,
+  token: string | undefined,
+  // RC2: when a NON-EMPTY set, only these branch-protection-required contexts gate the PR — a red check outside
+  // the set is surfaced (nonRequiredFailingDetails) but never fails the gate. null/empty ⇒ fold ALL red checks
+  // (pre-RC2 behavior), the safe fallback when protection can't be read.
+  requiredContexts?: ReadonlySet<string> | null,
+): Promise<LiveCiAggregate> {
+  if (!headSha) return { ciState: "unverified", failingDetails: [], nonRequiredFailingDetails: [] };
+  // Only enforce a required subset when we actually resolved one; otherwise every red check is gate-failing.
+  const enforceRequiredOnly = requiredContexts != null && requiredContexts.size > 0;
+  const isRequired = (name: string): boolean => !enforceRequiredOnly || requiredContexts!.has(name);
   const failingDetails: LiveCiAggregate["failingDetails"] = [];
+  const nonRequiredFailingDetails: LiveCiAggregate["nonRequiredFailingDetails"] = [];
   let total = 0;
   let anyPending = false;
 
@@ -1943,11 +1989,12 @@
       const status = (run.status ?? "").toLowerCase();
       if (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false) {
         const summary = [run.output?.title, run.output?.summary].find((value): value is string => typeof value === "string" && value.trim().length > 0)?.trim().slice(0, 200);
-        failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) });
+        const detail = { name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) };
+        (isRequired(run.name) ? failingDetails : nonRequiredFailingDetails).push(detail);
       } else if (conclusion ? CI_PASSING_CONCLUSIONS.has(conclusion) : status === "completed") {
         // concluded and not failing → passing
-      } else {
-        anyPending = true; // queued / in_progress / not yet concluded
+      } else if (isRequired(run.name)) {
+        anyPending = true; // queued / in_progress / not yet concluded — only a REQUIRED check holds the gate
       }
     }
     if (!hasNextPage(result.link)) break;
@@ -1968,16 +2015,20 @@
     const name = ctx.context ?? "status";
     if (state === "failure" || state === "error") {
       const summary = typeof ctx.description === "string" ? ctx.description.trim().slice(0, 200) : "";
-      failingDetails.push({ name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) });
+      const detail = { name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) };
+      (isRequired(name) ? failingDetails : nonRequiredFailingDetails).push(detail);
     } else if (state === "success") {
       // passing
-    } else {
-      anyPending = true; // pending
+    } else if (isRequired(name)) {
+      anyPending = true; // pending — only a REQUIRED context holds the gate
     }
   }
 
+  // ciState reflects ONLY gate-failing (required, or all-when-unknown) checks. A repo whose only red check is a
+  // non-required codecov/* therefore reports "passed" and is eligible to merge/approve, with the codecov
+  // failure riding along in nonRequiredFailingDetails for the contributor to see.
   const ciState: LiveCiAggregate["ciState"] = failingDetails.length > 0 ? "failed" : anyPending ? "pending" : total > 0 ? "passed" : "unverified";
-  return { ciState, failingDetails };
+  return { ciState, failingDetails, nonRequiredFailingDetails };
 }
 
 /**
@@ -1993,6 +2044,20 @@
   return result?.data.mergeable_state ?? undefined;
 }
 
+/** RC1 (idempotent reviews): the PR's LIVE reviewDecision (APPROVED / CHANGES_REQUESTED / REVIEW_REQUIRED) via
+ *  GraphQL. The STORED reviewDecision is only written by the open-PR backfill and goes stale, so the action
+ *  planner's approve/request-changes dedup was blind and re-posted a review every cycle — the re-review loop.
+ *  Refreshing it live makes the dedup accurate. Best-effort: returns undefined on any error (caller falls back
+ *  to the stored value). */
+export async function fetchLivePullRequestReviewDecision(env: Env, repoFullName: string, prNumber: number, token: string | undefined): Promise<string | undefined> {
+  if (!token) return undefined;
+  const [owner, name] = repoFullName.split("/");
+  if (!owner || !name) return undefined;
+  const query = `query { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { pullRequest(number: ${prNumber}) { reviewDecision } } }`;
+  const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null } }>(env, query, token).catch(() => undefined);
+  return result?.data?.repository?.pullRequest?.reviewDecision ?? undefined;
+}
+
 async function fetchPullRequestDetailsFromGraphQl(
   env: Env,
   repoFullName: string,

src/github/pr-actions.ts

@@ -57,9 +57,32 @@
     ...(options.sha ? { sha: options.sha } : {}),
   });
   const data = response.data as { merged?: boolean; sha?: string };
   return { merged: data.merged ?? true, sha: data.sha ?? null };
 }
 
+/** Rebase a PR onto its base via GitHub's update-branch (merges the current base into the PR head). Keeps a
+ *  BEHIND PR current before reviewing/merging so the review + required CI run against the merged result —
+ *  reviewbot parity. `expectedHeadSha` guards against racing a head that moved since we read it. The PUT
+ *  returns 202 (update queued) on success; a caller treats any throw as best-effort (e.g. 422 when already
+ *  up to date or the branch is dirty/conflicting — those are handled by the gate, not retried here). */
+export async function updatePullRequestBranch(
+  env: Env,
+  installationId: number,
+  repoFullName: string,
+  pullNumber: number,
+  expectedHeadSha?: string | undefined,
+): Promise<void> {
+  const { owner, repo } = splitRepo(repoFullName);
+  const token = await createInstallationToken(env, installationId);
+  const octokit = new Octokit({ auth: token });
+  await octokit.request("PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch", {
+    owner,
+    repo,
+    pull_number: pullNumber,
+    ...(expectedHeadSha ? { expected_head_sha: expectedHeadSha } : {}),
+  });
+}
+
 /** Post a plain issue/PR comment (used for the templated close message before closing). */
 export async function createIssueComment(env: Env, installationId: number, repoFullName: string, issueNumber: number, body: string): Promise<{ id: number }> {
   const { owner, repo } = splitRepo(repoFullName);

src/index.ts

@@ -39,22 +39,25 @@
 async function enqueueScheduledJobs(env: Env, controller: ScheduledController): Promise<void> {
   const scheduledAt = new Date(controller.scheduledTime ?? Date.now());
   const minute = scheduledAt.getUTCMinutes();
   const hour = scheduledAt.getUTCHours();
   const isHourly = minute === 0;
   const isFullSyncWindow = isHourly && hour % 6 === 0;
-  const jobs: JobMessage[] = [
-    { type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" },
-    { type: "repair-data-fidelity", requestedBy: "schedule" },
-    { type: "refresh-installation-health", requestedBy: "schedule" },
-  ];
+  // The light auto-maintain sweep runs EVERY cron tick (~every 2 min) so an approved+clean PR MERGES and a
+  // red-CI non-owner PR CLOSES promptly — reviewbot parity (its cron fired every minute). It re-fetches LIVE CI +
+  // mergeable and only ACTS (merge/close/hold); it never re-runs the AI, so it is cheap enough for this cadence.
+  // Previously this was gated by `isHourly`, so an approved PR could wait ~an hour for its merge pass.
+  const jobs: JobMessage[] = [{ type: "agent-regate-sweep", requestedBy: "schedule" }];
+  // The heavier sync/health jobs keep their ~30-minute cadence even though the cron now ticks every ~2 minutes.
+  if (minute % 30 === 0) {
+    jobs.push({ type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" });
+    jobs.push({ type: "repair-data-fidelity", requestedBy: "schedule" });
+    jobs.push({ type: "refresh-installation-health", requestedBy: "schedule" });
+  }
   if (isHourly) {
     jobs.push({ type: "refresh-registry", requestedBy: "schedule" });
     jobs.push({ type: "refresh-scoring-model", requestedBy: "schedule" });
     jobs.push({ type: "refresh-upstream-drift", requestedBy: "schedule" });
     jobs.push({ type: "rollup-product-usage", requestedBy: "schedule", days: 7 });
-    // Agent layer (#777): re-gate stale open PRs hourly. Fans out to one job per agent-configured repo;
-    // webhooks don't fire when a PR's base advances, so this is what keeps those verdicts fresh.
-    jobs.push({ type: "agent-regate-sweep", requestedBy: "schedule" });
     // Convergence (ops / observability, flag GITTENSORY_REVIEW_OPS). Hourly anomaly scan over gittensory's own
     // review-outcome data. Enqueued ONLY when the flag is ON — flag-OFF (default) this job is never created,
     // so the cron tick does ZERO new work and the enqueued set is byte-identical to today.