diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9344c12276..dcd9beeb1a 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,13 @@ 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. + /* 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) { console.log( diff --git a/src/review/pr-reconciliation.ts b/src/review/pr-reconciliation.ts index f5746935e8..a3c3eec9cc 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, 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..b82ea15a7d 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 }> = []; 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 }>(); 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,10 @@ 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. 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/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/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/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..6bb9598204 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,82 @@ 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 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, {}); + 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 }); @@ -5553,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 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(),