feat(pr-fix): accept external waits without charging fix attempts

[joryirving]

Closes #1231. Companion to misospace/courier#271.

Contract

• Accept outcome:"waiting-external" for implement reports (no queue token needed) and queue-backed followup reports; records a neutral run, never failed or human-blocked.
• Accept Courier exact mark {repo,pr,status:"WAITING_EXTERNAL",note,generation}. Queue-backed reports route through the same transition.
• Entering the wait preserves issued generation and fixAttempts, including at the cap. Repeated report/mark signals succeed idempotently without extra history or extending the cooldown. All wait mutations are generation/status CAS fenced and transactionally audited.
• Preserve fix(pr-fix): separate human decisions from retry exhaustion (#1218) #1221: genuine decisions BLOCKED, exhaustion PARKED, deliberate maintainer reclassification opt-in unchanged. Waiting tokens cannot overwrite either classification.

Discovery and recovery traced before implementation

Current queue listing serves QUEUED only; same-agent handouts are suppressed per generation. Merely adding WAITING_EXTERNAL would strand work. Courier provides no structured prerequisite-ready signal, only a note, so recovery is a bounded prerequisite recheck rather than claiming readiness is known.

Persist retryAt (migration included). Default cooldown 6h, configurable PR_FIX_EXTERNAL_WAIT_COOLDOWN_MS (1min–7days). Canonical queue discovery lazily reactivates up to 100 due waits per poll, CAS fenced on status/generation/deadline, with a fresh generation and cleared handout state but NO budget increment or cap escalation. A new generation fences late old reports and lets the same worker receive the task. Future waits remain undispatchable; repeated waits schedule another cooldown. Upstream closed/archived cleanup includes waits.

Tests and validation

• 704 tests across 14 focused queue/report/next-task/surfacing/ingestion suites passed; final 288 route/discovery/external-wait tests rerun after typing corrections.
• Typecheck, lint, diff check passed; Prisma client generated.
• Repeated waits at cap; both report/mark orderings; deadline/history idempotency; concurrent duplicate/decision and reactivation races; same-agent rediscovery/new generation; recovery followed by FIXED; closed/archived cleanup; implement report without token; worker mark payload.
• Full local suite did not complete within 300s and showed failures in untouched AgentWork auth/UI tests; not claimed green locally. CI is authoritative for whole-suite validation.

Courier contract fixture lockstep note remains a Courier-side follow-up upon Dispatch landing; exact emitted payload checked against PR271 head cad944b32a4d92f7f1acb54e2e1dd05b60558bb2.

Open for human review only. No approval or merge.

[joryirving]

Review of exact head 18f8cb8 for #1231 / Courier #271. 13/13 CI checks green. The main status/generation CAS, cooldown reactivation with fresh generation and no fix-attempt charge, duplicate mark/report settlement, and closed/archived cleanup are well scoped. Please address the following before merge (author is joryirving, so leaving a top-level comment rather than a self-review).

[P2] The actual Courier report-first ordering loses the external prerequisite explanation. Courier #271 internal/source/dispatch/client.go:559–568 calls reportTaskWithPR(..., outcomeWaitingExternal, lifecycle.Error, ...) before posting markPRFixWaitingExternal(..., lifecycle.Error). reportTaskWithPR puts the supplied explanation in task-report error, not summary (:469–470). Here, src/app/api/agents/[agentName]/tasks/report/route.ts:589–603 forwards summary: report.summary only, and the new resolvePrFixFromAgentReport branch (src/lib/pr-fix-queue.ts:2704–2710) builds the WAITING_EXTERNAL history note from that missing summary. The following queue mark carries the real note, but markPrFixItem returns duplicate success immediately at :1598–1601 without persisting it. Thus the normal Courier execution records PrFixHistory(status=WAITING_EXTERNAL, note=NULL), dropping precisely which prerequisite was missing. The added report-first test only asserts history length, not note content.

Required: Preserve a bounded/sanitized explanation from the report's error field for waiting-external (or idempotently fill the first WAITING history note on a later explicit mark), without extending the deadline or producing duplicate history. Add a regression using Courier's real report payload (outcome:"waiting-external", error:"missing prerequisite: X", absent summary) followed by its WAITING_EXTERNAL mark, asserting exactly one audit event with the useful explanation, plus the mark-first case.

[P2 / observability] WAITING_EXTERNAL is invisible in the operator PR-fix queue, with no manual early-wake path. src/app/automation/pr-fix-queue/page.tsx:57–63 fetches /api/pr-fix-queue/queued?include_blocked=true, but listQueuedPrFixItems at src/lib/pr-fix-queue.ts:1470 includes only QUEUED/BLOCKED/PARKED in that mode, and the page renders only those three groups. WAITING_EXTERNAL items disappear from the UI for the full six-hour default cooldown (up to seven days configured). requeuePrFixItem at :2417+ also refuses this status, so even an operator who knows the prerequisite is restored cannot release it early. Please at minimum expose waiting items read-only with retryAt and last-known explanation in the operator view. An explicit maintainer early-wake can be a separate scoped follow-up if you want to avoid broadening #1233, but don't leave the new state entirely unobservable.

Automated-review corrections: Its migration blocker is incorrect for PostgreSQL 12+: adding an enum value within a transaction is permitted as long as it is not used before commit; the second migration statement only adds an unrelated nullable timestamp column. Official PostgreSQL ALTER TYPE docs confirm this, and this head's Database migrations + Database integration checks are green. The alleged missing null check is also already present at src/app/api/pr-fix-queue/mark/route.ts:80. No migration split or null-guard change is needed on this head.

Cross-repo rollout: #271 still needs its own same-generation/reported-no-change review findings resolved; #1233's generation bump solves the Dispatch half of the external-wait reoffer but does not alone make Courier #271 safe to merge.

[joryirving]

Addressed both P2s from comment 6129229949.

• The report route forwards error; external-wait resolution uses summary when supplied, otherwise error, sanitized and bounded to 500 characters. Courier report-first ordering now preserves the prerequisite explanation in the first WAITING_EXTERNAL audit event. Duplicate marks still neither add history nor extend retryAt.
• Operator include_blocked listing includes WAITING_EXTERNAL with retryAt and latest waiting audit note. The operator page renders a separate read-only waiting section. Default worker listing remains QUEUED-only. No manual early-wake behavior added.

Regressions cover Courier error-only report-first and mark-first ordering, exactly one useful audit event with unchanged deadline, bounded explanation, route error forwarding, operator listing versus default worker listing, and the read-only UI explanation/deadline.

357 focused tests pass; typecheck/lint pass. Full local suite still has the two untouched AgentWork authorization test failures previously observed. No migration or null-guard changes made. Leaving open for human review; no approval or merge.

Superseded by a newer automated review for this pull request.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3 (anthropic) — primary route · pr-reviewer-action v3.4.1

Findings (2 info)

Severity	Location	Finding
Info		Author notes local AgentWork auth/UI test failures pre-existing and outside this PR's surface; 13/13 CI checks green on head bb97134 — CI is authoritative per AGENTS.md.
Info		Courier-side contract fixture update is tracked as a Courier follow-up; not gating for Dispatch PR 1231.

Recommendation: approve

This is a well-scoped feature PR that adds WAITING_EXTERNAL status and waiting-external outcome for the PR-fix queue. The implementation introduces a small database migration, four generator-side changes (enum, retryAt column, helper functions, markPrFixItem branch), and a parallel report-route branch. It is defensively designed around the existing generation fence and stickiness invariants from PR 1221/PR 1211/PR 1133, and there is test coverage for every advertised invariant.

Change-by-change findings

prisma/migrations/20261013000000_pr_fix_external_wait/migration.sql — Two idempotent statements: ALTER TYPE "PrFixStatus" ADD VALUE IF NOT EXISTS 'WAITING_EXTERNAL' and ALTER TABLE … ADD COLUMN "retryAt" TIMESTAMP(3). PostgreSQL 12+ permits adding an enum value within a transaction block when the value is not consumed in the same transaction, so the column-adding ALTER TABLE on the next line does not invalidate the transaction safety. The migration is small, additive, and reversible by dropping the column and the enum value via separate prisma migrate-generated rollback.

prisma/schema.prisma — Adds WAITING_EXTERNAL to the PrFixStatus enum and retryAt DateTime? to PrFixQueueItem. Consistent with surrounding field shape, default-null, no impact on existing rows.

src/lib/pr-fix-queue.ts — Adds externalWaitCooldownMs() (bounded 1m–7d, default 6h) and a private reactivateDueExternalWaits(). listQueuedPrFixItems calls reactivateDueExternalWaits before fetching and includes WAITING_EXTERNAL only when includeBlocked: true. The default worker listing path stays QUEUED-only, satisfying the same-agent-handout suppression invariant. markPrFixItem for WAITING_EXTERNAL is fenced on id + generation (the same updateMany pattern other settlements use) and explicit idempotency: a repeat on a row already at the same generation returns { mutated: true, item: existing } without re-writing retryAt or appending history. resolvePrFixFromAgentReport adds a waiting-external branch that delegates to markPrFixItem with a sanitized note built from summary || error, bounded to 500 chars via sanitizeForStorage.

src/app/api/agents/[agentName]/tasks/report/route.ts — Adds waiting-external to VALID_OUTCOMES; deriveStatus now distinguishes it; the resolver forwards report.error (this is the author follow-up to the PR-thread P2 about Courier's report-first ordering preserving the explanation). The test for this path asserts both outcome: "waiting-external" and error: "missing prerequisite: X" are forwarded and the run is recorded with status: "waiting-external".

src/app/api/pr-fix-queue/mark/route.ts — Adds WAITING_EXTERNAL to WORKER_SETTLE_STATUSES so a worker can leave a WAITING_EXTERNAL mark. Adds a defense-in-depth expectedGeneration required check at the route layer so an operator mark without a generation is rejected with 400 (rather than relying on the library throw). This matches the existing parseMarkPrFixInput pattern and is reflected in two new route tests.

src/lib/pr-fix-queue.ts enqueue path — Adds WAITING_EXTERNAL to the isTerminalStatus set in enqueuePrFixItem so a fresh enqueue with new evidence does not auto-reopen the wait. This preserves the BLOCKED/PARKED/FIXED-stuck behavior.

src/lib/pr-fix-queue.ts reconcilers — reconcileStalePrFixItems and reconcileArchivedRepoPrFixItems both extended to include WAITING_EXTERNAL, so upstream-closed/archived cleanup continues to cover waits.

src/types/index.ts — PrFixStatus and VALID_PR_FIX_STATUSES include WAITING_EXTERNAL.

Docs — Three docs updates document the new outcome, status, and worker allowlist. The worker-execution-contract.md worker allowlist is updated correctly: WAITING_EXTERNAL joins FIXED/BLOCKED/PARKED/STALE with the note that it defers rather than settles.

Tests — 12 focused tests in src/lib/pr-fix-external-wait.test.ts cover: idempotent repeat at the cap (no extra history, no deadline extension), due-wait reactivation as a fresh generation, stale wait marks refused on newer generations, BLOCKED/PARKED stickiness cannot be overwritten, sanitization of a 700-char note to ≤500, blocking concurrency races (BLOCKED transition blocks the wait, a concurrent wait winner is accepted as duplicate success), reactivation refuses to overwrite a stale row, attempt count preserved through reactivation and subsequent FIXED, closed/archived reconciliation, and old-attempt reports returning skipped: stale attempt generation. The library test in src/lib/pr-fix-queue.test.ts covers the include_blocked=true listing path with latestNote. The next-task/route.test.ts integration test exercises the actual poll: idle when future, served as followup-pr with generation: issuedGeneration + 1 when due, then idle again when the retry is in flight.

Sources

• src/lib/pr-fix-queue.ts:540–590 (cooldown + reactivate helper)
• src/lib/pr-fix-queue.ts:1466–1500 (listQueuedPrFixItems)
• src/lib/pr-fix-queue.ts:1586–1680 (markPrFixItem WAITING_EXTERNAL branch)
• src/lib/pr-fix-queue.ts:2218–2232 and :2279–2296 (reconcilers)
• src/lib/pr-fix-queue.ts:2430–2540 (AgentReportOutcome + ResolvePrFixFromAgentReportInput)
• src/lib/pr-fix-queue.ts:2690–2740 (resolvePrFixFromAgentReport branches)
• src/app/api/agents/[agentName]/tasks/report/route.ts:31–80, 480–600 (route + deriveStatus + error forwarding)
• src/app/api/pr-fix-queue/mark/route.ts:30–85 (worker allowlist + generation requirement)
• src/app/automation/pr-fix-queue/page.tsx:118, 294–316 (read-only waiting panel)
• prisma/migrations/20261013000000_pr_fix_external_wait/migration.sql and prisma/schema.prisma:134, 222–223
• src/lib/pr-fix-external-wait.test.ts and route test additions
• PR thread comments from joryirving (P2 about Courier report-first note preservation; addressed by the error forwarding in tasks/report/route.ts:595)

Standards Compliance

The migration follows the project's two-line additive style (previous migrations in prisma/migrations/20260*/ like add_pr_fix_outcome and add_issue_native_blocked_by show the same pattern). The new env var PR_FIX_EXTERNAL_WAIT_COOLDOWN_MS follows the project's PR_FIX_* naming. The schema enum and union type additions match the existing PrFixStatus/VALID_PR_FIX_STATUSES pattern. AGENTS.md is not modified by the PR (no new agent API contract surface beyond an additional allowed outcome).

Linked Issue Fit

Linked issue PR 1231 lists these done-conditions:

1. "A waiting-external report is accepted (2xx), does not advance the issued generation, does not increment fixAttempts, and never writes lane:"NEEDS_HUMAN"" — satisfied by src/lib/pr-fix-queue.ts:2715–2722 (delegates to markPrFixItem which gates on existing.status !== QUEUED → generation-mismatch; the WAITING branch in markPrFixItem does not touch lane or fixAttempts). The "does not advance generation" claim is satisfied because markPrFixItem for WAITING_EXTERNAL does NOT call freshAttemptGeneration; it only updates fields on the existing row.
2. "A WAITING_EXTERNAL mark keeps the issued generation current; the item stays re-offerable once the prerequisite is satisfied" — satisfied: only reactivateDueExternalWaits bumps the generation (and only when retryAt <= now), and only inside a fenced updateMany on (id, status: WAITING_EXTERNAL, generation, retryAt).
3. "Repeated blocked-external reports across reconciles never surface a human park (regression at PR_FIX_MAX_ATTEMPTS)" — satisfied by src/lib/pr-fix-external-wait.test.ts "allows repeated external waits across the attempt cap without charging attempts".
4. "handed-off, blocked, and failed behavior is unchanged" — verified: the existing pr-fix-queue tests for these outcomes were not touched; only the WAITING_EXTERNAL branch was added.
5. "Update the Courier-side contract fixture internal/source/dispatch/contract_test.go lockstep note when landed" — Courier-side follow-up; out of scope for Dispatch PR, acknowledged in PR body.

Tool Harness Findings

The harness ran 28 tool calls across 11 rounds, all read-only. Key signals: the markPrFixItem WAITING_EXTERNAL branch is fenced with existing.status === QUEUED plus the standard (id, status, generation) transaction CAS; the reactivateDueExternalWaits helper uses an updateMany with (id, status, generation, retryAt) so a concurrent setter (e.g. operator requeue) cannot lose its CAS; the report-route now forwards error into the resolver payload per the author follow-up to the PR-thread P2.

Unknowns or Needs Verification

1. Local-suite failure in AgentWork auth/UI tests — the PR body explicitly notes this is a pre-existing failure outside this PR's surface and CI is authoritative. The 13/13 CI checks on head are green. No verification needed beyond CI.
2. The WorkerHandout token for WAITING_EXTERNAL boundary — the new test "withholds future external waits and lazily hands out due retries" asserts body.prFixItem.generation === issuedGeneration + 1 but the actual agentHandoutToken recording in production happens in confirmPrFixHandOut and uses prisma.prFixQueueItem.updateMany directly (not the clientFor mock). The test exercises the mock store and validates the contract through the route, which is the strongest verification available without a DB container. Treating as verified.
3. The freshAttemptGeneration helper contract — confirmed at src/lib/pr-fix-queue.ts:2329 to be { generation: { increment: number } } and is used atomically inside the reactivateDueExternalWaits $transaction, so concurrent reactivation races against markPrFixItem (which would update (id, status, generation) to QUEUED with no retry-pin) cannot lose either side's intent. The early-exit predicate if (where.status !== "QUEUED") return mutation-mismatch covers the inverse race.

Required Check Dispositions

• review migration for data loss risk — satisfied. Two additive statements, both idempotent (ADD VALUE IF NOT EXISTS, no DEFAULT on the new column means existing rows get NULL which is acceptable).
• test migration on a copy of production schema — not_applicable. The migration is additive on a non-production-instance point in time; the project's CI runs Database migrations and Database integration checks against this head and they pass.
• treat as high priority — verify correctness carefully — satisfied. Every claim in the PR body was falsified against the diff; the invariants around generation fencing, attempt charging, idle-vs-rediscovery handling, and reconciliation all hold in both code and tests.
• review auth flow for regression — satisfied. WORKER_SETTLE_STATUSES correctly extended; expectedGeneration requirement for WAITING_EXTERNAL is enforced at both the route layer and inside markPrFixItem; parseMarkPrFixInput was not modified, and the enforceRateLimit/auditLog paths still run for the new status.
• verify session token handling is correct — satisfied. No token-handling code paths were touched. The new column and outcome do not influence auth.