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

[joryirving]

Fixes the REVIEW_FEEDBACK fix loop that churns a coder forever on a PR — including the "my review loops forever" case. Closes #1000 and #1001.

#1000 — merged PRs never stay reaped

The per-sync reap (reconcileStalePrFixItems) already marks a merged/closed PR's item STALE, and it runs (the scheduler fires pr-followup every 15m). But enqueuePrFixItem then flips any non-known status back to QUEUED on the next fresh review/check event — so the reap is undone every sweep and the bridge keeps dispatching a coder against a PR that no longer exists. Observed live on pr-reviewer-action #593/#595 (both merged hours earlier, still QUEUED and re-touched every tick).

Fix: STALE and IGNORED are now sticky — re-observed evidence never resurrects them; only an explicit requeue reopens (and that already refuses merged/closed PRs). The reap's closed-PR window is also widened 30 → 100 so a merge that scrolled past the recent window is still caught.

#1001 — the loop is unbounded

Each coder push draws a fresh automated review with a new evidenceKey, so a human CHANGES_REQUESTED that N automated fixes never satisfy re-queues indefinitely (the reviewer's decision stays CHANGES_REQUESTED until the human re-reviews, which per-iteration they won't).

Fix: distinct evidence keys count fix attempts; past PR_FIX_MAX_ATTEMPTS (default 5) the item is routed to a human (BLOCKED / NEEDS_HUMAN) instead of re-queuing. Applies to all pr-fix types — a check failing 6+ times isn't converging either.

Notes

• No schema change — attempts derive from the existing evidenceKeys array; no Prisma migration.
• The existing evidence-dedupe (chore(deps): lock file maintenance #25) and the FIXED-reopen recovery ([P2] A pr-fix item marked FIXED on workload exit alone strands the PR forever, with no way back #940) are preserved; the new terminal-stickiness and cap sit alongside them in the single enqueue choke point.
• Regression tests added for both the STALE-resurrection path and the attempt cap. tsc --noEmit, eslint, and the full pr-fix-queue / pr-followup-ingestion suites (109 tests) pass.

A follow-up worth doing separately: capture the review author (human vs the AI reviewer bot) so a human review can escalate faster than a bot one — today the ingestion only records the PR author. The attempt cap makes the loop terminate regardless.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)

Recommendation

Approve. This is a tightly scoped, well-explained fix that directly addresses the two linked issues (PR 1000 merged-PR reap resurrection, PR 1001 unbounded human-review loop) with the smallest possible change to the single enqueue choke point. CI is fully green (lint, typecheck, tests, db integration, docker build, smoke). The new test coverage exercises both the STALE-stickiness path and the attempt-cap path, and the existing PR 940 FIXED-recovery path is preserved (its test was even updated to lift the new cap out of the way).

Change-by-change findings

src/lib/pr-fix-queue.ts — core logic

• maxPrFixAttempts() (new exported helper): parses PR_FIX_MAX_ATTEMPTS with a safe integer+positive guard, defaults to 5. Clean, env-overridable, matches the AGENTS.md convention of additive env vars with safe defaults. No docs update required for a non-required tunable, but a one-line .env.example entry would be a nice follow-up.
• Terminal-status stickiness (PR 1000): STALE and IGNORED are now sticky — re-observed evidence never resurrects them. The resolution chain (isTerminalStatus → reopenFixStale → isKnownEvidence → capExceeded → fallback) is ordered correctly: terminal beats the PR 940 recovery (which only applies to FIXED anyway, but the explicit ordering avoids future regressions), recovery beats dedupe, dedupe beats cap, cap is the inner guard. Good.
• Attempt cap (PR 1001): nextEvidenceKeys.length > maxPrFixAttempts() after append triggers BLOCKED + NEEDS_HUMAN with an audit note. The surfacePrFixBlocked side-effect is only invoked in the cap-exceeded branch, which is the correct semantics — NEEDS_HUMAN lane should always surface, and the existing test verifies toHaveBeenCalledTimes(1). The cap also requires !isKnownEvidence, so repeat evidence can't artificially inflate the counter — correct.
• Lane resolution: resolvedLane is initialized to the incoming lane and only overridden in the cap-exceeded path. The previous code unconditionally overwrote lane on every update, which was actually a latent bug for the PR 940 recovery (it would clobber any operator-applied lane change); the new code preserves the incoming lane in the non-cap branches. Net improvement.
• History note: when statusNote is set, it's written into the history row alongside the existing PR 940 reopen note. Audit trail is preserved.
• Edge case — IGNORED stickiness: worth flagging that IGNORED items are also made sticky here. The PR body and PR 1000 only explicitly call out STALE, but making IGNORED sticky too is the right call: re-observed evidence shouldn't unsuppress an explicitly ignored PR either. Consistent with the existing IGNORED rejection in requeuePrFixItem (line ~578 per impact scan). No issue.

src/app/api/pr-followup/sync/route.ts — window widening

• One-line change: fetchClosedPullRequests(repoFullName, 30) → fetchClosedPullRequests(repoFullName, 100). Aligns with the rest of the file, which already uses per_page=100 throughout (lines 164-178 per impact scan) and matches reconcile/route.ts which uses 100 for the same call. Safe widening, no rate-limit concern beyond what already exists (the loop is already rate-limit-guarded by the existing rateLimited break at the top of the repo loop).

