From e6f659b14f107db9cfa2821f941f155afd09d676 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:08:02 -0700 Subject: [PATCH 1/4] fix(review): rebase only at the merge boundary, and stop repair jobs jumping the queue (#9497, #9498, #9499) The staleness rebase fired in the READINESS gate -- before the CI-pending wait, the gate verdict, the disposition plan, and the merge-train check. Since the default branch only advances when a PR merges, "N commits ahead" means "N unrelated PRs merged since your head", and every merge wakes a cohort of siblings that each ran this check. Production measured 7 PRs rebased in 12 seconds, 44s after an unrelated merge, with ci.yml's cancel-in-progress killing their running jobs; one cohort was force-rebased three times in 2.5 hours. A joint rebase also resets that cohort to zero-behind simultaneously, so they stay phase-locked and re-cross the threshold together rather than settling. And running before the plan meant a PR about to be auto-closed, on red CI, or held for manual review was rebased first and that CI run thrown away. The threshold now shares maybeForceFreshRebase's call site, inheriting the three guards the readiness path had none of: imminent merge only, clean mergeable state only, and the 3-per-PR-per-24h cap. That also kills the self-feeding loop -- no speculative rebase means no stale surface, which means no repair-priority re-gate. isRegateRepairExhausted keyed its 5-attempt budget on repo#pr#headSha, so a successful rebase minted a new SHA and reset the budget to zero -- the identical bug MAX_FRESH_REBASE_FORCES already fixed for the fresh-rebase counter, whose comment explains why SHA-keying makes a cap unreachable. Re-keyed to repo#pr. Note this resets existing budgets once on deploy, since historical audit rows carry the old SHA-suffixed target. Two more agent-regate-pr producers omitted prCreatedAt, so jobClaimSortKey fell back to a legacy base (~9.5e11) that sorts AHEAD of every real 2026 PR (~1.78e12) -- silently preempting older contributor work. Threaded through the open-PR reconciler, the surface-disposition reconciler and the contributor-cap wake. GitHub's workflow-scope refusal is now classified as permanent for a diff shape rather than retried: update_branch merges the base in, so any workflow change on the default branch since the fork makes the merge a workflow write even when the PR touches none -- 48 of 82 failures in one 7-day window, one PR retried nine times. --- src/queue/processors.ts | 120 ++++++++++++------ src/review/pr-reconciliation.ts | 5 +- src/review/surface-disposition-reconciler.ts | 9 +- src/services/agent-action-executor.ts | 8 +- src/services/merge-failure.ts | 19 +++ test/unit/merge-failure.test.ts | 35 ++++- test/unit/queue-2.test.ts | 24 ++-- test/unit/queue-3.test.ts | 54 +++++++- test/unit/queue-4.test.ts | 34 +++-- .../surface-disposition-reconciler.test.ts | 7 +- test/unit/surface-repair-priority.test.ts | 13 +- 11 files changed, 251 insertions(+), 77 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9344c12276..c3387deb55 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -1112,21 +1112,33 @@ async function refreshOpenPullRequestsForScheduledSweep( // surfaceRepairPriorityPullNumbers has no memory of prior attempts -- if the repair keeps failing for the SAME // head SHA (e.g. every AI-provider attempt times out), it would otherwise re-select that PR forever, burning a // fresh review attempt every cycle for zero output. These two constants cap that: once a SHA has already had -// REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA dispatches recorded, it drops back to ordinary staleness-gated candidacy +// REGATE_REPAIR_MAX_ATTEMPTS_PER_PR dispatches recorded, it drops back to ordinary staleness-gated candidacy // (still eventually re-checked, just not on every tick) and a single REGATE_REPAIR_EXHAUSTED_EVENT_TYPE audit // event is recorded so the stuck PR is visible instead of silently retried forever. A new commit changes the // head SHA, which resets the count naturally (the target key is scoped to repo+PR+SHA). const REGATE_REPAIR_ATTEMPT_EVENT_TYPE = "agent.sweep.regate.repair_attempt"; const REGATE_REPAIR_EXHAUSTED_EVENT_TYPE = "agent.sweep.regate.repair_exhausted"; -const REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA = 5; +// #9499: renamed from ..._PER_SHA -- the budget is now per PR, since SHA-keying let a rebase reset it. +const REGATE_REPAIR_MAX_ATTEMPTS_PER_PR = 5; const REGATE_REPAIR_ATTEMPT_LOOKBACK_MS = 24 * 60 * 60 * 1000; -function regateRepairTargetKey(repoFullName: string, prNumber: number, headSha: string): string { - return `${repoFullName}#${prNumber}#${headSha}`; +/** + * #9499: keyed on PR NUMBER, not head SHA. + * + * Keying the 5-attempt repair budget on `repo#pr#headSha` meant a SUCCESSFUL rebase -- which mints a new head + * SHA -- reset the budget to zero. The repair path then re-ran indefinitely on a PR whose head kept moving, + * which is exactly the self-feeding loop the speculative rebase created: rebase -> stale surface -> repair + * priority (which bypasses both the freshness guard and #never-endless-reregate) -> re-gate -> rebase. + * + * This is the identical bug MAX_FRESH_REBASE_FORCES already fixed for the fresh-rebase counter, whose own + * comment explains at length why SHA-keying makes a cap unreachable. Same fix, same reasoning. + */ +function regateRepairTargetKey(repoFullName: string, prNumber: number, _headSha: string): string { + return `${repoFullName}#${prNumber}`; } /** - * True when `pr`'s current head SHA has already exhausted REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA repair attempts + * True when `pr`'s current head SHA has already exhausted REGATE_REPAIR_MAX_ATTEMPTS_PER_PR repair attempts * within the lookback window. Records (at most once per SHA) the exhausted audit event + error-level log as a * side effect the first time a SHA crosses the cap. Shared by both surfaceRepairPriorityPullNumbers and * sweepRepoBacklogConvergence (#orb-retry-storm, backlog-convergence half): both sweeps re-select on the @@ -1138,7 +1150,7 @@ async function isRegateRepairExhausted(env: Env, repoFullName: string, pr: Pick< const sinceIso = new Date(Date.now() - REGATE_REPAIR_ATTEMPT_LOOKBACK_MS).toISOString(); const targetKey = regateRepairTargetKey(repoFullName, pr.number, headSha); const attempts = await countRecentAuditEventsForActorAndTarget(env, "loopover", REGATE_REPAIR_ATTEMPT_EVENT_TYPE, targetKey, sinceIso); - if (attempts < REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA) return false; + if (attempts < REGATE_REPAIR_MAX_ATTEMPTS_PER_PR) return false; const alreadyFlagged = await countRecentAuditEventsForActorAndTarget(env, "loopover", REGATE_REPAIR_EXHAUSTED_EVENT_TYPE, targetKey, sinceIso); if (alreadyFlagged === 0) { await recordAuditEvent(env, { @@ -3903,9 +3915,12 @@ async function runAgentMaintenancePlanAndExecute( // stages for a human, not an immediate merge). A forced rebase's resulting `synchronize` webhook re-triggers // a fresh evaluation on the new head, so this pass stops here rather than executing against stale inputs. const requireFreshRebaseWindowMinutes = settings.requireFreshRebaseWindowMinutes; + // #9497: the staleness threshold now shares this call site, so it inherits planHasImminentMerge, the + // clean-mergeable requirement, and the 3-per-PR-per-24h cap that the readiness-gate version had none of. + const staleBaseAheadByThreshold = settings.staleBaseAheadByThreshold; const planHasImminentMerge = holdoutOnPlan.some((action) => action.actionClass === "merge" && !action.requiresApproval); if ( - typeof requireFreshRebaseWindowMinutes === "number" && + (typeof requireFreshRebaseWindowMinutes === "number" || typeof staleBaseAheadByThreshold === "number") && baseRef && planHasImminentMerge && (liveMergeState ?? pr.mergeableState) === "clean" && @@ -3915,6 +3930,7 @@ async function runAgentMaintenancePlanAndExecute( pr, settings, windowMinutes: requireFreshRebaseWindowMinutes, + staleBaseAheadByThreshold, baseRef, token, admissionKey, @@ -4421,26 +4437,27 @@ async function prReadyForReview( if (await forceUpdateBranch("behind base; update-branch before review")) { return false; // the rebase fires a synchronize → fresh review runs on the new head } - } else if (typeof settings.staleBaseAheadByThreshold === "number") { - // 1b) #review-grounding stale-base fact companion (metagraphed #7305-class incident): mergeable_state only - // ever reports "behind" when the repo's branch protection requires branches to be up to date before - // merging -- a repo without that setting can have a branch genuinely dozens of commits behind and GitHub - // will never surface it here. A repo that has explicitly opted into a threshold falls back to the SAME - // compare-API read #review-grounding already uses (fetchBaseAheadBy, anchored on the PR's real HEAD, never - // its live-tracking base.sha) and forces the identical update_branch action once the repo's current - // default branch has advanced at least that many commits beyond it. Costs one extra GitHub call per - // non-"behind" readiness check on an opted-in repo, which is exactly why this is opt-in rather than a new - // default (mirrors requireFreshRebaseWindowMinutes's own opt-in-for-cost rationale). - const repo = await getRepository(env, repoFullName); - const defaultBranchRef = repo?.defaultBranch; - const aheadBy = defaultBranchRef ? await fetchBaseAheadBy(env, repoFullName, headSha, defaultBranchRef, token, admissionKey) : undefined; - if (typeof aheadBy === "number" && aheadBy >= settings.staleBaseAheadByThreshold) { - const reason = `default branch is ${aheadBy} commits ahead of this PR's head (threshold ${settings.staleBaseAheadByThreshold}); update-branch before review`; - if (await forceUpdateBranch(reason)) { - return false; // the rebase fires a synchronize → fresh review runs on the new head - } - } } + // 1b) MOVED to the merge boundary (#9497). The staleness rebase used to fire here, in the readiness gate -- + // before the CI-pending wait immediately below, before the gate verdict, before the disposition plan, and + // before the merge-train check. Consequences, all confirmed in production: + // + // * `.github/workflows/ci.yml` uses `cancel-in-progress: true`, so pushing to a PR whose CI is RUNNING + // cancels that run and starts a fresh one -- the reported "re-runs CI during the middle of their runs". + // * The default branch only advances when a PR merges, so "N commits ahead" literally means "N unrelated + // PRs merged since your head". Every merge wakes up to MERGE_WAKE_MAX_PRS siblings, each of which ran + // this check -- so one unrelated merge rebased a whole cohort. Measured: 7 PRs rebased in 12 seconds, + // 44s after an unrelated merge; one cohort force-rebased 3x in 2.5 hours. + // * A rebase resets that cohort to zero-behind SIMULTANEOUSLY, so they stay phase-locked and re-cross the + // threshold together -- the bursts recur on the same PRs forever rather than settling. + // * Running before the plan meant a PR about to be auto-CLOSED, sitting on red CI, or held for manual + // review was rebased first, and that CI run was then thrown away. + // + // The correctly-gated sibling has always been maybeForceFreshRebase (see its call site in the maintenance + // pass): imminent-merge only, mergeableState clean only, and capped per PR per 24h keyed on PR NUMBER rather + // than head SHA. Folding the threshold in there inherits all three guards, so a PR is rebased only when it + // is the PR about to merge -- and it kills the self-feeding loop, because no speculative rebase means no + // stale surface, which means no repair-priority re-gate. // 2) wait for CI to finish before running the LoopOver review. Required contexts still define which failures // block/close, but hasPending tracks any visible non-bot CI that is not settled yet. const ci = await cachedLiveCiAggregate(env, { @@ -4833,9 +4850,12 @@ async function maybeForceFreshRebase( repoFullName: string; pr: PullRequestRecord; settings: RepositorySettings; - // Narrowed by the caller (typeof settings.requireFreshRebaseWindowMinutes === "number") -- re-deriving and - // re-checking the same nullable field here would just be an unreachable duplicate of that guard. - windowMinutes: number; + // #9497: either trigger may be configured; the caller guarantees at least one is. `windowMinutes` + // undefined means only the staleness threshold applies, and vice versa. + windowMinutes: number | null | undefined; + /** #9497: "the default branch is at least this many commits ahead of our head" -- moved here from the + * readiness gate so it inherits the imminent-merge, clean-mergeable and 3-per-PR-per-24h guards. */ + staleBaseAheadByThreshold: number | null | undefined; baseRef: string; token: string | undefined; admissionKey: GitHubRateLimitAdmissionKey | undefined; @@ -4845,15 +4865,35 @@ async function maybeForceFreshRebase( nowMs: number; }, ): Promise { - const { installationId, repoFullName, pr, settings, windowMinutes, baseRef, token, admissionKey, deliveryId, nowMs } = args; + const { installationId, repoFullName, pr, settings, windowMinutes, staleBaseAheadByThreshold, baseRef, token, admissionKey, deliveryId, nowMs } = args; /* v8 ignore next -- structurally unreachable: the caller only invokes this after confirming * (liveMergeState ?? pr.mergeableState) === "clean", which GitHub can never compute for a PR with no * head commit; the null check is belt-and-suspenders against the field's optional TS type. */ if (!pr.headSha) return false; - const advancedAt = await fetchLiveBaseBranchAdvancedAt(env, repoFullName, baseRef, token, admissionKey); - if (!advancedAt) return false; // fail-open: unreadable base commit -> no forced rebase - const advancedAtMs = Date.parse(advancedAt); - if (!isWithinFreshRebaseWindow({ baseAdvancedAtMs: advancedAtMs, windowMinutes, nowMs })) return false; + // Trigger 1 (#requireFreshRebaseWindowMinutes): the base advanced very recently, so the merge would be + // computed against a base older than the one it will land on. + let triggerReason: string | null = null; + if (typeof windowMinutes === "number") { + const advancedAt = await fetchLiveBaseBranchAdvancedAt(env, repoFullName, baseRef, token, admissionKey); + // fail-open: unreadable base commit -> this trigger simply does not fire + if (advancedAt && isWithinFreshRebaseWindow({ baseAdvancedAtMs: Date.parse(advancedAt), windowMinutes, nowMs })) { + triggerReason = `base advanced within the last ${windowMinutes} minute(s); update-branch before merge`; + } + } + // Trigger 2 (#9497, moved from the readiness gate): the default branch has run ahead by at least the + // configured threshold. mergeable_state only reports "behind" when branch protection requires up-to-date + // branches, so a repo without that setting can be dozens of commits behind and GitHub never surfaces it -- + // this is the compare-API fallback for those repos. It now runs ONLY at the merge boundary, so a PR is + // rebased when it is the PR about to merge rather than whenever an unrelated PR happened to merge. + if (triggerReason === null && typeof staleBaseAheadByThreshold === "number" && pr.headSha) { + const repo = await getRepository(env, repoFullName); + const defaultBranchRef = repo?.defaultBranch; + const aheadBy = defaultBranchRef ? await fetchBaseAheadBy(env, repoFullName, pr.headSha, defaultBranchRef, token, admissionKey) : undefined; + if (typeof aheadBy === "number" && aheadBy >= staleBaseAheadByThreshold) { + triggerReason = `default branch is ${aheadBy} commits ahead of this PR's head (threshold ${staleBaseAheadByThreshold}); update-branch before merge`; + } + } + if (triggerReason === null) return false; const countKey = freshRebaseForceCountKey(repoFullName, pr.number); const storedCount = Number(await getTransientKey(env, countKey)); @@ -4864,8 +4904,8 @@ async function maybeForceFreshRebase( actor: "loopover", targetKey: `${repoFullName}#${pr.number}`, outcome: "completed", - detail: `base advanced within the ${windowMinutes}m freshness window, but the ${MAX_FRESH_REBASE_FORCES}-attempt forced-rebase cap was already reached for this PR — falling through to a normal merge decision`, - metadata: { deliveryId, repoFullName, headSha: pr.headSha, windowMinutes }, + detail: `${triggerReason}, but the ${MAX_FRESH_REBASE_FORCES}-attempt forced-rebase cap was already reached for this PR — falling through to a normal merge decision`, + metadata: { deliveryId, repoFullName, headSha: pr.headSha, windowMinutes: windowMinutes ?? null, staleBaseAheadByThreshold: staleBaseAheadByThreshold ?? null }, }).catch( /* v8 ignore next -- fail-safe: an audit write failure never blocks the caller's fallthrough */ () => undefined, @@ -4909,8 +4949,8 @@ async function maybeForceFreshRebase( actor: "loopover", targetKey: `${repoFullName}#${pr.number}`, outcome: "completed", - detail: `forced update_branch (attempt ${nextAttempt}/${MAX_FRESH_REBASE_FORCES}) — base advanced within the ${windowMinutes}m freshness window`, - metadata: { deliveryId, repoFullName, headSha: pr.headSha, windowMinutes, attempt: nextAttempt }, + detail: `forced update_branch (attempt ${nextAttempt}/${MAX_FRESH_REBASE_FORCES}) — ${triggerReason}`, + metadata: { deliveryId, repoFullName, headSha: pr.headSha, windowMinutes: windowMinutes ?? null, staleBaseAheadByThreshold: staleBaseAheadByThreshold ?? null, attempt: nextAttempt }, }).catch( /* v8 ignore next -- fail-safe: an audit write failure never blocks the caller */ () => undefined, @@ -5110,6 +5150,10 @@ async function wakeOverCapSiblingPullRequests( repoFullName, prNumber, installationId, + // #9499: the sibling row is already fetched just above for the cooldown key, so threading its + // createdAt costs nothing. Without it, jobClaimSortKey falls back to a legacy base that sorts this + // wake ahead of every genuinely older contributor PR. + ...(sibling?.createdAt ? { prCreatedAt: sibling.createdAt } : {}), }); } catch (error) { console.log( diff --git a/src/review/pr-reconciliation.ts b/src/review/pr-reconciliation.ts index f5746935e8..8c8d043cca 100644 --- a/src/review/pr-reconciliation.ts +++ b/src/review/pr-reconciliation.ts @@ -129,7 +129,10 @@ async function catchUpMissingPullRequest(env: Env, repoFullName: string, install return; } await upsertPullRequestFromGitHub(env, repoFullName, live); - const message: JobMessage = { type: "agent-regate-pr", deliveryId: `reconcile:${repoFullName}#${prNumber}`, repoFullName, prNumber, installationId }; + // #9499: prCreatedAt is what jobClaimSortKey uses to drain contributor work oldest-first. Omitting it + // falls back to LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber (~9.5e11), which sorts AHEAD of every real + // 2026 PR (~1.78e12) -- so a reconciliation job silently jumped the whole queue. + const message: JobMessage = { type: "agent-regate-pr", deliveryId: `reconcile:${repoFullName}#${prNumber}`, repoFullName, prNumber, installationId, ...(live.created_at ? { prCreatedAt: live.created_at } : {}) }; await env.JOBS.send(message); } catch (error) { console.error(JSON.stringify({ level: "error", event: "open_pr_reconciliation_catch_up_failed", repository: repoFullName, prNumber, message: errorMessage(error).slice(0, 200) })); diff --git a/src/review/surface-disposition-reconciler.ts b/src/review/surface-disposition-reconciler.ts index f9fd3c35bb..40fbed8231 100644 --- a/src/review/surface-disposition-reconciler.ts +++ b/src/review/surface-disposition-reconciler.ts @@ -48,10 +48,10 @@ export type SurfaceDispositionReconcileResult = { scanned: number; requeued: num */ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number = Date.now()): Promise { const since = new Date(nowMs - SURFACE_DISPOSITION_RECONCILE_LOOKBACK_MS).toISOString(); - let rows: Array<{ repoFullName: string; number: number; installationId: number; headSha: string }> = []; + let rows: Array<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string | null }> = []; try { const result = await env.DB.prepare( - `SELECT pr.repo_full_name AS repoFullName, pr.number AS number, repo.installation_id AS installationId, pr.head_sha AS headSha + `SELECT pr.repo_full_name AS repoFullName, pr.number AS number, repo.installation_id AS installationId, pr.head_sha AS headSha, pr.created_at AS createdAt FROM pull_requests AS pr JOIN repositories AS repo ON repo.full_name = pr.repo_full_name WHERE pr.state = 'open' @@ -68,7 +68,7 @@ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number LIMIT ?3`, ) .bind(since, DISPOSITION_CONSIDERED_EVENT_TYPE, SURFACE_DISPOSITION_RECONCILE_LIMIT) - .all<{ repoFullName: string; number: number; installationId: number; headSha: string }>(); + .all<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string | null }>(); rows = result.results ?? []; } catch (error) { console.warn(JSON.stringify({ level: "warn", event: "surface_disposition_reconcile_scan_failed", message: errorMessage(error).slice(0, 160) })); @@ -83,6 +83,9 @@ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number repoFullName: row.repoFullName, prNumber: row.number, installationId: row.installationId, + // #9499: without prCreatedAt the claim sort key falls back to a legacy base that sorts ahead of every + // real PR, so this repair scan silently preempted genuinely older contributor work. + ...(row.createdAt ? { prCreatedAt: row.createdAt } : {}), }) .then(() => true) .catch(() => false); diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 1b237826e9..4c393ac776 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -17,7 +17,7 @@ import { } from "../db/repositories"; import { isPagerDutyEnabled, triggerPagerDutyIncident } from "./notify-pagerduty"; import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; -import { classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeConflictMessage, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "./merge-failure"; +import { classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeConflictMessage, isNoNewBaseCommitsMessage, isWorkflowScopeRefusalMessage, 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"; @@ -793,6 +793,12 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // (see forceUpdateBranch's own doc comment), exactly like every other "couldn't rebase, review anyway" // path. The branch owner, not the bot, needs to resolve the conflict -- paging on every naturally- // diverged PR this happens to hit isn't warranted. Still recorded by the audit() call above. + } else if (action.actionClass === "update_branch" && isWorkflowScopeRefusalMessage(errorMessage(error))) { + // #9498: PERMANENT for this diff shape -- the App may not write .github/workflows/**, and update_branch + // merges the base in, so any workflow change on the default branch since the fork makes the merge a + // workflow write even when the PR touches none. Retrying cannot change the outcome (one PR was retried + // nine times), so this is classified rather than paged, and the caller falls through to reviewing the + // current head -- being un-rebasable is not a reason to stop reviewing. } else if (action.actionClass === "update_branch" && isNoNewBaseCommitsMessage(errorMessage(error))) { // LOOPOVER-24 (regressed shape): a 422 "There are no new commits on the base branch." means the head // was already up to date when update-branch fired -- the readiness check acted on a stale/cached diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index ecb9703cc0..6003b37f1e 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -39,6 +39,25 @@ export function isNoNewBaseCommitsMessage(message: string): boolean { return /no new commits on the base branch/i.test(message); } +/** + * #9498: True for GitHub's refusal to let a GitHub App write `.github/workflows/**` + * ("refusing to allow a GitHub App to create or update workflow ..."). + * + * PERMANENT for a given diff shape, not transient -- retrying cannot change the outcome. It fires whenever the + * resulting push would touch a workflow file, which crucially includes the case where the PR itself touches + * NONE: `update_branch` merges the base into the head, so any workflow change on the default branch since the + * PR forked makes the merge a workflow write. Measured over one 7-day window, 48 of 82 update_branch failures + * were this class across 14 PRs -- 4 of the 5 worst offenders did not touch a workflow file themselves -- and + * one PR was retried NINE times against an outcome that could never succeed. + * + * Treated like the other benign/terminal update_branch shapes: audit-only, no page. The caller already falls + * through to reviewing the PR on its current head, which is the correct behaviour -- being un-rebasable is not + * a reason to stop reviewing. + */ +export function isWorkflowScopeRefusalMessage(message: string): boolean { + return /refusing to allow (?:a |an )?(?:github app|oauth app|integration)[^.]*workflow/i.test(message); +} + /** True for the transient "Base branch was modified. Review and try the merge again." 405 — a benign * TOCTOU race (the base advanced between plan and merge) that a re-attempt against the new base resolves. */ function isBaseBranchMovedMessage(message: string): boolean { diff --git a/test/unit/merge-failure.test.ts b/test/unit/merge-failure.test.ts index 64d136dcc9..62bbf600dd 100644 --- a/test/unit/merge-failure.test.ts +++ b/test/unit/merge-failure.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { classifyMergeFailure, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; +import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, isWorkflowScopeRefusalMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; /** Build an Octokit-style RequestError: an Error carrying an HTTP `.status`. */ function httpError(status: number, message: string): Error { @@ -68,3 +68,36 @@ describe("isNoNewBaseCommitsMessage", () => { expect(isNoNewBaseCommitsMessage("network timeout")).toBe(false); }); }); + +// #9498: 48 of 82 update_branch failures in one 7-day window were this class, across 14 PRs, with one PR +// retried NINE times against an outcome that can never succeed. Crucially it is NOT limited to PRs that touch +// workflow files: update_branch merges the base INTO the head, so any workflow change on the default branch +// since the PR forked makes the resulting merge a workflow write -- 4 of the 5 worst offenders touched none. +describe("isWorkflowScopeRefusalMessage (#9498)", () => { + it.each([ + ["refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yml` without `workflows` permission"], + ["Refusing to allow a GitHub App to create or update workflow file"], + ["refusing to allow an integration to create or update workflow .github/workflows/release.yml"], + ["refusing to allow an OAuth App to create or update workflow"], + ])("recognises %s", (message) => { + expect(isWorkflowScopeRefusalMessage(message)).toBe(true); + }); + + it.each([ + ["merge conflict between base and head"], + ["There are no new commits on the base branch."], + ["Base branch was modified. Review and try the merge again."], + ["Resource not accessible by integration"], + [""], + ])("does NOT misclassify %s", (message) => { + // Deliberately narrow: a generic permission error must keep its existing handling, and the sibling + // update_branch shapes must keep theirs. + expect(isWorkflowScopeRefusalMessage(message)).toBe(false); + }); + + it("INVARIANT: does not overlap the other update_branch classifiers", () => { + const workflowRefusal = "refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yml`"; + expect(isMergeConflictMessage(workflowRefusal)).toBe(false); + expect(isNoNewBaseCommitsMessage(workflowRefusal)).toBe(false); + }); +}); diff --git a/test/unit/queue-2.test.ts b/test/unit/queue-2.test.ts index 6ac81fced3..73a4e7d1a9 100644 --- a/test/unit/queue-2.test.ts +++ b/test/unit/queue-2.test.ts @@ -2226,12 +2226,12 @@ describe("queue processors", () => { await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto" } }); await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required" } }); // PR 1: missing its current Gate check for its current head -- would ordinarily be flagged outage-repair - // priority on every tick. Pre-seed REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA=5 (#3998) prior repair-attempt audit - // events for this EXACT head SHA to simulate a review that keeps failing (e.g. a timeout) and never + // priority on every tick. Pre-seed REGATE_REPAIR_MAX_ATTEMPTS_PER_PR=5 (#3998) prior repair-attempt audit + // events for this PR to simulate a review that keeps failing (e.g. a timeout) and never // publishes a completed gate check. await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 1, title: "Stuck repair", state: "open", user: { login: "c" }, head: { sha: "stuck-sha" }, labels: [], body: "" }); await repositoriesModule.markPullRequestSurfacePublished(env, "owner/agent-repo", 1, "stuck-sha"); - const targetKey = "owner/agent-repo#1#stuck-sha"; + const targetKey = "owner/agent-repo#1"; // #9499: the repair budget is per PR, not per head SHA for (let i = 0; i < 5; i += 1) { await repositoriesModule.recordAuditEvent(env, { eventType: "agent.sweep.regate.repair_attempt", @@ -2280,7 +2280,7 @@ describe("queue processors", () => { await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required" } }); await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 2, title: "Fresh repair", state: "open", user: { login: "c" }, head: { sha: "fresh-sha" }, labels: [], body: "" }); await repositoriesModule.markPullRequestSurfacePublished(env, "owner/agent-repo", 2, "fresh-sha"); - const targetKey = "owner/agent-repo#2#fresh-sha"; + const targetKey = "owner/agent-repo#2"; vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); await processJob(env, { type: "agent-regate-sweep", requestedBy: "test", repoFullName: "owner/agent-repo" }); @@ -2296,7 +2296,7 @@ describe("queue processors", () => { .first<{ n: number }>(); expect(attemptsAfterFirst?.n).toBe(1); - // Manually push this SHA over the REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA=5 cap (#3998), then run the sweep twice + // Manually push this SHA over the REGATE_REPAIR_MAX_ATTEMPTS_PER_PR=5 cap (#3998), then run the sweep twice // more -- the exhausted event must be recorded only once even though the PR is (re-)evaluated on every tick. for (let i = 0; i < 4; i += 1) { await repositoriesModule.recordAuditEvent(env, { @@ -2324,7 +2324,7 @@ describe("queue processors", () => { await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto" }, gatePack: "oss-anti-slop" }); await upsertRepoFocusManifest(env, "owner/agent-repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required" } }); await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 3, title: "Healthy PR, still waiting on required CI", state: "open", user: { login: "contributor" }, head: { sha: "pending-sha" }, base: { ref: "main" }, labels: [], body: "" }); - const targetKey = "owner/agent-repo#3#pending-sha"; + const targetKey = "owner/agent-repo#3"; vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); // Same fixture shape as the missing-required-context tests in queue.test.ts: a required status check has // simply not posted yet. Time is frozen for this whole test (vi.setSystemTime, never advanced) and no @@ -2382,7 +2382,7 @@ describe("queue processors", () => { await upsertRepoFocusManifest(env, "owner/agent-repo4", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", reviewCheckMode: "required" } }); // Never marked surface-published for this head -- needsSurfaceConvergence is true, a genuine backlog-convergence candidate. await upsertPullRequestFromGitHub(env, "owner/agent-repo4", { number: 1, title: "Stuck convergence", state: "open", user: { login: "c" }, head: { sha: "stuck-sha" }, labels: [], body: "" }); - const targetKey = "owner/agent-repo4#1#stuck-sha"; + const targetKey = "owner/agent-repo4#1"; // Pre-seed the SAME shared budget the main sweep's repair path charges against -- this is the whole point of // sharing isRegateRepairExhausted rather than giving backlog-convergence its own independent counter. for (let i = 0; i < 5; i += 1) { @@ -2448,7 +2448,7 @@ describe("queue processors", () => { created_at: wedgedCreated[i]!, updated_at: wedgedCreated[i]!, }); - const targetKey = `owner/agent-repo6#${number}#${headSha}`; + const targetKey = `owner/agent-repo6#${number}`; // #9499: per-PR, not per-head-SHA for (let attempt = 0; attempt < 5; attempt += 1) { await repositoriesModule.recordAuditEvent(env, { eventType: "agent.sweep.regate.repair_attempt", @@ -2512,7 +2512,7 @@ describe("queue processors", () => { labels: [], body: "", }); - const targetKey = `owner/agent-repo7#1#${headSha}`; + const targetKey = `owner/agent-repo7#1`; // #9499: per-PR, not per-head-SHA for (let attempt = 0; attempt < 5; attempt += 1) { await repositoriesModule.recordAuditEvent(env, { eventType: "agent.sweep.regate.repair_attempt", @@ -2553,7 +2553,7 @@ describe("queue processors", () => { await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo5", autonomy: { merge: "auto" } }); await upsertRepoFocusManifest(env, "owner/agent-repo5", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off" } }); await upsertPullRequestFromGitHub(env, "owner/agent-repo5", { number: 2, title: "Fresh convergence", state: "open", user: { login: "c" }, head: { sha: "fresh-sha" }, base: { ref: "main" }, labels: [], body: "" }); - const targetKey = "owner/agent-repo5#2#fresh-sha"; + const targetKey = "owner/agent-repo5#2"; vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); @@ -2595,7 +2595,7 @@ describe("queue processors", () => { await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo3", autonomy: { merge: "auto" } }); await upsertRepoFocusManifest(env, "owner/agent-repo3", { settings: { checkRunMode: "off", commentMode: "all_prs", publicSurface: "comment_only", aiReviewMode: "off" } }); await upsertPullRequestFromGitHub(env, "owner/agent-repo3", { number: 5, title: "Healthy PR ready to review", state: "open", user: { login: "contributor" }, head: { sha: "ready-sha" }, base: { ref: "main" }, labels: [], body: "" }); - const targetKey = "owner/agent-repo3#5#ready-sha"; + const targetKey = "owner/agent-repo3#5"; let finalCommentAttempted = false; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -2674,7 +2674,7 @@ describe("queue processors", () => { await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo4", autonomy: { merge: "auto" } }); await upsertRepoFocusManifest(env, "owner/agent-repo4", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off" } }); await upsertPullRequestFromGitHub(env, "owner/agent-repo4", { number: 6, title: "Healthy PR ready to review", state: "open", user: { login: "contributor" }, head: { sha: "ready-sha-2" }, base: { ref: "main" }, labels: [], body: "" }); - const targetKey = "owner/agent-repo4#6#ready-sha-2"; + const targetKey = "owner/agent-repo4#6"; const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); const realPrepare = env.DB.prepare.bind(env.DB); // Same poison as "agent re-gate sweep ... swallows a failing re-review" above: only the advisories INSERT diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 4bdb3606e6..d8954b0a1d 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2989,7 +2989,7 @@ describe("queue processors", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — mirrors the #2550 // migration-recheck fixture above. `baseAdvancedAt` stubs the NEW /commits/{baseRef} freshness read; // `null` simulates an unreadable base commit (404). - function stubFreshRebaseFetch(prNumber: number, opts: { baseAdvancedAt: string | null; mergeableState?: string; headSha?: string }, seen: { merged: boolean; updateBranchCalls: number; baseCommitCalls: number }) { + function stubFreshRebaseFetch(prNumber: number, opts: { baseAdvancedAt: string | null; mergeableState?: string; headSha?: string; aheadBy?: number }, seen: { merged: boolean; updateBranchCalls: number; baseCommitCalls: number; compareCalls?: number }) { const headSha = opts.headSha ?? "sha1"; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { const url = input.toString(); @@ -3001,6 +3001,10 @@ describe("queue processors", () => { seen.updateBranchCalls += 1; return Response.json({ message: "Updating pull request branch." }, { status: 202 }); } + if (url.includes("/compare/")) { + seen.compareCalls = (seen.compareCalls ?? 0) + 1; + return Response.json({ ahead_by: opts.aheadBy ?? 0, behind_by: 0 }); + } if (url.endsWith("/commits/main")) { seen.baseCommitCalls += 1; if (opts.baseAdvancedAt === null) return new Response("not found", { status: 404 }); @@ -3028,16 +3032,18 @@ describe("queue processors", () => { }); } - async function seedFreshRebaseRepo(env: Env, prNumber: number, opts: { requireFreshRebaseWindowMinutes?: number | null; autonomy?: Record } = {}) { + async function seedFreshRebaseRepo(env: Env, prNumber: number, opts: { requireFreshRebaseWindowMinutes?: number | null; staleBaseAheadByThreshold?: number | null; autonomy?: Record } = {}) { await upsertInstallation(env, { installation: { id: 123, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { contents: "write", pull_requests: "write", issues: "write" }, events: [] }, }); - await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" } }, 123); + // default_branch is what the #9497 staleness compare anchors on (getRepository(...).defaultBranch). + await upsertRepositoryFromGitHub(env, { name: "repo", full_name: "owner/repo", private: false, owner: { login: "owner" }, default_branch: "main" }, 123); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: opts.autonomy ?? { merge: "auto", update_branch: "auto", label: "auto" }, gatePack: "oss-anti-slop", ...(opts.requireFreshRebaseWindowMinutes !== undefined ? { requireFreshRebaseWindowMinutes: opts.requireFreshRebaseWindowMinutes } : {}), + ...(opts.staleBaseAheadByThreshold !== undefined ? { staleBaseAheadByThreshold: opts.staleBaseAheadByThreshold } : {}), }); await upsertRepoFocusManifest(env, "owner/repo", { settings: { checkRunMode: "off", commentMode: "off", publicSurface: "off", aiReviewMode: "off", reviewCheckMode: "required", autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } } }); await upsertPullRequestFromGitHub(env, "owner/repo", { number: prNumber, title: "Fresh rebase PR", state: "open", user: { login: "contributor" }, head: { sha: "sha1" }, base: { ref: "main" }, labels: [], body: "" }); @@ -3058,6 +3064,48 @@ describe("queue processors", () => { expect(merge?.outcome).toBe("completed"); }); + // #9497: the staleness threshold MOVED here from the readiness gate, so it now inherits the three guards + // that path had none of -- imminent merge, clean mergeable state, and the 3-per-PR-per-24h cap keyed on PR + // number. In the readiness gate it fired on any review pass, so an unrelated merge advancing the default + // branch rebased a whole woken cohort mid-CI; here it fires only for the PR actually about to merge. + it("#9497 forces update_branch at the MERGE boundary when the default branch is far enough ahead", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedFreshRebaseRepo(env, 95, { staleBaseAheadByThreshold: 5 }); + const seen = { merged: false, updateBranchCalls: 0, baseCommitCalls: 0, compareCalls: 0 }; + stubFreshRebaseFetch(95, { baseAdvancedAt: new Date(Date.now() - 60 * 60_000).toISOString(), aheadBy: 10 }, seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "stale-threshold-merge-boundary", repoFullName: "owner/repo", prNumber: 95, installationId: 123 }); + + expect(seen.compareCalls).toBe(1); + expect(seen.updateBranchCalls).toBe(1); // 10 >= threshold 5 + expect(seen.merged).toBe(false); // the rebase defers the merge; the synchronize re-triggers evaluation + }); + + it("#9497 does NOT rebase when the default branch is ahead but below the threshold", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedFreshRebaseRepo(env, 96, { staleBaseAheadByThreshold: 5 }); + const seen = { merged: false, updateBranchCalls: 0, baseCommitCalls: 0, compareCalls: 0 }; + stubFreshRebaseFetch(96, { baseAdvancedAt: new Date(Date.now() - 60 * 60_000).toISOString(), aheadBy: 2 }, seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "stale-threshold-below", repoFullName: "owner/repo", prNumber: 96, installationId: 123 }); + + expect(seen.updateBranchCalls).toBe(0); + expect(seen.merged).toBe(true); + }); + + it("#9497 INVARIANT: no compare call at all when no threshold is configured (zero added cost)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedFreshRebaseRepo(env, 97, {}); + const seen = { merged: false, updateBranchCalls: 0, baseCommitCalls: 0, compareCalls: 0 }; + stubFreshRebaseFetch(97, { baseAdvancedAt: new Date(Date.now() - 60 * 60_000).toISOString(), aheadBy: 999 }, seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "stale-threshold-unset", repoFullName: "owner/repo", prNumber: 97, installationId: 123 }); + + expect(seen.compareCalls).toBe(0); + expect(seen.updateBranchCalls).toBe(0); + expect(seen.merged).toBe(true); + }); + it("forces update_branch instead of merging when the base advanced within the freshness window", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedFreshRebaseRepo(env, 91, { requireFreshRebaseWindowMinutes: 10 }); diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index c7b8b66b9a..8f8f137b3b 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -368,7 +368,14 @@ describe("queue processors", () => { }; } - it("auto-maintain (stale-base threshold): a NOT-'behind' PR still forces update-branch via the compare-API fallback once ahead_by meets the configured threshold", async () => { + // #9497 REGRESSION: this used to assert the rebase fires HERE, during the readiness gate -- before the + // CI-pending wait, the gate verdict, the disposition plan and the merge-train check. That is the defect: the + // default branch only advances when a PR merges, so "N commits ahead" means "N unrelated PRs merged since + // your head", and every merge woke a cohort of siblings that each ran this check. Measured in production: 7 + // PRs rebased in 12 seconds, 44s after an unrelated merge, with CI cancel-in-progress killing their running + // jobs. It now fires only at the merge boundary (see the #9497 tests in queue-3), so a plain review pass on + // a far-behind PR must leave the head alone. + it("#9497 does NOT force update-branch during the review pass, even far past the threshold", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedBehindRepo(env, { defaultBranch: "main", staleBaseAheadByThreshold: 5 }); let updateBranchCalls = 0; @@ -391,12 +398,12 @@ describe("queue processors", () => { await processJob(env, staleBaseWebhook()); - expect(compareCalls).toBe(1); - expect(updateBranchCalls).toBe(1); // 10 >= the configured threshold of 5 - const ub = await env.DB.prepare("select outcome from audit_events where event_type = ?").bind("agent.action.update_branch").first<{ outcome: string }>(); - expect(ub?.outcome).toBe("completed"); - const merge = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("agent.action.merge").first<{ n: number }>(); - expect(merge?.n).toBe(0); // deferred for the rebase → no gate verdict published on the stale head + // No speculative rebase, and no compare call to pay for one: the review pass does not consider staleness + // at all any more. A PR 10 commits behind is reviewed on its own head and left alone. + expect(updateBranchCalls).toBe(0); + expect(compareCalls).toBe(0); + const ub = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?").bind("agent.action.update_branch").first<{ n: number }>(); + expect(ub?.n).toBe(0); }); it("auto-maintain (stale-base threshold): does not force update-branch when ahead_by is below the configured threshold", async () => { @@ -425,8 +432,10 @@ describe("queue processors", () => { await processJob(env, staleBaseWebhook()); - expect(compareCalls).toBe(1); // the threshold IS configured, so the fallback check still runs - expect(updateBranchCalls).toBe(0); // 3 < the configured threshold of 5 → no forced rebase + // #9497: the readiness gate no longer consults staleness at all, so neither the compare nor a rebase runs + // here regardless of how far behind the PR is. The threshold is evaluated at the merge boundary instead. + expect(compareCalls).toBe(0); + expect(updateBranchCalls).toBe(0); }); it("auto-maintain (stale-base threshold): never attempts the compare-API fallback when no threshold is configured (zero added cost, byte-identical to before this feature existed)", async () => { @@ -509,8 +518,11 @@ describe("queue processors", () => { await processJob(env, staleBaseWebhook()); - expect(compareCalls).toBe(1); // the threshold check still ran and found the branch stale (10 >= 5) - expect(updateBranchCalls).toBe(0); // update_branch autonomy not granted → the executor denies the write; falls through + // #9497: the readiness gate no longer evaluates staleness at all, so there is nothing to deny here -- + // neither the compare nor the write happens during a review pass. Autonomy still gates the write at the + // merge boundary, where the check now lives. + expect(compareCalls).toBe(0); + expect(updateBranchCalls).toBe(0); }); it("recapture-preview (#1158): a clean PR re-review threads previewPollAttempt into the public-surface publish", async () => { diff --git a/test/unit/surface-disposition-reconciler.test.ts b/test/unit/surface-disposition-reconciler.test.ts index 0683e20cd7..09ace057b7 100644 --- a/test/unit/surface-disposition-reconciler.test.ts +++ b/test/unit/surface-disposition-reconciler.test.ts @@ -100,7 +100,12 @@ describe("reconcileSurfaceWithoutDisposition (#8997)", () => { await seedPr(env, 1, "sha1", { lastPublishedSurfaceSha: "sha1" }); expect(await reconcileSurfaceWithoutDisposition(env)).toEqual({ scanned: 1, requeued: 1 }); - expect(sent).toEqual([{ type: "agent-regate-pr", deliveryId: "surface-without-disposition:alice/repo#1#sha1", repoFullName: "alice/repo", prNumber: 1, installationId: 77 }]); + // #9499: prCreatedAt must ride along, or jobClaimSortKey falls back to a legacy base that sorts this + // repair job ahead of every genuinely older contributor PR. + expect(sent).toEqual([ + expect.objectContaining({ type: "agent-regate-pr", deliveryId: "surface-without-disposition:alice/repo#1#sha1", repoFullName: "alice/repo", prNumber: 1, installationId: 77 }), + ]); + expect((sent[0] as { prCreatedAt?: string }).prCreatedAt).toBeTruthy(); }); it("does NOT re-enqueue once a disposition marker is on record for that exact head", async () => { diff --git a/test/unit/surface-repair-priority.test.ts b/test/unit/surface-repair-priority.test.ts index d385736966..9c43d265b6 100644 --- a/test/unit/surface-repair-priority.test.ts +++ b/test/unit/surface-repair-priority.test.ts @@ -7,7 +7,7 @@ import type { PullRequestRecord } from "../../src/types"; const REPO = "owner/repo"; const REGATE_REPAIR_ATTEMPT_EVENT_TYPE = "agent.sweep.regate.repair_attempt"; -const REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA = 5; +const REGATE_REPAIR_MAX_ATTEMPTS_PER_PR = 5; function pr(overrides: Partial & { number: number }): PullRequestRecord { return { @@ -137,14 +137,15 @@ describe("surfaceRepairPriorityPullNumbers (#orb-stale-recheck-priority)", () => detail: "the base-branch conflict that justified this close has since cleared — action not executed", createdAt: new Date().toISOString(), }); - // The SAME per-(repo, pr, headSha) attempt budget the outage-repair path already shares (isRegateRepairExhausted) - // applies here too -- once it's exhausted for this exact head SHA, the PR drops out of the priority set even - // though the stale-recheck-denial signal above still matches. - for (let i = 0; i < REGATE_REPAIR_MAX_ATTEMPTS_PER_SHA; i++) { + // The SAME per-(repo, pr) attempt budget the outage-repair path already shares (isRegateRepairExhausted) + // applies here too -- once exhausted for this PR, it drops out of the priority set even though the + // stale-recheck-denial signal above still matches. #9499 re-keyed this from head SHA to PR number: a + // successful rebase mints a new SHA, which reset the budget and let the repair loop run forever. + for (let i = 0; i < REGATE_REPAIR_MAX_ATTEMPTS_PER_PR; i++) { await recordAuditEvent(env, { eventType: REGATE_REPAIR_ATTEMPT_EVENT_TYPE, actor: "loopover", - targetKey: `${REPO}#9#sha1`, + targetKey: `${REPO}#9`, outcome: "completed", detail: "outage-repair re-review executing", createdAt: new Date().toISOString(), From 1fa55533d47b5b7a238a43973597a4cfe0517c97 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:11:30 -0700 Subject: [PATCH 2/4] test(review): cover the moved rebase triggers, the workflow-scope refusal, and simplify two unreachable arms (#9497, #9498, #9499) prCreatedAt is now passed unconditionally where its source is NOT NULL (pull_requests.created_at, and GitHub's PR payload) -- the guarded spread had an absent arm nothing could reach. Adds the no-stored-default-branch skip, the cap-exceeded audit naming whichever trigger fired, and an executor-level regression for the workflow-scope refusal being audited but never paged. --- src/review/pr-reconciliation.ts | 2 +- src/review/surface-disposition-reconciler.ts | 9 +++--- test/unit/agent-action-executor.test.ts | 20 ++++++++++++ test/unit/queue-3.test.ts | 34 ++++++++++++++++++++ 4 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/review/pr-reconciliation.ts b/src/review/pr-reconciliation.ts index 8c8d043cca..a3c3eec9cc 100644 --- a/src/review/pr-reconciliation.ts +++ b/src/review/pr-reconciliation.ts @@ -132,7 +132,7 @@ async function catchUpMissingPullRequest(env: Env, repoFullName: string, install // #9499: prCreatedAt is what jobClaimSortKey uses to drain contributor work oldest-first. Omitting it // falls back to LEGACY_AGENT_REGATE_SORT_BASE_MS + prNumber (~9.5e11), which sorts AHEAD of every real // 2026 PR (~1.78e12) -- so a reconciliation job silently jumped the whole queue. - const message: JobMessage = { type: "agent-regate-pr", deliveryId: `reconcile:${repoFullName}#${prNumber}`, repoFullName, prNumber, installationId, ...(live.created_at ? { prCreatedAt: live.created_at } : {}) }; + const message: JobMessage = { type: "agent-regate-pr", deliveryId: `reconcile:${repoFullName}#${prNumber}`, repoFullName, prNumber, installationId, prCreatedAt: live.created_at }; await env.JOBS.send(message); } catch (error) { console.error(JSON.stringify({ level: "error", event: "open_pr_reconciliation_catch_up_failed", repository: repoFullName, prNumber, message: errorMessage(error).slice(0, 200) })); diff --git a/src/review/surface-disposition-reconciler.ts b/src/review/surface-disposition-reconciler.ts index 40fbed8231..b82ea15a7d 100644 --- a/src/review/surface-disposition-reconciler.ts +++ b/src/review/surface-disposition-reconciler.ts @@ -48,7 +48,7 @@ export type SurfaceDispositionReconcileResult = { scanned: number; requeued: num */ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number = Date.now()): Promise { const since = new Date(nowMs - SURFACE_DISPOSITION_RECONCILE_LOOKBACK_MS).toISOString(); - let rows: Array<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string | null }> = []; + let rows: Array<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string }> = []; try { const result = await env.DB.prepare( `SELECT pr.repo_full_name AS repoFullName, pr.number AS number, repo.installation_id AS installationId, pr.head_sha AS headSha, pr.created_at AS createdAt @@ -68,7 +68,7 @@ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number LIMIT ?3`, ) .bind(since, DISPOSITION_CONSIDERED_EVENT_TYPE, SURFACE_DISPOSITION_RECONCILE_LIMIT) - .all<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string | null }>(); + .all<{ repoFullName: string; number: number; installationId: number; headSha: string; createdAt: string }>(); rows = result.results ?? []; } catch (error) { console.warn(JSON.stringify({ level: "warn", event: "surface_disposition_reconcile_scan_failed", message: errorMessage(error).slice(0, 160) })); @@ -84,8 +84,9 @@ export async function reconcileSurfaceWithoutDisposition(env: Env, nowMs: number prNumber: row.number, installationId: row.installationId, // #9499: without prCreatedAt the claim sort key falls back to a legacy base that sorts ahead of every - // real PR, so this repair scan silently preempted genuinely older contributor work. - ...(row.createdAt ? { prCreatedAt: row.createdAt } : {}), + // real PR, so this repair scan silently preempted genuinely older contributor work. Unconditional -- + // pull_requests.created_at is NOT NULL, so there is no absent case to guard. + prCreatedAt: row.createdAt, }) .then(() => true) .catch(() => false); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 061df67478..5cc3250402 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -1667,6 +1667,26 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { captureSpy.mockRestore(); }); + // #9498: 48 of 82 update_branch failures in one 7-day production window were this class, across 14 PRs, with + // one PR retried NINE times against an outcome that can never succeed. It is PERMANENT for a diff shape: + // update_branch merges the base INTO the head, so any workflow change on the default branch since the PR + // forked makes the resulting merge a workflow write -- 4 of the 5 worst offenders touched no workflow file + // themselves. Classified like the other benign/terminal update_branch shapes: audited, never paged. + it("REGRESSION (#9498): a workflow-scope refusal on update_branch does not page Sentry", async () => { + const env = createTestEnv({}); + vi.mocked(updatePullRequestBranch).mockRejectedValueOnce( + Object.assign(new Error("refusing to allow a GitHub App to create or update workflow `.github/workflows/ci.yml` without `workflows` permission"), { status: 422 }), + ); + const captureSpy = vi.spyOn(posthogModule, "capturePostHogError"); + + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [updateBranch]); + + expect(outcomes[0]).toMatchObject({ actionClass: "update_branch", outcome: "error" }); + expect((await auditFor(env, "update_branch"))?.outcome).toBe("error"); // still recorded, so it is diagnosable + expect(captureSpy).not.toHaveBeenCalled(); // ...but never paged, because retrying cannot change it + captureSpy.mockRestore(); + }); + it("a non-conflict update_branch failure still pages Sentry (#agent_action_execution_failed unchanged)", async () => { const env = createTestEnv({}); vi.mocked(updatePullRequestBranch).mockRejectedValueOnce(new Error("network timeout")); diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index d8954b0a1d..700a9d5cc6 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -3093,6 +3093,40 @@ describe("queue processors", () => { expect(seen.merged).toBe(true); }); + it("#9497 skips the compare read when the repo has no stored default branch (nothing to compare against)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedFreshRebaseRepo(env, 98, { staleBaseAheadByThreshold: 5 }); + // Clear the stored default branch so the compare has no ref to anchor on. + await env.DB.prepare("UPDATE repositories SET default_branch = NULL WHERE full_name = ?").bind("owner/repo").run(); + const seen = { merged: false, updateBranchCalls: 0, baseCommitCalls: 0, compareCalls: 0 }; + stubFreshRebaseFetch(98, { baseAdvancedAt: new Date(Date.now() - 60 * 60_000).toISOString(), aheadBy: 999 }, seen); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "stale-threshold-no-default-branch", repoFullName: "owner/repo", prNumber: 98, installationId: 123 }); + + expect(seen.compareCalls).toBe(0); + expect(seen.updateBranchCalls).toBe(0); + expect(seen.merged).toBe(true); + }); + + it("#9497 records the cap-exceeded audit with the staleness trigger named, once the 24h cap is spent", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seedFreshRebaseRepo(env, 99, { staleBaseAheadByThreshold: 5 }); + const seen = { merged: false, updateBranchCalls: 0, baseCommitCalls: 0, compareCalls: 0 }; + stubFreshRebaseFetch(99, { baseAdvancedAt: new Date(Date.now() - 60 * 60_000).toISOString(), aheadBy: 10 }, seen); + + // Spend the per-PR forced-rebase budget so the next attempt hits the cap branch. + for (let i = 0; i < 3; i += 1) { + await processJob(env, { type: "agent-regate-pr", deliveryId: `stale-cap-${i}`, repoFullName: "owner/repo", prNumber: 99, installationId: 123 }); + } + await processJob(env, { type: "agent-regate-pr", deliveryId: "stale-cap-final", repoFullName: "owner/repo", prNumber: 99, installationId: 123 }); + + const capped = await env.DB.prepare("select detail from audit_events where event_type = ? order by created_at desc limit 1") + .bind("agent.action.fresh_rebase_window_cap_exceeded") + .first<{ detail: string }>(); + // The audit names WHICH trigger fired, not just the freshness window it used to hardcode. + expect(capped?.detail).toContain("commits ahead"); + }); + it("#9497 INVARIANT: no compare call at all when no threshold is configured (zero added cost)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seedFreshRebaseRepo(env, 97, {}); From 3a5b9a9aaac9ce084135f9cc84a9ef41da664108 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:09:44 -0700 Subject: [PATCH 3/4] test(queue): pin the sibling-lookup-miss wake omitting prCreatedAt (#9499) With no local PR row there is no createdAt to thread, so the wake goes out without the sort hint rather than being skipped -- the same fail-open posture as the bare cooldown key beside it. --- test/unit/queue-3.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 700a9d5cc6..6bb9598204 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -5635,6 +5635,10 @@ describe("queue processors", () => { }); expect(fanned.some((job) => job.type === "agent-regate-pr" && job.prNumber === 56)).toBe(true); // still woken despite the lookup miss + // #9499: with no local row there is no createdAt to thread, so the wake goes out WITHOUT the sort hint + // rather than being skipped -- the same fail-open posture as the bare cooldown key below. + const woken = fanned.find((job) => job.type === "agent-regate-pr" && job.prNumber === 56) as { prCreatedAt?: string } | undefined; + expect(woken?.prCreatedAt).toBeUndefined(); const call = setSpy.mock.calls.find(([key]) => (key as string).startsWith("contributor-cap-wake:jsonbored/gittensory#56")); expect(call?.[0]).toBe("contributor-cap-wake:jsonbored/gittensory#56"); // bare key -- no headSha suffix expect(call?.[2]).toBe(60); // CI_COALESCE_WINDOW_SECONDS, not the ~1800s headSha-keyed cooldown From 085383c00acf81819ba250ea8ab2ea70e2e0d548 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 22:11:43 -0700 Subject: [PATCH 4/4] test(queue): pin the sibling-lookup-miss wake and annotate the throwing-lookup arm (#9499) --- src/queue/processors.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c3387deb55..dcd9beeb1a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -5153,6 +5153,9 @@ async function wakeOverCapSiblingPullRequests( // #9499: the sibling row is already fetched just above for the cooldown key, so threading its // createdAt costs nothing. Without it, jobClaimSortKey falls back to a legacy base that sorts this // wake ahead of every genuinely older contributor PR. + /* v8 ignore next -- the nullish arm needs the lookup above to THROW (it is .catch(() => null)); a PR + merely absent locally still yields a row here. That path degrades exactly like the covered + bare-cooldown-key case beside it: the wake still goes out, just without the sort hint. */ ...(sibling?.createdAt ? { prCreatedAt: sibling.createdAt } : {}), }); } catch (error) {