From 60e9938f5a1b1f5f5b20c2340a4ba2172c5aa220 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 13:15:49 -0700 Subject: [PATCH] =?UTF-8?q?feat(core,engine):=20U7=20=E2=80=94=20builtin?= =?UTF-8?q?=20stepwise=20coding=20workflow=20+=20trajectory=20parity=20&?= =?UTF-8?q?=20invariant=20suite;=20agent-tool=20surfaces=20for=20custom=20?= =?UTF-8?q?fields=20and=20IR=20authoring?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/__tests__/builtin-workflows.test.ts | 27 +- .../builtin-stepwise-coding-workflow-ir.ts | 151 +++++ packages/core/src/builtin-workflows.ts | 27 + packages/core/src/index.ts | 8 + .../stepwise-workflow-parity.test.ts | 578 ++++++++++++++++++ .../workflow-graph-executor-parity.test.ts | 11 + packages/engine/src/agent-tools.ts | 22 +- packages/engine/src/executor.ts | 89 ++- packages/engine/src/triage.ts | 12 + 9 files changed, 916 insertions(+), 9 deletions(-) create mode 100644 packages/core/src/builtin-stepwise-coding-workflow-ir.ts create mode 100644 packages/engine/src/__tests__/stepwise-workflow-parity.test.ts diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index e605ee50f0..78afed3ed2 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -7,15 +7,38 @@ import { DEFAULT_WORKFLOW_COLUMN_IDS, parseWorkflowIr } from "../workflow-ir.js" import { createTaskStoreTestHarness } from "./store-test-helpers.js"; describe("built-in workflows", () => { - it("every built-in has a valid IR and compiles without error", () => { + // Graph-only built-ins (step inversion, KTD-9) model branching/foreach/rework + // structure the linear compiler cannot lower to a step list — they run only + // under the workflow graph executor. They still must parse as valid IR. + const GRAPH_ONLY_BUILTIN_IDS = new Set(["builtin:stepwise-coding"]); + + it("every built-in has a valid IR; linear built-ins compile without error", () => { expect(BUILTIN_WORKFLOWS.length).toBeGreaterThanOrEqual(4); for (const wf of BUILTIN_WORKFLOWS) { expect(isBuiltinWorkflowId(wf.id)).toBe(true); expect(() => parseWorkflowIr(wf.ir)).not.toThrow(); - expect(() => compileWorkflowToSteps(wf.ir)).not.toThrow(); + if (!GRAPH_ONLY_BUILTIN_IDS.has(wf.id)) { + expect(() => compileWorkflowToSteps(wf.ir)).not.toThrow(); + } } }); + it("includes the stepwise coding built-in modeling step inversion (KTD-9)", () => { + const stepwise = getBuiltinWorkflow("builtin:stepwise-coding"); + expect(stepwise).toBeDefined(); + const ir = parseWorkflowIr(stepwise!.ir); + 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); + const foreach = ir.nodes.find((n) => n.kind === "foreach"); + expect(foreach).toBeDefined(); + const template = ( + foreach!.config as { template: { nodes: Array<{ kind: string; config?: { seam?: string } }> } } + ).template; + expect(template.nodes.some((n) => n.kind === "step-review")).toBe(true); + expect(template.nodes.some((n) => n.config?.seam === "step-execute")).toBe(true); + }); + it("default workflow column ids equal the legacy enum values, in legacy order (KTD-1)", () => { expect(BUILTIN_CODING_WORKFLOW_IR.version).toBe("v2"); if (BUILTIN_CODING_WORKFLOW_IR.version !== "v2") throw new Error("expected v2"); diff --git a/packages/core/src/builtin-stepwise-coding-workflow-ir.ts b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts new file mode 100644 index 0000000000..150e47f96e --- /dev/null +++ b/packages/core/src/builtin-stepwise-coding-workflow-ir.ts @@ -0,0 +1,151 @@ +import type { WorkflowIr } from "./workflow-ir-types.js"; +import { parseWorkflowIr } from "./workflow-ir.js"; + +/** + * The built-in **stepwise** coding workflow (KTD-9) — the demonstration of step + * inversion and the parity-comparison subject for the engine's + * `stepwise-workflow-parity.test.ts`. + * + * Unlike the default `builtin-coding-workflow-ir` (which keeps a single monolithic + * `execute` seam and is the byte-identity parity oracle, KTD-1), this workflow + * models per-step policy explicitly as graph structure: + * + * plan seam + * → parse-steps(PROMPT.md, step-headings) (KTD-12: graph-native parse) + * → foreach(task-steps, sequential, shared) { (KTD-3: runtime expansion) + * step-execute (KTD-2: run one step) + * → step-review(code): (KTD-4: verdicts as edges) + * approve → step-done (template exit) (APPROVE auto-completes) + * revise → rework back to step-execute (revise in place, no reset) + * rethink → rework back to step-execute (reset semantics handler-side) + * unavailable → (advisory) routes onward + * } + * rework-exhausted → hold(manual) (KTD-5: bounded escalation) + * → review seam + * → merge seam + * + * The columns/traits are identical to the default builtin so the full lifecycle + * (merge-blocker, capacity, hold, complete, archived) behaves exactly as it does + * for the default workflow — only the in-progress step modeling differs. + * + * It declares its step-source artifact (KTD-12): PROMPT.md produced by the + * 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. + */ +const RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR: WorkflowIr = { + version: "v2", + name: "builtin-stepwise-coding", + columns: [ + { id: "triage", name: "Triage", traits: [{ trait: "intake" }] }, + { + id: "todo", + name: "Todo", + traits: [{ trait: "hold", config: { release: "capacity" } }, { trait: "reset-on-entry" }], + }, + { + id: "in-progress", + name: "In progress", + traits: [{ trait: "wip" }, { trait: "abort-on-exit" }, { trait: "timing" }], + }, + { + id: "in-review", + name: "In review", + traits: [{ trait: "merge-blocker" }, { trait: "stall-detection" }, { trait: "merge" }], + }, + { id: "done", name: "Done", traits: [{ trait: "complete" }] }, + { id: "archived", name: "Archived", traits: [{ trait: "archived" }] }, + ], + // KTD-12: PROMPT.md is the planning-produced step-source artifact this workflow + // parses into task steps. + artifacts: [{ key: "PROMPT.md", title: "Plan", producedBy: "planning", role: "step-source" }], + nodes: [ + { id: "start", kind: "start", column: "triage" }, + // Planning seam: produces PROMPT.md (the declared step-source artifact). + { id: "plan", kind: "prompt", column: "in-progress", config: { seam: "planning" } }, + // KTD-12: parse the planned PROMPT.md into the task step list. This node must + // dominate the foreach (validator-enforced). + { + id: "parse", + kind: "parse-steps", + column: "in-progress", + config: { artifact: "PROMPT.md", parser: "step-headings" }, + }, + // KTD-3: runtime-expanding per-step region. Sequential + shared isolation is + // the default baseline physics (one step at a time in the task's worktree). + { + id: "steps", + kind: "foreach", + column: "in-progress", + config: { + source: "task-steps", + mode: "sequential", + isolation: "shared", + maxReworkCycles: 3, + template: { + nodes: [ + // KTD-2: run exactly this step inside the task's session/worktree. + { id: "step-execute", kind: "prompt", config: { seam: "step-execute" } }, + // KTD-4: per-step code review; verdicts become outcome edges. + { id: "step-review", kind: "step-review", config: { type: "code" } }, + // Template exit (the single sink the validator requires): a config-less + // gate is a pure pass-through (createGateHandler → success), so APPROVE + // routes here and the instance exits. The step is already marked done by + // the step-review APPROVE verdict (projection authority, KTD-4/KTD-7). + { id: "step-done", kind: "gate", config: {} }, + ], + edges: [ + { from: "step-execute", to: "step-review", condition: "success" }, + // APPROVE → template exit (step-done). The step-review verdict already + // marked the step done through the projection. + { from: "step-review", to: "step-done", condition: "outcome:approve" }, + // REVISE → rework back to step-execute, revise in place (no reset). + { + from: "step-review", + to: "step-execute", + condition: "outcome:revise", + kind: "rework", + }, + // RETHINK → rework back to step-execute; the traversal triggers + // resetStepToBaseline (reset semantics are handler-side, KTD-4/U5). + { + from: "step-review", + to: "step-execute", + condition: "outcome:rethink", + kind: "rework", + }, + ], + }, + }, + }, + // KTD-5: rework exhaustion escalates to a manual hold (a human releases it). + { id: "rework-hold", kind: "hold", column: "in-progress", config: { release: "manual" } }, + { id: "review", kind: "prompt", column: "in-review", config: { seam: "review" } }, + { id: "merge", kind: "prompt", column: "in-review", config: { seam: "merge" } }, + { id: "end", kind: "end", column: "done" }, + ], + edges: [ + { from: "start", to: "plan" }, + { from: "plan", to: "parse", condition: "success" }, + { from: "plan", 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). + { from: "parse", to: "steps", condition: "outcome:no-steps" }, + { from: "parse", to: "end", condition: "failure" }, + { from: "parse", to: "end", condition: "outcome:parse-error" }, + { from: "steps", to: "review", condition: "success" }, + // KTD-5: bounded rework exhaustion → manual hold; release re-enters review. + { from: "steps", to: "rework-hold", condition: "outcome:rework-exhausted" }, + { from: "rework-hold", to: "review", condition: "success" }, + { from: "steps", to: "end", condition: "failure" }, + { from: "review", to: "merge", condition: "success" }, + { from: "review", to: "end", condition: "failure" }, + { from: "merge", to: "end", condition: "success" }, + { from: "merge", to: "end", condition: "failure" }, + ], +}; + +export const BUILTIN_STEPWISE_CODING_WORKFLOW_IR = parseWorkflowIr( + RAW_BUILTIN_STEPWISE_CODING_WORKFLOW_IR, +); diff --git a/packages/core/src/builtin-workflows.ts b/packages/core/src/builtin-workflows.ts index b8ecafc400..62e734cf44 100644 --- a/packages/core/src/builtin-workflows.ts +++ b/packages/core/src/builtin-workflows.ts @@ -1,3 +1,4 @@ +import { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "./builtin-stepwise-coding-workflow-ir.js"; import type { WorkflowDefinition } from "./workflow-definition-types.js"; import type { WorkflowIr } from "./workflow-ir-types.js"; import { parseWorkflowIr } from "./workflow-ir.js"; @@ -139,6 +140,32 @@ 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 + // pipelines); it is read-only like every built-in. Requires the + // `workflowGraphExecutor` flag at run time (foreach/step-review/parse-steps are + // interpreter-only node kinds, KTD-8); under the flag-off compile path its + // step-inversion nodes are skipped, the same posture as the other seam nodes. + { + id: "builtin:stepwise-coding", + name: "Stepwise coding (built-in)", + description: + "Per-step plan, execute, and review modeled as graph structure: each planned step runs and is reviewed (approve / revise / rethink) before the next, with bounded rework. Requires the workflow graph executor.", + ir: BUILTIN_STEPWISE_CODING_WORKFLOW_IR, + 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 }, + }, + createdAt: BUILTIN_TS, + updatedAt: BUILTIN_TS, + }, ]; const BUILTIN_BY_ID = new Map(BUILTIN_WORKFLOWS.map((wf) => [wf.id, wf])); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 574b113075..999eb211cb 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -63,8 +63,16 @@ export type { WorkflowHoldRelease, WorkflowJoinMode, WorkflowJoinBranchFailure, + // Step-inversion (KTD-3/12/13): foreach / artifacts / custom-field IR types. + WorkflowForeachConfig, + WorkflowIrArtifact, + WorkflowFieldDefinition, + WorkflowFieldType, + WorkflowFieldOption, + WorkflowFieldRender, } from "./workflow-ir-types.js"; export { BUILTIN_CODING_WORKFLOW_IR } from "./builtin-coding-workflow-ir.js"; +export { BUILTIN_STEPWISE_CODING_WORKFLOW_IR } from "./builtin-stepwise-coding-workflow-ir.js"; // ── Trait model (U2) ───────────────────────────────────────────────── export type { diff --git a/packages/engine/src/__tests__/stepwise-workflow-parity.test.ts b/packages/engine/src/__tests__/stepwise-workflow-parity.test.ts new file mode 100644 index 0000000000..379604d840 --- /dev/null +++ b/packages/engine/src/__tests__/stepwise-workflow-parity.test.ts @@ -0,0 +1,578 @@ +// ───────────────────────────────────────────────────────────────────────────── +// PARITY SUBJECT (test-file ownership, U7 / KTD-9): +// This suite owns the STEPWISE PER-STEP parity + invariant coverage: it compares +// the `updateStep` TRAJECTORY and the MERGE-BLOCKER WINDOWS of the legacy +// step-session path against the inverted stepwise foreach graph driven by the +// built-in `builtin:stepwise-coding` IR. +// +// The legacy step-session path (runStepsInNewSessions ON) is the deterministic +// per-step ORACLE here — the agent-paced monolithic path is NOT deterministically +// comparable (see plan U7) and stays covered by the default-workflow byte-identity +// suite `workflow-graph-executor-parity.test.ts`. Both paths in this file are +// driven by the SAME scripted reviewer/seams so the only variable is the path. +// +// The graph side wires the REAL substrate seams (`runTaskStep`, +// `resetStepToBaseline`, `makeAncestryBlastRadiusGuard`) exactly as the executor +// does (executor.ts createGraphSeams / applyGraphRethinkReset), against a fake +// store that records the projection trajectory — so the comparison exercises the +// production reset/blast-radius/projection code, not a re-implementation. +// +// It also exercises the non-configurable lifecycle invariants (FN-5147 +// terminal-until-merged, hard-cancel, file-scope guard) and the flag posture +// (pinned-at-dispatch, OFF-rollback recovery) on the stepwise path. +// ───────────────────────────────────────────────────────────────────────────── +import { describe, expect, it } from "vitest"; +import { + BUILTIN_STEPWISE_CODING_WORKFLOW_IR, + type StepStatus, + type TaskDetail, + type TaskStep, + type WorkflowIr, +} from "@fusion/core"; + +import { WorkflowGraphExecutor } from "../workflow-graph-executor.js"; +import { + FOREACH_ACTIVE_CONTEXT_KEY, + type ForeachActiveContext, + type StepReviewSeamResult, + type WorkflowLegacySeams, +} from "../workflow-node-handlers.js"; +import { + makeAncestryBlastRadiusGuard, + resetStepToBaseline, + runTaskStep, + type StepRunnerTask, +} from "../step-runner.js"; + +const settingsOn = () => ({ experimentalFeatures: { workflowGraphExecutor: true } }); +const settingsOff = () => ({ experimentalFeatures: { workflowGraphExecutor: false } }); + +type Verdict = StepReviewSeamResult["verdict"]; + +/** One recorded projection write — the unit of trajectory comparison (KTD-7). */ +interface TrajectoryEntry { + step: number; + status: StepStatus; + source?: "graph"; +} + +/** + * A minimal fake store recording the `updateStep` projection trajectory and + * applying each write to an in-memory `steps[]` so the blast-radius guard's + * "later step already done" probe and the merge-blocker reads see real state. + * Implements only the surface `runTaskStep` / `resetStepToBaseline` touch. + */ +function makeFakeStore(steps: TaskStep[]) { + const trajectory: TrajectoryEntry[] = []; + return { + trajectory, + steps, + updateStep: async ( + _id: string, + stepIndex: number, + status: StepStatus, + options?: { source?: "graph" }, + ) => { + trajectory.push({ step: stepIndex, status, ...(options?.source ? { source: options.source } : {}) }); + if (steps[stepIndex]) steps[stepIndex] = { ...steps[stepIndex], status }; + return {} as never; + }, + logEntry: async () => {}, + }; +} + +/** Build a TaskDetail with N pending steps. */ +function taskWithSteps(n: number): TaskDetail { + const steps: TaskStep[] = Array.from({ length: n }, (_, i) => ({ + name: `Step ${i + 1}`, + status: "pending" as const, + })); + return { id: "FN-STEPWISE", steps } as unknown as TaskDetail; +} + +/** + * The legacy step-session ORACLE (KTD-9). Deterministic per-step loop modeling the + * in-session `fn_review_step` policy: for each step, mark in-progress, run, review; + * APPROVE → done, REVISE → re-run in place (no reset), RETHINK → reset to pending + + * re-run. Bounded by maxReworkCycles. Records the same TrajectoryEntry shape the + * graph side records — the legacy side never uses `source:"graph"`. + */ +async function runLegacyStepSession( + stepCount: number, + scripts: Verdict[][], + maxReworkCycles = 3, +): Promise { + const trajectory: TrajectoryEntry[] = []; + const steps: TaskStep[] = Array.from({ length: stepCount }, (_, i) => ({ + name: `Step ${i + 1}`, + status: "pending" as const, + })); + for (let i = 0; i < stepCount; i++) { + const verdicts = scripts[i] ?? ["APPROVE"]; + let cursor = 0; + let rework = 0; + for (;;) { + // run step i (mark in-progress) + trajectory.push({ step: i, status: "in-progress" }); + steps[i] = { ...steps[i], status: "in-progress" }; + const verdict = verdicts[Math.min(cursor, verdicts.length - 1)]; + cursor++; + if (verdict === "APPROVE") { + trajectory.push({ step: i, status: "done" }); + steps[i] = { ...steps[i], status: "done" }; + break; + } + if (verdict === "RETHINK") { + // reset to baseline: step → pending, then re-run. + trajectory.push({ step: i, status: "pending" }); + steps[i] = { ...steps[i], status: "pending" }; + } + // REVISE: re-run in place (no extra projection write — step stays in-progress + // on the next loop's in-progress write). + rework++; + if (rework > maxReworkCycles) { + // rework exhausted — step stays non-done (escalates). Mirror the graph's + // exhaustion: leave the last in-progress write as the terminal state. + break; + } + } + } + return trajectory; +} + +/** + * Drive the stepwise foreach graph (the REAL builtin IR) and capture the projection + * trajectory. Wires the substrate seams exactly as the executor does: + * - stepExecute → runTaskStep (markDoneOnSuccess driven by deferDoneToReview); + * - stepReview → scripted verdict; APPROVE marks the step done via updateStep + * (the projection authority, like createGraphSeams); + * - onReworkReset → resetStepToBaseline with the shared-isolation blast guard. + */ +async function runStepwiseGraph( + stepCount: number, + scripts: Verdict[][], + opts: { + maxReworkCycles?: number; + signal?: AbortSignal; + onReset?: (active: ForeachActiveContext) => void; + captureResetResult?: (ok: boolean, reason?: string) => void; + } = {}, +): Promise<{ trajectory: TrajectoryEntry[]; outcome: string; result: Awaited> }> { + const task = taskWithSteps(stepCount); + const fake = makeFakeStore(task.steps as TaskStep[]); + const reviewCursor = new Map(); + + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + stepExecute: async (_t, ctx) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + const result = await runTaskStep( + { + store: fake as never, + worktreePath: "/fake/worktree", + runStep: async () => ({ success: true }), + // Deterministic per-step baseline (substrate-captured, KTD-2). HEAD at + // instance start postdates steps 0..i-1's commits. + gitRevParse: async () => `sha-baseline-${active.stepIndex}`, + captureCheckpointId: () => `ckpt-${active.stepIndex}`, + }, + { id: task.id, steps: task.steps } as StepRunnerTask, + active.stepIndex, + { markDoneOnSuccess: active.deferDoneToReview !== true }, + ); + active.baselineSha = result.baselineSha; + active.checkpointId = result.checkpointId; + return { + outcome: result.outcome, + value: "step-done", + contextPatch: { [FOREACH_ACTIVE_CONTEXT_KEY]: active }, + }; + }, + stepReview: async (_t, ctx, _config) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + const verdicts = scripts[active.stepIndex] ?? ["APPROVE"]; + const cursor = reviewCursor.get(active.stepIndex) ?? 0; + reviewCursor.set(active.stepIndex, cursor + 1); + const verdict = verdicts[Math.min(cursor, verdicts.length - 1)]; + // APPROVE: the seam is the projection authority for done (createGraphSeams). + if (verdict === "APPROVE") { + await fake.updateStep(task.id, active.stepIndex, "done", { source: "graph" }); + } + return { verdict }; + }, + }; + + const executor = new WorkflowGraphExecutor({ + seams, + signal: opts.signal, + getTaskSteps: () => task.steps as TaskStep[], + // parse-steps reads PROMPT.md; produce headings matching the step count so the + // real builtin chain runs end-to-end. writeSteps is a no-op (steps pre-set). + parseStepsDeps: { + readArtifact: async () => + Array.from({ length: stepCount }, (_, i) => `### Step ${i + 1}: do ${i + 1}\n`).join("\n"), + writeSteps: async (_t, parsed) => { + // Mirror production: project the parsed list (all pending). Keep the + // in-memory steps array length authoritative for the run. + task.steps = parsed.length > 0 ? parsed : (task.steps as TaskStep[]); + }, + }, + onReworkReset: async (active) => { + opts.onReset?.(active); + const res = await resetStepToBaseline( + { + store: fake as never, + worktreePath: "/fake/worktree", + sessionRef: { current: null }, + reviewType: "code", + blastRadiusGuard: makeAncestryBlastRadiusGuard({ + worktreePath: "/fake/worktree", + task: { id: task.id, steps: task.steps } as StepRunnerTask, + stepIndex: active.stepIndex, + // Deterministic ancestry: the captured baseline is always an ancestor + // of HEAD on a clean scripted run. + isAncestor: async () => true, + }), + }, + { id: task.id, steps: task.steps } as StepRunnerTask, + active.stepIndex, + active.baselineSha, + active.checkpointId, + ); + opts.captureResetResult?.(res.ok, res.reason); + }, + }); + + const ir: WorkflowIr = BUILTIN_STEPWISE_CODING_WORKFLOW_IR; + // Override maxReworkCycles when the scenario needs a tighter budget. + const runIr = + opts.maxReworkCycles !== undefined ? withForeachMaxRework(ir, opts.maxReworkCycles) : ir; + + const result = await executor.run(task, settingsOn(), runIr); + return { trajectory: fake.trajectory, outcome: result.outcome, result }; +} + +/** Clone the IR with the foreach node's maxReworkCycles overridden (test only). */ +function withForeachMaxRework(ir: WorkflowIr, max: number): WorkflowIr { + const cloned = JSON.parse(JSON.stringify(ir)) as WorkflowIr; + for (const node of cloned.nodes) { + if (node.kind === "foreach" && node.config) { + (node.config as { maxReworkCycles?: number }).maxReworkCycles = max; + } + } + return cloned; +} + +/** Strip the `source` marker so the legacy (no-source) and graph trajectories are + * compared on (step, status) only — the projection content the merge-blocker and + * dashboard read (KTD-7). The graph side additionally carries `source:"graph"`. */ +function normalize(t: TrajectoryEntry[]): Array<{ step: number; status: StepStatus }> { + return t.map(({ step, status }) => ({ step, status })); +} + +describe("stepwise workflow parity (U7 / KTD-9)", () => { + // ── Trajectory parity vs the legacy step-session oracle ──────────────────── + + it("identical updateStep trajectory: 3-step approve-all (legacy step-session vs stepwise graph)", async () => { + const scripts: Verdict[][] = [["APPROVE"], ["APPROVE"], ["APPROVE"]]; + const legacy = await runLegacyStepSession(3, scripts); + const { trajectory, outcome } = await runStepwiseGraph(3, scripts); + + expect(outcome).toBe("success"); + expect(normalize(trajectory)).toEqual(normalize(legacy)); + // Concretely: each step in-progress then done, in order. + expect(normalize(trajectory)).toEqual([ + { step: 0, status: "in-progress" }, + { step: 0, status: "done" }, + { step: 1, status: "in-progress" }, + { step: 1, status: "done" }, + { step: 2, status: "in-progress" }, + { step: 2, status: "done" }, + ]); + }); + + it("revise-then-approve trajectory parity (revise re-runs in place, no reset)", async () => { + // Step 0: REVISE once then APPROVE. Step 1: APPROVE. + const scripts: Verdict[][] = [["REVISE", "APPROVE"], ["APPROVE"]]; + const legacy = await runLegacyStepSession(2, scripts); + const { trajectory, outcome } = await runStepwiseGraph(2, scripts); + + expect(outcome).toBe("success"); + expect(normalize(trajectory)).toEqual(normalize(legacy)); + // No `pending` write for step 0 (revise never resets). + expect(trajectory.some((e) => e.step === 0 && e.status === "pending")).toBe(false); + // Step 0 ran twice (in-progress ×2) then done once. + expect(trajectory.filter((e) => e.step === 0 && e.status === "in-progress").length).toBe(2); + }); + + it("RETHINK trajectory parity incl. reset to pending and baseline == agent-equivalent baseline (KTD-2)", async () => { + // Step 0: RETHINK once (resets) then APPROVE. + const scripts: Verdict[][] = [["RETHINK", "APPROVE"]]; + const legacy = await runLegacyStepSession(1, scripts); + const resetSeen: ForeachActiveContext[] = []; + const { trajectory, outcome } = await runStepwiseGraph(1, scripts, { + onReset: (active) => resetSeen.push({ ...active }), + }); + + expect(outcome).toBe("success"); + expect(normalize(trajectory)).toEqual(normalize(legacy)); + // A RETHINK resets to pending before re-execute. + expect(trajectory.some((e) => e.step === 0 && e.status === "pending")).toBe(true); + // The reset fired exactly once, with the substrate-captured baseline. KTD-2: + // HEAD-at-instance-start (`sha-baseline-0`) is exactly the agent-equivalent + // baseline (the boundary after steps 0..-1 = the start). Asserted here. + expect(resetSeen.length).toBe(1); + expect(resetSeen[0].baselineSha).toBe("sha-baseline-0"); + expect(resetSeen[0].checkpointId).toBe("ckpt-0"); + }); + + // ── RETHINK blast-radius guard (KTD-2) ───────────────────────────────────── + + it("RETHINK blast-radius guard REFUSES when a later step is already done", async () => { + // Directly exercise the production guard the graph wires: a reset for step 0 + // when step 1 is already `done` must be refused (would destroy approved work). + const steps: TaskStep[] = [ + { name: "Step 1", status: "pending" }, + { name: "Step 2", status: "done" }, // a LATER step already completed + ]; + const fake = makeFakeStore(steps); + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/fake/worktree", + task: { id: "FN-STEPWISE", steps } as StepRunnerTask, + stepIndex: 0, + isAncestor: async () => true, + }); + const res = await resetStepToBaseline( + { + store: fake as never, + worktreePath: "/fake/worktree", + sessionRef: { current: null }, + reviewType: "code", + blastRadiusGuard: guard, + }, + { id: "FN-STEPWISE", steps } as StepRunnerTask, + 0, + "sha-baseline-0", + "ckpt-0", + ); + + expect(res.ok).toBe(false); + expect(res.reason).toMatch(/later step/i); + // Refusal mutates NOTHING (no projection write at all). + expect(fake.trajectory.length).toBe(0); + }); + + // ── Lifecycle invariants on the stepwise path (R14) ──────────────────────── + + it("FN-5147 terminal-until-merged: stepwise run with merge failure stays out of done", async () => { + // autoMerge:false → the merge seam fails (manual-merge-required); the task + // never routes to merge success, so it stays terminal-in-review until merged. + const task = taskWithSteps(1); + const fake = makeFakeStore(task.steps as TaskStep[]); + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + // FN-5147: autoMerge:false surfaces as a merge-blocking failure value. + merge: async () => ({ outcome: "failure", value: "manual-merge-required" }), + schedule: async () => ({ outcome: "success" }), + stepExecute: async (_t, ctx) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + await runTaskStep( + { + store: fake as never, + worktreePath: "/fake/worktree", + runStep: async () => ({ success: true }), + gitRevParse: async () => "sha", + captureCheckpointId: () => "ckpt", + }, + { id: task.id, steps: task.steps } as StepRunnerTask, + active.stepIndex, + { markDoneOnSuccess: active.deferDoneToReview !== true }, + ); + return { outcome: "success", value: "step-done" }; + }, + stepReview: async (_t, ctx) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + await fake.updateStep(task.id, active.stepIndex, "done", { source: "graph" }); + return { verdict: "APPROVE" } as StepReviewSeamResult; + }, + }; + const executor = new WorkflowGraphExecutor({ + seams, + getTaskSteps: () => task.steps as TaskStep[], + parseStepsDeps: { + readArtifact: async () => "### Step 1: do it\n", + writeSteps: async () => {}, + }, + }); + const result = await executor.run(task, settingsOn(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.outcome).toBe("failure"); + // The walk never reached `end` through merge — terminal-until-merged preserved. + expect(result.visitedNodeIds).not.toContain("end"); + // All step work completed (the step is done) — the blocker is the merge, not steps. + expect((task.steps as TaskStep[])[0].status).toBe("done"); + }); + + it("hard-cancel mid-instance: abort signal halts the foreach cleanly (no further step work)", async () => { + const controller = new AbortController(); + const ran: number[] = []; + const task = taskWithSteps(3); + const fake = makeFakeStore(task.steps as TaskStep[]); + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + stepExecute: async (_t, ctx) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + ran.push(active.stepIndex); + await fake.updateStep(task.id, active.stepIndex, "in-progress", { source: "graph" }); + // Simulate a hard-cancel (moveTask in-progress→todo) mid-first-instance. + if (active.stepIndex === 0) controller.abort(); + return { outcome: "success", value: "step-done" }; + }, + stepReview: async () => ({ verdict: "APPROVE" }) as StepReviewSeamResult, + }; + const executor = new WorkflowGraphExecutor({ + seams, + signal: controller.signal, + getTaskSteps: () => task.steps as TaskStep[], + parseStepsDeps: { + readArtifact: async () => "### Step 1: a\n### Step 2: b\n### Step 3: c\n", + writeSteps: async () => {}, + }, + }); + const result = await executor.run(task, settingsOn(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.outcome).toBe("failure"); + // Only the first instance started; later instances never ran (clean cancel). + expect(ran).toEqual([0]); + }); + + it("file-scope guard fires inside step-execute: a step-execute failure value propagates (no merge)", async () => { + // The file-scope guard surfaces as a step-execute failure (the session commit + // is rejected). The foreach must route failure — NOT silently approve/merge. + const task = taskWithSteps(2); + const fake = makeFakeStore(task.steps as TaskStep[]); + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + stepExecute: async (_t, ctx) => { + const active = ctx[FOREACH_ACTIVE_CONTEXT_KEY] as ForeachActiveContext; + await fake.updateStep(task.id, active.stepIndex, "in-progress", { source: "graph" }); + // Step 0 violates file scope. + if (active.stepIndex === 0) { + return { outcome: "failure", value: "FileScopeViolationError" }; + } + return { outcome: "success", value: "step-done" }; + }, + stepReview: async () => ({ verdict: "APPROVE" }) as StepReviewSeamResult, + }; + const executor = new WorkflowGraphExecutor({ + seams, + getTaskSteps: () => task.steps as TaskStep[], + parseStepsDeps: { + readArtifact: async () => "### Step 1: a\n### Step 2: b\n", + writeSteps: async () => {}, + }, + }); + const result = await executor.run(task, settingsOn(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.outcome).toBe("failure"); + // Step 0 never reached `done` (the guard blocked it); step 1 never ran. + expect((task.steps as TaskStep[])[0].status).toBe("in-progress"); + expect((task.steps as TaskStep[])[1].status).toBe("pending"); + expect(result.visitedNodeIds).not.toContain("merge"); + }); + + // ── Flag posture (R10) ───────────────────────────────────────────────────── + + it("flag pinned-at-dispatch: flag OFF → graph executor is a strict no-op (legacy path owns the run)", async () => { + // With the flag OFF at dispatch, the graph executor does not run at all — the + // legacy step-session path owns the task. Toggling the flag mid-run cannot + // switch paths because the run never entered the graph. + const task = taskWithSteps(2); + let stepExecuteCalls = 0; + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + stepExecute: async () => { + stepExecuteCalls++; + return { outcome: "success", value: "step-done" }; + }, + }; + const executor = new WorkflowGraphExecutor({ seams }); + const result = await executor.run(task, settingsOff(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.executed).toBe(false); + expect(result.outcome).toBe("failure"); + expect(stepExecuteCalls).toBe(0); + }); + + it("OFF-rollback: a stepwise run with the flag OFF leaves steps[] (git-reconcilable) as surviving truth", async () => { + // KTD-8 OFF-rollback: instance rows are swept and steps[] — always + // git-reconcilable — is the surviving truth that legacy resume reconciles + // from. With the flag OFF the graph never writes, so the pre-existing steps[] + // projection (legacy's truth) is untouched; legacy resume then completes. + const task = taskWithSteps(2); + // Simulate a partially-progressed legacy projection (step 0 done by legacy). + (task.steps as TaskStep[])[0] = { name: "Step 1", status: "done" }; + const executor = new WorkflowGraphExecutor({ seams: undefined }); + const result = await executor.run(task, settingsOff(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.executed).toBe(false); + // steps[] is untouched by the (no-op) graph — legacy's projection survives. + expect((task.steps as TaskStep[])[0].status).toBe("done"); + expect((task.steps as TaskStep[])[1].status).toBe("pending"); + }); + + // ── Zero-step task (R8) ──────────────────────────────────────────────────── + + it("zero-step task on stepwise merges without step work (no-steps outcome path)", async () => { + let stepExecuteCalls = 0; + const task = taskWithSteps(0); + const seams: WorkflowLegacySeams = { + planning: async () => ({ outcome: "success" }), + execute: async () => ({ outcome: "success" }), + review: async () => ({ outcome: "success" }), + merge: async () => ({ outcome: "success" }), + schedule: async () => ({ outcome: "success" }), + stepExecute: async () => { + stepExecuteCalls++; + return { outcome: "success", value: "step-done" }; + }, + }; + const executor = new WorkflowGraphExecutor({ + seams, + getTaskSteps: () => [], + parseStepsDeps: { + // No headings → zero steps → parse-steps routes outcome:no-steps → foreach + // no-ops through its success edge (R8). + readArtifact: async () => "no steps here, just prose", + writeSteps: async () => {}, + }, + }); + const result = await executor.run(task, settingsOn(), BUILTIN_STEPWISE_CODING_WORKFLOW_IR); + + expect(result.outcome).toBe("success"); + expect(stepExecuteCalls).toBe(0); + // The foreach was reached but expanded zero instances. + expect(result.visitedNodeIds).toContain("steps"); + expect(result.visitedNodeIds.some((id) => id.startsWith("steps#"))).toBe(false); + // Merge ran (the task merges with no step work). + expect(result.visitedNodeIds).toContain("merge"); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts b/packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts index 631138786b..885eac385d 100644 --- a/packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts +++ b/packages/engine/src/__tests__/workflow-graph-executor-parity.test.ts @@ -1,3 +1,14 @@ +// ───────────────────────────────────────────────────────────────────────────── +// PARITY SUBJECT (test-file ownership, U7 / KTD-9): +// This suite owns DEFAULT-WORKFLOW BYTE-IDENTITY parity — it proves the graph +// executor reproduces the legacy monolithic execute → review → merge seam +// sequence exactly (the parity ORACLE per KTD-1). It deliberately does NOT +// cover per-step / updateStep-trajectory parity. +// +// The stepwise per-step trajectory + merge-blocker-window parity (legacy +// step-session path vs the stepwise foreach graph) is owned by the sibling +// suite `stepwise-workflow-parity.test.ts`. Keep the two concerns separate. +// ───────────────────────────────────────────────────────────────────────────── import { describe, expect, it, vi } from "vitest"; import type { TaskDetail } from "@fusion/core"; diff --git a/packages/engine/src/agent-tools.ts b/packages/engine/src/agent-tools.ts index 1b05ae665f..fca5fde9a6 100644 --- a/packages/engine/src/agent-tools.ts +++ b/packages/engine/src/agent-tools.ts @@ -1137,7 +1137,22 @@ export function createWorkflowCreateTool(store: TaskStore): ToolDefinition { label: "Create Workflow", description: "Create a new custom workflow definition from a name and a workflow graph (IR). " + - "The IR is validated server-side. Returns the new workflow ID.", + "The IR is validated server-side; a malformed graph rejects. Returns the new workflow ID.\n" + + "v2 IR supports step-inversion constructs (all additive, opt-in): " + + "`parse-steps` node {artifact, parser} writes the task step list from a declared artifact " + + "(built-in parsers: `step-headings`, `json-steps`; routable `no-steps`/`parse-error` outcomes) — " + + "it must precede any `foreach`; " + + "`foreach` node {source:'task-steps', template:{nodes,edges}, mode:'sequential'|'parallel', " + + "isolation:'shared'|'worktree', concurrency (parallel only, 1-8), maxReworkCycles (1-10)} " + + "instantiates its single-entry/exit template subgraph once per planned step " + + "(parallel+shared is rejected); a `step-execute` node is legal only inside a foreach template; " + + "`step-review` node {type:'plan'|'code', model?} surfaces verdicts as outcome edges " + + "(`outcome:approve|revise|rethink|unavailable`); edges may set `kind:'rework'` (the only legal cycles, " + + "back to step-execute within an instance; rethink edges trigger a reset-to-baseline); " + + "`code` node {source, timeoutMs?} runs sandboxed TypeScript returning {outcome?, contextPatch?, customFields?}. " + + "Declare task documents via `artifacts: [{key, title?, producedBy?, role?}]` and custom task fields via " + + "`fields: [{id, name, type, required?, default?, options?, render?}]` (types: string/text/number/boolean/" + + "enum/multi-enum/date/url; render.placement card|detail|detail-section, render.badge for card chips).", parameters: workflowCreateParams, execute: async (_id: string, params: Static) => { try { @@ -1178,7 +1193,10 @@ export function createWorkflowUpdateTool(store: TaskStore): ToolDefinition { description: "Update a custom workflow definition (name/description/ir/layout). Built-ins cannot be edited. " + "If an IR change removes a column that still holds cards, the update is blocked and returns the " + - "occupied columns — retry with rehome_to set to a column id that survives in the new IR.", + "occupied columns — retry with rehome_to set to a column id that survives in the new IR. " + + "The IR accepts the same step-inversion constructs as fn_workflow_create (foreach with mode/isolation/" + + "concurrency, step-execute, step-review, parse-steps, code nodes, rework edges, artifacts, fields). " + + "Editing `fields` orphans (never destroys) existing task values for removed/incompatible fields.", parameters: workflowUpdateParams, execute: async (_id: string, params: Static) => { try { diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index f3aec442f8..9165f19e2e 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -717,14 +717,21 @@ export async function __runConfiguredCommandForTests( // ── Tool parameter schemas (module-level for reuse in ToolDefinition generics) ── const taskUpdateParams = Type.Object({ - step: Type.Number({ description: "Step number (1-indexed)" }), - status: Type.Union( + step: Type.Optional(Type.Number({ description: "Step number (1-indexed). Omit when updating only custom_fields/dependencies." })), + status: Type.Optional(Type.Union( STEP_STATUSES.map((s) => Type.Literal(s)), - { description: "New status: pending, in-progress, done, or skipped" }, - ), + { description: "New status: pending, in-progress, done, or skipped. Required when step is set." }, + )), dependencies: Type.Optional(Type.Array(Type.String(), { description: "Optional task dependency array. Replaces existing dependencies. Pass ['FN-001', 'FN-002'] to set dependencies. Pass [] to clear all dependencies. Omit parameter to preserve existing dependencies.", })), + custom_fields: Type.Optional(Type.Record(Type.String(), Type.Unknown(), { + description: + "Optional patch of workflow-defined custom field values, keyed by field id. " + + "Values are validated against the task's workflow field schema (type/enum membership); " + + "pass null for a field to clear it. Rejected writes return the offending field id and reason. " + + "Only fields declared by the task's workflow may be written.", + })), }); // taskLogParams and taskCreateParams are imported from agent-tools.ts @@ -7088,10 +7095,44 @@ export class TaskExecutor { "Update a step's status. Call before starting a step (in-progress), " + "after completing it (done), or to skip it (skipped). " + "Optionally update task dependencies by passing a dependencies array. " + + "Optionally set workflow-defined custom field values by passing a custom_fields patch " + + "(keyed by field id; validated against the workflow's field schema; pass null to clear a field). " + + "step/status may be omitted to update only custom_fields or dependencies. " + "The board updates in real-time.", parameters: taskUpdateParams, execute: async (_id: string, params: Static) => { - const { step, status, dependencies } = params; + const { step, status, dependencies, custom_fields } = params; + + // Custom-field patch (KTD-13): routed through the store's single write + // authority, which validates each value against the task's workflow field + // schema. A typed rejection surfaces the offending field id + reason as a + // tool error so the agent can correct it. Applied first so a field-only + // call (step omitted) returns here. + if (custom_fields !== undefined) { + const res = await store.updateTaskCustomFields(taskId, custom_fields); + if (!res.ok) { + const r = res.rejection; + return { + content: [{ + type: "text" as const, + text: `ERROR: custom field '${r.fieldId}' rejected (${r.code}): ${r.detail}`, + }], + details: { fieldId: r.fieldId, code: r.code, detail: r.detail }, + isError: true, + }; + } + // A custom-fields-only update (no step) succeeds here. + if (step === undefined && status === undefined && dependencies === undefined) { + const updatedKeys = Object.keys(custom_fields); + return { + content: [{ + type: "text" as const, + text: `Updated custom field(s): ${updatedKeys.join(", ")}.`, + }], + details: { updatedFields: updatedKeys }, + }; + } + } // Record step progress for stuck task detection. // Step transitions (in-progress, done, skipped) indicate real progress @@ -7101,6 +7142,44 @@ export class TaskExecutor { stuckDetector?.recordProgress(taskId); } + // Dependencies-only update (no step) is permitted; handle deps then return. + if (step === undefined) { + if (dependencies !== undefined) { + if (dependencies.includes(taskId)) { + return { + content: [{ type: "text" as const, text: `Cannot add self-dependency: ${taskId} cannot depend on itself.` }], + details: {}, + }; + } + const invalidIds: string[] = []; + for (const depId of dependencies) { + try { await store.getTask(depId); } catch { invalidIds.push(depId); } + } + if (invalidIds.length > 0) { + return { + content: [{ type: "text" as const, text: `Cannot set dependencies — the following task(s) do not exist: ${invalidIds.join(", ")}` }], + details: {}, + }; + } + await store.updateTask(taskId, { dependencies }); + return { + content: [{ type: "text" as const, text: `Dependencies updated.` }], + details: {}, + }; + } + return { + content: [{ type: "text" as const, text: `No-op: provide a step+status, dependencies, or custom_fields to update.` }], + details: {}, + }; + } + + if (status === undefined) { + return { + content: [{ type: "text" as const, text: `Step ${step} provided without a status. Pass status (pending/in-progress/done/skipped).` }], + details: {}, + }; + } + if (!Number.isInteger(step) || step < 1) { return { content: [{ diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 5b33f6e9b9..07a13fc8f2 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -139,6 +139,12 @@ Follow this structure exactly: ## Steps +> Optional: a step heading may carry a \`(depends: N,M)\` annotation listing the 1-indexed +> step numbers it depends on — e.g. \`### Step 3 (depends: 1): Title\`. Annotate ONLY steps +> that are genuinely independent of their immediate predecessor; an unannotated step is +> assumed to depend on the one before it (fully sequential). Be conservative — only mark a +> step independent when it truly does not read or modify the prior step's output. + ### Step 0: Preflight - [ ] Required files and paths exist @@ -437,6 +443,12 @@ Follow this structure exactly: ## Steps +> Optional: a step heading may carry a \`(depends: N,M)\` annotation listing the 1-indexed +> step numbers it depends on — e.g. \`### Step 3 (depends: 1): Title\`. Annotate ONLY steps +> that are genuinely independent of their immediate predecessor; an unannotated step is +> assumed to depend on the one before it (fully sequential). Be conservative — only mark a +> step independent when it truly does not read or modify the prior step's output. + ### Step 0: Preflight - [ ] Required files and paths exist