fix(pr-fix): stop the merged-PR and human-review fix loops (#1000, #1001)

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<number, "merged" | "closed">();

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<number, "merged" | "closed">([[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 () => {

src/lib/pr-fix-queue.ts

@@ -194,6 +194,17 @@ function metadataPatch(input: EnqueuePrFixInput): Record<string, string | number
   return patch;
 }
 
+/**
+ * Bound on automatic fix attempts before a REVIEW_FEEDBACK/CI item is handed to
+ * a human instead of re-queued. Distinct evidence keys count attempts (each
+ * coder push draws a fresh automated review → a new key). Overridable via
+ * PR_FIX_MAX_ATTEMPTS; defaults to 5.
+ */
+export function maxPrFixAttempts(): number {
+  const n = Number(process.env.PR_FIX_MAX_ATTEMPTS);
+  return Number.isInteger(n) && n > 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;