From 5ceb676d3dc98311aabe714b0ba6f489cba0ce4f Mon Sep 17 00:00:00 2001 From: Courier Date: Sun, 11 Oct 2026 13:43:25 +0000 Subject: [PATCH 1/7] fix(pr-fix): separate human decisions from retry exhaustion (#1218) Introduce a structured outcome model for the PR-fix queue so needs-human means an actual unresolved design/scope/authorization/conflict decision, not bounded retry exhaustion. - Add PARKED status and PrFixOutcome (human_decision | no_progress | infra_failure | security_failure); persist outcome on PrFixQueueItem and PrFixHistory, plus decisionDetail for human decisions. - BLOCKED is now reserved for human_decision (NEEDS_HUMAN lane, needs-human label, marker comment rendering question/options/evidence). Every automatic cap/give-up (attempt, reclaim, refused-FIXED, post-dispatch reopen, failed report) parks as PARKED/no_progress with a diagnostic comment and no needs-human label. - Infra/auth and security failures park and fail closed: never blindly retried, never mislabeled as human design work. - BLOCKED and PARKED are sticky to automatic enqueue/evidence; only explicit operator requeue or a maintainer mark to QUEUED releases them. FIXED keeps its #940 and new-evidence reopen paths. All writes stay generation-pinned. - tasks/report accepts blockReason/decision; a bare blocked report is no_progress (parked), not a human decision. - Truthful metrics: surface workerExecutions (fixAttempts) and enqueueRows separately instead of conflating enqueue rows with worker attempts. - Docs: new pr-fix-outcome-policy.md and updated pr-review-fix-queue.md. --- docs/pr-fix-outcome-policy.md | 30 ++ docs/pr-review-fix-queue.md | 27 +- .../migration.sql | 4 + prisma/schema.prisma | 9 + .../[agentName]/tasks/report/route.test.ts | 105 ++++ .../agents/[agentName]/tasks/report/route.ts | 59 +++ .../[agentName]/work-summary/route.test.ts | 30 +- .../agents/[agentName]/work-summary/route.ts | 6 +- src/app/api/pr-fix-queue/mark/route.ts | 14 +- src/app/api/pr-fix-queue/queued/route.test.ts | 13 + src/app/api/pr-fix-queue/queued/route.ts | 2 + src/lib/pr-fix-queue-report.test.ts | 58 ++- src/lib/pr-fix-queue.test.ts | 464 +++++++++++++++--- src/lib/pr-fix-queue.ts | 462 +++++++++++++---- src/lib/pr-fix-surfacing.test.ts | 149 +++++- src/lib/pr-fix-surfacing.ts | 214 +++++++- src/lib/pr-followup-ingestion.test.ts | 23 + src/lib/pr-followup-ingestion.ts | 2 + src/mcp/server.ts | 2 +- src/types/index.ts | 36 +- 20 files changed, 1490 insertions(+), 219 deletions(-) create mode 100644 docs/pr-fix-outcome-policy.md create mode 100644 prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql diff --git a/docs/pr-fix-outcome-policy.md b/docs/pr-fix-outcome-policy.md new file mode 100644 index 00000000..a0a28c75 --- /dev/null +++ b/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 `` comment rendering the question, options, and evidence | +| `no_progress` | Bounded retry or no-progress exhaustion | `PARKED` | `NORMAL` | Diagnostic `` 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`. + +## 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. diff --git a/docs/pr-review-fix-queue.md b/docs/pr-review-fix-queue.md index 697e2e7d..95bf82a9 100644 --- a/docs/pr-review-fix-queue.md +++ b/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 `` 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 `` 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 diff --git a/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql new file mode 100644 index 00000000..da8c7332 --- /dev/null +++ b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql @@ -0,0 +1,4 @@ +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; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index ddfd14be..481382b9 100644 --- a/prisma/schema.prisma +++ b/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. @@ -231,6 +239,7 @@ model PrFixHistory { reason String? @db.Text note String? @db.Text evidenceKey String? + outcome String? @@index([itemId]) @@index([at]) diff --git a/src/app/api/agents/[agentName]/tasks/report/route.test.ts b/src/app/api/agents/[agentName]/tasks/report/route.test.ts index a3acca9e..75b5b530 100644 --- a/src/app/api/agents/[agentName]/tasks/report/route.test.ts +++ b/src/app/api/agents/[agentName]/tasks/report/route.test.ts @@ -458,6 +458,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; diff --git a/src/app/api/agents/[agentName]/tasks/report/route.ts b/src/app/api/agents/[agentName]/tasks/report/route.ts index 674efc69..bc1db007 100644 --- a/src/app/api/agents/[agentName]/tasks/report/route.ts +++ b/src/app/api/agents/[agentName]/tasks/report/route.ts @@ -6,6 +6,7 @@ import { prisma } from "@/lib/prisma"; import { authorizeRequest, authErrorResponse } from "@/lib/auth"; import { enforceWorkerAgentScope } from "@/lib/worker-identity"; import { resolvePrFixFromAgentReport, MAX_EVIDENCE_LENGTH, type ResolvePrFixFromAgentReportResult } from "@/lib/pr-fix-queue"; +import { isValidPrFixOutcome, VALID_PR_FIX_OUTCOMES, type PrFixDecisionContext } from "@/types"; const VALID_TASK_TYPES = ["implement", "followup-pr"] as const; type ValidTaskType = (typeof VALID_TASK_TYPES)[number]; @@ -33,6 +34,12 @@ export interface TaskReportBody { error?: string; // #1121: evidence string (commit SHAs / paths) carried by an already_addressed report; recorded in settlement history. evidence?: string; + // #1218: for a `blocked` report, the structured reason. `human_decision` + // marks BLOCKED with the decision payload; anything else (or a bare give-up) + // marks PARKED. Validated against PrFixOutcome. + blockReason?: string; + // #1218: the human-decision payload attached to a `human_decision` block. + decision?: PrFixDecisionContext; // #1120: worker-reported task start time (ISO 8601). Accepted only when // parseable, not in the future, and within 24h of the report; otherwise // omitted and the report time is used. It participates in the idempotency @@ -192,6 +199,50 @@ export async function POST( } } + // #1218: a `blocked` report may carry a structured reason and a + // human-decision payload. Validate both here so a malformed value is a 400 + // rather than a silently-ignored field. They are only meaningful for a + // `blocked` outcome; the resolver ignores them otherwise. + let blockReason: string | undefined; + if (raw.blockReason !== undefined) { + if (typeof raw.blockReason !== "string" || !isValidPrFixOutcome(raw.blockReason.trim())) { + return errorResponse( + `blockReason must be one of: ${VALID_PR_FIX_OUTCOMES.join(", ")}`, + 400, + ); + } + blockReason = raw.blockReason.trim(); + } + + let decision: PrFixDecisionContext | undefined; + if (raw.decision !== undefined && raw.decision !== null) { + if (typeof raw.decision !== "object" || Array.isArray(raw.decision)) { + return errorResponse("decision must be an object", 400); + } + const rawDecision = raw.decision as Record; + const parsedDecision: PrFixDecisionContext = {}; + if (rawDecision.question !== undefined && rawDecision.question !== null) { + if (typeof rawDecision.question !== "string") { + return errorResponse("decision.question must be a string", 400); + } + if (rawDecision.question.trim()) parsedDecision.question = rawDecision.question.trim(); + } + if (rawDecision.options !== undefined && rawDecision.options !== null) { + if (!Array.isArray(rawDecision.options) || rawDecision.options.some((o) => typeof o !== "string")) { + return errorResponse("decision.options must be an array of strings", 400); + } + const options = (rawDecision.options as string[]).map((o) => o.trim()).filter(Boolean); + if (options.length > 0) parsedDecision.options = options; + } + if (rawDecision.evidence !== undefined && rawDecision.evidence !== null) { + if (typeof rawDecision.evidence !== "string") { + return errorResponse("decision.evidence must be a string", 400); + } + if (rawDecision.evidence.trim()) parsedDecision.evidence = rawDecision.evidence.trim(); + } + decision = parsedDecision; + } + // #1121: evidence is only meaningful for an already_addressed settlement — it // is trimmed, bounded, and recorded in the PR-fix settlement history. On any // other outcome it is ignored (never hashed, never persisted). A non-string @@ -258,6 +309,10 @@ export async function POST( summary: raw.summary as string | undefined, error: raw.error as string | undefined, evidence, + // #1218: structured block classification; participates in the idempotency + // payload hash like the other report fields. + blockReason, + decision, // #1120: normalized worker start time; undefined → report time (now). startedAt: resolveReportedStartedAt(raw.startedAt, now), prFixItem, @@ -358,6 +413,10 @@ export async function POST( outcome: report.outcome, summary: report.summary, evidence: report.evidence, + // #1218: a `blocked` report's structured reason / decision payload; + // the resolver ignores both for any other outcome. + blockReason: report.blockReason, + decision: report.decision, // #1074: settlement is authorized by the attempt token the worker was // issued; a report without one never mutates the queue. attempt: prFixItem ? { itemId: prFixItem.id, generation: prFixItem.generation } : null, diff --git a/src/app/api/agents/[agentName]/work-summary/route.test.ts b/src/app/api/agents/[agentName]/work-summary/route.test.ts index 36f71a8d..9e5f4fdc 100644 --- a/src/app/api/agents/[agentName]/work-summary/route.test.ts +++ b/src/app/api/agents/[agentName]/work-summary/route.test.ts @@ -121,9 +121,9 @@ describe("GET /api/agents/[agentName]/work-summary", () => { backlog: { queued: 0, inProgress: 0 }, }); expect(body.prFixes).toEqual({ - local: { total: 0, blocked: 0 }, - frontier: { total: 0, blocked: 0 }, - needsHuman: { total: 0, blocked: 0 }, + local: { total: 0, blocked: 0, parked: 0 }, + frontier: { total: 0, blocked: 0, parked: 0 }, + needsHuman: { total: 0, blocked: 0, parked: 0 }, }); }); @@ -181,9 +181,27 @@ describe("GET /api/agents/[agentName]/work-summary", () => { expect(res.status).toBe(200); const body = await res.json(); - expect(body.prFixes.local).toEqual({ total: 3, blocked: 1 }); - expect(body.prFixes.frontier).toEqual({ total: 1, blocked: 0 }); - expect(body.prFixes.needsHuman).toEqual({ total: 2, blocked: 2 }); + expect(body.prFixes.local).toEqual({ total: 3, blocked: 1, parked: 0 }); + expect(body.prFixes.frontier).toEqual({ total: 1, blocked: 0, parked: 0 }); + expect(body.prFixes.needsHuman).toEqual({ total: 2, blocked: 2, parked: 0 }); + }); + + it("counts PARKED items under parked, not blocked (#1218)", async () => { + mocks.prFixFindMany.mockResolvedValue([ + { lane: "NORMAL", status: "PARKED" }, + { lane: "NORMAL", status: "PARKED" }, + { lane: "NORMAL", status: "BLOCKED" }, + { lane: "NORMAL", status: "QUEUED" }, + ]); + + const res = await makeRequest( + `http://localhost/api/agents/${TEST_AGENT}/work-summary`, + ); + + expect(res.status).toBe(200); + const body = await res.json(); + // Total counts every surfaced item; parked and blocked are disjoint. + expect(body.prFixes.local).toEqual({ total: 4, blocked: 1, parked: 2 }); }); it("defaults missing lane to NORMAL for PR fixes", async () => { diff --git a/src/app/api/agents/[agentName]/work-summary/route.ts b/src/app/api/agents/[agentName]/work-summary/route.ts index ca38bc6a..265b2571 100644 --- a/src/app/api/agents/[agentName]/work-summary/route.ts +++ b/src/app/api/agents/[agentName]/work-summary/route.ts @@ -8,7 +8,7 @@ import { getConfiguredLanes, getDefaultClaimableLane, resolveLaneId } from "@/li import { applyRenovateIssueExclusion } from "@/lib/issue-filters"; type WorkSummaryLaneCounts = { queued: number; inProgress: number }; -type PrFixLaneCounts = { total: number; blocked: number }; +type PrFixLaneCounts = { total: number; blocked: number; parked: number }; interface WorkSummaryResponse { agentName: string; @@ -97,7 +97,7 @@ export async function GET(request: Request, { params }: { params: Promise<{ agen const prFixLaneKeys: Record = { NORMAL: "local", ESCALATED: "frontier", NEEDS_HUMAN: "needsHuman" }; const prFixCounts: Record = {}; for (const key of Object.values(prFixLaneKeys)) { - prFixCounts[key] = { total: 0, blocked: 0 }; + prFixCounts[key] = { total: 0, blocked: 0, parked: 0 }; } for (const item of prFixItems) { @@ -108,6 +108,8 @@ export async function GET(request: Request, { params }: { params: Promise<{ agen prFixCounts[laneKey].total++; if (item.status === "BLOCKED") { prFixCounts[laneKey].blocked++; + } else if (item.status === "PARKED") { + prFixCounts[laneKey].parked++; } } diff --git a/src/app/api/pr-fix-queue/mark/route.ts b/src/app/api/pr-fix-queue/mark/route.ts index b6ca81a4..c31479de 100644 --- a/src/app/api/pr-fix-queue/mark/route.ts +++ b/src/app/api/pr-fix-queue/mark/route.ts @@ -30,10 +30,16 @@ export async function POST(request: Request) { const input = parseMarkPrFixInput(body); if ("error" in input) return errorResponse(input.error, 400); - // Worker tokens may only settle attempts (FIXED/BLOCKED/STALE): moving - // an item back to QUEUED or IGNORED changes routing and requires a - // maintainer token (#1111). - if ((input.status === "QUEUED" || input.status === "IGNORED") && auth.type === "bearer" && auth.tier === "worker") { + // Worker tokens may only settle attempts (FIXED/BLOCKED/PARKED/STALE): + // PARKED lets a worker report bounded exhaustion or an operational + // failure (#1218). Moving an item back to QUEUED or IGNORED changes + // routing and requires a maintainer token (#1111). + const WORKER_SETTLE_STATUSES = ["FIXED", "BLOCKED", "PARKED", "STALE"]; + if ( + auth.type === "bearer" && + auth.tier === "worker" && + !WORKER_SETTLE_STATUSES.includes(input.status) + ) { try { await prisma.auditLog.create({ data: { diff --git a/src/app/api/pr-fix-queue/queued/route.test.ts b/src/app/api/pr-fix-queue/queued/route.test.ts index 7bc6ea77..ba62401a 100644 --- a/src/app/api/pr-fix-queue/queued/route.test.ts +++ b/src/app/api/pr-fix-queue/queued/route.test.ts @@ -79,6 +79,19 @@ describe("GET /api/pr-fix-queue/queued", () => { expect(call.includeBlocked).toBe(true); }); + it("returns PARKED items when include_blocked=true (#1218)", async () => { + mocks.listQueuedPrFixItems.mockResolvedValue([ + { id: "fix-1", repo: "org/repo", pr: 42, status: "PARKED" }, + ]); + + const res = await GET(request("http://localhost/api/pr-fix-queue/queued?include_blocked=true")); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body[0]).toMatchObject({ id: "fix-1", status: "PARKED" }); + expect(mocks.listQueuedPrFixItems.mock.calls[0][1].includeBlocked).toBe(true); + }); + it("returns 400 for invalid lane", async () => { const res = await GET(request("http://localhost/api/pr-fix-queue/queued?lane=invalid-lane")); diff --git a/src/app/api/pr-fix-queue/queued/route.ts b/src/app/api/pr-fix-queue/queued/route.ts index 2a32585f..5b82b050 100644 --- a/src/app/api/pr-fix-queue/queued/route.ts +++ b/src/app/api/pr-fix-queue/queued/route.ts @@ -14,6 +14,8 @@ export async function GET(request: Request) { try { const { searchParams } = new URL(request.url); const lane = searchParams.get("lane"); + // include_blocked surfaces BLOCKED (needs-human) and PARKED (exhausted / + // operational failure) items alongside QUEUED ones (#1218). const includeBlocked = searchParams.get("include_blocked") === "true"; const prioritizeByType = searchParams.get("prioritize_by_type") !== "false"; // default true diff --git a/src/lib/pr-fix-queue-report.test.ts b/src/lib/pr-fix-queue-report.test.ts index e91e716c..1901ec7b 100644 --- a/src/lib/pr-fix-queue-report.test.ts +++ b/src/lib/pr-fix-queue-report.test.ts @@ -37,6 +37,7 @@ vi.mock("./github-prs", () => ({ // feed wiring — there is no longer anything to mock from this surface. vi.mock("./pr-fix-surfacing", () => ({ surfacePrFixBlocked: vi.fn(async () => null), + surfacePrFixParked: vi.fn(async () => null), surfacePrFixRequeued: vi.fn(async () => null), surfacePrFixUnblocked: vi.fn(async () => null), extractUrlsFromText: vi.fn(() => []), @@ -230,18 +231,51 @@ describe("resolvePrFixFromAgentReport", () => { expect(prisma.prFixQueueItem.update).not.toHaveBeenCalled(); }); - it("marks the item BLOCKED immediately on a `blocked` outcome (no PR check)", async () => { + it("parks the item immediately on a bare `blocked` outcome (no PR check)", async () => { (prisma.prFixQueueItem.findUnique as ReturnType).mockResolvedValue({ ...FIXTURE_ITEM }); const result = await resolvePrFixFromAgentReport(baseInput({ outcome: "blocked", attempt: ATTEMPT, })); - expect(result.action).toBe("blocked"); + // #1218: a bare give-up is bounded no-progress exhaustion, not a human + // decision — it is PARKED, never labeled needs-human. + expect(result.action).toBe("parked"); + expect(result.reason).toContain("parked"); // No need to hit GitHub for a hard block — the agent hit a wall. expect(fetchPullRequestMergeStateMock).not.toHaveBeenCalled(); // #1074: generation-conditional write. - expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalled(); + expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ status: "PARKED", lane: "NORMAL", outcome: "no_progress" }), + }), + ); + }); + + it("marks the item BLOCKED when a `blocked` outcome carries a human_decision reason", async () => { + (prisma.prFixQueueItem.findUnique as ReturnType).mockResolvedValue({ ...FIXTURE_ITEM }); + (prisma.prFixQueueItem.updateMany as ReturnType).mockResolvedValue({ count: 1 }); + + const decision = { question: "Which behavior is intended?", options: ["A", "B"], evidence: "see thread" }; + const result = await resolvePrFixFromAgentReport(baseInput({ + outcome: "blocked", + attempt: ATTEMPT, + blockReason: "human_decision", + decision, + })); + expect(result.action).toBe("blocked"); + // No need to hit GitHub for a hard block — the agent hit a wall. + expect(fetchPullRequestMergeStateMock).not.toHaveBeenCalled(); + expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + data: expect.objectContaining({ + status: "BLOCKED", + lane: "NEEDS_HUMAN", + outcome: "human_decision", + decisionDetail: decision, + }), + }), + ); }); it("reopens a fresh attempt on a `failed` outcome (#1133)", async () => { @@ -275,25 +309,27 @@ describe("resolvePrFixFromAgentReport", () => { expect(fetchPullRequestMergeStateMock).not.toHaveBeenCalled(); }); - it("routes a `failed` outcome to a human past the fix-attempt cap (#1133)", async () => { + it("parks a `failed` outcome past the fix-attempt cap (#1133, #1218)", async () => { (prisma.prFixQueueItem.findUnique as ReturnType).mockResolvedValue({ ...FIXTURE_ITEM, fixAttempts: 5, }); + (prisma.prFixQueueItem.updateMany as ReturnType).mockResolvedValue({ count: 1 }); const result = await resolvePrFixFromAgentReport(baseInput({ outcome: "failed", attempt: ATTEMPT, })); expect(result.matched).toBe(true); - expect(result.action).toBe("blocked"); + expect(result.action).toBe("parked"); expect(result.reason).toContain("cap"); expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledWith( expect.objectContaining({ where: { id: "item-42", status: "QUEUED", generation: 1 }, data: expect.objectContaining({ - status: "BLOCKED", - lane: "NEEDS_HUMAN", + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", }), }), ); @@ -515,9 +551,9 @@ describe("resolvePrFixFromAgentReport", () => { expect(fetchPullRequestMergeStateMock).toHaveBeenCalledTimes(1); }); - it("writes no BLOCKED state when a blocked report hits a concurrent re-issue at commit time", async () => { + it("writes no parked state when a blocked report hits a concurrent re-issue at commit time", async () => { // Same faithful re-issue chain as the FIXED case above (#1119): the - // guarded BLOCKED write must not retry once the re-read shows the row + // guarded parked write must not retry once the re-read shows the row // moved — exactly one updateMany call. (prisma.prFixQueueItem.findUnique as ReturnType) .mockResolvedValueOnce({ ...FIXTURE_ITEM }) @@ -533,13 +569,13 @@ describe("resolvePrFixFromAgentReport", () => { expect(result.matched).toBe(true); expect(result.action).toBe("skipped"); expect(result.reason).toContain("generation-mismatch"); - // The BLOCKED write was generation-conditional (and, for a settlement, + // The parked write was generation-conditional (and, for a settlement, // guarded on the post-dispatch evidence list being empty, #1119) and // matched nothing. expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledTimes(1); expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledWith({ where: { id: "item-42", generation: 1, postDispatchEvidenceKeys: { equals: [] } }, - data: expect.objectContaining({ status: "BLOCKED" }), + data: expect.objectContaining({ status: "PARKED" }), }); expect(prisma.prFixQueueItem.update).not.toHaveBeenCalled(); expect(prisma.prFixHistory.create).not.toHaveBeenCalled(); diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 8cf36af8..1a79a4d3 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, beforeEach, afterEach, vi } from "vitest"; -import { createLinkedPrFixItem, enqueuePrFixItem, normalizeQueueRepo, listQueuedPrFixItems, markPrFixItem, toAgentQueuePrFixItem, reconcileStalePrFixItems, requeuePrFixItem, buildPrFixBlockedContext, parseMarkPrFixInput, resolvePrFixFromAgentReport, ackPrFixHandout, parseAckPrFixHandoutInput, reclaimStalePrFixHandouts, prFixHandoutTimeoutMs, maxPrFixReclaims, agentAlreadyHanded, DEFAULT_PR_FIX_HANDOUT_TIMEOUT_MS, DEFAULT_PR_FIX_MAX_RECLAIMS, PrFixQueueClient, type MarkPrFixResult } from "./pr-fix-queue"; +import { createLinkedPrFixItem, enqueuePrFixItem, normalizeQueueRepo, listQueuedPrFixItems, markPrFixItem, toAgentQueuePrFixItem, reconcileStalePrFixItems, requeuePrFixItem, buildPrFixBlockedContext, parseMarkPrFixInput, parseEnqueuePrFixInput, resolvePrFixFromAgentReport, ackPrFixHandout, parseAckPrFixHandoutInput, reclaimStalePrFixHandouts, prFixHandoutTimeoutMs, maxPrFixReclaims, agentAlreadyHanded, DEFAULT_PR_FIX_HANDOUT_TIMEOUT_MS, DEFAULT_PR_FIX_MAX_RECLAIMS, PrFixQueueClient, type MarkPrFixResult } from "./pr-fix-queue"; function mutatedItem(result: MarkPrFixResult): any { if (!result.mutated) throw new Error(`expected mutation, got ${result.reason}`); @@ -9,6 +9,7 @@ function mutatedItem(result: MarkPrFixResult): any { const { surfacingMocks, lessonFeedMocks, githubPrsMocks } = vi.hoisted(() => ({ surfacingMocks: { surfacePrFixBlocked: vi.fn().mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }), + surfacePrFixParked: vi.fn().mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }), surfacePrFixRequeued: vi.fn().mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }), surfacePrFixUnblocked: vi.fn().mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }), extractUrlsFromText: vi.fn((text: string) => { @@ -28,6 +29,7 @@ const { surfacingMocks, lessonFeedMocks, githubPrsMocks } = vi.hoisted(() => ({ vi.mock("./pr-fix-surfacing", () => ({ surfacePrFixBlocked: surfacingMocks.surfacePrFixBlocked, + surfacePrFixParked: surfacingMocks.surfacePrFixParked, surfacePrFixRequeued: surfacingMocks.surfacePrFixRequeued, surfacePrFixUnblocked: surfacingMocks.surfacePrFixUnblocked, extractUrlsFromText: surfacingMocks.extractUrlsFromText, @@ -126,6 +128,10 @@ function makeClient(): PrFixQueueClient & { // so a freshly created row reads back as "no ack, never reclaimed". handoutAcks: [], // mirrors the column's @default([]) handoutReclaims: 0, // mirrors the column's @default(0) + // #1218 outcome classification columns: default to null so a freshly + // created row reads back as "no outcome / no decision". + outcome: null, + decisionDetail: null, queuedAt: new Date(Date.UTC(2026, 0, 1, 0, seq)), updatedAt: new Date(Date.UTC(2026, 0, 1, 0, seq)), ...data, @@ -286,6 +292,8 @@ describe("PR review-fix queue", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixRequeued.mockReset(); surfacingMocks.surfacePrFixRequeued.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); }); @@ -377,7 +385,7 @@ describe("PR review-fix queue", () => { expect(await listQueuedPrFixItems(client, { includeBlocked: true })).toEqual([]); }); - it("blocks a REVIEW_FEEDBACK item after PR_FIX_MAX_ATTEMPTS fix attempts (#1001)", async () => { + it("parks a REVIEW_FEEDBACK item after PR_FIX_MAX_ATTEMPTS fix attempts (#1001, #1218)", async () => { const prev = process.env.PR_FIX_MAX_ATTEMPTS; process.env.PR_FIX_MAX_ATTEMPTS = "3"; try { @@ -390,12 +398,14 @@ describe("PR review-fix queue", () => { const fixed = await markPrFixItem(client, { repo: "org/repo", pr: 9, status: "fixed" }); expect(mutatedItem(fixed)?.status).toBe("FIXED"); } - // The 4th 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); + // The 4th attempt exceeds the cap → bounded exhaustion PARKS the item + // (#1218) instead of re-queuing or labeling it needs-human. + const parked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 9, lane: "NORMAL", reason: "review", feedback: "f4", evidenceKey: "review:4" }); + expect(parked.status).toBe("PARKED"); + expect(parked.lane).toBe("NORMAL"); + expect(parked.outcome).toBe("no_progress"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); } finally { if (prev === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; else process.env.PR_FIX_MAX_ATTEMPTS = prev; @@ -410,6 +420,8 @@ describe("item URL is identity, write-once (#1098)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); }); it("keeps the PR URL when a CI-failure re-enqueue carries a job URL", async () => { @@ -531,6 +543,8 @@ describe("work generation identity (#1044)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixRequeued.mockReset(); surfacingMocks.surfacePrFixRequeued.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); @@ -788,6 +802,8 @@ describe("pr-fix surfacing integration", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixRequeued.mockReset(); surfacingMocks.surfacePrFixRequeued.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); }); @@ -1088,12 +1104,15 @@ describe("enqueuePrFixItem evidence dedupe", () => { }); describe("buildPrFixBlockedContext", () => { - it("derives totalAttempts and lastAttemptSummary from feedback", async () => { + it("derives workerExecutions and lastAttemptSummary from the item/feedback", async () => { const client = makeClient(); - const item = { repo: "org/repo", pr: 7, feedback: ["first attempt", "final attempt"] }; + const item = { repo: "org/repo", pr: 7, feedback: ["first attempt", "final attempt"], fixAttempts: 4 }; const context = await buildPrFixBlockedContext(client, item); - expect(context.totalAttempts).toBe(2); + expect(context.workerExecutions).toBe(4); + expect(context.enqueueRows).toBe(0); expect(context.lastAttemptSummary).toBe("final attempt"); + // The enqueue/feedback row count is never emitted as totalAttempts (#1218). + expect(context.totalAttempts).toBeUndefined(); }); it("records the BLOCKED mark note as the final failure signature and groups attempts by lane", async () => { @@ -1109,6 +1128,8 @@ describe("buildPrFixBlockedContext", () => { const context = await buildPrFixBlockedContext(client, item); expect(context.finalFailureSignature).toBe("tests failed after 3 attempts"); expect(context.attemptsByLane).toEqual({ NORMAL: 1, ESCALATED: 1 }); + expect(context.enqueueRows).toBe(2); + expect(context.workerExecutions).toBe(item.fixAttempts); }); it("extracts and dedupes failing run links from feedback", async () => { @@ -1125,10 +1146,10 @@ describe("buildPrFixBlockedContext", () => { expect(context.failingRunLinks).toEqual(["https://github.com/org/repo/actions/runs/1"]); }); - it("uses uncapped history for totalAttempts", async () => { + it("uses the history enqueue count for enqueueRows, never totalAttempts", async () => { // 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). + // enqueue counting, not the #1001 bound (which would otherwise park the + // later attempts). const prev = process.env.PR_FIX_MAX_ATTEMPTS; process.env.PR_FIX_MAX_ATTEMPTS = "100"; try { @@ -1145,8 +1166,9 @@ describe("buildPrFixBlockedContext", () => { } const context = await buildPrFixBlockedContext(client, client.items.find((item) => item.pr === 8)); - expect(context.totalAttempts).toBe(13); + expect(context.enqueueRows).toBe(13); expect(context.attemptsByLane).toEqual({ NORMAL: 13 }); + expect(context.totalAttempts).toBeUndefined(); } finally { if (prev === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; else process.env.PR_FIX_MAX_ATTEMPTS = prev; @@ -1156,7 +1178,7 @@ describe("buildPrFixBlockedContext", () => { it("returns no per-lane/signature data when absent (backwards compatible)", async () => { const client = makeClient(); const context = await buildPrFixBlockedContext(client, { repo: "org/repo", pr: 1, feedback: [] }); - expect(context.totalAttempts).toBeUndefined(); + expect(context.workerExecutions).toBeNull(); expect(context.finalFailureSignature).toBeUndefined(); expect(context.lastAttemptSummary).toBeUndefined(); expect(context.failingRunLinks).toBeUndefined(); @@ -1208,7 +1230,7 @@ describe("requeuePrFixItem surface cleanup", () => { surfacingMocks.surfacePrFixRequeued.mockClear(); await expect(requeuePrFixItem(client, { repo: "org/repo", pr: 72 })) - .rejects.toThrow("not BLOCKED or FIXED"); + .rejects.toThrow("not BLOCKED, PARKED, or FIXED"); expect(surfacingMocks.surfacePrFixRequeued).not.toHaveBeenCalled(); }); @@ -1390,6 +1412,8 @@ describe("#1121 already_addressed settlement", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixRequeued.mockReset(); surfacingMocks.surfacePrFixRequeued.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); @@ -1456,7 +1480,7 @@ describe("#1121 already_addressed settlement", () => { expect(reopened.fixAttempts).toBe(2); }); - it("a repeated already_addressed disagreement with an unchanged head is bounded to BLOCKED at the cap", async () => { + it("a repeated already_addressed disagreement with an unchanged head is bounded to PARKED at the cap (#1218)", async () => { const prevCap = process.env.PR_FIX_MAX_ATTEMPTS; process.env.PR_FIX_MAX_ATTEMPTS = "3"; try { @@ -1473,16 +1497,17 @@ describe("#1121 already_addressed settlement", () => { repo: "acme/widgets", pr: 1121, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k1", headSha: "h", }); - if (enq.status === "BLOCKED") break; + if (enq.status === "PARKED") break; await markPrFixItem(fresh, { repo: "acme/widgets", pr: 1121, status: "FIXED", alreadyAddressed: true, }); } - expect(enq.status).toBe("BLOCKED"); + expect(enq.status).toBe("PARKED"); const row = await fresh.prFixQueueItem.findUnique({ where: { repo_pr: { repo: "acme/widgets", pr: 1121 } } }); - expect(row.status).toBe("BLOCKED"); - expect(row.lane).toBe("NEEDS_HUMAN"); + expect(row.status).toBe("PARKED"); + expect(row.lane).toBe("NORMAL"); + expect(row.outcome).toBe("no_progress"); } finally { if (prevCap === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; else process.env.PR_FIX_MAX_ATTEMPTS = prevCap; @@ -1497,6 +1522,8 @@ describe("attempt baseline + generation-conditional writes (#1074)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); // Default to "head moved" so the guard passes unless a test overrides. githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("movedsha"); @@ -1600,6 +1627,8 @@ describe("settlement races and baseline fallback (#1074)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("movedsha"); githubPrsMocks.fetchPullRequestMergeState.mockReset(); @@ -1885,6 +1914,8 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { process.env.PR_FIX_MAX_ATTEMPTS = "2"; surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("movedsha"); }); @@ -1917,8 +1948,8 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { await markPrFixItem(client, { repo: "org/repo", pr: 2, status: "fixed" }); await enqueuePrFixItem(client, { repo: "org/repo", pr: 2, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2", headSha: "H2" }); await markPrFixItem(client, { repo: "org/repo", pr: 2, status: "fixed" }); - const blocked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 2, lane: "NORMAL", reason: "r", feedback: "f3", evidenceKey: "review:3", headSha: "H3" }); - expect(blocked.status).toBe("BLOCKED"); + const parked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 2, lane: "NORMAL", reason: "r", feedback: "f3", evidenceKey: "review:3", headSha: "H3" }); + expect(parked.status).toBe("PARKED"); const requeued = await requeuePrFixItem(client, { repo: "org/repo", pr: 2 }); expect(requeued.fixAttempts).toBe(1); @@ -1928,7 +1959,7 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { expect(reopened.fixAttempts).toBe(2); }); - it("counts refused no-push runs and hands the PR to a human past the cap", async () => { + it("counts refused no-push runs and parks the PR past the cap (#1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 3, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "review:1", headSha: "H1" }); githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("H1"); @@ -1937,13 +1968,15 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { expect(mutatedItem(retried).fixAttempts).toBe(2); const capped = await markPrFixItem(client, { repo: "org/repo", pr: 3, status: "FIXED", expectedGeneration: 2 }); - expect(mutatedItem(capped).status).toBe("BLOCKED"); - expect(mutatedItem(capped).lane).toBe("NEEDS_HUMAN"); + expect(mutatedItem(capped).status).toBe("PARKED"); + expect(mutatedItem(capped).lane).toBe("NORMAL"); + expect(mutatedItem(capped).outcome).toBe("no_progress"); expect(mutatedItem(capped).generation).toBe(2); - expect(client.history.at(-1)).toMatchObject({ status: "BLOCKED", lane: "NEEDS_HUMAN" }); + expect(client.history.at(-1)).toMatchObject({ status: "PARKED", lane: "NORMAL", outcome: "no_progress" }); expect(client.history.at(-1).note).toContain("Refused FIXED"); expect(client.history.at(-1).note).toContain("Bounded at 2 fix attempts"); - expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); }); it("counts a mark back to QUEUED as an attempt without resetting the budget", async () => { @@ -1954,14 +1987,14 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { expect(queued.status).toBe("QUEUED"); expect(queued.fixAttempts).toBe(2); - // The budget is spent, so the next return goes to a human. + // The budget is spent, so the next return is parked. expect(mutatedItem(await markPrFixItem(client, { repo: "org/repo", pr: 4, status: "fixed" })).status).toBe("FIXED"); - const blocked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 4, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2", headSha: "H2" }); - expect(blocked.status).toBe("BLOCKED"); - expect(blocked.lane).toBe("NEEDS_HUMAN"); + const parked = await enqueuePrFixItem(client, { repo: "org/repo", pr: 4, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2", headSha: "H2" }); + expect(parked.status).toBe("PARKED"); + expect(parked.lane).toBe("NORMAL"); }); - it("caps the #940 no-progress tombstone reopen like any other return to QUEUED", async () => { + it("parks the #940 no-progress tombstone reopen past the cap (#1218)", async () => { const input = { repo: "org/repo", pr: 5, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "review:1", headSha: "H1" }; await enqueuePrFixItem(client, input); expect(mutatedItem(await markPrFixItem(client, { repo: "org/repo", pr: 5, status: "fixed" })).status).toBe("FIXED"); @@ -1971,11 +2004,11 @@ describe("attempt cap counts fix attempts, not evidence (#1103)", () => { expect(reopened.status).toBe("QUEUED"); expect(reopened.fixAttempts).toBe(2); - // Past the cap the reopen hands the PR to a human instead. + // Past the cap the reopen is parked instead. expect(mutatedItem(await markPrFixItem(client, { repo: "org/repo", pr: 5, status: "fixed" })).status).toBe("FIXED"); const capped = await enqueuePrFixItem(client, input); - expect(capped.status).toBe("BLOCKED"); - expect(capped.lane).toBe("NEEDS_HUMAN"); + expect(capped.status).toBe("PARKED"); + expect(capped.lane).toBe("NORMAL"); }); }); @@ -1986,6 +2019,8 @@ describe("fresh attempts always get a head baseline (#1104)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); }); @@ -2035,7 +2070,9 @@ describe("fresh attempts always get a head baseline (#1104)", () => { it("an enqueue reopen without a head observation keeps the last observed head", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 7, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1" }); - await markPrFixItem(client, { repo: "org/repo", pr: 7, status: "blocked" }); + // FIXED (not BLOCKED) so new evidence reopens: BLOCKED/PARKED are sticky + // and would not reopen (#1218). + await markPrFixItem(client, { repo: "org/repo", pr: 7, status: "fixed" }); const reopened = await enqueuePrFixItem(client, { repo: "org/repo", pr: 7, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2" }); expect(reopened.status).toBe("QUEUED"); expect(reopened.attemptHeadSha).toBe("H1"); @@ -2062,6 +2099,8 @@ describe("needs-human cleanup on every exit from BLOCKED (#1105)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixUnblocked.mockReset(); surfacingMocks.surfacePrFixUnblocked.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); @@ -2081,10 +2120,11 @@ describe("needs-human cleanup on every exit from BLOCKED (#1105)", () => { expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 1, "requeued", undefined); }); - it("folds to a requeued notice when new evidence reopens the item", async () => { - const reopened = await enqueuePrFixItem(client, { repo: "org/repo", pr: 1, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2", headSha: "H2" }); - expect(reopened.status).toBe("QUEUED"); - expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 1, "requeued", expect.stringContaining("New evidence")); + it("does NOT reopen a BLOCKED item on new evidence (sticky, #1218) and does not retract", async () => { + const after = await enqueuePrFixItem(client, { repo: "org/repo", pr: 1, lane: "NORMAL", reason: "r", feedback: "f2", evidenceKey: "review:2", headSha: "H2" }); + expect(after.status).toBe("BLOCKED"); + expect(after.lane).toBe("NEEDS_HUMAN"); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); }); it("folds to a requeued notice when a FIXED mark is refused back to QUEUED", async () => { @@ -2116,6 +2156,8 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixUnblocked.mockReset(); surfacingMocks.surfacePrFixUnblocked.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); @@ -2267,7 +2309,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(marked.postDispatchEvidenceKeys).toEqual([]); }); - it("post-dispatch evidence past the attempt cap gives up to a human", async () => { + it("post-dispatch evidence past the attempt cap parks the item (#1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 26, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2290,8 +2332,9 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(result.mutated).toBe(true); const capped = mutatedItem(result); - expect(capped.status).toBe("BLOCKED"); - expect(capped.lane).toBe("NEEDS_HUMAN"); + expect(capped.status).toBe("PARKED"); + expect(capped.lane).toBe("NORMAL"); + expect(capped.outcome).toBe("no_progress"); expect(capped.postDispatchEvidenceKeys).toEqual([]); // Gave up: no generation bump, no attempt increment. expect(capped.generation).toBe(1); @@ -2680,7 +2723,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(client.history.at(-1).note).toContain("forced a fresh attempt"); }); - it("the forced fallback gives up to a human when the item is at the cap (#1124)", async () => { + it("the forced fallback parks the item when it is at the cap (#1124, #1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 38, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2706,18 +2749,20 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(result.mutated).toBe(true); const item = client.items[0]; - // Capped: the forced fallback routes to a human instead of a fresh run. - expect(item.status).toBe("BLOCKED"); - expect(item.lane).toBe("NEEDS_HUMAN"); + // Capped: the forced fallback parks the item instead of a fresh run. + expect(item.status).toBe("PARKED"); + expect(item.lane).toBe("NORMAL"); + expect(item.outcome).toBe("no_progress"); expect(item.generation).toBe(1); expect(item.fixAttempts).toBe(5); expect(item.postDispatchEvidenceKeys).toEqual([]); - expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); // Evidence landed on the fast path, every loop iteration, AND the forced // fallback write — strictly more than the 3-iteration loop allows. expect(landCount).toBeGreaterThan(3); - // The capped forced-fallback history row names the human hand-off. - expect(client.history.at(-1).note).toContain("routed to a human (#1119)"); + // The capped forced-fallback history row names the parked hand-off. + expect(client.history.at(-1).note).toContain("parked (#1119)"); }); it("a post-dispatch check encoded from an `@`-bearing headSha degrades to an unknown head and still reopens", async () => { @@ -2873,7 +2918,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 45, "requeued", undefined); }); - it("a capped post-dispatch reopen surfaces the block when the item was not already BLOCKED", async () => { + it("a capped post-dispatch reopen surfaces the parked notice when the item was not already parked", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 46, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2885,19 +2930,20 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { }); // At the attempt cap (maxPrFixAttempts defaults to 5). client.items[0].fixAttempts = 5; - surfacingMocks.surfacePrFixBlocked.mockClear(); + surfacingMocks.surfacePrFixParked.mockClear(); const result = await markPrFixItem(client, { repo: "org/repo", pr: 46, status: "FIXED", expectedGeneration: 1, }); - // The give-up routes to a human; the item was QUEUED (not BLOCKED), so - // the block must be surfaced. + // The give-up parks the item; it was QUEUED (not parked), so the parked + // notice must be surfaced — with no needs-human label. const capped = mutatedItem(result); - expect(capped.status).toBe("BLOCKED"); - expect(capped.lane).toBe("NEEDS_HUMAN"); - expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + expect(capped.status).toBe("PARKED"); + expect(capped.lane).toBe("NORMAL"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); }); it("a stale post-dispatch check settles with the 'no longer applies' history note", async () => { @@ -3044,6 +3090,8 @@ describe("enqueue post-dispatch append pinning (#1134)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixUnblocked.mockReset(); surfacingMocks.surfacePrFixUnblocked.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestHeadSha.mockReset(); @@ -3391,6 +3439,8 @@ describe("reclaim stale hand-outs (#1211)", () => { client = makeClient(); surfacingMocks.surfacePrFixBlocked.mockReset(); surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); surfacingMocks.surfacePrFixUnblocked.mockReset(); surfacingMocks.surfacePrFixUnblocked.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); githubPrsMocks.fetchPullRequestState.mockReset(); @@ -3486,7 +3536,7 @@ describe("reclaim stale hand-outs (#1211)", () => { expect(client.history.length).toBe(historyBefore); }); - it("routes to a human (BLOCKED/NEEDS_HUMAN) once the reclaim cap is reached", async () => { + it("parks (PARKED/NORMAL) once the reclaim cap is reached (#1218)", async () => { const prev = process.env.PR_FIX_MAX_RECLAIMS; process.env.PR_FIX_MAX_RECLAIMS = "3"; try { @@ -3499,24 +3549,27 @@ describe("reclaim stale hand-outs (#1211)", () => { expect(report.blocked).toBe(1); expect(report.reclaimed).toBe(0); const after = client.items[0]; - expect(after.status).toBe("BLOCKED"); - expect(after.lane).toBe("NEEDS_HUMAN"); + expect(after.status).toBe("PARKED"); + expect(after.lane).toBe("NORMAL"); + expect(after.outcome).toBe("no_progress"); expect(after.generation).toBe(1); // terminal: no identity bump expect(after.agentHandouts).toEqual([]); expect(after.handoutAcks).toEqual([]); expect(after.dispatchedGeneration).toBeNull(); expect(after.dispatchedAt).toBeNull(); const last = client.history.at(-1); - expect(last).toMatchObject({ action: "mark", status: "BLOCKED", lane: "NEEDS_HUMAN" }); + expect(last).toMatchObject({ action: "mark", status: "PARKED", lane: "NORMAL", outcome: "no_progress" }); expect(last.note).toContain("PR_FIX_MAX_RECLAIMS=3"); expect(last.note).toContain("1211"); + // The reclaim cap never applies the needs-human label/marker (#1218). + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); } finally { if (prev === undefined) delete process.env.PR_FIX_MAX_RECLAIMS; else process.env.PR_FIX_MAX_RECLAIMS = prev; } }); - it("accumulates the reclaim bound across repeated sweeps until the cap routes to a human", async () => { + it("accumulates the reclaim bound across repeated sweeps until the cap parks the item", async () => { await seedStaleUnacked(215); // The sweep clears the stamp on each reclaim, so a further sweep only sees // a stale hand-out once the item is re-stamped stale — mirroring next-task @@ -3542,7 +3595,7 @@ describe("reclaim stale hand-outs (#1211)", () => { } expect(generations).toEqual([2, 3, 4]); - // The 4th sweep is past the cap: BLOCKED / NEEDS_HUMAN, no further bump. + // The 4th sweep is past the cap: PARKED / NORMAL, no further bump. restampStale(); const finalReport = await reclaimStalePrFixHandouts(client, { now: STALE_NOW, @@ -3551,8 +3604,8 @@ describe("reclaim stale hand-outs (#1211)", () => { }); expect(finalReport.blocked).toBe(1); expect(finalReport.reclaimed).toBe(0); - expect(client.items[0].status).toBe("BLOCKED"); - expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); expect(client.items[0].generation).toBe(4); }); @@ -3993,3 +4046,280 @@ describe("reclaim stale hand-outs (#1211)", () => { } }); }); + +// Issue #1218: genuine human decisions (BLOCKED + NEEDS_HUMAN) are separated +// from bounded retry/no-progress exhaustion and operational failures (PARKED). +describe("PR-fix outcome separation (#1218)", () => { + let client: ReturnType; + + beforeEach(() => { + client = makeClient(); + surfacingMocks.surfacePrFixBlocked.mockReset(); + surfacingMocks.surfacePrFixBlocked.mockResolvedValue({ labelApplied: true, commentPosted: true, errors: [] }); + surfacingMocks.surfacePrFixParked.mockReset(); + surfacingMocks.surfacePrFixParked.mockResolvedValue({ commentPosted: true, commentUpdated: false, skippedTerminal: false, errors: [] }); + surfacingMocks.surfacePrFixUnblocked.mockReset(); + surfacingMocks.surfacePrFixUnblocked.mockResolvedValue({ labelRemoved: true, commentUpdated: true, errors: [] }); + githubPrsMocks.fetchPullRequestHeadSha.mockReset(); + githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("movedsha"); + githubPrsMocks.fetchPullRequestMergeState.mockReset(); + githubPrsMocks.fetchPullRequestMergeState.mockResolvedValue({ mergeableState: null, mergeable: null }); + }); + + it("parks a failed report past the fix-attempt cap and never labels it human", async () => { + const prev = process.env.PR_FIX_MAX_ATTEMPTS; + process.env.PR_FIX_MAX_ATTEMPTS = "1"; + try { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 301, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k301", headSha: "H1", + }); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, + repoFullName: "org/repo", + pullRequestNumber: 301, + outcome: "failed", + summary: "worker failed", + attempt: { itemId: client.items[0].id, generation: 1 }, + }); + + expect(result.action).toBe("parked"); + const row = client.items[0]; + expect(row.status).toBe("PARKED"); + expect(row.lane).toBe("NORMAL"); + expect(row.outcome).toBe("no_progress"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + } finally { + if (prev === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; + else process.env.PR_FIX_MAX_ATTEMPTS = prev; + } + }); + + it("marks BLOCKED + NEEDS_HUMAN and surfaces the decision on a human_decision blocked report", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 302, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k302", + }); + const decision = { + question: "Keep the legacy endpoint?", + options: ["Keep it", "Remove it"], + evidence: "Breaking change to the public API.", + }; + + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, + repoFullName: "org/repo", + pullRequestNumber: 302, + outcome: "blocked", + summary: "needs a design call", + blockReason: "human_decision", + decision, + attempt: { itemId: client.items[0].id, generation: 1 }, + }); + + expect(result.action).toBe("blocked"); + const row = client.items[0]; + expect(row.status).toBe("BLOCKED"); + expect(row.lane).toBe("NEEDS_HUMAN"); + expect(row.outcome).toBe("human_decision"); + expect(row.decisionDetail).toEqual(decision); + expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + const surfaceInput = surfacingMocks.surfacePrFixBlocked.mock.calls[0][0] as { decision?: unknown }; + expect(surfaceInput.decision).toEqual(decision); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + }); + + it("parks a bare blocked report (a give-up is NOT a human decision)", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 303, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k303", + }); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, + repoFullName: "org/repo", + pullRequestNumber: 303, + outcome: "blocked", + summary: "gave up", + attempt: { itemId: client.items[0].id, generation: 1 }, + }); + + expect(result.action).toBe("parked"); + const row = client.items[0]; + expect(row.status).toBe("PARKED"); + expect(row.lane).toBe("NORMAL"); + expect(row.outcome).toBe("no_progress"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("parks an explicit infra_failure blocked report without a needs-human label", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 304, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k304", + }); + const result = await resolvePrFixFromAgentReport({ + client: client as PrFixQueueClient, + repoFullName: "org/repo", + pullRequestNumber: 304, + outcome: "blocked", + blockReason: "infra_failure", + attempt: { itemId: client.items[0].id, generation: 1 }, + }); + + expect(result.action).toBe("parked"); + expect(client.items[0].status).toBe("PARKED"); + expect(client.items[0].outcome).toBe("infra_failure"); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("reopens to QUEUED as a fresh attempt on fresh feedback after a productive FIXED push", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 305, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", + }); + githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("H2"); + const fixed = await markPrFixItem(client, { repo: "org/repo", pr: 305, status: "FIXED", expectedGeneration: 1 }); + expect(mutatedItem(fixed).status).toBe("FIXED"); + const budgetBefore = client.items[0].fixAttempts; + + const reopened = await enqueuePrFixItem(client, { + repo: "org/repo", pr: 305, lane: "NORMAL", reason: "new review round", feedback: "f2", evidenceKey: "review:2", headSha: "H2", + }); + expect(reopened.status).toBe("QUEUED"); + expect(reopened.generation).toBe(2); + expect(reopened.fixAttempts).toBe(budgetBefore + 1); + }); + + it("consumes no budget and does not flip status on re-observed known feedback", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 306, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", + }); + const before = { status: client.items[0].status, fixAttempts: client.items[0].fixAttempts, generation: client.items[0].generation }; + + const after = await enqueuePrFixItem(client, { + repo: "org/repo", pr: 306, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", + }); + expect(after.status).toBe(before.status); + expect(after.fixAttempts).toBe(before.fixAttempts); + expect(after.generation).toBe(before.generation); + }); + + it("a late worker generation cannot overwrite a human-decision outcome", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 307, lane: "NEEDS_HUMAN", reason: "needs decision", feedback: "f", evidenceKey: "k307", + }); + expect(client.items[0].status).toBe("BLOCKED"); + expect(client.items[0].outcome).toBe("human_decision"); + // A concurrent re-issue bumps the row; the late worker still holds gen 1. + client.items[0].generation = 2; + + const result = await markPrFixItem(client, { + repo: "org/repo", pr: 307, status: "FIXED", expectedGeneration: 1, + }); + + expect(result).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0].status).toBe("BLOCKED"); + expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].outcome).toBe("human_decision"); + }); + + it("a late worker generation cannot overwrite a parked outcome", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 308, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k308", + }); + const parked = await markPrFixItem(client, { repo: "org/repo", pr: 308, status: "PARKED", outcome: "no_progress", expectedGeneration: 1 }); + expect(mutatedItem(parked).status).toBe("PARKED"); + client.items[0].generation = 2; + + const result = await markPrFixItem(client, { + repo: "org/repo", pr: 308, status: "FIXED", expectedGeneration: 1, + }); + + expect(result).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0].status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("no_progress"); + }); + + it("keeps the attempt cap and the reclaim cap independent: a reclaim does not consume the attempt budget", async () => { + const prevCap = process.env.PR_FIX_MAX_ATTEMPTS; + process.env.PR_FIX_MAX_ATTEMPTS = "2"; + try { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 309, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k309", headSha: "H1", + }); + const row = client.items[0]; + // At the attempt cap, but with zero reclaims — the reclaim brake is a + // separate budget and must still fire. + row.fixAttempts = 2; + row.dispatchedGeneration = row.generation; + row.dispatchedAt = new Date("2026-01-01T00:00:00Z"); + row.agentHandouts = [`courier@${row.generation}`]; + row.handoutAcks = []; + row.handoutReclaims = 0; + + const report = await reclaimStalePrFixHandouts(client, { + now: new Date("2026-01-01T01:00:00Z"), + maxReclaims: 3, + isPrMergedOrClosed: async () => false, + }); + + expect(report.reclaimed).toBe(1); + expect(client.items[0].status).toBe("QUEUED"); + // The reclaim bumped the reclaim budget, never the worker-execution budget. + expect(client.items[0].handoutReclaims).toBe(1); + expect(client.items[0].fixAttempts).toBe(2); + expect(client.items[0].generation).toBe(2); + } finally { + if (prevCap === undefined) delete process.env.PR_FIX_MAX_ATTEMPTS; + else process.env.PR_FIX_MAX_ATTEMPTS = prevCap; + } + }); + + it("parses and validates outcome/decision on enqueue and mark inputs", () => { + // Enqueue: valid outcome + decision round-trip. + const enq = parseEnqueuePrFixInput({ + repo: "o/r", pr: 1, reason: "r", feedback: "f", evidenceKey: "e", + outcome: "infra_failure", + decision: { question: "q", options: ["a", "b"], evidence: "ev" }, + }); + if ("error" in enq) throw new Error(enq.error); + expect(enq.outcome).toBe("infra_failure"); + expect(enq.decision).toEqual({ question: "q", options: ["a", "b"], evidence: "ev" }); + + // Enqueue: invalid outcome. + expect(parseEnqueuePrFixInput({ + repo: "o/r", pr: 1, reason: "r", feedback: "f", evidenceKey: "e", outcome: "nonsense", + })).toEqual({ error: "Invalid outcome" }); + + // Enqueue: malformed decision. + expect(parseEnqueuePrFixInput({ + repo: "o/r", pr: 1, reason: "r", feedback: "f", evidenceKey: "e", decision: { options: [1] }, + })).toEqual({ error: expect.stringContaining("decision.options") }); + + // Mark: BLOCKED defaults to human_decision; PARKED defaults to no_progress. + const blocked = parseMarkPrFixInput({ repo: "o/r", pr: 1, status: "BLOCKED" }); + if ("error" in blocked) throw new Error(blocked.error); + expect(blocked.outcome).toBe("human_decision"); + + const parked = parseMarkPrFixInput({ repo: "o/r", pr: 1, status: "PARKED" }); + if ("error" in parked) throw new Error(parked.error); + expect(parked.outcome).toBe("no_progress"); + + const parkedExplicit = parseMarkPrFixInput({ repo: "o/r", pr: 1, status: "PARKED", outcome: "security_failure" }); + if ("error" in parkedExplicit) throw new Error(parkedExplicit.error); + expect(parkedExplicit.outcome).toBe("security_failure"); + + expect(parseMarkPrFixInput({ repo: "o/r", pr: 1, status: "PARKED", outcome: "bogus" })).toEqual({ error: "Invalid outcome" }); + }); + + it("exposes outcome on the agent-queue item and PARKED in the queue listing", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 310, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k310", + }); + await markPrFixItem(client, { repo: "org/repo", pr: 310, status: "PARKED", outcome: "security_failure" }); + + const listed = await listQueuedPrFixItems(client, { includeBlocked: true }); + expect(listed.map((i) => i.pr)).toContain(310); + const parkedRow = listed.find((i) => i.pr === 310)!; + expect(toAgentQueuePrFixItem(parkedRow).outcome).toBe("security_failure"); + // PARKED is excluded from the default (QUEUED-only) listing. + expect((await listQueuedPrFixItems(client)).map((i) => i.pr)).not.toContain(310); + }); +}); diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 9308e414..43ef80d5 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -1,5 +1,5 @@ -import { normalizePrFixLane, normalizePrFixStatus, normalizePrFixType, PrFixLane, PrFixStatus, PrFixType, PR_FIX_TYPE_PRIORITY } from "@/types"; -import { surfacePrFixBlocked, surfacePrFixRequeued, surfacePrFixUnblocked, extractUrlsFromText, type PrFixUnblockOutcome } from "./pr-fix-surfacing"; +import { normalizePrFixLane, normalizePrFixStatus, normalizePrFixType, normalizePrFixOutcome, PrFixLane, PrFixOutcome, PrFixStatus, PrFixType, PrFixDecisionContext, PR_FIX_TYPE_PRIORITY } from "@/types"; +import { surfacePrFixBlocked, surfacePrFixParked, surfacePrFixRequeued, surfacePrFixUnblocked, extractUrlsFromText, type PrFixUnblockOutcome } from "./pr-fix-surfacing"; import { prisma } from "@/lib/prisma"; import { fetchPullRequestMergeState, fetchPullRequestHeadSha, fetchPullRequestState } from "./github-prs"; import { fetchRepositoryMetadata } from "./github-code-search"; @@ -54,6 +54,14 @@ export interface EnqueuePrFixInput { title?: string | null; headSha?: string | null; author?: string | null; + /** + * Structured outcome (#1218). Only meaningful when the enqueue resolves to a + * PARKED state (e.g. an explicit infra/security failure); a BLOCKED result + * always records `human_decision`. Defaults to `no_progress` for PARKED. + */ + outcome?: string | null; + /** Human-decision payload persisted on a BLOCKED result (#1218). */ + decision?: PrFixDecisionContext | null; } export interface CreateLinkedPrFixInput { @@ -95,6 +103,14 @@ export interface MarkPrFixInput { // item; a repeated disagreement reopens via #940 and counts toward fixAttempts. alreadyAddressed?: boolean; evidence?: string | null; + /** + * Structured outcome (#1218) for a BLOCKED/PARKED mark. BLOCKED defaults to + * `human_decision`; PARKED defaults to `no_progress`. Ignored for other + * statuses. + */ + outcome?: string | null; + /** Human-decision payload persisted when the mark resolves to BLOCKED (#1218). */ + decision?: PrFixDecisionContext | null; } export interface RequeuePrFixInput { @@ -109,6 +125,46 @@ export function nonEmpty(value: unknown): value is string { return typeof value === "string" && value.trim().length > 0; } +/** + * The structured outcome a status implies (#1218). BLOCKED is reserved for a + * genuine human decision; PARKED carries the explicit outcome or `no_progress`. + * Any other status has no outcome. + */ +export function resolvePrFixOutcome(status: PrFixStatus, explicit?: string | null): PrFixOutcome | null { + if (status === "BLOCKED") return "human_decision"; + if (status === "PARKED") return normalizePrFixOutcome(explicit) ?? "no_progress"; + return null; +} + +/** + * Validate and normalize a `decision` payload (#1218). Returns the cleaned + * context, `null` when absent, or an error string when malformed. + */ +function parseDecisionContext(value: unknown): { decision: PrFixDecisionContext | null; error?: string } { + if (value === undefined || value === null) return { decision: null }; + if (typeof value !== "object" || Array.isArray(value)) { + return { decision: null, error: "decision must be an object" }; + } + const raw = value as Record; + const decision: PrFixDecisionContext = {}; + if (raw.question !== undefined && raw.question !== null) { + if (typeof raw.question !== "string") return { decision: null, error: "decision.question must be a string" }; + if (raw.question.trim()) decision.question = raw.question.trim(); + } + if (raw.options !== undefined && raw.options !== null) { + if (!Array.isArray(raw.options) || raw.options.some((o) => typeof o !== "string")) { + return { decision: null, error: "decision.options must be an array of strings" }; + } + const options = raw.options.map((o) => (o as string).trim()).filter(Boolean); + if (options.length > 0) decision.options = options; + } + if (raw.evidence !== undefined && raw.evidence !== null) { + if (typeof raw.evidence !== "string") return { decision: null, error: "decision.evidence must be a string" }; + if (raw.evidence.trim()) decision.evidence = raw.evidence.trim(); + } + return { decision }; +} + /** * Canonical queue identity for a repository (#1145). * @@ -132,6 +188,13 @@ export function parseEnqueuePrFixInput(body: unknown): EnqueuePrFixInput | { err if (!nonEmpty(input.reason)) return { error: "Missing required field: reason" }; if (!nonEmpty(input.feedback)) return { error: "Missing required field: feedback" }; if (!nonEmpty(input.evidenceKey)) return { error: "Missing required field: evidenceKey" }; + if (input.outcome !== undefined && input.outcome !== null) { + if (typeof input.outcome !== "string" || !normalizePrFixOutcome(input.outcome)) { + return { error: "Invalid outcome" }; + } + } + const decisionResult = parseDecisionContext(input.decision); + if (decisionResult.error) return { error: decisionResult.error }; return { repo: input.repo.trim(), @@ -147,6 +210,8 @@ export function parseEnqueuePrFixInput(body: unknown): EnqueuePrFixInput | { err title: typeof input.title === "string" ? input.title : null, headSha: typeof input.headSha === "string" ? input.headSha : null, author: typeof input.author === "string" ? input.author : null, + ...(typeof input.outcome === "string" ? { outcome: normalizePrFixOutcome(input.outcome) } : {}), + ...(decisionResult.decision ? { decision: decisionResult.decision } : {}), }; } @@ -181,16 +246,28 @@ export function parseMarkPrFixInput(body: unknown): MarkPrFixInput | { error: st if (evidence && evidence.length > MAX_EVIDENCE_LENGTH) { return { error: `evidence must be at most ${MAX_EVIDENCE_LENGTH} characters` }; } + const status = normalizePrFixStatus(input.status) as PrFixStatus; + if (input.outcome !== undefined && input.outcome !== null) { + if (typeof input.outcome !== "string" || !normalizePrFixOutcome(input.outcome)) { + return { error: "Invalid outcome" }; + } + } + const decisionResult = parseDecisionContext(input.decision); + if (decisionResult.error) return { error: decisionResult.error }; + const explicitOutcome = typeof input.outcome === "string" ? normalizePrFixOutcome(input.outcome) : null; + const outcome = resolvePrFixOutcome(status, explicitOutcome); return { repo: input.repo.trim(), pr: Number(input.pr), - status: normalizePrFixStatus(input.status) as PrFixStatus, + status, note: typeof input.note === "string" ? input.note : null, expectedGeneration: typeof input.generation === "number" && input.generation !== null ? input.generation : undefined, attemptHeadSha: typeof input.attemptHeadSha === "string" ? input.attemptHeadSha.trim() : null, ...(alreadyAddressed ? { alreadyAddressed: true } : {}), ...(evidence ? { evidence } : {}), + ...(outcome ? { outcome } : {}), + ...(decisionResult.decision ? { decision: decisionResult.decision } : {}), }; } @@ -331,25 +408,26 @@ function laneLabel(lane: string | null | undefined): string { } /** - * Build the surfacing context for a BLOCKED item from data Dispatch actually has: - * the item's feedback (one entry per enqueue/attempt proxy — the latest is the - * best available last-attempt context) and its history rows. All fields are - * optional/fallback-safe so historical rows and old callers still surface - * something useful. Never throws. + * Build the surfacing context for a BLOCKED/PARKED item from data Dispatch + * actually has: the item's worker-execution count (`fixAttempts`), its feedback + * (the latest is the best available last-attempt context) and its history rows. + * All fields are optional/fallback-safe so historical rows and old callers + * still surface something useful. Never throws. + * + * Metrics truthfulness (#1218): `workerExecutions` is the dispatchable attempt + * count and `enqueueRows` is the enqueue/evidence row count; the enqueue count + * is deliberately NOT emitted as `totalAttempts`. */ export async function buildPrFixBlockedContext( client: PrFixQueueClient, - item: { repo: string; pr: number; feedback?: string[] | null }, + item: { repo: string; pr: number; feedback?: string[] | null; fixAttempts?: number | null }, ): Promise { const context: import("./pr-fix-surfacing").PrFixSurfaceContext = {}; const feedback = Array.isArray(item.feedback) ? item.feedback : []; - const totalAttempts = feedback.length > 0 ? feedback.length : undefined; - if (typeof totalAttempts === "number") context.totalAttempts = totalAttempts; const links: string[] = []; let lastAttemptSummary: string | null = null; - let historyLoaded = false; for (const entry of feedback) { if (!entry) continue; lastAttemptSummary = entry; @@ -359,15 +437,20 @@ export async function buildPrFixBlockedContext( if (links.length > 0) context.failingRunLinks = links; if (lastAttemptSummary) context.lastAttemptSummary = lastAttemptSummary; - // Attempts grouped by lane, plus the final failure signature from the BLOCKED - // tombstone note. Historical rows without a lane are counted under "unknown". + // Worker executions are the real bound; report them explicitly, never as the + // enqueue row count (#1218). + if (typeof item.fixAttempts === "number") context.workerExecutions = item.fixAttempts; + else context.workerExecutions = null; + + // Enqueue rows grouped by lane, plus the final failure signature from the + // latest BLOCKED or PARKED tombstone note. Historical rows without a lane are + // counted under "unknown". try { if (client.prFixHistory?.findMany) { const history = await client.prFixHistory.findMany({ where: { item: { repo: item.repo, pr: item.pr } }, orderBy: { at: "desc" }, }); - historyLoaded = true; const attemptsByLane: Record = {}; let enqueueCount = 0; @@ -377,20 +460,18 @@ export async function buildPrFixBlockedContext( const lane = laneLabel(h.lane); attemptsByLane[lane] = (attemptsByLane[lane] ?? 0) + 1; } - if (enqueueCount > 0) context.totalAttempts = enqueueCount; + context.enqueueRows = enqueueCount; if (Object.keys(attemptsByLane).length > 0) context.attemptsByLane = attemptsByLane; - const blocked = history.find((h) => h.action === "mark" && h.status === "BLOCKED" && h.note); - if (blocked?.note) context.finalFailureSignature = blocked.note; + const terminal = history.find( + (h) => h.action === "mark" && (h.status === "BLOCKED" || h.status === "PARKED") && h.note, + ); + if (terminal?.note) context.finalFailureSignature = terminal.note; } } catch { // Failure to load history is non-fatal; we still surface with the rest. } - if (!historyLoaded && typeof totalAttempts === "number") { - context.totalAttempts = totalAttempts; - } - return context; } @@ -800,12 +881,14 @@ export async function reclaimStalePrFixHandouts( continue; } - // PR is open. Past the reclaim cap, hand the item to a human. + // PR is open. Past the reclaim cap, PARK the item (#1218): repeated + // unacknowledged hand-outs are bounded no-progress exhaustion, not a + // human decision, so it must not carry the needs-human label. const reclaimCount = item.handoutReclaims ?? 0; if (reclaimCount >= maxReclaims) { - // Same ack-list pin (#1211 review): the terminal BLOCKED write + // Same ack-list pin (#1211 review): the terminal PARKED write // would otherwise wipe a freshly-acknowledged live attempt off - // NEEDS_HUMAN, never to be reclaimed again. + // the parked state, never to be reclaimed again. const acksBeforeBlock: string[] = Array.isArray(item.handoutAcks) ? item.handoutAcks : []; const { count } = await client.$transaction(async (tx) => { const result = await tx.prFixQueueItem.updateMany({ @@ -816,8 +899,10 @@ export async function reclaimStalePrFixHandouts( handoutAcks: { equals: acksBeforeBlock }, }, data: { - status: "BLOCKED", - lane: "NEEDS_HUMAN", + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + decisionDetail: null, agentHandouts: [], handoutAcks: [], dispatchedGeneration: null, @@ -829,9 +914,10 @@ export async function reclaimStalePrFixHandouts( data: { itemId: item.id, action: "mark", - status: "BLOCKED", - lane: "NEEDS_HUMAN", - note: `Hand-out reclaimed ${reclaimCount} times without acknowledgement (PR_FIX_MAX_RECLAIMS=${maxReclaims}); routed to a human (#1211).`, + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + note: `Hand-out reclaimed ${reclaimCount} times without acknowledgement (PR_FIX_MAX_RECLAIMS=${maxReclaims}); parked (#1211).`, }, }); } @@ -942,22 +1028,29 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque snapshot.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 = snapshot.status === "STALE" || snapshot.status === "IGNORED"; + // Human-decision BLOCKED and operational PARKED states are sticky too + // (#1218): a fresh enqueue must not auto-reopen either. Only an + // explicit operator requeue (or a mark to QUEUED) releases them. A + // FIXED tombstone keeps its existing reopen paths (#940 + new evidence). + const isTerminalStatus = + snapshot.status === "STALE" || + snapshot.status === "IGNORED" || + snapshot.status === "BLOCKED" || + snapshot.status === "PARKED"; const nextEvidenceKeys = uniqueAppend(snapshot.evidenceKeys ?? [], input.evidenceKey, 40); let resolvedStatus: PrFixStatus; let resolvedLane: PrFixLane = lane; let statusNote: string | null = null; + let resolvedOutcome: PrFixOutcome | null = null; + let resolvedDecision: PrFixDecisionContext | null = null; if (isTerminalStatus) { resolvedStatus = snapshot.status; // sticky — never resurrect a gone PR + // Preserve the stored lane: a sticky BLOCKED must stay NEEDS_HUMAN + // and a sticky PARKED must stay NORMAL, regardless of the enqueue's + // lane argument (#1218). + resolvedLane = snapshot.lane ?? resolvedLane; } else if (reopenFixStale) { resolvedStatus = nextStatus; // #940 recovery from a no-progress FIXED tombstone } else if (isKnownEvidence) { @@ -969,18 +1062,31 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque // Bound the fix loop (#1001). Only a transition back to QUEUED opens a // new attempt — more evidence on already-QUEUED work (the rest of one // review's inline comments) is the same attempt and never counts - // (#1103). 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. + // (#1103). Past the cap, stop re-queuing and PARK the PR (#1218): this + // is bounded exhaustion, not a human decision, so it must not carry the + // needs-human label. Otherwise a human CHANGES_REQUESTED that N + // automated fixes never satisfy loops forever. const priorAttempts = snapshot.fixAttempts ?? 1; if ( resolvedStatus === "QUEUED" && snapshot.status !== "QUEUED" && priorAttempts >= maxPrFixAttempts() ) { - resolvedStatus = "BLOCKED"; - resolvedLane = "NEEDS_HUMAN"; - statusNote = `Bounded at ${priorAttempts} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); routed to a human instead of re-queuing (#1001).`; + resolvedStatus = "PARKED"; + resolvedLane = "NORMAL"; + resolvedOutcome = normalizePrFixOutcome(input.outcome) ?? "no_progress"; + statusNote = `Bounded at ${priorAttempts} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked instead of re-queuing (#1001).`; + } + + if (resolvedStatus === "BLOCKED") { + resolvedOutcome = "human_decision"; + resolvedDecision = input.decision ?? snapshot.decisionDetail ?? null; + } else if (resolvedStatus === "PARKED") { + resolvedOutcome = + resolvedOutcome ?? + normalizePrFixOutcome(input.outcome) ?? + normalizePrFixOutcome(snapshot.outcome) ?? + "no_progress"; } // A transition from a non-QUEUED status back to QUEUED is a fresh @@ -1054,6 +1160,11 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque ...(isFreshAttempt ? { postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } : {}), + // Structured outcome (#1218): recorded on the same write as the + // status. Only BLOCKED/PARKED carry one; decisionDetail is persisted + // only for a human-decision BLOCKED. + ...(resolvedOutcome ? { outcome: resolvedOutcome } : {}), + ...(resolvedStatus === "BLOCKED" && resolvedDecision ? { decisionDetail: resolvedDecision } : {}), ...metadataPatch(input), }; @@ -1061,6 +1172,7 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque data, statusNote, reopenFixStale, + outcome: resolvedOutcome, // #1134: the append above is computed from THIS snapshot's key list, // so the write must be pinned on exactly that list. The // isFreshAttempt reset-to-[] is mutually exclusive with the append @@ -1133,6 +1245,7 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque reason: input.reason, evidenceKey: input.evidenceKey, }; + if (decided.outcome) historyData.outcome = decided.outcome; if (decided.reopenFixStale) { historyData.note = `Reopened: PR head SHA unchanged since FIXED (${snapshot.headSha}); re-detected evidence on a no-progress tombstone (#940).`; } else if (decided.statusNote) { @@ -1158,20 +1271,50 @@ export async function enqueuePrFixItem(client: PrFixQueueClient, rawInput: Enque // A brand-new item is a fresh attempt: capture the head the sync // observed now as its immutable per-attempt baseline (#1074). attemptHeadSha: input.headSha ?? null, + // A brand-new NEEDS_HUMAN enqueue is a genuine human decision (#1218). + ...(nextStatus === "BLOCKED" + ? { outcome: "human_decision", ...(input.decision ? { decisionDetail: input.decision } : {}) } + : {}), ...metadataPatch(input), }, }); await tx.prFixHistory.create({ - data: { itemId: created.id, action: "enqueue", lane: created.lane, reason: input.reason, evidenceKey: input.evidenceKey }, + data: { + itemId: created.id, + action: "enqueue", + lane: created.lane, + reason: input.reason, + evidenceKey: input.evidenceKey, + ...(nextStatus === "BLOCKED" ? { outcome: "human_decision" } : {}), + }, }); return created; }); - if (previousStatus !== "BLOCKED" && item.status === "BLOCKED") { + const wasBlocked = previousStatus === "BLOCKED"; + const wasParked = previousStatus === "PARKED"; + if (item.status === "BLOCKED" && !wasBlocked) { + const context = await buildPrFixBlockedContext(client, item); + await surfacePrFixBlocked({ + repo: input.repo, + pr: input.pr, + reason: item.reason, + latestNote: null, + context, + decision: item.decisionDetail ?? input.decision ?? null, + }); + } else if (item.status === "PARKED" && !wasParked) { const context = await buildPrFixBlockedContext(client, item); - await surfacePrFixBlocked({ repo: input.repo, pr: input.pr, reason: item.reason, latestNote: null, context }); - } else if (previousStatus === "BLOCKED" && item.status === "QUEUED") { - await retractNeedsHuman(input.repo, input.pr, "requeued", "New evidence reopened this item."); + await surfacePrFixParked({ + repo: input.repo, + pr: input.pr, + reason: item.reason, + outcome: item.outcome ?? "no_progress", + latestNote: null, + context, + }); + } else if ((wasBlocked || wasParked) && (item.status === "QUEUED" || item.status === "FIXED")) { + await retractNeedsHuman(input.repo, input.pr, item.status === "FIXED" ? "resolved" : "requeued", "New evidence reopened this item."); } return item; } @@ -1241,7 +1384,7 @@ export async function createLinkedPrFixItem( export async function listQueuedPrFixItems(client: PrFixQueueClient, options: { lane?: string | null; includeBlocked?: boolean; prioritizeByType?: boolean } = {}) { const lane = options.lane ? normalizePrFixLane(options.lane) : undefined; - const status = options.includeBlocked ? { in: ["QUEUED", "BLOCKED"] } : "QUEUED"; + const status = options.includeBlocked ? { in: ["QUEUED", "BLOCKED", "PARKED"] } : "QUEUED"; const items = await client.prFixQueueItem.findMany({ where: { status, ...(lane ? { lane } : {}) }, @@ -1361,9 +1504,10 @@ export async function markPrFixItem( const existing = await client.prFixQueueItem.findUnique({ where: { repo_pr: { repo: input.repo, pr: input.pr } } }); if (!existing) return { mutated: false, reason: "not-found" }; - // Give-up: BLOCKED items always land in NEEDS_HUMAN so the existing red - // badge actually means something and so the bridge's ACTIONABLE_LANES - // filter continues to skip them. See bridge/prfix.py ACTIONABLE_LANES. + // BLOCKED is reserved for a genuine human decision (#1218): lane NEEDS_HUMAN, + // outcome human_decision, surfaced with the needs-human label + marker. PARKED + // is bounded exhaustion or an operational failure: lane NORMAL, no + // needs-human label. The bridge's ACTIONABLE_LANES filter skips BLOCKED. const data: Record = { status: nextStatus }; // Reopen a settlement as a fresh attempt on post-dispatch evidence (#1119): @@ -1379,13 +1523,23 @@ export async function markPrFixItem( const writeReopen = async (tx: any, row: any, noteSuffix = ""): Promise => { const reopenCapped = (row.fixAttempts ?? 1) >= maxPrFixAttempts(); const reopenData: Record = reopenCapped - ? { status: "BLOCKED", lane: "NEEDS_HUMAN", postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } + ? { + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + decisionDetail: null, + postDispatchEvidenceKeys: [], + agentHandouts: [], + handoutAcks: [], + } : { status: "QUEUED", lane: "NORMAL", ...freshAttemptGeneration(), fixAttempts: { increment: 1 }, attemptHeadSha: row.headSha ?? row.attemptHeadSha ?? null, + outcome: null, + decisionDetail: null, postDispatchEvidenceKeys: [], // Fresh attempt: per-agent hand-out records for the consumed // generation must not suppress its re-dispatch (#1133), and the @@ -1408,9 +1562,10 @@ export async function markPrFixItem( action: "mark", status: reopenData.status, lane: reopenData.lane, + outcome: reopenCapped ? "no_progress" : null, note: (reopenCapped - ? `New evidence arrived after this attempt was handed out, but bounded at ${row.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); routed to a human (#1119).` + ? `New evidence arrived after this attempt was handed out, but bounded at ${row.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked (#1119).` : `New evidence arrived after this attempt was handed out; reopened as a fresh attempt at the current head (#1119).`) + noteSuffix, }, }); @@ -1419,9 +1574,16 @@ export async function markPrFixItem( const surfaceReopen = async (before: any, reopened: any) => { const reopenCapped = (before.fixAttempts ?? 1) >= maxPrFixAttempts(); - if (reopenCapped && before.status !== "BLOCKED") { + if (reopenCapped && before.status !== "BLOCKED" && before.status !== "PARKED") { const context = await buildPrFixBlockedContext(client, reopened); - await surfacePrFixBlocked({ repo: input.repo, pr: input.pr, reason: reopened.reason, latestNote: input.note ?? null, context }); + await surfacePrFixParked({ + repo: input.repo, + pr: input.pr, + reason: reopened.reason, + outcome: "no_progress", + latestNote: input.note ?? null, + context, + }); } else if (!reopenCapped) { // Leaving (a would-be) settlement back to a live QUEUED attempt: make sure a // needs-human marker is not left behind and surface the new attempt (#1119). @@ -1429,15 +1591,15 @@ export async function markPrFixItem( } }; - // A settlement (FIXED/BLOCKED) on an item whose post-dispatch evidence list - // still holds an actionable entry must NOT absorb that evidence: reopen as a - // fresh dispatchable attempt so a worker actually runs on it (#1119). + // A settlement (FIXED/BLOCKED/PARKED) on an item whose post-dispatch evidence + // list still holds an actionable entry must NOT absorb that evidence: reopen + // as a fresh dispatchable attempt so a worker actually runs on it (#1119). // Evaluated only for settlements, so a non-settle mark never triggers a head - // fetch. A capped item still gives up to a human, matching the other + // fetch. A capped item still gives up (PARKED, #1218), matching the other // fresh-attempt caps. The head fetch inside the actionability check always // runs OUTSIDE any transaction: Prisma's ~5s interactive-transaction // timeout must never have to span a GitHub round-trip (#1124 review). - const settlesAttempt = nextStatus === "FIXED" || nextStatus === "BLOCKED"; + const settlesAttempt = nextStatus === "FIXED" || nextStatus === "BLOCKED" || nextStatus === "PARKED"; const reopenOnPostDispatch = settlesAttempt && (await hasActionablePostDispatchEvidence(existing.postDispatchEvidenceKeys ?? [], input.repo, input.pr)); @@ -1457,8 +1619,18 @@ export async function markPrFixItem( if (nextStatus === "BLOCKED") { data.lane = "NEEDS_HUMAN"; + data.outcome = "human_decision"; + if (input.decision) data.decisionDetail = input.decision; + } else if (nextStatus === "PARKED") { + data.lane = "NORMAL"; + data.outcome = resolvePrFixOutcome("PARKED", input.outcome); + data.decisionDetail = null; } else if (nextStatus === "QUEUED") { data.lane = "NORMAL"; + // A mark back to QUEUED releases a BLOCKED/PARKED item: clear the outcome + // and human-decision payload (#1218). + data.outcome = null; + data.decisionDetail = null; // Marking a non-QUEUED item back to QUEUED creates a fresh dispatchable // attempt (same semantics as requeuePrFixItem) — bump the generation // alongside the status flip so the work identity changes (#1044). @@ -1481,6 +1653,13 @@ export async function markPrFixItem( } else if (nextStatus === "STALE" || nextStatus === "IGNORED") { // Terminal: the recorded entries are moot; clear them (#1119). data.postDispatchEvidenceKeys = []; + data.outcome = null; + data.decisionDetail = null; + } else { + // FIXED (and any other status): a settled item carries no BLOCKED/PARKED + // classification. + data.outcome = null; + data.decisionDetail = null; } // No-progress guard (#940, rebuilt in #1074): run it BEFORE any write, @@ -1499,18 +1678,28 @@ export async function markPrFixItem( // why we rejected. No FIXED row is ever written. // // The refused run was a spent attempt, so it counts toward the cap; - // past it, hand the PR to a human instead of re-running a worker that - // keeps pushing nothing (#1103). The retry keeps the refused baseline: - // the head is unchanged, so it is still the newest observed (#1104). + // past it, PARK the PR instead of re-running a worker that keeps pushing + // nothing (#1103, #1218). The retry keeps the refused baseline: the head + // is unchanged, so it is still the newest observed (#1104). const refusalCapped = (existing.fixAttempts ?? 1) >= maxPrFixAttempts(); const refusalData: Record = refusalCapped - ? { status: "BLOCKED", lane: "NEEDS_HUMAN", postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } + ? { + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + decisionDetail: null, + postDispatchEvidenceKeys: [], + agentHandouts: [], + handoutAcks: [], + } : { status: "QUEUED", lane: "NORMAL", ...freshAttemptGeneration(), fixAttempts: { increment: 1 }, attemptHeadSha: input.attemptHeadSha ?? baseline ?? null, + outcome: null, + decisionDetail: null, // The generation bump invalidates any recorded entries and proves // no stale post-dispatch list on the fresh attempt (#1119). postDispatchEvidenceKeys: [], @@ -1535,18 +1724,26 @@ export async function markPrFixItem( action: "mark", status: refusalData.status, lane: refusalData.lane, + outcome: refusalCapped ? "no_progress" : null, note: refusalCapped - ? `${refusedNote} Bounded at ${existing.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); routed to a human instead of re-queuing (#1103).` + ? `${refusedNote} Bounded at ${existing.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked instead of re-queuing (#1103).` : refusedNote, }, }); return tx.prFixQueueItem.findUnique({ where: { id: existing.id } }); }); if (!refusal) return { mutated: false, reason: "generation-mismatch" }; - if (refusalCapped && existing.status !== "BLOCKED") { + if (refusalCapped && existing.status !== "BLOCKED" && existing.status !== "PARKED") { const context = await buildPrFixBlockedContext(client, refusal); - await surfacePrFixBlocked({ repo: input.repo, pr: input.pr, reason: refusal.reason, latestNote: input.note ?? null, context }); - } else if (!refusalCapped && existing.status === "BLOCKED") { + await surfacePrFixParked({ + repo: input.repo, + pr: input.pr, + reason: refusal.reason, + outcome: "no_progress", + latestNote: input.note ?? null, + context, + }); + } else if (!refusalCapped && (existing.status === "BLOCKED" || existing.status === "PARKED")) { await retractNeedsHuman(input.repo, input.pr, "requeued", input.note); } return { mutated: true, item: refusal }; @@ -1556,12 +1753,32 @@ export async function markPrFixItem( // Shared tail for every successful mark write — settlement and non-settle // marks alike. const finishMarked = async (row: any): Promise => { - // Only surface a blocked notification when THIS mark is what first puts - // the item into BLOCKED (from a non-BLOCKED status). + // Only surface a terminal notification when THIS mark is what first puts + // the item into BLOCKED/PARKED (from a different status). if (nextStatus === "BLOCKED" && existing.status !== "BLOCKED") { const context = await buildPrFixBlockedContext(client, row); - await surfacePrFixBlocked({ repo: input.repo, pr: input.pr, reason: row.reason, latestNote: input.note ?? null, context }); - } else if (existing.status === "BLOCKED" && (nextStatus === "QUEUED" || nextStatus === "FIXED")) { + await surfacePrFixBlocked({ + repo: input.repo, + pr: input.pr, + reason: row.reason, + latestNote: input.note ?? null, + context, + decision: row.decisionDetail ?? input.decision ?? null, + }); + } else if (nextStatus === "PARKED" && existing.status !== "PARKED") { + const context = await buildPrFixBlockedContext(client, row); + await surfacePrFixParked({ + repo: input.repo, + pr: input.pr, + reason: row.reason, + outcome: row.outcome ?? resolvePrFixOutcome("PARKED", input.outcome) ?? "no_progress", + latestNote: input.note ?? null, + context, + }); + } else if ( + (existing.status === "BLOCKED" || existing.status === "PARKED") && + (nextStatus === "QUEUED" || nextStatus === "FIXED") + ) { await retractNeedsHuman(input.repo, input.pr, nextStatus === "FIXED" ? "resolved" : "requeued", input.note); } return { mutated: true, item: row }; @@ -1592,7 +1809,14 @@ export async function markPrFixItem( if (count !== 1) return null; const row = await tx.prFixQueueItem.findUnique({ where: { id: existing.id } }); await tx.prFixHistory.create({ - data: { itemId: row.id, action: "mark", status: nextStatus, lane: row.lane, note: input.alreadyAddressed ? alreadyAddressedHistoryNote(input) : input.note ?? undefined }, + data: { + itemId: row.id, + action: "mark", + status: nextStatus, + lane: row.lane, + outcome: row.outcome ?? resolvePrFixOutcome(nextStatus, input.outcome), + note: input.alreadyAddressed ? alreadyAddressedHistoryNote(input) : input.note ?? undefined, + }, }); return row; }); @@ -1659,6 +1883,7 @@ export async function markPrFixItem( action: "mark", status: nextStatus, lane: row.lane, + outcome: row.outcome ?? resolvePrFixOutcome(nextStatus, input.outcome), note: [ input.alreadyAddressed ? alreadyAddressedHistoryNote(input) : input.note, "Recorded post-dispatch evidence no longer applies (intermediate head); cleared (#1119).", @@ -1682,13 +1907,23 @@ export async function markPrFixItem( // WITHOUT the keys pin, so racing evidence cannot block it. const forceCapped = (lastFresh.fixAttempts ?? 1) >= maxPrFixAttempts(); const forceData: Record = forceCapped - ? { status: "BLOCKED", lane: "NEEDS_HUMAN", postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } + ? { + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + decisionDetail: null, + postDispatchEvidenceKeys: [], + agentHandouts: [], + handoutAcks: [], + } : { status: "QUEUED", lane: "NORMAL", ...freshAttemptGeneration(), fixAttempts: { increment: 1 }, attemptHeadSha: lastFresh.headSha ?? lastFresh.attemptHeadSha ?? null, + outcome: null, + decisionDetail: null, postDispatchEvidenceKeys: [], // Fresh attempt: per-agent hand-out records for the consumed // generation must not suppress its re-dispatch (#1133), and the @@ -1711,12 +1946,13 @@ export async function markPrFixItem( action: "mark", status: forceData.status, lane: forceData.lane, + outcome: forceCapped ? "no_progress" : null, // Preserve the caller's note alongside the canned forced-fallback text // (same join style as the stale-clear settle branch). note: [ input.note, forceCapped - ? `Post-dispatch evidence kept landing while this settlement was in flight, but bounded at ${lastFresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); routed to a human (#1119).` + ? `Post-dispatch evidence kept landing while this settlement was in flight, but bounded at ${lastFresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked (#1119).` : `Post-dispatch evidence kept landing while this settlement was in flight; forced a fresh attempt so the racing evidence is not absorbed (#1119).`, ] .filter(Boolean) @@ -1762,7 +1998,7 @@ export async function reconcileStalePrFixItems( where: { repo, pr: { in: Array.from(prNumbers) }, - status: { in: ["QUEUED", "BLOCKED"] }, + status: { in: ["QUEUED", "BLOCKED", "PARKED"] }, }, }); checked += staleCandidates.length; @@ -1823,7 +2059,7 @@ export async function reconcileArchivedRepoPrFixItems( let markedStale = 0; let errored = 0; - const active = await client.prFixQueueItem.findMany({ where: { status: { in: ["QUEUED", "BLOCKED"] } } }); + const active = await client.prFixQueueItem.findMany({ where: { status: { in: ["QUEUED", "BLOCKED", "PARKED"] } } }); const byRepo = new Map(); for (const item of active) byRepo.set(item.repo, [...(byRepo.get(item.repo) ?? []), item]); @@ -1928,6 +2164,7 @@ export function toAgentQueuePrFixItem(item: any) { title: item.title, lane: item.lane, status: item.status, + outcome: item.outcome ?? null, reason: item.reason, feedback: item.feedback ?? [], evidenceKeys: item.evidenceKeys ?? [], @@ -1971,11 +2208,11 @@ export async function requeuePrFixItem(client: PrFixQueueClient, rawInput: Reque const item = await client.$transaction(async (tx) => { const existing = await tx.prFixQueueItem.findUnique({ where: { repo_pr: { repo: input.repo, pr: input.pr } } }); if (!existing) return null; - // Requeue acts on both BLOCKED (the original case) and FIXED (recovery - // from a no-progress tombstone, #940). STALE is rejected because the - // upstream PR is gone. - if (existing.status !== "BLOCKED" && existing.status !== "FIXED") { - throw new Error(`Cannot requeue: item is ${existing.status}, not BLOCKED or FIXED`); + // Requeue acts on BLOCKED (a human decision), PARKED (bounded exhaustion + // or an operational failure, #1218) and FIXED (recovery from a no-progress + // tombstone, #940). STALE is rejected because the upstream PR is gone. + if (existing.status !== "BLOCKED" && existing.status !== "PARKED" && existing.status !== "FIXED") { + throw new Error(`Cannot requeue: item is ${existing.status}, not BLOCKED, PARKED, or FIXED`); } const reopenedFrom = existing.status; const updated = await tx.prFixQueueItem.update({ @@ -1990,6 +2227,9 @@ export async function requeuePrFixItem(client: PrFixQueueClient, rawInput: Reque data: { status: "QUEUED", lane: "NORMAL", + // The operator release clears the BLOCKED/PARKED classification (#1218). + outcome: null, + decisionDetail: null, ...freshAttemptGeneration(), fixAttempts: 1, attemptHeadSha: existing.headSha ?? null, @@ -2012,7 +2252,7 @@ export async function requeuePrFixItem(client: PrFixQueueClient, rawInput: Reque lane: "NORMAL", note: (input.note ? `${input.note} (reopened from ${reopenedFrom})` : `operator requeue from ${reopenedFrom}`) + - (reopenedFrom === "FIXED" ? " — #940 recovery" : ""), + (reopenedFrom === "FIXED" ? " — #940 recovery" : reopenedFrom === "PARKED" ? " — operator release of a parked item" : ""), }, }); return updated; @@ -2072,6 +2312,11 @@ export interface ResolvePrFixFromAgentReportInput { summary?: string | null; // #1121 evidence string (commit SHAs/paths) carried by an `already_addressed` report. evidence?: string | null; + // #1218: for a `blocked` report, the structured reason. `human_decision` + // marks BLOCKED with the decision payload; anything else (or a bare + // give-up) marks PARKED — a bare give-up is NOT a human decision. + blockReason?: string | null; + decision?: PrFixDecisionContext | null; client?: PrFixQueueClient; // #1074: required for queue settlement. When present, the report settles // exactly the item + generation the token identifies; when absent (legacy @@ -2081,7 +2326,7 @@ export interface ResolvePrFixFromAgentReportInput { export interface ResolvePrFixFromAgentReportResult { matched: boolean; - action: "none" | "blocked" | "fixed" | "deferred" | "requeued" | "skipped"; + action: "none" | "blocked" | "parked" | "fixed" | "deferred" | "requeued" | "skipped"; itemId?: number | null; reason: string; // Echoes the attempt generation the report carried (null when the report @@ -2214,12 +2459,21 @@ export async function resolvePrFixFromAgentReport( const expectedGeneration = attempt?.generation ?? undefined; if (input.outcome === "blocked") { + // #1218: a `blocked` report is a genuine human decision only when it + // carries a `human_decision` blockReason. A bare give-up (or any other + // outcome) is bounded no-progress exhaustion → PARKED, never labeled + // needs-human. + const resolved = normalizePrFixOutcome(input.blockReason) ?? "no_progress"; + const isHumanDecision = resolved === "human_decision"; const markResult = await markPrFixItem(client as PrFixQueueClient, { repo, pr, - status: "BLOCKED", + status: isHumanDecision ? "BLOCKED" : "PARKED", note: input.summary ?? null, expectedGeneration, + ...(isHumanDecision + ? { outcome: "human_decision", ...(input.decision ? { decision: input.decision } : {}) } + : { outcome: resolved }), }); if (!markResult.mutated) { return { @@ -2232,9 +2486,9 @@ export async function resolvePrFixFromAgentReport( } return { matched: true, - action: "blocked", + action: isHumanDecision ? "blocked" : "parked", itemId: existing.id ?? null, - reason: "agent reported blocked", + reason: isHumanDecision ? "agent reported blocked (human decision)" : `agent reported blocked; parked (${resolved})`, attemptGeneration: attempt?.generation ?? null, }; } @@ -2276,13 +2530,23 @@ export async function resolvePrFixFromAgentReport( if (expectedGeneration !== undefined && fresh.generation !== expectedGeneration) break; const capped = (fresh.fixAttempts ?? 1) >= maxPrFixAttempts(); const data: Record = capped - ? { status: "BLOCKED", lane: "NEEDS_HUMAN", postDispatchEvidenceKeys: [], agentHandouts: [], handoutAcks: [] } + ? { + status: "PARKED", + lane: "NORMAL", + outcome: "no_progress", + decisionDetail: null, + postDispatchEvidenceKeys: [], + agentHandouts: [], + handoutAcks: [], + } : { status: "QUEUED", lane: "NORMAL", ...freshAttemptGeneration(), fixAttempts: { increment: 1 }, attemptHeadSha: fresh.headSha ?? fresh.attemptHeadSha ?? null, + outcome: null, + decisionDetail: null, postDispatchEvidenceKeys: [], // Fresh attempt: the consumed generation's hand-out records must // not suppress the item's re-dispatch (#1133), and its @@ -2311,10 +2575,11 @@ export async function resolvePrFixFromAgentReport( action: "mark", status: data.status, lane: data.lane, + outcome: capped ? "no_progress" : null, note: [ input.summary ?? null, capped - ? `Agent reported failed; bounded at ${fresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}), routed to a human instead of re-queuing (#1133).` + ? `Agent reported failed; bounded at ${fresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}), parked instead of re-queuing (#1133).` : `Agent reported failed; reopened as a fresh attempt so the consumed generation cannot strand the queue (#1133).`, ] .filter(Boolean) @@ -2339,16 +2604,23 @@ export async function resolvePrFixFromAgentReport( attemptGeneration: attempt?.generation ?? null, }; } - if (settled.capped && existing.status !== "BLOCKED") { + if (settled.capped && existing.status !== "BLOCKED" && existing.status !== "PARKED") { const context = await buildPrFixBlockedContext(client, settled.row); - await surfacePrFixBlocked({ repo, pr, reason: settled.row.reason, latestNote: input.summary ?? null, context }); + await surfacePrFixParked({ + repo, + pr, + reason: settled.row.reason, + outcome: "no_progress", + latestNote: input.summary ?? null, + context, + }); } return { matched: true, - action: settled.capped ? "blocked" : "requeued", + action: settled.capped ? "parked" : "requeued", itemId: existing.id ?? null, reason: settled.capped - ? "agent reported failed; past the fix-attempt cap, routed to a human" + ? "agent reported failed; past the fix-attempt cap, parked" : "agent reported failed; reopened as a fresh attempt at the next generation", attemptGeneration: attempt?.generation ?? null, }; diff --git a/src/lib/pr-fix-surfacing.test.ts b/src/lib/pr-fix-surfacing.test.ts index 82927a8c..86258c61 100644 --- a/src/lib/pr-fix-surfacing.test.ts +++ b/src/lib/pr-fix-surfacing.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi, beforeEach } from "vitest"; -import { buildNeedsHumanComment, surfacePrFixBlocked, surfacePrFixRequeued, surfacePrFixUnblocked, NEEDS_HUMAN_LABEL, NEEDS_HUMAN_COMMENT_MARKER } from "./pr-fix-surfacing"; +import { buildNeedsHumanComment, buildParkedComment, surfacePrFixBlocked, surfacePrFixParked, surfacePrFixRequeued, surfacePrFixUnblocked, NEEDS_HUMAN_LABEL, NEEDS_HUMAN_COMMENT_MARKER, PARKED_COMMENT_MARKER } from "./pr-fix-surfacing"; const { mocks } = vi.hoisted(() => ({ mocks: { @@ -50,6 +50,29 @@ describe("buildNeedsHumanComment", () => { const body = buildNeedsHumanComment(baseInput); expect(body).toMatch(/Posted automatically by Dispatch on \d{4}-\d{2}-\d{2}T/); }); + + it("renders the decision question, options, and evidence when present (#1218)", () => { + const body = buildNeedsHumanComment({ + ...baseInput, + decision: { + question: "Should we keep the legacy endpoint?", + options: ["Keep it", "Remove it"], + evidence: "Breaking change in the public API.", + }, + }); + expect(body).toContain("**Decision needed:** Should we keep the legacy endpoint?"); + expect(body).toContain("**Options:**"); + expect(body).toContain("- Keep it"); + expect(body).toContain("- Remove it"); + expect(body).toContain("**Evidence:** Breaking change in the public API."); + }); + + it("omits the decision block when absent", () => { + const body = buildNeedsHumanComment(baseInput); + expect(body).not.toContain("Decision needed"); + expect(body).not.toContain("**Options:**"); + expect(body).not.toContain("**Evidence:**"); + }); }); describe("surfacePrFixBlocked", () => { @@ -262,37 +285,132 @@ describe("surfacePrFixBlocked — terminal PR guard", () => { }); describe("buildNeedsHumanComment — richer context", () => { - it("renders totalAttempts, attemptsByLane, finalFailure, run links, and last attempt", () => { + it("renders workerExecutions and enqueueRows separately, never as 'attempts' (#1218)", () => { const body = buildNeedsHumanComment({ ...baseInput, context: { - totalAttempts: 3, + workerExecutions: 3, + enqueueRows: 7, attemptsByLane: { NORMAL: 2, ESCALATED: 1 }, finalFailureSignature: "tests failed after 3 attempts", failingRunLinks: ["https://github.com/org/repo/actions/runs/42"], lastAttemptSummary: "final attempt still failing", + // Retained for back-compat but must NOT be rendered as "attempts". + totalAttempts: 99, }, }); - expect(body).toContain("**Total attempts:** 3"); - expect(body).toContain("**NORMAL:** 2 attempts"); - expect(body).toContain("**ESCALATED:** 1 attempt"); + expect(body).toContain("**Worker executions:** 3"); + expect(body).toContain("**Enqueue rows:** 7"); + expect(body).toContain("**Enqueues by lane:**"); + expect(body).toContain("**NORMAL:** 2"); + expect(body).toContain("**ESCALATED:** 1"); expect(body).toContain("**Final failure:** tests failed after 3 attempts"); expect(body).toContain("https://github.com/org/repo/actions/runs/42"); expect(body).toContain("**Last attempt:** final attempt still failing"); + // The legacy totalAttempts is never rendered as "attempts". + expect(body).not.toContain("Total attempts"); + expect(body).not.toContain("99"); }); it("omits context sections that are absent (backwards compatible)", () => { const body = buildNeedsHumanComment(baseInput); - expect(body).not.toContain("Total attempts"); - expect(body).not.toContain("Attempts by lane"); + expect(body).not.toContain("Worker executions"); + expect(body).not.toContain("Enqueue rows"); + expect(body).not.toContain("Enqueues by lane"); expect(body).not.toContain("Final failure"); expect(body).not.toContain("Last attempt"); }); it("omits empty attemptsByLane and no-op context", () => { const body = buildNeedsHumanComment({ ...baseInput, context: { attemptsByLane: {} } }); - expect(body).not.toContain("Attempts by lane"); + expect(body).not.toContain("Enqueues by lane"); + }); +}); + +describe("buildParkedComment (#1218)", () => { + it("includes the parked marker, reason, and outcome", () => { + const body = buildParkedComment({ ...baseInput, reason: "bounded at 5 attempts", outcome: "no_progress" }); + expect(body).toContain(PARKED_COMMENT_MARKER); + expect(body).toContain("**Reason:** bounded at 5 attempts"); + expect(body).toContain("**Outcome:** no_progress"); + expect(body).not.toContain(NEEDS_HUMAN_COMMENT_MARKER); + expect(body).toMatch(/Posted automatically by Dispatch on \d{4}-\d{2}-\d{2}T/); + }); + + it("defaults a missing/unknown outcome to no_progress", () => { + expect(buildParkedComment(baseInput)).toContain("**Outcome:** no_progress"); + expect(buildParkedComment({ ...baseInput, outcome: "nonsense" })).toContain("**Outcome:** no_progress"); + }); + + it("renders infra_failure and security_failure headlines", () => { + expect(buildParkedComment({ ...baseInput, outcome: "infra_failure" })).toContain("infrastructure or authentication failure"); + expect(buildParkedComment({ ...baseInput, outcome: "security_failure" })).toContain("security failure"); + }); + + it("renders metrics and context without the needs-human claim", () => { + const body = buildParkedComment({ + ...baseInput, + outcome: "no_progress", + context: { workerExecutions: 5, enqueueRows: 12, lastAttemptSummary: "still failing" }, + }); + expect(body).toContain("**Worker executions:** 5"); + expect(body).toContain("**Enqueue rows:** 12"); + expect(body).toContain("**Last attempt:** still failing"); + expect(body).not.toContain("needs human attention"); + }); +}); + +describe("surfacePrFixParked (#1218)", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.addIssueLabel.mockResolvedValue(undefined); + mocks.addIssueComment.mockResolvedValue({ url: null }); + mocks.updateIssueComment.mockResolvedValue(undefined); + mocks.removeIssueLabel.mockResolvedValue(undefined); + mocks.fetchIssueComments.mockResolvedValue([]); + mocks.fetchPullRequestState.mockResolvedValue({ state: "open", mergedAt: null }); + }); + + it("posts a parked comment and applies NO label", async () => { + const result = await surfacePrFixParked({ ...baseInput, outcome: "no_progress" }); + expect(mocks.addIssueComment).toHaveBeenCalledWith( + "org/repo", + 42, + expect.stringContaining(PARKED_COMMENT_MARKER), + ); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + expect(result.commentPosted).toBe(true); + expect(result.errors).toEqual([]); + }); + + it("updates an existing needs-human marker comment in place (single marker)", async () => { + mocks.fetchIssueComments.mockResolvedValue([ + { id: 11, body: `${NEEDS_HUMAN_COMMENT_MARKER}\n> old block` }, + ]); + const result = await surfacePrFixParked({ ...baseInput, outcome: "no_progress" }); + expect(mocks.addIssueComment).not.toHaveBeenCalled(); + expect(result.commentUpdated).toBe(true); + expect(mocks.updateIssueComment).toHaveBeenCalledWith( + "org/repo", + 11, + expect.stringContaining(PARKED_COMMENT_MARKER), + ); + }); + + it("writes nothing on a terminal PR", async () => { + mocks.fetchPullRequestState.mockResolvedValue({ state: "closed", mergedAt: "2026-05-14T22:00:19Z" }); + const result = await surfacePrFixParked({ ...baseInput, outcome: "no_progress" }); + expect(result.skippedTerminal).toBe(true); + expect(mocks.addIssueComment).not.toHaveBeenCalled(); + expect(mocks.addIssueLabel).not.toHaveBeenCalled(); + }); + + it("never throws when the comment lookup fails", async () => { + mocks.fetchIssueComments.mockRejectedValue(new Error("GitHub API error: 502")); + const result = await surfacePrFixParked({ ...baseInput, outcome: "no_progress" }); + expect(result.commentPosted).toBe(false); + expect(result.errors).toHaveLength(1); }); }); @@ -392,4 +510,17 @@ describe("surfacePrFixUnblocked (#1105)", () => { await surfacePrFixUnblocked("org/repo", 42, "requeued"); expect(mocks.updateIssueComment.mock.calls[0][2]).toContain("**requeued**"); }); + + it("detects and folds a PARKED marker, preserving it (#1218)", async () => { + mocks.fetchIssueComments.mockResolvedValue([ + { id: 8, body: `${PARKED_COMMENT_MARKER}\n> parked after 5 attempts` }, + ]); + const result = await surfacePrFixUnblocked("org/repo", 42, "requeued", "operator released"); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, NEEDS_HUMAN_LABEL); + const body = mocks.updateIssueComment.mock.calls[0][2] as string; + expect(body.startsWith(PARKED_COMMENT_MARKER)).toBe(true); + expect(body).toContain("**requeued**"); + expect(body).toContain("operator released"); + expect(result).toMatchObject({ labelRemoved: true, commentUpdated: true, errors: [] }); + }); }); diff --git a/src/lib/pr-fix-surfacing.ts b/src/lib/pr-fix-surfacing.ts index e52980d7..84d20149 100644 --- a/src/lib/pr-fix-surfacing.ts +++ b/src/lib/pr-fix-surfacing.ts @@ -6,14 +6,25 @@ import { removeIssueLabel, updateIssueComment, } from "@/lib/github"; +import { normalizePrFixOutcome, type PrFixDecisionContext, type PrFixOutcome } from "@/types"; export const NEEDS_HUMAN_LABEL = "needs-human"; export const NEEDS_HUMAN_COMMENT_MARKER = ""; +export const PARKED_COMMENT_MARKER = ""; export interface PrFixSurfaceContext { - /** Total number of fix attempts observed, when known. */ - totalAttempts?: number | null; - /** Fix attempts grouped by lane, keyed by lane name (or "unknown"). */ + /** + * Dispatchable worker executions for this item (`PrFixQueueItem.fixAttempts`). + * This — not the enqueue/feedback row count — is the number that actually + * bounds the loop. + */ + workerExecutions?: number | null; + /** + * Number of `enqueue` history rows (evidence observations), reported + * separately from worker executions so the two are never conflated (#1218). + */ + enqueueRows?: number | null; + /** Enqueue rows grouped by lane, keyed by lane name (or "unknown"). */ attemptsByLane?: Record; /** Concise description of the final failure once the loop gave up. */ finalFailureSignature?: string | null; @@ -21,6 +32,8 @@ export interface PrFixSurfaceContext { failingRunLinks?: string[]; /** Summary/context from the last attempt, when any data is available. */ lastAttemptSummary?: string | null; + /** Retained for back-compat; do NOT render it as "attempts" (#1218). */ + totalAttempts?: number | null; } export interface SurfacePrFixBlockedInput { @@ -29,6 +42,26 @@ export interface SurfacePrFixBlockedInput { reason: string; latestNote?: string | null; context?: PrFixSurfaceContext | null; + decision?: PrFixDecisionContext | null; +} + +export interface SurfacePrFixParkedInput { + repo: string; + pr: number; + reason: string; + outcome?: string | null; + latestNote?: string | null; + context?: PrFixSurfaceContext | null; +} + +export interface SurfacePrFixParkedResult { + /** True when a fresh marker comment was posted. */ + commentPosted: boolean; + /** True when an existing marker comment was PATCHed in place. */ + commentUpdated: boolean; + /** True when the PR was already merged or closed, so nothing was written. */ + skippedTerminal: boolean; + errors: string[]; } export interface SurfacePrFixBlockedResult { @@ -45,9 +78,9 @@ export interface SurfacePrFixBlockedResult { function formatLaneAttempts(context: PrFixSurfaceContext): string | null { if (!context.attemptsByLane || Object.keys(context.attemptsByLane).length === 0) return null; const parts = Object.entries(context.attemptsByLane) - .map(([lane, count]) => `- **${lane}:** ${count} attempt${count === 1 ? "" : "s"}`) + .map(([lane, count]) => `- **${lane}:** ${count}`) .join("\n"); - return `\n**Attempts by lane:**\n${parts}`; + return `\n**Enqueues by lane:**\n${parts}`; } function formatFailingRunLinks(links: string[] | undefined): string | null { @@ -56,6 +89,34 @@ function formatFailingRunLinks(links: string[] | undefined): string | null { return `\n**Failing run(s):**\n${list.map((u) => `- ${u}`).join("\n")}`; } +/** + * Metrics truthfulness (#1218): report the worker-execution count and the + * enqueue/evidence row count as two distinct numbers. Neither is labelled + * "attempts" — conflating them is exactly what made exhaustion look like a + * design decision. + */ +function formatMetrics(context: PrFixSurfaceContext): string[] { + const lines: string[] = []; + if (typeof context.workerExecutions === "number") { + lines.push(`**Worker executions:** ${context.workerExecutions}`); + } + if (typeof context.enqueueRows === "number") { + lines.push(`**Enqueue rows:** ${context.enqueueRows}`); + } + return lines; +} + +function formatDecision(decision: PrFixDecisionContext | null | undefined): string[] { + if (!decision) return []; + const lines: string[] = []; + if (decision.question) lines.push("", `**Decision needed:** ${decision.question}`); + if (Array.isArray(decision.options) && decision.options.length > 0) { + lines.push("", "**Options:**", ...decision.options.map((o) => `- ${o}`)); + } + if (decision.evidence) lines.push("", `**Evidence:** ${decision.evidence}`); + return lines; +} + /** * Build the markdown comment body for a BLOCKED PR-fix item. * @@ -73,11 +134,67 @@ export function buildNeedsHumanComment(input: SurfacePrFixBlockedInput): string `**Reason:** ${input.reason}`, ]; - const totalAttempts = context.totalAttempts; - if (typeof totalAttempts === "number" && totalAttempts > 0) { - lines.push("", `**Total attempts:** ${totalAttempts}`); + for (const metric of formatMetrics(context)) lines.push("", metric); + + const laneText = formatLaneAttempts(context); + if (laneText) lines.push("", laneText); + + if (context.finalFailureSignature) { + lines.push("", `**Final failure:** ${context.finalFailureSignature}`); + } + + const runLinks = formatFailingRunLinks(context.failingRunLinks); + if (runLinks) lines.push("", runLinks); + + if (context.lastAttemptSummary) { + lines.push("", `**Last attempt:** ${context.lastAttemptSummary}`); + } + + lines.push(...formatDecision(input.decision)); + + if (input.latestNote) { + lines.push("", `**Latest note:** ${input.latestNote}`); } + lines.push( + "", + `Posted automatically by Dispatch on ${new Date().toISOString()}`, + ); + + return lines.join("\n"); +} + +const PARKED_HEADLINES: Record = { + no_progress: + "> ⏸️ This PR fix item is **PARKED** after repeated attempts without progress. It is not waiting on a human decision; an operator can requeue it to retry.", + infra_failure: + "> ⏸️ This PR fix item is **PARKED** after an infrastructure or authentication failure. An operator can requeue it once the environment is healthy.", + security_failure: + "> ⏸️ This PR fix item is **PARKED** after a security failure. An operator must investigate before requeuing.", + human_decision: + "> ⏸️ This PR fix item is **PARKED**. An operator can requeue it to retry.", +}; + +/** + * Build the markdown comment body for a PARKED PR-fix item (#1218). Mirrors + * `buildNeedsHumanComment` but carries no needs-human claim: a parked item is + * bounded exhaustion or an operational failure, not a design decision. + */ +export function buildParkedComment(input: SurfacePrFixParkedInput): string { + const context = input.context ?? {}; + const outcome = normalizePrFixOutcome(input.outcome) ?? "no_progress"; + const lines: string[] = [ + PARKED_COMMENT_MARKER, + "", + PARKED_HEADLINES[outcome], + "", + `**Reason:** ${input.reason}`, + "", + `**Outcome:** ${outcome}`, + ]; + + for (const metric of formatMetrics(context)) lines.push("", metric); + const laneText = formatLaneAttempts(context); if (laneText) lines.push("", laneText); @@ -123,8 +240,21 @@ function isTerminalState(prState: { state: string | null; mergedAt: string | nul return prState.state === null || prState.state === "closed" || Boolean(prState.mergedAt); } +/** + * The marker a Dispatch-owned comment starts with, if any (#1218). A BLOCKED + * item writes the needs-human marker; a PARKED item writes the parked marker. + * Both surfaces recognize EITHER marker so a re-surface (or an unblock) folds + * the single existing comment in place instead of posting a duplicate. + */ +function detectDispatchMarker(body: string | null | undefined): string | null { + const trimmed = (body ?? "").trimStart(); + if (trimmed.startsWith(NEEDS_HUMAN_COMMENT_MARKER)) return NEEDS_HUMAN_COMMENT_MARKER; + if (trimmed.startsWith(PARKED_COMMENT_MARKER)) return PARKED_COMMENT_MARKER; + return null; +} + function isDispatchMarkerComment(body: string | null | undefined): boolean { - return (body ?? "").trimStart().startsWith(NEEDS_HUMAN_COMMENT_MARKER); + return detectDispatchMarker(body) !== null; } /** @@ -202,6 +332,60 @@ export async function surfacePrFixBlocked(input: SurfacePrFixBlockedInput): Prom return result; } +/** + * Surface a PARKED PR-fix item on GitHub (#1218): post (or update in place) an + * explanatory marker comment. Unlike `surfacePrFixBlocked` it applies NO label + * — a parked item is bounded exhaustion or an operational failure, not a + * request for a human design decision. Best-effort — never throws, never writes + * to a terminal/unknown PR. + */ +export async function surfacePrFixParked(input: SurfacePrFixParkedInput): Promise { + const result: SurfacePrFixParkedResult = { + commentPosted: false, + commentUpdated: false, + skippedTerminal: false, + errors: [], + }; + + let prState: { state: string | null; mergedAt: string | null }; + try { + prState = await fetchPullRequestState(input.repo, input.pr); + } catch (error) { + console.error(`pr-fix-surfacing parked PR state error for ${input.repo}#${input.pr}:`, error); + prState = { state: null, mergedAt: null }; + } + if (isTerminalState(prState)) { + result.skippedTerminal = true; + console.warn( + `pr-fix-surfacing: skipping parked surface for ${input.repo}#${input.pr} — PR state=${prState.state ?? "unknown"}` + + `${prState.mergedAt ? ` merged=${prState.mergedAt}` : ""}; not commenting on a finished PR`, + ); + return result; + } + + try { + const existing = await fetchIssueComments(input.repo, input.pr, 100, "desc"); + const marker = existing.find((c) => isDispatchMarkerComment(c.body)); + const body = buildParkedComment(input); + + if (marker) { + if (typeof marker.id === "number") { + await updateIssueComment(input.repo, marker.id, body); + result.commentUpdated = true; + } + } else { + await addIssueComment(input.repo, input.pr, body); + result.commentPosted = true; + } + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`pr-fix-surfacing parked comment error for ${input.repo}#${input.pr}:`, error); + result.errors.push(`comment: ${message}`); + } + + return result; +} + export interface SurfacePrFixRequeuedResult { labelRemoved: boolean; commentUpdated: boolean; @@ -277,16 +461,20 @@ export async function surfacePrFixUnblocked( try { const existing = await fetchIssueComments(repo, pr, 100, "desc"); - const marker = existing.find((c) => isDispatchMarkerComment(c.body)); - if (marker && typeof marker.id === "number") { + const markerComment = existing.find((c) => isDispatchMarkerComment(c.body)); + if (markerComment && typeof markerComment.id === "number") { + // Preserve whichever marker the existing comment carried (needs-human or + // parked, #1218) so the folded notice stays recognizable and no second + // marker comment is created. + const marker = detectDispatchMarker(markerComment.body) ?? NEEDS_HUMAN_COMMENT_MARKER; const lines = [ - NEEDS_HUMAN_COMMENT_MARKER, + marker, "", UNBLOCK_HEADLINES[outcome], ]; if (notice) lines.push("", `**Note:** ${notice}`); lines.push("", `Updated automatically by Dispatch on ${new Date().toISOString()}`); - await updateIssueComment(repo, marker.id, lines.join("\n")); + await updateIssueComment(repo, markerComment.id, lines.join("\n")); result.commentUpdated = true; } } catch (error) { diff --git a/src/lib/pr-followup-ingestion.test.ts b/src/lib/pr-followup-ingestion.test.ts index bf766c29..d1a40fcc 100644 --- a/src/lib/pr-followup-ingestion.test.ts +++ b/src/lib/pr-followup-ingestion.test.ts @@ -1115,6 +1115,10 @@ describe("isInformationalComment", () => { expect(isInformationalComment("\n> ⚠️ needs human")).toBe(true); }); + it("flags dispatch's own PARKED notice (#1218)", () => { + expect(isInformationalComment("\n> ⏸️ parked after repeated attempts")).toBe(true); + }); + it("does NOT flag an ai-pr-reviewer comment even with a marker", () => { const body = ` @@ -1169,6 +1173,25 @@ describe("ingestCommentEvent — informational comments", () => { expect(client.items).toHaveLength(1); expect(client.items[0].lane).toBe("NORMAL"); }); + + it("does not enqueue dispatch's own PARKED marker comment (#1218)", async () => { + process.env.PR_FOLLOWUP_BOT_IDENTITIES = "itsmiso-ai"; + const client = makeClient(); + + const key = await ingestCommentEvent(client, { + repoFullName: "misospace/llmkube-images", + prNumber: 116, + branch: "foreman/x", + url: "https://github.com/misospace/llmkube-images/pull/116", + title: "Something", + author: "itsmiso-ai", + commentBody: "\n> ⏸️ This PR fix item is PARKED after repeated attempts. An operator can requeue it.", + commentId: "c-parked", + }); + + expect(key).toBeNull(); + expect(client.items).toHaveLength(0); + }); }); // pr-reviewer-action#467: a detailed human review — file:line references, explicit diff --git a/src/lib/pr-followup-ingestion.ts b/src/lib/pr-followup-ingestion.ts index 7481dd55..a2296bdb 100644 --- a/src/lib/pr-followup-ingestion.ts +++ b/src/lib/pr-followup-ingestion.ts @@ -12,6 +12,7 @@ import { EnqueuePrFixInput, enqueuePrFixItem, normalizeQueueRepo, PrFixQueueClie import { CONFLICTING_STATUSES, FAILURE_CONCLUSIONS } from "@/lib/linked-pr-health"; import { extractFailureMarker, extractFailureWorkflow, isScanFailure } from "@/lib/ci-failure-ingestion"; import { getConfiguredLanes, getEscalationLane } from "@/lib/lane-config"; +import { PARKED_COMMENT_MARKER } from "@/lib/pr-fix-surfacing"; // ─── Types ────────────────────────────────────────────────────────────────── @@ -208,6 +209,7 @@ export function parseAiReviewerFindings(body: string | undefined | null): AiRevi const INFORMATIONAL_COMMENT_MARKERS = [ "", + PARKED_COMMENT_MARKER, ]; /** diff --git a/src/mcp/server.ts b/src/mcp/server.ts index c6d5e17d..99bfcc62 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -378,7 +378,7 @@ export function createServer(): McpServerType { "List queued PR-fix items (CI failures, CHANGES_REQUESTED reviews, merge conflicts) awaiting a fix push. Optionally filter by lane (e.g. 'NORMAL', 'ESCALATED') or include BLOCKED items.", inputSchema: { lane: z.string().optional().describe("Filter by PR-fix lane (e.g. 'NORMAL', 'ESCALATED', 'NEEDS_HUMAN')"), - includeBlocked: z.boolean().optional().describe("Include BLOCKED (needs-human) items (default false)"), + includeBlocked: z.boolean().optional().describe("Include BLOCKED (needs-human) and PARKED (exhausted / operational failure) items (default false)"), }, }, listPrFixesHandler, diff --git a/src/types/index.ts b/src/types/index.ts index bf6ea350..4ad8a106 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -249,10 +249,42 @@ export function isValidEscalatedOutcome(outcome: string): outcome is EscalatedOu // ─── PR Review-Fix Queue Constants and Helpers ─────────────────────────────── export type PrFixLane = "NORMAL" | "ESCALATED" | "NEEDS_HUMAN"; -export type PrFixStatus = "QUEUED" | "FIXED" | "BLOCKED" | "STALE" | "IGNORED"; +export type PrFixStatus = "QUEUED" | "FIXED" | "BLOCKED" | "PARKED" | "STALE" | "IGNORED"; export const VALID_PR_FIX_LANES: PrFixLane[] = ["NORMAL", "ESCALATED", "NEEDS_HUMAN"]; -export const VALID_PR_FIX_STATUSES: PrFixStatus[] = ["QUEUED", "FIXED", "BLOCKED", "STALE", "IGNORED"]; +export const VALID_PR_FIX_STATUSES: PrFixStatus[] = ["QUEUED", "FIXED", "BLOCKED", "PARKED", "STALE", "IGNORED"]; + +// ─── PR Fix Outcome Constants ──────────────────────────────────────────────── +// +// A structured classification of WHY a PR-fix item left the active lane (#1218). +// `BLOCKED` is reserved for a genuine human decision (`human_decision`); every +// bounded retry/no-progress exhaustion and every infrastructure/auth/security +// failure is `PARKED` instead. Keeping these separate stops operational failure +// from masquerading as a design decision in the queue. +export type PrFixOutcome = "human_decision" | "no_progress" | "infra_failure" | "security_failure"; + +export const VALID_PR_FIX_OUTCOMES: PrFixOutcome[] = ["human_decision", "no_progress", "infra_failure", "security_failure"]; + +export function isValidPrFixOutcome(value: string): value is PrFixOutcome { + return VALID_PR_FIX_OUTCOMES.includes(value as PrFixOutcome); +} + +export function normalizePrFixOutcome(value?: string | null): PrFixOutcome | null { + if (!value) return null; + const normalized = value.trim().toLowerCase(); + return isValidPrFixOutcome(normalized) ? normalized : null; +} + +/** + * The human-decision payload attached to a BLOCKED item (#1218): the question + * an operator must answer, the concrete options, and any supporting evidence. + * Every field is optional so a minimally-reported decision still surfaces. + */ +export interface PrFixDecisionContext { + question?: string; + options?: string[]; + evidence?: string; +} // ─── PR Fix Type Constants ─────────────────────────────────────────────────── export type PrFixType = "MERGE_CONFLICT" | "CI_FAILURE" | "REVIEW_FEEDBACK" | "OTHER"; From 1108c02724b279ee6997ed17dad9e3618b1b93cd Mon Sep 17 00:00:00 2001 From: Courier Date: Sun, 11 Oct 2026 13:56:02 +0000 Subject: [PATCH 2/7] fix(pr-fix): address #1218 review findings - Retract the needs-human label when a BLOCKED item is later marked PARKED (surfacePrFixParked gains removeNeedsHumanLabel; finishMarked sets it on BLOCKED -> PARKED) so the label never disagrees with the parked outcome. - Add tests: infra/security outcomes through the mark surface path, PARKED sticky to automatic enqueue, and stale-generation no-ops on the reclaim and post-dispatch reopen paths. - Clarify the migration (TEXT/JSONB columns avoid the in-transaction ADD VALUE cast) and document advisory fresh-enqueue outcome semantics. --- docs/pr-fix-outcome-policy.md | 2 +- .../migration.sql | 5 + src/lib/pr-fix-queue.test.ts | 154 ++++++++++++++++++ src/lib/pr-fix-queue.ts | 3 + src/lib/pr-fix-surfacing.test.ts | 23 +++ src/lib/pr-fix-surfacing.ts | 23 +++ 6 files changed, 209 insertions(+), 1 deletion(-) diff --git a/docs/pr-fix-outcome-policy.md b/docs/pr-fix-outcome-policy.md index a0a28c75..8d9950f4 100644 --- a/docs/pr-fix-outcome-policy.md +++ b/docs/pr-fix-outcome-policy.md @@ -13,7 +13,7 @@ | `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`. +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 diff --git a/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql index da8c7332..6bdca4a3 100644 --- a/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql +++ b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql @@ -1,3 +1,8 @@ +-- 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; diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 1a79a4d3..5f96d4b3 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -3569,6 +3569,33 @@ describe("reclaim stale hand-outs (#1211)", () => { } }); + it("a concurrent re-issue no-ops the terminal PARKED write at the reclaim cap (#1218)", async () => { + const row = await seedStaleUnacked(230); + // At the reclaim cap: the sweep's terminal write would PARK this item. + row.handoutReclaims = 3; + const historyBefore = client.history.length; + // A concurrent re-issue bumps the generation between the sweep's read and + // its terminal write, so the generation pin must make the PARKED write a + // no-op rather than parking a row a newer generation now owns. + client.hooks.beforeUpdateMany = () => { + client.items[0].generation = client.items[0].generation + 1; + }; + const report = await reclaimStalePrFixHandouts(client, { + now: STALE_NOW, + maxReclaims: 3, + isPrMergedOrClosed: async () => false, + }); + + expect(report.blocked).toBe(0); + expect(report.staled).toBe(0); + expect(report.reclaimed).toBe(0); + expect(report.skipped).toBe(1); + expect(client.items[0].status).toBe("QUEUED"); + expect(client.items[0].generation).toBe(2); + expect(client.history.length).toBe(historyBefore); + expect(client.history.some((h) => h.status === "PARKED")).toBe(false); + }); + it("accumulates the reclaim bound across repeated sweeps until the cap parks the item", async () => { await seedStaleUnacked(215); // The sweep clears the stamp on each reclaim, so a further sweep only sees @@ -4322,4 +4349,131 @@ describe("PR-fix outcome separation (#1218)", () => { // PARKED is excluded from the default (QUEUED-only) listing. expect((await listQueuedPrFixItems(client)).map((i) => i.pr)).not.toContain(310); }); + + it("retracts the needs-human label when a BLOCKED item is later parked", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 311, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k311", + }); + const blocked = await markPrFixItem(client, { repo: "org/repo", pr: 311, status: "BLOCKED" }); + expect(mutatedItem(blocked).status).toBe("BLOCKED"); + // The BLOCKED transition is what applied the needs-human label. + expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + surfacingMocks.surfacePrFixParked.mockClear(); + + const parked = await markPrFixItem(client, { + repo: "org/repo", pr: 311, status: "PARKED", outcome: "no_progress", + }); + + expect(mutatedItem(parked).status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("no_progress"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ removeNeedsHumanLabel: true, outcome: "no_progress" }), + ); + // The prior surface was the BLOCKED one that applied the label. + expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); + }); + + it("does not retract the label when a QUEUED item parks directly", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 315, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k315", + }); + const parked = await markPrFixItem(client, { + repo: "org/repo", pr: 315, status: "PARKED", outcome: "no_progress", + }); + expect(mutatedItem(parked).status).toBe("PARKED"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ removeNeedsHumanLabel: false }), + ); + }); + + it("surfaces infra_failure through the mark park path without a needs-human label", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 312, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k312", + }); + const result = await markPrFixItem(client, { + repo: "org/repo", pr: 312, status: "PARKED", outcome: "infra_failure", + }); + + expect(mutatedItem(result).status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("infra_failure"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ outcome: "infra_failure" }), + ); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("surfaces security_failure through the mark park path without a needs-human label", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 313, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k313", + }); + const result = await markPrFixItem(client, { + repo: "org/repo", pr: 313, status: "PARKED", outcome: "security_failure", + }); + + expect(mutatedItem(result).status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("security_failure"); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ outcome: "security_failure" }), + ); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("does NOT reopen a PARKED item on new evidence (sticky, #1218)", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 316, lane: "NORMAL", reason: "r", feedback: "f1", + evidenceKey: "review:1", headSha: "H1", + }); + const parked = await markPrFixItem(client, { + repo: "org/repo", pr: 316, status: "PARKED", outcome: "no_progress", + }); + expect(mutatedItem(parked).status).toBe("PARKED"); + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + surfacingMocks.surfacePrFixUnblocked.mockClear(); + + // A fresh automated review lands after the park (a new evidence key). + const after = await enqueuePrFixItem(client, { + repo: "org/repo", pr: 316, lane: "NORMAL", reason: "r", feedback: "f2", + evidenceKey: "review:2", headSha: "H2", + }); + + expect(after.status).toBe("PARKED"); + expect(after.lane).toBe("NORMAL"); + expect(after.outcome).toBe("no_progress"); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("a stale generation cannot reopen a PARKED item via post-dispatch evidence", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 317, lane: "NORMAL", reason: "r", feedback: "f", + evidenceKey: "review:1", headSha: "H1", + }); + const parked = await markPrFixItem(client, { + repo: "org/repo", pr: 317, status: "PARKED", outcome: "no_progress", expectedGeneration: 1, + }); + expect(mutatedItem(parked).status).toBe("PARKED"); + // A current, actionable post-dispatch entry sits on the parked row (a + // review entry is actionable with no GitHub call). + client.items[0].postDispatchEvidenceKeys = ["review:2@H2"]; + // A concurrent re-issue bumps the row; the late worker still holds gen 1. + client.items[0].generation = 2; + const historyBefore = client.history.length; + + const result = await markPrFixItem(client, { + repo: "org/repo", pr: 317, status: "FIXED", expectedGeneration: 1, + }); + + expect(result).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0].status).toBe("PARKED"); + expect(client.items[0].generation).toBe(2); + expect(client.items[0].postDispatchEvidenceKeys).toEqual(["review:2@H2"]); + expect(client.history.length).toBe(historyBefore); + }); }); diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 43ef80d5..ab354a77 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -1774,6 +1774,9 @@ export async function markPrFixItem( outcome: row.outcome ?? resolvePrFixOutcome("PARKED", input.outcome) ?? "no_progress", latestNote: input.note ?? null, context, + // A BLOCKED item that parks is no longer awaiting a human decision + // (#1218): retract the needs-human label it carried as BLOCKED. + removeNeedsHumanLabel: existing.status === "BLOCKED", }); } else if ( (existing.status === "BLOCKED" || existing.status === "PARKED") && diff --git a/src/lib/pr-fix-surfacing.test.ts b/src/lib/pr-fix-surfacing.test.ts index 86258c61..0b59565a 100644 --- a/src/lib/pr-fix-surfacing.test.ts +++ b/src/lib/pr-fix-surfacing.test.ts @@ -412,6 +412,29 @@ describe("surfacePrFixParked (#1218)", () => { expect(result.commentPosted).toBe(false); expect(result.errors).toHaveLength(1); }); + + it("retracts the needs-human label when removeNeedsHumanLabel is set (#1218)", async () => { + const result = await surfacePrFixParked({ + ...baseInput, + outcome: "no_progress", + removeNeedsHumanLabel: true, + }); + expect(mocks.removeIssueLabel).toHaveBeenCalledWith("org/repo", 42, NEEDS_HUMAN_LABEL); + expect(result.labelRemoved).toBe(true); + // The retraction happens before/with the parked notice, never in place of it. + expect(mocks.addIssueComment).toHaveBeenCalledWith( + "org/repo", + 42, + expect.stringContaining(PARKED_COMMENT_MARKER), + ); + expect(result.errors).toEqual([]); + }); + + it("leaves the needs-human label alone when removeNeedsHumanLabel is absent", async () => { + const result = await surfacePrFixParked({ ...baseInput, outcome: "no_progress" }); + expect(mocks.removeIssueLabel).not.toHaveBeenCalled(); + expect(result.labelRemoved).toBe(false); + }); }); describe("surfacePrFixRequeued", () => { diff --git a/src/lib/pr-fix-surfacing.ts b/src/lib/pr-fix-surfacing.ts index 84d20149..674b7b10 100644 --- a/src/lib/pr-fix-surfacing.ts +++ b/src/lib/pr-fix-surfacing.ts @@ -52,9 +52,17 @@ export interface SurfacePrFixParkedInput { outcome?: string | null; latestNote?: string | null; context?: PrFixSurfaceContext | null; + /** + * Best-effort retraction of the `needs-human` label (#1218). A BLOCKED item + * that later parks (bounded exhaustion / infra / security) must not keep the + * label that claims a human decision is pending. Off by default. + */ + removeNeedsHumanLabel?: boolean; } export interface SurfacePrFixParkedResult { + /** True when the `needs-human` label was removed (only when requested). */ + labelRemoved: boolean; /** True when a fresh marker comment was posted. */ commentPosted: boolean; /** True when an existing marker comment was PATCHed in place. */ @@ -341,6 +349,7 @@ export async function surfacePrFixBlocked(input: SurfacePrFixBlockedInput): Prom */ export async function surfacePrFixParked(input: SurfacePrFixParkedInput): Promise { const result: SurfacePrFixParkedResult = { + labelRemoved: false, commentPosted: false, commentUpdated: false, skippedTerminal: false, @@ -363,6 +372,20 @@ export async function surfacePrFixParked(input: SurfacePrFixParkedInput): Promis return result; } + // A BLOCKED item that parks is no longer awaiting a human decision (#1218): + // retract the needs-human label before writing the parked notice so the two + // never disagree. Best-effort — a failed retraction is recorded, never thrown. + if (input.removeNeedsHumanLabel === true) { + try { + await removeIssueLabel(input.repo, input.pr, NEEDS_HUMAN_LABEL); + result.labelRemoved = true; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error(`pr-fix-surfacing parked label error for ${input.repo}#${input.pr}:`, error); + result.errors.push(`label: ${message}`); + } + } + try { const existing = await fetchIssueComments(input.repo, input.pr, 100, "desc"); const marker = existing.find((c) => isDispatchMarkerComment(c.body)); From 3a4ba0529c576e2bcbe2fd0f60f7a0b6937727a8 Mon Sep 17 00:00:00 2001 From: Courier Date: Sun, 11 Oct 2026 20:30:38 +0000 Subject: [PATCH 3/7] fix(pr-fix): address #1218 review findings (sticky BLOCKED/PARKED, PARKED UI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settles the three blockers joryirving raised on the #1218 PR: P1 — A genuine human-decision report can be auto-reopened instead of BLOCKED. settlesAttempt included BLOCKED, so a worker reporting `blocked` with blockReason: human_decision while fresh post-dispatch feedback arrived silently flipped the row back to QUEUED (or PARKED at cap), cleared the decision fields, and re-dispatched without an operator. Narrow the post-dispatch reopen path to FIXED only; BLOCKED and PARKED settlements are now sticky on fresh evidence. The bounded re-decision loop and the forced-fallback tail both honor the same rule; the sticky pair skips the head fetch so a never-released row never costs a GitHub round-trip. New evidence stays recorded on the row for operator visibility; only an explicit operator requeue (or a maintainer mark to QUEUED) releases it. P2 — Stale-handout reclaim-cap PARKED transition didn't surface diagnostics. reclaimStalePrFixHandouts wrote PARKED + history but never called surfacePrFixParked, so the parked marker comment was missing on this transition while every other cap path posted it. Add the call outside the DB transaction so operators see every PARKED item in one place, and rename the report counter: a new `parked` field is the truthful name going forward; `blocked` is kept as a backwards-compatible alias. P1 (operator UI) — PARKED rows were invisible in src/app/automation/pr-fix-queue/page.tsx. Partition them into their own visible group with the structured outcome, the worker's attempt count, and the same requeue action the BLOCKED group already had. Tests: - Update the pre-existing tests that expected BLOCKED → QUEUED reopen to expect the new sticky behavior with the operator-facing decision payload preserved. - Add a focused report-handler test for the reviewer-flagged scenario (human_decision + actionable post-dispatch evidence). - Reassert that the cap parks transition posts the parked diagnostic. Local validation: 176 files / 4191 tests pass, 16 skipped, 1 todo. `npm run typecheck` clean. `npm run lint` clean (one pre-existing unrelated warning in src/app/login/page.tsx). CI/checks on this PR are pending — not claimed green. --- src/app/automation/pr-fix-queue/page.tsx | 102 +++++++++++++++++++-- src/lib/pr-fix-queue-report.test.ts | 46 ++++++++++ src/lib/pr-fix-queue.test.ts | 82 ++++++++++++----- src/lib/pr-fix-queue.ts | 109 +++++++++++++++++++---- 4 files changed, 296 insertions(+), 43 deletions(-) diff --git a/src/app/automation/pr-fix-queue/page.tsx b/src/app/automation/pr-fix-queue/page.tsx index 76dfc36d..0b151c40 100644 --- a/src/app/automation/pr-fix-queue/page.tsx +++ b/src/app/automation/pr-fix-queue/page.tsx @@ -6,7 +6,7 @@ import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; import { Badge } from "@/components/ui/badge"; import { Button } from "@/components/ui/button"; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@/components/ui/table"; -import { AlertTriangle, ExternalLink, RefreshCw } from "lucide-react"; +import { AlertTriangle, ExternalLink, PauseCircle, RefreshCw } from "lucide-react"; interface PrFixItem { id: string; @@ -18,8 +18,17 @@ interface PrFixItem { title: string | null; url: string | null; queuedAt: string; + outcome?: string | null; + fixAttempts?: number | null; } +const PARKED_OUTCOME_LABELS: Record = { + no_progress: "no progress", + infra_failure: "infra / auth failure", + security_failure: "security failure", + human_decision: "human decision", +}; + function formatTime(isoString: string): string { return new Date(isoString).toLocaleString(); } @@ -32,6 +41,12 @@ function prTitle(item: PrFixItem): string { return item.title ?? `${item.repo}#${item.pr}`; } +function parkedReason(item: PrFixItem): string { + const outcome = PARKED_OUTCOME_LABELS[item.outcome ?? ""] ?? item.outcome ?? "no progress"; + const attempts = typeof item.fixAttempts === "number" ? item.fixAttempts : null; + return attempts != null ? `${outcome} (after ${attempts} attempt${attempts === 1 ? "" : "s"})` : outcome; +} + export default function PrFixQueuePage() { const [items, setItems] = useState([]); const [loading, setLoading] = useState(true); @@ -60,7 +75,7 @@ export default function PrFixQueuePage() { fetchData(); }, [fetchData]); - const requeueBlocked = useCallback(async (item: PrFixItem) => { + const requeue = useCallback(async (item: PrFixItem) => { const key = `${item.repo}#${item.pr}`; setRequeueingKey(key); setRequeueError(""); @@ -99,6 +114,7 @@ export default function PrFixQueuePage() { }, [fetchData]); const blocked = items.filter((i) => i.status === "BLOCKED"); + const parked = items.filter((i) => i.status === "PARKED"); const queued = items.filter((i) => i.status === "QUEUED"); if (loading) { @@ -110,7 +126,10 @@ export default function PrFixQueuePage() {

PR Fix Queue

-

BLOCKED items need human attention; QUEUED items are waiting on automation.

+

+ BLOCKED items need human attention. PARKED items are bounded exhaustion or + operational failures and can be requeued once the underlying cause is fixed. +

); -} +} \ No newline at end of file diff --git a/src/lib/pr-fix-queue-report.test.ts b/src/lib/pr-fix-queue-report.test.ts index 4caa1d07..3432ca84 100644 --- a/src/lib/pr-fix-queue-report.test.ts +++ b/src/lib/pr-fix-queue-report.test.ts @@ -663,4 +663,50 @@ describe("resolvePrFixFromAgentReport", () => { // it settles (a red PR is never marked FIXED off an unverified report). expect(fetchPullRequestMergeStateMock).toHaveBeenCalledWith("acme/widgets", 1234); }); + + it("keeps a human_decision report sticky even with actionable post-dispatch evidence (#1218)", async () => { + // A dispatched worker reports `blocked` carrying a `human_decision` + // reason while a fresh review arrived after hand-out. The pre-#1218 + // reopen path silently flipped the row back to QUEUED (or PARKED at + // cap), cleared the decision payload, and re-dispatched without an + // operator. The fix: a BLOCKED settlement is sticky — fresh evidence + // is absorbed by the BLOCKED, the operator-facing decision payload is + // preserved, and only an explicit operator requeue releases it. + (prisma.prFixQueueItem.findUnique as ReturnType).mockResolvedValue({ + ...FIXTURE_ITEM, + postDispatchEvidenceKeys: ["review:fresh@H1"], + }); + (prisma.prFixQueueItem.updateMany as ReturnType).mockResolvedValue({ count: 1 }); + + const decision = { question: "needs a human decision", evidence: "fresh review" }; + const result = await resolvePrFixFromAgentReport(baseInput({ + outcome: "blocked", + attempt: ATTEMPT, + blockReason: "human_decision", + decision, + })); + + expect(result.action).toBe("blocked"); + expect(result.matched).toBe(true); + // Sticky: the settlement write is status=BLOCKED with the decision + // payload preserved, NOT a reopen write that would set status=QUEUED. + const calls = (prisma.prFixQueueItem.updateMany as ReturnType).mock.calls; + expect(calls.length).toBeGreaterThan(0); + const lastWrite = calls.at(-1)?.[0]; + expect(lastWrite).toMatchObject({ + where: expect.objectContaining({ id: "item-42", generation: 1 }), + data: expect.objectContaining({ + status: "BLOCKED", + lane: "NEEDS_HUMAN", + outcome: "human_decision", + decisionDetail: decision, + }), + }); + // No FIXED-redirect reopen was attempted: no fresh generation bump, no + // attempt increment, no queued reset. + expect(lastWrite?.data).not.toMatchObject({ generation: expect.anything() }); + expect(lastWrite?.data).not.toMatchObject({ fixAttempts: expect.anything() }); + // The merge-state probe was skipped — a hard block never reaches GitHub. + expect(fetchPullRequestMergeStateMock).not.toHaveBeenCalled(); + }); }); diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 5f96d4b3..77e03894 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -2185,7 +2185,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(client.items[0].evidenceKeys).toEqual(["review:1", "review:2"]); }); - it("evidence after hand-out turns a BLOCKED settlement into a fresh QUEUED attempt", async () => { + it("a BLOCKED settlement is sticky on fresh post-dispatch evidence (#1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 22, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2204,16 +2204,27 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { const result = await markPrFixItem(client, { repo: "org/repo", pr: 22, status: "BLOCKED", + decision: { question: "needs a human decision", evidence: "fresh feedback" }, expectedGeneration: 1, }); expect(result.mutated).toBe(true); - const reopened = mutatedItem(result); - expect(reopened.status).toBe("QUEUED"); - expect(reopened.generation).toBe(2); - expect(reopened.fixAttempts).toBe(2); - expect(reopened.lane).toBe("NORMAL"); - expect(reopened.postDispatchEvidenceKeys).toEqual([]); + const settled = mutatedItem(result); + // BLOCKED is sticky (#1218): fresh post-dispatch evidence must not flip the + // row back to QUEUED. The new evidence is absorbed by the BLOCKED + // settlement and stays recorded on the row for operator visibility. + expect(settled.status).toBe("BLOCKED"); + expect(settled.lane).toBe("NEEDS_HUMAN"); + expect(settled.outcome).toBe("human_decision"); + expect(settled.decisionDetail).toEqual({ + question: "needs a human decision", + evidence: "fresh feedback", + }); + expect(settled.generation).toBe(1); + expect(settled.fixAttempts).toBe(1); + // Only the keys-clear write touched the post-dispatch field; the keys are + // absorbed by the BLOCKED settlement. + expect(settled.postDispatchEvidenceKeys).toEqual([]); }); it("evidence after hand-out turns a FIXED settlement into a fresh QUEUED attempt (regardless of head movement)", async () => { @@ -2309,7 +2320,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(marked.postDispatchEvidenceKeys).toEqual([]); }); - it("post-dispatch evidence past the attempt cap parks the item (#1218)", async () => { + it("a BLOCKED settlement past the attempt cap stays sticky and never reopens (#1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 26, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2327,18 +2338,21 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { const result = await markPrFixItem(client, { repo: "org/repo", pr: 26, status: "BLOCKED", + decision: { question: "needs a human decision" }, expectedGeneration: 1, }); expect(result.mutated).toBe(true); - const capped = mutatedItem(result); - expect(capped.status).toBe("PARKED"); - expect(capped.lane).toBe("NORMAL"); - expect(capped.outcome).toBe("no_progress"); - expect(capped.postDispatchEvidenceKeys).toEqual([]); - // Gave up: no generation bump, no attempt increment. - expect(capped.generation).toBe(1); - expect(capped.fixAttempts).toBe(5); + const settled = mutatedItem(result); + // BLOCKED is sticky even past the cap (#1218): the cap is a fresh-attempt + // guard, not a release-from-BLOCKED guard. Only an explicit operator + // requeue or maintainer mark to QUEUED releases it. + expect(settled.status).toBe("BLOCKED"); + expect(settled.lane).toBe("NEEDS_HUMAN"); + expect(settled.outcome).toBe("human_decision"); + expect(settled.postDispatchEvidenceKeys).toEqual([]); + expect(settled.generation).toBe(1); + expect(settled.fixAttempts).toBe(5); }); it("a stale settlement token cannot reopen on post-dispatch evidence", async () => { @@ -2761,8 +2775,10 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { // Evidence landed on the fast path, every loop iteration, AND the forced // fallback write — strictly more than the 3-iteration loop allows. expect(landCount).toBeGreaterThan(3); - // The capped forced-fallback history row names the parked hand-off. - expect(client.history.at(-1).note).toContain("parked (#1119)"); + // The capped forced-fallback history row names the parked hand-off. The + // note was retagged from #1119 to #1218 when the same fallback also + // covers the sticky BLOCKED/PARKED settlements. + expect(client.history.at(-1).note).toContain("parked (#1218)"); }); it("a post-dispatch check encoded from an `@`-bearing headSha degrades to an unknown head and still reopens", async () => { @@ -2872,7 +2888,7 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { expect(settled.postDispatchEvidenceKeys).toEqual([]); }); - it("a post-dispatch reopen retracts the needs-human marker (surfacePrFixUnblocked)", async () => { + it("a BLOCKED settlement with post-dispatch evidence stays sticky and never retracts (#1218)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 44, lane: "NORMAL", reason: "r", feedback: "f1", evidenceKey: "review:1", headSha: "H1", @@ -2886,13 +2902,17 @@ describe("post-dispatch evidence reopens the attempt (#1119)", () => { const result = await markPrFixItem(client, { repo: "org/repo", pr: 44, status: "BLOCKED", + decision: { question: "needs a human decision", evidence: "fresh feedback" }, expectedGeneration: 1, }); - // The initial post-dispatch reopen leaves the would-be settlement: the - // needs-human marker must be folded back into a requeued notice. - expect(mutatedItem(result).status).toBe("QUEUED"); - expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 44, "requeued", undefined); + // BLOCKED is sticky on fresh post-dispatch evidence (#1218): the row + // stays BLOCKED with the operator-facing decision payload. Only an + // explicit operator requeue releases it; surfacePrFixUnblocked must NOT + // be called (no needs-human retraction happened). + expect(mutatedItem(result).status).toBe("BLOCKED"); + expect(mutatedItem(result).lane).toBe("NEEDS_HUMAN"); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); }); it("a gap-loop post-dispatch reopen retracts the needs-human marker too", async () => { @@ -3542,10 +3562,15 @@ describe("reclaim stale hand-outs (#1211)", () => { try { const row = await seedStaleUnacked(205); row.handoutReclaims = 3; + surfacingMocks.surfacePrFixParked.mockClear(); const report = await reclaimStalePrFixHandouts(client, { now: STALE_NOW, isPrMergedOrClosed: async () => false, }); + // Both `parked` and `blocked` count the reclaim-cap transition (#1218): + // `blocked` is the legacy alias for backwards-compatible dashboards, + // `parked` is the truthful name going forward. + expect(report.parked).toBe(1); expect(report.blocked).toBe(1); expect(report.reclaimed).toBe(0); const after = client.items[0]; @@ -3563,6 +3588,17 @@ describe("reclaim stale hand-outs (#1211)", () => { expect(last.note).toContain("1211"); // The reclaim cap never applies the needs-human label/marker (#1218). expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + // The reclaim cap posts the parked diagnostic (#1218), matching the + // other cap paths so operators see every PARKED item in one place. + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ + repo: "org/repo", + pr: 205, + outcome: "no_progress", + latestNote: expect.stringContaining("1211"), + }), + ); } finally { if (prev === undefined) delete process.env.PR_FIX_MAX_RECLAIMS; else process.env.PR_FIX_MAX_RECLAIMS = prev; diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 619a0b66..e3022299 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -713,6 +713,14 @@ export interface ReclaimStalePrFixHandoutsReport { examined: number; reclaimed: number; staled: number; + /** + * Items PARKED at the reclaim cap (#1218). Legacy callers may still read + * `blocked` for the same count — both are incremented in the cap path so + * old dashboards keep working, but `parked` is the truthful name going + * forward (a parked item is bounded exhaustion, not a human decision). + */ + parked: number; + /** @deprecated Use `parked` (#1218). Retained for callers that still key off it. */ blocked: number; skipped: number; unknownState: number; @@ -772,6 +780,7 @@ export async function reclaimStalePrFixHandouts( examined: 0, reclaimed: 0, staled: 0, + parked: 0, blocked: 0, skipped: 0, unknownState: 0, @@ -923,8 +932,36 @@ export async function reclaimStalePrFixHandouts( } return result; }); - if (count === 1) report.blocked++; - else report.skipped++; + if (count === 1) { + report.parked++; + // Legacy alias: the reclaim-cap path counted toward `blocked` + // historically. Bumping both keeps older dashboards working + // (#1218) while `parked` is the truthful name going forward. + report.blocked++; + // Park the GitHub-side surface outside the DB transaction + // (#1218 review): the other cap paths call surfacePrFixParked + // here, so the reclaim-cap transition must post the same + // diagnostic marker so operators see every PARKED item in one + // place. Best-effort — a missed GitHub write must not block the + // sweep. + try { + await surfacePrFixParked({ + repo: item.repo, + pr: item.pr, + reason: item.reason, + outcome: "no_progress", + latestNote: `Reclaimed ${reclaimCount} times without acknowledgement; parked (#1211).`, + context: await buildPrFixBlockedContext(client, { ...item, fixAttempts: item.fixAttempts ?? null }), + }); + } catch (error) { + report.errors.push({ + itemId: item.id, + error: error instanceof Error ? error.message : String(error), + }); + } + } else { + report.skipped++; + } continue; } @@ -1591,17 +1628,19 @@ export async function markPrFixItem( } }; - // A settlement (FIXED/BLOCKED/PARKED) on an item whose post-dispatch evidence - // list still holds an actionable entry must NOT absorb that evidence: reopen - // as a fresh dispatchable attempt so a worker actually runs on it (#1119). - // Evaluated only for settlements, so a non-settle mark never triggers a head - // fetch. A capped item still gives up (PARKED, #1218), matching the other - // fresh-attempt caps. The head fetch inside the actionability check always + // Only FIXED settlements reopen on fresh post-dispatch evidence (#1218): + // human-decision BLOCKED and operational PARKED are sticky — a fresh + // post-dispatch entry on a sticky settlement stays recorded on the row for + // operator visibility, and only an explicit operator requeue (or a + // maintainer mark to QUEUED) releases it. A capped FIXED still gives up + // (PARKED), matching the other fresh-attempt caps; BLOCKED/PARKED skip the + // cap path entirely. The head fetch inside the actionability check always // runs OUTSIDE any transaction: Prisma's ~5s interactive-transaction // timeout must never have to span a GitHub round-trip (#1124 review). const settlesAttempt = nextStatus === "FIXED" || nextStatus === "BLOCKED" || nextStatus === "PARKED"; + const reopenEligible = nextStatus === "FIXED"; const reopenOnPostDispatch = - settlesAttempt && + reopenEligible && (await hasActionablePostDispatchEvidence(existing.postDispatchEvidenceKeys ?? [], input.repo, input.pr)); // The initial reopen attempt can miss its keys/generation pin when evidence // is recorded between the check above and the write. That is not a @@ -1854,7 +1893,11 @@ export async function markPrFixItem( } lastFresh = fresh; const freshKeys: string[] = fresh.postDispatchEvidenceKeys ?? []; - if (await hasActionablePostDispatchEvidence(freshKeys, input.repo, input.pr)) { + // #1218 sticky settlement: a BLOCKED/PARKED settle must absorb the keys + // (cleared below) rather than reopen. Only FIXED settlements can be + // auto-released by fresh evidence; the head fetch is skipped for the + // sticky pair so a never-released row never costs a GitHub round-trip. + if (reopenEligible && (await hasActionablePostDispatchEvidence(freshKeys, input.repo, input.pr))) { const reopened = await client.$transaction(async (tx) => writeReopen(tx, fresh, " Post-dispatch evidence landed while this settlement was in flight."), ); @@ -1901,13 +1944,49 @@ export async function markPrFixItem( } // The loop exhausted: the pinned writes kept losing the race to evidence - // that keeps landing. Never return no-mutation for a settle mark merely - // because of that — the item would stay QUEUED at the generation the - // worker already consumed, next-task would keep handing the spent + // that keeps landing. Only FIXED settlements can be force-released by + // racing evidence — BLOCKED/PARKED are sticky (#1218), so for them we + // drop straight to the unguarded settle (clearing keys + honoring the + // caller's status). For FIXED, never return no-mutation merely because + // of the racing evidence — the item would stay QUEUED at the generation + // the worker already consumed, next-task would keep handing the spent // generation first to the deduping worker, and the lane would starve // (#1124 review). Force a fresh attempt (or, past the cap, a human // hand-off): pinned on id (+ the generation when the token defines one) // WITHOUT the keys pin, so racing evidence cannot block it. + if (!reopenEligible) { + // Sticky-settlement force: clear keys without status flip. The settlement + // data already encodes lane/outcome/decisionDetail for the requested + // status; writing the cleared-keys update is what the gap-reopen loop's + // "no-reopen" branch would have done if it had won the keys pin. + const stickyRow = await client.$transaction(async (tx) => { + const { count } = await tx.prFixQueueItem.updateMany({ + where: { + id: existing.id, + ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + }, + data: { ...data, postDispatchEvidenceKeys: [] }, + }); + if (count !== 1) return null; + const row = await tx.prFixQueueItem.findUnique({ where: { id: existing.id } }); + await tx.prFixHistory.create({ + data: { + itemId: row.id, + action: "mark", + status: nextStatus, + lane: row.lane, + outcome: row.outcome ?? resolvePrFixOutcome(nextStatus, input.outcome), + note: input.alreadyAddressed ? alreadyAddressedHistoryNote(input) : input.note ?? undefined, + }, + }); + return row; + }); + if (!stickyRow) { + const remaining = await client.prFixQueueItem.findUnique({ where: { id: existing.id } }); + return { mutated: false, reason: remaining ? "generation-mismatch" : "not-found" }; + } + return finishMarked(stickyRow); + } const forceCapped = (lastFresh.fixAttempts ?? 1) >= maxPrFixAttempts(); const forceData: Record = forceCapped ? { @@ -1955,8 +2034,8 @@ export async function markPrFixItem( note: [ input.note, forceCapped - ? `Post-dispatch evidence kept landing while this settlement was in flight, but bounded at ${lastFresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked (#1119).` - : `Post-dispatch evidence kept landing while this settlement was in flight; forced a fresh attempt so the racing evidence is not absorbed (#1119).`, + ? `Post-dispatch evidence kept landing while this settlement was in flight, but bounded at ${lastFresh.fixAttempts ?? 1} fix attempts (PR_FIX_MAX_ATTEMPTS=${maxPrFixAttempts()}); parked (#1218).` + : `Post-dispatch evidence kept landing while this settlement was in flight; forced a fresh attempt so the racing evidence is not absorbed (#1218).`, ] .filter(Boolean) .join(" "), From 27c37c54618fad3d6e1295fc00fda074efb3c47c Mon Sep 17 00:00:00 2001 From: Courier Date: Mon, 12 Oct 2026 03:56:04 +0000 Subject: [PATCH 4/7] fix(pr-fix): pin settlement status to QUEUED so worker marks cannot erase a recorded human decision (#1218 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A worker-tier mark to BLOCKED/PARKED on a row that already records BLOCKED/PARKED/FIXED at the same generation must no-op rather than overwrite the genuine human classification. markPrFixItem now requires 'status: QUEUED' at the commit-time updateMany for worker-tier BLOCKED/PARKED settlements (and the re-decision / sticky force branches); a same-generation second mark, or a read→write gap that flipped the row to BLOCKED/PARKED, returns 'generation-mismatch' instead of erasing the recorded decision. Operator reclassification (BLOCKED → PARKED to correct an outdated decision, or vice versa) is a separate, explicitly-authenticated path: the route handler forwards the auth-derived actorTier, and only a maintainer-tier caller with allowStickyReclassify=true bypasses the status pin. Workers cannot forge a maintainer tier — the tier is sourced from the auth result, not the request body. Audit log records the 'reclassify=true' flag when the operator path is taken. Tests: - new: 'status pin prevents worker erase of a recorded decision' suite covers sequential BLOCKED→PARKED, sequential PARKED→BLOCKED, commit-time guard on an already-BLOCKED row, operator reclassification, and fail-closed worker allowStickyReclassify=true. - updated: 'retracts the needs-human label when a BLOCKED item is later parked' now exercises the operator reclassification path. - updated: pr-fix-queue-report.test.ts where-clause expectation to include the new status: QUEUED pin. --- src/app/api/pr-fix-queue/mark/route.test.ts | 14 +- src/app/api/pr-fix-queue/mark/route.ts | 23 ++- src/lib/pr-fix-queue-report.test.ts | 5 +- src/lib/pr-fix-queue.test.ts | 189 +++++++++++++++++++- src/lib/pr-fix-queue.ts | 52 +++++- 5 files changed, 271 insertions(+), 12 deletions(-) diff --git a/src/app/api/pr-fix-queue/mark/route.test.ts b/src/app/api/pr-fix-queue/mark/route.test.ts index 247a6248..bdba5228 100644 --- a/src/app/api/pr-fix-queue/mark/route.test.ts +++ b/src/app/api/pr-fix-queue/mark/route.test.ts @@ -140,9 +140,11 @@ describe("POST /api/pr-fix-queue/mark", () => { expect(res.status).toBe(200); // No expectedGeneration on the input: the operator mark is unconditional. - expect(mocks.markPrFixItem).toHaveBeenCalledWith(expect.anything(), { - repo: "org/repo", pr: 42, status: "DONE", - }); + // #1218 review: the route forwards the auth tier so markPrFixItem can + // honor the commit-time status pin correctly. + expect(mocks.markPrFixItem).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + repo: "org/repo", pr: 42, status: "DONE", actorTier: "maintainer", + })); }); it("allows an operator mark without a generation (basic mode)", async () => { @@ -165,9 +167,9 @@ describe("POST /api/pr-fix-queue/mark", () => { expect(res.status).toBe(200); // No expectedGeneration on the input: the operator mark is unconditional. - expect(mocks.markPrFixItem).toHaveBeenCalledWith(expect.anything(), { - repo: "org/repo", pr: 42, status: "DONE", - }); + expect(mocks.markPrFixItem).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + repo: "org/repo", pr: 42, status: "DONE", actorTier: "maintainer", + })); }); it("returns 404 when item not found", async () => { diff --git a/src/app/api/pr-fix-queue/mark/route.ts b/src/app/api/pr-fix-queue/mark/route.ts index c31479de..f4489a3f 100644 --- a/src/app/api/pr-fix-queue/mark/route.ts +++ b/src/app/api/pr-fix-queue/mark/route.ts @@ -59,6 +59,21 @@ export async function POST(request: Request) { return errorResponse("Marking an item QUEUED or IGNORED requires a maintainer token", 403); } + // #1218 review: the mark's commit-time status pin (status === "QUEUED" for + // BLOCKED/PARKED settlements) must be honored for worker callers — a + // same-generation second mark cannot erase a recorded human decision. + // Operators can opt in to reclassify via `allowStickyReclassify`; workers + // have no equivalent. The tier value is sourced from `auth` here rather + // than the request body so a worker cannot forge a maintainer tier to + // bypass the pin. Strip any body-supplied `allowStickyReclassify` for + // worker requests for defense in depth. + const actorTier: "worker" | "maintainer" = + auth.tier === "maintainer" ? "maintainer" : "worker"; + const allowStickyReclassify = + actorTier === "maintainer" && body && typeof body === "object" && !Array.isArray(body) + ? (body as Record).allowStickyReclassify === true + : false; + // #1074: bearer (agent/bridge) marks must settle a specific attempt, so // the generation token is required. Operator paths (oidc session, basic, // disabled) keep the optional behavior for compatibility. @@ -118,7 +133,11 @@ export async function POST(request: Request) { } } - const result = await markPrFixItem(asPrFixQueueClient(prisma), input); + const result = await markPrFixItem(asPrFixQueueClient(prisma), { + ...input, + actorTier, + ...(allowStickyReclassify ? { allowStickyReclassify: true } : {}), + }); if (!result.mutated) { if (result.reason === "generation-mismatch") { // The item moved to a newer attempt between the caller's read and @@ -141,7 +160,7 @@ export async function POST(request: Request) { success: true, beforeLabels: [], afterLabels: [], - notes: `pr=${input.pr} status=${item.status}${input.note ? ` note=${input.note}` : ""}`, + notes: `pr=${input.pr} status=${item.status}${allowStickyReclassify ? " reclassify=true" : ""}${input.note ? ` note=${input.note}` : ""}`, }, }); diff --git a/src/lib/pr-fix-queue-report.test.ts b/src/lib/pr-fix-queue-report.test.ts index 3432ca84..649d2ba7 100644 --- a/src/lib/pr-fix-queue-report.test.ts +++ b/src/lib/pr-fix-queue-report.test.ts @@ -588,8 +588,11 @@ describe("resolvePrFixFromAgentReport", () => { // guarded on the post-dispatch evidence list being empty, #1119) and // matched nothing. expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledTimes(1); + // #1218 review: the worker-tier PARKED settle is also guarded on the row + // being QUEUED, so a stale generation pins status: QUEUED alongside the + // post-dispatch evidence guard (#1119) and id/generation (#1074). expect(prisma.prFixQueueItem.updateMany).toHaveBeenCalledWith({ - where: { id: "item-42", generation: 1, postDispatchEvidenceKeys: { equals: [] } }, + where: { id: "item-42", generation: 1, postDispatchEvidenceKeys: { equals: [] }, status: "QUEUED" }, data: expect.objectContaining({ status: "PARKED" }), }); expect(prisma.prFixQueueItem.update).not.toHaveBeenCalled(); diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index 77e03894..f8e6fc6b 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -4386,7 +4386,7 @@ describe("PR-fix outcome separation (#1218)", () => { expect((await listQueuedPrFixItems(client)).map((i) => i.pr)).not.toContain(310); }); - it("retracts the needs-human label when a BLOCKED item is later parked", async () => { + it("retracts the needs-human label when a BLOCKED item is later parked by an operator reclassification (#1218 review)", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 311, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k311", }); @@ -4396,8 +4396,16 @@ describe("PR-fix outcome separation (#1218)", () => { expect(surfacingMocks.surfacePrFixBlocked).toHaveBeenCalledTimes(1); surfacingMocks.surfacePrFixParked.mockClear(); + // #1218 review: explicit operator reclassification — only honored for + // maintainer-tier callers with `allowStickyReclassify`. Worker calls at + // the same generation must no-op (see the regression tests below). const parked = await markPrFixItem(client, { - repo: "org/repo", pr: 311, status: "PARKED", outcome: "no_progress", + repo: "org/repo", + pr: 311, + status: "PARKED", + outcome: "no_progress", + actorTier: "maintainer", + allowStickyReclassify: true, }); expect(mutatedItem(parked).status).toBe("PARKED"); @@ -4512,4 +4520,181 @@ describe("PR-fix outcome separation (#1218)", () => { expect(client.items[0].postDispatchEvidenceKeys).toEqual(["review:2@H2"]); expect(client.history.length).toBe(historyBefore); }); + + // #1218 review: same-generation second worker marks must not erase a + // recorded human decision or parked classification. These cover both + // sequential marks (separate calls, same expectedGeneration) and the + // read→write gap (the row already records the prior settlement when the + // mark is issued, so the live status pin catches it at commit time). + describe("status pin prevents worker erase of a recorded decision (#1218 review)", () => { + it("a sequential worker mark BLOCKED -> PARKED at unchanged generation no-ops", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 318, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k318", + }); + const decision = { + question: "Which behavior?", + options: ["A", "B"], + evidence: "spec mismatch", + }; + const blocked = await markPrFixItem(client, { + repo: "org/repo", pr: 318, status: "BLOCKED", decision, expectedGeneration: 1, + }); + expect(mutatedItem(blocked).status).toBe("BLOCKED"); + expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].outcome).toBe("human_decision"); + expect(client.items[0].decisionDetail).toEqual(decision); + const historyAfterBlocked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + + // The same worker holds generation 1 and now reports PARKED — the prior + // BLOCKED is sticky. The mark must no-op, not park it. + const secondMark = await markPrFixItem(client, { + repo: "org/repo", pr: 318, status: "PARKED", outcome: "no_progress", expectedGeneration: 1, + }); + + expect(secondMark).toEqual({ mutated: false, reason: "generation-mismatch" }); + // The original decision and label are intact. + expect(client.items[0].status).toBe("BLOCKED"); + expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].outcome).toBe("human_decision"); + expect(client.items[0].decisionDetail).toEqual(decision); + // No history row was appended (the mark was a no-op). + expect(client.history.length).toBe(historyAfterBlocked); + // The parked surface must not fire — there is no parked classification. + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + // The blocked surface must not fire either — the decision was already + // surfaced on the prior call. + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("a sequential worker mark PARKED -> BLOCKED at unchanged generation no-ops", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 319, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k319", + }); + const parked = await markPrFixItem(client, { + repo: "org/repo", pr: 319, status: "PARKED", outcome: "no_progress", expectedGeneration: 1, + }); + expect(mutatedItem(parked).status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("no_progress"); + const historyAfterParked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + + // Worker tries to escalate back to BLOCKED at the same generation — + // the sticky state pin must refuse, since PARKED was a real cap and + // only operator reclassification or requeue should release it. + const escalation = await markPrFixItem(client, { + repo: "org/repo", pr: 319, status: "BLOCKED", decision: { question: "?" }, expectedGeneration: 1, + }); + + expect(escalation).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0].status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("no_progress"); + expect(client.items[0].decisionDetail).toBeNull(); + expect(client.history.length).toBe(historyAfterParked); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + // Commit-time revalidation: the row is already BLOCKED in the DB when the + // worker's PARKED settlement runs its updateMany (whether because the + // worker just read it as BLOCKED in pre-read, or because a concurrent + // same-generation settle landed between read and write). The status pin + // catches both at the commit boundary. + it("a worker PARKED settle no-ops at commit time when the DB row is already BLOCKED at the same generation", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 320, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k320", + }); + const decision = { question: "Keep legacy?", options: ["Yes", "No"] }; + await markPrFixItem(client, { + repo: "org/repo", pr: 320, status: "BLOCKED", decision, expectedGeneration: 1, + }); + expect(client.items[0].status).toBe("BLOCKED"); + const historyAfterBlocked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + + // Worker holds generation 1 and reports PARKED. The status pin must + // refuse because the row already records a human decision at the + // same generation — both the worker's pre-read and the DB row see + // BLOCKED, so the commit-time updateMany matches nothing. + const secondMark = await markPrFixItem(client, { + repo: "org/repo", pr: 320, status: "PARKED", outcome: "no_progress", expectedGeneration: 1, + }); + expect(secondMark).toEqual({ mutated: false, reason: "generation-mismatch" }); + // Decision, label, and outcome remain intact. + expect(client.items[0].status).toBe("BLOCKED"); + expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].outcome).toBe("human_decision"); + expect(client.items[0].decisionDetail).toEqual(decision); + expect(client.history.length).toBe(historyAfterBlocked); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + }); + + it("an operator reclassification can downgrade BLOCKED -> PARKED with allowStickyReclassify", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 321, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k321", + }); + const decision = { question: "Which?", options: ["A", "B"] }; + const blocked = await markPrFixItem(client, { + repo: "org/repo", pr: 321, status: "BLOCKED", decision, expectedGeneration: 1, + }); + expect(mutatedItem(blocked).status).toBe("BLOCKED"); + surfacingMocks.surfacePrFixParked.mockClear(); + + // Operator explicitly opts in to reclassify — the status pin is + // relaxed for this single call. The prior BLOCKED's outcome is + // overwritten with `no_progress`; decisionDetail clears as a side + // effect of the reclassify. + const parked = await markPrFixItem(client, { + repo: "org/repo", + pr: 321, + status: "PARKED", + outcome: "no_progress", + expectedGeneration: 1, + actorTier: "maintainer", + allowStickyReclassify: true, + }); + + expect(mutatedItem(parked).status).toBe("PARKED"); + expect(client.items[0].lane).toBe("NORMAL"); + expect(client.items[0].outcome).toBe("no_progress"); + expect(client.items[0].decisionDetail).toBeNull(); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledTimes(1); + expect(surfacingMocks.surfacePrFixParked).toHaveBeenCalledWith( + expect.objectContaining({ removeNeedsHumanLabel: true, outcome: "no_progress" }), + ); + }); + + it("a worker caller cannot bypass the pin by sending allowStickyReclassify=true (fail-closed)", async () => { + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 322, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k322", + }); + const blocked = await markPrFixItem(client, { + repo: "org/repo", pr: 322, status: "BLOCKED", expectedGeneration: 1, + }); + expect(mutatedItem(blocked).status).toBe("BLOCKED"); + const historyBefore = client.history.length; + + // Worker sends allowStickyReclassify=true; without a maintainer tier + // the pin is honored (fail-closed). + const secondMark = await markPrFixItem(client, { + repo: "org/repo", + pr: 322, + status: "PARKED", + outcome: "no_progress", + expectedGeneration: 1, + allowStickyReclassify: true, + }); + + expect(secondMark).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0].status).toBe("BLOCKED"); + expect(client.items[0].lane).toBe("NEEDS_HUMAN"); + expect(client.items[0].outcome).toBe("human_decision"); + expect(client.history.length).toBe(historyBefore); + }); + }); }); diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index b19a48fe..50a9bb77 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -111,6 +111,28 @@ export interface MarkPrFixInput { outcome?: string | null; /** Human-decision payload persisted when the mark resolves to BLOCKED (#1218). */ decision?: PrFixDecisionContext | null; + /** + * Caller's auth tier (#1218 review): used to gate the BLOCKED/PARKED status + * pin at commit time. A worker-tier mark to BLOCKED/PARKED on an already + * recorded BLOCKED/PARKED/FIXED row must no-op (return `generation-mismatch`) + * so a same-generation second mark cannot erase a genuine human decision. + * Defaults to `"worker"` (fail-closed): callers that don't explicitly assert + * a maintainer tier cannot bypass the pin. The route handler is the only + * trusted source of this value — `parseMarkPrFixInput` does not accept it + * from a request body, so a worker cannot forge a maintainer tier. + */ + actorTier?: "worker" | "maintainer"; + /** + * Explicit reclassification opt-in (#1218 review). When `true` AND + * `actorTier === "maintainer"`, the BLOCKED/PARKED status pin is relaxed + * for a single mark so an operator can correct a recorded classification + * (BLOCKED → PARKED for an outdated decision, or vice versa). Honored only + * for non-worker callers; a worker request that sets this is silently + * ignored (fail-closed). Intended for explicit operator reclassification — + * it is NEVER the path used by routine worker settlement, which always + * settles QUEUED → BLOCKED/PARKED. + */ + allowStickyReclassify?: boolean; } export interface RequeuePrFixInput { @@ -1547,6 +1569,19 @@ export async function markPrFixItem( // needs-human label. The bridge's ACTIONABLE_LANES filter skips BLOCKED. const data: Record = { status: nextStatus }; + // #1218 review: a recorded human decision (BLOCKED) and a recorded operator + // PARKED are sticky to worker settlements — the commit-time status pin below + // refuses a worker-tier BLOCKED/PARKED mark on a row that already records + // BLOCKED/PARKED/FIXED, so a same-generation second mark (or a read→write + // gap that flipped the row) cannot erase the decision. Reclassification by + // an operator is a separate, explicitly-authenticated path (`allowStickyReclassify` + // honored only when `actorTier === "maintainer"`); worker callers cannot + // bypass the pin. + const isOperatorReclassify = + input.actorTier === "maintainer" && input.allowStickyReclassify === true; + const pinToQueuedForStickySettlement = + (nextStatus === "BLOCKED" || nextStatus === "PARKED") && !isOperatorReclassify; + // Reopen a settlement as a fresh attempt on post-dispatch evidence (#1119): // the guarded write, the cap give-up, and the history row, all inside one // tiny transaction the caller passes in. Pinned on the row's id, its @@ -1840,12 +1875,19 @@ export async function markPrFixItem( // `data`: it would let a stale write clobber a concurrently recorded // entry. Unqualified marks take the same guarded updateMany path (no // generation clause) so the settle-gap guard applies to them too. + // #1218 review: a worker-tier BLOCKED/PARKED settlement additionally + // requires `status: "QUEUED"` at the row's pinned page — a same-generation + // second mark cannot downgrade a recorded human decision or parked + // classification, and a read→write-gap that flipped the row to BLOCKED + // also no-ops the write. Operator reclassification explicitly opts in + // via `allowStickyReclassify` to bypass the pin. if (!reopenPinnedMissed) { const settledRow = await client.$transaction(async (tx) => { const settleWhere: Record = { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), ...(settlesAttempt ? { postDispatchEvidenceKeys: { equals: [] } } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }; const { count } = await tx.prFixQueueItem.updateMany({ where: settleWhere, data }); if (count !== 1) return null; @@ -1911,13 +1953,17 @@ export async function markPrFixItem( // normally, clear them, and say so in the history (#1119). The write is // pinned to the exact keys snapshot this no-reopen decision was made on, // so an entry landing during the head fetch above no-ops the write - // instead of being cleared and absorbed. + // instead of being cleared and absorbed. #1218 review: also pin + // `status: QUEUED` for a worker-tier BLOCKED/PARKED settle so a row that + // already flipped into BLOCKED/PARKED during the gap cannot be downgraded + // by a stale settle. const clearedRow = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ where: { id: fresh.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), postDispatchEvidenceKeys: { equals: freshKeys }, + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: { ...data, postDispatchEvidenceKeys: [] }, }); @@ -1959,11 +2005,15 @@ export async function markPrFixItem( // data already encodes lane/outcome/decisionDetail for the requested // status; writing the cleared-keys update is what the gap-reopen loop's // "no-reopen" branch would have done if it had won the keys pin. + // #1218 review: also pin `status: QUEUED` for a worker-tier BLOCKED/PARKED + // force so a row that already recorded a decision during the gap cannot + // be downgraded. const stickyRow = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ where: { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: { ...data, postDispatchEvidenceKeys: [] }, }); From 531eaceae3a234b2cb81c35386f019745e6b3870 Mon Sep 17 00:00:00 2001 From: Courier Date: Mon, 12 Oct 2026 04:13:03 +0000 Subject: [PATCH 5/7] docs(pr-fix): update handoutReclaims docs to reflect PARKED exhaustion (#1218) The reclaim-cap path now parks as PARKED in the NORMAL lane with outcome=no_progress (bounded no-progress exhaustion, not a human decision), but three doc comments still described the old BLOCKED + NEEDS_HUMAN behavior: - prisma/schema.prisma handoutReclaims column comment - prisma/migrations/20261009000000_add_pr_fix_handout_acks/migration.sql - src/lib/pr-fix-queue.ts reclaimStalePrFixHandouts docstring All three now describe the same parking behavior the code path implements (src/lib/pr-fix-queue.ts:915-988) and point at the operator-requeue release path. No code change. Settles the minor stale-doc finding from the automated review at head 27c37c5. Local validation: typecheck clean; lint 0 errors (1 pre-existing unrelated warning in src/app/login/page.tsx); full vitest 4273 passed / 16 skipped / 1 todo; prisma validate valid. --- .../migration.sql | 10 ++++++---- prisma/schema.prisma | 5 ++++- src/lib/pr-fix-queue.ts | 5 ++++- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/prisma/migrations/20261009000000_add_pr_fix_handout_acks/migration.sql b/prisma/migrations/20261009000000_add_pr_fix_handout_acks/migration.sql index 515b7b58..128dbb14 100644 --- a/prisma/migrations/20261009000000_add_pr_fix_handout_acks/migration.sql +++ b/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; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index a7cf700a..7a421f15 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -214,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[] diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 50a9bb77..2dce0f70 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -780,7 +780,10 @@ async function defaultIsPrMergedOrClosed(repo: string, pr: number): Promise Date: Mon, 12 Oct 2026 08:32:44 -0600 Subject: [PATCH 6/7] fix(pr-fix): pin same-generation worker FIXED/STALE marks to active queue status (#1218 review) --- src/lib/pr-fix-queue.test.ts | 98 ++++++++++++++++++++++++++++++++++-- src/lib/pr-fix-queue.ts | 38 +++++++++----- 2 files changed, 120 insertions(+), 16 deletions(-) diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index f8e6fc6b..fa05de2d 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -2111,7 +2111,7 @@ describe("needs-human cleanup on every exit from BLOCKED (#1105)", () => { }); it("folds to a resolved notice when a mark settles the item FIXED", async () => { - await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed", note: "fixed by hand" }); + await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed", note: "fixed by hand", actorTier: "maintainer" }); expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 1, "resolved", "fixed by hand"); }); @@ -2129,7 +2129,7 @@ describe("needs-human cleanup on every exit from BLOCKED (#1105)", () => { it("folds to a requeued notice when a FIXED mark is refused back to QUEUED", async () => { githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("H1"); - const refused = mutatedItem(await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed" })); + const refused = mutatedItem(await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed", actorTier: "maintainer" })); expect(refused.status).toBe("QUEUED"); expect(surfacingMocks.surfacePrFixUnblocked).toHaveBeenCalledWith("org/repo", 1, "requeued", undefined); }); @@ -2144,7 +2144,7 @@ describe("needs-human cleanup on every exit from BLOCKED (#1105)", () => { it("never lets a cleanup failure break the transition", async () => { surfacingMocks.surfacePrFixUnblocked.mockRejectedValue(new Error("network down")); - const result = await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed" }); + const result = await markPrFixItem(client, { repo: "org/repo", pr: 1, status: "fixed", actorTier: "maintainer" }); expect(mutatedItem(result).status).toBe("FIXED"); }); }); @@ -4634,6 +4634,98 @@ describe("PR-fix outcome separation (#1218)", () => { expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); }); + it.each([ + { name: "FIXED with head moved", body: { status: "FIXED", note: "pushed" }, headSha: "live-head" }, + { name: "FIXED via alreadyAddressed (guard bypassed)", body: { status: "FIXED", alreadyAddressed: true, note: "handled" }, headSha: null }, + { name: "STALE", body: { status: "STALE", note: "stale" }, headSha: null }, + ])("a sequential worker mark BLOCKED -> $name at unchanged generation no-ops", async ({ body, headSha }) => { + const decision = { question: "Which behavior?", options: ["A", "B"], evidence: "conflict" }; + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 322, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k322", + }); + await markPrFixItem(client, { + repo: "org/repo", pr: 322, status: "BLOCKED", decision, expectedGeneration: 1, + }); + expect(client.items[0].status).toBe("BLOCKED"); + const historyAfterBlocked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + surfacingMocks.surfacePrFixUnblocked.mockClear(); + githubPrsMocks.fetchPullRequestHeadSha.mockReset(); + githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue(headSha); + + const secondMark = await markPrFixItem(client, { + repo: "org/repo", pr: 322, expectedGeneration: 1, ...body, + }); + + expect(secondMark).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0]).toMatchObject({ + status: "BLOCKED", generation: 1, lane: "NEEDS_HUMAN", outcome: "human_decision", decisionDetail: decision, + }); + expect(client.history).toHaveLength(historyAfterBlocked); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); + }); + + it("a sequential worker mark BLOCKED -> FIXED with head unchanged no-ops instead of re-queuing", async () => { + const decision = { question: "Which behavior?", options: ["A", "B"], evidence: "conflict" }; + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 323, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k323", headSha: "baseline-sha", + }); + await markPrFixItem(client, { + repo: "org/repo", pr: 323, status: "BLOCKED", decision, expectedGeneration: 1, + }); + const historyAfterBlocked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixUnblocked.mockClear(); + githubPrsMocks.fetchPullRequestHeadSha.mockReset(); + githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue("baseline-sha"); + + // Head unchanged: without the status pin the refusal fallback would + // bump a fresh QUEUED attempt (or PARK at cap) and erase the decision. + const secondMark = await markPrFixItem(client, { + repo: "org/repo", pr: 323, status: "FIXED", note: "pushed nothing", expectedGeneration: 1, + }); + + expect(secondMark).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0]).toMatchObject({ + status: "BLOCKED", generation: 1, lane: "NEEDS_HUMAN", outcome: "human_decision", decisionDetail: decision, + }); + expect(client.history).toHaveLength(historyAfterBlocked); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); + }); + + it.each(["FIXED", "STALE"])("a worker %s settle no-ops at commit time when the row flips to BLOCKED mid-gap", async (status) => { + const decision = { question: "Which behavior?", options: ["A", "B"], evidence: "conflict" }; + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 324, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k324", + }); + const historyLength = client.history.length; + client.hooks.beforeUpdateMany = () => { + Object.assign(client.items[0], { status: "BLOCKED", lane: "NEEDS_HUMAN", outcome: "human_decision", decisionDetail: decision }); + }; + surfacingMocks.surfacePrFixParked.mockClear(); + surfacingMocks.surfacePrFixBlocked.mockClear(); + surfacingMocks.surfacePrFixUnblocked.mockClear(); + githubPrsMocks.fetchPullRequestHeadSha.mockReset(); + githubPrsMocks.fetchPullRequestHeadSha.mockResolvedValue(null); + + const raced = await markPrFixItem(client, { + repo: "org/repo", pr: 324, status: status as any, note: "raced", expectedGeneration: 1, + }); + + expect(raced).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0]).toMatchObject({ + status: "BLOCKED", generation: 1, decisionDetail: decision, lane: "NEEDS_HUMAN", + }); + expect(client.history).toHaveLength(historyLength); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixBlocked).not.toHaveBeenCalled(); + expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); + }); + it("an operator reclassification can downgrade BLOCKED -> PARKED with allowStickyReclassify", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 321, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k321", diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 2dce0f70..8e215830 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -1582,8 +1582,17 @@ export async function markPrFixItem( // bypass the pin. const isOperatorReclassify = input.actorTier === "maintainer" && input.allowStickyReclassify === true; + // #1218 review (follow-up): the pin covers every terminal outcome a worker + // token may submit — FIXED and STALE as well as BLOCKED/PARKED. A recorded + // BLOCKED/PARKED settlement never bumps the generation, so a same-generation + // worker FIXED (happy path, refused-head rollback, evidence reopen/force) or + // STALE mark could otherwise release or erase the recorded decision. + // Maintainer-tier callers (operator requeue/reclassification, internal + // reconciliation) keep their broader transitions. const pinToQueuedForStickySettlement = - (nextStatus === "BLOCKED" || nextStatus === "PARKED") && !isOperatorReclassify; + input.actorTier !== "maintainer" && + (nextStatus === "FIXED" || nextStatus === "BLOCKED" || nextStatus === "PARKED" || nextStatus === "STALE") && + !isOperatorReclassify; // Reopen a settlement as a fresh attempt on post-dispatch evidence (#1119): // the guarded write, the cap give-up, and the history row, all inside one @@ -1626,6 +1635,7 @@ export async function markPrFixItem( where: { id: row.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), postDispatchEvidenceKeys: { equals: row.postDispatchEvidenceKeys ?? [] }, }, data: reopenData, @@ -1790,6 +1800,7 @@ export async function markPrFixItem( where: { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: refusalData, }); @@ -1878,12 +1889,12 @@ export async function markPrFixItem( // `data`: it would let a stale write clobber a concurrently recorded // entry. Unqualified marks take the same guarded updateMany path (no // generation clause) so the settle-gap guard applies to them too. - // #1218 review: a worker-tier BLOCKED/PARKED settlement additionally - // requires `status: "QUEUED"` at the row's pinned page — a same-generation - // second mark cannot downgrade a recorded human decision or parked - // classification, and a read→write-gap that flipped the row to BLOCKED - // also no-ops the write. Operator reclassification explicitly opts in - // via `allowStickyReclassify` to bypass the pin. + // #1218 review: a worker-tier terminal settlement (FIXED/BLOCKED/PARKED/ + // STALE) additionally requires `status: "QUEUED"` at the row's pinned + // page — a same-generation second mark cannot release or downgrade a + // recorded human decision or parked classification, and a read→write-gap + // that flipped the row also no-ops the write. Operator reclassification + // explicitly opts in via `allowStickyReclassify` to bypass the pin. if (!reopenPinnedMissed) { const settledRow = await client.$transaction(async (tx) => { const settleWhere: Record = { @@ -1957,9 +1968,9 @@ export async function markPrFixItem( // pinned to the exact keys snapshot this no-reopen decision was made on, // so an entry landing during the head fetch above no-ops the write // instead of being cleared and absorbed. #1218 review: also pin - // `status: QUEUED` for a worker-tier BLOCKED/PARKED settle so a row that - // already flipped into BLOCKED/PARKED during the gap cannot be downgraded - // by a stale settle. + // `status: QUEUED` for a worker-tier terminal settle so a row that + // already flipped into a recorded settlement during the gap cannot be + // overwritten by a stale settle. const clearedRow = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ where: { @@ -2008,9 +2019,9 @@ export async function markPrFixItem( // data already encodes lane/outcome/decisionDetail for the requested // status; writing the cleared-keys update is what the gap-reopen loop's // "no-reopen" branch would have done if it had won the keys pin. - // #1218 review: also pin `status: QUEUED` for a worker-tier BLOCKED/PARKED - // force so a row that already recorded a decision during the gap cannot - // be downgraded. + // #1218 review: also pin `status: QUEUED` for a worker-tier terminal + // force so a row that already recorded a settlement during the gap + // cannot be overwritten. const stickyRow = await client.$transaction(async (tx) => { const { count } = await tx.prFixQueueItem.updateMany({ where: { @@ -2071,6 +2082,7 @@ export async function markPrFixItem( where: { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: forceData, }); From e6360d0cba0775e86ab411888feb8a609f2e95ee Mon Sep 17 00:00:00 2001 From: Jory Irving Date: Mon, 12 Oct 2026 08:42:39 -0600 Subject: [PATCH 7/7] fix(pr-fix): require explicit opt-in for maintainer BLOCKED/PARKED reclassification (#1218 review) --- src/lib/pr-fix-queue.test.ts | 27 +++++++++++++++++++++++++++ src/lib/pr-fix-queue.ts | 18 +++++++++++++----- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/lib/pr-fix-queue.test.ts b/src/lib/pr-fix-queue.test.ts index fa05de2d..e9deef6f 100644 --- a/src/lib/pr-fix-queue.test.ts +++ b/src/lib/pr-fix-queue.test.ts @@ -4726,6 +4726,33 @@ describe("PR-fix outcome separation (#1218)", () => { expect(surfacingMocks.surfacePrFixUnblocked).not.toHaveBeenCalled(); }); + it("a maintainer BLOCKED -> PARKED without allowStickyReclassify no-ops", async () => { + const decision = { question: "Which?", options: ["A", "B"] }; + await enqueuePrFixItem(client, { + repo: "org/repo", pr: 325, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k325", + }); + await markPrFixItem(client, { + repo: "org/repo", pr: 325, status: "BLOCKED", decision, expectedGeneration: 1, + }); + expect(client.items[0].status).toBe("BLOCKED"); + const historyAfterBlocked = client.history.length; + surfacingMocks.surfacePrFixParked.mockClear(); + + // Maintainer tier alone must not be enough: overwriting a recorded + // human decision requires the explicit allowStickyReclassify opt-in. + const downgrade = await markPrFixItem(client, { + repo: "org/repo", pr: 325, status: "PARKED", outcome: "no_progress", expectedGeneration: 1, + actorTier: "maintainer", + }); + + expect(downgrade).toEqual({ mutated: false, reason: "generation-mismatch" }); + expect(client.items[0]).toMatchObject({ + status: "BLOCKED", lane: "NEEDS_HUMAN", outcome: "human_decision", decisionDetail: decision, + }); + expect(client.history).toHaveLength(historyAfterBlocked); + expect(surfacingMocks.surfacePrFixParked).not.toHaveBeenCalled(); + }); + it("an operator reclassification can downgrade BLOCKED -> PARKED with allowStickyReclassify", async () => { await enqueuePrFixItem(client, { repo: "org/repo", pr: 321, lane: "NORMAL", reason: "r", feedback: "f", evidenceKey: "k321", diff --git a/src/lib/pr-fix-queue.ts b/src/lib/pr-fix-queue.ts index 8e215830..4783ceae 100644 --- a/src/lib/pr-fix-queue.ts +++ b/src/lib/pr-fix-queue.ts @@ -1587,12 +1587,20 @@ export async function markPrFixItem( // BLOCKED/PARKED settlement never bumps the generation, so a same-generation // worker FIXED (happy path, refused-head rollback, evidence reopen/force) or // STALE mark could otherwise release or erase the recorded decision. - // Maintainer-tier callers (operator requeue/reclassification, internal - // reconciliation) keep their broader transitions. + // BLOCKED/PARKED keep the explicit opt-in contract from the previous + // review: even a maintainer must pass allowStickyReclassify to overwrite a + // recorded BLOCKED/PARKED classification. FIXED/STALE are pinned for every + // non-maintainer caller; maintainer-tier transitions (operator resolution + // of a blocked item, internal reconciliation) stay unpinned. const pinToQueuedForStickySettlement = - input.actorTier !== "maintainer" && - (nextStatus === "FIXED" || nextStatus === "BLOCKED" || nextStatus === "PARKED" || nextStatus === "STALE") && - !isOperatorReclassify; + ( + (nextStatus === "BLOCKED" || nextStatus === "PARKED") && + !isOperatorReclassify + ) || + ( + (nextStatus === "FIXED" || nextStatus === "STALE") && + input.actorTier !== "maintainer" + ); // Reopen a settlement as a fresh attempt on post-dispatch evidence (#1119): // the guarded write, the cap give-up, and the history row, all inside one