From 227281dc32c091210cb5df0b4f301a0f9d92a8ef Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 22 Jul 2026 18:32:36 -0700 Subject: [PATCH] FN-8503: preserve unbounded Code Review retries Keep Code Review remediation retry policies accurate across graph execution and recovery. - Preserve unlimited retry presentation when Code Review has no configured cap - Enforce finite Code Review caps during failed-step recovery - Validate non-negative revision settings and document the active retry policy Files changed: .../fn-8503-unbounded-code-review-retries.md | 7 ++ docs/workflow-steps.md | 2 +- .../core/src/__tests__/builtin-workflows.test.ts | 8 +- packages/core/src/builtin-workflow-settings.ts | 4 + .../workflow-graph-optional-step-fix.test.ts | 135 +++++++++++++++++++++ packages/engine/src/executor.ts | 51 ++++++-- 6 files changed, 193 insertions(+), 14 deletions(-) Fusion-Task-Id: FN-8503 Fusion-Task-Lineage: 7bd555d1-23e5-42ea-b6f5-0b9fe4da7f94 Co-authored-by: Fusion (runfusion.ai) --- .../fn-8503-unbounded-code-review-retries.md | 7 + docs/workflow-steps.md | 2 +- .../src/__tests__/builtin-workflows.test.ts | 8 +- .../core/src/builtin-workflow-settings.ts | 4 + .../workflow-graph-optional-step-fix.test.ts | 135 ++++++++++++++++++ packages/engine/src/executor.ts | 51 +++++-- 6 files changed, 193 insertions(+), 14 deletions(-) create mode 100644 .changeset/fn-8503-unbounded-code-review-retries.md diff --git a/.changeset/fn-8503-unbounded-code-review-retries.md b/.changeset/fn-8503-unbounded-code-review-retries.md new file mode 100644 index 0000000000..a6699ebdc5 --- /dev/null +++ b/.changeset/fn-8503-unbounded-code-review-retries.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Keep default Code Review remediation retries unlimited and show the active policy. +category: feature +dev: Code Review retry prompts now preserve resolved unlimited or finite workflow revision budgets. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index d414d77bbf..1bdd833339 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -751,7 +751,7 @@ Retryable graph failures at explicit remediation nodes (for example `code-review During a live graph run, an enabled **pre-merge** optional step that returns `REVISE` (including the built-in **Code Review** / `code-review` and **Browser Verification** / `browser-verification` groups) sends the task back to the executor for a fix pass before the graph continues to review or merge. The workflow graph restarts on the next executor pass, re-launches task execution, and reopens the terminal verification/delivery suffix plus the nearest preceding implementation step so the verdict-demanded fix can be made rather than merely replaying a trivial trailing step. The optional step re-runs only after the executor drives those reopened steps back to `done`; the cycle repeats until the step returns `APPROVE` / `APPROVE_WITH_NOTES` or the resolved revision budget is exhausted. Generic optional gates use the workflow/project `maxPostReviewFixes` value (built-in default: 3 fix passes). Built-in Plan Review and Code Review default to `"unbounded"` so they continue until approval unless `planReviewMaxRevisions`, `codeReviewMaxRevisions`, or the node's `config.maxRevisions` sets a numeric cap. The aggregate `postReviewFixCount` remains for dashboard visibility, but budget checks count attempts per workflow-step key so Plan Review, Code Review, and Browser Verification do not consume each other's caps. -The same resolved per-step budget is used by self-healing when it revives an `in-review` task that is parked with a failed pre-merge workflow result. If the failed step's IR cannot be resolved, self-healing falls back to `maxPostReviewFixes` so existing behavior is preserved. `"unbounded"` relies on the optional step eventually approving; a step that always returns `REVISE` will continue cycling until a human intervenes or another guard (pause, worktree/lease, auto-merge policy, dependency blocker) stops recovery. When the budget is exhausted or disabled, behavior falls through to the prior semantics: advisory results remain non-blocking and gate failures remain failed/parked. +The same resolved per-step budget is used by self-healing when it revives an `in-review` task that is parked with a failed pre-merge workflow result. If the failed step's IR cannot be resolved, self-healing falls back to `maxPostReviewFixes` so existing behavior is preserved. `"unbounded"` relies on the optional step eventually approving; a step that always returns `REVISE` will continue cycling until a human intervenes or another guard (pause, worktree/lease, auto-merge policy, dependency blocker) stops recovery. The remediation instructions show `attempt/unbounded` and unlimited remaining retries for this policy rather than a misleading legacy `3/3` label. When the budget is exhausted or disabled, behavior falls through to the prior semantics: advisory results remain non-blocking and gate failures remain failed/parked. Post-merge optional groups never trigger this send-back path because merge has already happened; their failures are recorded/logged as non-blocking post-merge results. diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index a17eb6856c..6e07a997ce 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -73,9 +73,11 @@ describe("built-in workflows", () => { expect(ir.settings?.find((setting) => setting.id === "planReviewMaxRevisions"), workflow.id).not.toHaveProperty( "default", ); - expect(ir.settings?.find((setting) => setting.id === "codeReviewMaxRevisions"), workflow.id).not.toHaveProperty( - "default", - ); + const codeReviewCap = ir.settings?.find((setting) => setting.id === "codeReviewMaxRevisions"); + expect(codeReviewCap, workflow.id).not.toHaveProperty("default"); + // Empty is the canonical unlimited policy; stored numeric values must be + // non-negative whole numbers so `0` has the documented disable semantics. + expect(codeReviewCap, workflow.id).toMatchObject({ type: "number", minimum: 0, integer: true }); } }); diff --git a/packages/core/src/builtin-workflow-settings.ts b/packages/core/src/builtin-workflow-settings.ts index d4c031cc28..75303d884d 100644 --- a/packages/core/src/builtin-workflow-settings.ts +++ b/packages/core/src/builtin-workflow-settings.ts @@ -475,6 +475,8 @@ export const BUILTIN_REVIEW_REVISION_SETTINGS: WorkflowSettingDefinition[] = [ id: "planReviewMaxRevisions", name: "Plan Review revision cap", type: "number", + minimum: 0, + integer: true, /* * FNXC:WorkflowRevisionBudget 2026-06-30-19:45: * Built-in Plan Review/spec remediation is unbounded when this workflow value is unset. Operators can store a non-negative integer per workflow to cap automatic replans, and `0` disables automatic Plan Review revision entirely without duplicating a read-only built-in workflow. @@ -486,6 +488,8 @@ export const BUILTIN_REVIEW_REVISION_SETTINGS: WorkflowSettingDefinition[] = [ id: "codeReviewMaxRevisions", name: "Code Review revision cap", type: "number", + minimum: 0, + integer: true, /* * FNXC:WorkflowRevisionBudget 2026-06-30-19:45: * Built-in Code Review remediation is unbounded when this workflow value is unset. Operators can store a non-negative integer per workflow to cap automatic code-fix passes, and `0` disables automatic Code Review remediation for that workflow. 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 47037bc933..67403cbb34 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 @@ -1,5 +1,8 @@ import "./executor-test-helpers.js"; import { beforeEach, describe, expect, it, vi } from "vitest"; +import { mkdtemp, mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import type { Task } from "@fusion/core"; import { TaskExecutor } from "../executor.js"; @@ -219,6 +222,9 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { testCase.feedback, testCase.stepName, expect.stringContaining("requested revision"), + true, + false, + { attempt: 1, max: 3 }, ); } }); @@ -274,6 +280,9 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { "packages/engine/src/example.ts:1 needs a guard", "Code Review", expect.stringContaining("requested revision"), + true, + false, + { attempt: 1, max: 2 }, ); expect(store.updateTask.mock.invocationCallOrder[0]).toBeLessThan(sendBack.mock.invocationCallOrder[0]); }); @@ -640,6 +649,132 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { expect(sendBackCalls).toEqual([0, 1, 2]); }); + it("keeps graph-owned Code Review remediation unbounded past the legacy three-pass display cap", async () => { + for (const count of [0, 1, 2, 3, 4, 5, 6]) { + const store = createMockStore(); + const liveTask = task({ + postReviewFixCount: count, + log: Array.from({ length: count }, (_, index) => revisionLog("Code Review", "code-review", index + 1)), + }); + store.getTask.mockResolvedValue(liveTask); + // The generic optional-gate fallback stays three; the graph-owned Code Review + // node must not inherit it when the workflow-specific value is unset. + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + await expect((executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, { + ...reviseInfo, + nodeId: "code-review", + })).resolves.toBe(true); + + expect(store.logEntry).toHaveBeenCalledWith( + liveTask.id, + expect.stringContaining(`attempt ${count + 1}/unbounded`), + expect.stringContaining("Workflow revision key: code-review"), + undefined, + ); + expect(sendBack).toHaveBeenCalledWith( + liveTask, + liveTask.worktree, + reviseInfo.feedback, + reviseInfo.stepName, + expect.any(String), + true, + false, + { attempt: count + 1, max: undefined }, + ); + } + }); + + it("keeps the retry presentation aligned with the next attempt during failed-step recovery", async () => { + const store = createMockStore(); + const liveTask = task({ + column: "in-review", + log: Array.from({ length: 3 }, (_, index) => revisionLog("Code Review", "code-review", index + 1)), + workflowStepResults: [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + phase: "pre-merge", + status: "failed", + output: "Fix the review finding.", + completedAt: new Date().toISOString(), + }], + }); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + await expect(executor.recoverFailedPreMergeWorkflowStep(liveTask)).resolves.toBe(true); + + expect(sendBack).toHaveBeenCalledWith( + liveTask, + liveTask.worktree, + "Fix the review finding.", + "Code Review", + expect.any(String), + true, + false, + { attempt: 4, max: undefined }, + ); + }); + + /* + * FNXC:WorkflowRevisionBudget 2026-07-22-18:30: + * Self-healing calls the failed-step recovery seam directly. It must not + * bypass an operator's finite Code Review cap merely because the candidate + * filter was skipped or raced; unlimited remains eligible by default. + */ + it.each([ + { label: "zero automatic remediations", codeReviewMaxRevisions: 0, attempts: 0 }, + { label: "an exhausted finite cap", codeReviewMaxRevisions: 2, attempts: 2 }, + ])("does not recover Code Review after $label", async ({ codeReviewMaxRevisions, attempts }) => { + const store = createMockStore(); + const liveTask = task({ + column: "in-review", + log: Array.from({ length: attempts }, (_, index) => revisionLog("Code Review", "code-review", index + 1)), + workflowStepResults: [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + phase: "pre-merge", + status: "failed", + output: "Fix the review finding.", + completedAt: new Date().toISOString(), + }], + }); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3, codeReviewMaxRevisions }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + await expect(executor.recoverFailedPreMergeWorkflowStep(liveTask)).resolves.toBe(false); + + expect(sendBack).not.toHaveBeenCalled(); + }); + + it("writes an unbounded retry label into Code Review remediation instructions", async () => { + const root = await mkdtemp(join(tmpdir(), "fn-8503-")); + const fusionDir = join(root, ".fusion"); + const promptPath = join(fusionDir, "tasks", "FN-7066", "PROMPT.md"); + try { + await mkdir(join(fusionDir, "tasks", "FN-7066"), { recursive: true }); + await writeFile(promptPath, "# Task\n\n## Steps\n- Fix it\n"); + const store = createMockStore(); + store.getFusionDir.mockReturnValue(fusionDir); + const executor = new TaskExecutor(store, "/tmp/test"); + + await (executor as any).injectWorkflowStepFailureInstructions( + task(), + "Address the Code Review finding.", + "Code Review", + { attempt: 6, max: undefined }, + ); + + await expect(readFile(promptPath, "utf8")).resolves.toContain("**Retry:** 6/unbounded (unlimited remaining)"); + } finally { + await rm(root, { recursive: true, force: true }); + } + }); + it("lets per-step maxRevisions override the global budget", async () => { for (const count of [1, 2]) { const store = createMockStore(); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 2825bd745a..8e0a106429 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -4989,6 +4989,9 @@ export class TaskExecutor { info.feedback, info.stepName, `Pre-merge optional workflow step "${info.stepName}" requested revision`, + true, + false, + { attempt: nextCount, max: budget.unbounded ? undefined : budget.max }, ); return true; } @@ -5087,9 +5090,9 @@ export class TaskExecutor { * * Picks the latest failed pre-merge workflow step result (there is usually only * one, but if several ran we want the most recent), injects its feedback into - * `PROMPT.md`, resets steps, and schedules todo → in-progress. The call site - * is responsible for enforcing the `maxPostReviewFixes` budget before invoking - * this method — this method itself does no accounting. + * `PROMPT.md`, resets steps, and schedules todo → in-progress. The caller may + * account for a scheduled retry, but this method independently enforces the + * effective finite-or-unlimited revision budget before it can reopen work. * * @returns true when the task was sent back, false when no eligible failed * step exists (caller should skip). @@ -5123,6 +5126,18 @@ export class TaskExecutor { const feedback = target.output?.trim() || "(no feedback captured)"; const stepName = target.workflowStepName || target.workflowStepId || "Unknown"; + const budget = await this.resolveFailedPreMergeWorkflowStepBudget(task, target); + /* + * FNXC:WorkflowRevisionBudget 2026-07-22-18:30: + * Failed-step recovery is also a remediation entry point, not merely a + * retry-label formatter. Enforce the same finite Code Review budget here + * as live and restart-local graph remediation: an unset policy remains + * unlimited, while zero or an exhausted explicit cap cannot silently send + * work back for another fix. Progress-loop termination stays owned by the + * graph executor's signature guard rather than this budget check. + */ + if (!budget.unbounded && (!Number.isFinite(budget.max) || budget.max <= 0)) return false; + if (!budget.unbounded && budget.attempts >= budget.max) return false; await this.sendTaskBackForFix( task, @@ -5130,6 +5145,9 @@ export class TaskExecutor { feedback, stepName, `Auto-revived from in-review: pre-merge workflow step "${stepName}" had failed`, + true, + false, + { attempt: budget.attempts + 1, max: budget.unbounded ? undefined : budget.max }, ); return true; } catch (err: unknown) { @@ -16084,6 +16102,7 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} reason: string, preserveResumeState: boolean = true, mergeVerificationFailure: boolean = false, + retryPresentation?: { attempt: number; max?: number }, ): Promise { const taskId = task.id; this.clearCompletedTaskWatchdog(taskId); @@ -16103,9 +16122,20 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} `${reason} — moved back to in-progress for remediation`, ); - // 3. Inject failure feedback into PROMPT.md using the existing method - // Pass MAX_WORKFLOW_STEP_RETRIES to indicate retries are exhausted (shows "3/3 (0 remaining)") - await this.injectWorkflowStepFailureInstructions(task, failureFeedback, stepName, MAX_WORKFLOW_STEP_RETRIES); + /* + * FNXC:CodeReviewRetryBudget 2026-07-22-00:00: + * A graph-owned Code Review REVISE is not a workflow-step hard-failure retry. + * Preserve its resolved per-step budget in PROMPT.md: unset Code Review policy + * is unlimited, while an explicit finite value (including zero at the gate) + * remains operator-visible. The execute requeue progress-signature guard, not + * this display, remains the safety boundary for unchanged remediation loops. + */ + await this.injectWorkflowStepFailureInstructions( + task, + failureFeedback, + stepName, + retryPresentation ?? { attempt: MAX_WORKFLOW_STEP_RETRIES, max: MAX_WORKFLOW_STEP_RETRIES }, + ); // 4. Re-open only the last step for a single in-place fix pass. Earlier // done steps stay done so the executor doesn't redo finished work. @@ -16145,7 +16175,7 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} task: Task, failureFeedback: string, stepName: string, - retryCount: number, + retry: { attempt: number; max?: number }, ): Promise { const promptPath = join(this.store.getFusionDir(), "tasks", task.id, "PROMPT.md"); @@ -16158,7 +16188,8 @@ ${failureContext.output.slice(0, VERIFICATION_LOG_MAX_CHARS)} return; } - const remainingRetries = MAX_WORKFLOW_STEP_RETRIES - retryCount; + const retryLabel = retry.max === undefined ? "unbounded" : String(retry.max); + const remainingRetries = retry.max === undefined ? "unlimited" : String(Math.max(0, retry.max - retry.attempt)); const failureSectionHeader = "## Workflow Step Failure"; const scopeGuard = this.buildWorkflowFailureScopeGuard(task, content); const failureSectionContent = `${failureSectionHeader} @@ -16172,7 +16203,7 @@ ${failureFeedback} ${scopeGuard} -**Retry:** ${retryCount}/${MAX_WORKFLOW_STEP_RETRIES} (${remainingRetries} remaining) +**Retry:** ${retry.attempt}/${retryLabel} (${remainingRetries} remaining) **Important:** This is a workflow step failure — fix the issues above by making the necessary code changes. The task has been sent back to in-progress for remediation. The executor will attempt to fix the issues on the next pass. @@ -16215,7 +16246,7 @@ ${scopeGuard} // Write updated content try { await writeFile(promptPath, newContent); - executorLog.log(`${task.id}: injected workflow step failure instructions into PROMPT.md (retry ${retryCount}/${MAX_WORKFLOW_STEP_RETRIES})`); + executorLog.log(`${task.id}: injected workflow step failure instructions into PROMPT.md (retry ${retry.attempt}/${retryLabel})`); } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); executorLog.error(`${task.id}: failed to inject workflow step failure instructions: ${errorMessage}`);