fix(pr-fix): separate human decisions from retry exhaustion (#1218)

docs/pr-fix-outcome-policy.md

@@ -0,0 +1,30 @@
+# PR-fix outcome policy
+
+## Purpose
+
+`needs-human` must mean an actual unresolved design, scope, authorization, or conflicting-requirements decision that automation cannot safely make. Retry exhaustion is not a human decision; it is recorded separately so bounded automation failures do not create false human escalations.
+
+## Outcomes
+
+| Outcome | Meaning | Status | Lane | Surfacing |
+|---------|---------|--------|------|-----------|
+| `human_decision` | A genuine unresolved decision requiring a person | `BLOCKED` | `NEEDS_HUMAN` | `needs-human` label and `<!-- dispatch-pr-fix-blocked -->` comment rendering the question, options, and evidence |
+| `no_progress` | Bounded retry or no-progress exhaustion | `PARKED` | `NORMAL` | Diagnostic `<!-- dispatch-pr-fix-parked -->` comment; no `needs-human` label |
+| `infra_failure` | Infrastructure or authentication failure | `PARKED` | `NORMAL` | Diagnostic parked comment; fail closed, not blindly retried |
+| `security_failure` | Security failure | `PARKED` | `NORMAL` | Diagnostic parked comment; fail closed, not blindly retried |
+
+The structured outcome is stored on `PrFixQueueItem` and `PrFixHistory`; human decisions may also include `decisionDetail`. A `tasks/report` request may provide `blockReason` and `decision` (`question`, `options`, `evidence`). A bare `outcome: "blocked"` without `blockReason` means `no_progress`, not `human_decision`. An `outcome` supplied on a brand-new `NORMAL`-lane enqueue is advisory and only takes effect if/when the item parks (a fresh item starts `QUEUED`).
+
+## Budgets and recovery
+
+`fixAttempts` counts dispatchable worker executions. Hand-out reclamation (`handoutReclaims`) and `PR_FIX_HANDOUT_TIMEOUT_MS` wall time are separate budgets: a stale hand-out that never reached a worker does not consume an execution attempt. Stale or already-known feedback also consumes no execution budget. No new numeric thresholds are introduced.
+
+Automatic enqueue/evidence does not release `BLOCKED` or `PARKED`. An operator explicitly requeues either status, or a maintainer marks the item back to `QUEUED`; requeue releases the parked/blocked state. `FIXED` retains its existing #940 and new-evidence reopen paths.
+
+## Safety and reporting
+
+Automatic give-up caps (including attempt, reclaim, refused-`FIXED`, post-dispatch reopen, and failed-report caps) park as `PARKED`/`no_progress`. Infrastructure/authentication and security failures also park, never becoming human design work or being blindly retried. Writes are generation-guarded: a late or stale worker generation cannot overwrite a newer outcome.
+
+Surfaced metrics distinguish `workerExecutions` (`fixAttempts`) from `enqueueRows` (enqueue history-row count). Enqueue, feedback, and sync/history rows are not worker executions.
+
+This policy does not claim the exact freshness root cause of PR #1021. Courier #263, which concerns skipping already-consumed pending heads during discovery/deduplication, is a separate scope and is not implemented by this outcome model.

docs/pr-review-fix-queue.md

@@ -11,7 +11,8 @@ Each item is deduped by `(repo, pr)` and stores:
 - `repo`, `pr`, `branch`, `url`, `title`
 - optional source `issue` (extracted from PR title/body)
 - `lane`: `NORMAL`, `ESCALATED`, or `NEEDS_HUMAN`
-- `status`: `QUEUED`, `FIXED`, `BLOCKED`, `STALE`, or `IGNORED`
+- `status`: `QUEUED`, `FIXED`, `BLOCKED`, `PARKED`, `STALE`, or `IGNORED`
+- `outcome`: `human_decision`, `no_progress`, `infra_failure`, or `security_failure`; human decisions may also store `decisionDetail`
 - `reason`, `feedback[]`, `evidenceKeys[]`
 - `headSha` and `author` metadata
 - `generation` — Dispatch-owned work generation (see below)
@@ -63,7 +64,8 @@ signature-verification model (see the `WEBHOOK_SECRET` row in the README).
 Incoming feedback is classified as **actionable** or **needs_human**:
 
 - **Actionable** → `NORMAL` lane: specific error messages, test failures, code-level fixes, lint complaints
-- **Needs human** → `NEEDS_HUMAN` lane: vague requests, missing context, security-sensitive changes without specifics
+- **Human decision** → `NEEDS_HUMAN` lane: an unresolved design, scope, authorization, or conflicting-requirements decision that automation cannot safely make
+- **Insufficient information or operational failure** → do not classify as a human decision solely for being vague or security-sensitive; bounded no-progress, infrastructure/auth, and security failures park as `PARKED` in the `NORMAL` lane
 
 Items in the `NEEDS_HUMAN` lane receive `BLOCKED` status and are excluded from the normal agent queue unless `include_blocked=true`.
 
@@ -91,7 +93,7 @@ Items in the `NEEDS_HUMAN` lane receive `BLOCKED` status and are excluded from t
 
 ### Mark item status
 
-`POST /api/pr-fix-queue/mark` — Marks an item `FIXED`, `BLOCKED`, `STALE`, or `IGNORED` and records a history event. Requires auth.
+`POST /api/pr-fix-queue/mark` — Marks an item `FIXED`, `BLOCKED`, `PARKED`, `STALE`, or `IGNORED` and records a history event. Requires auth. The body may include an `outcome` (`human_decision`, `no_progress`, `infra_failure`, or `security_failure`) and a `decision` object (`question`, `options`, and `evidence`) when recording a human decision.
 
 **Request body:**
 ```json
@@ -106,6 +108,8 @@ Items in the `NEEDS_HUMAN` lane receive `BLOCKED` status and are excluded from t
 
 `generation` is **required for bearer (agent/bridge) marks** — a bearer request without it is rejected with `400` (`generation is required for agent/bridge marks (#1074)`) — and optional for operator paths (OIDC session, basic auth, disabled mode), which keep the unconditional behavior for compatibility. When supplied, the write is generation-conditional: if the item's current generation no longer matches, the request is rejected with `409` and nothing is mutated (the caller's read predates a re-issued attempt). A `FIXED` mark additionally re-reads the item's per-attempt `attemptHeadSha` baseline before writing; if the PR head has not moved since the attempt was recorded, the item is refused `FIXED` (back to `QUEUED`, or `BLOCKED` once the attempt cap is spent) — and the refusal never transiently stores `FIXED`. Every fresh attempt (enqueue reopen, requeue, mark back to `QUEUED`, refusal) records its baseline from the newest head Dispatch has observed, so a sync that sees the worker's push before its report cannot turn a real fix into a refusal. A mark for an unknown item returns `404`.
 
+`POST /api/agents/{agentName}/tasks/report` accepts optional `blockReason` and `decision` fields for a blocked follow-up report. `blockReason` is one of `human_decision`, `no_progress`, `infra_failure`, or `security_failure`; `decision` may include `question`, `options`, and `evidence`. A bare `outcome: "blocked"` without `blockReason` is classified as `no_progress` and parks the item rather than declaring a human decision.
+
 ## Assignment queue behavior
 
 `GET /api/agents/:agentName/queue` prepends queued PR review-fix items before ranked issue work. This preserves the worker contract: review-fix work is consumed before selecting new board work.
@@ -225,16 +229,21 @@ until a health reconcile observes the PR closed.
 
 ```
 QUEUED → FIXED (completed)
-QUEUED → BLOCKED (needs human review)
+QUEUED → BLOCKED (genuine human decision)
+QUEUED → PARKED (bounded exhaustion, infrastructure/auth failure, or security failure)
 QUEUED → STALE (no longer relevant)
 QUEUED → IGNORED (deliberately skipped)
 ```
 
-`NEEDS_HUMAN` lane items start with `BLOCKED` status and require explicit marking to change state.
+`BLOCKED` in the `NEEDS_HUMAN` lane is reserved for genuine unresolved design, scope, authorization, or conflicting-requirements decisions. It is surfaced with the `needs-human` label and a `<!-- dispatch-pr-fix-blocked -->` marker comment that renders the decision question, options, and evidence. Bounded retry/no-progress exhaustion, infrastructure/auth failures, and security failures instead become `PARKED` in the `NORMAL` lane, with a diagnostic `<!-- dispatch-pr-fix-parked -->` comment and no `needs-human` label. Both outcomes remain sticky to automatic enqueue/evidence; explicit operator requeue (or a maintainer mark to `QUEUED`) releases them.
 
 ### Attempt cap
 
-`PR_FIX_MAX_ATTEMPTS` (default 5) bounds dispatched fix attempts per item, tracked as `fixAttempts`. An item starts at attempt 1; each return to `QUEUED` (new evidence after a fix, the #940 no-progress reopen, a mark back to `QUEUED`, a refused no-push `FIXED`) is another attempt. Once the count reaches the cap, the next return goes to `BLOCKED` in the `NEEDS_HUMAN` lane instead. Evidence that lands while the item is already `QUEUED`, such as the other inline comments of the same review, is the same attempt and never counts. `POST /api/pr-fix-queue/requeue` resets the count to 1.
+`PR_FIX_MAX_ATTEMPTS` (default 5) bounds dispatched fix attempts per item, tracked as `fixAttempts`. An item starts at attempt 1; each return to `QUEUED` (new evidence after a fix, the #940 no-progress reopen, a mark back to `QUEUED`, a refused no-push `FIXED`) is another attempt. Hitting the cap parks the item as `PARKED` with outcome `no_progress`; it does not add the `needs-human` label. Evidence that lands while the item is already `QUEUED`, such as the other inline comments of the same review, is the same attempt and never counts. `POST /api/pr-fix-queue/requeue` resets the count to 1.
+
+## Outcome classification
+
+`BLOCKED` is for genuine human decisions; `PARKED` is for bounded exhaustion and fail-closed operational/security failures. See [PR-fix outcome policy](pr-fix-outcome-policy.md) for the classification, surfacing, budgets, and operator recovery contract.
 
 ## Hand-out acknowledgement and reclamation (#1211)
 
@@ -297,9 +306,9 @@ as the `pr-fix-sweep` job:
 - A **merged or closed** PR goes `STALE` (nothing left to fix).
 - **Unknown PR state** defers the item (`unknownState`): a possibly-closed PR is
   never reclaimed into a fresh attempt.
-- Past `PR_FIX_MAX_RECLAIMS` (default 3) the item goes `BLOCKED` in the
-  `NEEDS_HUMAN` lane instead of being reclaimed again. Operator requeue
-  resets the recovery budget.
+- Past `PR_FIX_MAX_RECLAIMS` (default 3) the item parks as `PARKED` with
+  outcome `no_progress`; it does not become `BLOCKED` or enter the
+  `NEEDS_HUMAN` lane. Operator requeue resets the recovery budget.
 
 ### Migration handling for pre-existing stamped rows
 

prisma/migrations/20261009000000_add_pr_fix_handout_acks/migration.sql

@@ -11,9 +11,11 @@
 -- attempt clears the list.
 --
 -- `handoutReclaims` counts bounded automatic reclamations of stale
--- unacknowledged hand-outs. Past PR_FIX_MAX_RECLAIMS the item goes to BLOCKED
--- in the NEEDS_HUMAN lane instead of being reclaimed again. No backfill: a row
--- that was never acknowledged simply starts empty, and an already-handled row
--- starts at zero.
+-- unacknowledged hand-outs. Past PR_FIX_MAX_RECLAIMS the item parks as PARKED
+-- in the NORMAL lane with outcome=`no_progress` (#1218) — bounded
+-- no-progress exhaustion, not a human decision, so the `needs-human` label is
+-- never applied. Release requires an explicit operator requeue or a
+-- maintainer mark to QUEUED. No backfill: a row that was never acknowledged
+-- simply starts empty, and an already-handled row starts at zero.
 ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "handoutAcks" TEXT[] NOT NULL DEFAULT ARRAY[]::TEXT[];
 ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "handoutReclaims" INTEGER NOT NULL DEFAULT 0;

prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql

@@ -0,0 +1,9 @@
+-- The new columns are TEXT/JSONB, not the "PrFixStatus"/enum type, so the
+-- ALTER TYPE ... ADD VALUE below is never referenced by a column cast inside
+-- this migration transaction. Postgres forbids using a newly added enum value
+-- in the same transaction that added it; keeping these columns as plain
+-- TEXT/JSONB avoids that constraint entirely.
+ALTER TYPE "PrFixStatus" ADD VALUE IF NOT EXISTS 'PARKED';
+ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "outcome" TEXT;
+ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "decisionDetail" JSONB;
+ALTER TABLE "PrFixHistory" ADD COLUMN IF NOT EXISTS "outcome" TEXT;

prisma/schema.prisma

@@ -131,6 +131,7 @@ enum PrFixStatus {
   QUEUED
   FIXED
   BLOCKED
+  PARKED
   STALE
   IGNORED
 }
@@ -155,6 +156,13 @@ model PrFixQueueItem {
   feedback     String[]       @default([])
   evidenceKeys String[]       @default([])
   status       PrFixStatus    @default(QUEUED)
+  // Structured outcome classification (#1218): why the item is in its current
+  // non-active terminal-ish state. Only meaningful for BLOCKED (human_decision)
+  // and PARKED (no_progress | infra_failure | security_failure).
+  outcome      String?
+  // Human-decision payload for a BLOCKED item (#1218): the question, options,
+  // and evidence an operator needs to make the call. Null for non-human states.
+  decisionDetail Json?
   // Dispatch-owned work generation (#1044): 1 on creation, bumped when a
   // non-QUEUED item becomes dispatchable again as a fresh attempt. Workers
   // consume (id, generation) as an opaque work identity via next-task.
@@ -206,7 +214,10 @@ model PrFixQueueItem {
   // Bounded automatic reclamations of stale unacknowledged hand-outs (#1211):
   // counts how many times the reclaimer has reopened this item because a
   // stamped hand-out was never acknowledged. Past PR_FIX_MAX_RECLAIMS the item
-  // goes to BLOCKED in the NEEDS_HUMAN lane instead of being reclaimed again.
+  // parks as PARKED in the NORMAL lane with outcome=`no_progress` (#1218) —
+  // bounded no-progress exhaustion, never a human decision, so the
+  // `needs-human` label is never applied. Release requires an explicit
+  // operator requeue or a maintainer mark to QUEUED.
   handoutReclaims Int @default(0)
   author       String?
   history      PrFixHistory[]
@@ -231,6 +242,7 @@ model PrFixHistory {
   reason      String?        @db.Text
   note        String?        @db.Text
   evidenceKey String?
+  outcome     String?
 
   @@index([itemId])
   @@index([at])

src/app/api/agents/[agentName]/tasks/report/route.test.ts

@@ -479,6 +479,111 @@ describe("POST /api/agents/[agentName]/tasks/report — validation", () => {
   });
 });
 
+describe("POST /api/agents/[agentName]/tasks/report — structured block outcome (#1218)", () => {
+  beforeEach(() => {
+    delete process.env.DISPATCH_AUTH_MODE;
+    resetAuthCaches();
+    vi.clearAllMocks();
+  });
+
+  const blockedReport = {
+    taskType: "followup-pr",
+    outcome: "blocked",
+    repoFullName: "o/r",
+    pullRequestNumber: 5,
+    prFixItem: { id: "item-1", generation: 1 },
+  };
+
+  it("passes a human_decision blockReason and decision payload through to the resolver", async () => {
+    const decision = { question: "Which behavior?", options: ["keep", "change"], evidence: "see thread" };
+    const res = await postRequest({
+      ...blockedReport,
+      blockReason: "human_decision",
+      decision,
+    });
+
+    expect(res.status).toBe(200);
+    const body = await res.json();
+    expect(body.report.blockReason).toBe("human_decision");
+    expect(body.report.decision).toEqual(decision);
+    expect(prFixResolveMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        outcome: "blocked",
+        blockReason: "human_decision",
+        decision,
+      }),
+    );
+  });
+
+  it("accepts a blocked report without a blockReason (bare give-up → parked)", async () => {
+    const res = await postRequest({ ...blockedReport });
+
+    expect(res.status).toBe(200);
+    expect(prFixResolveMock).toHaveBeenCalledWith(
+      expect.objectContaining({ outcome: "blocked", blockReason: undefined }),
+    );
+  });
+
+  it("returns 400 for an invalid blockReason", async () => {
+    const res = await postRequest({ ...blockedReport, blockReason: "not-a-real-outcome" });
+
+    expect(res.status).toBe(400);
+    expect((await res.json()).error).toMatch(/blockReason/);
+    expect(mockAgentRun.create).not.toHaveBeenCalled();
+    expect(prFixResolveMock).not.toHaveBeenCalled();
+  });
+
+  it("returns 400 when blockReason is not a string", async () => {
+    const res = await postRequest({ ...blockedReport, blockReason: 42 });
+
+    expect(res.status).toBe(400);
+    expect((await res.json()).error).toMatch(/blockReason/);
+  });
+
+  it("returns 400 when decision is not a non-array object", async () => {
+    const asString = await postRequest({ ...blockedReport, decision: "nope" });
+    expect(asString.status).toBe(400);
+    expect((await asString.json()).error).toMatch(/decision must be an object/);
+
+    const asArray = await postRequest({ ...blockedReport, decision: ["a", "b"] });
+    expect(asArray.status).toBe(400);
+    expect((await asArray.json()).error).toMatch(/decision must be an object/);
+  });
+
+  it("returns 400 when decision fields are malformed", async () => {
+    const badQuestion = await postRequest({ ...blockedReport, decision: { question: 123 } });
+    expect(badQuestion.status).toBe(400);
+    expect((await badQuestion.json()).error).toMatch(/decision\.question/);
+
+    const badOptions = await postRequest({ ...blockedReport, decision: { options: "not-an-array" } });
+    expect(badOptions.status).toBe(400);
+    expect((await badOptions.json()).error).toMatch(/decision\.options/);
+
+    const nonStringOptions = await postRequest({ ...blockedReport, decision: { options: ["ok", 5] } });
+    expect(nonStringOptions.status).toBe(400);
+    expect((await nonStringOptions.json()).error).toMatch(/decision\.options/);
+
+    const badEvidence = await postRequest({ ...blockedReport, decision: { evidence: 5 } });
+    expect(badEvidence.status).toBe(400);
+    expect((await badEvidence.json()).error).toMatch(/decision\.evidence/);
+  });
+
+  it("normalizes a valid decision payload (trims strings, drops empty options)", async () => {
+    const res = await postRequest({
+      ...blockedReport,
+      blockReason: "human_decision",
+      decision: { question: "  Which?  ", options: ["  A  ", "", "B"], evidence: "  ctx  " },
+    });
+
+    expect(res.status).toBe(200);
+    expect(prFixResolveMock).toHaveBeenCalledWith(
+      expect.objectContaining({
+        decision: { question: "Which?", options: ["A", "B"], evidence: "ctx" },
+      }),
+    );
+  });
+});
+
 describe("POST /api/agents/[agentName]/tasks/report — AgentRun persistence", () => {
   beforeEach(() => {
     delete process.env.DISPATCH_AUTH_MODE;