src/lib/pr-fix-queue.test.ts — regression coverage

• New test "does not resurrect a STALE item on new evidence (#1000)" exercises the exact failure mode described in the linked issue: reap → STALE → fresh evidence → must stay STALE. Asserts both the status and the queue-listing side effect (includeBlocked: true returns []). Good.
• New test "blocks a REVIEW_FEEDBACK item after PR_FIX_MAX_ATTEMPTS distinct attempts (#1001)" sets the env var to 3, enqueues 3 distinct attempts (all QUEUED), then a 4th distinct attempt flips to BLOCKED/NEEDS_HUMAN and asserts surfacePrFixBlocked is called once. The try/finally env restore is correct.
• The existing buildPrFixBlockedContext "uses uncapped history for totalAttempts" test was updated to raise the cap to 100 so the history-based counter (which is uncapped and looks at all 13 enqueues) is not confounded by the new cap-based re-laning. Good hygiene — the comment explicitly calls out that this test exercises history counting, not the PR 1001 bound.

Sources

• Linked issues PR 1000 and PR 1001 bodies confirm the intended fix shape (STALE-not-reaped, unbounded human-review loop). PR implements both with the simplest correct mechanism.
• AGENTS.md label-hygiene section is not relevant here (no label changes).
• AGENTS.md env-var table is the precedent for PR_FIX_MAX_ATTEMPTS as an additive, optional, safe-defaulted env var.
• Repository history (#942) confirms the pre-existing PR 940 FIXED-recovery pattern that this PR sits alongside, not replaces.
• Repository impact scan shows fetchClosedPullRequests(repoFullName, 100) is already the established convention in reconcile/route.ts and elsewhere — the window widening aligns with existing usage.

Standards Compliance

• No agent-specific names in generic docs: PR body uses generic foreman-coder/coder terminology consistent with the codebase's agent labels. No issues.
• Prisma schema: no schema changes; attempts derive from existing evidenceKeys array. Matches AGENTS.md guidance to keep relations strict and avoid migrations for derived state.
• API routes / HTTP status codes: the existing /api/pr-fix-queue/enqueue route is unchanged in shape; this is purely internal logic. No new routes added.
• Error handling: maxPrFixAttempts() uses Number.isInteger(n) && n > 0 to guard against NaN, negative, and fractional values from a misconfigured env var. Defensive.
• Validation: the new env-var read is the only external input; validation is correct.
• No secrets: no env files or secrets touched.
• Tech stack consistency: pure TypeScript change in lib/ + a one-line numeric tweak in an existing route. No new dependencies.

Linked Issue Fit

PR 1000 — "pr-fix not reaped on PR merge/close"

• Issue asks: "drop (or mark terminal) any open REVIEW_FEEDBACK / CI-failure pr-fix records for that PR so they are never dispatched again."
• PR fix: makes STALE (and IGNORED) sticky in enqueuePrFixItem, so the per-sync reap is no longer undone by the next fresh evidence event. Widens the reap window from 30→100 closed PRs so merges that scrolled past the recent window are still caught.
• Acceptance criteria: ✅ met. The reap itself already marks STALE; this PR closes the resurrection hole and widens the window.

PR 1001 — "the loop is unbounded"

• Issue title in PR body describes a human CHANGES_REQUESTED that automated fixes never satisfy, re-queueing indefinitely.
• PR fix: distinct evidenceKeys count attempts; past PR_FIX_MAX_ATTEMPTS (default 5) the item is routed to BLOCKED / NEEDS_HUMAN instead of re-queuing.
• Acceptance criteria: ✅ met. The loop is now bounded; a test explicitly verifies the cap-exceeded path triggers surfacePrFixBlocked.

Both linked issues' stated problem, evidence, expected behavior, and "where" guidance are addressed by this PR.

Tool Harness Findings

The native tool-calling loop issued no tool calls; CI was the only ground truth available. CI is fully green on commit c9a72faf: lint, typecheck, tests, db integration, db migrations, coverage, docker build (both variants), npm audit, and smoke all pass. The PR body claims 109 tests pass, which is consistent with CI's Tests: success state.

Unknowns or Needs Verification

• .env.example documentation for PR_FIX_MAX_ATTEMPTS: not added in this PR. Not strictly required (AGENTS.md env-var table is the canonical source for tunable vars, and this is optional/advanced), but a one-line # PR_FIX_MAX_ATTEMPTS=5 entry would round out the change. Minor, not blocking.
• CI/failure pr-fix types under the cap: the PR body notes "Applies to all pr-fix types — a check failing 6+ times isn't converging either." The test only exercises the REVIEW_FEEDBACK path; the CI-failure path is covered by the same code branch but not by an explicit test. Acceptable given the unified resolution logic, but a future PR could add a CI-failure-specific test for symmetry. Not blocking.
• Follow-up noted by author: "capture the review author (human vs AI reviewer bot) so a human review can escalate faster." The author explicitly defers this to a follow-up and notes the attempt cap makes the loop terminate regardless. Reasonable scope discipline.

No blockers found.