diff --git a/src/review/active-review-reconciliation.ts b/src/review/active-review-reconciliation.ts index d7bf44cfe..c23543f67 100644 --- a/src/review/active-review-reconciliation.ts +++ b/src/review/active-review-reconciliation.ts @@ -112,9 +112,15 @@ export async function runActiveReviewReconciliation(env: Env, nowMs: number = Da if (!changed) continue; // a concurrent pass already terminalized (or restarted) this row first reconciled.push({ repoFullName: row.repoFullName, pullNumber: row.pullNumber }); incr("loopover_active_review_reconciliation_terminalized_total", { repo: row.repoFullName }); - console.error( + // warn, not error (LOOPOVER-2K): a successful terminalization is this feature WORKING, not an anomaly -- + // logging it at error level turned a backlog drain into 547 Sentry error events in a day (one per healed + // row, message-fingerprint-collapsed into a single escalating issue). warn stays visible in stdout/ + // Workers Logs but sits below the default SENTRY_MIN_SEVERITY of "error" (selfhost/sentry.ts's + // resolveSentryMinSeverity), so Sentry only sees it when an operator has explicitly lowered the + // threshold. The row_error/scan-error paths below stay at error: a FAILED heal is still an anomaly. + console.warn( JSON.stringify({ - level: "error", + level: "warn", event: "active_review_reconciliation_orphan_terminalized", repository: row.repoFullName, pullNumber: row.pullNumber, diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index f86d778c9..d43636d92 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -15,7 +15,7 @@ import { upsertGlobalContributorBlacklist, } from "../db/repositories"; import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; -import { classifyMergeFailure, isMergeConflictMessage, MERGE_RETRY_CAP } from "./merge-failure"; +import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { resolveDispositionReason } from "../review/outcomes-wire"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; @@ -626,6 +626,11 @@ 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" && 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 + // mergeable_state read. Nothing went wrong and nothing is stuck: the caller falls through to reviewing + // the current head exactly as in the conflict case above. Audit-only; never a Sentry page. } else { // Non-merge action classes have no retry loop -- a single failure here is already this pass's terminal // outcome (the planner may re-attempt on the next sweep if the underlying condition clears itself), so diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index 8000ce699..950440be9 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -30,6 +30,15 @@ export function isMergeConflictMessage(message: string): boolean { return /merge conflict|not mergeable|cannot be merged|has conflicts|conflicts? with the base/i.test(message); } +/** True for the 422 "There are no new commits on the base branch." update-branch rejection (LOOPOVER-24, + * regressed shape): the readiness check saw mergeable_state "behind" but the head already contained every + * base commit by the time update-branch fired -- a stale-mergeable-state race, not a failure. The PR is in + * exactly the state update_branch exists to reach, so the executor treats it as benign (audit-only, no + * Sentry capture), same as {@link isMergeConflictMessage}'s update_branch carve-out. */ +export function isNoNewBaseCommitsMessage(message: string): boolean { + return /no new commits on the base branch/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/active-review-reconciliation.test.ts b/test/unit/active-review-reconciliation.test.ts index 115055811..4bffce283 100644 --- a/test/unit/active-review-reconciliation.test.ts +++ b/test/unit/active-review-reconciliation.test.ts @@ -119,15 +119,19 @@ describe("runActiveReviewReconciliation (#webhook-reorder-clobber)", () => { await seedStaleActiveRow(env, "owner/repo", 1, 9500, STALE_ACTIVE_REVIEW_MIN_AGE_MS + 60_000); vi.spyOn(backfillModule, "fetchLivePullRequestState").mockResolvedValueOnce("closed"); const errors = vi.spyOn(console, "error").mockImplementation(() => undefined); + const warns = vi.spyOn(console, "warn").mockImplementation(() => undefined); const reconciled = await runActiveReviewReconciliation(env); expect(reconciled).toEqual([{ repoFullName: "owner/repo", pullNumber: 1 }]); expect(await hasActiveReviewForHeadSha(env, "owner/repo", 1, "sha1")).toBe(false); expect(counterValue("loopover_active_review_reconciliation_terminalized_total", { repo: "owner/repo" })).toBe(1); - const logged = errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("active_review_reconciliation_orphan_terminalized")); + // REGRESSION (LOOPOVER-2K): the per-row success log is warn-level -- a successful self-heal must never + // reach Sentry's error-level forwarder (547 error events in a day during the backlog drain). + const logged = warns.mock.calls.map((c) => String(c[0])).find((line) => line.includes("active_review_reconciliation_orphan_terminalized")); expect(logged).toBeDefined(); - expect(JSON.parse(logged!)).toMatchObject({ level: "error", event: "active_review_reconciliation_orphan_terminalized", repository: "owner/repo", pullNumber: 1 }); + expect(JSON.parse(logged!)).toMatchObject({ level: "warn", event: "active_review_reconciliation_orphan_terminalized", repository: "owner/repo", pullNumber: 1 }); + expect(errors.mock.calls.map((c) => String(c[0])).find((line) => line.includes("active_review_reconciliation_orphan_terminalized"))).toBeUndefined(); }); it("leaves a stale row alone when the LIVE check says the PR is still open", async () => { diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 22e6c5f6b..7b289879b 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -1472,6 +1472,23 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { captureSpy.mockRestore(); }); + it("REGRESSION (LOOPOVER-24, regressed shape): a 422 'no new commits on the base branch' update_branch failure does not page Sentry", async () => { + const env = createTestEnv({}); + // The readiness check saw a stale "behind" mergeable_state; the head was already up to date when + // update-branch fired. GitHub rejects with this 422 -- a benign no-op, not a failure to page on. + vi.mocked(updatePullRequestBranch).mockRejectedValueOnce( + Object.assign(new Error("There are no new commits on the base branch. - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch"), { status: 422 }), + ); + const captureSpy = vi.spyOn(sentryModule, "captureError"); + + 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"); + expect(captureSpy).not.toHaveBeenCalled(); + 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 ae0c6c076..64d136dcc 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, MERGE_RETRY_CAP } from "../../src/services/merge-failure"; +import { classifyMergeFailure, isNoNewBaseCommitsMessage, 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 { @@ -57,3 +57,14 @@ describe("classifyMergeFailure", () => { expect(MERGE_RETRY_CAP).toBeGreaterThan(0); }); }); + +describe("isNoNewBaseCommitsMessage", () => { + it("REGRESSION (LOOPOVER-24, regressed shape): matches GitHub's 422 'no new commits on the base branch' text", () => { + expect(isNoNewBaseCommitsMessage("There are no new commits on the base branch. - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch")).toBe(true); + }); + + it("does not match unrelated update-branch failures (conflicts, transients)", () => { + expect(isNoNewBaseCommitsMessage("merge conflict between base and head")).toBe(false); + expect(isNoNewBaseCommitsMessage("network timeout")).toBe(false); + }); +});