From dc7bb409481be82924d98e34c7ef61f24621e00b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 15:34:43 -0700 Subject: [PATCH] FN-7990: share worktree classifier so Code Review acquires a worktree Unify write-capability classification so graph preparation acquires a worktree for inline-fix Code Review before runtime runs, eliminating the immediate no-worktree-for-write-node failure. - Add shared workflowNodeRequiresWorktree helper for preparation and runtime - Plumb optional-group context and reviewerInlineFixes into graph preparation - Acquire/reuse/reacquire worktrees for write-capable inline review nodes - Keep Plan Review and disabled inline fixes read-only - Add regression tests and a patch changeset Files changed: .changeset/fn-7990-code-review-worktree.md | 7 ++ .../__tests__/ce-workflow-step-executor.test.ts | 97 ++++++++++++++++++++++ .../workflow-node-execution-needs.test.ts | 47 +++++++++++ packages/engine/src/executor.ts | 32 +++---- packages/engine/src/workflow-graph-executor.ts | 52 ++++++++---- .../engine/src/workflow-node-execution-needs.ts | 46 ++++++++++ 6 files changed, 243 insertions(+), 38 deletions(-) Fusion-Task-Id: FN-7990 Fusion-Task-Lineage: f5d19181-0b98-4827-8adb-069f7dc05c03 Co-authored-by: Fusion (runfusion.ai) --- .changeset/fn-7990-code-review-worktree.md | 7 ++ .../ce-workflow-step-executor.test.ts | 97 +++++++++++++++++++ .../workflow-node-execution-needs.test.ts | 47 +++++++++ packages/engine/src/executor.ts | 32 +++--- .../engine/src/workflow-graph-executor.ts | 52 ++++++---- .../src/workflow-node-execution-needs.ts | 46 +++++++++ 6 files changed, 243 insertions(+), 38 deletions(-) create mode 100644 .changeset/fn-7990-code-review-worktree.md create mode 100644 packages/engine/src/__tests__/workflow-node-execution-needs.test.ts create mode 100644 packages/engine/src/workflow-node-execution-needs.ts diff --git a/.changeset/fn-7990-code-review-worktree.md b/.changeset/fn-7990-code-review-worktree.md new file mode 100644 index 0000000000..62e953dc3d --- /dev/null +++ b/.changeset/fn-7990-code-review-worktree.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent inline Code Review steps from failing before they can run. +category: fix +dev: Shares workflow write-capability classification between graph preparation and runtime. diff --git a/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts b/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts index 07cef58f7d..2f76df9d29 100644 --- a/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts +++ b/packages/engine/src/__tests__/ce-workflow-step-executor.test.ts @@ -357,6 +357,103 @@ describe("CE workflow-step executor integration", () => { ); }); + it.each([ + ["acquires an absent worktree", undefined, "/tmp/test/.worktrees/acquired-code-review", 1], + ["reuses a live worktree", "/tmp/test/.worktrees/live-code-review", "/tmp/test/.worktrees/live-code-review", 0], + ["reacquires a stale worktree", "/tmp/test/.worktrees/stale-code-review", "/tmp/test/.worktrees/acquired-code-review", 1], + ])("prepares an inline-fix Code Review node when it %s", async (_scenario, existingWorktree, expectedWorktree, acquisitionCount) => { + mockedExistsSync.mockImplementation((path) => path !== "/tmp/test/.worktrees/stale-code-review"); + const store = createMockStore(); + let live = baseStepTask({ + worktree: existingWorktree, + branch: existingWorktree ? "fusion/fn-ce-1" : undefined, + enabledWorkflowSteps: ["code-review"], + }); + store.getTask.mockImplementation(async () => live as any); + store.updateTask.mockImplementation(async (_id: string, patch: Record) => { + live = { ...live, ...patch }; + return live as any; + }); + const { executor } = makeExecutor(store); + vi.spyOn(executor as any, "createWorktree").mockResolvedValue({ + path: "/tmp/test/.worktrees/acquired-code-review", + branch: "fusion/fn-ce-1", + }); + vi.spyOn(executor as any, "captureBaseCommitSha").mockResolvedValue(undefined); + const executeStep = vi.spyOn(executor as any, "executeWorkflowStep").mockResolvedValue({ success: true, output: "APPROVE" }); + const requirements: any[] = []; + const codeReview = { + id: "code-review-step", + kind: "gate", + config: { name: "Code Review", prompt: "Review the implementation." }, + }; + const ir: WorkflowIr = { + version: "v2", + name: "code-review-worktree-test", + columns: [{ id: "in-progress", name: "In Progress", traits: [] }], + nodes: [ + { id: "start", kind: "start" }, + { + id: "code-review", + kind: "optional-group", + config: { name: "Code Review", defaultOn: true, template: { nodes: [codeReview], edges: [] } }, + }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "code-review" }, + { from: "code-review", to: "end", condition: "success" }, + ], + }; + const settings = { ...(await store.getSettings()), reviewerInlineFixes: true }; + const graph = new WorkflowGraphExecutor({ + prepareNodeExecution: (graphNode, task, requirement) => { + requirements.push(requirement); + return (executor as any).prepareGraphNodeExecution(graphNode, task, settings, requirement); + }, + runCustomNode: (graphNode, task, context) => + (executor as any).runGraphCustomNode(graphNode, task, settings, undefined, context), + }); + + const result = await graph.run(live as any, settings, ir); + + expect(requirements).toContainEqual({ requiresWorktree: true, reason: "write-capable-node" }); + expect((executor as any).createWorktree).toHaveBeenCalledTimes(acquisitionCount); + expect(executeStep).toHaveBeenCalledTimes(1); + expect(executeStep.mock.calls[0]?.[2]).toBe(expectedWorktree); + expect(result).toMatchObject({ outcome: "success" }); + expect(result.context["node:code-review:outcome"]).not.toBe("no-worktree-for-write-node"); + }); + + it("keeps disabled inline fixes and Plan Review read-only during graph preparation", async () => { + const requirements: any[] = []; + const graph = new WorkflowGraphExecutor({ + prepareNodeExecution: (_node, _task, requirement) => { requirements.push(requirement); }, + handlers: { gate: async () => ({ outcome: "success" }) }, + }); + const optionalGroup = (id: string, name: string) => ({ + id, + kind: "optional-group" as const, + config: { name, defaultOn: true, template: { nodes: [{ id: `${id}-step`, kind: "gate" as const, config: { name } }], edges: [] } }, + }); + const ir: WorkflowIr = { + version: "v2", + name: "readonly-review-worktree-test", + columns: [], + nodes: [{ id: "start", kind: "start" }, optionalGroup("code-review", "Code Review"), optionalGroup("plan-review", "Plan Review"), { id: "end", kind: "end" }], + edges: [ + { from: "start", to: "code-review" }, + { from: "code-review", to: "plan-review", condition: "success" }, + { from: "plan-review", to: "end", condition: "success" }, + ], + }; + await graph.run(baseStepTask({ enabledWorkflowSteps: ["code-review", "plan-review"] }) as any, { + experimentalFeatures: {}, + reviewerInlineFixes: false, + }, ir); + expect(requirements).toEqual([]); + }); + it("finalizes a merge-confirmed workflow graph task that is stranded before done", async () => { const store = createMockStore(); let live = baseStepTask({ diff --git a/packages/engine/src/__tests__/workflow-node-execution-needs.test.ts b/packages/engine/src/__tests__/workflow-node-execution-needs.test.ts new file mode 100644 index 0000000000..a24efcb386 --- /dev/null +++ b/packages/engine/src/__tests__/workflow-node-execution-needs.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import type { WorkflowIrNode } from "@fusion/core"; +import { workflowNodeRequiresWorktree } from "../workflow-node-execution-needs.js"; + +function node(overrides: Partial = {}): WorkflowIrNode { + return { id: "node", kind: "prompt", ...overrides }; +} + +describe("workflowNodeRequiresWorktree", () => { + it.each([ + ["coding tool mode", node({ config: { toolMode: "coding" } })], + ["script node", node({ kind: "script" })], + ["named script", node({ config: { scriptName: "validate" } })], + ["CLI command", node({ config: { executor: "cli", cliCommand: "pnpm lint" } })], + ["CLI agent", node({ config: { executor: "cli-agent" } })], + ])("requires a worktree for %s", (_name, workflowNode) => { + expect(workflowNodeRequiresWorktree(workflowNode)).toBe(true); + }); + + it.each([ + ["review name", node({ id: "review", config: { name: "Code Review" } }), undefined], + ["verification name", node({ id: "verify", config: { name: "Browser Verification" } }), undefined], + ["explicit inline fix config", node({ config: { reviewCanFixInline: true } }), undefined], + ["code review optional group", node(), "code-review"], + ["browser verification optional group", node(), "browser-verification"], + ])("requires a worktree for inline fixes from %s", (_name, workflowNode, optionalGroupId) => { + expect(workflowNodeRequiresWorktree(workflowNode, { optionalGroupId })).toBe(true); + }); + + it("keeps inline-fix reviews read-only when disabled", () => { + expect(workflowNodeRequiresWorktree(node({ config: { name: "Code Review" } }), { reviewerInlineFixes: false })).toBe(false); + expect(workflowNodeRequiresWorktree(node(), { + optionalGroupId: "code-review", + reviewerInlineFixes: false, + })).toBe(false); + }); + + it.each([ + node({ id: "plan-review-step", config: { name: "Code Review" } }), + node({ config: { name: "Plan Review" } }), + node(), + ])("keeps Plan Review read-only", (workflowNode) => { + expect(workflowNodeRequiresWorktree(workflowNode, { + optionalGroupId: workflowNode.id === "node" ? "plan-review" : undefined, + })).toBe(false); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 3d28b72bba..601304f614 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -46,6 +46,7 @@ import { } 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 type { WorkflowNodePreparationRequirement, WorkflowNodeResult } from "./workflow-graph-executor.js"; +import { workflowNodeRequiresWorktree } from "./workflow-node-execution-needs.js"; import type { AuditPrimitiveInput, PreparedWorktree, @@ -7687,34 +7688,23 @@ export class TaskExecutor { const rawCliCommand = executorKind === "cli" && typeof cfg.cliCommand === "string" && cfg.cliCommand.trim() ? cfg.cliCommand.trim() : undefined; - const nodeNameForReviewDetection = typeof cfg.name === "string" && cfg.name.trim() ? cfg.name.trim() : node.id; - const isPlanReviewNode = - node.id === "plan-review-step" - || nodeNameForReviewDetection === "Plan Review" - || optionalGroupId === "plan-review"; - const inlineFixesEnabledForNode = (settings as Settings & { reviewerInlineFixes?: boolean }).reviewerInlineFixes !== false; - const reviewTypeNode = - isPlanReviewNode - || cfg.reviewCanFixInline === true - || /(?:^|\b)(?:review|verification)(?:\b|$)/i.test(nodeNameForReviewDetection) - || optionalGroupId === "code-review" - || optionalGroupId === "browser-verification"; - const inlineFixesMakeNodeWriteCapable = - inlineFixesEnabledForNode - && executorKind !== "cli" - && reviewTypeNode - && !isPlanReviewNode; - // Isolation guard: write-capable nodes must run inside a task worktree, not // the shared repo root. Before the execute seam runs, live.worktree is unset // — a coding/script/CLI node falling back to this.rootDir would mutate the // main checkout and cross-contaminate other tasks. Reject such nodes until a // worktree exists. Read-only nodes (default toolMode) are safe against root. /* - FNXC:WorkflowReviewers 2026-07-01-13:28: - Inline-fix Code Review, Browser Verification, and custom review nodes become write-capable even when the workflow definition says `toolMode: readonly`, so the isolation guard must see that before selecting a worktree. Plan Review is excluded because it uses the narrow PROMPT.md writer instead of source-file write tools. + FNXC:WorkflowReviewers 2026-07-15-00:00: + Inline-fix Code Review, Browser Verification, and custom review nodes become + write-capable even when their workflow definition says `toolMode: readonly`. + Use the shared classifier consumed by graph preparation so issue #2075 cannot + leave runtime requiring a worktree that preparation declined to acquire. + Plan Review remains excluded because it uses the narrow PROMPT.md writer. */ - const writeCapable = cfg.toolMode === "coding" || inlineFixesMakeNodeWriteCapable || node.kind === "script" || Boolean(scriptName) || Boolean(rawCliCommand); + const writeCapable = workflowNodeRequiresWorktree(node, { + optionalGroupId, + reviewerInlineFixes: (settings as Settings & { reviewerInlineFixes?: boolean }).reviewerInlineFixes, + }); const executionTarget = writeCapable ? await this.store.getTask(live.id) : live; if (writeCapable && !executionTarget.worktree && !this.workspaceConfig) { return { outcome: "failure", value: "no-worktree-for-write-node" }; diff --git a/packages/engine/src/workflow-graph-executor.ts b/packages/engine/src/workflow-graph-executor.ts index 0d4e6c7cb0..f49ad2ea48 100644 --- a/packages/engine/src/workflow-graph-executor.ts +++ b/packages/engine/src/workflow-graph-executor.ts @@ -41,9 +41,14 @@ import { } from "./workflow-graph-foreach.js"; import { runLoop, runOptionalGroup } from "./workflow-graph-loop.js"; import type { WorkflowNodeRunnerRegistry } from "./workflow-node-runner.js"; +import { workflowNodeRequiresWorktree } from "./workflow-node-execution-needs.js"; export type WorkflowNodeOutcome = "success" | "failure"; +type WorkflowNodeSettings = Pick & { + reviewerInlineFixes?: boolean; +}; + export type WorkflowNodeAbortKind = "engine-pause"; export const WORKFLOW_INTERRUPTED_NODE_ID_CONTEXT_KEY = "workflow:interruptedNodeId"; @@ -79,7 +84,7 @@ export interface WorkflowTaskProjection { export interface WorkflowNodeExecutionContext { task: TaskDetail; - settings: Pick | undefined; + settings: WorkflowNodeSettings | undefined; context: Record; /** Set during concurrent branch execution; fail-fast aborts via this signal. * Undefined on the sequential path (zero behavior change for linear graphs). */ @@ -339,7 +344,7 @@ export class WorkflowGraphExecutor { public async run( task: TaskDetail, - settings: (Pick & Partial>) | undefined, + settings: (WorkflowNodeSettings & Partial>) | undefined, ir: WorkflowIr = BUILTIN_CODING_WORKFLOW_IR, ): Promise { const startNode = ir.nodes.find((node) => node.kind === "start"); @@ -1236,7 +1241,7 @@ export class WorkflowGraphExecutor { private async executeNodeWithRetries( node: WorkflowIrNode, task: TaskDetail, - settings: Pick | undefined, + settings: WorkflowNodeSettings | undefined, context: Record, workflow: WorkflowIr, signal?: AbortSignal, @@ -1255,7 +1260,7 @@ export class WorkflowGraphExecutor { // Fail-fast cancellation: a branch or top-level graph abort mid-retry stops re-trying. if (signal?.aborted) return this.withEnginePauseAbortContext(node, { outcome: "failure", value: "aborted" }); try { - await this.prepareNodeExecution(node, task); + await this.prepareNodeExecution(node, task, context, settings); const progressRecord = recordProgress && this.shouldRecordNodeProgress(node) ? await this.recordNodeProgressStart(task.id, node) : null; @@ -1395,23 +1400,36 @@ export class WorkflowGraphExecutor { }); } - private async prepareNodeExecution(node: WorkflowIrNode, task: TaskDetail): Promise { - const requirement = this.classifyNodePreparation(node); + private async prepareNodeExecution( + node: WorkflowIrNode, + task: TaskDetail, + context: Record, + settings: WorkflowNodeSettings | undefined, + ): Promise { + const requirement = this.classifyNodePreparation(node, context, settings); if (!requirement.requiresWorktree) return; await this.deps.prepareNodeExecution?.(node, task, requirement); } - private classifyNodePreparation(node: WorkflowIrNode): WorkflowNodePreparationRequirement { - const cfg = node.config ?? {}; - const executorKind = typeof cfg.executor === "string" ? cfg.executor : "model"; - const hasScriptName = typeof cfg.scriptName === "string" && cfg.scriptName.trim().length > 0; - const hasCliCommand = executorKind === "cli" && typeof cfg.cliCommand === "string" && cfg.cliCommand.trim().length > 0; - const requiresWorktree = - cfg.toolMode === "coding" - || node.kind === "script" - || executorKind === "cli-agent" - || hasScriptName - || hasCliCommand; + private classifyNodePreparation( + node: WorkflowIrNode, + context: Record, + settings: WorkflowNodeSettings | undefined, + ): WorkflowNodePreparationRequirement { + const optionalGroupId = typeof context[WORKFLOW_OPTIONAL_GROUP_CONTEXT_KEY] === "string" + ? context[WORKFLOW_OPTIONAL_GROUP_CONTEXT_KEY] + : undefined; + /* + * FNXC:WorkflowExecution 2026-07-15-00:00: + * Graph preparation receives the optional-group context and effective inline-fix + * setting so it applies the same classifier as runtime. Only an explicit false + * disables inline fixes, preserving the default-enabled review worktree contract + * that prevents issue #2075's pre-review no-worktree failure. + */ + const requiresWorktree = workflowNodeRequiresWorktree(node, { + optionalGroupId, + reviewerInlineFixes: settings?.reviewerInlineFixes, + }); return { requiresWorktree, reason: requiresWorktree ? "write-capable-node" : undefined, diff --git a/packages/engine/src/workflow-node-execution-needs.ts b/packages/engine/src/workflow-node-execution-needs.ts new file mode 100644 index 0000000000..e140804991 --- /dev/null +++ b/packages/engine/src/workflow-node-execution-needs.ts @@ -0,0 +1,46 @@ +import type { WorkflowIrNode } from "@fusion/core"; + +export interface WorkflowNodeExecutionNeedsOptions { + optionalGroupId?: string; + /** Inline review fixes are enabled unless settings explicitly disable them. */ + reviewerInlineFixes?: boolean; +} + +/** + * FNXC:WorkflowExecution 2026-07-15-00:00: + * Issue #2075 exposed divergent worktree classifiers: graph preparation treated + * inline-fix reviews as read-only while runtime rejected them without a worktree. + * This pure helper is the single source of truth for write-capable workflow nodes; + * preparation and runtime must both use it before selecting an execution target. + */ +export function workflowNodeRequiresWorktree( + node: WorkflowIrNode, + { optionalGroupId, reviewerInlineFixes }: WorkflowNodeExecutionNeedsOptions = {}, +): boolean { + const cfg = node.config ?? {}; + const executorKind = typeof cfg.executor === "string" ? cfg.executor : "model"; + const scriptName = typeof cfg.scriptName === "string" && cfg.scriptName.trim() + ? cfg.scriptName + : undefined; + const rawCliCommand = executorKind === "cli" && typeof cfg.cliCommand === "string" && cfg.cliCommand.trim() + ? cfg.cliCommand + : undefined; + const nodeName = typeof cfg.name === "string" && cfg.name.trim() ? cfg.name.trim() : node.id; + const isPlanReview = node.id === "plan-review-step" || nodeName === "Plan Review" || optionalGroupId === "plan-review"; + const isInlineFixReview = reviewerInlineFixes !== false + && executorKind !== "cli" + && !isPlanReview + && ( + cfg.reviewCanFixInline === true + || /(?:^|\b)(?:review|verification)(?:\b|$)/i.test(nodeName) + || optionalGroupId === "code-review" + || optionalGroupId === "browser-verification" + ); + + return cfg.toolMode === "coding" + || node.kind === "script" + || executorKind === "cli-agent" + || Boolean(scriptName) + || Boolean(rawCliCommand) + || isInlineFixReview; +}