diff --git a/.changeset/fn-7228-code-review-blocks.md b/.changeset/fn-7228-code-review-blocks.md new file mode 100644 index 0000000000..44a4202608 --- /dev/null +++ b/.changeset/fn-7228-code-review-blocks.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Make built-in Code Review block merge when it requests revisions. +category: fix +dev: Generic built-in code-review optional groups now use gateMode: gate; Browser Verification remains advisory. diff --git a/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts b/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts index 88689b9344..db0e1808ca 100644 --- a/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts +++ b/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts @@ -8,6 +8,7 @@ import { serializeWorkflowIr, } from "../index.js"; import { BROWSER_VERIFICATION_GROUP_ID, BROWSER_VERIFICATION_STEP_NODE_ID } from "../builtin-browser-verification-group.js"; +import { CODE_REVIEW_GROUP_ID, CODE_REVIEW_STEP_NODE_ID } from "../builtin-code-review-group.js"; import type { WorkflowIrV2 } from "../workflow-ir-types.js"; const EXECUTE_NODE_MAX_RETRIES = 2; @@ -21,6 +22,15 @@ function browserVerificationInnerConfig(ir: WorkflowIrV2): Record { + const group = ir.nodes.find((node) => node.id === CODE_REVIEW_GROUP_ID); + expect(group?.kind).toBe("optional-group"); + const template = group?.config?.template as { nodes?: Array<{ id: string; config?: Record }> } | undefined; + const inner = template?.nodes?.find((node) => node.id === CODE_REVIEW_STEP_NODE_ID); + expect(inner).toBeDefined(); + return inner?.config ?? {}; +} + function executeNodeConfig(ir = BUILTIN_CODING_WORKFLOW_IR): Record { const executeNodes = ir.nodes.filter((node) => node.id === "execute" && node.config?.seam === "execute"); expect(executeNodes).toHaveLength(1); @@ -69,6 +79,10 @@ describe("builtin coding workflow ir", () => { gateMode: "advisory", requiresBrowser: true, }); + expect(codeReviewInnerConfig(BUILTIN_CODING_WORKFLOW_IR)).toMatchObject({ + toolMode: "readonly", + gateMode: "gate", + }); // execute → browser-verification → code-review → review on the success path; the // pre-merge code-review optional-group sits next to browser-verification. failure → end. expect(BUILTIN_CODING_WORKFLOW_IR.edges).toEqual( diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index cf7f90b1a0..6a98eee941 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -101,6 +101,18 @@ describe("built-in workflows", () => { } }); + it("all built-in Code Review optional groups are blocking gates", () => { + for (const workflow of BUILTIN_WORKFLOWS) { + const codeReview = workflow.ir.nodes.find((node) => node.id === "code-review"); + if (!codeReview) continue; + expect(codeReview.kind, workflow.id).toBe("optional-group"); + const template = codeReview.config?.template as { nodes?: Array<{ id: string; config?: Record }> } | undefined; + const inner = template?.nodes?.find((node) => node.id === CODE_REVIEW_STEP_NODE_ID); + expect(inner, workflow.id).toBeDefined(); + expect(inner?.config?.gateMode, workflow.id).toBe("gate"); + } + }); + it("built-in workflow layouts cover every authored node", () => { for (const workflow of BUILTIN_WORKFLOWS) { const missingLayoutNodes = workflow.ir.nodes diff --git a/packages/core/src/builtin-code-review-group.ts b/packages/core/src/builtin-code-review-group.ts index 7d0554c408..e0edaa23ca 100644 --- a/packages/core/src/builtin-code-review-group.ts +++ b/packages/core/src/builtin-code-review-group.ts @@ -14,16 +14,21 @@ The group sits on the pre-merge success path (execute → [browser-verification key; the inner template node carries a DISTINCT id (`code-review-step`) because a template node id may not collide with the group/top-level node id (U1 validation). -The inner node mirrors the dashboard's `stepTemplateToNode` projection of the canonical -`code-review` step: a `prompt` node carrying the prompt, `toolMode` (readonly — review -reads the diff, never mutates), and `gateMode` (advisory — non-blocking, like the -existing review; operators can promote to a gate). +The inner node is a blocking gate: a REVISE verdict must stop review/merge until +the executor remediates the finding or the task exhausts its revision budget. FNXC:CodeReviewStep 2026-06-25-00:00: U6 deleted the built-in step-template catalog; the inner node's literal -name/description/prompt/toolMode/gateMode are now inlined here directly (byte-identical -to the former `code-review` catalog entry). These built-ins are the parity oracle, so -the produced node bytes must NOT change. +name/description/prompt/toolMode/gateMode are now inlined here directly. These built-ins +are the parity oracle; intentional behavior changes must update this file and the +built-in workflow tests together. + +FNXC:CodeReviewStep 2026-06-29-10:42: +Code Review is not advisory. FN-7228 reached merge after Code Review returned REVISE +because the generic built-in was authored as advisory and the graph continued after the +remediation budget was exhausted. Keep browser verification advisory, but make Code +Review a gate so REVISE records a blocking failed workflow step and cannot advance to +review or merge. */ /** Stable per-task enable key + group node id. */ @@ -70,8 +75,8 @@ Be specific: cite \`file:line\` for every finding and explain the concrete failu * matches where the browser-verification group sits (in-progress) so the editor renders * the group in the implementation column. * - * Mirrors `stepTemplateToNode(code-review)`: a single `prompt` node whose config carries - * the inlined prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. + * A single `prompt` node whose config carries the inlined prompt + + * `toolMode: "readonly"` + blocking `gateMode: "gate"`. */ export function codeReviewOptionalGroupNode( column: string, @@ -96,7 +101,7 @@ export function codeReviewOptionalGroupNode( description: CODE_REVIEW_DESCRIPTION, prompt: CODE_REVIEW_PROMPT, toolMode: "readonly", - gateMode: "advisory", + gateMode: "gate", }, }, ], diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index 4800c6554e..467447b594 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -79,7 +79,7 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { // Pre-merge optional browser-verification (optional-group, default OFF). browserVerificationOptionalGroupNode("in-progress"), // FNXC:CodeReviewStep 2026-06-25-15:00: - // Pre-merge Code Review as a DEFAULT-ON optional-group (advisory), on the success path + // Pre-merge Code Review as a DEFAULT-ON optional-group (blocking gate), on the success path // between browser-verification and review (execute → browser-verification → // code-review → review). Runs for every coding task by default (defaultOn:true) but is // toggleable off per task; disabled → byte-inert pass-through. diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index bf656cef34..91b4c7dcc9 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -143,7 +143,7 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // and the rework-exhausted manual-release path flow through this node. browserVerificationOptionalGroupNode("in-progress"), // FNXC:CodeReviewStep 2026-06-25-15:00: - // Pre-merge Code Review as a DEFAULT-ON optional-group (advisory), on the post-foreach + // Pre-merge Code Review as a DEFAULT-ON optional-group (blocking gate), on the post-foreach // success path between browser-verification and review (steps → browser-verification → // code-review → review). It sits after the foreach so it runs EXACTLY ONCE pre-merge // (never per step-instance); both the foreach-success and rework-exhausted manual- 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 e374250021..f98d9ab38a 100644 --- a/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-optional-group.test.ts @@ -723,7 +723,11 @@ describe("WorkflowGraphExecutor optional-group", () => { calls.push(node.id); if ((groupId === "code-review" && node.id === "code-review-step") || (groupId === "browser-verification" && node.id === "browser-verification-step")) { - return { outcome: "success", value: "REVISE", contextPatch: { output: `${groupId} finding` } }; + return { + outcome: groupId === "code-review" ? "failure" : "success", + value: "REVISE", + contextPatch: { output: `${groupId} finding` }, + }; } return { outcome: "success" }; }, @@ -767,7 +771,11 @@ describe("WorkflowGraphExecutor optional-group", () => { prompt: async (node) => { if ((groupId === "code-review" && node.id === "code-review-step") || (groupId === "browser-verification" && node.id === "browser-verification-step")) { - return { outcome: "success", value: "REVISE", contextPatch: { output: `stepwise ${groupId} finding` } }; + return { + outcome: groupId === "code-review" ? "failure" : "success", + value: "REVISE", + contextPatch: { output: `stepwise ${groupId} finding` }, + }; } return { outcome: "success" }; }, @@ -790,4 +798,56 @@ describe("WorkflowGraphExecutor optional-group", () => { expect(stepwiseResult.context[`node:${groupId}:fixScheduled`]).toBe(true); } }); + + it("blocks builtin coding review and merge when Code Review requests revision and no remediation is scheduled", async () => { + const requestFix = vi.fn(async () => false); + const calls: string[] = []; + const executor = new WorkflowGraphExecutor({ + handlers: { + "parse-steps": async () => ({ outcome: "success", value: "no-steps" }), + prompt: async (node) => { + calls.push(node.id); + if (node.id === "code-review-step") { + return { + outcome: "failure", + value: "REVISE", + contextPatch: { output: "blocking code review finding" }, + }; + } + return { outcome: "success" }; + }, + }, + requestPreMergeOptionalStepFix: requestFix, + }); + + const result = await executor.run({ + ...taskWith(["plan-review", "code-review"]), + id: "FN-7228-regression", + steps: [], + workflowStepResults: [ + { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + phase: "pre-merge", + status: "passed", + startedAt: "2026-06-29T17:00:00.000Z", + completedAt: "2026-06-29T17:00:01.000Z", + }, + ], + } as TaskDetail, settingsOn(), BUILTIN_CODING_WORKFLOW_IR); + + expect(requestFix).toHaveBeenCalledWith("FN-7228-regression", expect.objectContaining({ + stepName: "Code Review", + feedback: "blocking code review finding", + nodeId: "code-review", + status: "failed", + verdict: "REVISE", + })); + expect(result.outcome).toBe("failure"); + expect(result.visitedNodeIds).toContain("code-review::code-review-step"); + expect(result.visitedNodeIds).not.toContain("review"); + expect(result.visitedNodeIds).not.toContain("merge-gate"); + expect(result.visitedNodeIds).not.toContain("merge-attempt"); + expect(calls).not.toContain("review"); + }); });