From 214af98591c399542e371c08c45cb16b1c616676 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 16:29:10 -0700 Subject: [PATCH] FN-7977: hold Plan Review provider failures without replan regression Prevent provider, model, transport, and abort failures from bouncing tasks back to planning after they enter execution. - Classify non-plan-defect Plan Review failures and skip needs-replan handoff - Terminate graph traversal with plan-review-provider-failure-hold and retry in place - Guard triage recovery so advanced column/worktree/step state is never overwritten - Document planning-recovery no-regression invariant and add regression tests - Add patch changeset for the operator-facing fix Files changed: .changeset/fn-7977-planning-failure-no-regression.md | 7 ++ docs/architecture.md | 1 + docs/workflow-steps.md | 2 +- packages/engine/src/__tests__/replan-target.test.ts | 17 +++- packages/engine/src/__tests__/transient-error-detector.test.ts | 32 +++++- packages/engine/src/__tests__/triage.test.ts | 110 +++++++++++++++++++++ packages/engine/src/__tests__/workflow-graph-optional-group.test.ts | 46 ++++++++- packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts | 36 +++++++ packages/engine/src/executor.ts | 62 +++++++++++- packages/engine/src/replan-target.ts | 22 +++++ packages/engine/src/transient-error-detector.ts | 37 +++++++ packages/engine/src/triage.ts | 73 +++++++++++--- packages/engine/src/workflow-graph-executor.ts | 45 ++++++++- 13 files changed, 466 insertions(+), 24 deletions(-) Fusion-Task-Id: FN-7977 Fusion-Task-Lineage: 6d62d3ca-c6f3-4d02-a377-d7fd59f0c0f9 Co-authored-by: Fusion (runfusion.ai) --- .../fn-7977-planning-failure-no-regression.md | 7 ++ docs/architecture.md | 1 + docs/workflow-steps.md | 2 +- .../src/__tests__/replan-target.test.ts | 17 ++- .../transient-error-detector.test.ts | 32 ++++- packages/engine/src/__tests__/triage.test.ts | 110 ++++++++++++++++++ .../workflow-graph-optional-group.test.ts | 46 +++++++- .../workflow-graph-optional-step-fix.test.ts | 36 ++++++ packages/engine/src/executor.ts | 62 +++++++++- packages/engine/src/replan-target.ts | 22 ++++ .../engine/src/transient-error-detector.ts | 37 ++++++ packages/engine/src/triage.ts | 73 ++++++++++-- .../engine/src/workflow-graph-executor.ts | 45 ++++++- 13 files changed, 466 insertions(+), 24 deletions(-) create mode 100644 .changeset/fn-7977-planning-failure-no-regression.md diff --git a/.changeset/fn-7977-planning-failure-no-regression.md b/.changeset/fn-7977-planning-failure-no-regression.md new file mode 100644 index 0000000000..8c76d4173a --- /dev/null +++ b/.changeset/fn-7977-planning-failure-no-regression.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Transient provider failures of the Plan Review gate no longer bounce tasks back to planning. +category: fix +dev: workflow-graph-executor shouldRequestPreMergeFix + executor requestPreMergeOptionalStepFix now classify plan-review hard failures via isTransientError/isOperatorActionableAgentError/model-fallback signatures and skip the needs-replan handoff for non-plan-defect failures; genuine REVISE still replans. Fixes issue #2124 / FN-7977. diff --git a/docs/architecture.md b/docs/architecture.md index 6c5380fff7..97b45652ae 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -2179,6 +2179,7 @@ Project settings expose per-category caps (`maxBranchConflictRecoveries`, `maxRe This section preserves the detailed lifecycle/self-healing contracts that were formerly in `AGENTS.md`. +- **Planning-recovery no-regression (FN-7977)**: a provider, model-selection, transport, or deterministic planning failure may only mutate a task after re-reading its live row and proving it remains in the planning stage. Execution/terminal columns, a worktree, or materialized steps prove advancement; stale triage recovery must leave that column, status, worktree, and step progress untouched. A genuine Plan Review `REVISE` remains a separate, explicit replan signal. - **Orphan `fusion/*` branches**: branches with zero unique commits vs `main` are pruned by `cleanupOrphanedBranches` (`branch:orphan-prune`). Branches with unique commits are not auto-rescued; operators inspect and clean them manually via standard git tooling (`git branch -D`, `git worktree remove`, etc.). - **Stale active branches**: self-healing's `reclaim-stale-active-branches` stage prunes a `fusion/` branch with zero unique commits when no usable worktree mapping exists, then clears `task.branch`/`task.worktree`/`task.baseCommitSha`. It must defer reclaim (emit `branch:stale-active-reclaim-deferred`) when the task worktree is in `activeSessionRegistry`, when `executionStartedAt` is within `STALE_ACTIVE_BRANCH_EXECUTION_GRACE_MS` (10 minutes), or when the mapped worktree has uncommitted changes. - **Worktree metadata reconcile ordering (FN-4962)**: `reconcile-task-worktree-metadata` must run before `reclaim-stale-active-branches`; stale `task.worktree` metadata is rebound to live `fusion/` worktrees when present (`task:auto-recover-worktree-metadata-rebound`) or cleared (`task:auto-recover-worktree-metadata-cleared`) when absent. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 3609319715..37b65abb12 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -212,7 +212,7 @@ The default built-in catalog entry `builtin:coding` is backed by a Stepwise-deri - `triage` → `plan` → `plan-review` (default-on optional plan review) → `parse-steps` → `foreach(step-execute)` → `browser-verification` (optional) → `code-review` (default-on optional final review) → `merge-gate` / branch-group integration / `merge-attempt` / retry or manual hold → `end` -If the Plan Review reviewer is unavailable before producing a verdict, the task stays in triage as `status: "plan-review-unavailable"` with a short backoff. That retry state is not a replan: Fusion rereads the existing non-empty `PROMPT.md`, preserves it unchanged, and reruns only Plan Review/finalization while holding a global agent concurrency slot for the reviewer lane. A reviewer revision verdict moves the task to `needs-replan`; missing/empty/invalid prompt content fails clearly instead of restarting the planner. +If the Plan Review reviewer is unavailable before producing a verdict, the task stays in triage as `status: "plan-review-unavailable"` with a short backoff. That retry state is not a replan: Fusion rereads the existing non-empty `PROMPT.md`, preserves it unchanged, and reruns only Plan Review/finalization while holding a global agent concurrency slot for the reviewer lane. The graph applies the same invariant after execution has started: a transient/provider/model/abort Plan Review failure without a `REVISE` verdict remains visible in its current task state and never moves the task backward to `needs-replan`. A reviewer revision verdict moves the task to `needs-replan`; missing/empty/invalid prompt content fails clearly instead of restarting the planner. Workflow Plan Review is separate from manual plan approval. Project `planApprovalMode: "auto-approve-all"` bypasses only the final manual `awaiting-approval` plan gate after the plan is specified and any enabled Plan Review passes; it does not disable Plan Review or other explicit safety gates. diff --git a/packages/engine/src/__tests__/replan-target.test.ts b/packages/engine/src/__tests__/replan-target.test.ts index da38c75454..4e0146ed26 100644 --- a/packages/engine/src/__tests__/replan-target.test.ts +++ b/packages/engine/src/__tests__/replan-target.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import type { TaskStore } from "@fusion/core"; -import { moveTaskToReplanColumn, resolveReplanTargetColumn } from "../replan-target.js"; +import { hasAdvancedPastPlanning, isTaskStillInPlanningStage, moveTaskToReplanColumn, resolveReplanTargetColumn } from "../replan-target.js"; /* FNXC:WorkflowReplan 2026-07-12-23:55: @@ -18,6 +18,21 @@ function storeWithSelection(workflowId: string | undefined): TaskStore { } as unknown as TaskStore; } +describe("planning-stage guard", () => { + it.each([ + [{ column: "triage", worktree: null, steps: [] }, true, "empty triage task"], + [{ column: "todo", worktree: null, steps: [] }, true, "unplanned todo seed"], + [{ column: "todo", worktree: "/tmp/FN-1", steps: [] }, false, "todo task with a worktree"], + [{ column: "todo", worktree: null, steps: [{ id: "step-1" }] }, false, "todo task with materialized steps"], + [{ column: "in-progress", worktree: null, steps: [] }, false, "in-progress task"], + [{ column: "in-review", worktree: null, steps: [] }, false, "in-review task"], + [{ column: "done", worktree: null, steps: [] }, false, "completed task"], + ] as const)("recognizes %s", (task, expected) => { + expect(isTaskStillInPlanningStage(task)).toBe(expected); + expect(hasAdvancedPastPlanning(task)).toBe(!expected); + }); +}); + describe("resolveReplanTargetColumn", () => { it("targets triage for the default Coding workflow", async () => { const store = storeWithSelection("builtin:coding"); diff --git a/packages/engine/src/__tests__/transient-error-detector.test.ts b/packages/engine/src/__tests__/transient-error-detector.test.ts index e58e90c603..751dc43810 100644 --- a/packages/engine/src/__tests__/transient-error-detector.test.ts +++ b/packages/engine/src/__tests__/transient-error-detector.test.ts @@ -11,6 +11,7 @@ import { isProviderModelNotFoundError, isUnsupportedMessageRoleError, isNonContinuableSessionError, + isNonPlanDefectPlanReviewFailure, TRANSIENT_ERROR_PATTERNS, } from "../transient-error-detector.js"; import { isUsageLimitError } from "../usage-limit-detector.js"; @@ -66,8 +67,8 @@ describe("Transient Error Detector", () => { expect(isTransientError("Connection Reset")).toBe(true); }); - it("matches 'ECONNREFUSED'", () => { - expect(isTransientError("ECONNREFUSED")).toBe(true); + it("matches connection reset errno messages", () => { + expect(isTransientError("ECONNRESET")).toBe(true); expect(isTransientError("Error: ECONNREFUSED")).toBe(true); }); @@ -174,6 +175,33 @@ describe("Transient Error Detector", () => { }); }); + describe("isNonPlanDefectPlanReviewFailure", () => { + it.each([ + "429 Too Many Requests from the provider", + "403 forbidden: model access is not enabled for this account", + "Unable to select a usable model after 2 attempts (primary example/model)", + "ECONNRESET while contacting reviewer", + "WebSocket closed 1006", + "request was aborted", + ])("keeps provider failure in place: %s", (errorMessage) => { + expect(isNonPlanDefectPlanReviewFailure({ errorMessage })).toBe(true); + }); + + it("keeps raw abort and exception failure values in place", () => { + expect(isNonPlanDefectPlanReviewFailure({ failureValue: "exception" })).toBe(true); + expect(isNonPlanDefectPlanReviewFailure({ failureValue: "aborted" })).toBe(true); + }); + + it("never classifies a genuine REVISE verdict as a provider failure", () => { + expect(isNonPlanDefectPlanReviewFailure({ + verdict: "REVISE", + errorMessage: "429 Too Many Requests", + failureValue: "exception", + })).toBe(false); + expect(isNonPlanDefectPlanReviewFailure({ errorMessage: "PROMPT.md is missing acceptance criteria" })).toBe(false); + }); + }); + describe("classifyError", () => { it("classifies usage limit errors as 'usage-limit'", () => { expect(classifyError("rate limit exceeded")).toBe("usage-limit"); diff --git a/packages/engine/src/__tests__/triage.test.ts b/packages/engine/src/__tests__/triage.test.ts index 3519c3cb7e..57a0e6900b 100644 --- a/packages/engine/src/__tests__/triage.test.ts +++ b/packages/engine/src/__tests__/triage.test.ts @@ -4869,6 +4869,116 @@ describe("taskCreate tool model inheritance", () => { // fallbackProvider/fallbackModelId via mockCreateFnAgent call args here. }); + it.each([ + ["transient provider failure", "upstream connect error"], + ["operator-actionable provider failure", "No API key for provider: anthropic"], + ["generic planning failure", "planner protocol failed"], + ])("keeps an advanced task in place after a %s", async (_label, errorMessage) => { + const task = { + id: "FN-7977-ADVANCED", + description: "Do not overwrite execution after a stale planning run", + column: "triage", + status: "planning", + dependencies: [], + steps: [], + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as unknown as Task; + let liveTask = { ...task, attachments: [], comments: [] } as unknown as Task; + const store = createMockStore({ + getTask: vi.fn().mockImplementation(async () => liveTask), + updateTask: vi.fn().mockImplementation(async (_id: string, patch: Partial) => { + if (patch.status === "planning") { + liveTask = { + ...liveTask, + column: "in-progress", + status: "executing", + worktree: "/tmp/fusion/FN-7977-ADVANCED", + steps: [{ id: "implementation", status: "in-progress" }], + } as unknown as Task; + } + }), + }); + mockCreateFnAgent.mockRejectedValue(new Error(errorMessage)); + + await new TriageProcessor(store, "/test/root", { pollIntervalMs: 100_000 }).specifyTask(task); + + expect(liveTask).toMatchObject({ + column: "in-progress", + status: "executing", + worktree: "/tmp/fusion/FN-7977-ADVANCED", + steps: [{ id: "implementation", status: "in-progress" }], + }); + expect(store.updateTask).toHaveBeenCalledTimes(1); + expect(store.updateTask).toHaveBeenCalledWith("FN-7977-ADVANCED", { status: "planning" }); + }); + + it("keeps advanced worktree and steps after model fallback exhaustion", async () => { + const task = { + id: "FN-7977-MODEL", + description: "Preserve execution after planner model fallback exhaustion", + column: "triage", + dependencies: [], + steps: [], + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as unknown as Task; + let liveTask = { ...task, attachments: [], comments: [] } as unknown as Task; + const store = createMockStore({ + getTask: vi.fn().mockImplementation(async () => liveTask), + updateTask: vi.fn().mockImplementation(async (_id: string, patch: Partial) => { + if (patch.status === "planning") { + liveTask = { ...liveTask, column: "in-review", status: "reviewing", worktree: "/tmp/FN-7977-MODEL", steps: [{ id: "1" }] } as unknown as Task; + } + }), + }); + mockCreateFnAgent.mockResolvedValue({ session: { state: {}, sessionManager: {}, prompt: vi.fn(), dispose: vi.fn(), navigateTree: vi.fn() } }); + const { ModelFallbackExhaustedError, promptWithFallback } = await import("../pi.js"); + (promptWithFallback as ReturnType).mockRejectedValueOnce(new ModelFallbackExhaustedError({ + primaryModel: "antigravity/gemini-3.5-flash-low", + attempts: 2, + triggerPoint: "prompt-time", + underlyingReason: "403 provider access forbidden", + })); + + await new TriageProcessor(store, "/test/root", { pollIntervalMs: 100_000 }).specifyTask(task); + + expect(liveTask).toMatchObject({ column: "in-review", status: "reviewing", worktree: "/tmp/FN-7977-MODEL", steps: [{ id: "1" }] }); + expect(store.updateTask).toHaveBeenCalledTimes(1); + }); + + it("keeps advanced worktree and steps after deterministic validation recovery", async () => { + const task = { + id: "FN-7977-VALIDATION", + description: "Preserve execution after stale deterministic validation retry", + column: "triage", + dependencies: [], + steps: [], + log: [], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + } as unknown as Task; + let liveTask = { ...task, attachments: [], comments: [] } as unknown as Task; + const store = createMockStore({ + getTask: vi.fn().mockImplementation(async () => liveTask), + updateTask: vi.fn().mockImplementation(async (_id: string, patch: Partial) => { + if (patch.status === "planning") { + liveTask = { ...liveTask, column: "in-progress", status: "executing", worktree: "/tmp/FN-7977-VALIDATION", steps: [{ id: "1" }] } as unknown as Task; + } + }), + }); + mockCreateFnAgent.mockResolvedValue({ session: { state: {}, sessionManager: {}, prompt: vi.fn(), dispose: vi.fn(), navigateTree: vi.fn() } }); + const { promptWithFallback } = await import("../pi.js"); + (promptWithFallback as ReturnType).mockResolvedValueOnce(undefined); + + await new TriageProcessor(store, "/test/root", { pollIntervalMs: 100_000 }).specifyTask(task); + + expect(liveTask).toMatchObject({ column: "in-progress", status: "executing", worktree: "/tmp/FN-7977-VALIDATION", steps: [{ id: "1" }] }); + expect(store.updateTask).toHaveBeenCalledTimes(1); + }); + it("escalates to error state when triage retries are exhausted via specifyTask", async () => { const task = { id: "FN-201", diff --git a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts index 912d40b0c2..4699a3cfcc 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it, vi } from "vitest"; import { BUILTIN_CODING_WORKFLOW_IR, BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "@fusion/core"; import type { TaskDetail, WorkflowIr } from "@fusion/core"; -import { WorkflowGraphExecutor, type WorkflowNodeHandler } from "../workflow-graph-executor.js"; +import { + PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE, + WorkflowGraphExecutor, + type WorkflowNodeHandler, +} from "../workflow-graph-executor.js"; /* FNXC:WorkflowOptionalGroup 2026-06-21-14:05: @@ -466,6 +470,46 @@ describe("WorkflowGraphExecutor optional-group", () => { ])); }); + it("keeps transient Plan Review provider failures in place without synthesizing REVISE", async () => { + const requestFix = vi.fn(async () => true); + const records: unknown[] = []; + const executor = new WorkflowGraphExecutor({ + handlers: { + prompt: async (node) => node.id === "plan-review-step" + ? { + outcome: "failure", + value: "exception", + contextPatch: { + output: "Unable to select a usable model after 2 attempts (429 Too Many Requests)", + }, + } + : { outcome: "success" }, + }, + recordWorkflowStepResult: async (_taskId, result) => { records.push(result); }, + requestPreMergeOptionalStepFix: requestFix, + }); + + const result = await executor.run( + taskWith(["plan-review"]), + settingsOn(), + BUILTIN_CODING_WORKFLOW_IR, + ); + + expect(requestFix).not.toHaveBeenCalled(); + expect(result.outcome).toBe("failure"); + expect(result.context["node:plan-review:value"]).toBe(PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE); + expect(result.visitedNodeIds).toContain("plan-review"); + expect(result.visitedNodeIds).not.toContain("plan-replan"); + expect(result.visitedNodeIds).not.toContain("execute"); + expect(records).toEqual(expect.arrayContaining([ + expect.objectContaining({ + workflowStepId: "plan-review", + status: "failed", + output: expect.stringContaining("Unable to select a usable model"), + }), + ])); + }); + it("uses an explicit graph replan node for Plan Review REVISE and does not execute before replan completes", async () => { const requestFix = vi.fn(async () => true); const calls: string[] = []; diff --git a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts index 4db53bfbea..5155c9083f 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts @@ -303,6 +303,42 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { expect(store.moveTask).not.toHaveBeenCalled(); }); + it.each([ + { label: "rate limited provider", feedback: "429 Too Many Requests", failureValue: undefined }, + { label: "model fallback exhaustion", feedback: "Unable to select a usable model after 2 attempts", failureValue: undefined }, + { label: "operator-actionable model access", feedback: "403 forbidden: insufficient permissions for this model", failureValue: undefined }, + { label: "network transport", feedback: "ECONNRESET while contacting reviewer", failureValue: undefined }, + { label: "websocket transport", feedback: "WebSocket closed 1006", failureValue: undefined }, + { label: "abort diagnostic", feedback: "request was aborted", failureValue: undefined }, + { label: "raw exception", feedback: "(no feedback captured)", failureValue: "exception" }, + { label: "raw abort", feedback: "(no feedback captured)", failureValue: "aborted" }, + ])("keeps a $label Plan Review failure in place without replanning", async ({ feedback, failureValue }) => { + const store = createMockStore(); + const liveTask = task({ column: "in-progress", status: null }); + store.getTask.mockResolvedValue(liveTask); + const executor = new TaskExecutor(store, "/tmp/test"); + + const scheduled = await (executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, { + stepName: "Plan Review", + feedback, + phase: "pre-merge" as const, + status: "failed" as const, + verdict: undefined, + failureValue, + nodeId: "plan-review", + }); + + expect(scheduled).toBe(false); + expect(store.moveTask).not.toHaveBeenCalled(); + expect(store.updateTask).not.toHaveBeenCalledWith(liveTask.id, expect.objectContaining({ status: "needs-replan" }), undefined); + expect(store.logEntry).toHaveBeenCalledWith( + liveTask.id, + "Plan Review provider failure — task kept in place", + expect.stringContaining(liveTask.column), + undefined, + ); + }); + it("clears stale pause-abort provenance silently before a fresh unpaused execution dispatch", async () => { const store = createMockStore(); const liveTask = task({ column: "todo", paused: false, userPaused: false }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 4a6ee63426..be3113a705 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -44,7 +44,12 @@ import { type ForeachActiveContext, type WorkflowLegacySeams, } from "./workflow-node-handlers.js"; -import { MERGE_REGION_KINDS, WORKFLOW_NODE_ENGINE_PAUSE_ABORT_KIND, WORKFLOW_OPTIONAL_GROUP_CONTEXT_KEY } from "./workflow-graph-executor.js"; +import { + MERGE_REGION_KINDS, + PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE, + WORKFLOW_NODE_ENGINE_PAUSE_ABORT_KIND, + WORKFLOW_OPTIONAL_GROUP_CONTEXT_KEY, +} from "./workflow-graph-executor.js"; import type { WorkflowNodePreparationRequirement, WorkflowNodeResult } from "./workflow-graph-executor.js"; import { workflowNodeRequiresWorktree } from "./workflow-node-execution-needs.js"; import type { @@ -166,7 +171,7 @@ import { AgentLogger } from "./agent-logger.js"; import { createLogger, executorLog, reviewerLog, formatError } from "./logger.js"; import { TokenCapDetector } from "./token-cap-detector.js"; import { isUsageLimitError, checkSessionError, type UsageLimitPauser } from "./usage-limit-detector.js"; -import { isNonContinuableSessionError, isTransientError, isSilentTransientError } from "./transient-error-detector.js"; +import { isNonContinuableSessionError, isNonPlanDefectPlanReviewFailure, isTransientError, isSilentTransientError } from "./transient-error-detector.js"; import { withRateLimitRetry } from "./rate-limit-retry.js"; import { detectExternalIntegrationEvidenceGaps, @@ -4480,6 +4485,8 @@ export class TaskExecutor { phase: CoreWorkflowStepResult["phase"]; status: CoreWorkflowStepResult["status"]; verdict?: string; + /** Raw graph node result when no reviewer verdict was produced. */ + failureValue?: string; nodeId?: string; maxRevisions?: unknown; }, @@ -4496,6 +4503,26 @@ export class TaskExecutor { */ if (info.status === "advisory_failure" && info.verdict !== "REVISE") return false; if (info.verdict !== undefined && info.verdict !== "REVISE") return false; + /* + * FNXC:PlanReviewReplan 2026-07-15-12:00: + * FN-7977 / issue #2124: graph traversal is the primary guard, but this + * compatibility seam also receives explicit remediation edges and future + * callers. A provider/model/transport failure without a genuine REVISE must + * be logged and left in its current execution column, never sent to replan. + */ + if (isNonPlanDefectPlanReviewFailure({ + verdict: info.verdict, + errorMessage: info.feedback, + failureValue: info.failureValue, + })) { + await this.store.logEntry( + taskId, + "Plan Review provider failure — task kept in place", + `Plan Review failed without a REVISE verdict due to a provider, model, transport, or abort condition. The task remains in ${liveTask.column}; no automatic replan was scheduled.\n\nDiagnostic:\n${info.feedback}`, + this.getRunContextFor(taskId), + ); + return false; + } /* * FNXC:PlanReviewReplan 2026-06-29-00:41: * Plan Review is pre-execution spec validation, so a failed/revision result @@ -8835,6 +8862,37 @@ export class TaskExecutor { return; } const live = loadedLive; + if (this.graphFailureValue(result) === PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE) { + /* + * FNXC:PlanReviewReplan 2026-07-15-16:35: + * FN-7977: graph-native Plan Review provider failures are a bounded + * in-place retry. They must not follow the built-in failure edge into + * plan-replan or overwrite a progressed card's column, worktree, or steps. + */ + const priorRetries = live.graphResumeRetryCount ?? 0; + if (priorRetries < MAX_TRANSIENT_GRAPH_RESUME_RETRIES) { + const nextRetries = priorRetries + 1; + const message = `Plan Review provider failure — retrying in place (${nextRetries}/${MAX_TRANSIENT_GRAPH_RESUME_RETRIES})`; + executorLog.warn(`${task.id}: ${message}`); + await this.store.logEntry(task.id, message, undefined, this.getRunContextFor(task.id)); + await this.store.updateTask(task.id, { + graphResumeRetryCount: nextRetries, + }, this.getRunContextFor(task.id)); + const scheduleRetry = () => { + this.execute(live).catch((err) => + executorLog.error(`Failed Plan Review provider retry for ${task.id}:`, err), + ); + }; + const handle = setTimeout(scheduleRetry, TRANSIENT_GRAPH_RESUME_RETRY_BACKOFF_MS); + handle.unref?.(); + } else { + const message = "Plan Review provider retry budget exhausted — task remains held in its current state"; + executorLog.warn(`${task.id}: ${message}`); + await this.store.logEntry(task.id, message, undefined, this.getRunContextFor(task.id)); + } + await this.persistTokenUsage(task.id); + return; + } if (live.mergeDetails?.mergeConfirmed === true && live.column !== "done") { if (await this.finalizeMergeConfirmedWorkflowGraphTask(live.id, "graph-failure")) { await this.persistTokenUsage(task.id); diff --git a/packages/engine/src/replan-target.ts b/packages/engine/src/replan-target.ts index 2704a7af65..1eb7439d2e 100644 --- a/packages/engine/src/replan-target.ts +++ b/packages/engine/src/replan-target.ts @@ -20,6 +20,28 @@ service scans, so parking a needs-replan card in their custom entry column stran write. "triage" preserves the pre-workflow-aware behavior for these workflows: the move is legal from every legacy column and eligibleTriageTasks re-specifies unconditionally. */ +/* + * FNXC:WorkflowReplan 2026-07-15-13:15: + * FN-7977: a planning/provider recovery may finish after another engine lane has + * started execution. Recovery callers must prove the live row is still planning + * before writing planning state; worktrees, materialized steps, and execution or + * terminal columns are durable evidence that the task has advanced. + */ +export function hasAdvancedPastPlanning(task: Pick): boolean { + return ( + task.column === "in-progress" + || task.column === "in-review" + || task.column === "done" + || task.column === "archived" + || task.worktree != null + || (task.steps?.length ?? 0) > 0 + ); +} + +export function isTaskStillInPlanningStage(task: Pick): boolean { + return !hasAdvancedPastPlanning(task); +} + export async function resolveReplanTargetColumn(store: TaskStore, taskId: string): Promise { try { const ir = await resolveWorkflowIrForTask(store, taskId); diff --git a/packages/engine/src/transient-error-detector.ts b/packages/engine/src/transient-error-detector.ts index 4d4d5326e2..bbf89995f4 100644 --- a/packages/engine/src/transient-error-detector.ts +++ b/packages/engine/src/transient-error-detector.ts @@ -42,6 +42,7 @@ export const TRANSIENT_ERROR_PATTERNS: RegExp[] = [ // Connection establishment failures - usually temporary /Connection refused/i, /connection reset/i, + /ECONNRESET/i, /ECONNREFUSED/i, /ETIMEDOUT/i, /socket hang up/i, @@ -104,6 +105,42 @@ export function isTransientError(errorMessage: string): boolean { return TRANSIENT_ERROR_PATTERNS.some((pattern) => pattern.test(errorMessage)); } +/* + * FNXC:PlanReviewReplan 2026-07-15-12:00: + * FN-7977 / issue #2124: a Plan Review provider, model-selection, or transport + * failure is not evidence that the plan needs revision. This extends FN-7561's + * advisory-failure guard to hard failures so execution state never regresses to + * planning unless a reviewer actually returned REVISE. + */ +const MODEL_FALLBACK_EXHAUSTED_PATTERN = /unable to select a usable model after\s+\d+\s+attempt/i; + +/** + * Identifies failed Plan Review calls that must stay in place rather than trigger + * the plan-revision handoff. The raw node failure value preserves abort/exception + * cases when a provider produced no diagnostic message. + */ +export function isNonPlanDefectPlanReviewFailure(input: { + verdict?: string; + errorMessage?: string; + failureValue?: string; +}): boolean { + if (input.verdict === "REVISE") return false; + + const failureValue = input.failureValue?.trim().toLowerCase(); + if (failureValue === "exception" || failureValue === "aborted") return true; + + const errorMessage = input.errorMessage?.trim(); + return Boolean( + errorMessage + && ( + isTransientError(errorMessage) + || isUsageLimitError(errorMessage) + || isOperatorActionableAgentError(errorMessage) + || MODEL_FALLBACK_EXHAUSTED_PATTERN.test(errorMessage) + ) + ); +} + /* FNXC:Reliability-ErrorClassification 2026-07-12-20:10: A long-running agent session holds its OAuth access token in memory. Claude Max access tokens rotate mid-run (~8 h lifetime); the in-flight call fails with a 401 {"type":"authentication_error","message":"Invalid authentication credentials"} even though the credentials file has already been refreshed, and the very next call succeeds. These must classify as TRANSIENT (retryable) and NOT operator-actionable, so in-run retry (withRateLimitRetry) and durable-agent heartbeat error recovery (FN-7835/FN-7844/FN-7859) auto-recover instead of parking agents paused with pauseReason "error-unrecoverable". Previously the message matched the operator-actionable /credential/ and /unauthorized/ patterns and defaulted to "permanent", so a routine token rotation parked every durable agent for manual operator repair. diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index d78e6cea80..ced2a4c7ad 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -108,6 +108,7 @@ import type { AgentSession, } from "@earendil-works/pi-coding-agent"; import { ModelFallbackExhaustedError, describeModel, formatModelMarkerDetails, promptWithFallback } from "./pi.js"; +import { isTaskStillInPlanningStage } from "./replan-target.js"; import { createResolvedAgentSession, extractRuntimeHint, @@ -1009,7 +1010,9 @@ export class TriageProcessor { const agentWork = async () => { // Set status only after the semaphore slot has been acquired, so // tasks waiting in the queue don't appear as "planning". - await this.store.updateTask(task.id, { status: "planning" }); + if (!await this.updatePlanningStateIfStillCurrent(task, { status: "planning" })) { + return; + } const stuckDetector = this.options.stuckTaskDetector; @@ -1410,7 +1413,7 @@ export class TriageProcessor { this.pauseAborted.delete(task.id); planLog.log(`${task.id} aborted by pause — clearing status`); const restoreStatus = this.restoreStatusAfterInterruptedTriageWork(task); - await this.store.updateTask(task.id, { status: restoreStatus }).catch((err: unknown) => { + await this.updatePlanningStateIfStillCurrent(task, { status: restoreStatus }).catch((err: unknown) => { const msg = err instanceof Error ? err.message : String(err); planLog.warn(`${task.id}: failed to restore status to '${restoreStatus}' during pause-abort cleanup: ${msg}`); }); @@ -1496,7 +1499,7 @@ export class TriageProcessor { planLog.warn(`${task.id} ${retryMessage}`); await this.store.logEntry(task.id, retryMessage); const restoreStatus = this.restoreStatusAfterInterruptedTriageWork(task); - await this.store.updateTask(task.id, { + await this.updatePlanningStateIfStillCurrent(task, { status: restoreStatus, error: null, recoveryRetryCount: decision.nextState.recoveryRetryCount, @@ -1515,13 +1518,14 @@ export class TriageProcessor { task.id, failureMessage, ); - await this.store.updateTask(task.id, { + if (await this.updatePlanningStateIfStillCurrent(task, { status: "failed", error: failureMessage, recoveryRetryCount: null, nextRecoveryAt: null, - }); - await this.backfillBlankTitleAfterTerminalTriageFailure(task); + })) { + await this.backfillBlankTitleAfterTerminalTriageFailure(task); + } return; } @@ -1576,7 +1580,7 @@ export class TriageProcessor { // For interrupted recovery states, restore the original triage-held status; // otherwise clear to null so the next poll can re-pick ordinary tasks up. const restoreStatus = this.restoreStatusAfterInterruptedTriageWork(task); - await this.store.updateTask(task.id, { status: restoreStatus }).catch((err: unknown) => { + await this.updatePlanningStateIfStillCurrent(task, { status: restoreStatus }).catch((err: unknown) => { const msg = err instanceof Error ? err.message : String(err); planLog.warn(`${task.id}: failed to restore status to '${restoreStatus}' during pause-abort error cleanup: ${msg}`); }); @@ -1603,7 +1607,7 @@ export class TriageProcessor { const msg = logErr instanceof Error ? logErr.message : String(logErr); planLog.warn(`${task.id}: failed to log planner fallback exhaustion: ${msg}`); }); - await this.store.updateTask(task.id, { + const persisted = await this.updatePlanningStateIfStillCurrent(task, { status: "failed", error: failureMessage, recoveryRetryCount: null, @@ -1611,7 +1615,9 @@ export class TriageProcessor { }).catch((updateErr: unknown) => { const msg = updateErr instanceof Error ? updateErr.message : String(updateErr); planLog.warn(`${task.id}: failed to persist planner fallback exhaustion: ${msg}`); + return false; }); + if (!persisted) return; await this.backfillBlankTitleAfterTerminalTriageFailure(task); this.options.onSpecifyError?.(task, err); return; @@ -1629,7 +1635,7 @@ export class TriageProcessor { const msg = logErr instanceof Error ? logErr.message : String(logErr); planLog.warn(`${task.id}: failed to persist operator-actionable specification failure: ${msg}`); }); - await this.store.updateTask(task.id, { + const persisted = await this.updatePlanningStateIfStillCurrent(task, { status: "failed", error: failureMessage, recoveryRetryCount: null, @@ -1637,7 +1643,9 @@ export class TriageProcessor { }).catch((updateErr: unknown) => { const msg = updateErr instanceof Error ? updateErr.message : String(updateErr); planLog.warn(`${task.id}: failed to park operator-actionable specification failure: ${msg}`); + return false; }); + if (!persisted) return; await this.backfillBlankTitleAfterTerminalTriageFailure(task); this.options.onSpecifyError?.(task, err instanceof Error ? err : new Error(errorMessage)); return; @@ -1660,7 +1668,7 @@ export class TriageProcessor { }); } const restoreStatus = this.restoreStatusAfterInterruptedTriageWork(task); - await this.store.updateTask(task.id, { + await this.updatePlanningStateIfStillCurrent(task, { status: restoreStatus, recoveryRetryCount: decision.nextState.recoveryRetryCount, nextRecoveryAt: decision.nextState.nextRecoveryAt, @@ -1677,14 +1685,16 @@ export class TriageProcessor { const msg = err instanceof Error ? err.message : String(err); planLog.warn(`${task.id}: failed to log transient-error retries-exhausted entry: ${msg}`); }); - await this.store.updateTask(task.id, { + const persisted = await this.updatePlanningStateIfStillCurrent(task, { error: `Specification failed after ${MAX_RECOVERY_RETRIES} transient errors: ${errorMessage}`, recoveryRetryCount: null, nextRecoveryAt: null, }).catch((err: unknown) => { const msg = err instanceof Error ? err.message : String(err); planLog.warn(`${task.id}: failed to persist transient-error retries-exhausted state: ${msg}`); + return false; }); + if (!persisted) return; await this.backfillBlankTitleAfterTerminalTriageFailure(task); this.options.onSpecifyError?.(task, err instanceof Error ? err : new Error(errorMessage)); return; @@ -1692,7 +1702,7 @@ export class TriageProcessor { // For interrupted recovery states, restore the original triage-held status; // otherwise clear to null so the next poll can re-pick ordinary tasks up. const restoreStatus = this.restoreStatusAfterInterruptedTriageWork(task); - await this.store.updateTask(task.id, { status: restoreStatus }).catch((restoreErr: unknown) => { + await this.updatePlanningStateIfStillCurrent(task, { status: restoreStatus }).catch((restoreErr: unknown) => { const msg = restoreErr instanceof Error ? restoreErr.message : String(restoreErr); planLog.warn(`${task.id}: failed to restore status to '${restoreStatus}' after planning error: ${msg}`); }); @@ -2014,6 +2024,45 @@ export class TriageProcessor { return [taskList, taskSearch, taskShow, taskCreate]; } + /** + * Atomically preserve a task that advanced while this triage session awaited a + * provider response. `updateTaskAtomic` holds the task lock across the live-row + * predicate and patch, closing the scheduler-transition race. + */ + private async updatePlanningStateIfStillCurrent( + task: Task, + patch: Parameters[1], + ): Promise { + if (typeof this.store.updateTaskAtomic !== "function") { + // Compatibility adapters used by older embedded hosts do not expose the + // core task lock; current TaskStore implementations always take the atomic path. + const liveTask = await Promise.resolve(this.store.getTask(task.id)).catch(() => task) ?? task; + if (!isTaskStillInPlanningStage(liveTask)) { + planLog.warn(`${task.id}: ignored stale triage recovery after task advanced to ${liveTask.column}`); + return false; + } + await this.store.updateTask(task.id, patch); + return true; + } + + let persisted = false; + await this.store.updateTaskAtomic(task.id, (liveTask) => { + if (!isTaskStillInPlanningStage(liveTask)) { + /* + * FNXC:Triage 2026-07-15-16:35: + * FN-7977: a provider or validation failure must never overwrite an + * advanced task with planning/failed/retry state. Evaluate this predicate + * under the task lock so scheduler advancement cannot race the recovery write. + */ + planLog.warn(`${task.id}: ignored stale triage recovery after task advanced to ${liveTask.column}`); + return null; + } + persisted = true; + return patch; + }); + return persisted; + } + private restoreStatusAfterInterruptedTriageWork(task: Task): Task["status"] | null { /* FNXC:PlanReview 2026-06-29-16:56: diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index f49ad2ea48..8ed4723617 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -10,6 +10,7 @@ import type { WorkflowStepResult, } from "@fusion/core"; import { BUILTIN_CODING_WORKFLOW_IR, PLAN_REVIEW_GROUP_ID, WorkflowIrError, getWorkflowExtensionRegistry, resolveMaxReworkCycles, isExperimentalFeatureEnabled, GRAPH_NATIVE_POST_MERGE_FLAG, isCompletionSummaryNode } from "@fusion/core"; +import { isNonPlanDefectPlanReviewFailure } from "./transient-error-detector.js"; import { createDefaultNodeHandlers, @@ -49,6 +50,9 @@ type WorkflowNodeSettings = Pick & { reviewerInlineFixes?: boolean; }; +/** A classified Plan Review provider outage terminates the graph without replan traversal. */ +export const PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE = "plan-review-provider-failure-hold"; + export type WorkflowNodeAbortKind = "engine-pause"; export const WORKFLOW_INTERRUPTED_NODE_ID_CONTEXT_KEY = "workflow:interruptedNodeId"; @@ -221,6 +225,8 @@ export interface WorkflowGraphExecutorDeps { phase: WorkflowStepResult["phase"]; status: WorkflowStepResult["status"]; verdict?: string; + /** Raw node result retained for non-verdict provider-failure classification. */ + failureValue?: string; nodeId?: string; maxRevisions?: unknown; }) => Promise | boolean; @@ -792,11 +798,23 @@ export class WorkflowGraphExecutor { /* * FNXC:PlanReviewReplan 2026-06-29-00:41: * Plan Review sits between specification and execution. A REVISE verdict - * or hard failure at this node means PROMPT.md needs another planning pass, - * not executor remediation. Forward the failure into the same pre-merge fix - * seam with a synthesized REVISE verdict so the executor can route it back - * to triage and then let approved replans continue through todo/execution. + * means PROMPT.md needs another planning pass, not executor remediation. */ + /* + * FNXC:PlanReviewReplan 2026-07-15-12:00: + * FN-7977 / issue #2124: do not fabricate REVISE from a hard Plan Review + * provider failure. Transport, rate-limit, model-selection, abort, and + * operator-actionable errors must remain visible in place so completed + * execution work cannot bounce back to the planner column. + */ + const nonPlanDefectPlanReviewFailure = + node.id === PLAN_REVIEW_GROUP_ID + && stepStatus === "failed" + && isNonPlanDefectPlanReviewFailure({ + verdict, + errorMessage: stepOutput ?? stepNotes, + failureValue: verdictRaw, + }); /* * FNXC:PlanReview 2026-06-29-02:05: * Plan Review should send a task back to triage only for an actual @@ -807,7 +825,23 @@ export class WorkflowGraphExecutor { const shouldRequestPreMergeFix = stepPhase === "pre-merge" && (stepStatus === "advisory_failure" || stepStatus === "failed") - && (verdict === "REVISE" || (node.id === PLAN_REVIEW_GROUP_ID && stepStatus === "failed")); + && (verdict === "REVISE" || ( + node.id === PLAN_REVIEW_GROUP_ID + && stepStatus === "failed" + && !nonPlanDefectPlanReviewFailure + )); + if (nonPlanDefectPlanReviewFailure) { + /* + * FNXC:PlanReviewReplan 2026-07-15-16:35: + * FN-7977: a classified provider/model/transport failure is a retryable + * hold, not a Plan Review REVISE. Do not traverse the built-in failure + * edge to plan-replan without remediation context; the executor retries + * this explicit hold in place and preserves advanced execution state. + */ + context[`node:${node.id}:outcome`] = "failure"; + context[`node:${node.id}:value`] = PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE; + return { outcome: "failure", value: PLAN_REVIEW_PROVIDER_FAILURE_HOLD_VALUE }; + } if (shouldRequestPreMergeFix) { const feedback = stepOutput?.trim() || stepNotes?.trim() @@ -820,6 +854,7 @@ export class WorkflowGraphExecutor { phase: stepPhase, status: stepStatus, verdict: verdict ?? (node.id === PLAN_REVIEW_GROUP_ID ? "REVISE" : undefined), + ...(!verdict && verdictRaw !== undefined ? { failureValue: verdictRaw } : {}), nodeId: node.id, maxRevisions: node.config?.maxRevisions, };