diff --git a/.changeset/v2-fix-steps-and-catalog-repair.md b/.changeset/v2-fix-steps-and-catalog-repair.md new file mode 100644 index 0000000000..3879af12fc --- /dev/null +++ b/.changeset/v2-fix-steps-and-catalog-repair.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: A failed final check or review now shows named fix steps on the card; per-step failures stay in their step. +category: fix +dev: Adds `fix-steps-from-failed-gates.test.ts`, driving the real `requestPreMergeOptionalStepFix` and `appendReviewRemediationSteps` against the real built-in registry and asserting on `task.steps`: a `verification` failure and a `code-review` REVISE each append pending named steps with remediation provenance, a review failure with no REVISE verdict appends nothing, no other node id can reach the appender (so a per-step test failure is fixed inside its step), and `builtin:coding-ideas` keeps reopen-trailing. Also repairs three leftovers from the V2 rework: the missing `builtin:coding-ideas-v2` entry in `builtin-workflows-lifecycle.test.ts` EXPECTATIONS (catalog-coverage assertion was red on main), the registry description and layout (ghost `verification`/`verification-remediation`/`completion-summary`/`post-merge-verification` keys removed, `documentation-delivery` repositioned after `code-review` so the editor diagram matches the graph), and the `implementation-only-leakage` audit, which no longer flags `testing|verification` now that the planner emits that step deliberately. diff --git a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts index 2decf78c08..1921d8e312 100644 --- a/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts +++ b/packages/core/src/workflows/builtin-coding-ideas-v2-workflow-ir.ts @@ -9,30 +9,29 @@ import { codeReviewRemediationStepsNode } from "./builtin-workflow-remediation-n const clone = (ir: WorkflowIr): WorkflowIr => JSON.parse(JSON.stringify(ir)) as WorkflowIr; /* -FNXC:CodingIdeasV2Workflow 2026-08-24-05:35: +FNXC:CodingIdeasV2Workflow 2026-08-26-05:56: Operator intent: keep the Coding (Ideas) board exactly as it is (manual "Ideas" intake, autoTriage -false), but stop hiding testing and documentation inside the implementation checklist. They become -VISIBLE review-column gates, and the merge is the last thing that happens after delivery. +false), and enforce ONE rule the inherited board does not — nothing in `in-review` writes code. -in-progress : steps = implementation only -in-review : verification -> documentation-delivery -> completion-summary -> code-review -> merge +in-progress : steps (implementation + its tests) -> the executor's own final verification +in-review : code-review -> documentation-delivery -> merge -Ordering is NOT cosmetic. `execute-workflow-graph.ts` refuses any write-capable node once a Code -Review APPROVE exists (`workspace-review-seal-required`): a passed review seals the tree so nothing -unreviewed can reach main. `verification-step` (its name matches the write-capable classifier) and -`documentation-delivery-step` (`toolMode: "coding"`) are both write-capable, so both MUST precede -`code-review`. builtin:review-gated-coding places them after it and therefore deadlocks on every -task the moment the review approves — that defect is the reason this ordering is explicit here. +Work and proof of work both finish in `in-progress`. The executor plans and runs its own tests, then +the FN-3345 gate re-runs the project's configured test/build commands as an independent measurement; +a red result appends NAMED fix steps to the card instead of letting it advance (see +bounce-verification-failure.ts). A card only crosses into review once that is green. -`completion-summary` runs BEFORE `code-review`, matching the inherited graph. It escapes the review -seal (it is `toolMode: "readonly"`, so the write-capable classifier ignores it), which made "summary -last, so it can describe the approved state" look correct — and it is wrong. The node still acquires -a task worktree, and ANY node running between the review and the merge changes the tree the review -approved, so `canMergeTask` refuses with "task has no provable approval for the content being -merged" (FN-180's review-diff fingerprint). Measured: the pipeline-smoke S01 run on this workflow -failed exactly there, then looped through verification-remediation. The seal is not the only thing -ordering these nodes; the merge fingerprint is the other, and it is stricter. -It stays best-effort with a success-only edge — a summary failure must never wedge a task. +`in-review` is then three read-only milestones. Code Review judges the work — it is the only gate +that can hold the card. Documentation reports on it. The merge is the last thing that happens. + +Ordering is still NOT cosmetic, for a stricter reason than the review seal: ANY node that runs +between the review and the merge changes the tree the review approved, so `canMergeTask` refuses +with "task has no provable approval for the content being merged" (FN-180's review-diff +fingerprint). Measured on pipeline-smoke S01, where a readonly completion-summary node placed after +the review failed exactly there — readonly was not enough, because it still acquired a worktree. +Documentation may follow the review ONLY because it writes nothing at all: no repository files, no +worktree-visible change. The write-capable seal (`workspace-review-seal-required`) is the second, +weaker constraint on this ordering. */ const RAW_BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR: WorkflowIr = (() => { const ir = clone(BUILTIN_CODING_IDEAS_WORKFLOW_IR); @@ -177,21 +176,11 @@ unhandled failure modes.`; /* Every inherited edge touching `completion-summary` dies with the node; the lane is rebuilt below. */ ir.edges = ir.edges.filter((edge) => edge.from !== "completion-summary" && edge.to !== "completion-summary"); - /* - FNXC:CodingIdeasV2Workflow 2026-08-24-05:35: - Both remediation loops re-enter at `verification`, never directly at `code-review`. That is what - keeps the documentation honest: a REVISE sends the fix back to in-progress, then the walk replays - verification AND documentation-delivery, so the docs and changeset are regenerated to include what - the review demanded before it re-reads them. Re-entering at `code-review` would leave the docs - describing a tree that no longer exists. `verification` is the rework-region head (`reworkRegion: - true`, `maxReworkCycles: 3`), which is what makes these edges legal. - */ /* FNXC:CodingIdeasV2Workflow 2026-08-24-20:40: - Push ONLY the genuinely new edges. `completion-summary -> code-review`, `code-review -> merge-gate` - and the code-review rework are inherited from Coding (Ideas) and were being re-pushed, so the - graph carried each of them twice — a duplicated success edge out of a review gate is a second, - competing traversal of the same lane. + Push ONLY the genuinely new edges. `code-review -> merge-gate` and the code-review rework are + inherited from Coding (Ideas) and were being re-pushed, so the graph carried each of them twice — + a duplicated success edge out of a review gate is a second, competing traversal of the same lane. */ /* `code-review -> merge-gate` is inherited and must not survive: Documentation now sits between them. */ ir.edges = ir.edges.filter((edge) => !(edge.from === "code-review" && edge.to === "merge-gate")); diff --git a/packages/core/src/workflows/builtin-workflows.ts b/packages/core/src/workflows/builtin-workflows.ts index 1dea107d96..a502aaec3d 100644 --- a/packages/core/src/workflows/builtin-workflows.ts +++ b/packages/core/src/workflows/builtin-workflows.ts @@ -589,17 +589,21 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ updatedAt: BUILTIN_TS, }, /* - * FNXC:CodingIdeasV2Workflow 2026-08-24-05:35: - * Same Ideas board as builtin:coding-ideas, but testing and documentation stop being hidden - * checklist items inside the implementation steps and become visible in-review gates, with the - * merge last. Every write-capable gate runs BEFORE Code Review because a passed review seals the - * tree (`workspace-review-seal-required`); the readonly completion summary runs after it. + * FNXC:CodingIdeasV2Workflow 2026-08-26-05:56: + * Same Ideas board as builtin:coding-ideas, with one rule: in-review NEVER writes code. + * Implementation and its tests finish in `in-progress` — the executor's own final verification + * runs the project's configured test/build commands there, and a red result appends named fix + * steps rather than letting the card advance. `in-review` is then three read-only milestones: + * Code Review judges, Documentation reports, and the merge is the last thing that happens. + * The layout is ordered left-to-right in that same sequence so the editor's diagram reads the way + * the graph runs; nodes deleted from the graph must be deleted from this map too, or the editor + * keeps positioning ghosts. */ { id: "builtin:coding-ideas-v2", name: "Coding (Ideas) V2", description: - "Capture-first coding pipeline with visible review-column gates: park ideas in a manual intake, plan, implement per step, then verify, document, review, summarize, and merge.", + "Capture-first coding pipeline with a read-only review lane: park ideas in a manual intake, plan, implement and test per step, then review, document, and merge.", kind: "workflow", ir: BUILTIN_CODING_IDEAS_V2_WORKFLOW_IR, layout: { @@ -611,21 +615,17 @@ export const BUILTIN_WORKFLOWS: WorkflowDefinition[] = [ parse: { x: 570, y: 160 }, steps: { x: 740, y: 160 }, "review-pending-handoff": { x: 740, y: 320 }, - verification: { x: 910, y: 160 }, - "verification-remediation": { x: 910, y: 320 }, + "code-review": { x: 910, y: 160 }, + "code-review-remediation": { x: 910, y: 320 }, "documentation-delivery": { x: 1080, y: 160 }, - "code-review": { x: 1250, y: 160 }, - "code-review-remediation": { x: 1250, y: 320 }, - "completion-summary": { x: 1420, y: 160 }, - "merge-gate": { x: 1590, y: 160 }, - "branch-group-member-integration": { x: 1760, y: 80 }, - "branch-group-promotion": { x: 1930, y: 80 }, - "merge-attempt": { x: 2100, y: 160 }, - "merge-retry": { x: 2270, y: 80 }, - "recovery-router": { x: 2270, y: 240 }, - "merge-manual-hold": { x: 1760, y: 240 }, - "post-merge-verification": { x: 2440, y: 160 }, - end: { x: 2610, y: 160 }, + "merge-gate": { x: 1250, y: 160 }, + "branch-group-member-integration": { x: 1420, y: 80 }, + "branch-group-promotion": { x: 1590, y: 80 }, + "merge-manual-hold": { x: 1420, y: 240 }, + "merge-attempt": { x: 1760, y: 160 }, + "merge-retry": { x: 1930, y: 80 }, + "recovery-router": { x: 1930, y: 240 }, + end: { x: 2100, y: 160 }, }, createdAt: BUILTIN_TS, updatedAt: BUILTIN_TS, diff --git a/packages/engine/src/__tests__/builtin-workflows-lifecycle.test.ts b/packages/engine/src/__tests__/builtin-workflows-lifecycle.test.ts index 77b22cceaa..2cc724d0ac 100644 --- a/packages/engine/src/__tests__/builtin-workflows-lifecycle.test.ts +++ b/packages/engine/src/__tests__/builtin-workflows-lifecycle.test.ts @@ -328,6 +328,26 @@ const EXPECTATIONS: BuiltinExpectation[] = [ finalColumn: "done", leasedGates: ["plan-review", "code-review"], }, + /* + FNXC:CodingIdeasV2Workflow 2026-08-26-05:56: + Same board and same crossings as builtin:coding-ideas — the difference is entirely INSIDE the two + working columns, so the trail is identical and that is the point: a read-only review lane must not + change where the card goes, only what happens while it is there. + This entry was missing when the workflow was registered, which left the catalog-coverage assertion + red on main while every other test in this file passed. + */ + { + id: "builtin:coding-ideas-v2", + entryColumn: "ideas", + trail: [ + ["ideas", "todo", "graph"], + ["todo", "in-progress", "scheduler"], + ["in-progress", "in-review", "graph"], + ["in-review", "done", "graph"], + ], + finalColumn: "done", + leasedGates: ["plan-review", "code-review"], + }, { id: "builtin:legacy-coding", entryColumn: "triage", diff --git a/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts b/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts new file mode 100644 index 0000000000..af95058ad2 --- /dev/null +++ b/packages/engine/src/__tests__/fix-steps-from-failed-gates.test.ts @@ -0,0 +1,215 @@ +/* +FNXC:VerificationRemediation 2026-08-26-05:56: +The operator contract for Coding (Ideas) V2, stated as behaviour a card shows: + + - A failing test INSIDE a step is the step's own problem. The executor fixes it there, and it must + never become a separate fix step \u2014 otherwise every red test during implementation would litter + the checklist with work the session is already doing. + - A failing FINAL verification (the project's configured test/build commands, run once after every + planned step succeeds) DOES become fix steps: the session that could have fixed it in place is + over, so the work has to be visible on the card and re-dispatched. + - A Code Review REVISE does the same, from the reviewer's findings. + +These tests drive the REAL routing seam (`requestPreMergeOptionalStepFix`) and the REAL appender +against the REAL V2 workflow IR, and assert on `task.steps` \u2014 the list an operator reads on the card. +A spy on the appender would prove only that a function was called; the appender's `Verification` +branch was present, correct, and caller-less for days, which is exactly the failure a spy misses. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task, TaskStep } from "@fusion/core"; +import { getBuiltinWorkflow } from "@fusion/core"; + +import { appendReviewRemediationSteps } from "../executor/append-review-remediation-steps.js"; +import { requestPreMergeOptionalStepFix } from "../executor/request-pre-merge-optional-step-fix.js"; + +const FAILING_OUTPUT = [ + "test command `pnpm test` failed (exit 1):", + " FAIL packages/engine/src/retry.ts:42", + " expected 3 retries, received 1", +].join("\n"); + +const PROMPT = [ + "# Task: FN-VR-2", + "", + "## File Scope", + "", + "- `packages/engine/src/*`", + "", + "## Steps", + "", + "### Step 1: Add the retry guard", + "", + "### Step 2: Testing & Verification", + "", +].join("\n"); + +/* + A card that has finished every planned step and is at the verification boundary. + The workflow is SELECTED by id and resolved through the real built-in registry, because that is what + decides the bounce shape — injecting an IR object here would be resolved away and prove nothing. +*/ +function harness(workflowId = "builtin:coding-ideas-v2") { + const task = { + id: "FN-VR-2", + column: "in-progress", + worktree: "/tmp/fn-vr-2", + prompt: PROMPT, + modifiedFiles: ["packages/engine/src/retry.ts"], + steps: [ + { name: "Add the retry guard", status: "done" }, + { name: "Testing & Verification", status: "done" }, + ] as TaskStep[], + } as Task; + + const store = { + getTask: vi.fn(async () => task), + getSettings: vi.fn(async () => ({ autoMerge: true })), + logEntry: vi.fn(async () => undefined), + updateTask: vi.fn(async (_id: string, patch: Partial) => { Object.assign(task, patch); return task; }), + appendRemediationSteps: vi.fn(async (_id: string, steps: readonly TaskStep[], options: { wave?: number }) => { + const appended = steps.map((step) => ({ ...step, status: "pending" as const })); + task.steps = [...(task.steps ?? []), ...appended]; + return { task, appended, appendedCount: appended.length, wave: options.wave ?? 1 }; + }), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId })), + getWorkflowDefinition: vi.fn(async (id: string) => { + const workflow = getBuiltinWorkflow(id); + return workflow ? { ir: workflow.ir } : undefined; + }), + }; + + const sendTaskBackForFix = vi.fn(async () => undefined); + const deps = { + store: store as never, + getRunContextFor: () => undefined, + recoverMissingRequiredArtifacts: vi.fn(async () => undefined), + parkPlanReviewReplanCapExhausted: vi.fn(async () => undefined), + clearPausedAborted: vi.fn(), + readTaskArtifact: async () => task.prompt, + appendReviewRemediationSteps: (live: Task, info: never) => appendReviewRemediationSteps( + { store: store as never, readTaskArtifact: async () => task.prompt, sendTaskBackForFix }, + live, + info, + ), + workflowLifecycleMovesInFlight: new Set(), + sendTaskBackForFix, + }; + + const pending = () => (task.steps ?? []).filter((step) => step.status === "pending"); + return { task, store, deps, sendTaskBackForFix, pending }; +} + +describe("fix steps appear on the card when a gate fails", () => { + it("turns a failing FINAL verification into named work on the card", async () => { + const { deps, task, pending, sendTaskBackForFix } = harness(); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Verification (test)", + feedback: FAILING_OUTPUT, + phase: "pre-merge", + status: "failed", + nodeId: "verification", + }); + + expect(scheduled).toBe(true); + expect(pending()).toHaveLength(1); + expect(pending()[0]!.name).toContain("packages/engine/src/retry.ts"); + expect(pending()[0]!.remediation).toMatchObject({ gate: "Verification", wave: 1 }); + // Completed implementation work is preserved: remediation appends, it never reopens. + expect(task.steps?.slice(0, 2).map((step) => step.status)).toEqual(["done", "done"]); + // And the card is actually re-dispatched to run that step. + expect(sendTaskBackForFix).toHaveBeenCalledTimes(1); + }); + + it("turns a Code Review REVISE into named work on the card", async () => { + const { deps, task, pending, sendTaskBackForFix } = harness(); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Code Review", + feedback: "The guard is inverted.", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + nodeId: "code-review", + findings: [{ + id: "finding-1", + title: "inverted guard", + body: "Reverse the retry guard condition", + filePath: "packages/engine/src/retry.ts", + line: 42, + severity: "critical", + }], + }); + + expect(scheduled).toBe(true); + expect(pending()).toHaveLength(1); + expect(pending()[0]!.name).toContain("Reverse the retry guard condition"); + expect(pending()[0]!.remediation).toMatchObject({ + gate: "Code Review", + findingId: "finding-1", + filePath: "packages/engine/src/retry.ts", + }); + expect(sendTaskBackForFix).toHaveBeenCalledTimes(1); + }); + + /* + A gate that failed for transport/provider reasons produced no verdict and therefore no findings. + Manufacturing work from it would hand the executor an invented task. + */ + it("creates nothing from a Code Review failure that carries no REVISE verdict", async () => { + const { deps, task, pending, sendTaskBackForFix } = harness(); + + const scheduled = await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Code Review", + feedback: "reviewer session aborted", + phase: "pre-merge", + status: "failed", + nodeId: "code-review", + }); + + expect(scheduled).toBe(false); + expect(pending()).toHaveLength(0); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + }); + + /* + THE RULE THIS PROTECTS: a red test inside a step is fixed inside that step. Only the two named + gates may append work, so no per-step failure can reach the appender and litter the checklist. + */ + it("never creates a fix step for a per-step failure", async () => { + const { task, pending, sendTaskBackForFix, store } = harness(); + + for (const nodeId of ["step-execute", "steps", "parse", undefined]) { + const appended = await appendReviewRemediationSteps( + { store: store as never, readTaskArtifact: async () => task.prompt, sendTaskBackForFix }, + task, + { stepName: "Step 1", feedback: FAILING_OUTPUT, phase: "pre-merge", status: "failed", nodeId } as never, + ); + expect(appended, `nodeId ${String(nodeId)} must not be able to append work`).toBe(false); + } + + expect(pending()).toHaveLength(0); + expect(store.appendRemediationSteps).not.toHaveBeenCalled(); + expect(sendTaskBackForFix).not.toHaveBeenCalled(); + }); + + /* + The appended shape is selected by the WORKFLOW, not by this seam: Coding (Ideas) reopens its + trailing step instead, and must keep doing so. + */ + it("leaves the inherited Coding (Ideas) workflow on its reopen-trailing bounce", async () => { + const { deps, task, pending } = harness("builtin:coding-ideas"); + + await requestPreMergeOptionalStepFix(deps as never, task.id, task, { + stepName: "Code Review", + feedback: "The guard is inverted.", + phase: "pre-merge", + status: "failed", + verdict: "REVISE", + nodeId: "code-review", + findings: [{ id: "f1", title: "t", body: "b", filePath: "packages/engine/src/retry.ts", severity: "critical" }], + }); + + expect(pending(), "named remediation belongs to V2, not to the inherited board").toHaveLength(0); + }); +}); diff --git a/packages/engine/src/__tests__/review-gated-step-preservation.test.ts b/packages/engine/src/__tests__/review-gated-step-preservation.test.ts index 6824e6f6b7..3343b5f47b 100644 --- a/packages/engine/src/__tests__/review-gated-step-preservation.test.ts +++ b/packages/engine/src/__tests__/review-gated-step-preservation.test.ts @@ -29,4 +29,23 @@ describe("review-gated parse-step preservation", () => { expect(writes).toEqual([[{ name: "Wire documentation link resolver", status: "pending" }, { name: "Testing & Verification", status: "pending" }]]); expect(audits).toContain("implementation-only-leakage"); }); + + /* + FNXC:PlanningDocumentationStep 2026-08-26-05:56: + A planned `Testing & Verification` step is the INTENDED plan on these workflows — the executor owns + testing because a readonly reviewer cannot run commands. Auditing it as review-gate leakage made + every card report a problem with its own correct plan, which is how a signal stops being read. + */ + it("does not report leakage for the testing step the planner is meant to emit", async () => { + const writes: TaskStep[][] = []; + const audits: string[] = []; + const runner = new ParseStepsNodeRunner({ + readArtifact: async () => "### Step 1: Add the retry guard\n### Step 2: Testing & Verification", + writeSteps: async (_task, steps) => { writes.push(steps); }, + audit: (reason) => audits.push(reason), + }); + await runner.run(node({ artifact: "PROMPT.md", parser: "step-headings", implementationOnlySteps: true }), { task: task(), context: {} }); + expect(writes[0]?.map((step) => step.name)).toEqual(["Add the retry guard", "Testing & Verification"]); + expect(audits).not.toContain("implementation-only-leakage"); + }); }); diff --git a/packages/engine/src/workflow-node-runners/parse-steps-runner.ts b/packages/engine/src/workflow-node-runners/parse-steps-runner.ts index 84711e80f8..880a9727a4 100644 --- a/packages/engine/src/workflow-node-runners/parse-steps-runner.ts +++ b/packages/engine/src/workflow-node-runners/parse-steps-runner.ts @@ -133,9 +133,20 @@ export class ParseStepsNodeRunner implements WorkflowNodeRunner { return step; }); if (cfg.implementationOnlySteps === true) { - const leakage = steps.filter((step) => /(^|[^a-z])(testing|verification|documentation|delivery)([^a-z]|$)/i.test(step.name)); + /* + FNXC:PlanningDocumentationStep 2026-08-26-05:56: + TESTING IS NO LONGER LEAKAGE. This audit used to flag `testing|verification` too, from the + revision that moved test execution into a review-column gate. That reversed: a readonly + reviewer cannot run commands, so testing belongs to the executor and the planner emits a + `Testing & Verification` step ON PURPOSE. Flagging it made every card on such a workflow report + review-gate leakage for its own intended plan, which trains an operator to ignore the signal. + Documentation and delivery ARE still leakage: those are produced by the in-review Documentation + milestone, and a step planning them is duplicated work (see stripDocumentationDeliveryStep). + Detection stays deliberately non-destructive — an implementation step name can legitimately + contain these words. + */ + const leakage = steps.filter((step) => /(^|[^a-z])(documentation|delivery)([^a-z]|$)/i.test(step.name)); if (leakage.length > 0) { - // Detection is deliberately non-destructive: implementation names can legitimately contain these words. this.audit("implementation-only-leakage", `parse-steps node '${node.id}' detected possible review-gate work: ${leakage.map((step) => step.name).join(", ")}`); } }