From 987abc763b2a8b9b60d6c27100f782887f5063ba Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 27 Jun 2026 23:01:10 -0700 Subject: [PATCH] FN-7162: reopen actionable steps after review bounces Post-verdict remediation now resumes executable work before rechecking optional gates. - Reopen the terminal verification/delivery suffix together with the nearest actionable implementation step for revision bounces. - Cover Code Review, Browser Verification, gate-promoted revisions, and non-revision optional outcomes with regression tests. - Document post-verdict bounce semantics and add a patch changeset for the published CLI package. Files changed: .changeset/fn-7162-post-verdict-bounce-step-completion.md | 7 ++ docs/workflow-steps.md | 9 +- packages/engine/src/__tests__/executor-step-session.test.ts | 117 +++++++++++++++++++-- packages/engine/src/__tests__/workflow-graph-optional-step-fix.test.ts | 60 +++++++++++ packages/engine/src/executor.ts | 51 +++++++-- 5 files changed, 222 insertions(+), 22 deletions(-) Fusion-Task-Id: FN-7162 Fusion-Task-Lineage: 8d422fc9-54e8-431d-97bb-e7a4a6763eaa Co-authored-by: Fusion (runfusion.ai) --- ...162-post-verdict-bounce-step-completion.md | 7 ++ docs/workflow-steps.md | 9 +- .../__tests__/executor-step-session.test.ts | 117 ++++++++++++++++-- .../workflow-graph-optional-step-fix.test.ts | 60 +++++++++ packages/engine/src/executor.ts | 51 ++++++-- 5 files changed, 222 insertions(+), 22 deletions(-) create mode 100644 .changeset/fn-7162-post-verdict-bounce-step-completion.md diff --git a/.changeset/fn-7162-post-verdict-bounce-step-completion.md b/.changeset/fn-7162-post-verdict-bounce-step-completion.md new file mode 100644 index 0000000000..6b005073c0 --- /dev/null +++ b/.changeset/fn-7162-post-verdict-bounce-step-completion.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Tasks sent back by Code Review or Browser Verification verdicts now re-run and complete their steps before re-checking. +category: fix +dev: Fixes the post-verdict remediation bounce (requestPreMergeOptionalStepFix → sendTaskBackForFix → reopenLastStepForRevision → scheduleWorkflowRerun → graph re-run) so resumed execution re-launches the executor and drives reopened implementation/verification/delivery steps and the verdict-demanded fix to done across both in-progress and in-review bounce sources, bounded by the existing maxRevisions/maxPostReviewFixes budget. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index 785bc16c1c..9e3f1616db 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -558,8 +558,8 @@ For pre-merge gate hard failures, the graph executor drives remediation through 1. Retry the failing check up to `MAX_WORKFLOW_STEP_RETRIES` within the same execution lifecycle 2. On retry exhaustion, add a steering comment with failure details and inject a `Workflow Step Failure` section into `PROMPT.md` -3. Reopen only the last implementation step (`pending`) so prior completed work remains preserved -4. Schedule `todo → in-progress` after guard unwind, triggering a fresh executor remediation run +3. Reopen the terminal verification/delivery suffix plus the nearest preceding implementation step (`pending`) so the resumed pass can address feedback without discarding unrelated completed work +4. Schedule `todo → in-progress` after guard unwind, triggering a fresh executor remediation run that must complete every reopened step before the workflow step re-evaluates Tasks are not parked in `in-review` for this remediable path unless additional terminal failures occur. @@ -590,9 +590,12 @@ Enabled PRE-merge optional-group REVISE findings should be acted on before revie FNXC:WorkflowOptionalStepRevisionBudget 2026-06-27-12:55: Workflow authors can override the global optional-step remediation budget per optional-group via `maxRevisions`, including `"unbounded"` for loops that should continue until the step approves. Document both inline executor and self-healing semantics because they must resolve the same budget for parked review recovery. + +FNXC:WorkflowOptionalStepFix 2026-06-27-22:51: +A post-verdict bounce from Code Review or Browser Verification must not reopen only a trivial trailing documentation step. The next executor pass reopens the actionable implementation step plus terminal verification/delivery steps, completes them to `done`, and only then lets the optional group re-evaluate across both in-progress and in-review bounce sources. --> -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, so the optional step re-runs against the fixed diff; the cycle repeats until the step returns `APPROVE` / `APPROVE_WITH_NOTES` or the resolved revision budget is exhausted. By default, each step uses the workflow/project `maxPostReviewFixes` value (built-in default: 3 fix passes). A workflow author can override that for a specific `optional-group` with `config.maxRevisions`: a non-negative integer sets that step's ceiling, `0` disables automatic fixes for that step, and `"unbounded"` removes the ceiling check. The counter remains the task's shared `postReviewFixCount`; per-step counters are not maintained. +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. By default, each step uses the workflow/project `maxPostReviewFixes` value (built-in default: 3 fix passes). A workflow author can override that for a specific `optional-group` with `config.maxRevisions`: a non-negative integer sets that step's ceiling, `0` disables automatic fixes for that step, and `"unbounded"` removes the ceiling check. The counter remains the task's shared `postReviewFixCount`; per-step counters are not maintained. 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. diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index bd103cd448..4b9241bfdf 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/packages/engine/src/__tests__/executor-step-session.test.ts @@ -481,15 +481,15 @@ describe("Workflow Steps Execution", () => { expect(store.moveTask).toHaveBeenCalledWith("FN-001", "in-review"); }); - it("routes exhausted prompt-mode workflow hard failures back to remediation and only reopens the last step", async () => { + it("routes exhausted prompt-mode workflow hard failures back to remediation and reopens actionable steps", async () => { // This test was previously written as an end-to-end run through // executor.execute(...) with vi.useFakeTimers(), but that path hung // deterministically under the 15 s budget: createResolvedAgentSession's // workflow-step Promise.race used a frozen 360 s setTimeout, and the // rejection from the mock prompt never reached the catch block in time. // The behavior we actually need to lock down is: - // 1. sendTaskBackForFix re-opens only the last completed step - // (reopenLastStepForRevision) — earlier done steps stay done. + // 1. sendTaskBackForFix re-opens the actionable implementation step plus + // any trailing verification/delivery step, not just a trivial last step. // 2. The rerun bounce uses preserveResumeState so step progress and // the worktree survive the in-progress → todo hop. // 3. PROMPT.md gains the Workflow Step Failure section with the @@ -519,8 +519,8 @@ describe("Workflow Steps Execution", () => { column: "in-progress" as const, dependencies: [] as string[], steps: [ - { name: "Step 0", status: "done" as const }, - { name: "Step 1", status: "done" as const }, + { name: "Implementation", status: "done" as const }, + { name: "Documentation & Delivery", status: "done" as const }, ], currentStep: 1, log: [] as any[], @@ -590,7 +590,10 @@ describe("Workflow Steps Execution", () => { "Workflow step failed", ); - // (1) failure comment + only the last step re-opened + // (1) failure comment + the implementation-bearing step is re-opened with the trailing delivery step. + // Before FN-7162, reopenLastStepForRevision returned only [1] here, so a + // Code Review / Browser Verification REVISE could re-run Documentation & + // Delivery against unchanged implementation work and loop until budget exhaustion. expect(store.addTaskComment).toHaveBeenCalledWith( "FN-001", expect.stringContaining("Workflow step failed"), @@ -599,8 +602,7 @@ describe("Workflow Steps Execution", () => { const reopenedStepIndexes = store.updateStep.mock.calls .filter((call: any[]) => call[0] === "FN-001" && call[2] === "pending") .map((call: any[]) => call[1]); - expect(reopenedStepIndexes).toContain(1); - expect(reopenedStepIndexes).not.toContain(0); + expect(reopenedStepIndexes).toEqual([0, 1]); // performWorkflowRerunBounce was invoked synchronously by the spy // above; flush microtasks so its awaited store calls settle before @@ -633,6 +635,105 @@ describe("Workflow Steps Execution", () => { injectSpy.mockRestore(); }); + it("keeps post-verdict reopening bounded across all-done, mixed, and single-step states", async () => { + const cases = [ + { + id: "all-done-terminal", + steps: [ + { name: "Implementation", status: "done" as const }, + { name: "Documentation & Delivery", status: "done" as const }, + ], + expectedIndexes: [0, 1], + expectedCurrent: 0, + }, + { + id: "mixed-terminal-pending", + steps: [ + { name: "Implementation", status: "done" as const }, + { name: "Documentation & Delivery", status: "pending" as const }, + ], + expectedIndexes: [0], + expectedCurrent: 0, + }, + { + id: "single-step", + steps: [{ name: "Implementation", status: "done" as const }], + expectedIndexes: [0], + expectedCurrent: 0, + }, + ]; + + for (const testCase of cases) { + const store = createMockStore(); + const mutableTask = { + id: `FN-7162-${testCase.id}`, + title: "Test", + description: "Test task", + column: "in-progress" as const, + dependencies: [] as string[], + steps: testCase.steps.map((step) => ({ ...step })), + currentStep: testCase.steps.length - 1, + log: [] as any[], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + store.updateStep.mockImplementation(async (_taskId: string, stepIndex: number, status: string) => { + mutableTask.steps[stepIndex]!.status = status as any; + return {}; + }); + + const executor = new TaskExecutor(store, "/tmp/test"); + const reopened = await (executor as unknown as { + reopenLastStepForRevision: ( + taskId: string, + task: typeof mutableTask, + ) => Promise<{ index: number; name: string; indexes: number[] } | null>; + }).reopenLastStepForRevision(mutableTask.id, mutableTask); + + expect(reopened?.indexes).toEqual(testCase.expectedIndexes); + expect(reopened?.index).toBe(testCase.expectedCurrent); + expect(store.updateStep.mock.calls.map((call: any[]) => call[1])).toEqual(testCase.expectedIndexes); + expect(store.updateTask).toHaveBeenCalledWith(mutableTask.id, { currentStep: testCase.expectedCurrent }); + expect(mutableTask.steps.some((step) => step.status === "pending")).toBe(true); + } + }); + + it("reopens terminal verification and delivery suffix with the implementation step", async () => { + const store = createMockStore(); + const mutableTask = { + id: "FN-7162-SUFFIX", + title: "Test", + description: "Test task", + column: "in-progress" as const, + dependencies: [] as string[], + steps: [ + { name: "Implementation", status: "done" as const }, + { name: "Testing & Verification", status: "done" as const }, + { name: "Documentation & Delivery", status: "done" as const }, + ], + currentStep: 2, + log: [] as any[], + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + store.updateStep.mockImplementation(async (_taskId: string, stepIndex: number, status: string) => { + mutableTask.steps[stepIndex]!.status = status as any; + return {}; + }); + + const executor = new TaskExecutor(store, "/tmp/test"); + const reopened = await (executor as unknown as { + reopenLastStepForRevision: ( + taskId: string, + task: typeof mutableTask, + ) => Promise<{ index: number; name: string; indexes: number[] } | null>; + }).reopenLastStepForRevision(mutableTask.id, mutableTask); + + expect(reopened).toEqual({ index: 0, name: "Implementation", indexes: [0, 1, 2] }); + expect(store.updateStep.mock.calls.map((call: any[]) => call[1])).toEqual([0, 1, 2]); + expect(store.updateTask).toHaveBeenCalledWith("FN-7162-SUFFIX", { currentStep: 0 }); + }); + // FNXC:WorkflowOptionalStepFix 2026-06-27-13:30: // Regression for the FN-7122 deadlock: a pre-merge optional-step REVISE // reopens the last plan step to `pending` and schedules a rerun bounce, but 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 72604a0a11..d9ee88d6a2 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 @@ -38,6 +38,66 @@ describe("TaskExecutor pre-merge optional-step fix seam", () => { resetExecutorMocks(); }); + it("sends Code Review, Browser Verification, and gate-promoted pre-merge revisions back for remediation", async () => { + const cases = [ + { stepName: "Code Review", status: "advisory_failure" as const, feedback: "review finding" }, + { stepName: "Browser Verification", status: "advisory_failure" as const, feedback: "browser finding" }, + { stepName: "Code Review", status: "failed" as const, feedback: "gate-promoted finding" }, + ]; + + for (const testCase of cases) { + const store = createMockStore(); + const liveTask = task({ postReviewFixCount: 0, worktree: "/tmp/fusion/fn-7066" }); + store.getTask.mockResolvedValue(liveTask); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + const scheduled = await (executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, { + ...reviseInfo, + stepName: testCase.stepName, + status: testCase.status, + feedback: testCase.feedback, + }); + + expect(scheduled).toBe(true); + expect(sendBack).toHaveBeenCalledWith( + liveTask, + "/tmp/fusion/fn-7066", + testCase.feedback, + testCase.stepName, + expect.stringContaining("requested revision"), + ); + } + }); + + it("does not bounce post-merge, fast-mode skipped, approved, or non-revision optional outcomes", async () => { + const cases = [ + { phase: "post-merge" as const, status: "advisory_failure" as const, verdict: "REVISE" }, + { phase: "pre-merge" as const, status: "passed" as const, verdict: "APPROVE" }, + { phase: "pre-merge" as const, status: "passed" as const, verdict: "workflow-step-skipped" }, + { phase: "pre-merge" as const, status: "advisory_failure" as const, verdict: "APPROVE_WITH_NOTES" }, + ]; + + for (const testCase of cases) { + const store = createMockStore(); + const liveTask = task({ postReviewFixCount: 0 }); + store.getTask.mockResolvedValue(liveTask); + store.getSettings.mockResolvedValue({ maxPostReviewFixes: 3 }); + const executor = new TaskExecutor(store, "/tmp/test"); + const sendBack = vi.spyOn(executor as any, "sendTaskBackForFix").mockResolvedValue(undefined); + + const scheduled = await (executor as any).requestPreMergeOptionalStepFix(liveTask.id, liveTask, { + ...reviseInfo, + ...testCase, + }); + + expect(scheduled).toBe(false); + expect(sendBack).not.toHaveBeenCalled(); + expect(store.updateTask).not.toHaveBeenCalledWith(liveTask.id, expect.objectContaining({ postReviewFixCount: expect.any(Number) }), undefined); + } + }); + it("consumes budget before sending the task back for optional-step remediation", async () => { const store = createMockStore(); const liveTask = task({ postReviewFixCount: 0 }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index b42703ae7d..f872de1fc1 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -11913,34 +11913,63 @@ export class TaskExecutor { } /** - * Re-open the last non-pending step so a revision/failure handler gives the - * executor exactly one pending slot to re-enter through. Returns the index - * and name of the step that was flipped to `pending`, or null when there - * was nothing to re-open. + * Re-open the implementation-bearing slice of work for a revision/failure + * handler. Returns the earliest reopened step and all reopened indexes, or + * null when there was nothing to re-open. */ private async reopenLastStepForRevision( taskId: string, task: Task, - ): Promise<{ index: number; name: string } | null> { + ): Promise<{ index: number; name: string; indexes: number[] } | null> { const steps = task.steps; if (steps.length === 0) return null; - let targetIndex = -1; + let lastNonPendingIndex = -1; for (let i = steps.length - 1; i >= 0; i--) { if (steps[i].status !== "pending") { - targetIndex = i; + lastNonPendingIndex = i; break; } } - if (targetIndex === -1) { + if (lastNonPendingIndex === -1) { await this.store.updateTask(taskId, { currentStep: 0 }); return null; } - await this.store.updateStep(taskId, targetIndex, "pending"); - await this.store.updateTask(taskId, { currentStep: targetIndex }); - return { index: targetIndex, name: steps[targetIndex].name }; + // Match step-title words rather than arbitrary substrings so an implementation step + // like "DataVerificationLayer" is not treated as a trailing delivery/check step. + const isTerminalVerificationOrDeliveryStep = (name: string): boolean => + /(^|[^a-z])(testing|verification|documentation|delivery)([^a-z]|$)/i.test(name); + + const resetIndexes = new Set([lastNonPendingIndex]); + if (isTerminalVerificationOrDeliveryStep(steps[lastNonPendingIndex].name)) { + let cursor = lastNonPendingIndex; + while (cursor >= 0 && isTerminalVerificationOrDeliveryStep(steps[cursor].name)) { + resetIndexes.add(cursor); + cursor--; + } + while (cursor >= 0 && steps[cursor].status === "pending") { + cursor--; + } + if (cursor >= 0) { + resetIndexes.add(cursor); + } + } + + const indexes = [...resetIndexes].sort((a, b) => a - b); + /* + FNXC:WorkflowOptionalStepFix 2026-06-27-18:03: + Code Review / Browser Verification REVISE bounces must reopen the step that can actually make the requested code change, not only a trailing Documentation & Delivery or Testing & Verification step. Otherwise the graph rerun can complete a trivial terminal step, re-evaluate the optional group against unchanged code, and loop or strand pending work. Reopen the trailing verification/delivery suffix plus the nearest preceding implementation step so both in-progress and in-review bounce sources re-launch execution on actionable work before optional-step re-evaluation. + */ + for (const index of indexes) { + if (steps[index].status !== "pending") { + await this.store.updateStep(taskId, index, "pending"); + } + } + const currentStep = indexes[0] ?? lastNonPendingIndex; + await this.store.updateTask(taskId, { currentStep }); + return { index: currentStep, name: steps[currentStep].name, indexes }; } /**