diff --git a/src/app/api/pr-followup/sync/route.ts b/src/app/api/pr-followup/sync/route.ts index 7795c9ea..52fe9ad9 100644 --- a/src/app/api/pr-followup/sync/route.ts +++ b/src/app/api/pr-followup/sync/route.ts @@ -365,7 +365,7 @@ export async function POST(request: NextRequest) { for (const repoFullName of repoFullNames) { if (rateLimited) break; try { - const closedPrs = await fetchClosedPullRequests(repoFullName, 30); + const closedPrs = await fetchClosedPullRequests(repoFullName, 100); if (closedPrs.length > 0) { mergedOrClosedPrsByRepo.set(repoFullName, new Set(closedPrs.map((pr) => pr.number))); const statesMap = new Map(); diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 7ce3a31c..feac42f9 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -184,6 +184,44 @@ describe("PR review-fix queue", () => { expect(await listQueuedPrFixItems(client, { lane: "NORMAL" })).toEqual([]); expect(client.history.at(-1)).toMatchObject({ action: "mark", status: "FIXED", note: "pushed fix + validation" }); }); + + it("does not resurrect a STALE item on new evidence (#1000)", async () => { + await enqueuePrFixItem(client, { repo: "org/repo", pr: 7, lane: "NORMAL", reason: "review", feedback: "f1", evidenceKey: "review:1" }); + // PR merges → the per-sync reap stales it. + await reconcileStalePrFixItems( + client, + new Map([["org/repo", new Set([7])]]), + new Map([["org/repo", new Map([[7, "merged"]])]]), + ); + expect(client.items[0].status).toBe("STALE"); + + // A fresh automated review lands after the merge (a new evidence key). + // Before #1000 this flipped the item back to QUEUED and the coder looped + // forever on a merged PR. It must stay STALE. + const after = await enqueuePrFixItem(client, { repo: "org/repo", pr: 7, lane: "NORMAL", reason: "review", feedback: "f2", evidenceKey: "review:2" }); + expect(after.status).toBe("STALE"); + expect(await listQueuedPrFixItems(client, { includeBlocked: true })).toEqual([]); + }); + + it("blocks a REVIEW_FEEDBACK item after PR_FIX_MAX_ATTEMPTS distinct attempts (#1001)", async () => { + const prev = process.env.PR_FIX_MAX_ATTEMPTS; + process.env.PR_FIX_MAX_ATTEMPTS = "3"; + try { + for (let i = 1; i <= 3; i++) { + const item = await enqueuePrFixItem(client, { repo: "org/repo", pr: 9, lane: "NORMAL", reason: "review", feedback: `f${i}`, evidenceKey: `review:${i}` }); + expect(item.status).toBe("QUEUED"); + } + // The 4th distinct attempt exceeds the cap → hand to a human instead of + // re-queuing (the human-review-forever case). + const blocked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 9, lane: "NORMAL", reason: "review", feedback: "f4", evidenceKey: "review:4" }); + expect(blocked.status).toBe("BLOCKED"); + expect(blocked.lane).toBe("NEEDS_HUMAN"); + expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + } finally { + if (prev === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; + else process.env.PR_FIX_MAX_ATTEMPTS = prev; + } + }); }); describe("reconcileStalePrFixItems", () => { @@ -641,21 +679,31 @@ describe("buildPrFixBlockedContext", () => { }); it("uses uncapped history for totalAttempts", async () => { - const client = makeClient(); - for (let i = 0; i < 13; i += 1) { - await enqueuePrFixItem(client, { - repo: "org/repo", - pr: 8, - lane: "NORMAL", - reason: `failure ${i}`, - feedback: `attempt ${i}`, - evidenceKey: `k${i}`, - }); + // Raise the attempt cap out of the way — this test exercises history-based + // attempt counting, not the #1001 bound (which would otherwise re-lane the + // later attempts to NEEDS_HUMAN). + const prev = process.env.PR_FIX_MAX_ATTEMPTS; + process.env.PR_FIX_MAX_ATTEMPTS = "100"; + try { + const client = makeClient(); + for (let i = 0; i < 13; i += 1) { + await enqueuePrFixItem(client, { + repo: "org/repo", + pr: 8, + lane: "NORMAL", + reason: `failure ${i}`, + feedback: `attempt ${i}`, + evidenceKey: `k${i}`, + }); + } + + const context = await buildPrFixBlockedContext(client, client.items.find((item) => item.pr === 8)); + expect(context.totalAttempts).toBe(13); + expect(context.attemptsByLane).toEqual({ NORMAL: 13 }); + } finally { + if (prev === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; + else process.env.PR_FIX_MAX_ATTEMPTS = prev; } - - const context = await buildPrFixBlockedContext(client, client.items.find((item) => item.pr === 8)); - expect(context.totalAttempts).toBe(13); - expect(context.attemptsByLane).toEqual({ NORMAL: 13 }); }); it("returns no per-lane/signature data when absent (backwards compatible)", async () => { diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index f4a3dbfb..b5fe8cc1 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -194,6 +194,17 @@ function metadataPatch(input: EnqueuePrFixInput): Record 0 ? n : 5; +} + export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueuePrFixInput) { const lane = normalizePrFixLane(input.lane); const type = normalizePrFixType(input.type); @@ -228,19 +239,49 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP existing.headSha === input.headSha; const reopenFixStale = isKnownEvidence && headShaUnchanged; + // Terminal states are sticky. Once an item is STALE (its PR merged or + // closed) or IGNORED, re-observed evidence must NOT resurrect it to + // QUEUED — the only way back is an explicit requeue, which refuses + // merged/closed PRs. Without this the per-sync reap that stales a merged + // PR is immediately undone by the next fresh review/check event, looping + // a coder forever on a PR that no longer exists (#1000; observed on + // pr-reviewer-action #593/#595). + const isTerminalStatus = existing.status === "STALE" || existing.status === "IGNORED"; + + // Bound the fix loop. Each coder push draws a fresh automated review with + // a new evidenceKey, so distinct keys count fix attempts. Past the cap, + // stop re-queuing and hand the PR to a human — otherwise a human + // CHANGES_REQUESTED that N automated fixes never satisfy loops forever + // (#1001). + const nextEvidenceKeys = uniqueAppend(existing.evidenceKeys ?? [], input.evidenceKey, 40); + const capExceeded = !isKnownEvidence && nextEvidenceKeys.length > maxPrFixAttempts(); + + let resolvedStatus: PrFixStatus; + let resolvedLane: PrFixLane = lane; + let statusNote: string | null = null; + if (isTerminalStatus) { + resolvedStatus = existing.status; // sticky — never resurrect a gone PR + } else if (reopenFixStale) { + resolvedStatus = nextStatus; // #940 recovery from a no-progress FIXED tombstone + } else if (isKnownEvidence) { + resolvedStatus = existing.status; // #25 anti-churn: repeat evidence never flips status + } else if (capExceeded) { + resolvedStatus = "BLOCKED"; + resolvedLane = "NEEDS_HUMAN"; + statusNote = `Bounded at ${nextEvidenceKeys.length} attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); routed to a human instead of re-queuing (#1001).`; + } else { + resolvedStatus = nextStatus; + } + const updated = await tx.prFixQueueItem.update({ where: { id: existing.id }, data: { - lane, + lane: resolvedLane, type, - // New evidence on a stale FIXED → reopen to QUEUED so the loop - // dispatches another fix attempt. Without the reopen we'd write - // another `enqueue` history row against a `FIXED` tombstone and - // strand the PR (the worked example in #940). - status: reopenFixStale ? nextStatus : isKnownEvidence ? existing.status : nextStatus, + status: resolvedStatus, reason: input.reason, feedback: uniqueAppend(existing.feedback ?? [], input.feedback, 12), - evidenceKeys: uniqueAppend(existing.evidenceKeys ?? [], input.evidenceKey, 40), + evidenceKeys: nextEvidenceKeys, ...metadataPatch(input), }, }); @@ -253,6 +294,8 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, input: EnqueueP }; if (reopenFixStale) { historyData.note = `Reopened: PR head SHA unchanged since FIXED (${existing.headSha}); re-detected evidence on a no-progress tombstone (#940).`; + } else if (statusNote) { + historyData.note = statusNote; } await tx.prFixHistory.create({ data: historyData }); return updated;