diff --git a/docs/pr-fix-outcome-policy.md b/docs/pr-fix-outcome-policy.md new file mode 100644 index 00000000..8d9950f4 --- /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`. An `outcome` supplied on a brand-new `NORMAL`-lane enqueue is advisory and only takes effect if/when the item parks (a fresh item starts `QUEUED`). + +## Budgets and recovery + +`fixAttempts` counts dispatchable worker executions. Hand-out reclamation (`handoutReclaims`) and `PR_FIX_HANDOUT_TIMEOUT_MS` wall time are separate budgets: a stale hand-out that never reached a worker does not consume an execution attempt. Stale or already-known feedback also consumes no execution budget. No new numeric thresholds are introduced. + +Automatic enqueue/evidence does not release `BLOCKED` or `PARKED`. An operator explicitly requeues either status, or a maintainer marks the item back to `QUEUED`; requeue releases the parked/blocked state. `FIXED` retains its existing #940 and new-evidence reopen paths. + +## Safety and reporting + +Automatic give-up caps (including attempt, reclaim, refused-`FIXED`, post-dispatch reopen, and failed-report caps) park as `PARKED`/`no_progress`. Infrastructure/authentication and security failures also park, never becoming human design work or being blindly retried. Writes are generation-guarded: a late or stale worker generation cannot overwrite a newer outcome. + +Surfaced metrics distinguish `workerExecutions` (`fixAttempts`) from `enqueueRows` (enqueue history-row count). Enqueue, feedback, and sync/history rows are not worker executions. + +This policy does not claim the exact freshness root cause of PR #1021. Courier #263, which concerns skipping already-consumed pending heads during discovery/deduplication, is a separate scope and is not implemented by this outcome model. 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/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/migrations/20261012000000_add_pr_fix_outcome/migration.sql b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql new file mode 100644 index 00000000..6bdca4a3 --- /dev/null +++ b/prisma/migrations/20261012000000_add_pr_fix_outcome/migration.sql @@ -0,0 +1,9 @@ +-- The new columns are TEXT/JSONB, not the "PrFixStatus"/enum type, so the +-- ALTER TYPE ... ADD VALUE below is never referenced by a column cast inside +-- this migration transaction. Postgres forbids using a newly added enum value +-- in the same transaction that added it; keeping these columns as plain +-- TEXT/JSONB avoids that constraint entirely. +ALTER TYPE "PrFixStatus" ADD VALUE IF NOT EXISTS 'PARKED'; +ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "outcome" TEXT; +ALTER TABLE "PrFixQueueItem" ADD COLUMN IF NOT EXISTS "decisionDetail" JSONB; +ALTER TABLE "PrFixHistory" ADD COLUMN IF NOT EXISTS "outcome" TEXT; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index b41ea725..7a421f15 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. @@ -206,7 +214,10 @@ model PrFixQueueItem { // Bounded automatic reclamations of stale unacknowledged hand-outs (#1211): // counts how many times the reclaimer has reopened this item because a // stamped hand-out was never acknowledged. Past PR_FIX_MAX_RECLAIMS the item - // goes to BLOCKED in the NEEDS_HUMAN lane instead of being reclaimed again. + // parks as PARKED in the NORMAL lane with outcome=`no_progress` (#1218) — + // bounded no-progress exhaustion, never a human decision, so the + // `needs-human` label is never applied. Release requires an explicit + // operator requeue or a maintainer mark to QUEUED. handoutReclaims Int @default(0) author String? history PrFixHistory[] @@ -231,6 +242,7 @@ model PrFixHistory { reason String? @db.Text note String? @db.Text evidenceKey String? + outcome String? @@index([itemId]) @@index([at]) 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 920b713c..da858464 100644 --- a/src/app/api/agents/[agentName]/tasks/report/route.test.ts +++ b/src/app/api/agents/[agentName]/tasks/report/route.test.ts @@ -479,6 +479,111 @@ describe("POST /api/agents/[agentName]/tasks/report — validation", () => { }); }); +describe("POST /api/agents/[agentName]/tasks/report — structured block outcome (#1218)", () => { + beforeEach(() => { + delete process.env.DISPATCH_AUTH_MODE; + resetAuthCaches(); + vi.clearAllMocks(); + }); + + const blockedReport = { + taskType: "followup-pr", + outcome: "blocked", + repoFullName: "o/r", + pullRequestNumber: 5, + prFixItem: { id: "item-1", generation: 1 }, + }; + + it("passes a human_decision blockReason and decision payload through to the resolver", async () => { + const decision = { question: "Which behavior?", options: ["keep", "change"], evidence: "see thread" }; + const res = await postRequest({ + ...blockedReport, + blockReason: "human_decision", + decision, + }); + + expect(res.status).toBe(200); + const body = await res.json(); + expect(body.report.blockReason).toBe("human_decision"); + expect(body.report.decision).toEqual(decision); + expect(prFixResolveMock).toHaveBeenCalledWith( + expect.objectContaining({ + outcome: "blocked", + blockReason: "human_decision", + decision, + }), + ); + }); + + it("accepts a blocked report without a blockReason (bare give-up → parked)", async () => { + const res = await postRequest({ ...blockedReport }); + + expect(res.status).toBe(200); + expect(prFixResolveMock).toHaveBeenCalledWith( + expect.objectContaining({ outcome: "blocked", blockReason: undefined }), + ); + }); + + it("returns 400 for an invalid blockReason", async () => { + const res = await postRequest({ ...blockedReport, blockReason: "not-a-real-outcome" }); + + expect(res.status).toBe(400); + expect((await res.json()).error).toMatch(/blockReason/); + expect(mockAgentRun.create).not.toHaveBeenCalled(); + expect(prFixResolveMock).not.toHaveBeenCalled(); + }); + + it("returns 400 when blockReason is not a string", async () => { + const res = await postRequest({ ...blockedReport, blockReason: 42 }); + + expect(res.status).toBe(400); + expect((await res.json()).error).toMatch(/blockReason/); + }); + + it("returns 400 when decision is not a non-array object", async () => { + const asString = await postRequest({ ...blockedReport, decision: "nope" }); + expect(asString.status).toBe(400); + expect((await asString.json()).error).toMatch(/decision must be an object/); + + const asArray = await postRequest({ ...blockedReport, decision: ["a", "b"] }); + expect(asArray.status).toBe(400); + expect((await asArray.json()).error).toMatch(/decision must be an object/); + }); + + it("returns 400 when decision fields are malformed", async () => { + const badQuestion = await postRequest({ ...blockedReport, decision: { question: 123 } }); + expect(badQuestion.status).toBe(400); + expect((await badQuestion.json()).error).toMatch(/decision\.question/); + + const badOptions = await postRequest({ ...blockedReport, decision: { options: "not-an-array" } }); + expect(badOptions.status).toBe(400); + expect((await badOptions.json()).error).toMatch(/decision\.options/); + + const nonStringOptions = await postRequest({ ...blockedReport, decision: { options: ["ok", 5] } }); + expect(nonStringOptions.status).toBe(400); + expect((await nonStringOptions.json()).error).toMatch(/decision\.options/); + + const badEvidence = await postRequest({ ...blockedReport, decision: { evidence: 5 } }); + expect(badEvidence.status).toBe(400); + expect((await badEvidence.json()).error).toMatch(/decision\.evidence/); + }); + + it("normalizes a valid decision payload (trims strings, drops empty options)", async () => { + const res = await postRequest({ + ...blockedReport, + blockReason: "human_decision", + decision: { question: " Which? ", options: [" A ", "", "B"], evidence: " ctx " }, + }); + + expect(res.status).toBe(200); + expect(prFixResolveMock).toHaveBeenCalledWith( + expect.objectContaining({ + decision: { question: "Which?", options: ["A", "B"], evidence: "ctx" }, + }), + ); + }); +}); + describe("POST /api/agents/[agentName]/tasks/report — AgentRun persistence", () => { beforeEach(() => { delete process.env.DISPATCH_AUTH_MODE; diff --git a/src/app/api/agents/[agentName]/tasks/report/route.ts b/src/app/api/agents/[agentName]/tasks/report/route.ts index 14c9de76..8d0d5737 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, getBoundWorkerAgent } 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"; import { resolveAlreadyDoneFromAgentReport, type ReportCloseCriterion, @@ -47,6 +48,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; // #1143: the already_done close proposal. The worker supplies per-criterion // pinned excerpts; Dispatch re-derives the catalog and applies the shared // close policy. Not close authorization on its own. @@ -282,6 +289,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/#1143: evidence is meaningful for an already_addressed settlement and // for an already_done close — it is trimmed, bounded, and recorded in the // settlement/close history. On any other outcome it is ignored (never hashed, @@ -432,6 +483,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, close: closeCriteria ? { criteria: closeCriteria } : undefined, // #1120: normalized worker start time; undefined → report time (now). startedAt: resolveReportedStartedAt(raw.startedAt, now), @@ -536,6 +591,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.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 b6ca81a4..f4489a3f 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: { @@ -53,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. @@ -112,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 @@ -135,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/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/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 14e236d7..649d2ba7 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(() => []), @@ -245,18 +246,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 () => { @@ -290,25 +324,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", }), }), ); @@ -530,9 +566,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 }) @@ -548,13 +584,16 @@ 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); + // #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: [] } }, - data: expect.objectContaining({ status: "BLOCKED" }), + where: { id: "item-42", generation: 1, postDispatchEvidenceKeys: { equals: [] }, status: "QUEUED" }, + data: expect.objectContaining({ status: "PARKED" }), }); expect(prisma.prFixQueueItem.update).not.toHaveBeenCalled(); expect(prisma.prFixHistory.create).not.toHaveBeenCalled(); @@ -627,4 +666,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 8cf36af8..e9deef6f 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(); @@ -2072,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"); }); @@ -2081,15 +2120,16 @@ 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 () => { 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); }); @@ -2104,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"); }); }); @@ -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(); @@ -2143,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", @@ -2162,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 () => { @@ -2267,7 +2320,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("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", @@ -2285,17 +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("BLOCKED"); - expect(capped.lane).toBe("NEEDS_HUMAN"); - 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 () => { @@ -2680,7 +2737,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 +2763,22 @@ 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. 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 () => { @@ -2827,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", @@ -2841,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 () => { @@ -2873,7 +2938,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 +2950,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 +3110,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 +3459,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,37 +3556,83 @@ 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 { 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]; - 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(); + // 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; } }); - it("accumulates the reclaim bound across repeated sweeps until the cap routes to a human", async () => { + 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 // a stale hand-out once the item is re-stamped stale — mirroring next-task @@ -3542,7 +3658,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 +3667,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 +4109,711 @@ 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); + }); + + 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", + }); + 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(); + + // #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", + 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(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); + }); + + // #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.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("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", + }); + 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 ad9cf5c8..4783ceae 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,36 @@ 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; + /** + * 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 { @@ -109,6 +147,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 +210,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 +232,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 +268,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 +430,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 +459,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 +482,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; } @@ -632,6 +735,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; @@ -669,7 +780,10 @@ async function defaultIsPrMergedOrClosed(repo: string, pr: number): Promise= 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 +933,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,16 +948,45 @@ 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).`, }, }); } 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; } @@ -942,22 +1090,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 +1124,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 +1222,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 +1234,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 +1307,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 +1333,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 }); - } else if (previousStatus === "BLOCKED" && item.status === "QUEUED") { - await retractNeedsHuman(input.repo, input.pr, "requeued", "New evidence reopened this 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 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 +1446,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,11 +1566,42 @@ 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 }; + // #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; + // #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. + // 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 = + ( + (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 // tiny transaction the caller passes in. Pinned on the row's id, its @@ -1379,13 +1615,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 @@ -1397,6 +1643,7 @@ export async function markPrFixItem( where: { id: row.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), postDispatchEvidenceKeys: { equals: row.postDispatchEvidenceKeys ?? [] }, }, data: reopenData, @@ -1408,9 +1655,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 +1667,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,17 +1684,19 @@ 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). - // 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 - // 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"; + 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 @@ -1457,8 +1714,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 +1748,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 +1773,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: [], @@ -1524,6 +1808,7 @@ export async function markPrFixItem( where: { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: refusalData, }); @@ -1535,18 +1820,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 +1849,35 @@ 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, + // 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") && + (nextStatus === "QUEUED" || nextStatus === "FIXED") + ) { await retractNeedsHuman(input.repo, input.pr, nextStatus === "FIXED" ? "resolved" : "requeued", input.note); } return { mutated: true, item: row }; @@ -1581,18 +1897,32 @@ 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 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 = { 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; 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; }); @@ -1627,7 +1957,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."), ); @@ -1641,13 +1975,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 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: { id: fresh.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), postDispatchEvidenceKeys: { equals: freshKeys }, + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: { ...data, postDispatchEvidenceKeys: [] }, }); @@ -1659,6 +1997,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).", @@ -1673,22 +2012,72 @@ 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. + // #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: { + id: existing.id, + ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), + }, + 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 - ? { 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 @@ -1701,6 +2090,7 @@ export async function markPrFixItem( where: { id: existing.id, ...(expectedGeneration !== undefined ? { generation: expectedGeneration } : {}), + ...(pinToQueuedForStickySettlement ? { status: "QUEUED" } : {}), }, data: forceData, }); @@ -1711,13 +2101,14 @@ 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; 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(" "), @@ -1762,7 +2153,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 +2214,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 +2319,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 +2363,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 +2382,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 +2407,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; @@ -2075,6 +2470,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 @@ -2084,7 +2484,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 @@ -2240,12 +2640,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 { @@ -2258,9 +2667,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, }; } @@ -2302,13 +2711,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 @@ -2337,10 +2756,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) @@ -2365,16 +2785,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..0b59565a 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,155 @@ 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); + }); + + 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); }); }); @@ -392,4 +533,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..674b7b10 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,34 @@ 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; + /** + * 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. */ + commentUpdated: boolean; + /** True when the PR was already merged or closed, so nothing was written. */ + skippedTerminal: boolean; + errors: string[]; } export interface SurfacePrFixBlockedResult { @@ -45,9 +86,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 +97,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 +142,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 +248,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 +340,75 @@ 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 = { + labelRemoved: false, + 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; + } + + // 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)); + 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 +484,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";