diff --git a/.changeset/fn-8006-plan-review-retry-storm.md b/.changeset/fn-8006-plan-review-retry-storm.md new file mode 100644 index 0000000000..4f190c9cad --- /dev/null +++ b/.changeset/fn-8006-plan-review-retry-storm.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Plan Review no longer loops forever on reviewer retry storms — it fails the task with a clear error. +category: fix +dev: runPlanReviewBeforeExecution now terminalizes RetryStormError (status "failed", serialized error, nextRecoveryAt cleared) instead of re-queuing plan-review-unavailable, which had let reviewerFallbackRetryCount climb unbounded past maxReviewerFallbackRetries. diff --git a/docs/architecture.md b/docs/architecture.md index 6c1fd5e875..9f5f89ac5d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2176,7 +2176,7 @@ UI contract boundary: Fusion derives a per-task `retrySummary` at read time by aggregating retry counters (stuck-kill, recovery, task_done, workflow-step, verification, post-review-fix, merge-conflict bounce, branch-conflict recovery, reviewer context retry, reviewer fallback retry). The engine emits a structured `retry-burned` log channel with `{ taskId, agentId, role, category, attempt, total, breakdown }` so token-cost telemetry can correlate retry burn with spend. -Project settings expose per-category caps (`maxBranchConflictRecoveries`, `maxReviewerContextRetries`, `maxReviewerFallbackRetries`) plus a master cap (`maxTotalRetriesBeforeFail`). When a cap is exceeded, engine code throws `RetryStormError`; executor terminal failure handling serializes this into `task.error` so dashboard surfaces can render structured failure details. +Project settings expose per-category caps (`maxBranchConflictRecoveries`, `maxReviewerContextRetries`, `maxReviewerFallbackRetries`) plus a master cap (`maxTotalRetriesBeforeFail`). When a cap is exceeded, engine code throws `RetryStormError`; executor and triage Plan Review terminal failure handling serialize this into `task.error` so dashboard surfaces can render structured failure details. Plan Review must terminalize this guard rather than re-queue `plan-review-unavailable`, which would otherwise continue burning the reviewer-fallback budget. ## Lifecycle invariants diff --git a/packages/engine/src/__tests__/triage-plan-review-unavailable-retry.test.ts b/packages/engine/src/__tests__/triage-plan-review-unavailable-retry.test.ts index 4dfbc8cefa..18cee3e93a 100644 --- a/packages/engine/src/__tests__/triage-plan-review-unavailable-retry.test.ts +++ b/packages/engine/src/__tests__/triage-plan-review-unavailable-retry.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, vi, afterEach } from "vitest"; -import type { Settings, Task, TaskStore } from "@fusion/core"; +import { RetryStormError, type Settings, type Task, type TaskStore } from "@fusion/core"; import { join } from "node:path"; import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises"; import { readFileSync } from "node:fs"; @@ -209,6 +209,51 @@ describe("Plan Review unavailable retry", () => { ); }); + it("terminalizes a reviewer retry storm instead of scheduling another unavailable retry", async () => { + const rootDir = await createFixtureRoot(); + roots.push(rootDir); + const task = createRetryTask({ id: "FN-PLAN-RETRY-STORM", reviewerFallbackRetryCount: 3 }); + const prompt = `# Task: ${task.id} - Existing draft\n\n## Mission\n\nKeep this exact text.\n`; + await writePrompt(rootDir, task.id, prompt); + const store = createStore(task); + const storm = new RetryStormError({ + category: "reviewerFallback", + total: 3, + cap: 2, + breakdown: { + stuckKill: 0, + recovery: 0, + taskDone: 0, + worktreeSession: 0, + workflowStep: 0, + verification: 0, + postReviewFix: 0, + mergeConflict: 0, + branchConflict: 0, + reviewerContext: 0, + reviewerFallback: 3, + total: 3, + }, + }); + mockReviewStep.mockRejectedValue(storm); + + await retryTask(rootDir, task, store); + + expect(store.updateTask).toHaveBeenCalledWith(task.id, expect.objectContaining({ + status: "failed", + error: expect.stringContaining('"type":"RetryStormError"'), + nextRecoveryAt: null, + })); + expect(store.updateTask).not.toHaveBeenCalledWith(task.id, expect.objectContaining({ + status: "plan-review-unavailable", + })); + expect(store.logEntry).toHaveBeenCalledWith( + task.id, + "[pre-merge] Workflow step failed: Plan Review", + expect.stringContaining('"type":"RetryStormError"'), + ); + }); + it.each([ { name: "missing", contents: null, expectedError: /could not read existing PROMPT\.md/i }, { name: "whitespace-only", contents: " \n\t\n", expectedError: /PROMPT\.md.*(empty|whitespace)/i }, diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 22297022c2..f550ed4408 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -12,6 +12,8 @@ import type { import { DUPLICATE_OF_METADATA_KEY, PLAN_REVIEW_GROUP_ID, + RetryStormError, + serializeRetryStormError, TaskDeletedError, buildTriageMemoryInstructions, isUnplannedSeedPrompt, @@ -2339,6 +2341,7 @@ export class TriageProcessor { instead of step-checkbox language that does not match this gate. Inline PROMPT.md repair remains allowed so the reviewer can fix-and-APPROVE instead of REVISE-looping. */ + let reviewFailure: unknown; const review = await reviewStep( this.rootDir, task.id, @@ -2362,6 +2365,7 @@ export class TriageProcessor { onSessionEnded: (session) => this.unregisterSubagentSession(task.id, session), }, ).catch((error: unknown) => { + reviewFailure = error; const message = error instanceof Error ? error.message : String(error); planLog.warn(`${task.id}: Plan Review unavailable before execution (${message})`); return { @@ -2372,6 +2376,35 @@ export class TriageProcessor { }); const completedAt = new Date().toISOString(); + /* + FNXC:PlanReview 2026-07-15-18:00: + A reviewer-fallback RetryStormError is a terminal guard, not a transient reviewer + outage. Preserve its structured core serialization and stop here so Plan Review does + not re-enter `plan-review-unavailable`, where another poll would re-increment + reviewerFallbackRetryCount beyond the cap. + */ + if (reviewFailure instanceof RetryStormError) { + const terminalError = JSON.stringify(serializeRetryStormError(reviewFailure)); + const output = review.review || reviewFailure.message; + await this.recordPlanReviewWorkflowResult(task, { + workflowStepId: PLAN_REVIEW_GROUP_ID, + workflowStepName: "Plan Review", + phase: "pre-merge", + status: "failed", + output, + notes: review.summary, + startedAt, + completedAt, + }); + await this.store.logEntry(task.id, "[pre-merge] Workflow step failed: Plan Review", terminalError); + await this.store.updateTask(task.id, { + status: "failed", + error: terminalError, + nextRecoveryAt: null, + }); + return "blocked"; + } + if (review.verdict === "APPROVE") { await this.recordPlanReviewWorkflowResult(task, { workflowStepId: PLAN_REVIEW_GROUP_ID,