From 7772ab3647876c6780b95e0f55c275a451e85467 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 25 Jun 2026 17:34:50 -0700 Subject: [PATCH 1/3] feat: add built-in Code Review pre-merge workflow step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a configurable "Code Review" diff-review step to the built-in coding and stepwise coding workflows as a default-OFF optional-group prompt gate. It reuses the existing workflow-step machinery and the shared trailing-verdict convention (REVISE blocks, APPROVE/APPROVE_WITH_NOTES pass) — no engine verification code. - New `code-review` WORKFLOW_STEP_TEMPLATE (toolMode readonly, gateMode advisory, phase pre-merge) focused on the correctness value tests miss: logic bugs, edge cases, intent-vs-implementation drift, regressions, error handling, contracts. - New builtin-code-review-group.ts mirroring builtin-browser-verification-group.ts (stable group id `code-review`, distinct inner node id `code-review-step`). - Wired into builtin-coding-workflow-ir.ts and builtin-stepwise-coding-workflow-ir.ts on the pre-merge path next to browser-verification, default OFF / opt-in. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/code-review-workflow-step.md | 7 ++ .../builtin-code-review-group.test.ts | 115 ++++++++++++++++++ .../builtin-coding-workflow-ir.test.ts | 6 +- .../__tests__/workflow-optional-steps.test.ts | 16 ++- .../workflow-step-templates-verdict.test.ts | 1 + .../core/src/builtin-code-review-group.ts | 76 ++++++++++++ .../core/src/builtin-coding-workflow-ir.ts | 13 +- .../builtin-stepwise-coding-workflow-ir.ts | 15 ++- packages/core/src/types.ts | 45 +++++++ 9 files changed, 286 insertions(+), 8 deletions(-) create mode 100644 .changeset/code-review-workflow-step.md create mode 100644 packages/core/src/__tests__/builtin-code-review-group.test.ts create mode 100644 packages/core/src/builtin-code-review-group.ts diff --git a/.changeset/code-review-workflow-step.md b/.changeset/code-review-workflow-step.md new file mode 100644 index 0000000000..d2bac71de6 --- /dev/null +++ b/.changeset/code-review-workflow-step.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Add an optional built-in "Code Review" pre-merge step to the coding workflows. +category: feature +dev: New `code-review` WORKFLOW_STEP_TEMPLATE (toolMode readonly, gateMode advisory, phase pre-merge) plus a default-OFF `optional-group` node (group id `code-review`, inner `code-review-step`) wired into the built-in coding + stepwise coding workflows next to browser-verification. Opt-in via task `enabledWorkflowSteps`; reuses the shared prompt-gate verdict machinery (no engine verification code). diff --git a/packages/core/src/__tests__/builtin-code-review-group.test.ts b/packages/core/src/__tests__/builtin-code-review-group.test.ts new file mode 100644 index 0000000000..03ce69dc7a --- /dev/null +++ b/packages/core/src/__tests__/builtin-code-review-group.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from "vitest"; +import { + CODE_REVIEW_GROUP_ID, + CODE_REVIEW_STEP_NODE_ID, + codeReviewOptionalGroupNode, +} from "../builtin-code-review-group.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; +import { WORKFLOW_STEP_TEMPLATES } from "../types.js"; +import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; +import { + resolveDefaultOnOptionalGroupIds, + resolveWorkflowOptionalSteps, +} from "../workflow-optional-steps.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-12:00: +Coverage for the built-in "Code Review" pre-merge workflow step: the catalog template +fields, the default-OFF optional-group node built from it, and its wiring into the coding ++ stepwise built-ins. Mirrors the browser-verification group/IR suite — code review is a +WORKFLOW prompt-gate (shared verdict machinery), not engine verification code. +*/ + +describe("code-review WORKFLOW_STEP_TEMPLATE", () => { + const template = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + + it("exists with the expected catalog fields", () => { + expect(template).toBeTruthy(); + expect(template!.name).toBe("Code Review"); + expect(template!.toolMode).toBe("readonly"); + // Advisory default → non-blocking until an operator promotes it to a gate (parity + // with browser-verification). + expect(template!.gateMode).toBe("advisory"); + expect(template!.phase).toBe("pre-merge"); + expect(template!.description.length).toBeGreaterThan(0); + }); + + it("ends with the shared trailing verdict convention and reads the diff", () => { + const prompt = template!.prompt; + expect(prompt).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + expect(prompt).not.toContain('"verdict":"PASS"'); + expect(prompt).not.toContain('"verdict":"FAIL"'); + // Focused on the value tests miss + reads the diff against the base. + expect(prompt).toMatch(/git diff/); + expect(prompt).toMatch(/out of scope/i); + }); +}); + +describe("codeReviewOptionalGroupNode", () => { + it("builds a default-OFF optional-group with the stable group id and distinct inner id", () => { + const node = codeReviewOptionalGroupNode("in-progress"); + expect(node.id).toBe(CODE_REVIEW_GROUP_ID); + expect(CODE_REVIEW_GROUP_ID).toBe("code-review"); + expect(CODE_REVIEW_STEP_NODE_ID).toBe("code-review-step"); + expect(node.id).not.toBe(CODE_REVIEW_STEP_NODE_ID); // U1: inner id ≠ group id. + expect(node.kind).toBe("optional-group"); + expect(node.column).toBe("in-progress"); + expect(node.config?.name).toBe("Code Review"); + expect(node.config?.defaultOn).toBe(false); + + const template = node.config?.template as { nodes: { id: string; kind: string; config?: Record }[] }; + expect(template.nodes).toHaveLength(1); + const inner = template.nodes[0]; + expect(inner.id).toBe(CODE_REVIEW_STEP_NODE_ID); + expect(inner.kind).toBe("prompt"); + expect(inner.config?.toolMode).toBe("readonly"); + expect(inner.config?.gateMode).toBe("advisory"); + expect(String(inner.config?.prompt)).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + }); +}); + +describe("built-in coding + stepwise workflows wire code-review default-OFF on the pre-merge path", () => { + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s includes the code-review optional-group and still parses/round-trips", (_name, ir) => { + const byId = new Map(ir.nodes.map((n) => [n.id, n])); + const group = byId.get("code-review"); + expect(group?.kind).toBe("optional-group"); + expect(group?.config?.name).toBe("Code Review"); + expect(group?.config?.defaultOn).toBe(false); + expect(group?.column).toBe("in-progress"); + + // Pre-merge wiring: ... → browser-verification → code-review → review; failure → end. + expect(ir.edges).toEqual( + expect.arrayContaining([ + expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "end", condition: "failure" }), + ]), + ); + + // The built-in still compiles/validates with the new node (parse round-trips). + const reparsed = parseWorkflowIr(serializeWorkflowIr(ir)); + expect(reparsed).toEqual(parseWorkflowIr(ir)); + }); + + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s: code-review is advertised as an opt-in toggle but never default-seeded", (_name, ir) => { + // Enabling it (task.enabledWorkflowSteps includes `code-review`) surfaces it on the + // pre-merge path; the resolver advertises the toggle keyed by the group id. + const advertised = resolveWorkflowOptionalSteps(ir).find((s) => s.templateId === "code-review"); + expect(advertised).toEqual({ + templateId: "code-review", + name: "Code Review", + description: "", + phase: "pre-merge", + defaultOn: false, + }); + // Default OFF → byte-inert pass-through: never auto-seeded into a new task. + expect(resolveDefaultOnOptionalGroupIds(ir)).not.toContain("code-review"); + }); +}); 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 7d7f31bfdf..db3815a4d4 100644 --- a/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts +++ b/packages/core/src/__tests__/builtin-coding-workflow-ir.test.ts @@ -53,11 +53,13 @@ describe("builtin coding workflow ir", () => { expect(group?.kind).toBe("optional-group"); expect(group?.config?.name).toBe("Browser Verification"); expect(group?.config?.defaultOn).toBe(false); - // execute → browser-verification → review on the success path; failure → end. + // 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( expect.arrayContaining([ expect.objectContaining({ from: "execute", to: "browser-verification", condition: "success" }), - expect.objectContaining({ from: "browser-verification", to: "review", condition: "success" }), + expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), expect.objectContaining({ from: "browser-verification", to: "end", condition: "failure" }), ]), ); diff --git a/packages/core/src/__tests__/workflow-optional-steps.test.ts b/packages/core/src/__tests__/workflow-optional-steps.test.ts index 5f101815d0..c591cc2cd5 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -108,10 +108,11 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { ]); }); - it("resolves the built-in coding/stepwise browser-verification optional-group (U6)", () => { - // U6 migrated both built-ins: `browser-verification` is now an optional-group - // node (default OFF), so the resolver advertises exactly one toggle entry per - // built-in, keyed by the group node id `browser-verification`. + it("resolves the built-in coding/stepwise browser-verification + code-review optional-groups", () => { + // U6 migrated both built-ins: `browser-verification` is an optional-group node + // (default OFF). The pre-merge `code-review` optional-group (also default OFF) sits + // next to it on the same success path, so the resolver advertises exactly these two + // toggle entries per built-in, in node order (browser-verification → code-review). const expected = [ { templateId: "browser-verification", @@ -120,6 +121,13 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { phase: "pre-merge" as const, defaultOn: false, }, + { + templateId: "code-review", + name: "Code Review", + description: "", + phase: "pre-merge" as const, + defaultOn: false, + }, ]; expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(expected); expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(expected); diff --git a/packages/core/src/__tests__/workflow-step-templates-verdict.test.ts b/packages/core/src/__tests__/workflow-step-templates-verdict.test.ts index 3a22821f22..152831a23e 100644 --- a/packages/core/src/__tests__/workflow-step-templates-verdict.test.ts +++ b/packages/core/src/__tests__/workflow-step-templates-verdict.test.ts @@ -8,6 +8,7 @@ const TARGET_IDS = [ "performance-review", "accessibility-check", "browser-verification", + "code-review", "frontend-ux-design", ] as const; diff --git a/packages/core/src/builtin-code-review-group.ts b/packages/core/src/builtin-code-review-group.ts new file mode 100644 index 0000000000..5a9f47252c --- /dev/null +++ b/packages/core/src/builtin-code-review-group.ts @@ -0,0 +1,76 @@ +import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-12:00: +The built-in coding and stepwise-coding workflows express the optional `code-review` +step as an `optional-group` container node on the pre-merge path (default OFF), +mirroring `builtin-browser-verification-group.ts` exactly. Enabled (the task's +`enabledWorkflowSteps` includes the group id) → the diff-review step runs ONCE +pre-merge between implementation and review. Disabled → the group passes through +(byte-inert), so the step is purely opt-in. + +The group node id `code-review` is the STABLE per-task enable key (KTD-2). 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` WORKFLOW_STEP_TEMPLATE: a `prompt` node carrying the template's prompt, +`toolMode` (readonly — review reads the diff, never mutates), and `gateMode` (advisory +default — non-blocking until an operator promotes it). Sourcing prompt/toolMode/gateMode +from the catalog keeps the built-in byte-identical to the template a human would insert +from the palette (KTD-5). +*/ + +function resolveCodeReviewTemplate() { + const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + if (!tpl) { + throw new Error("code-review WORKFLOW_STEP_TEMPLATE is missing"); + } + return tpl; +} + +const CODE_REVIEW_TEMPLATE = resolveCodeReviewTemplate(); + +/** Stable per-task enable key + group node id. */ +export const CODE_REVIEW_GROUP_ID = "code-review"; + +/** Inner template node id — distinct from the group id (template-node-id collision rule, U1). */ +export const CODE_REVIEW_STEP_NODE_ID = "code-review-step"; + +/** + * Build the `code-review` optional-group node placed on a workflow's pre-merge path. + * `column` 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 catalog prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. + */ +export function codeReviewOptionalGroupNode(column: string): WorkflowIrNode { + const tpl = CODE_REVIEW_TEMPLATE; + return { + id: CODE_REVIEW_GROUP_ID, + kind: "optional-group", + column, + config: { + name: tpl.name, + defaultOn: false, + template: { + nodes: [ + { + id: CODE_REVIEW_STEP_NODE_ID, + kind: "prompt", + config: { + name: tpl.name, + description: tpl.description, + prompt: tpl.prompt ?? "", + toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", + gateMode: tpl.gateMode ?? "advisory", + }, + }, + ], + edges: [], + }, + }, + }; +} diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index b506ffec31..3ed5d173ba 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -3,6 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; +import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; /** * The built-in default workflow as a v2 IR. Its six columns have ids that are @@ -75,6 +76,12 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { }, // Pre-merge optional browser-verification (optional-group, default OFF). browserVerificationOptionalGroupNode("in-progress"), + // FNXC:CodeReviewStep 2026-06-25-12:00: + // Pre-merge optional Code Review (optional-group, default OFF), placed next to the + // browser-verification group on the same success path (execute → browser-verification + // → code-review → review). Disabled → passes through inert; enabled → diff-reviews the + // change once pre-merge. + codeReviewOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, @@ -101,7 +108,10 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { // execute → browser-verification (optional-group) → review. When the group is // disabled it passes through with outcome=success and routes straight to review. { from: "execute", to: "browser-verification", condition: "success" }, - { from: "browser-verification", to: "review", condition: "success" }, + // browser-verification → code-review → review. Each optional-group passes through with + // outcome=success when disabled, so a task with both off routes straight to review. + { from: "browser-verification", to: "code-review", condition: "success" }, + { from: "code-review", to: "review", condition: "success" }, { from: "review", to: "merge-gate", condition: "success" }, { from: "merge-gate", to: "branch-group-member-integration", condition: "outcome:auto-on" }, { from: "merge-gate", to: "merge-manual-hold", condition: "outcome:auto-off" }, @@ -118,6 +128,7 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { { from: "planning", to: "end", condition: "failure" }, { from: "execute", to: "end", condition: "failure" }, { from: "browser-verification", to: "end", condition: "failure" }, + { from: "code-review", to: "end", condition: "failure" }, { from: "review", to: "end", condition: "failure" }, { from: "merge-attempt", to: "end", condition: "failure" }, ], diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index 39f14f740e..4d6115df40 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -3,6 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; +import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; /** * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step @@ -134,6 +135,14 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // disabled the group passes through inert. Both the normal foreach-success path // and the rework-exhausted manual-release path flow through this node. browserVerificationOptionalGroupNode("in-progress"), + // FNXC:CodeReviewStep 2026-06-25-12:00: + // Pre-merge optional Code Review (optional-group, default OFF), placed next to the + // browser-verification group on the post-foreach success path (steps → browser- + // verification → code-review → review). Same R-3 run-once guarantee: it sits after the + // foreach so an enabled task runs it EXACTLY ONCE pre-merge, never per step-instance; + // disabled → inert pass-through. Both the foreach-success and rework-exhausted manual- + // release paths flow through it. + codeReviewOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, @@ -172,8 +181,12 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // KTD-5: bounded rework exhaustion → manual hold; release re-enters the group. { from: "steps", to: "rework-hold", condition: "outcome:rework-exhausted" }, { from: "rework-hold", to: "browser-verification", condition: "success" }, - { from: "browser-verification", to: "review", condition: "success" }, + // browser-verification → code-review → review; each optional-group passes through + // (outcome=success) when disabled, so a task with both off routes straight to review. + { from: "browser-verification", to: "code-review", condition: "success" }, + { from: "code-review", to: "review", condition: "success" }, { from: "browser-verification", to: "end", condition: "failure" }, + { from: "code-review", to: "end", condition: "failure" }, { from: "steps", to: "end", condition: "failure" }, { from: "review", to: "merge-gate", condition: "success" }, { from: "review", to: "end", condition: "failure" }, diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 2d27f1d78b..40af737a30 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1102,6 +1102,51 @@ Use these agent-browser commands for verification: {"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"..."} Note: Refs (@e1, @e2) are invalidated after page navigation. Re-snapshot after clicking links or form submissions.`, + }, + { + /* + FNXC:CodeReviewStep 2026-06-25-12:00: + Built-in "Code Review" catalog template: a configurable pre-merge prompt-gate that + diff-reviews the task's changes for the correctness value automated tests miss + (logic bugs, broken edge cases, intent-vs-implementation drift, regressions in + touched paths, error handling, contract/signature breaks). This is the WORKFLOW-layer + code review — it reuses the shared prompt-gate verdict machinery, NOT engine + verification code. gateMode defaults to "advisory" (non-blocking) exactly like + browser-verification, so it is opt-in/non-blocking until an operator promotes it to a + blocking gate. phase "pre-merge" places it before merge. toolMode "readonly": review + reads the diff/files, it does not mutate the worktree. + */ + id: "code-review", + name: "Code Review", + description: "Diff-review the task's changes for correctness bugs, regressions, and intent mismatches that tests miss", + category: "Quality", + icon: "git-pull-request", + toolMode: "readonly", + gateMode: "advisory", + phase: "pre-merge", + prompt: `You are a senior code reviewer. Review the task's diff for the correctness value automated tests do NOT catch. + +## Step 1: Read the change +1. Read the full diff against the base branch: \`git diff ...HEAD\` (or \`git diff \`). Determine the base from the task context / merge target. +2. Read the changed files in full where the diff is non-trivial, so you see the surrounding code paths the change touches — not just the hunks. + +## Step 2: Review focus (the value tests miss) +1. **Correctness / logic bugs** — wrong conditions, inverted boolean/comparison logic, off-by-one, incorrect operator precedence, mishandled return values. +2. **Broken edge cases** — empty/undefined/null inputs, zero/duplicate/boundary values, concurrency and ordering assumptions. +3. **Intent vs implementation** — does the code actually do what the task/PROMPT.md describes? Flag silent scope drift or partial implementations. +4. **Regressions in touched code paths** — does the change break or weaken an existing behavior in the files it edits or their callers? +5. **Error handling** — swallowed errors, unhandled rejections/exceptions, missing validation at trust boundaries, misleading error messages. +6. **Contract / signature changes** — changed function/exported-type signatures, API request/response shapes, or serialization that breaks existing callers. + +Be specific: cite \`file:line\` for every finding and explain the concrete failure it causes. + +## Output Requirements +- Fast-bail: if the diff is trivial, generated, or out-of-scope for code review (e.g. pure docs/config/formatting with no logic), output {"verdict":"APPROVE","notes":"out of scope: code review"} immediately and stop. +- APPROVE: no correctness concerns; use empty or brief notes. +- APPROVE_WITH_NOTES: shippable, but include non-blocking advisories (with file:line) in notes. +- REVISE: a correctness bug, regression, or contract break requires changes; include file:line and the concrete failure plus remediation in notes. +- Final output: output exactly one trailing JSON object on the final line (no markdown fences, no surrounding prose): +{"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"..."}`, }, { id: "frontend-ux-design", From a50074d411c52c17f0f8dc78a0f6401c08ff13ae Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 25 Jun 2026 17:57:08 -0700 Subject: [PATCH 2/3] feat: make Code Review a standard always-on pre-merge step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Design correction: Code Review is now a STANDARD, default-ON step in the built-in coding and stepwise coding workflows — not a default-off optional-group toggle. It is a regular advisory `prompt` node on the pre-merge success path (execute → [browser-verification optional] → code-review → review), so it runs for every coding task with no enabledWorkflowSteps gating. Advisory gateMode means it does not change merge outcomes; operators can promote it to a blocking gate. - Replace the optional-group module with a standard prompt-node builder (builtin-code-review-group.ts → builtin-code-review-node.ts). - Keep the `code-review` WORKFLOW_STEP_TEMPLATE in the catalog (editor palette). - Edges unchanged: code-review → review on success, code-review → end on failure (mirrors the existing review node, no dead-end). - Update tests + changeset for the standard always-on (no-toggle) semantics. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/code-review-workflow-step.md | 4 +- .../builtin-code-review-group.test.ts | 115 ------------------ .../builtin-code-review-node.test.ts | 97 +++++++++++++++ .../__tests__/workflow-optional-steps.test.ts | 18 +-- .../core/src/builtin-code-review-group.ts | 76 ------------ packages/core/src/builtin-code-review-node.ts | 57 +++++++++ .../core/src/builtin-coding-workflow-ir.ts | 13 +- .../builtin-stepwise-coding-workflow-ir.ts | 17 ++- 8 files changed, 176 insertions(+), 221 deletions(-) delete mode 100644 packages/core/src/__tests__/builtin-code-review-group.test.ts create mode 100644 packages/core/src/__tests__/builtin-code-review-node.test.ts delete mode 100644 packages/core/src/builtin-code-review-group.ts create mode 100644 packages/core/src/builtin-code-review-node.ts diff --git a/.changeset/code-review-workflow-step.md b/.changeset/code-review-workflow-step.md index d2bac71de6..ac9faa5c62 100644 --- a/.changeset/code-review-workflow-step.md +++ b/.changeset/code-review-workflow-step.md @@ -2,6 +2,6 @@ "@runfusion/fusion": minor --- -summary: Add an optional built-in "Code Review" pre-merge step to the coding workflows. +summary: Add a standard pre-merge Code Review step to the built-in coding workflows. category: feature -dev: New `code-review` WORKFLOW_STEP_TEMPLATE (toolMode readonly, gateMode advisory, phase pre-merge) plus a default-OFF `optional-group` node (group id `code-review`, inner `code-review-step`) wired into the built-in coding + stepwise coding workflows next to browser-verification. Opt-in via task `enabledWorkflowSteps`; reuses the shared prompt-gate verdict machinery (no engine verification code). +dev: New always-on `code-review` prompt node (toolMode readonly, gateMode advisory, phase pre-merge) on the pre-merge success path (execute → browser-verification → code-review → review) of both the built-in coding and stepwise coding workflows. Runs for every coding task by default with no `enabledWorkflowSteps` gating; advisory so it does not change merge outcomes (operators can promote it to a blocking gate). Reuses the shared prompt-gate verdict machinery (no engine verification code). The `code-review` WORKFLOW_STEP_TEMPLATE is also available in the editor palette. diff --git a/packages/core/src/__tests__/builtin-code-review-group.test.ts b/packages/core/src/__tests__/builtin-code-review-group.test.ts deleted file mode 100644 index 03ce69dc7a..0000000000 --- a/packages/core/src/__tests__/builtin-code-review-group.test.ts +++ /dev/null @@ -1,115 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - CODE_REVIEW_GROUP_ID, - CODE_REVIEW_STEP_NODE_ID, - codeReviewOptionalGroupNode, -} from "../builtin-code-review-group.js"; -import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; -import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; -import { WORKFLOW_STEP_TEMPLATES } from "../types.js"; -import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; -import { - resolveDefaultOnOptionalGroupIds, - resolveWorkflowOptionalSteps, -} from "../workflow-optional-steps.js"; - -/* -FNXC:CodeReviewStep 2026-06-25-12:00: -Coverage for the built-in "Code Review" pre-merge workflow step: the catalog template -fields, the default-OFF optional-group node built from it, and its wiring into the coding -+ stepwise built-ins. Mirrors the browser-verification group/IR suite — code review is a -WORKFLOW prompt-gate (shared verdict machinery), not engine verification code. -*/ - -describe("code-review WORKFLOW_STEP_TEMPLATE", () => { - const template = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); - - it("exists with the expected catalog fields", () => { - expect(template).toBeTruthy(); - expect(template!.name).toBe("Code Review"); - expect(template!.toolMode).toBe("readonly"); - // Advisory default → non-blocking until an operator promotes it to a gate (parity - // with browser-verification). - expect(template!.gateMode).toBe("advisory"); - expect(template!.phase).toBe("pre-merge"); - expect(template!.description.length).toBeGreaterThan(0); - }); - - it("ends with the shared trailing verdict convention and reads the diff", () => { - const prompt = template!.prompt; - expect(prompt).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); - expect(prompt).not.toContain('"verdict":"PASS"'); - expect(prompt).not.toContain('"verdict":"FAIL"'); - // Focused on the value tests miss + reads the diff against the base. - expect(prompt).toMatch(/git diff/); - expect(prompt).toMatch(/out of scope/i); - }); -}); - -describe("codeReviewOptionalGroupNode", () => { - it("builds a default-OFF optional-group with the stable group id and distinct inner id", () => { - const node = codeReviewOptionalGroupNode("in-progress"); - expect(node.id).toBe(CODE_REVIEW_GROUP_ID); - expect(CODE_REVIEW_GROUP_ID).toBe("code-review"); - expect(CODE_REVIEW_STEP_NODE_ID).toBe("code-review-step"); - expect(node.id).not.toBe(CODE_REVIEW_STEP_NODE_ID); // U1: inner id ≠ group id. - expect(node.kind).toBe("optional-group"); - expect(node.column).toBe("in-progress"); - expect(node.config?.name).toBe("Code Review"); - expect(node.config?.defaultOn).toBe(false); - - const template = node.config?.template as { nodes: { id: string; kind: string; config?: Record }[] }; - expect(template.nodes).toHaveLength(1); - const inner = template.nodes[0]; - expect(inner.id).toBe(CODE_REVIEW_STEP_NODE_ID); - expect(inner.kind).toBe("prompt"); - expect(inner.config?.toolMode).toBe("readonly"); - expect(inner.config?.gateMode).toBe("advisory"); - expect(String(inner.config?.prompt)).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); - }); -}); - -describe("built-in coding + stepwise workflows wire code-review default-OFF on the pre-merge path", () => { - it.each([ - ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], - ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], - ])("%s includes the code-review optional-group and still parses/round-trips", (_name, ir) => { - const byId = new Map(ir.nodes.map((n) => [n.id, n])); - const group = byId.get("code-review"); - expect(group?.kind).toBe("optional-group"); - expect(group?.config?.name).toBe("Code Review"); - expect(group?.config?.defaultOn).toBe(false); - expect(group?.column).toBe("in-progress"); - - // Pre-merge wiring: ... → browser-verification → code-review → review; failure → end. - expect(ir.edges).toEqual( - expect.arrayContaining([ - expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), - expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), - expect.objectContaining({ from: "code-review", to: "end", condition: "failure" }), - ]), - ); - - // The built-in still compiles/validates with the new node (parse round-trips). - const reparsed = parseWorkflowIr(serializeWorkflowIr(ir)); - expect(reparsed).toEqual(parseWorkflowIr(ir)); - }); - - it.each([ - ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], - ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], - ])("%s: code-review is advertised as an opt-in toggle but never default-seeded", (_name, ir) => { - // Enabling it (task.enabledWorkflowSteps includes `code-review`) surfaces it on the - // pre-merge path; the resolver advertises the toggle keyed by the group id. - const advertised = resolveWorkflowOptionalSteps(ir).find((s) => s.templateId === "code-review"); - expect(advertised).toEqual({ - templateId: "code-review", - name: "Code Review", - description: "", - phase: "pre-merge", - defaultOn: false, - }); - // Default OFF → byte-inert pass-through: never auto-seeded into a new task. - expect(resolveDefaultOnOptionalGroupIds(ir)).not.toContain("code-review"); - }); -}); diff --git a/packages/core/src/__tests__/builtin-code-review-node.test.ts b/packages/core/src/__tests__/builtin-code-review-node.test.ts new file mode 100644 index 0000000000..579745c264 --- /dev/null +++ b/packages/core/src/__tests__/builtin-code-review-node.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from "vitest"; +import { + CODE_REVIEW_NODE_ID, + codeReviewStepNode, +} from "../builtin-code-review-node.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; +import { WORKFLOW_STEP_TEMPLATES } from "../types.js"; +import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; +import { resolveWorkflowOptionalSteps } from "../workflow-optional-steps.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-13:30: +Coverage for the STANDARD, always-on "Code Review" pre-merge step: the catalog template +fields, the regular `prompt` node built from it, and its wiring as a default-ON step in +the coding + stepwise built-ins (no enabledWorkflowSteps gating). Code review is a +WORKFLOW prompt step (shared verdict machinery), not engine verification code. +*/ + +describe("code-review WORKFLOW_STEP_TEMPLATE", () => { + const template = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + + it("exists with the expected catalog fields", () => { + expect(template).toBeTruthy(); + expect(template!.name).toBe("Code Review"); + expect(template!.toolMode).toBe("readonly"); + // Advisory → non-blocking by default (like the existing review); operators can promote. + expect(template!.gateMode).toBe("advisory"); + expect(template!.phase).toBe("pre-merge"); + expect(template!.description.length).toBeGreaterThan(0); + }); + + it("ends with the shared trailing verdict convention and reads the diff", () => { + const prompt = template!.prompt; + expect(prompt).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + expect(prompt).not.toContain('"verdict":"PASS"'); + expect(prompt).not.toContain('"verdict":"FAIL"'); + // Focused on the value tests miss + reads the diff against the base. + expect(prompt).toMatch(/git diff/); + expect(prompt).toMatch(/out of scope/i); + }); +}); + +describe("codeReviewStepNode", () => { + it("builds a standard advisory readonly prompt node keyed by the catalog id", () => { + const node = codeReviewStepNode("in-progress"); + expect(node.id).toBe(CODE_REVIEW_NODE_ID); + expect(CODE_REVIEW_NODE_ID).toBe("code-review"); + // Standard node, NOT an optional-group toggle. + expect(node.kind).toBe("prompt"); + expect(node.column).toBe("in-progress"); + expect(node.config?.name).toBe("Code Review"); + expect(node.config?.toolMode).toBe("readonly"); + expect(node.config?.gateMode).toBe("advisory"); + expect(node.config?.defaultOn).toBeUndefined(); // no optional-group toggle semantics. + expect(String(node.config?.prompt)).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + }); +}); + +describe("built-in coding + stepwise workflows wire code-review as a standard always-on step", () => { + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s includes a default-ON code-review prompt node between browser-verification and review", (_name, ir) => { + const byId = new Map(ir.nodes.map((n) => [n.id, n])); + const node = byId.get("code-review"); + // Always present as a standard prompt node (not an optional-group, no toggle). + expect(node?.kind).toBe("prompt"); + expect(node?.config?.name).toBe("Code Review"); + expect(node?.config?.gateMode).toBe("advisory"); + expect(node?.config?.toolMode).toBe("readonly"); + + // Pre-merge wiring: ... → browser-verification → code-review → review; failure → end + // (mirrors how the existing review node fails to end — no dead-end). + expect(ir.edges).toEqual( + expect.arrayContaining([ + expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "end", condition: "failure" }), + ]), + ); + + // The built-in still compiles/validates with the standard node (parse round-trips). + const reparsed = parseWorkflowIr(serializeWorkflowIr(ir)); + expect(reparsed).toEqual(parseWorkflowIr(ir)); + }); + + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s: code-review is NOT advertised as an optional-step toggle (always-on, no gating)", (_name, ir) => { + // Standard step → never surfaces in the optional-step toggle list (it is not gated + // on task.enabledWorkflowSteps; it runs for every coding task). + const toggles = resolveWorkflowOptionalSteps(ir).map((s) => s.templateId); + expect(toggles).not.toContain("code-review"); + }); +}); diff --git a/packages/core/src/__tests__/workflow-optional-steps.test.ts b/packages/core/src/__tests__/workflow-optional-steps.test.ts index c591cc2cd5..2d72b029fa 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -108,11 +108,12 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { ]); }); - it("resolves the built-in coding/stepwise browser-verification + code-review optional-groups", () => { - // U6 migrated both built-ins: `browser-verification` is an optional-group node - // (default OFF). The pre-merge `code-review` optional-group (also default OFF) sits - // next to it on the same success path, so the resolver advertises exactly these two - // toggle entries per built-in, in node order (browser-verification → code-review). + it("resolves the built-in coding/stepwise browser-verification optional-group (U6)", () => { + // U6 migrated both built-ins: `browser-verification` is now an optional-group + // node (default OFF), so the resolver advertises exactly one toggle entry per + // built-in, keyed by the group node id `browser-verification`. The standard + // always-on `code-review` prompt node is NOT an optional-group, so it never + // surfaces as a toggle entry here. const expected = [ { templateId: "browser-verification", @@ -121,13 +122,6 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { phase: "pre-merge" as const, defaultOn: false, }, - { - templateId: "code-review", - name: "Code Review", - description: "", - phase: "pre-merge" as const, - defaultOn: false, - }, ]; expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(expected); expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(expected); diff --git a/packages/core/src/builtin-code-review-group.ts b/packages/core/src/builtin-code-review-group.ts deleted file mode 100644 index 5a9f47252c..0000000000 --- a/packages/core/src/builtin-code-review-group.ts +++ /dev/null @@ -1,76 +0,0 @@ -import type { WorkflowIrNode } from "./workflow-ir-types.js"; -import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; - -/* -FNXC:CodeReviewStep 2026-06-25-12:00: -The built-in coding and stepwise-coding workflows express the optional `code-review` -step as an `optional-group` container node on the pre-merge path (default OFF), -mirroring `builtin-browser-verification-group.ts` exactly. Enabled (the task's -`enabledWorkflowSteps` includes the group id) → the diff-review step runs ONCE -pre-merge between implementation and review. Disabled → the group passes through -(byte-inert), so the step is purely opt-in. - -The group node id `code-review` is the STABLE per-task enable key (KTD-2). 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` WORKFLOW_STEP_TEMPLATE: a `prompt` node carrying the template's prompt, -`toolMode` (readonly — review reads the diff, never mutates), and `gateMode` (advisory -default — non-blocking until an operator promotes it). Sourcing prompt/toolMode/gateMode -from the catalog keeps the built-in byte-identical to the template a human would insert -from the palette (KTD-5). -*/ - -function resolveCodeReviewTemplate() { - const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); - if (!tpl) { - throw new Error("code-review WORKFLOW_STEP_TEMPLATE is missing"); - } - return tpl; -} - -const CODE_REVIEW_TEMPLATE = resolveCodeReviewTemplate(); - -/** Stable per-task enable key + group node id. */ -export const CODE_REVIEW_GROUP_ID = "code-review"; - -/** Inner template node id — distinct from the group id (template-node-id collision rule, U1). */ -export const CODE_REVIEW_STEP_NODE_ID = "code-review-step"; - -/** - * Build the `code-review` optional-group node placed on a workflow's pre-merge path. - * `column` 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 catalog prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. - */ -export function codeReviewOptionalGroupNode(column: string): WorkflowIrNode { - const tpl = CODE_REVIEW_TEMPLATE; - return { - id: CODE_REVIEW_GROUP_ID, - kind: "optional-group", - column, - config: { - name: tpl.name, - defaultOn: false, - template: { - nodes: [ - { - id: CODE_REVIEW_STEP_NODE_ID, - kind: "prompt", - config: { - name: tpl.name, - description: tpl.description, - prompt: tpl.prompt ?? "", - toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", - gateMode: tpl.gateMode ?? "advisory", - }, - }, - ], - edges: [], - }, - }, - }; -} diff --git a/packages/core/src/builtin-code-review-node.ts b/packages/core/src/builtin-code-review-node.ts new file mode 100644 index 0000000000..426a8d6b53 --- /dev/null +++ b/packages/core/src/builtin-code-review-node.ts @@ -0,0 +1,57 @@ +import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-13:30: +Code Review is a STANDARD, always-on step in the built-in coding and stepwise-coding +workflows — NOT a default-off optional-group toggle. It is a regular `prompt` node on the +pre-merge success path (execute → [browser-verification optional] → code-review → review), +so it runs for EVERY coding task by default with no `enabledWorkflowSteps` gating. + +gateMode is "advisory" (sourced from the catalog template): like the existing `review` +seam it does not change merge outcomes — it just adds the diff-correctness review to the +standard flow. Operators can promote it to a blocking gate later. toolMode is "readonly": +review reads the diff/files, it does not mutate the worktree. + +The node id `code-review` is also the catalog template id, so the built-in node stays +byte-identical to the `code-review` template a human would insert from the editor palette +(prompt/toolMode/gateMode all sourced from the catalog). +*/ + +function resolveCodeReviewTemplate() { + const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + if (!tpl) { + throw new Error("code-review WORKFLOW_STEP_TEMPLATE is missing"); + } + return tpl; +} + +const CODE_REVIEW_TEMPLATE = resolveCodeReviewTemplate(); + +/** Standard pre-merge code-review node id (also the catalog template id). */ +export const CODE_REVIEW_NODE_ID = "code-review"; + +/** + * Build the standard, always-on `code-review` prompt node placed on a workflow's + * pre-merge success path between browser-verification and review. `column` matches the + * pre-merge implementation column (`in-progress`) so the single in-progress → in-review + * status transition stays at code-review → review. + * + * Mirrors `stepTemplateToNode(code-review)`: a `prompt` node whose config carries the + * catalog prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. + */ +export function codeReviewStepNode(column: string): WorkflowIrNode { + const tpl = CODE_REVIEW_TEMPLATE; + return { + id: CODE_REVIEW_NODE_ID, + kind: "prompt", + column, + config: { + name: tpl.name, + description: tpl.description, + prompt: tpl.prompt ?? "", + toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", + gateMode: tpl.gateMode ?? "advisory", + }, + }; +} diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index 3ed5d173ba..94ecbc7aed 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -3,7 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; -import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; +import { codeReviewStepNode } from "./builtin-code-review-node.js"; /** * The built-in default workflow as a v2 IR. Its six columns have ids that are @@ -76,12 +76,11 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { }, // Pre-merge optional browser-verification (optional-group, default OFF). browserVerificationOptionalGroupNode("in-progress"), - // FNXC:CodeReviewStep 2026-06-25-12:00: - // Pre-merge optional Code Review (optional-group, default OFF), placed next to the - // browser-verification group on the same success path (execute → browser-verification - // → code-review → review). Disabled → passes through inert; enabled → diff-reviews the - // change once pre-merge. - codeReviewOptionalGroupNode("in-progress"), + // FNXC:CodeReviewStep 2026-06-25-13:30: + // STANDARD always-on pre-merge Code Review prompt node (advisory), on the success + // path between browser-verification and review (execute → browser-verification → + // code-review → review). Runs for every coding task by default — no enable toggle. + codeReviewStepNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index 4d6115df40..ae7d9525f7 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -3,7 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; -import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; +import { codeReviewStepNode } from "./builtin-code-review-node.js"; /** * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step @@ -135,14 +135,13 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // disabled the group passes through inert. Both the normal foreach-success path // and the rework-exhausted manual-release path flow through this node. browserVerificationOptionalGroupNode("in-progress"), - // FNXC:CodeReviewStep 2026-06-25-12:00: - // Pre-merge optional Code Review (optional-group, default OFF), placed next to the - // browser-verification group on the post-foreach success path (steps → browser- - // verification → code-review → review). Same R-3 run-once guarantee: it sits after the - // foreach so an enabled task runs it EXACTLY ONCE pre-merge, never per step-instance; - // disabled → inert pass-through. Both the foreach-success and rework-exhausted manual- - // release paths flow through it. - codeReviewOptionalGroupNode("in-progress"), + // FNXC:CodeReviewStep 2026-06-25-13:30: + // STANDARD always-on pre-merge Code Review prompt node (advisory), 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- + // release paths flow through it. Runs for every task by default — no enable toggle. + codeReviewStepNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, From 4ea60843224ea1845b16f2e8e0a4c30c9c5297cf Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 25 Jun 2026 18:16:59 -0700 Subject: [PATCH 3/3] feat: make Code Review a default-on toggleable optional-group step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refinement: Code Review is now a DEFAULT-ON but toggleable `optional-group` in the built-in coding and stepwise coding workflows (defaultOn:true), not a standard always-on node. It is part of the existing pre-merge flow (execute → [browser-verification optional] → code-review → review) and runs for every coding task by default, yet an operator can toggle it off per task by removing `code-review` from enabledWorkflowSteps; disabled → byte-inert pass-through. Advisory gateMode keeps it non-blocking (operators can promote to a gate); toolMode readonly. - Restore the optional-group builder (builtin-code-review-node.ts → -group.ts) with config.defaultOn:true; stable group id `code-review`, inner id `code-review-step`. - Wire the default-on optional-group into both built-in coding IRs. - Fix store default-workflow seeding: interpreter-deferred built-ins (which carry optional-group nodes) previously bailed to `undefined` in materializeDefaultWorkflowSteps, dropping default-on group seeding under a project-default workflow. Now they seed resolveDefaultOnOptionalGroupIds, mirroring the explicit-workflow path, so defaultOn:true actually takes effect (the executor enables a group strictly via enabledWorkflowSteps.includes(node.id)). - Update tests + changeset; full @fusion/core suite green (356 files). Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/code-review-workflow-step.md | 4 +- .../builtin-code-review-group.test.ts | 114 ++++++++++++++++++ .../builtin-code-review-node.test.ts | 97 --------------- .../src/__tests__/builtin-workflows.test.ts | 49 +++++--- .../__tests__/workflow-optional-steps.test.ts | 24 +++- .../workflow-restart-durability.test.ts | 10 +- .../core/src/builtin-code-review-group.ts | 80 ++++++++++++ packages/core/src/builtin-code-review-node.ts | 57 --------- .../core/src/builtin-coding-workflow-ir.ts | 13 +- .../builtin-stepwise-coding-workflow-ir.ts | 11 +- packages/core/src/store.ts | 15 ++- 11 files changed, 280 insertions(+), 194 deletions(-) create mode 100644 packages/core/src/__tests__/builtin-code-review-group.test.ts delete mode 100644 packages/core/src/__tests__/builtin-code-review-node.test.ts create mode 100644 packages/core/src/builtin-code-review-group.ts delete mode 100644 packages/core/src/builtin-code-review-node.ts diff --git a/.changeset/code-review-workflow-step.md b/.changeset/code-review-workflow-step.md index ac9faa5c62..fd8acc1755 100644 --- a/.changeset/code-review-workflow-step.md +++ b/.changeset/code-review-workflow-step.md @@ -2,6 +2,6 @@ "@runfusion/fusion": minor --- -summary: Add a standard pre-merge Code Review step to the built-in coding workflows. +summary: Add a default-on, toggleable pre-merge Code Review step to the built-in coding workflows. category: feature -dev: New always-on `code-review` prompt node (toolMode readonly, gateMode advisory, phase pre-merge) on the pre-merge success path (execute → browser-verification → code-review → review) of both the built-in coding and stepwise coding workflows. Runs for every coding task by default with no `enabledWorkflowSteps` gating; advisory so it does not change merge outcomes (operators can promote it to a blocking gate). Reuses the shared prompt-gate verdict machinery (no engine verification code). The `code-review` WORKFLOW_STEP_TEMPLATE is also available in the editor palette. +dev: New `code-review` optional-group node (defaultOn:true, toolMode readonly, gateMode advisory, phase pre-merge) on the pre-merge success path (execute → browser-verification → code-review → review) of both the built-in coding and stepwise coding workflows. Runs for every coding task by default (seeded into enabledWorkflowSteps via resolveDefaultOnOptionalGroupIds) but is toggleable off per task; advisory so it does not change merge outcomes (operators can promote it to a blocking gate). Also fixes default-workflow task creation to seed default-on optional groups for interpreter-deferred built-ins (previously dropped). Reuses the shared prompt-gate verdict machinery (no engine verification code). The `code-review` WORKFLOW_STEP_TEMPLATE is also available in the editor palette. diff --git a/packages/core/src/__tests__/builtin-code-review-group.test.ts b/packages/core/src/__tests__/builtin-code-review-group.test.ts new file mode 100644 index 0000000000..5265daa935 --- /dev/null +++ b/packages/core/src/__tests__/builtin-code-review-group.test.ts @@ -0,0 +1,114 @@ +import { describe, expect, it } from "vitest"; +import { + CODE_REVIEW_GROUP_ID, + CODE_REVIEW_STEP_NODE_ID, + codeReviewOptionalGroupNode, +} from "../builtin-code-review-group.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; +import { WORKFLOW_STEP_TEMPLATES } from "../types.js"; +import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; +import { + resolveDefaultOnOptionalGroupIds, + resolveWorkflowOptionalSteps, +} from "../workflow-optional-steps.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-15:00: +Coverage for the DEFAULT-ON but TOGGLEABLE "Code Review" pre-merge step: the catalog +template fields, the `optional-group` node (defaultOn:true) built from it, and its wiring +into the coding + stepwise built-ins as a default-on optional group. Code review is a +WORKFLOW prompt-gate (shared verdict machinery), not engine verification code. +*/ + +describe("code-review WORKFLOW_STEP_TEMPLATE", () => { + const template = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + + it("exists with the expected catalog fields", () => { + expect(template).toBeTruthy(); + expect(template!.name).toBe("Code Review"); + expect(template!.toolMode).toBe("readonly"); + // Advisory → non-blocking by default (like the existing review); operators can promote. + expect(template!.gateMode).toBe("advisory"); + expect(template!.phase).toBe("pre-merge"); + expect(template!.description.length).toBeGreaterThan(0); + }); + + it("ends with the shared trailing verdict convention and reads the diff", () => { + const prompt = template!.prompt; + expect(prompt).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + expect(prompt).not.toContain('"verdict":"PASS"'); + expect(prompt).not.toContain('"verdict":"FAIL"'); + // Focused on the value tests miss + reads the diff against the base. + expect(prompt).toMatch(/git diff/); + expect(prompt).toMatch(/out of scope/i); + }); +}); + +describe("codeReviewOptionalGroupNode", () => { + it("builds a DEFAULT-ON optional-group with the stable group id and distinct inner id", () => { + const node = codeReviewOptionalGroupNode("in-progress"); + expect(node.id).toBe(CODE_REVIEW_GROUP_ID); + expect(CODE_REVIEW_GROUP_ID).toBe("code-review"); + expect(CODE_REVIEW_STEP_NODE_ID).toBe("code-review-step"); + expect(node.id).not.toBe(CODE_REVIEW_STEP_NODE_ID); // U1: inner id ≠ group id. + expect(node.kind).toBe("optional-group"); + expect(node.column).toBe("in-progress"); + expect(node.config?.name).toBe("Code Review"); + // Default-ON (runs by default), but still an optional-group → toggleable per task. + expect(node.config?.defaultOn).toBe(true); + + const template = node.config?.template as { nodes: { id: string; kind: string; config?: Record }[] }; + expect(template.nodes).toHaveLength(1); + const inner = template.nodes[0]; + expect(inner.id).toBe(CODE_REVIEW_STEP_NODE_ID); + expect(inner.kind).toBe("prompt"); + expect(inner.config?.toolMode).toBe("readonly"); + expect(inner.config?.gateMode).toBe("advisory"); + expect(String(inner.config?.prompt)).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); + }); +}); + +describe("built-in coding + stepwise workflows wire code-review as a default-ON optional group", () => { + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s includes the default-ON code-review optional-group and still parses/round-trips", (_name, ir) => { + const byId = new Map(ir.nodes.map((n) => [n.id, n])); + const group = byId.get("code-review"); + expect(group?.kind).toBe("optional-group"); + expect(group?.config?.name).toBe("Code Review"); + expect(group?.config?.defaultOn).toBe(true); + expect(group?.column).toBe("in-progress"); + + // Pre-merge wiring: ... → browser-verification → code-review → review; failure → end. + expect(ir.edges).toEqual( + expect.arrayContaining([ + expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), + expect.objectContaining({ from: "code-review", to: "end", condition: "failure" }), + ]), + ); + + // The built-in still compiles/validates with the new node (parse round-trips). + const reparsed = parseWorkflowIr(serializeWorkflowIr(ir)); + expect(reparsed).toEqual(parseWorkflowIr(ir)); + }); + + it.each([ + ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], + ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], + ])("%s: code-review is advertised as a toggle AND seeded into the default-on set", (_name, ir) => { + // Advertised as a toggleable optional step (so operators can turn it off per task)… + const advertised = resolveWorkflowOptionalSteps(ir).find((s) => s.templateId === "code-review"); + expect(advertised).toEqual({ + templateId: "code-review", + name: "Code Review", + description: "", + phase: "pre-merge", + defaultOn: true, + }); + // …and in the default-on set, so default-on actually takes effect (new tasks seed it). + expect(resolveDefaultOnOptionalGroupIds(ir)).toContain("code-review"); + }); +}); diff --git a/packages/core/src/__tests__/builtin-code-review-node.test.ts b/packages/core/src/__tests__/builtin-code-review-node.test.ts deleted file mode 100644 index 579745c264..0000000000 --- a/packages/core/src/__tests__/builtin-code-review-node.test.ts +++ /dev/null @@ -1,97 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { - CODE_REVIEW_NODE_ID, - codeReviewStepNode, -} from "../builtin-code-review-node.js"; -import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; -import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "../builtin-stepwise-coding-workflow-ir.js"; -import { WORKFLOW_STEP_TEMPLATES } from "../types.js"; -import { parseWorkflowIr, serializeWorkflowIr } from "../workflow-ir.js"; -import { resolveWorkflowOptionalSteps } from "../workflow-optional-steps.js"; - -/* -FNXC:CodeReviewStep 2026-06-25-13:30: -Coverage for the STANDARD, always-on "Code Review" pre-merge step: the catalog template -fields, the regular `prompt` node built from it, and its wiring as a default-ON step in -the coding + stepwise built-ins (no enabledWorkflowSteps gating). Code review is a -WORKFLOW prompt step (shared verdict machinery), not engine verification code. -*/ - -describe("code-review WORKFLOW_STEP_TEMPLATE", () => { - const template = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); - - it("exists with the expected catalog fields", () => { - expect(template).toBeTruthy(); - expect(template!.name).toBe("Code Review"); - expect(template!.toolMode).toBe("readonly"); - // Advisory → non-blocking by default (like the existing review); operators can promote. - expect(template!.gateMode).toBe("advisory"); - expect(template!.phase).toBe("pre-merge"); - expect(template!.description.length).toBeGreaterThan(0); - }); - - it("ends with the shared trailing verdict convention and reads the diff", () => { - const prompt = template!.prompt; - expect(prompt).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); - expect(prompt).not.toContain('"verdict":"PASS"'); - expect(prompt).not.toContain('"verdict":"FAIL"'); - // Focused on the value tests miss + reads the diff against the base. - expect(prompt).toMatch(/git diff/); - expect(prompt).toMatch(/out of scope/i); - }); -}); - -describe("codeReviewStepNode", () => { - it("builds a standard advisory readonly prompt node keyed by the catalog id", () => { - const node = codeReviewStepNode("in-progress"); - expect(node.id).toBe(CODE_REVIEW_NODE_ID); - expect(CODE_REVIEW_NODE_ID).toBe("code-review"); - // Standard node, NOT an optional-group toggle. - expect(node.kind).toBe("prompt"); - expect(node.column).toBe("in-progress"); - expect(node.config?.name).toBe("Code Review"); - expect(node.config?.toolMode).toBe("readonly"); - expect(node.config?.gateMode).toBe("advisory"); - expect(node.config?.defaultOn).toBeUndefined(); // no optional-group toggle semantics. - expect(String(node.config?.prompt)).toMatch(/"verdict":"APPROVE\|APPROVE_WITH_NOTES\|REVISE"/); - }); -}); - -describe("built-in coding + stepwise workflows wire code-review as a standard always-on step", () => { - it.each([ - ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], - ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], - ])("%s includes a default-ON code-review prompt node between browser-verification and review", (_name, ir) => { - const byId = new Map(ir.nodes.map((n) => [n.id, n])); - const node = byId.get("code-review"); - // Always present as a standard prompt node (not an optional-group, no toggle). - expect(node?.kind).toBe("prompt"); - expect(node?.config?.name).toBe("Code Review"); - expect(node?.config?.gateMode).toBe("advisory"); - expect(node?.config?.toolMode).toBe("readonly"); - - // Pre-merge wiring: ... → browser-verification → code-review → review; failure → end - // (mirrors how the existing review node fails to end — no dead-end). - expect(ir.edges).toEqual( - expect.arrayContaining([ - expect.objectContaining({ from: "browser-verification", to: "code-review", condition: "success" }), - expect.objectContaining({ from: "code-review", to: "review", condition: "success" }), - expect.objectContaining({ from: "code-review", to: "end", condition: "failure" }), - ]), - ); - - // The built-in still compiles/validates with the standard node (parse round-trips). - const reparsed = parseWorkflowIr(serializeWorkflowIr(ir)); - expect(reparsed).toEqual(parseWorkflowIr(ir)); - }); - - it.each([ - ["builtin coding", BUILTIN_CODING_WORKFLOW_IR], - ["builtin stepwise", BUILTIN_STEPWISE_CODING_WORKFLOW_IR], - ])("%s: code-review is NOT advertised as an optional-step toggle (always-on, no gating)", (_name, ir) => { - // Standard step → never surfaces in the optional-step toggle list (it is not gated - // on task.enabledWorkflowSteps; it runs for every coding task). - const toggles = resolveWorkflowOptionalSteps(ir).map((s) => s.templateId); - expect(toggles).not.toContain("code-review"); - }); -}); diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 01c1e04c25..87f3962dcc 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -600,12 +600,28 @@ describe("built-in workflows", () => { } }); - it("create-time branching built-in workflowId records selection without throwing", async () => { + it("create-time branching built-in workflowId records selection and seeds the default-on code-review group", async () => { const task = await store.createTask({ description: "explicit builtin coding", workflowId: "builtin:coding" }); const detail = await store.getTask(task.id); - expect(detail.enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:coding", stepIds: [] }); + // FNXC:CodeReviewStep — builtin:coding carries the DEFAULT-ON `code-review` + // optional-group, so the explicit-workflow create path seeds it into the task's + // enabledWorkflowSteps (and records it in the selection). + expect(detail.enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); + }); + + it("a task can disable code-review by creating with explicit enabledWorkflowSteps excluding it", async () => { + // Default-on but TOGGLEABLE: an explicit (non-empty) enabledWorkflowSteps wins over + // the workflow's default-on seeding, so omitting `code-review` disables it. + const task = await store.createTask({ + description: "coding without code review", + workflowId: "builtin:coding", + enabledWorkflowSteps: ["browser-verification"], + }); + const detail = await store.getTask(task.id); + expect(detail.enabledWorkflowSteps ?? []).not.toContain("code-review"); + expect(detail.enabledWorkflowSteps ?? []).toEqual(["browser-verification"]); }); it("branching built-in project defaults do not throw", async () => { @@ -613,34 +629,35 @@ describe("built-in workflows", () => { description: "implicit builtin default", }); - // U6: builtin:coding now carries the `browser-verification` optional-group - // (an interpreter-deferred construct), so its DEFAULT-workflow materialization - // falls back to no legacy WorkflowStep rows and records no selection row — - // identical to the stepwise built-in below. The group is defaultOn:false, so - // enabledWorkflowSteps stays empty. + // FNXC:CodeReviewStep — builtin:coding/stepwise are interpreter-deferred (they + // carry optional-group nodes), so DEFAULT-workflow materialization records no legacy + // WorkflowStep rows. They DO carry the DEFAULT-ON `code-review` optional-group, so + // the project-default create path now seeds `code-review` into enabledWorkflowSteps + // and records a selection (mirroring the explicit-workflow path) — that is how + // default-on actually takes effect. browser-verification stays off (defaultOn:false). await store.setDefaultWorkflowId("builtin:coding"); const codingTask = await store.createTask({ description: "default builtin coding" }); - expect((await store.getTask(codingTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(codingTask.id)).toBeUndefined(); + expect((await store.getTask(codingTask.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect(store.getTaskWorkflowSelection(codingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); const reservedCodingTask = await store.createTaskWithReservedId( { description: "reserved default builtin coding" }, { taskId: "reserved-default-builtin-coding" }, ); - expect((await store.getTask(reservedCodingTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toBeUndefined(); + expect((await store.getTask(reservedCodingTask.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); await store.setDefaultWorkflowId("builtin:stepwise-coding"); const stepwiseTask = await store.createTask({ description: "default builtin stepwise" }); - expect((await store.getTask(stepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(stepwiseTask.id)).toBeUndefined(); + expect((await store.getTask(stepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect(store.getTaskWorkflowSelection(stepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["code-review"] }); const reservedStepwiseTask = await store.createTaskWithReservedId( { description: "reserved default builtin stepwise" }, { taskId: "reserved-default-builtin-stepwise" }, ); - expect((await store.getTask(reservedStepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect(store.getTaskWorkflowSelection(reservedStepwiseTask.id)).toBeUndefined(); + expect((await store.getTask(reservedStepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect(store.getTaskWorkflowSelection(reservedStepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["code-review"] }); }); it("rejects selecting the PR lifecycle fragment for a task", async () => { diff --git a/packages/core/src/__tests__/workflow-optional-steps.test.ts b/packages/core/src/__tests__/workflow-optional-steps.test.ts index 2d72b029fa..941bce6495 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -108,12 +108,10 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { ]); }); - it("resolves the built-in coding/stepwise browser-verification optional-group (U6)", () => { - // U6 migrated both built-ins: `browser-verification` is now an optional-group - // node (default OFF), so the resolver advertises exactly one toggle entry per - // built-in, keyed by the group node id `browser-verification`. The standard - // always-on `code-review` prompt node is NOT an optional-group, so it never - // surfaces as a toggle entry here. + it("resolves the built-in coding/stepwise browser-verification (off) + code-review (on) optional-groups", () => { + // Both built-ins carry two optional-group toggles on the pre-merge path, in node order: + // `browser-verification` (default OFF) then `code-review` (default ON — runs by default + // but is toggleable off per task). const expected = [ { templateId: "browser-verification", @@ -122,10 +120,24 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { phase: "pre-merge" as const, defaultOn: false, }, + { + templateId: "code-review", + name: "Code Review", + description: "", + phase: "pre-merge" as const, + defaultOn: true, + }, ]; expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(expected); expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(expected); }); + + it("seeds code-review (default ON) but not browser-verification (default OFF) for the built-ins", () => { + // resolveDefaultOnOptionalGroupIds drives which groups a new task gets enabled by + // default: code-review is on, browser-verification is off. + expect(resolveDefaultOnOptionalGroupIds(BUILTIN_CODING_WORKFLOW_IR)).toEqual(["code-review"]); + expect(resolveDefaultOnOptionalGroupIds(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(["code-review"]); + }); }); describe("resolveDefaultOnOptionalGroupIds (task-creation seeding)", () => { diff --git a/packages/core/src/__tests__/workflow-restart-durability.test.ts b/packages/core/src/__tests__/workflow-restart-durability.test.ts index 79c2667acf..17c6bc454f 100644 --- a/packages/core/src/__tests__/workflow-restart-durability.test.ts +++ b/packages/core/src/__tests__/workflow-restart-durability.test.ts @@ -238,7 +238,9 @@ describe("workflow restart durability for explicit selections", () => { const customSelectionBefore = store().getTaskWorkflowSelection(customTask.id); expect(customSelectionBefore?.workflowId).toBe(workflow.id); expect(customSelectionBefore?.stepIds).toHaveLength(2); - expect(store().getTaskWorkflowSelection(builtinTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: [] }); + // FNXC:CodeReviewStep — builtin:coding carries the DEFAULT-ON `code-review` + // optional-group, so the create-time workflowId path seeds it into the selection. + expect(store().getTaskWorkflowSelection(builtinTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); await reopenAsDiskBackedStore(); @@ -249,9 +251,9 @@ describe("workflow restart durability for explicit selections", () => { for (const stepId of customSelection?.stepIds ?? []) { expect(await store().getWorkflowStep(stepId)).toBeDefined(); } - expect(store().getTaskWorkflowSelection(builtinTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: [] }); - expect((await store().getTask(builtinTask.id)).enabledWorkflowSteps ?? []).toEqual([]); - expect((await taskJsonEnabledWorkflowSteps(builtinTask.id)) ?? []).toEqual([]); + expect(store().getTaskWorkflowSelection(builtinTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); + expect((await store().getTask(builtinTask.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect((await taskJsonEnabledWorkflowSteps(builtinTask.id)) ?? []).toEqual(["code-review"]); }); it("fails closed when a selected custom workflow definition is missing without corrupting the dangling selection", async () => { diff --git a/packages/core/src/builtin-code-review-group.ts b/packages/core/src/builtin-code-review-group.ts new file mode 100644 index 0000000000..9aac33174f --- /dev/null +++ b/packages/core/src/builtin-code-review-group.ts @@ -0,0 +1,80 @@ +import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; + +/* +FNXC:CodeReviewStep 2026-06-25-15:00: +Code Review is a DEFAULT-ON but TOGGLEABLE step in the built-in coding and +stepwise-coding workflows: an `optional-group` container node with `defaultOn: true`. +It is part of the existing flows and runs for every coding task by default (the +default-on resolver seeds `code-review` into a new task's enabledWorkflowSteps), yet an +operator can turn it off per task by removing `code-review` from enabledWorkflowSteps — +when disabled the group passes through byte-inert, restoring the exact prior flow. + +The group sits on the pre-merge success path (execute → [browser-verification optional] +→ code-review → review). The group node id `code-review` is the STABLE per-task enable +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` WORKFLOW_STEP_TEMPLATE: a `prompt` node carrying the template's prompt, +`toolMode` (readonly — review reads the diff, never mutates), and `gateMode` (advisory — +non-blocking, like the existing review; operators can promote to a gate). Sourcing +prompt/toolMode/gateMode from the catalog keeps the built-in byte-identical to the +template a human would insert from the palette (KTD-5). +*/ + +function resolveCodeReviewTemplate() { + const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); + if (!tpl) { + throw new Error("code-review WORKFLOW_STEP_TEMPLATE is missing"); + } + return tpl; +} + +const CODE_REVIEW_TEMPLATE = resolveCodeReviewTemplate(); + +/** Stable per-task enable key + group node id. */ +export const CODE_REVIEW_GROUP_ID = "code-review"; + +/** Inner template node id — distinct from the group id (template-node-id collision rule, U1). */ +export const CODE_REVIEW_STEP_NODE_ID = "code-review-step"; + +/** + * Build the `code-review` optional-group node placed on a workflow's pre-merge path. + * `defaultOn: true` makes it run by default while remaining togglable per task. `column` + * 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 catalog prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. + */ +export function codeReviewOptionalGroupNode(column: string): WorkflowIrNode { + const tpl = CODE_REVIEW_TEMPLATE; + return { + id: CODE_REVIEW_GROUP_ID, + kind: "optional-group", + column, + config: { + name: tpl.name, + // Default-ON: runs for every coding task by default, but operators can toggle it + // off per task (remove `code-review` from enabledWorkflowSteps). + defaultOn: true, + template: { + nodes: [ + { + id: CODE_REVIEW_STEP_NODE_ID, + kind: "prompt", + config: { + name: tpl.name, + description: tpl.description, + prompt: tpl.prompt ?? "", + toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", + gateMode: tpl.gateMode ?? "advisory", + }, + }, + ], + edges: [], + }, + }, + }; +} diff --git a/packages/core/src/builtin-code-review-node.ts b/packages/core/src/builtin-code-review-node.ts deleted file mode 100644 index 426a8d6b53..0000000000 --- a/packages/core/src/builtin-code-review-node.ts +++ /dev/null @@ -1,57 +0,0 @@ -import type { WorkflowIrNode } from "./workflow-ir-types.js"; -import { WORKFLOW_STEP_TEMPLATES } from "./types.js"; - -/* -FNXC:CodeReviewStep 2026-06-25-13:30: -Code Review is a STANDARD, always-on step in the built-in coding and stepwise-coding -workflows — NOT a default-off optional-group toggle. It is a regular `prompt` node on the -pre-merge success path (execute → [browser-verification optional] → code-review → review), -so it runs for EVERY coding task by default with no `enabledWorkflowSteps` gating. - -gateMode is "advisory" (sourced from the catalog template): like the existing `review` -seam it does not change merge outcomes — it just adds the diff-correctness review to the -standard flow. Operators can promote it to a blocking gate later. toolMode is "readonly": -review reads the diff/files, it does not mutate the worktree. - -The node id `code-review` is also the catalog template id, so the built-in node stays -byte-identical to the `code-review` template a human would insert from the editor palette -(prompt/toolMode/gateMode all sourced from the catalog). -*/ - -function resolveCodeReviewTemplate() { - const tpl = WORKFLOW_STEP_TEMPLATES.find((t) => t.id === "code-review"); - if (!tpl) { - throw new Error("code-review WORKFLOW_STEP_TEMPLATE is missing"); - } - return tpl; -} - -const CODE_REVIEW_TEMPLATE = resolveCodeReviewTemplate(); - -/** Standard pre-merge code-review node id (also the catalog template id). */ -export const CODE_REVIEW_NODE_ID = "code-review"; - -/** - * Build the standard, always-on `code-review` prompt node placed on a workflow's - * pre-merge success path between browser-verification and review. `column` matches the - * pre-merge implementation column (`in-progress`) so the single in-progress → in-review - * status transition stays at code-review → review. - * - * Mirrors `stepTemplateToNode(code-review)`: a `prompt` node whose config carries the - * catalog prompt + `toolMode: "readonly"` + `gateMode: "advisory"`. - */ -export function codeReviewStepNode(column: string): WorkflowIrNode { - const tpl = CODE_REVIEW_TEMPLATE; - return { - id: CODE_REVIEW_NODE_ID, - kind: "prompt", - column, - config: { - name: tpl.name, - description: tpl.description, - prompt: tpl.prompt ?? "", - toolMode: tpl.toolMode === "coding" ? "coding" : "readonly", - gateMode: tpl.gateMode ?? "advisory", - }, - }; -} diff --git a/packages/core/src/builtin-coding-workflow-ir.ts b/packages/core/src/builtin-coding-workflow-ir.ts index 94ecbc7aed..741189fc0c 100644 --- a/packages/core/src/builtin-coding-workflow-ir.ts +++ b/packages/core/src/builtin-coding-workflow-ir.ts @@ -3,7 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; -import { codeReviewStepNode } from "./builtin-code-review-node.js"; +import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; /** * The built-in default workflow as a v2 IR. Its six columns have ids that are @@ -76,11 +76,12 @@ const RAW_BUILTIN_CODING_WORKFLOW_IR: WorkflowIr = { }, // Pre-merge optional browser-verification (optional-group, default OFF). browserVerificationOptionalGroupNode("in-progress"), - // FNXC:CodeReviewStep 2026-06-25-13:30: - // STANDARD always-on pre-merge Code Review prompt node (advisory), on the success - // path between browser-verification and review (execute → browser-verification → - // code-review → review). Runs for every coding task by default — no enable toggle. - codeReviewStepNode("in-progress"), + // FNXC:CodeReviewStep 2026-06-25-15:00: + // Pre-merge Code Review as a DEFAULT-ON optional-group (advisory), 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. + codeReviewOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index ae7d9525f7..b1c8724679 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -3,7 +3,7 @@ import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "./builtin-workflow-settings.js"; import { builtinPromptConfig } from "./builtin-workflow-prompts.js"; import { browserVerificationOptionalGroupNode } from "./builtin-browser-verification-group.js"; -import { codeReviewStepNode } from "./builtin-code-review-node.js"; +import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; /** * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step @@ -135,13 +135,14 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { // disabled the group passes through inert. Both the normal foreach-success path // and the rework-exhausted manual-release path flow through this node. browserVerificationOptionalGroupNode("in-progress"), - // FNXC:CodeReviewStep 2026-06-25-13:30: - // STANDARD always-on pre-merge Code Review prompt node (advisory), on the post-foreach + // FNXC:CodeReviewStep 2026-06-25-15:00: + // Pre-merge Code Review as a DEFAULT-ON optional-group (advisory), 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- - // release paths flow through it. Runs for every task by default — no enable toggle. - codeReviewStepNode("in-progress"), + // release paths flow through it. Runs for every task by default (defaultOn:true) but is + // toggleable off per task; disabled → byte-inert pass-through. + codeReviewOptionalGroupNode("in-progress"), { id: "review", kind: "prompt", column: "in-review", config: builtinPromptConfig("review", "Review") }, { id: "merge-gate", kind: "merge-gate", column: "in-review", config: { gate: "auto-merge" } }, { id: "merge-retry", kind: "retry-backoff", column: "in-review", config: { policy: "merge", maxAttempts: 3 } }, diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 56c7611f4f..9b4eeb08fd 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -16283,7 +16283,20 @@ ${stepsSection}`; try { inputs = compileWorkflowToSteps(def.ir); } catch (err) { - if (isBuiltinWorkflowId(workflowId) && isInterpreterDeferredWorkflowCompileError(err)) return undefined; + // FNXC:CodeReviewStep 2026-06-25-15:00: + // Interpreter-deferred built-ins (e.g. builtin:coding/stepwise, which carry + // optional-group nodes) cannot lower to legacy WorkflowStep rows, but they may + // still carry DEFAULT-ON optional groups (e.g. `code-review`) that must be seeded + // into the new task's `enabledWorkflowSteps` for default-on to actually take + // effect — the executor enables a group strictly via + // `enabledWorkflowSteps.includes(node.id)` with no defaultOn fallback. Mirror the + // explicit-workflow path (`materializeExplicitWorkflowSteps`) by recording a + // selection seeded with the default-on group ids instead of bailing to `undefined` + // (which dropped the seeding and silently disabled default-on groups under a + // project-default workflow). + if (isBuiltinWorkflowId(workflowId) && isInterpreterDeferredWorkflowCompileError(err)) { + return { workflowId, stepIds: resolveDefaultOnOptionalGroupIds(def.ir) }; + } throw err; } // FNXC:WorkflowOptionalGroup 2026-06-21-14:20: seed `enabledWorkflowSteps`