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) <noreply@runfusion.ai>
This commit is contained in:
gsxdsm
7
.changeset/fn-8503-unbounded-code-review-retries.md
Normal file
7
.changeset/fn-8503-unbounded-code-review-retries.md
Normal file
@@ -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.
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<void> {
|
||||
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<void> {
|
||||
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}`);
|
||||
|
||||
Reference in New Issue
Block a user