diff --git a/.changeset/stepwise-default-coding.md b/.changeset/stepwise-default-coding.md index c9c9ff7edf..8472cbbec3 100644 --- a/.changeset/stepwise-default-coding.md +++ b/.changeset/stepwise-default-coding.md @@ -2,6 +2,6 @@ "@runfusion/fusion": minor --- -summary: Make Coding use stepwise execution with final review and keep Legacy coding available. +summary: Make Coding use stepwise execution with default-on Plan Review and final Code Review gates. category: feature -dev: `builtin:coding` now uses the stepwise final-review graph; the old monolithic graph is `builtin:legacy-coding`. +dev: `builtin:coding` now uses the stepwise graph with `plan-review` before execution and no per-step or mandatory final review; the old graph is `builtin:legacy-coding`. diff --git a/docs/plans/2026-06-29-001-feat-stepwise-final-review-workflow-plan.md b/docs/plans/2026-06-29-001-feat-stepwise-final-review-workflow-plan.md index bc660e91f6..0becf85235 100644 --- a/docs/plans/2026-06-29-001-feat-stepwise-final-review-workflow-plan.md +++ b/docs/plans/2026-06-29-001-feat-stepwise-final-review-workflow-plan.md @@ -15,8 +15,8 @@ execution: code | Field | Value | |---|---| -| Objective | Make default Coding use stepwise execution with one final review, keep the original monolithic graph as Legacy coding, rename old Stepwise coding to Coding (per-step review), and fix workflow selection so new tasks actually attach to selected built-ins such as Coding (per-step review) and Compound engineering. | -| Authority | User requests in this session: "create a built in stepwise coding version that doesn't review each step but just does a review of everything at the end"; "creating a new task in stepwise coding don't attach it to the workflow. it goes to the default coding built in workflow"; "and the compound engineering workflow doesn't work." | +| Objective | Make default Coding use stepwise execution with default-on optional Plan Review before execution and default-on optional Code Review at the end, keep the original monolithic graph as Legacy coding, rename old Stepwise coding to Coding (per-step review), and fix workflow selection so new tasks actually attach to selected built-ins such as Coding (per-step review) and Compound engineering. | +| Authority | User requests in this session: "create a built in stepwise coding version that doesn't review each step but just does a review of everything at the end"; "in the new default stepwise the plan review should be an optional step also like code review"; "make sure plan review is also an optional step before a task moves from plan to execution"; "plan review should be default on"; "per step coding should get plan review also"; "creating a new task in stepwise coding don't attach it to the workflow. it goes to the default coding built in workflow"; "and the compound engineering workflow doesn't work." | | Execution profile | Standard code change across `@fusion/core`, dashboard task creation surfaces/routes, docs, and focused tests. | | Stop conditions | Do not change existing `builtin:stepwise-coding` per-step review semantics; do not mask workflow selection bugs by only changing display labels; do not alter runtime primitives unless validation proves a missing generic capability. | | Tail ownership | Implementation should add focused core tests and run file-scoped verification, not the full suite. | @@ -27,15 +27,15 @@ execution: code ### Summary -Fusion's default Coding workflow should use graph-owned step execution without the overhead of per-step AI review. The workflow should preserve the stepwise execution model (`PROMPT.md` parsing plus one-step-at-a-time execution) and then rely on the existing whole-task review path after all implementation steps complete. The original monolithic coding graph should remain available as Legacy coding, and the old per-step-review Stepwise workflow should display as Coding (per-step review). +Fusion's default Coding workflow should use graph-owned step execution without the overhead of per-step AI review. The workflow should preserve the stepwise execution model (`PROMPT.md` parsing plus one-step-at-a-time execution), run a default-on optional Plan Review before moving from planning to execution, and run exactly one end-of-work review surface through the default-on optional Code Review gate. The original monolithic coding graph should remain available as Legacy coding, and the old per-step-review Stepwise workflow should display as Coding (per-step review) while also gaining the same default-on optional Plan Review before execution. Workflow selection must also be reliable. When an operator creates a task from a workflow lane or picker, the selected workflow must persist onto the task's `task_workflow_selection` row and govern execution. The reported failures are that creating a new task in Stepwise coding falls back to `builtin:coding`, and the Compound engineering workflow does not work. Those are selection/materialization bugs, not just labeling issues. ### Problem Frame -`builtin:stepwise-coding` currently demonstrates step inversion by parsing `PROMPT.md`, iterating through `Task.steps[]`, running each step, and routing each step through `step-review` with approve/revise/rethink outcomes. That is useful when every step needs independent review, but it is heavier than needed for workflows that only want a final whole-task review. +`builtin:stepwise-coding` currently demonstrates step inversion by parsing `PROMPT.md`, iterating through `Task.steps[]`, running each step, and routing each step through `step-review` with approve/revise/rethink outcomes. That remains useful when every step needs independent review. It now also needs the same default-on optional Plan Review gate before execution as the new default Coding workflow. -The default `builtin:coding` already performs a whole-task execution seam followed by optional pre-merge gates and final review, but it does not expose per-step execution as authored workflow graph structure. The requested variant fills the middle: graph-owned step execution, no per-step review, final review after the full implementation. +The default `builtin:coding` already performs a whole-task execution seam followed by optional pre-merge gates and final review, but it does not expose per-step execution as authored workflow graph structure. The requested variant fills the middle: graph-owned step execution, default-on optional plan review before execution, no per-step review, and a single optional code review gate after the full implementation. During initial investigation, the store-level explicit/default workflow paths already have tests for default `builtin:stepwise-coding` selection seeding. That makes the likely failure surface the dashboard create-task path: the UI may display a selected lane while submitting `workflowId` as `undefined`, or it may submit optional-step state in a way that suppresses `input.workflowId` in `TaskStore.createTask`. Compound engineering adds another dimension because it is plugin-gated (`fusion-plugin-compound-engineering`) and can fail either at visibility/selection time or at runtime if the required bundled plugin/skills are unavailable. @@ -44,30 +44,32 @@ During initial investigation, the store-level explicit/default workflow paths al - R1. `builtin:coding` uses the Stepwise-derived final-review graph and remains the default coding workflow/fallback. - R2. The new workflow preserves stepwise planning and execution: planning produces `PROMPT.md`, a `parse-steps` node parses it, and a `foreach(source:"task-steps")` region runs one `step-execute` node per planned step. - R3. The new workflow must not include a `step-review` node in the foreach template and must not include per-step revise/rethink routing. -- R4. After all planned steps complete, the workflow runs the same pre-merge optional groups as the coding built-ins: `browser-verification` default off and `code-review` default on. -- R5. After optional pre-merge groups, the workflow runs the existing final `review` seam and the standard merge-gate / branch-group / merge-attempt region. -- R6. The existing `builtin:stepwise-coding` graph remains unchanged for users who rely on per-step review and rework, but its user-facing name becomes Coding (per-step review). -- R7. The original monolithic coding graph remains selectable as `builtin:legacy-coding` with user-facing name Legacy coding. -- R8. Creating a task while Coding (per-step review) is selected must persist `workflowId: "builtin:stepwise-coding"` and not silently resolve the task to `builtin:coding`. -- R9. Creating a task while Compound engineering is selected must either persist `workflowId: "builtin:compound-engineering"` and execute that workflow, or clearly block selection/create with a plugin-gating explanation when the required plugin is unavailable. It must not silently fall back to `builtin:coding`. -- R10. Task creation surfaces must preserve the distinction between `workflowId: undefined` (inherit project default), `workflowId: null` (explicit no workflow), and `workflowId: string` (explicit selected workflow). -- R11. Explicit enabled optional steps must not accidentally suppress an explicit workflow selection unless the caller intentionally opts into trusted low-level behavior. User-facing create flows must be able to submit both the selected workflow and enabled optional-group IDs. -- R12. Board workflow lane task creation, global New Task modal creation, list view creation, quick-entry creation, planning/subtask/mission task creation, and agent/tool-created tasks must be enumerated and tested according to the workflow selection contract. +- R4. `builtin:coding` and `builtin:stepwise-coding` include a `plan-review` optional group between `plan` and `parse-steps`; it is default-on and toggleable per task. +- R5. After all planned steps complete, `builtin:coding` runs the same pre-merge optional groups as the coding built-ins: `browser-verification` default off and `code-review` default on. +- R6. `builtin:coding` has no mandatory final `review` seam; the end-of-work review is controlled by the `code-review` optional group, and success/disabled pass-through routes to the merge gate. +- R7. The existing `builtin:stepwise-coding` graph keeps per-step review and rework for users who rely on that behavior, gains the default-on optional Plan Review before execution, and its user-facing name becomes Coding (per-step review). +- R8. The original monolithic coding graph remains selectable as `builtin:legacy-coding` with user-facing name Legacy coding. +- R9. Creating a task while Coding (per-step review) is selected must persist `workflowId: "builtin:stepwise-coding"` and not silently resolve the task to `builtin:coding`. +- R10. Creating a task while Compound engineering is selected must either persist `workflowId: "builtin:compound-engineering"` and execute that workflow, or clearly block selection/create with a plugin-gating explanation when the required plugin is unavailable. It must not silently fall back to `builtin:coding`. +- R11. Task creation surfaces must preserve the distinction between `workflowId: undefined` (inherit project default), `workflowId: null` (explicit no workflow), and `workflowId: string` (explicit selected workflow). +- R12. Explicit enabled optional steps must not accidentally suppress an explicit workflow selection unless the caller intentionally opts into trusted low-level behavior. User-facing create flows must be able to submit both the selected workflow and enabled optional-group IDs. +- R13. Board workflow lane task creation, global New Task modal creation, list view creation, quick-entry creation, planning/subtask/mission task creation, and agent/tool-created tasks must be enumerated and tested according to the workflow selection contract. ### Scope Boundaries -- In scope: a Stepwise-derived final-review IR module, built-in registry/export updates, docs catalog updates, and tests that prove default Coding has no per-step review node while final review remains. +- In scope: a Stepwise-derived default Coding IR module, built-in registry/export updates, docs catalog updates, and tests that prove default Coding has Plan Review before execution, no per-step review node, no mandatory final review seam, and only optional Code Review at the end. - In scope: diagnose and fix create-time workflow selection loss for Stepwise coding and Compound engineering across dashboard/API/store boundaries. - In scope: add regression coverage proving selected workflow IDs are persisted, returned, and used to resolve the task workflow IR. - Out of scope: runtime changes to `WorkflowGraphExecutor`, per-step parallelization, new reviewer verdict semantics, dashboard redesign, or changes to `builtin:coding` / `builtin:stepwise-coding` behavior. -- Out of scope: removing the default-on pre-merge `code-review` optional group. The request removes per-step review only; the whole-task pre-merge code review remains the "review of everything at the end" gate alongside the final `review` seam. +- Out of scope: removing the default-on pre-merge `code-review` optional group. The request removes per-step review and the mandatory final review seam from default Coding; the whole-task pre-merge code review remains the "review of everything at the end" gate. - Out of scope: redesigning plugin installation. This plan may add a clearer gating/error path for Compound engineering, but it should not rebuild the plugin manager. ### Acceptance Examples -- AE1. Given a task selects `builtin:coding`, when workflow selection resolves, then the built-in registry returns a v2 IR with `parse-steps`, `foreach`, `browser-verification`, `code-review`, `review`, and merge nodes. +- AE1. Given a task selects `builtin:coding`, when workflow selection resolves, then the built-in registry returns a v2 IR with `plan-review`, `parse-steps`, `foreach`, `browser-verification`, `code-review`, and merge nodes, and no `review` seam node. - AE2. Given the `builtin:coding` IR, when its foreach template is inspected, then it contains `step-execute` and a pass-through exit node, but no `step-review`. -- AE3. Given optional-step defaults are resolved for `builtin:coding`, then `code-review` is seeded by default and `browser-verification` is not. +- AE3. Given optional-step defaults are resolved for `builtin:coding`, then `plan-review` and `code-review` are seeded by default and `browser-verification` is not. +- AE3b. Given optional-step defaults are resolved for `builtin:stepwise-coding`, then `plan-review` and `code-review` are seeded by default and `browser-verification` is not. - AE4. Given docs list built-in workflows, then the new workflow is documented separately from `builtin:stepwise-coding`. - AE5. Given the board/list workflow selector is set to Stepwise coding, when a new task is created from that workflow context, then `store.getTaskWorkflowSelection(task.id)?.workflowId` is `builtin:stepwise-coding`. - AE6. Given the board/list workflow selector is set to Compound engineering and the required plugin is available/enabled, when a new task is created from that workflow context, then `store.getTaskWorkflowSelection(task.id)?.workflowId` is `builtin:compound-engineering`. @@ -78,7 +80,7 @@ During initial investigation, the store-level explicit/default workflow paths al - **Original symptom 1:** Creating a new task from/with Stepwise coding selected attaches the task to default `builtin:coding` instead of `builtin:stepwise-coding`. - **Exact reproduction 1:** In a project with built-in workflows enabled, select Stepwise coding in a task creation surface, create a task, then inspect `GET /api/tasks/:id/workflow` or `TaskStore.getTaskWorkflowSelection(task.id)`. -- **Assertion it is gone 1:** The created task's workflow selection row has `workflowId: "builtin:stepwise-coding"` and `enabledWorkflowSteps` is seeded according to that workflow (`["code-review"]` by default). +- **Assertion it is gone 1:** The created task's workflow selection row has `workflowId: "builtin:stepwise-coding"` and `enabledWorkflowSteps` is seeded according to that workflow (`["plan-review", "code-review"]` by default). - **Original symptom 2:** Compound engineering workflow "doesn't work". - **Exact reproduction 2:** Select/create a task with `workflowId: "builtin:compound-engineering"` through the same surfaces and inspect both selection persistence and the first workflow resolution/execution failure. - **Assertion it is gone 2:** With the CE plugin available/enabled, the task persists `workflowId: "builtin:compound-engineering"` and resolves to the CE IR; without the plugin, creation/selection fails visibly with the required plugin ID rather than silently falling back. @@ -86,7 +88,7 @@ During initial investigation, the store-level explicit/default workflow paths al ### Surface Enumeration - **Create surfaces:** New Task modal (`packages/dashboard/app/components/NewTaskModal.tsx`), TaskForm workflow picker (`packages/dashboard/app/components/TaskForm.tsx`), board/list workflow lane quick create (`packages/dashboard/app/components/Column.tsx`, `packages/dashboard/app/components/QuickEntryBox.tsx`, `packages/dashboard/app/components/ListView.tsx`), mission triage (`packages/dashboard/app/components/MissionManager.tsx` and `packages/dashboard/src/mission-routes.ts`), planning/subtask routes (`packages/dashboard/src/routes/register-planning-subtask-routes.ts`), and agent/API creation (`packages/dashboard/src/routes/register-task-workflow-routes.ts`, `packages/engine/src/agent-tools.ts`). -- **Workflow IDs:** `builtin:coding`, `builtin:stepwise-coding`, new final-review stepwise workflow, `builtin:compound-engineering`, custom workflow IDs, `null` no-workflow, and `undefined` inherit-default. +- **Workflow IDs:** `builtin:coding`, `builtin:legacy-coding`, `builtin:stepwise-coding`, `builtin:compound-engineering`, custom workflow IDs, `null` no-workflow, and `undefined` inherit-default. - **Plugin states:** Compound engineering plugin installed/enabled, installed/disabled, unavailable, and bundled path resolution failure. - **Optional-step states:** `enabledWorkflowSteps` omitted, empty array, default-on only, custom toggled values, and explicit values combined with non-default workflow ID. - **Board states:** task starts in default `triage`/workflow intake column, workflow-specific custom columns, and workflow lane selected independently from project default. @@ -104,22 +106,25 @@ During initial investigation, the store-level explicit/default workflow paths al - KTD-2. Model step completion as `step-execute -> step-done` inside the foreach template. The existing stepwise workflow uses `step-review` as the authority that marks a step done. Without per-step review, the foreach template should let `step-execute` complete the step through the existing step-execution primitive. The template still needs a single exit, so use a config-less `gate` node as the pass-through sink, mirroring the existing `step-done` pattern. -- KTD-3. Keep whole-task review surfaces unchanged. - The new workflow should keep `browser-verification`, `code-review`, final `review`, and the merge region after the foreach. This satisfies the requested "review of everything at the end" without introducing a new review mechanism. +- KTD-3. Make Plan Review a default-on optional gate before execution in both stepwise-style coding workflows. + `plan-review` belongs between `plan` and `parse-steps`, so the plan can be reviewed before planned steps are parsed into executable work. It is default-on and task-toggleable like `code-review`. -- KTD-4. Register as a normal selectable built-in. +- KTD-4. Make default Coding's end review controlled only by the optional Code Review gate. + The new default workflow keeps `browser-verification`, `code-review`, and the merge region after the foreach, but removes the mandatory final `review` seam. This satisfies the requested "review of everything at the end" while letting the operator turn it off via the `code-review` optional step. + +- KTD-5. Register as a normal selectable built-in. Add the new workflow to `BUILTIN_WORKFLOWS` with `kind: "workflow"` and let `defaultEnabledBuiltinWorkflowIds()` include it by default, matching non-plugin-gated selectable built-ins. -- KTD-5. Focus verification on IR shape and registry behavior. +- KTD-6. Focus verification on IR shape and registry behavior. This is primarily a built-in graph definition change. Targeted tests should validate parse/round-trip, registry presence, non-compilable built-in classification, optional-group defaults, docs-adjacent catalog expectations, and the exact absence of `step-review`. -- KTD-6. Fix workflow selection at the boundary where intent is lost. +- KTD-7. Fix workflow selection at the boundary where intent is lost. Store-level explicit workflow creation already records `task_workflow_selection` for built-ins, including Stepwise coding, when `workflowId` reaches `TaskStore.createTask` and `enabledWorkflowSteps` is omitted. The implementation must prove whether the lost value happens in the UI submit payload, API normalization, duplicate-reconcile response, or store precedence rule. The fix should be at that boundary, not by changing resolver fallback behavior. -- KTD-7. User-facing create flows must support workflow ID plus optional-step toggles together. +- KTD-8. User-facing create flows must support workflow ID plus optional-step toggles together. Current store logic intentionally treats explicit `enabledWorkflowSteps` as a trusted override that can suppress `input.workflowId`. That is dangerous for UI create flows because the workflow picker and optional-step toggles are independent controls. Either the UI/API must omit `enabledWorkflowSteps` unless the user explicitly changed them, or the store/API must preserve `workflowId` while applying explicit optional IDs for that selected workflow. The chosen fix must keep low-level backward compatibility explicit and tested. -- KTD-8. Compound engineering must fail closed on missing plugin requirements. +- KTD-9. Compound engineering must fail closed on missing plugin requirements. `builtin:compound-engineering` is plugin-gated by `fusion-plugin-compound-engineering`. A missing plugin should prevent selection/execution with a visible requirement, not degrade into default coding. Tests should cover both the persistence path and plugin-gated availability semantics. ### High-Level Technical Design @@ -127,14 +132,14 @@ During initial investigation, the store-level explicit/default workflow paths al ```mermaid flowchart TB Start[start] --> Plan[plan prompt] - Plan --> Parse[parse-steps: PROMPT.md] + Plan --> PlanReview[plan-review optional-group] + PlanReview --> Parse[parse-steps: PROMPT.md] Parse --> Steps[foreach task-steps] Steps --> StepExecute[step-execute] StepExecute --> StepDone[step-done gate] Steps --> Browser[browser-verification optional-group] Browser --> CodeReview[code-review optional-group] - CodeReview --> FinalReview[final review seam] - FinalReview --> MergeGate[merge-gate and merge region] + CodeReview --> MergeGate[merge-gate and merge region] MergeGate --> End[end] ``` @@ -150,8 +155,9 @@ flowchart TB ### Assumptions - The existing `step-execute` primitive marks the active step done on success when no `step-review` node is present. This is documented in `packages/engine/src/step-runner.ts` and should be confirmed with a focused graph test if implementation exposes uncertainty. -- The Stepwise-derived final-review graph is implemented as a reusable IR module but registered under `builtin:coding`; the original monolithic graph is registered as `builtin:legacy-coding`. -- Pre-merge `code-review` remains the default-on final whole-task code review gate. The final `review` seam remains the lifecycle review before merge. +- The Stepwise-derived graph is implemented as a reusable IR module but registered under `builtin:coding`; the original monolithic graph is registered as `builtin:legacy-coding`. +- `plan-review` and `code-review` are default-on optional groups for `builtin:coding` and `builtin:stepwise-coding`; `browser-verification` remains default-off. +- Pre-merge `code-review` is the only final whole-task review gate in default Coding. ### Sequencing @@ -175,7 +181,7 @@ flowchart TB - **Files:** - Create `packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts` - Modify `packages/core/src/index.ts` -- **Approach:** Copy the stable lifecycle skeleton from `packages/core/src/builtin-stepwise-coding-workflow-ir.ts`: same columns, `PROMPT.md` artifact declaration, planning/parse/foreach, optional groups, final review, merge region, and settings. Inside the foreach template, include only `step-execute` and `step-done` with a success edge. Remove per-step `step-review`, `outcome:revise`, `outcome:rethink`, and rework-hold routing unless validation requires a generic failure path. +- **Approach:** Copy the stable lifecycle skeleton from `packages/core/src/builtin-stepwise-coding-workflow-ir.ts`: same columns, `PROMPT.md` artifact declaration, planning/plan-review/parse/foreach, optional groups, merge region, and settings. Inside the foreach template, include only `step-execute` and `step-done` with a success edge. Remove per-step `step-review`, `outcome:revise`, `outcome:rethink`, rework-hold routing, and the mandatory final `review` seam. - **FNXC comment requirement:** Add or update a concise FNXC comment in the new IR file explaining that this built-in exists because operators need graph-owned step execution with one whole-task review at the end rather than per-step review. - **Test Scenarios:** - The IR parses and round-trips. @@ -198,7 +204,7 @@ flowchart TB - `getBuiltinWorkflow("builtin:stepwise-final-review-coding")` returns the new workflow. - `defaultEnabledBuiltinWorkflowIds()` includes the new ID because it is selectable and not plugin-gated. - `NON_COMPILABLE_BUILTIN_IDS` includes the new ID in compiler tests because the graph uses interpreter-only node kinds. - - Built-in registry tests distinguish existing `builtin:stepwise-coding` as per-step-review and the new workflow as final-review-only. + - Built-in registry tests distinguish existing `builtin:stepwise-coding` as per-step-review and default `builtin:coding` as final-code-review-only. - **Verification:** Extend `packages/core/src/__tests__/builtin-workflows.test.ts`. ### U3. Preserve optional-gate behavior for the new workflow @@ -212,7 +218,7 @@ flowchart TB - **Approach:** Expand test matrices that currently cover `BUILTIN_CODING_WORKFLOW_IR` and `BUILTIN_STEPWISE_CODING_WORKFLOW_IR` to include the new IR where the assertion is about shared coding optional gates. Keep per-step-review-specific assertions targeted only at `BUILTIN_STEPWISE_CODING_WORKFLOW_IR`. - **Test Scenarios:** - `resolveWorkflowOptionalSteps(newIr)` returns browser verification default off and code review default on. - - `resolveDefaultOnOptionalGroupIds(newIr)` returns `["code-review"]`. + - `resolveDefaultOnOptionalGroupIds(newIr)` returns `["plan-review", "code-review"]`. - The new workflow routes `browser-verification -> code-review -> review`. - Code review failure routes to `end` as in the other coding built-ins. - **Verification:** Run focused core tests listed in the Verification Contract. @@ -284,13 +290,13 @@ flowchart TB - Skill-backed CE nodes still request CE skills by both namespaced and bare forms. - **Verification:** Run focused core/route tests for CE gating and selection. -### U8. Add the new final-review workflow to the fixed creation matrix +### U8. Add the new default Coding workflow to the fixed creation matrix -- **Goal:** Ensure the new final-review stepwise workflow benefits from the same fixed creation path and appears in workflow selectors. +- **Goal:** Ensure the new default Stepwise-derived Coding workflow benefits from the same fixed creation path and appears in workflow selectors. - **Requirements:** R1, R7, R10, R11, R12 - **Files:** - Modify the tests added in U2, U3, U6, and U7 to include the new workflow ID once U1/U2 create it -- **Approach:** After the selection bug is fixed for existing Stepwise and Compound engineering workflows, include the new final-review workflow in the same create/select matrix so future built-ins do not regress. +- **Approach:** After the selection bug is fixed for existing Stepwise and Compound engineering workflows, include the new default Coding workflow in the same create/select matrix so future built-ins do not regress. - **Test Scenarios:** - Creating a task with the new workflow selected persists the new workflow ID. - Optional-step defaults seed `code-review` and do not erase the selected workflow. @@ -302,7 +308,7 @@ flowchart TB | Scope | Command | Proves | |---|---|---| -| Built-in registry and IR shape | `pnpm --filter @fusion/core exec vitest run src/__tests__/builtin-workflows.test.ts src/__tests__/builtin-coding-workflow-ir.test.ts --silent=passed-only --reporter=dot` | New workflow registers, parses, round-trips, and has final-review-only stepwise shape. | +| Built-in registry and IR shape | `pnpm --filter @fusion/core exec vitest run src/__tests__/builtin-workflows.test.ts src/__tests__/builtin-coding-workflow-ir.test.ts --silent=passed-only --reporter=dot` | New workflow registers, parses, round-trips, has Plan Review before execution, and has no per-step or mandatory final review in default Coding. | | Optional groups | `pnpm --filter @fusion/core exec vitest run src/__tests__/workflow-optional-steps.test.ts src/__tests__/builtin-code-review-group.test.ts --silent=passed-only --reporter=dot` | Browser verification and code review defaults/wiring match other coding built-ins. | | Stepwise create regression | `pnpm --filter @fusion/dashboard exec vitest run app/components/__tests__/TaskForm.test.tsx app/components/__tests__/NewTaskModal.test.tsx app/components/__tests__/ListView.test.tsx app/components/__tests__/QuickEntryBox.test.tsx app/components/__tests__/board-quickcreate-workflow-lane-visibility.test.tsx app/__tests__/api-tasks.test.ts src/routes/__tests__/task-create-workflow-route.test.ts --silent=passed-only --reporter=dot` | Selected workflow ID survives picker, quick-create, UI/task-create payload, and route boundaries and does not fall back to `builtin:coding`. | | Store workflow selection | `pnpm --filter @fusion/core exec vitest run src/__tests__/builtin-workflows.test.ts --silent=passed-only --reporter=dot` | `TaskStore.createTask` and reserved-ID creation persist explicit/default workflow selections correctly. | @@ -318,11 +324,11 @@ Do not run `pnpm test:full` or `pnpm verify:workspace` for this scoped change. U - The new workflow is available from `getBuiltinWorkflow` under a stable `builtin:` ID. - The new workflow executes planned steps through `parse-steps` and `foreach` without any `step-review` node. -- The new workflow keeps browser verification, code review, final review, and merge behavior aligned with existing coding built-ins. +- The new workflow keeps browser verification, code review, and merge behavior aligned with existing coding built-ins while removing the mandatory final review seam from default Coding. - Existing `builtin:stepwise-coding` behavior and tests remain intact. - Creating a task from Stepwise coding persists `builtin:stepwise-coding` and never silently falls back to `builtin:coding`. - Creating/selecting Compound engineering either persists `builtin:compound-engineering` when available or gives a clear plugin-gating error when unavailable. - Workflow creation tests cover UI/API/store surfaces where workflow intent can be lost. -- Docs describe the distinction between per-step-review stepwise coding and final-review-only stepwise coding. +- Docs describe the distinction between per-step-review stepwise coding and default Coding's final-code-review-only path. - A valid changeset exists for `@runfusion/fusion`. - Focused tests in the Verification Contract pass. diff --git a/docs/workflow-editor.md b/docs/workflow-editor.md index 02ea100535..c2ac92a64e 100644 --- a/docs/workflow-editor.md +++ b/docs/workflow-editor.md @@ -165,12 +165,12 @@ Save is blocked by client-side issues such as unplaced nodes and blocking column Fusion ships built-in workflows as read-only references: -- `builtin:coding` — the default Stepwise-based coding lifecycle: plan steps, execute them one at a time, then review and merge the full result. +- `builtin:coding` — the default Stepwise-based coding lifecycle: plan steps, execute them one at a time, then run the optional final Code Review gate and merge. - `builtin:legacy-coding` — the original monolithic coding lifecycle for tasks that should not use graph-owned step execution. - `builtin:quick-fix` — a short path for trivial or no-commit/decision work. - `builtin:review-heavy` — a standard execute/review/merge path with an additional gated security review. - `builtin:compound-engineering` — a plugin-gated Compound Engineering pipeline: `/ce-plan` writes the CE plan doc, optional `ce-doc-review` can pressure-test plans (markdown gets autofix/Open Questions write-back; HTML uses DOM-safe helper mutations, including canonical checklist repair, only when safety is proven and otherwise report-only with no write), `/ce-work` implements, `/ce-code-review` gates merge, and autoMerge-off projects route through the CE PR/feedback skills before Fusion's manual merge seam. -- `builtin:stepwise-coding` — Coding (per-step review): a graph variant that models per-step parse, execute, review, and rework structure. +- `builtin:stepwise-coding` — Coding (per-step review): a graph variant with optional Plan Review before execution and per-step parse, execute, review, and rework structure. - `builtin:design` — a UI-heavy work path with a gated design/UX review before standard review and merge. Built-ins can be viewed, exported, and used as templates, but their graph, columns, field declarations, and setting declarations are not editable. Their per-project setting **values** are editable from the Settings panel's Values tab. Selectable built-ins all use a capacity-released queue column (`todo` or a workflow-specific backlog) that dispatches to the active WIP column through the standard hold/release sweep. diff --git a/docs/workflow-steps.md b/docs/workflow-steps.md index ef4337dfe0..6a93540f73 100644 --- a/docs/workflow-steps.md +++ b/docs/workflow-steps.md @@ -44,13 +44,13 @@ Decision-only or investigation tasks can also declare `noCommitsExpected` / `**N | Workflow | ID | Notes | |---|---|---| -| Coding | `builtin:coding` | Default Stepwise-based coding lifecycle: plan steps, execute them one at a time, then review and merge the full result. | +| Coding | `builtin:coding` | Default Stepwise-based coding lifecycle: plan steps, execute them one at a time, then run the optional final Code Review gate and merge. | | Legacy coding | `builtin:legacy-coding` | Original monolithic coding lifecycle for tasks that should not use graph-owned step execution. | | Quick fix | `builtin:quick-fix` | Short path for trivial or no-commit/decision work; omits the standard review stage. | | Review-heavy | `builtin:review-heavy` | Standard execute/review/merge path with an additional gated security review. | | Marketing | `builtin:marketing` | Content pipeline with custom Ideation, Backlog, Drafting, Editorial review, Published, and Archived columns plus structured marketing brief/draft/editorial prompts; drafts are persisted as task documents for review while the workflow reuses standard lifecycle traits and merge primitives. | | Compound engineering | `builtin:compound-engineering` | Plugin-gated CE workflow that invokes `/ce-plan`, optional advisory `ce-doc-review` (markdown autofix; HTML DOM-safe mutation with report-only fallback), `/ce-work`, merge-blocking `/ce-code-review`, CE PR/feedback skills, Fusion merge, and learnings capture. | -| Coding (per-step review) | `builtin:stepwise-coding` | Graph-executor workflow that models per-step parse/execute/review/rework explicitly. | +| Coding (per-step review) | `builtin:stepwise-coding` | Graph-executor workflow with default-on Plan Review before execution and per-step parse/execute/review/rework. | | Design | `builtin:design` | UI-heavy work path that implements, persists a user-facing design preview task document, runs a gated design/UX review, then performs the standard review and merge. | | PR lifecycle | `builtin:pr-workflow` | Reusable PR lifecycle graph fragment (create PR → await review → respond → gate → merge); it is a fragment, not directly selectable as a task workflow. | | Lead generation | `builtin:lead-generation` | Selectable business workflow for sourcing, qualifying, enriching, and contacting leads with custom lead fields, stage columns, and reviewable enrichment/outreach task documents; requires the workflow graph executor for custom board columns. | @@ -178,13 +178,13 @@ The workflow runtime is the authoritative execution path for task lifecycle work The engine remains the substrate for scheduler dispatch, routing claims, persistence, concurrency limits, process supervision, storage, and audit plumbing. Lifecycle policy belongs in built-in or custom workflows. -The default built-in catalog entry `builtin:coding` is backed by a Stepwise-derived final-review graph. It is the resolver/runtime fallback for tasks with no workflow selection or an explicit default selection. Missing/corrupt explicit custom selections fail closed as workflow-resolution failures instead of silently running the default. The built-in IR parses planned steps, executes them sequentially without per-step review, then runs the whole-task review and merge region: +The default built-in catalog entry `builtin:coding` is backed by a Stepwise-derived graph with two default-on, toggleable review gates: `plan-review` before execution and `code-review` at the end of implementation. It is the resolver/runtime fallback for tasks with no workflow selection or an explicit default selection. Missing/corrupt explicit custom selections fail closed as workflow-resolution failures instead of silently running the default. The built-in IR parses planned steps, executes them sequentially without per-step review, then routes optional quality gates into the merge region: -- `triage` → `plan` → `parse-steps` → `foreach(step-execute)` → `optional-group` quality gates → `review` → `merge-gate` / branch-group integration / `merge-attempt` / retry or manual hold → `end` +- `triage` → `plan` → `plan-review` (default-on optional plan review) → `parse-steps` → `foreach(step-execute)` → `browser-verification` (optional) → `code-review` (default-on optional final review) → `merge-gate` / branch-group integration / `merge-attempt` / retry or manual hold → `end` `builtin:legacy-coding` is backed by the original monolithic `BUILTIN_CODING_WORKFLOW_IR`: `planning` → `execute` → optional quality gates → `review` → merge region. -`builtin:stepwise-coding` displays as Coding (per-step review). It is backed by `BUILTIN_STEPWISE_CODING_WORKFLOW_IR`; it keeps the same lifecycle columns/traits while modeling per-step parse/execute/review/rework as authored graph structure. +`builtin:stepwise-coding` displays as Coding (per-step review). It is backed by `BUILTIN_STEPWISE_CODING_WORKFLOW_IR`; it keeps the same lifecycle columns/traits while adding the default-on optional Plan Review before `parse-steps` and modeling per-step parse/execute/review/rework as authored graph structure. `builtin:marketing` is a non-coding content workflow with marketing-specific columns (`ideation`, `backlog`, `drafting`, `editorial-review`, `published`, `archived`) and prompt seams for content brief, draft, and editorial review. Its draft stage saves the primary content deliverable as a task document for human review, while the workflow uses the same lifecycle traits (`intake`, `hold`, `wip`, `merge-blocker`, `human-review`, `complete`, `archived`) and the same merge-gate/branch-group/merge-attempt primitive region as coding workflows, so scheduler, capacity, review blocking, and merge orchestration behavior remain standard. @@ -345,7 +345,8 @@ Node config (`WorkflowOptionalGroupConfig`): `{ name?, defaultOn?, maxRevisions? Built-in optional gates ship as inlined IR builders, not as a template catalog: - `builtin:coding` carries the `browser-verification` optional-group node (`builtin-browser-verification-group.ts`), default-off, so browser verification runs only for tasks whose `enabledWorkflowSteps` includes `browser-verification`. -- The `code-review` optional-group node (`builtin-code-review-group.ts`) is the inlined code-review gate. +- `builtin:coding` and `builtin:stepwise-coding` carry the `plan-review` optional-group node (`builtin-plan-review-group.ts`), default-on, before `parse-steps` so the plan can be reviewed before execution begins. +- The `code-review` optional-group node (`builtin-code-review-group.ts`) is the inlined code-review gate. On default `builtin:coding`, this is the only final review surface before merge; disabling it lets the graph continue from implementation/verification to the merge gate. - A workflow (for example compound-engineering) can add a **post-merge** optional-group node via the generic `postMergeOptionalGroupNode(...)` builder (`builtin-post-merge-group.ts`) — e.g. a `document` step that runs after merge. Create-time optional-step controls appear in the quick-add action row and the **New Task** dialog inline quick buttons for the active workflow. They resolve the workflow's optional-group nodes (plus plugin-contributed palette templates, see [Plugin-Contributed Steps](#plugin-contributed-steps)) into toggleable rows. Workflows with no optional groups render no trigger, and the selected node ids are submitted through `enabledWorkflowSteps` when the task is created. Unknown or removed ids are skipped during resolution so stale selections never render blank controls or break workflow loading. @@ -426,6 +427,7 @@ FN-7039 (U6) DELETED the `WORKFLOW_STEP_TEMPLATES` built-in catalog array (the f The built-in quality gates ship as inlined `optional-group` node builders in `@fusion/core`, not as a template catalog (the former `WORKFLOW_STEP_TEMPLATES` array was removed): - **Browser Verification** (`browser-verification`, `builtin-browser-verification-group.ts`) — browser-automation-style checks for UI validation flows; an optional-group node on `builtin:coding`, `builtin:legacy-coding`, and `builtin:stepwise-coding`. +- **Plan Review** (`plan-review`, `builtin-plan-review-group.ts`) — default-on plan readiness review before `builtin:coding` or `builtin:stepwise-coding` moves from planning to step parsing/execution. - **Code Review** (`code-review`, `builtin-code-review-group.ts`) — the inlined code-review gate. The Browser Verification inner prompt node carries `requiresBrowser: true` while keeping `toolMode: "coding"`. When that step runs, the executor best-effort adds the `agent-browser-navigation` skill (when the agent-browser plugin is installed), runs a bounded non-fatal `agent-browser --version` preflight, and writes start, availability, and finish entries into both the task log and the task's agent log. A missing or timed-out `agent-browser` binary is logged as an actionable warning rather than failing the step solely because of the preflight; the prompt can still fast-bail or report a normal verification failure. Because Bash tool events are already streamed to the agent log, `agent-browser open ...`, `agent-browser snapshot ...`, and related commands appear as the browser-verification activity the step performed. diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 0d79068420..20e5f8dff5 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -10,6 +10,7 @@ import { } from "../builtin-workflows.js"; import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; import { BROWSER_VERIFICATION_GROUP_ID, BROWSER_VERIFICATION_STEP_NODE_ID } from "../builtin-browser-verification-group.js"; +import { PLAN_REVIEW_GROUP_ID, PLAN_REVIEW_STEP_NODE_ID } from "../builtin-plan-review-group.js"; import { builtinPromptConfig, BUILTIN_SEAM_PROMPTS } from "../builtin-workflow-prompts.js"; import { BUILTIN_WORKFLOW_SETTINGS } from "../builtin-workflow-settings.js"; import { resolveColumnFlags } from "../trait-registry.js"; @@ -32,6 +33,12 @@ function browserVerificationInnerConfig(ir: { nodes: Array<{ id: string; kind: s return template?.nodes?.find((node) => node.id === BROWSER_VERIFICATION_STEP_NODE_ID)?.config ?? {}; } +function planReviewInnerConfig(ir: { nodes: Array<{ id: string; kind: string; config?: Record }> }): Record { + const group = ir.nodes.find((node) => node.id === PLAN_REVIEW_GROUP_ID); + const template = group?.config?.template as { nodes?: Array<{ id: string; config?: Record }> } | undefined; + return template?.nodes?.find((node) => node.id === PLAN_REVIEW_STEP_NODE_ID)?.config ?? {}; +} + function columnTraitMatrix(ir: { columns: Array<{ id: string; traits: Array<{ trait: string; config?: unknown }> }> }): Array<{ id: string; traits: Array<{ trait: string; config?: unknown }>; @@ -76,6 +83,9 @@ describe("built-in workflows", () => { if (ir.version !== "v2") throw new Error("expected v2"); // The chain: a parse-steps node dominating a foreach with a step-review template. expect(ir.nodes.some((n) => n.kind === "parse-steps")).toBe(true); + expect(ir.nodes.some((n) => n.id === "plan-review" && n.kind === "optional-group")).toBe(true); + expect(ir.edges.some((edge) => edge.from === "plan" && edge.to === "plan-review")).toBe(true); + expect(ir.edges.some((edge) => edge.from === "plan-review" && edge.to === "parse")).toBe(true); const foreach = ir.nodes.find((n) => n.kind === "foreach"); expect(foreach).toBeDefined(); const template = ( @@ -94,9 +104,13 @@ describe("built-in workflows", () => { expect(ir.nodes.some((node) => node.kind === "parse-steps")).toBe(true); expect(ir.nodes.map((node) => node.id)).toEqual( - expect.arrayContaining(["plan", "parse", "steps", "browser-verification", "code-review", "review", "merge-gate", "merge-attempt"]), + expect.arrayContaining(["plan", "plan-review", "parse", "steps", "browser-verification", "code-review", "merge-gate", "merge-attempt"]), ); expect(ir.nodes.some((node) => node.id === "rework-hold")).toBe(false); + expect(ir.nodes.some((node) => node.id === "review")).toBe(false); + expect(ir.edges.some((edge) => edge.from === "plan" && edge.to === "plan-review" && edge.condition === "success")).toBe(true); + expect(ir.edges.some((edge) => edge.from === "plan-review" && edge.to === "parse" && edge.condition === "success")).toBe(true); + expect(ir.edges.some((edge) => edge.from === "code-review" && edge.to === "merge-gate" && edge.condition === "success")).toBe(true); const foreach = ir.nodes.find((node) => node.kind === "foreach"); expect(foreach).toBeDefined(); @@ -180,7 +194,7 @@ describe("built-in workflows", () => { expect(coding).toBeDefined(); expect(coding!.id).toBe("builtin:coding"); expect(coding!.name).toBe("Coding (built-in)"); - expect(coding!.description).toContain("execute them one at a time"); + expect(coding!.description).toContain("optional final code review"); expect(coding!.kind).toBe("workflow"); expect(coding!.createdAt).toBe("2026-01-01T00:00:00.000Z"); expect(coding!.updatedAt).toBe("2026-01-01T00:00:00.000Z"); @@ -332,6 +346,12 @@ describe("built-in workflows", () => { const byId = new Map(ir.nodes.map((node) => [node.id, node])); expect(byId.get("plan")?.column).toBe("in-progress"); + expect(byId.get("plan-review")?.kind).toBe("optional-group"); + expect(byId.get("plan-review")?.column).toBe("in-progress"); + expect(planReviewInnerConfig(ir)).toMatchObject({ + toolMode: "readonly", + gateMode: "advisory", + }); expect(byId.get("parse")?.column).toBe("in-progress"); expect(byId.get("steps")?.column).toBe("in-progress"); // U6: the legacy `workflow-step` seam is replaced by the pre-merge @@ -344,7 +364,7 @@ describe("built-in workflows", () => { gateMode: "advisory", requiresBrowser: true, }); - expect(byId.get("review")?.column).toBe("in-review"); + expect(byId.get("review")).toBeUndefined(); // Merge is the native primitive region (FN-6035), placed in in-review. expect(byId.get("merge")).toBeUndefined(); expect(byId.get("merge-gate")?.column).toBe("in-review"); @@ -686,11 +706,15 @@ describe("built-in workflows", () => { expect(ce.ir.edges.some((edge) => edge.from === "code-review" && edge.to === "commit-pr")).toBe(true); }); - it("other built-in workflows retain their generic review nodes", () => { + it("non-default coding built-ins retain their generic review nodes", () => { const coding = getBuiltinWorkflow("builtin:coding")!; + const legacy = getBuiltinWorkflow("builtin:legacy-coding")!; + const stepwise = getBuiltinWorkflow("builtin:stepwise-coding")!; const reviewHeavy = getBuiltinWorkflow("builtin:review-heavy")!; - expect(coding.ir.nodes.some((node) => node.id === "review" && node.config?.seam === "review")).toBe(true); + expect(coding.ir.nodes.some((node) => node.id === "review" && node.config?.seam === "review")).toBe(false); + expect(legacy.ir.nodes.some((node) => node.id === "review" && node.config?.seam === "review")).toBe(true); + expect(stepwise.ir.nodes.some((node) => node.id === "review" && node.config?.seam === "review")).toBe(true); expect(reviewHeavy.ir.nodes.some((node) => node.id === "review" && node.config?.seam === "review")).toBe(true); }); @@ -788,13 +812,14 @@ describe("built-in workflows", () => { const coding = getBuiltinWorkflow("builtin:coding"); const plan = coding?.ir.nodes.find((node) => node.id === "plan"); const steps = coding?.ir.nodes.find((node) => node.id === "steps"); - const review = coding?.ir.nodes.find((node) => node.id === "review"); + const codeReview = coding?.ir.nodes.find((node) => node.id === "code-review"); const legacy = getBuiltinWorkflow("builtin:legacy-coding"); const legacyExecute = legacy?.ir.nodes.find((node) => node.id === "execute"); expect((plan?.config as { prompt?: string } | undefined)?.prompt).toContain("You are a task specification agent"); expect(steps?.kind).toBe("foreach"); - expect((review?.config as { prompt?: string } | undefined)?.prompt).toContain("You are an independent code and plan reviewer"); + expect(codeReview?.kind).toBe("optional-group"); + expect(coding?.ir.edges.some((edge) => edge.from === "code-review" && edge.to === "merge-gate")).toBe(true); expect((legacyExecute?.config as { prompt?: string } | undefined)?.prompt).toContain("You are a task execution agent"); // No `merge` seam node post-FN-6035 — merge runs as native primitives. expect(coding?.ir.nodes.find((node) => node.id === "merge")).toBeUndefined(); @@ -811,13 +836,14 @@ describe("built-in workflows", () => { // FNXC:WorkflowStepCRUD 2026-06-26-14:00: U7c — `selectTaskWorkflow` no longer // materializes legacy `workflow_steps` rows; it seeds `enabledWorkflowSteps` with the // workflow's DEFAULT-ON optional-group node ids, exactly matching the create-time path - // (a task that SELECTS builtin:coding now enables `code-review` just like one CREATED - // with builtin:coding — previously select returned [] and silently skipped the gate). + // (a task that SELECTS builtin:coding now enables default-on optional groups just + // like one CREATED with builtin:coding — previously select returned [] and silently + // skipped the gate). const expectedGroups: Record = { - "builtin:coding": ["code-review"], + "builtin:coding": ["plan-review", "code-review"], "builtin:legacy-coding": ["code-review"], "builtin:marketing": [], - "builtin:stepwise-coding": ["code-review"], + "builtin:stepwise-coding": ["plan-review", "code-review"], }; for (const workflowId of ["builtin:coding", "builtin:legacy-coding", "builtin:marketing", "builtin:stepwise-coding"]) { const task = await store.createTask({ description: `select ${workflowId}`, enabledWorkflowSteps: [] }); @@ -831,15 +857,15 @@ describe("built-in workflows", () => { } }); - it("create-time branching built-in workflowId records selection and seeds the default-on code-review group", async () => { + it("create-time branching built-in workflowId records selection and seeds the default-on review groups", async () => { const task = await store.createTask({ description: "explicit builtin coding", workflowId: "builtin:coding" }); const detail = await store.getTask(task.id); - // 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"] }); + // FNXC:PlanReviewStep/FNXC:CodeReviewStep — builtin:coding carries DEFAULT-ON + // `plan-review` and `code-review` optional groups, so the explicit-workflow + // create path seeds them into the task's enabledWorkflowSteps. + expect(detail.enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); + expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["plan-review", "code-review"] }); }); it("a task can disable code-review by creating with explicit enabledWorkflowSteps excluding it", async () => { @@ -850,14 +876,14 @@ describe("built-in workflows", () => { const task = await store.createTask({ description: "coding without code review", workflowId: "builtin:coding", - enabledWorkflowSteps: ["browser-verification"], + enabledWorkflowSteps: ["plan-review", "browser-verification"], }); const detail = await store.getTask(task.id); expect(detail.enabledWorkflowSteps ?? []).not.toContain("code-review"); - expect(detail.enabledWorkflowSteps ?? []).toEqual(["browser-verification"]); + expect(detail.enabledWorkflowSteps ?? []).toEqual(["plan-review", "browser-verification"]); expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:coding", - stepIds: ["browser-verification"], + stepIds: ["plan-review", "browser-verification"], }); }); @@ -865,13 +891,13 @@ describe("built-in workflows", () => { const task = await store.createTask({ description: "stepwise with toggles", workflowId: "builtin:stepwise-coding", - enabledWorkflowSteps: ["code-review"], + enabledWorkflowSteps: ["plan-review", "code-review"], }); - expect((await store.getTask(task.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect((await store.getTask(task.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:stepwise-coding", - stepIds: ["code-review"], + stepIds: ["plan-review", "code-review"], }); }); @@ -894,15 +920,15 @@ describe("built-in workflows", () => { { description: "reserved stepwise with toggles", workflowId: "builtin:stepwise-coding", - enabledWorkflowSteps: ["code-review"], + enabledWorkflowSteps: ["plan-review", "code-review"], }, { taskId: "reserved-stepwise-with-toggles" }, ); - expect((await store.getTask(task.id)).enabledWorkflowSteps ?? []).toEqual(["code-review"]); + expect((await store.getTask(task.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); expect(store.getTaskWorkflowSelection(task.id)).toEqual({ workflowId: "builtin:stepwise-coding", - stepIds: ["code-review"], + stepIds: ["plan-review", "code-review"], }); }); @@ -911,35 +937,34 @@ describe("built-in workflows", () => { description: "implicit builtin default", }); - // FNXC:CodeReviewStep — builtin:coding/stepwise are interpreter-deferred (they + // FNXC:PlanReviewStep/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). + // WorkflowStep rows. They DO carry DEFAULT-ON optional-group ids, so the project-default + // create path now seeds those ids into enabledWorkflowSteps and records a selection + // (mirroring the explicit-workflow path). 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(["code-review"]); - expect(store.getTaskWorkflowSelection(codingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); + expect((await store.getTask(codingTask.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); + expect(store.getTaskWorkflowSelection(codingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["plan-review", "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(["code-review"]); - expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["code-review"] }); + expect((await store.getTask(reservedCodingTask.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); + expect(store.getTaskWorkflowSelection(reservedCodingTask.id)).toEqual({ workflowId: "builtin:coding", stepIds: ["plan-review", "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(["code-review"]); - expect(store.getTaskWorkflowSelection(stepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["code-review"] }); + expect((await store.getTask(stepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); + expect(store.getTaskWorkflowSelection(stepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["plan-review", "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(["code-review"]); - expect(store.getTaskWorkflowSelection(reservedStepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["code-review"] }); + expect((await store.getTask(reservedStepwiseTask.id)).enabledWorkflowSteps ?? []).toEqual(["plan-review", "code-review"]); + expect(store.getTaskWorkflowSelection(reservedStepwiseTask.id)).toEqual({ workflowId: "builtin:stepwise-coding", stepIds: ["plan-review", "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 398cfd7880..8fd535dbad 100644 --- a/packages/core/src/__tests__/workflow-optional-steps.test.ts +++ b/packages/core/src/__tests__/workflow-optional-steps.test.ts @@ -109,11 +109,11 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { ]); }); - 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: + it("resolves the built-in coding/stepwise optional-groups in node order", () => { + // Legacy coding carries two optional-group toggles on the pre-merge path: // `browser-verification` (default OFF) then `code-review` (default ON — runs by default // but is toggleable off per task). - const expected = [ + const legacyExpected = [ { templateId: "browser-verification", name: "Browser Verification", @@ -129,17 +129,27 @@ describe("resolveWorkflowOptionalSteps (optional-group nodes)", () => { defaultOn: true, }, ]; - expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(expected); - expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(expected); - expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR)).toEqual(expected); + const stepwiseExpected = [ + { + templateId: "plan-review", + name: "Plan Review", + description: "", + phase: "pre-merge" as const, + defaultOn: true, + }, + ...legacyExpected, + ]; + expect(resolveWorkflowOptionalSteps(BUILTIN_CODING_WORKFLOW_IR)).toEqual(legacyExpected); + expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(stepwiseExpected); + expect(resolveWorkflowOptionalSteps(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR)).toEqual(stepwiseExpected); }); - it("seeds code-review (default ON) but not browser-verification (default OFF) for the built-ins", () => { + it("seeds default-on optional groups but not browser-verification for the built-ins", () => { // resolveDefaultOnOptionalGroupIds drives which groups a new task gets enabled by - // default: code-review is on, browser-verification is off. + // default: review groups are on, browser-verification is off. expect(resolveDefaultOnOptionalGroupIds(BUILTIN_CODING_WORKFLOW_IR)).toEqual(["code-review"]); - expect(resolveDefaultOnOptionalGroupIds(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(["code-review"]); - expect(resolveDefaultOnOptionalGroupIds(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR)).toEqual(["code-review"]); + expect(resolveDefaultOnOptionalGroupIds(BUILTIN_STEPWISE_CODING_WORKFLOW_IR)).toEqual(["plan-review", "code-review"]); + expect(resolveDefaultOnOptionalGroupIds(BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR)).toEqual(["plan-review", "code-review"]); }); }); diff --git a/packages/core/src/builtin-plan-review-group.ts b/packages/core/src/builtin-plan-review-group.ts new file mode 100644 index 0000000000..42db64554f --- /dev/null +++ b/packages/core/src/builtin-plan-review-group.ts @@ -0,0 +1,68 @@ +import type { WorkflowIrNode } from "./workflow-ir-types.js"; + +/* +FNXC:PlanReviewStep 2026-06-28-23:29: +The default Coding workflow needs an optional plan review before a task crosses from planning into execution. Model it as a DEFAULT-ON `optional-group` so operators get the same per-task toggle semantics as Code Review: enabled tasks review PROMPT.md before `parse-steps`, and disabled tasks pass through directly to execution. +*/ + +/** Stable per-task enable key + group node id. */ +export const PLAN_REVIEW_GROUP_ID = "plan-review"; + +/** Inner template node id — distinct from the group id (template-node-id collision rule, U1). */ +export const PLAN_REVIEW_STEP_NODE_ID = "plan-review-step"; + +const PLAN_REVIEW_NAME = "Plan Review"; + +const PLAN_REVIEW_DESCRIPTION = + "Review the task plan before execution for missing requirements, unsafe scope, and unclear implementation steps"; + +const PLAN_REVIEW_PROMPT = `You are a senior plan reviewer. Review the task's PROMPT.md before implementation starts. + +## Step 1: Read the plan +1. Read PROMPT.md and any task context the plan cites. +2. Confirm the plan captures the user's current requirements, expected workflow, file scope, and verification path. + +## Review focus +1. **Requirement coverage** — missing user requirements, changed requirements, acceptance criteria, or workflow constraints. +2. **Execution clarity** — vague steps, missing ordering/dependencies, or steps that cannot be executed by the coding agent. +3. **Scope control** — unsafe expansion, missing file-scope boundaries, or contradictions with project instructions. +4. **Verification quality** — absent or weak tests/checks for the behavior being changed. +5. **Risk callouts** — migrations, data-loss paths, external integrations, secrets, or plugin/runtime dependencies that need explicit handling. + +Be specific: cite the plan section or file path for every finding and explain the concrete correction. + +## Output Requirements +- APPROVE: the plan is ready for execution. +- APPROVE_WITH_NOTES: execution may proceed, but include non-blocking advisory notes. +- REVISE: the plan should be corrected before execution; include the missing or wrong requirement and the needed change. +- 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":"..."}`; + +/** Build the `plan-review` optional-group node placed between planning and execution. */ +export function planReviewOptionalGroupNode(column: string): WorkflowIrNode { + return { + id: PLAN_REVIEW_GROUP_ID, + kind: "optional-group", + column, + config: { + name: PLAN_REVIEW_NAME, + defaultOn: true, + template: { + nodes: [ + { + id: PLAN_REVIEW_STEP_NODE_ID, + kind: "prompt", + config: { + name: PLAN_REVIEW_NAME, + description: PLAN_REVIEW_DESCRIPTION, + prompt: PLAN_REVIEW_PROMPT, + toolMode: "readonly", + gateMode: "advisory", + }, + }, + ], + edges: [], + }, + }, + }; +} diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts index b1c8724679..bf656cef34 100644 --- a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -4,6 +4,7 @@ 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 { planReviewOptionalGroupNode } from "./builtin-plan-review-group.js"; /** * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step @@ -37,6 +38,11 @@ import { codeReviewOptionalGroupNode } from "./builtin-code-review-group.js"; * planning seam. The IR is v2-only (foreach/step-review/parse-steps are v2 node * kinds), so `downgradeIrToV1IfPure` refuses it and the flag-OFF rollback contract * (KTD-8) is preserved automatically. + * + * FNXC:PlanReviewStep 2026-06-28-23:29: + * Coding (per-step review) also needs the default-on optional Plan Review gate before + * execution. The group sits between `plan` and `parse` so operators can toggle plan + * review independently while preserving the per-step code review/rework loop. */ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { version: "v2", @@ -68,6 +74,7 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { { id: "start", kind: "start", column: "triage" }, // Planning seam: produces PROMPT.md (the declared step-source artifact). { id: "plan", kind: "prompt", column: "in-progress", config: builtinPromptConfig("planning", "Plan") }, + planReviewOptionalGroupNode("in-progress"), // KTD-12: parse the planned PROMPT.md into the task step list. This node must // dominate the foreach (validator-enforced). { @@ -165,8 +172,10 @@ const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { ], edges: [ { from: "start", to: "plan" }, - { from: "plan", to: "parse", condition: "success" }, + { from: "plan", to: "plan-review", condition: "success" }, { from: "plan", to: "end", condition: "failure" }, + { from: "plan-review", to: "parse", condition: "success" }, + { from: "plan-review", to: "end", condition: "failure" }, { from: "parse", to: "steps", condition: "success" }, // parse-steps no-steps defaults to success; route it explicitly to the foreach // (zero steps → foreach no-ops through its success edge, KTD-8/R8). diff --git a/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts index 8cd8c36eb3..dc925fcb06 100644 --- a/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts +++ b/packages/core/src/builtin-stepwise-final-review-coding-workflow-ir.ts @@ -1,6 +1,7 @@ import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "./builtin-stepwise-coding-workflow-ir.js"; +import { planReviewOptionalGroupNode } from "./builtin-plan-review-group.js"; function cloneWorkflowIr(ir: WorkflowIr): WorkflowIr { return JSON.parse(JSON.stringify(ir)) as WorkflowIr; @@ -9,6 +10,12 @@ function cloneWorkflowIr(ir: WorkflowIr): WorkflowIr { /* FNXC:WorkflowBuiltins 2026-06-28-23:09: Operators need graph-owned step execution without per-step AI review. This built-in preserves the per-step-review workflow's parse-steps and sequential foreach model, then runs the normal end-of-task browser/code-review/final-review/merge suffix after all planned steps finish. + +FNXC:WorkflowBuiltins 2026-06-28-23:29: +The new default Coding workflow should have only one review surface at the end, controlled by the `code-review` optional step. Keep the optional group default-on/toggleable, remove the mandatory final `review` seam from this derived graph, and route approved or disabled code review directly into the merge gate. + +FNXC:WorkflowBuiltins 2026-06-28-23:29: +Plan Review is also an optional step, but it runs before execution rather than at the end. Insert the `plan-review` group between `plan` and `parse` so a task can review PROMPT.md before planned steps become executable work. */ const RAW_BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR: WorkflowIr = (() => { const ir = cloneWorkflowIr(BUILTIN_STEPWISE_CODING_WORKFLOW_IR); @@ -25,15 +32,35 @@ const RAW_BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR: WorkflowIr = (() => throw new Error("stepwise final-review built-in requires the stepwise foreach template"); } + const planIndex = ir.nodes.findIndex((node) => node.id === "plan"); + if (planIndex < 0) { + throw new Error("stepwise final-review built-in requires a plan node"); + } + if (!ir.nodes.some((node) => node.id === "plan-review")) { + ir.nodes.splice(planIndex + 1, 0, planReviewOptionalGroupNode("in-progress")); + } + template.nodes = template.nodes.filter((node) => node.id !== "step-review"); template.edges = [ { from: "step-execute", to: "step-done", condition: "success" }, ]; ir.nodes = ir.nodes.filter((node) => node.id !== "rework-hold"); + ir.nodes = ir.nodes.filter((node) => node.id !== "review"); ir.edges = ir.edges.filter( - (edge) => edge.from !== "rework-hold" && edge.to !== "rework-hold", + (edge) => edge.from !== "rework-hold" && edge.to !== "rework-hold" && edge.from !== "review" && edge.to !== "review", ); + ir.edges = ir.edges.filter((edge) => !(edge.from === "plan" && edge.to === "parse")); + if (!ir.edges.some((edge) => edge.from === "plan" && edge.to === "plan-review")) { + ir.edges.push({ from: "plan", to: "plan-review", condition: "success" }); + } + if (!ir.edges.some((edge) => edge.from === "plan-review" && edge.to === "parse")) { + ir.edges.push({ from: "plan-review", to: "parse", condition: "success" }); + } + if (!ir.edges.some((edge) => edge.from === "plan-review" && edge.to === "end" && edge.condition === "failure")) { + ir.edges.push({ from: "plan-review", to: "end", condition: "failure" }); + } + ir.edges.push({ from: "code-review", to: "merge-gate", condition: "success" }); return ir; })(); diff --git a/packages/core/src/builtin-workflows.ts b/packages/core/src/builtin-workflows.ts index b0d6eed3e5..4cc0f08285 100644 --- a/packages/core/src/builtin-workflows.ts +++ b/packages/core/src/builtin-workflows.ts @@ -139,17 +139,17 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ { id: "builtin:coding", name: "Coding (built-in)", - description: "Default coding pipeline: plan steps, execute them one at a time, then review and merge the full result.", + description: "Default coding pipeline: plan steps, execute them one at a time, then run the optional final code review and merge.", kind: "workflow", ir: BUILTIN_STEPWISE_FINAL_REVIEW_CODING_WORKFLOW_IR, layout: { start: { x: 60, y: 160 }, plan: { x: 230, y: 160 }, - parse: { x: 400, y: 160 }, - steps: { x: 570, y: 160 }, - "browser-verification": { x: 740, y: 160 }, - "code-review": { x: 910, y: 160 }, - review: { x: 1080, y: 160 }, + "plan-review": { x: 400, y: 160 }, + parse: { x: 570, y: 160 }, + steps: { x: 740, y: 160 }, + "browser-verification": { x: 910, y: 160 }, + "code-review": { x: 1080, y: 160 }, "merge-gate": { x: 1250, y: 160 }, "branch-group-member-integration": { x: 1420, y: 80 }, "branch-group-promotion": { x: 1590, y: 80 }, @@ -374,8 +374,8 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ ], }), // The stepwise coding workflow (KTD-9) — step inversion as authored graph - // structure (parse-steps → foreach{ step-execute → step-review } → review → - // merge). Authored directly as a v2 IR (the `linear` helper only builds simple + // structure (plan-review → parse-steps → foreach{ step-execute → step-review } + // → optional gates → review → merge). Authored directly as a v2 IR (the `linear` helper only builds simple // pipelines); it is read-only like every built-in and runs on the default // workflow graph runtime. { @@ -388,12 +388,21 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ layout: { start: { x: 60, y: 160 }, plan: { x: 230, y: 160 }, - parse: { x: 400, y: 160 }, - steps: { x: 570, y: 160 }, - "rework-hold": { x: 570, y: 320 }, - review: { x: 740, y: 160 }, - merge: { x: 910, y: 160 }, - end: { x: 1080, y: 160 }, + "plan-review": { x: 400, y: 160 }, + parse: { x: 570, y: 160 }, + steps: { x: 740, y: 160 }, + "rework-hold": { x: 740, y: 320 }, + "browser-verification": { x: 910, y: 160 }, + "code-review": { x: 1080, y: 160 }, + review: { x: 1250, y: 160 }, + "merge-gate": { x: 1420, y: 160 }, + "branch-group-member-integration": { x: 1590, y: 80 }, + "branch-group-promotion": { x: 1760, y: 80 }, + "merge-attempt": { x: 1930, y: 160 }, + "merge-retry": { x: 2100, y: 80 }, + "recovery-router": { x: 2100, y: 240 }, + "merge-manual-hold": { x: 1590, y: 240 }, + end: { x: 2270, y: 160 }, }, createdAt: BUILTIN_TS, updatedAt: BUILTIN_TS, diff --git a/packages/dashboard/src/routes/__tests__/task-create-workflow-route.test.ts b/packages/dashboard/src/routes/__tests__/task-create-workflow-route.test.ts index 0ae59aa6a0..064e383fe4 100644 --- a/packages/dashboard/src/routes/__tests__/task-create-workflow-route.test.ts +++ b/packages/dashboard/src/routes/__tests__/task-create-workflow-route.test.ts @@ -127,9 +127,9 @@ describe("POST /tasks workflowId (U6/R3)", () => { }); it.each([ - ["default coding", "builtin:coding", ["code-review"]], + ["default coding", "builtin:coding", ["plan-review", "code-review"]], ["legacy coding", "builtin:legacy-coding", ["code-review"]], - ["coding per-step review", "builtin:stepwise-coding", ["code-review"]], + ["coding per-step review", "builtin:stepwise-coding", ["plan-review", "code-review"]], ])("%s workflow create/select/resolve works end to end", async (_label, workflowId, defaultSteps) => { const res = await post("/api/tasks", { description: `exercise ${workflowId}`,