From 2a3c285bae8077b1c8dfe09f3572f5924eb225cc Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 11:33:33 -0700 Subject: [PATCH 01/22] =?UTF-8?q?feat(core):=20U1=20=E2=80=94=20IR=20forea?= =?UTF-8?q?ch/step-review/parse-steps/code=20kinds,=20rework=20edges,=20de?= =?UTF-8?q?pendsOn=20parsing=20(FN=20step-inversion)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/src/__tests__/store-parsing.test.ts | 100 ++- .../src/__tests__/workflow-ir-foreach.test.ts | 551 ++++++++++++++++ packages/core/src/store.ts | 89 ++- packages/core/src/types.ts | 5 + packages/core/src/workflow-ir-types.ts | 83 ++- packages/core/src/workflow-ir.ts | 598 +++++++++++++++++- 6 files changed, 1413 insertions(+), 13 deletions(-) create mode 100644 packages/core/src/__tests__/workflow-ir-foreach.test.ts diff --git a/packages/core/src/__tests__/store-parsing.test.ts b/packages/core/src/__tests__/store-parsing.test.ts index cea876e75b..6f407c9793 100644 --- a/packages/core/src/__tests__/store-parsing.test.ts +++ b/packages/core/src/__tests__/store-parsing.test.ts @@ -6,7 +6,7 @@ import { existsSync } from "node:fs"; import * as projectMemory from "../project-memory.js"; import { AgentStore } from "../agent-store.js"; import { CentralDatabase } from "../central-db.js"; -import { InvalidFileScopeError, isValidFileScopeEntry, TaskStore, TaskHasDependentsError } from "../store.js"; +import { InvalidFileScopeError, isValidFileScopeEntry, parseStepHeadings, TaskStore, TaskHasDependentsError } from "../store.js"; import { buildResearchDocumentKey, type Task } from "../types.js"; import { createTaskStoreTestHarness, makeTmpDir } from "./store-test-helpers.js"; @@ -41,6 +41,104 @@ describe("TaskStore", () => { const steps = await store.parseStepsFromPrompt(task.id); expect(steps).toEqual([]); }); + + it("parses depends annotations from PROMPT.md (1-indexed → 0-indexed)", async () => { + const task = await store.createTask({ description: "Task with depends" }); + const dir = join(rootDir, ".fusion", "tasks", task.id); + await writeFile( + join(dir, "PROMPT.md"), + `# ${task.id}: Task + +## Steps + +### Step 1: First + +### Step 2 (depends: 1): Second + +### Step 3 (depends: 1,2): Third +`, + ); + const steps = await store.parseStepsFromPrompt(task.id); + expect(steps).toEqual([ + { name: "First", status: "pending" }, + { name: "Second", status: "pending", dependsOn: [0] }, + { name: "Third", status: "pending", dependsOn: [0, 1] }, + ]); + }); + }); + + describe("parseStepHeadings (step-inversion U1)", () => { + it("parses unannotated headings byte-identically to the legacy regex", () => { + const content = `## Steps + +### Step 0: Preflight + +- [ ] x + +### Step 1: Implementation + +### Step 2: Testing +`; + // The legacy behavior: name = text after the first colon, trimmed; no dependsOn. + expect(parseStepHeadings(content)).toEqual([ + { name: "Preflight", status: "pending" }, + { name: "Implementation", status: "pending" }, + { name: "Testing", status: "pending" }, + ]); + }); + + it("matches the legacy regex output exactly for varied unannotated headings", () => { + const content = [ + "### Step 0: A", + "### Step 12: Multi word title", + "### Step 3 — dash but no annotation: Real Name", + "### Step 4: trailing spaces here ", + "### Step 5 no colon at all", + "not a step heading: ignored", + ].join("\n"); + // Reference: the original regex. + const legacy: { name: string; status: "pending" }[] = []; + const re = /^###\s+Step\s+\d+[^:]*:\s*(.+)$/gm; + let m: RegExpExecArray | null; + while ((m = re.exec(content)) !== null) { + legacy.push({ name: m[1].trim(), status: "pending" }); + } + expect(parseStepHeadings(content)).toEqual(legacy); + }); + + it("parses (depends: 1,2) into 0-indexed dependsOn", () => { + expect(parseStepHeadings("### Step 3 (depends: 1,2): Title")).toEqual([ + { name: "Title", status: "pending", dependsOn: [0, 1] }, + ]); + }); + + it("dedupes and sorts depends values", () => { + expect(parseStepHeadings("### Step 5 (depends: 3,1,3,2): T")).toEqual([ + { name: "T", status: "pending", dependsOn: [0, 1, 2] }, + ]); + }); + + it("empty depends list yields no dependsOn", () => { + expect(parseStepHeadings("### Step 2 (depends: ): T")).toEqual([ + { name: "T", status: "pending" }, + ]); + }); + + it("falls back deterministically on a malformed depends annotation (name after colon following the paren)", () => { + // 'bad' is not a positive integer → fallback: name starts after the colon + // following the closing paren. + expect(parseStepHeadings("### Step 1 (depends: bad): Real Title")).toEqual([ + { name: "Real Title", status: "pending" }, + ]); + }); + + it("falls back deterministically when the annotation has no closing paren", () => { + // No closing paren → name starts after the FIRST colon (inside `depends:`), + // per the documented deterministic fallback. + expect(parseStepHeadings("### Step 1 (depends: 1,2 oops: Title")).toEqual([ + { name: "1,2 oops: Title", status: "pending" }, + ]); + }); }); diff --git a/packages/core/src/__tests__/workflow-ir-foreach.test.ts b/packages/core/src/__tests__/workflow-ir-foreach.test.ts new file mode 100644 index 0000000000..2bb114a9f6 --- /dev/null +++ b/packages/core/src/__tests__/workflow-ir-foreach.test.ts @@ -0,0 +1,551 @@ +import { describe, expect, it } from "vitest"; +import { + parseWorkflowIr, + serializeWorkflowIr, + downgradeIrToV1IfPure, + WorkflowIrError, +} from "../workflow-ir.js"; +import type { + WorkflowIrEdge, + WorkflowIrNode, + WorkflowIrV2, +} from "../workflow-ir-types.js"; + +// Step-inversion (U1) — foreach / step-review / parse-steps / code / rework / +// fields validation. + +const defaultColumns: WorkflowIrV2["columns"] = [ + { id: "todo", name: "todo", traits: [] }, + { id: "in-progress", name: "in-progress", traits: [] }, +]; + +function v2( + nodes: WorkflowIrNode[], + edges: WorkflowIrEdge[], + extra: Partial = {}, +): WorkflowIrV2 { + return { version: "v2", name: "test", columns: defaultColumns, nodes, edges, ...extra }; +} + +/** A minimal valid foreach template: step-execute → step-review(approve→exit). */ +function stepTemplate(): { nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[] } { + return { + nodes: [ + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "rev", kind: "step-review", config: { type: "code" } }, + { id: "exit", kind: "prompt" }, + ], + edges: [ + { from: "se", to: "rev" }, + { from: "rev", to: "exit", condition: "outcome:approve" }, + { from: "rev", to: "se", condition: "outcome:revise", kind: "rework" }, + ], + }; +} + +/** A graph: start → parse-steps → foreach → end. */ +function graphWithForeach( + foreachConfig: Record, + extra: Partial = {}, +): WorkflowIrV2 { + return v2( + [ + { id: "start", kind: "start" }, + { id: "ps", kind: "parse-steps", config: { artifact: "PROMPT.md", parser: "step-headings" } }, + { id: "fe", kind: "foreach", config: { source: "task-steps", template: stepTemplate(), ...foreachConfig } }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "ps" }, + { from: "ps", to: "fe" }, + { from: "fe", to: "end" }, + ], + extra, + ); +} + +describe("foreach validation", () => { + it("parses a valid foreach dominated by parse-steps", () => { + const ir = parseWorkflowIr(graphWithForeach({})) as WorkflowIrV2; + expect(ir.version).toBe("v2"); + const fe = ir.nodes.find((n) => n.id === "fe")!; + expect(fe.kind).toBe("foreach"); + }); + + it("rejects foreach with empty template", () => { + const ir = graphWithForeach({ template: { nodes: [], edges: [] } }); + expect(() => parseWorkflowIr(ir)).toThrow(/non-empty/); + }); + + it("rejects template with two entry nodes", () => { + const tmpl = { + nodes: [ + { id: "a", kind: "prompt", config: { seam: "step-execute" } }, + { id: "b", kind: "prompt" }, + ] as WorkflowIrNode[], + edges: [] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /exactly one entry/, + ); + }); + + it("rejects template with two exit nodes", () => { + const tmpl = { + nodes: [ + { id: "a", kind: "prompt", config: { seam: "step-execute" } }, + { id: "b", kind: "prompt" }, + { id: "c", kind: "prompt" }, + ] as WorkflowIrNode[], + edges: [ + { from: "a", to: "b" }, + { from: "a", to: "c" }, + ] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /exactly one (entry|exit)/, + ); + }); + + it("rejects nested foreach in a template", () => { + const tmpl = { + nodes: [ + { id: "inner", kind: "foreach", config: { source: "task-steps", template: stepTemplate() } }, + ] as WorkflowIrNode[], + edges: [] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /nested foreach/, + ); + }); + + it("rejects step-execute at the top level", () => { + const ir = v2( + [ + { id: "start", kind: "start" }, + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "se" }, + { from: "se", to: "end" }, + ], + ); + expect(() => parseWorkflowIr(ir)).toThrow(/only legal inside a foreach template/); + }); + + it("rejects step-execute inside a split branch (extends SEAM_FORBIDDEN_IN_BRANCH)", () => { + const tmpl = { + nodes: [ + { id: "split", kind: "split" }, + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "other", kind: "prompt" }, + { id: "join", kind: "join" }, + ] as WorkflowIrNode[], + edges: [ + { from: "split", to: "se" }, + { from: "split", to: "other" }, + { from: "se", to: "join" }, + { from: "other", to: "join" }, + ] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /step-execute.*forbidden inside a parallel branch/, + ); + }); + + it("rejects a rework edge crossing the template boundary", () => { + const tmpl = stepTemplate(); + // Point the rework edge at a node outside the template. + tmpl.edges = tmpl.edges.map((e) => + e.kind === "rework" ? { ...e, to: "end" } : e, + ); + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /both endpoints inside the same template/, + ); + }); + + it("rejects a top-level rework edge", () => { + const ir = v2( + [ + { id: "start", kind: "start" }, + { id: "a", kind: "prompt" }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "a" }, + { from: "a", to: "end" }, + { from: "end", to: "a", kind: "rework" }, + ], + ); + expect(() => parseWorkflowIr(ir)).toThrow(/only legal inside a foreach template/); + }); + + it("rejects foreach not dominated by a parse-steps node", () => { + const ir = v2( + [ + { id: "start", kind: "start" }, + { id: "fe", kind: "foreach", config: { source: "task-steps", template: stepTemplate() } }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "fe" }, + { from: "fe", to: "end" }, + ], + ); + expect(() => parseWorkflowIr(ir)).toThrow(/must be dominated by a parse-steps node/); + }); + + it("rejects foreach when parse-steps is only on one branch (not all paths)", () => { + // start → split into (ps→join) and (direct→join), join → fe. + const ir = v2( + [ + { id: "start", kind: "start" }, + { id: "split", kind: "split" }, + { id: "ps", kind: "parse-steps", config: { artifact: "PROMPT.md", parser: "step-headings" } }, + { id: "direct", kind: "prompt" }, + { id: "join", kind: "join" }, + { id: "fe", kind: "foreach", config: { source: "task-steps", template: stepTemplate() } }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "split" }, + { from: "split", to: "ps" }, + { from: "split", to: "direct" }, + { from: "ps", to: "join" }, + { from: "direct", to: "join" }, + { from: "join", to: "fe" }, + { from: "fe", to: "end" }, + ], + ); + expect(() => parseWorkflowIr(ir)).toThrow(/must be dominated by a parse-steps node/); + }); +}); + +describe("foreach mode / isolation / concurrency", () => { + it("rejects parallel + shared", () => { + expect(() => + parseWorkflowIr(graphWithForeach({ mode: "parallel", isolation: "shared" })), + ).toThrow(/cannot combine mode 'parallel' with isolation 'shared'/); + }); + + it("accepts parallel + worktree", () => { + expect(() => + parseWorkflowIr(graphWithForeach({ mode: "parallel", isolation: "worktree", concurrency: 4 })), + ).not.toThrow(); + }); + + it("rejects concurrency on sequential mode", () => { + expect(() => + parseWorkflowIr(graphWithForeach({ mode: "sequential", concurrency: 2 })), + ).toThrow(/concurrency is only valid in 'parallel' mode/); + }); + + it("rejects concurrency out of range", () => { + expect(() => + parseWorkflowIr(graphWithForeach({ mode: "parallel", isolation: "worktree", concurrency: 9 })), + ).toThrow(/concurrency must be an integer in 1\.\.8/); + expect(() => + parseWorkflowIr(graphWithForeach({ mode: "parallel", isolation: "worktree", concurrency: 0 })), + ).toThrow(/concurrency must be an integer in 1\.\.8/); + }); +}); + +describe("foreach maxReworkCycles clamp", () => { + it("rejects maxReworkCycles < 1", () => { + expect(() => parseWorkflowIr(graphWithForeach({ maxReworkCycles: 0 }))).toThrow( + /maxReworkCycles must be an integer >= 1/, + ); + }); + + it("clamps maxReworkCycles > 10 to 10", () => { + const ir = parseWorkflowIr(graphWithForeach({ maxReworkCycles: 99 })) as WorkflowIrV2; + const fe = ir.nodes.find((n) => n.id === "fe")!; + expect((fe.config as { maxReworkCycles: number }).maxReworkCycles).toBe(10); + }); + + it("keeps maxReworkCycles <= 10 unchanged", () => { + const ir = parseWorkflowIr(graphWithForeach({ maxReworkCycles: 5 })) as WorkflowIrV2; + const fe = ir.nodes.find((n) => n.id === "fe")!; + expect((fe.config as { maxReworkCycles: number }).maxReworkCycles).toBe(5); + }); +}); + +describe("step-review verdict routing", () => { + function templateWithReview(reviewEdges: WorkflowIrEdge[]): WorkflowIrV2 { + const tmpl = { + nodes: [ + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "rev", kind: "step-review", config: { type: "plan" } }, + { id: "exit", kind: "prompt" }, + ] as WorkflowIrNode[], + edges: [{ from: "se", to: "rev" }, ...reviewEdges], + }; + return graphWithForeach({ template: tmpl }); + } + + it("rejects step-review missing approve routing", () => { + expect(() => + parseWorkflowIr( + templateWithReview([ + { from: "rev", to: "se", condition: "outcome:revise", kind: "rework" }, + { from: "rev", to: "exit", condition: "outcome:other" }, + ]), + ), + ).toThrow(/must route outcome:approve/); + }); + + it("rejects step-review missing revise routing", () => { + expect(() => + parseWorkflowIr( + templateWithReview([{ from: "rev", to: "exit", condition: "outcome:approve" }]), + ), + ).toThrow(/must route outcome:revise/); + }); + + it("accepts approve+revise routing (rethink optional)", () => { + expect(() => + parseWorkflowIr( + templateWithReview([ + { from: "rev", to: "exit", condition: "outcome:approve" }, + { from: "rev", to: "se", condition: "outcome:revise", kind: "rework" }, + ]), + ), + ).not.toThrow(); + }); + + it("rejects a verdict-authoring step-review inside a split branch (advisory-only)", () => { + const tmpl = { + nodes: [ + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "split", kind: "split" }, + { id: "advrev", kind: "step-review", config: { type: "code" } }, + { id: "other", kind: "prompt" }, + { id: "join", kind: "join" }, + { id: "exit", kind: "prompt" }, + ] as WorkflowIrNode[], + edges: [ + { from: "se", to: "split" }, + { from: "split", to: "advrev" }, + { from: "split", to: "other" }, + // advisory review illegally carries approve routing + { from: "advrev", to: "join", condition: "outcome:approve" }, + { from: "other", to: "join" }, + { from: "join", to: "exit" }, + ] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).toThrow( + /advisory-only/, + ); + }); + + it("accepts an advisory step-review inside a split branch without verdict routing", () => { + const tmpl = { + nodes: [ + { id: "se", kind: "prompt", config: { seam: "step-execute" } }, + { id: "split", kind: "split" }, + { id: "advrev", kind: "step-review", config: { type: "code" } }, + { id: "other", kind: "prompt" }, + { id: "join", kind: "join" }, + { id: "rev", kind: "step-review", config: { type: "code" } }, + { id: "exit", kind: "prompt" }, + ] as WorkflowIrNode[], + edges: [ + { from: "se", to: "split" }, + { from: "split", to: "advrev" }, + { from: "split", to: "other" }, + { from: "advrev", to: "join" }, + { from: "other", to: "join" }, + { from: "join", to: "rev" }, + { from: "rev", to: "exit", condition: "outcome:approve" }, + { from: "rev", to: "se", condition: "outcome:revise", kind: "rework" }, + ] as WorkflowIrEdge[], + }; + expect(() => parseWorkflowIr(graphWithForeach({ template: tmpl }))).not.toThrow(); + }); +}); + +describe("parse-steps validation", () => { + it("rejects parse-steps with empty parser", () => { + const ir = graphWithForeach({}); + (ir.nodes.find((n) => n.id === "ps")!.config as Record).parser = ""; + expect(() => parseWorkflowIr(ir)).toThrow(/non-empty parser/); + }); + + it("rejects parse-steps referencing an undeclared artifact", () => { + const ir = graphWithForeach({}, { artifacts: [{ key: "OTHER.md" }] }); + expect(() => parseWorkflowIr(ir)).toThrow(/undeclared artifact 'PROMPT.md'/); + }); + + it("accepts parse-steps referencing a declared artifact", () => { + const ir = graphWithForeach({}, { artifacts: [{ key: "PROMPT.md", role: "step-source" }] }); + expect(() => parseWorkflowIr(ir)).not.toThrow(); + }); + + it("allows only PROMPT.md when no artifacts are declared", () => { + const ir = graphWithForeach({}); + (ir.nodes.find((n) => n.id === "ps")!.config as Record).artifact = "SPEC.md"; + expect(() => parseWorkflowIr(ir)).toThrow(/only 'PROMPT.md' is allowed/); + }); +}); + +describe("code node validation", () => { + function graphWithCode(config: Record): WorkflowIrV2 { + return v2( + [ + { id: "start", kind: "start" }, + { id: "c", kind: "code", config }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "c" }, + { from: "c", to: "end" }, + ], + ); + } + + it("rejects empty source", () => { + expect(() => parseWorkflowIr(graphWithCode({ source: "" }))).toThrow(/non-empty source/); + }); + + it("rejects source over 64KB", () => { + expect(() => parseWorkflowIr(graphWithCode({ source: "x".repeat(65537) }))).toThrow( + /exceeds 65536/, + ); + }); + + it("accepts valid source and timeout", () => { + expect(() => + parseWorkflowIr(graphWithCode({ source: "export default async () => ({})", timeoutMs: 30000 })), + ).not.toThrow(); + }); + + it("rejects timeoutMs out of range", () => { + expect(() => parseWorkflowIr(graphWithCode({ source: "x", timeoutMs: 999 }))).toThrow( + /timeoutMs must be an integer in 1000\.\.300000/, + ); + expect(() => parseWorkflowIr(graphWithCode({ source: "x", timeoutMs: 300001 }))).toThrow( + /timeoutMs must be an integer in 1000\.\.300000/, + ); + }); +}); + +describe("fields validation", () => { + function graphWithFields(fields: unknown): WorkflowIrV2 { + return v2( + [ + { id: "start", kind: "start" }, + { id: "end", kind: "end" }, + ], + [{ from: "start", to: "end" }], + { fields: fields as WorkflowIrV2["fields"] }, + ); + } + + it("accepts well-formed fields", () => { + expect(() => + parseWorkflowIr( + graphWithFields([ + { id: "sev", name: "Severity", type: "enum", options: [{ value: "lo", label: "Low" }] }, + { id: "note", name: "Note", type: "text", render: { placement: "detail", widget: "textarea" } }, + ]), + ), + ).not.toThrow(); + }); + + it("rejects duplicate field ids", () => { + expect(() => + parseWorkflowIr( + graphWithFields([ + { id: "a", name: "A", type: "string" }, + { id: "a", name: "A2", type: "number" }, + ]), + ), + ).toThrow(/duplicate field id 'a'/); + }); + + it("rejects unknown field type", () => { + expect(() => parseWorkflowIr(graphWithFields([{ id: "a", name: "A", type: "color" }]))).toThrow( + /unknown type 'color'/, + ); + }); + + it("requires options on enum/multi-enum", () => { + expect(() => parseWorkflowIr(graphWithFields([{ id: "a", name: "A", type: "enum" }]))).toThrow( + /must declare non-empty options/, + ); + expect(() => + parseWorkflowIr(graphWithFields([{ id: "a", name: "A", type: "multi-enum", options: [] }])), + ).toThrow(/must declare non-empty options/); + }); + + it("rejects options on non-enum types", () => { + expect(() => + parseWorkflowIr( + graphWithFields([{ id: "a", name: "A", type: "string", options: [{ value: "x", label: "X" }] }]), + ), + ).toThrow(/must not declare options/); + }); + + it("rejects bad render placement / widget", () => { + expect(() => + parseWorkflowIr(graphWithFields([{ id: "a", name: "A", type: "string", render: { placement: "footer" } }])), + ).toThrow(/render.placement 'footer' is not allowed/); + expect(() => + parseWorkflowIr(graphWithFields([{ id: "a", name: "A", type: "string", render: { widget: "slider" } }])), + ).toThrow(/render.widget 'slider' is not allowed/); + }); +}); + +describe("downgradeIrToV1IfPure refuses step-inversion features", () => { + it("returns v2 unchanged for a graph with a foreach", () => { + const ir = parseWorkflowIr(graphWithForeach({})) as WorkflowIrV2; + expect(downgradeIrToV1IfPure(ir).version).toBe("v2"); + }); + + it("returns v2 unchanged when fields/artifacts are declared even with pure-v1 nodes", () => { + const ir = v2( + [ + { id: "start", kind: "start", column: "todo" }, + { id: "end", kind: "end", column: "todo" }, + ], + [{ from: "start", to: "end" }], + { fields: [{ id: "a", name: "A", type: "string" }] }, + ); + expect(downgradeIrToV1IfPure(ir).version).toBe("v2"); + }); +}); + +describe("JSON round-trip stability", () => { + it("re-parses a serialized foreach graph identically", () => { + const ir = parseWorkflowIr(graphWithForeach({ maxReworkCycles: 3 })) as WorkflowIrV2; + const serialized = serializeWorkflowIr(ir); + const reparsed = parseWorkflowIr(serialized) as WorkflowIrV2; + expect(serializeWorkflowIr(reparsed)).toBe(serialized); + }); +}); + +describe("illegal cycle detection (rework exemption)", () => { + it("still rejects a non-rework cycle at the top level", () => { + const ir = v2( + [ + { id: "start", kind: "start" }, + { id: "a", kind: "prompt" }, + { id: "b", kind: "prompt" }, + { id: "end", kind: "end" }, + ], + [ + { from: "start", to: "a" }, + { from: "a", to: "b" }, + { from: "b", to: "a" }, + { from: "a", to: "end" }, + ], + ); + expect(() => parseWorkflowIr(ir)).toThrow(/illegal cycle/); + }); + + it("does not complain about the rework cycle inside a foreach template", () => { + // graphWithForeach's template has a rework edge rev → se; should parse fine. + expect(() => parseWorkflowIr(graphWithForeach({}))).not.toThrow(); + }); +}); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 3a436d8903..a477d46f3e 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -798,6 +798,87 @@ const KNOWN_FILE_SCOPE_ROOT_FILES = new Set([ "agents.md", ]); +/** + * Parse `### Step N:` headings into the task step list (step-inversion U1). + * + * Backward compatibility is exact: an UNannotated heading parses byte-identically + * to the legacy regex `^###\s+Step\s+\d+[^:]*:\s*(.+)$` (name = text after the + * first colon, trimmed). + * + * The annotation `### Step N (depends: 1,2): Title` is parsed explicitly (the + * legacy regex breaks on the colon inside `depends:`): depends values are + * 1-indexed step numbers in the document and are stored as 0-indexed indices on + * `dependsOn` (deduped, sorted, dropping values <= 0). + * + * Malformed `(depends: …)` annotations fall back deterministically: the heading + * is treated as `### Step N:` with the name starting after the FIRST colon + * following the closing paren (if present), else after the first colon — and no + * `dependsOn` is recorded. + */ +export function parseStepHeadings(content: string): import("./types.js").TaskStep[] { + const steps: import("./types.js").TaskStep[] = []; + // Legacy matcher — UNCHANGED from the original implementation, so unannotated + // headings (and every legacy edge case, including `[^:]*` spanning newlines) + // parse byte-identically. The full match (`m[0]`) is re-inspected only to layer + // the `(depends: …)` annotation on top. + const stepRegex = /^###\s+Step\s+\d+[^:]*:\s*(.+)$/gm; + // Well-formed annotation form: `### Step N (depends: …): name`. + const annotatedRegex = /^###\s+Step\s+\d+\s*\(depends:\s*([^)]*)\)\s*:\s*([^\n]+)$/; + + let match: RegExpExecArray | null; + while ((match = stepRegex.exec(content)) !== null) { + const full = match[0]; + + // No annotation present → byte-identical legacy behavior. + if (!full.includes("(depends:")) { + steps.push({ name: match[1].trim(), status: "pending" }); + continue; + } + + // 1) Well-formed depends annotation. + const annotated = annotatedRegex.exec(full); + if (annotated) { + const parsed = parseDependsList(annotated[1]); + const name = annotated[2].trim(); + if (parsed !== null) { + if (parsed.length > 0) steps.push({ name, status: "pending", dependsOn: parsed }); + else steps.push({ name, status: "pending" }); + continue; + } + } + + // 2) Annotation present but unparseable (bad values or no closing paren): + // deterministic fallback — name starts after the FIRST colon following the + // closing paren if present, else after the first colon. Operate on the + // first line of the match only (the heading line itself). + const line = full.split("\n")[0]; + const parenIdx = line.indexOf(")"); + const colonAfterParen = parenIdx >= 0 ? line.indexOf(":", parenIdx) : -1; + const colonIdx = colonAfterParen >= 0 ? colonAfterParen : line.indexOf(":"); + if (colonIdx >= 0) { + const fallbackName = line.slice(colonIdx + 1).trim(); + if (fallbackName) steps.push({ name: fallbackName, status: "pending" }); + } + } + return steps; +} + +/** Parse a `depends:` value list (1-indexed step numbers) into 0-indexed, + * deduped, sorted indices. Returns null if any token is not a positive integer. */ +function parseDependsList(raw: string): number[] | null { + const trimmed = raw.trim(); + if (trimmed === "") return []; + const tokens = trimmed.split(",").map((t) => t.trim()); + const out = new Set(); + for (const token of tokens) { + if (!/^\d+$/.test(token)) return null; + const n = Number(token); + if (!Number.isInteger(n) || n < 1) return null; + out.add(n - 1); + } + return [...out].sort((a, b) => a - b); +} + export function isValidFileScopeEntry(token: string): boolean { const trimmed = token.trim(); if (!trimmed) return false; @@ -8537,13 +8618,7 @@ export class TaskStore extends EventEmitter { if (!existsSync(promptPath)) return []; const content = await readFile(promptPath, "utf-8"); - const steps: import("./types.js").TaskStep[] = []; - const stepRegex = /^###\s+Step\s+\d+[^:]*:\s*(.+)$/gm; - let match; - while ((match = stepRegex.exec(content)) !== null) { - steps.push({ name: match[1].trim(), status: "pending" }); - } - return steps; + return parseStepHeadings(content); } /** diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 41ea3f2de9..fbf8f693da 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -1061,6 +1061,11 @@ export type StepStatus = "pending" | "in-progress" | "done" | "skipped"; export interface TaskStep { name: string; status: StepStatus; + /** Step-inversion (KTD-11): 0-indexed indices of steps this step depends on, + * parsed from the PROMPT.md `### Step N (depends: 1,2): Title` annotation + * (1-indexed step numbers in the doc → 0-indexed indices here). Absent for + * unannotated steps. */ + dependsOn?: number[]; } /** Correlation metadata linking a task mutation to the agent run that caused it. */ diff --git a/packages/core/src/workflow-ir-types.ts b/packages/core/src/workflow-ir-types.ts index d636618f8d..b96e538fb5 100644 --- a/packages/core/src/workflow-ir-types.ts +++ b/packages/core/src/workflow-ir-types.ts @@ -1,5 +1,9 @@ /** Node kinds. v1 kinds (start/prompt/script/gate/end) plus the v2 additions: - * `hold` (passive dwell column states), and `split`/`join` (parallel fan-out). */ + * `hold` (passive dwell column states), `split`/`join` (parallel fan-out), and + * the step-inversion additions (FN step-inversion, KTD-3/4/12/15): + * `foreach` (runtime-expanding per-step template region), `step-review` + * (per-step review verdicts as outcome edges), `parse-steps` (graph-native + * step-list parsing), and `code` (sandboxed TypeScript). */ export type WorkflowIrNodeKind = | "start" | "prompt" @@ -8,7 +12,11 @@ export type WorkflowIrNodeKind = | "end" | "hold" | "split" - | "join"; + | "join" + | "foreach" + | "step-review" + | "parse-steps" + | "code"; export interface WorkflowIrNode { id: string; @@ -22,6 +30,71 @@ export interface WorkflowIrEdge { from: string; to: string; condition?: string; + /** Step-inversion (KTD-5): `rework` edges are the only legal cycles, scoped to + * one foreach template instance and bounded by the foreach `maxReworkCycles`. + * They are exempt from cycle/parallelism complaints. */ + kind?: "rework"; +} + +/** Step-inversion (KTD-3): config for a `foreach` node — a runtime-expanding + * template region instantiated once per planned step. + * Defaults: `mode` sequential; `isolation` shared for sequential / worktree for + * parallel; `concurrency` parallel-only. */ +export interface WorkflowForeachConfig { + source: "task-steps"; + maxReworkCycles?: number; + mode?: "sequential" | "parallel"; + concurrency?: number; + isolation?: "shared" | "worktree"; + template: { + nodes: WorkflowIrNode[]; + edges: WorkflowIrEdge[]; + }; +} + +/** Step-inversion (KTD-12): a workflow-declared task document. Artifacts ride the + * existing task-documents machinery; `step-source` artifacts feed `parse-steps`. */ +export interface WorkflowIrArtifact { + key: string; + title?: string; + producedBy?: "planning" | "manual"; + role?: "step-source" | "context"; +} + +/** Step-inversion (KTD-13): the supported custom-field value types. */ +export type WorkflowFieldType = + | "string" + | "text" + | "number" + | "boolean" + | "enum" + | "multi-enum" + | "date" + | "url"; + +/** A single enum/multi-enum option (KTD-13). */ +export interface WorkflowFieldOption { + value: string; + label: string; + color?: string; +} + +/** Rendering instructions for a custom field (KTD-14). */ +export interface WorkflowFieldRender { + placement?: "card" | "detail" | "detail-section"; + widget?: "select" | "radio" | "chips" | "input" | "textarea" | "toggle"; + badge?: boolean; +} + +/** Step-inversion (KTD-13): a workflow-defined custom task field. */ +export interface WorkflowFieldDefinition { + id: string; + name: string; + type: WorkflowFieldType; + required?: boolean; + default?: unknown; + options?: WorkflowFieldOption[]; + render?: WorkflowFieldRender; } /** A single trait configuration applied to a column. The `trait` is an opaque @@ -61,13 +134,17 @@ export interface WorkflowIrV1 { edges: WorkflowIrEdge[]; } -/** A v2 workflow IR graph: v1 plus workflow-defined columns and node placement. */ +/** A v2 workflow IR graph: v1 plus workflow-defined columns and node placement. + * Step-inversion adds optional `artifacts` (KTD-12) and `fields` (KTD-13) + * declarations — both additive; absent on legacy graphs. */ export interface WorkflowIrV2 { version: "v2"; name: string; columns: WorkflowIrColumn[]; nodes: WorkflowIrNode[]; edges: WorkflowIrEdge[]; + artifacts?: WorkflowIrArtifact[]; + fields?: WorkflowFieldDefinition[]; } /** Either IR version. v1 graphs upgrade to v2 on parse (see parseWorkflowIr). */ diff --git a/packages/core/src/workflow-ir.ts b/packages/core/src/workflow-ir.ts index 95ed2ac100..6a3cc5eff8 100644 --- a/packages/core/src/workflow-ir.ts +++ b/packages/core/src/workflow-ir.ts @@ -7,6 +7,9 @@ import type { WorkflowIrV1, WorkflowIrV2, WorkflowHoldRelease, + WorkflowForeachConfig, + WorkflowFieldDefinition, + WorkflowFieldType, } from "./workflow-ir-types.js"; export class WorkflowIrError extends Error { @@ -25,8 +28,56 @@ const HOLD_RELEASE_KINDS: ReadonlySet = new Set([ ]); /** Seam config values that may not appear inside a parallel branch (KTD-11): - * one worktree/session per task and exclusive merge are physical constraints. */ -const SEAM_FORBIDDEN_IN_BRANCH: ReadonlySet = new Set(["execute", "merge"]); + * one worktree/session per task and exclusive merge are physical constraints. + * Step-inversion (KTD-4) extends this posture: `step-execute` seam prompt nodes + * may never appear in a split branch either. */ +const SEAM_FORBIDDEN_IN_BRANCH: ReadonlySet = new Set([ + "execute", + "merge", + "step-execute", +]); + +/** Step-inversion field-type whitelist (KTD-13). */ +const WORKFLOW_FIELD_TYPES: ReadonlySet = new Set([ + "string", + "text", + "number", + "boolean", + "enum", + "multi-enum", + "date", + "url", +]); + +const FIELD_RENDER_PLACEMENTS: ReadonlySet = new Set([ + "card", + "detail", + "detail-section", +]); + +const FIELD_RENDER_WIDGETS: ReadonlySet = new Set([ + "select", + "radio", + "chips", + "input", + "textarea", + "toggle", +]); + +/** Hard cap on a foreach `maxReworkCycles` (KTD-5: default 3, clamp >10 to 10, + * reject <1). */ +const MAX_REWORK_CYCLES_CAP = 10; + +/** Parallel concurrency bounds (KTD-3): range 1..8. */ +const MAX_FOREACH_CONCURRENCY = 8; + +/** The implicit step-source artifact allowed when no artifacts are declared. */ +const IMPLICIT_DEFAULT_ARTIFACT = "PROMPT.md"; + +/** True when a prompt node carries the `step-execute` seam (KTD-2/KTD-4). */ +function isStepExecuteNode(node: WorkflowIrNode): boolean { + return node.kind === "prompt" && node.config?.seam === "step-execute"; +} /** Default-workflow column ids in legacy enum order (KTD-1). */ export const DEFAULT_WORKFLOW_COLUMN_IDS = [ @@ -180,6 +231,501 @@ function innerJoinNext(joinId: string, outgoing: Map): return (outgoing.get(joinId) ?? []).find((e) => e.condition !== "failure")?.to; } +// --------------------------------------------------------------------------- +// Step-inversion validation (FN step-inversion, U1) +// --------------------------------------------------------------------------- + +/** True for a `rework`-kind edge (KTD-5). */ +function isReworkEdge(edge: WorkflowIrEdge): boolean { + return edge.kind === "rework"; +} + +/** Collect the set of node ids reachable from `start` following non-rework edges + * (rework edges are intra-template back-edges; the top-level reachability / + * dominance analysis ignores them). */ +function reachableFrom( + start: string, + outgoing: Map, +): Set { + const seen = new Set(); + const queue = [start]; + while (queue.length) { + const id = queue.shift()!; + if (seen.has(id)) continue; + seen.add(id); + for (const edge of outgoing.get(id) ?? []) { + if (isReworkEdge(edge)) continue; + if (!seen.has(edge.to)) queue.push(edge.to); + } + } + return seen; +} + +/** + * Validate a foreach `template` subgraph recursively (KTD-3): + * - non-empty; + * - exactly one entry (no incoming template edges) and one exit (no outgoing); + * - NO nested foreach; + * - `step-execute` seam nodes are legal here but never inside a split branch + * (SEAM_FORBIDDEN_IN_BRANCH already enforces this via validateParallelism); + * - rework edges legal only when both endpoints are inside this template; + * - step-review verdict routing rules (KTD-4). + */ +function validateForeach(node: WorkflowIrNode, topLevelNodeIds: Set): void { + const cfg = node.config as Partial | undefined; + if (!cfg || cfg.source !== "task-steps") { + throw new WorkflowIrError( + `foreach node '${node.id}' must declare source 'task-steps'`, + ); + } + const template = cfg.template; + if ( + !template || + !Array.isArray(template.nodes) || + !Array.isArray(template.edges) + ) { + throw new WorkflowIrError( + `foreach node '${node.id}' must declare a template with nodes and edges arrays`, + ); + } + if (template.nodes.length === 0) { + throw new WorkflowIrError(`foreach node '${node.id}' template must be non-empty`); + } + + // mode / isolation / concurrency (KTD-3). + const mode = cfg.mode ?? "sequential"; + if (mode !== "sequential" && mode !== "parallel") { + throw new WorkflowIrError( + `foreach node '${node.id}' mode must be 'sequential' or 'parallel'`, + ); + } + const isolation = cfg.isolation ?? (mode === "parallel" ? "worktree" : "shared"); + if (isolation !== "shared" && isolation !== "worktree") { + throw new WorkflowIrError( + `foreach node '${node.id}' isolation must be 'shared' or 'worktree'`, + ); + } + if (mode === "parallel" && isolation === "shared") { + throw new WorkflowIrError( + `foreach node '${node.id}' cannot combine mode 'parallel' with isolation 'shared' (concurrent writes in one worktree are unguardable races)`, + ); + } + if (cfg.concurrency !== undefined) { + if (mode !== "parallel") { + throw new WorkflowIrError( + `foreach node '${node.id}' concurrency is only valid in 'parallel' mode`, + ); + } + const c = cfg.concurrency; + if (typeof c !== "number" || !Number.isInteger(c) || c < 1 || c > MAX_FOREACH_CONCURRENCY) { + throw new WorkflowIrError( + `foreach node '${node.id}' concurrency must be an integer in 1..${MAX_FOREACH_CONCURRENCY}`, + ); + } + } + if (cfg.maxReworkCycles !== undefined) { + const m = cfg.maxReworkCycles; + if (typeof m !== "number" || !Number.isInteger(m) || m < 1) { + throw new WorkflowIrError( + `foreach node '${node.id}' maxReworkCycles must be an integer >= 1`, + ); + } + // >10 is clamped at parse time (clampForeachConfig); validation only rejects <1. + } + + const templateNodes = template.nodes; + const templateIds = new Set(templateNodes.map((n) => n.id)); + if (templateIds.size !== templateNodes.length) { + throw new WorkflowIrError( + `foreach node '${node.id}' template has duplicate node ids`, + ); + } + + // No nested foreach. + for (const inner of templateNodes) { + if (inner.kind === "foreach") { + throw new WorkflowIrError( + `foreach node '${node.id}' template may not contain a nested foreach ('${inner.id}')`, + ); + } + } + + // Edge endpoints must reference template nodes; rework edges must stay intra-template. + for (const edge of template.edges) { + const fromInside = templateIds.has(edge.from); + const toInside = templateIds.has(edge.to); + if (!fromInside || !toInside) { + if (isReworkEdge(edge)) { + throw new WorkflowIrError( + `rework edge '${edge.from}' -> '${edge.to}' in foreach '${node.id}' must have both endpoints inside the same template`, + ); + } + throw new WorkflowIrError( + `foreach node '${node.id}' template edge '${edge.from}' -> '${edge.to}' references a node outside the template`, + ); + } + } + + // Single entry / single exit (ignoring rework back-edges, which intentionally + // create incoming edges to earlier template nodes). + const incoming = new Map(); + const outgoingCount = new Map(); + for (const edge of template.edges) { + if (isReworkEdge(edge)) continue; + incoming.set(edge.to, (incoming.get(edge.to) ?? 0) + 1); + outgoingCount.set(edge.from, (outgoingCount.get(edge.from) ?? 0) + 1); + } + const entries = templateNodes.filter((n) => (incoming.get(n.id) ?? 0) === 0); + const exits = templateNodes.filter((n) => (outgoingCount.get(n.id) ?? 0) === 0); + if (entries.length !== 1) { + throw new WorkflowIrError( + `foreach node '${node.id}' template must have exactly one entry node (found ${entries.length})`, + ); + } + if (exits.length !== 1) { + throw new WorkflowIrError( + `foreach node '${node.id}' template must have exactly one exit node (found ${exits.length})`, + ); + } + + // Recurse: validate the template as its own region for parallelism + verdict + // routing. step-execute nodes legal here (they are not validated as forbidden + // at top level — that check lives in validateStepExecutePlacement). + const templateById = new Map(templateNodes.map((n) => [n.id, n])); + const templateOutgoing = buildOutgoing(template.edges); + validateParallelism(templateNodes, templateOutgoing, templateById); + validateStepReviewRouting(templateNodes, templateOutgoing, templateById, true); + + // Defensive: top-level node ids and template node ids should not collide + // (instance identity is `#:`, but a raw collision + // is still confusing). + for (const id of templateIds) { + if (topLevelNodeIds.has(id)) { + throw new WorkflowIrError( + `foreach node '${node.id}' template node id '${id}' collides with a top-level node id`, + ); + } + } +} + +/** step-execute seam nodes are legal ONLY inside a foreach template (KTD-4): + * reject any at the top level. (Inside-split-branch rejection is handled by + * SEAM_FORBIDDEN_IN_BRANCH within validateParallelism.) */ +function validateStepExecutePlacement(topLevelNodes: WorkflowIrNode[]): void { + for (const node of topLevelNodes) { + if (isStepExecuteNode(node)) { + throw new WorkflowIrError( + `step-execute seam node '${node.id}' is only legal inside a foreach template`, + ); + } + } +} + +/** + * step-review verdict routing (KTD-4). For each step-review node: + * - it must have outgoing edges covering `outcome:approve` and `outcome:revise`; + * - `outcome:rethink` optional (defaults to the revise target with reset semantics); + * - `outcome:unavailable` optional; + * - a step-review node inside a split branch is advisory-only: it must NOT carry + * rework or `outcome:approve` routing. + */ +function validateStepReviewRouting( + nodes: WorkflowIrNode[], + outgoing: Map, + nodesById: Map, + insideForeachTemplate: boolean, +): void { + // Determine which nodes sit inside a split branch (advisory-only zone). + const inBranch = nodesInSplitBranches(nodes, outgoing, nodesById); + + for (const node of nodes) { + if (node.kind !== "step-review") continue; + if (node.config?.type !== "plan" && node.config?.type !== "code") { + throw new WorkflowIrError( + `step-review node '${node.id}' must declare type 'plan' or 'code'`, + ); + } + if (node.config.model !== undefined && typeof node.config.model !== "string") { + throw new WorkflowIrError( + `step-review node '${node.id}' model must be a string when present`, + ); + } + + const edges = outgoing.get(node.id) ?? []; + const conditions = new Set(edges.map((e) => e.condition)); + const hasRework = edges.some(isReworkEdge); + + if (inBranch.has(node.id)) { + // Advisory-only inside a split branch: no rework, no approve routing. + if (hasRework) { + throw new WorkflowIrError( + `step-review node '${node.id}' inside a split branch is advisory-only and may not have rework edges`, + ); + } + if (conditions.has("outcome:approve")) { + throw new WorkflowIrError( + `step-review node '${node.id}' inside a split branch is advisory-only and may not carry outcome:approve routing`, + ); + } + continue; + } + + // Main-path step-review: must route approve and revise. + if (!conditions.has("outcome:approve")) { + throw new WorkflowIrError( + `step-review node '${node.id}' must route outcome:approve`, + ); + } + if (!conditions.has("outcome:revise")) { + throw new WorkflowIrError( + `step-review node '${node.id}' must route outcome:revise`, + ); + } + void insideForeachTemplate; + } +} + +/** Compute the set of node ids that lie strictly inside some split..join branch + * region. Walks each split's branches forward to the join. Lightweight; used + * for the step-review advisory-only rule. */ +function nodesInSplitBranches( + nodes: WorkflowIrNode[], + outgoing: Map, + nodesById: Map, +): Set { + const inBranch = new Set(); + const splits = nodes.filter((n) => n.kind === "split"); + for (const split of splits) { + for (const edge of outgoing.get(split.id) ?? []) { + let cursor: string | undefined = edge.to; + const visited = new Set(); + while (cursor && !visited.has(cursor)) { + const id: string = cursor; + visited.add(id); + const n = nodesById.get(id); + if (!n || n.kind === "join") break; + inBranch.add(id); + const next: WorkflowIrEdge | undefined = (outgoing.get(id) ?? []).find( + (e) => !isReworkEdge(e) && e.condition !== "failure", + ); + cursor = next?.to; + } + } + } + return inBranch; +} + +/** + * Cycle detection across the top-level graph that EXEMPTS rework edges (KTD-5). + * Any non-rework cycle is rejected; rework edges (intra-template back-edges) are + * skipped. Run over the top-level graph; template internals are validated + * separately. + */ +function validateNoIllegalCycles( + nodes: WorkflowIrNode[], + outgoing: Map, +): void { + const WHITE = 0; + const GRAY = 1; + const BLACK = 2; + const color = new Map(); + for (const n of nodes) color.set(n.id, WHITE); + + const visit = (id: string): void => { + color.set(id, GRAY); + for (const edge of outgoing.get(id) ?? []) { + if (isReworkEdge(edge)) continue; + const c = color.get(edge.to); + if (c === GRAY) { + throw new WorkflowIrError( + `Workflow IR has an illegal cycle (edge '${edge.from}' -> '${edge.to}'); only rework edges may form cycles`, + ); + } + if (c === WHITE) visit(edge.to); + } + color.set(id, BLACK); + }; + + for (const n of nodes) { + if (color.get(n.id) === WHITE) visit(n.id); + } +} + +/** + * Dominance check (KTD-3): every `foreach(source:"task-steps")` must be dominated + * by a `parse-steps` node — a parse-steps node lies on EVERY path from start to + * the foreach. Implemented via the classic "removal disconnects start from + * target" definition, which is correct for DAGs: for each parse-steps node, + * check whether the foreach is still reachable from start with that node removed. + * The foreach is dominated iff some parse-steps node's removal disconnects it. + */ +function validateForeachDominance( + nodes: WorkflowIrNode[], + edges: WorkflowIrEdge[], + outgoing: Map, +): void { + const startNode = nodes.find((n) => n.kind === "start"); + if (!startNode) return; // parse-time guarantees exactly one start. + const foreaches = nodes.filter( + (n) => n.kind === "foreach" && (n.config as { source?: unknown } | undefined)?.source === "task-steps", + ); + if (foreaches.length === 0) return; + const parseStepsNodes = nodes.filter((n) => n.kind === "parse-steps"); + + for (const fe of foreaches) { + // Reachable from start at all? + if (!reachableFrom(startNode.id, outgoing).has(fe.id)) { + throw new WorkflowIrError( + `foreach node '${fe.id}' is not reachable from the start node`, + ); + } + const dominated = parseStepsNodes.some((ps) => { + if (ps.id === fe.id) return false; + // Build outgoing with ps removed (as both source and target). + const trimmed = buildOutgoing( + edges.filter((e) => e.from !== ps.id && e.to !== ps.id), + ); + return !reachableFrom(startNode.id, trimmed).has(fe.id); + }); + if (!dominated) { + throw new WorkflowIrError( + `foreach node '${fe.id}' (source:'task-steps') must be dominated by a parse-steps node on every path from start`, + ); + } + } +} + +/** Validate `parse-steps` node config (KTD-12). */ +function validateParseStepsNodes(ir: WorkflowIrV2): void { + const declaredArtifacts = new Set((ir.artifacts ?? []).map((a) => a.key)); + const hasDeclaredArtifacts = (ir.artifacts ?? []).length > 0; + + for (const node of ir.nodes) { + if (node.kind !== "parse-steps") continue; + const cfg = node.config as { artifact?: unknown; parser?: unknown } | undefined; + const artifact = cfg?.artifact; + const parser = cfg?.parser; + if (typeof parser !== "string" || parser.trim() === "") { + throw new WorkflowIrError( + `parse-steps node '${node.id}' must declare a non-empty parser`, + ); + } + if (typeof artifact !== "string" || artifact.trim() === "") { + throw new WorkflowIrError( + `parse-steps node '${node.id}' must declare a non-empty artifact`, + ); + } + if (hasDeclaredArtifacts) { + if (!declaredArtifacts.has(artifact)) { + throw new WorkflowIrError( + `parse-steps node '${node.id}' references undeclared artifact '${artifact}'`, + ); + } + } else if (artifact !== IMPLICIT_DEFAULT_ARTIFACT) { + throw new WorkflowIrError( + `parse-steps node '${node.id}' references artifact '${artifact}', but only '${IMPLICIT_DEFAULT_ARTIFACT}' is allowed when no artifacts are declared`, + ); + } + } +} + +/** Validate `code` node config (KTD-15). TS is NOT compiled in core (esbuild + * check is engine/editor side). */ +function validateCodeNodes(nodes: WorkflowIrNode[]): void { + const MAX_SOURCE = 65536; + for (const node of nodes) { + if (node.kind !== "code") continue; + const cfg = node.config as { source?: unknown; timeoutMs?: unknown } | undefined; + const source = cfg?.source; + if (typeof source !== "string" || source.length === 0) { + throw new WorkflowIrError(`code node '${node.id}' must declare a non-empty source`); + } + if (source.length > MAX_SOURCE) { + throw new WorkflowIrError( + `code node '${node.id}' source exceeds ${MAX_SOURCE} characters`, + ); + } + if (cfg?.timeoutMs !== undefined) { + const t = cfg.timeoutMs; + if (typeof t !== "number" || !Number.isInteger(t) || t < 1000 || t > 300000) { + throw new WorkflowIrError( + `code node '${node.id}' timeoutMs must be an integer in 1000..300000`, + ); + } + } + } +} + +/** Validate `fields` declarations (KTD-13). */ +function validateFields(fields: WorkflowFieldDefinition[] | undefined): void { + if (fields === undefined) return; + if (!Array.isArray(fields)) { + throw new WorkflowIrError("Workflow IR fields must be an array"); + } + const seen = new Set(); + for (const field of fields) { + if (!field || typeof field.id !== "string" || field.id === "") { + throw new WorkflowIrError("Workflow field must have a non-empty id"); + } + if (seen.has(field.id)) { + throw new WorkflowIrError(`Workflow IR has duplicate field id '${field.id}'`); + } + seen.add(field.id); + if (typeof field.name !== "string" || field.name === "") { + throw new WorkflowIrError(`Workflow field '${field.id}' must have a non-empty name`); + } + if (!WORKFLOW_FIELD_TYPES.has(field.type)) { + throw new WorkflowIrError( + `Workflow field '${field.id}' has unknown type '${String(field.type)}'`, + ); + } + const isEnum = field.type === "enum" || field.type === "multi-enum"; + if (isEnum) { + if (!Array.isArray(field.options) || field.options.length === 0) { + throw new WorkflowIrError( + `Workflow field '${field.id}' of type '${field.type}' must declare non-empty options`, + ); + } + const optSeen = new Set(); + for (const opt of field.options) { + if (!opt || typeof opt.value !== "string" || opt.value === "") { + throw new WorkflowIrError( + `Workflow field '${field.id}' option must have a non-empty value`, + ); + } + if (typeof opt.label !== "string" || opt.label === "") { + throw new WorkflowIrError( + `Workflow field '${field.id}' option '${opt.value}' must have a non-empty label`, + ); + } + if (optSeen.has(opt.value)) { + throw new WorkflowIrError( + `Workflow field '${field.id}' has duplicate option value '${opt.value}'`, + ); + } + optSeen.add(opt.value); + } + } else if (field.options !== undefined) { + throw new WorkflowIrError( + `Workflow field '${field.id}' of type '${field.type}' must not declare options`, + ); + } + if (field.render !== undefined) { + const r = field.render; + if (r.placement !== undefined && !FIELD_RENDER_PLACEMENTS.has(r.placement)) { + throw new WorkflowIrError( + `Workflow field '${field.id}' render.placement '${String(r.placement)}' is not allowed`, + ); + } + if (r.widget !== undefined && !FIELD_RENDER_WIDGETS.has(r.widget)) { + throw new WorkflowIrError( + `Workflow field '${field.id}' render.widget '${String(r.widget)}' is not allowed`, + ); + } + } + } +} + function validateColumns(ir: WorkflowIrV2): void { if (!Array.isArray(ir.columns)) { throw new WorkflowIrError("Workflow IR v2 columns must be an array"); @@ -223,6 +769,48 @@ function validateV2(ir: WorkflowIrV2): void { const outgoing = buildOutgoing(ir.edges); validateParallelism(ir.nodes, outgoing, nodesById); + + // Step-inversion (U1) — additive validation. Order matters: validate node + // configs first, then structural rules. + const topLevelIds = new Set(ir.nodes.map((n) => n.id)); + validateStepExecutePlacement(ir.nodes); + for (const node of ir.nodes) { + if (node.kind === "foreach") validateForeach(node, topLevelIds); + } + validateStepReviewRouting(ir.nodes, outgoing, nodesById, false); + validateParseStepsNodes(ir); + validateCodeNodes(ir.nodes); + validateFields(ir.fields); + + // Rework edges are legal only intra-template; any rework edge at the top level + // is rejected (template rework edges are validated inside validateForeach and + // never appear in ir.edges). + for (const edge of ir.edges) { + if (isReworkEdge(edge)) { + throw new WorkflowIrError( + `rework edge '${edge.from}' -> '${edge.to}' is only legal inside a foreach template`, + ); + } + } + + validateNoIllegalCycles(ir.nodes, outgoing); + validateForeachDominance(ir.nodes, ir.edges, outgoing); +} + +/** Clamp foreach `maxReworkCycles` > cap down to the cap, in place, mirroring the + * maxRetries clamp posture (KTD-5). Reject-of-<1 happens in validation. */ +function clampForeachConfigs(ir: WorkflowIrV2): void { + for (const node of ir.nodes) { + if (node.kind !== "foreach") continue; + const cfg = node.config as Partial | undefined; + if ( + cfg && + typeof cfg.maxReworkCycles === "number" && + cfg.maxReworkCycles > MAX_REWORK_CYCLES_CAP + ) { + cfg.maxReworkCycles = MAX_REWORK_CYCLES_CAP; + } + } } export function parseWorkflowIr(input: string | WorkflowIr): WorkflowIr { @@ -249,6 +837,7 @@ export function parseWorkflowIr(input: string | WorkflowIr): WorkflowIr { return upgradeV1ToV2(ir); } + clampForeachConfigs(ir); validateV2(ir); return ir; } @@ -280,6 +869,11 @@ export function downgradeIrToV1IfPure(ir: WorkflowIr): WorkflowIr { if (!V1_NODE_KINDS.has(node.kind)) return ir; } + // Step-inversion declarations (artifacts/fields) are v2-only features. + if ((ir.artifacts && ir.artifacts.length > 0) || (ir.fields && ir.fields.length > 0)) { + return ir; + } + // Columns must be exactly the synthesized default set, same ids, same order, // with the minimal (placement-only) empty trait set. Any custom column, rename, // reorder, or applied trait forces v2. From 3d03505a62fa20ad8ba2b4d79a50d39d9bb61321 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 11:50:45 -0700 Subject: [PATCH 02/22] =?UTF-8?q?feat(engine):=20U2=20=E2=80=94=20runTaskS?= =?UTF-8?q?tep/resetStepToBaseline=20substrate=20seams=20with=20blast-radi?= =?UTF-8?q?us=20guard=20(RETHINK=20extraction)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../__tests__/executor-step-session.test.ts | 130 ++++++ .../engine/src/__tests__/step-runner.test.ts | 397 ++++++++++++++++++ packages/engine/src/executor.ts | 71 +--- packages/engine/src/index.ts | 15 + packages/engine/src/step-runner.ts | 375 +++++++++++++++++ 5 files changed, 938 insertions(+), 50 deletions(-) create mode 100644 packages/engine/src/__tests__/step-runner.test.ts create mode 100644 packages/engine/src/step-runner.ts diff --git a/packages/engine/src/__tests__/executor-step-session.test.ts b/packages/engine/src/__tests__/executor-step-session.test.ts index dd7ba60762..9fee7cdc53 100644 --- a/packages/engine/src/__tests__/executor-step-session.test.ts +++ b/packages/engine/src/__tests__/executor-step-session.test.ts @@ -3583,3 +3583,133 @@ describe("TaskExecutor loop recovery", () => { // ── Context limit error recovery tests ──────────────────────────────── +// ── U2 RETHINK delegation characterization (plan 2026-06-04-001, KTD-2) ── +// +// The legacy in-session fn_review_step RETHINK case now DELEGATES to +// step-runner.ts's resetStepToBaseline. These tests pin that the observable +// side effects are byte-identical to the pre-extraction block: git reset to +// the agent-supplied baseline, session rewind via navigateTree, step→pending, +// and the RETHINK log entry — all reached through the real executor session. +describe("U2: fn_review_step RETHINK delegates to resetStepToBaseline (characterization)", () => { + beforeEach(() => { + resetExecutorMocks(); + }); + + function runRethinkScenario(reviewType: "code" | "plan", navigateTree: any) { + const store = createMockStore(); + const baseTask = { + id: "FN-RT-1", + title: "Test", + description: "Test task", + column: "in-progress", + dependencies: [], + steps: [{ name: "Implement", status: "in-progress" }], + currentStep: 0, + log: [], + prompt: "# test\n## Steps\n### Step 1: Implement\n- [ ] implement", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }; + store.getTask.mockResolvedValue(baseTask as any); + // updateStep returns the task with the step persisted in-progress so the + // executor's checkpoint-capture path (executor.ts ~6517) populates the + // stepCheckpoints map that RETHINK rewinds to. + store.updateStep.mockResolvedValue({ + ...baseTask, + steps: [{ name: "Implement", status: "in-progress" }], + } as any); + + mockedReviewStep.mockResolvedValue({ + verdict: "RETHINK", + review: "wrong approach", + summary: "rejected approach", + } as any); + + let reviewToolError: unknown; + mockedCreateFnAgent.mockImplementation((async (opts: any) => { + const tools = opts.customTools || []; + return { + session: { + prompt: vi.fn().mockImplementation(async () => { + // First, flip the step to in-progress via fn_task_update so the + // checkpoint map is populated (mirrors the real session lifecycle). + const updateTool = tools.find((t: any) => t.name === "fn_task_update"); + if (updateTool) { + try { + await updateTool.execute("tool-update", { step: 1, status: "in-progress" }); + } catch { /* tool param shape varies; ignore */ } + } + const reviewTool = tools.find((t: any) => t.name === "fn_review_step"); + if (reviewTool) { + try { + await reviewTool.execute("tool-review", { + step: 1, + type: reviewType, + step_name: "Implement", + baseline: reviewType === "code" ? "agentBaselineSHA" : undefined, + }); + } catch (e) { + reviewToolError = e; + } + } + }), + dispose: vi.fn(), + subscribe: vi.fn(), + on: vi.fn(), + navigateTree, + sessionManager: { + getLeafId: vi.fn().mockReturnValue("leaf-pre-step"), + branchWithSummary: vi.fn(), + }, + state: {}, + }, + }; + }) as any); + + const executor = new TaskExecutor(store, "/tmp/test", {}); + return { store, baseTask, executor, getReviewToolError: () => reviewToolError }; + } + + it("code RETHINK: git reset to baseline, navigateTree rewind, step→pending, RETHINK log", async () => { + const navigateTree = vi.fn().mockResolvedValue(undefined); + const { store, baseTask, executor } = runRethinkScenario("code", navigateTree); + + await executor.execute(baseTask as any); + + // git reset --hard issued in the worktree (via the mocked exec). + const resetIssued = mockedExecSync.mock.calls.some( + (c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard agentBaselineSHA"), + ); + expect(resetIssued).toBe(true); + // Session rewound to the captured pre-step checkpoint. + expect(navigateTree).toHaveBeenCalledWith("leaf-pre-step", { summarize: false }); + // Step reset to pending through the projection sink. + expect(store.updateStep).toHaveBeenCalledWith("FN-RT-1", 0, "pending"); + // RETHINK log entry (code-review variant references the git reset). + expect(store.logEntry).toHaveBeenCalledWith( + "FN-RT-1", + expect.stringContaining("git reset to agentBaselineSHA"), + "rejected approach", + ); + }); + + it("plan RETHINK: no git reset, navigateTree rewind, step→pending, plan-rewound log", async () => { + const navigateTree = vi.fn().mockResolvedValue(undefined); + const { store, baseTask, executor } = runRethinkScenario("plan", navigateTree); + + await executor.execute(baseTask as any); + + const resetIssued = mockedExecSync.mock.calls.some( + (c) => typeof c[0] === "string" && (c[0] as string).includes("git reset --hard"), + ); + expect(resetIssued).toBe(false); + expect(navigateTree).toHaveBeenCalledWith("leaf-pre-step", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-RT-1", 0, "pending"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-RT-1", + expect.stringContaining("Step 1 plan rewound"), + "rejected approach", + ); + }); +}); + diff --git a/packages/engine/src/__tests__/step-runner.test.ts b/packages/engine/src/__tests__/step-runner.test.ts new file mode 100644 index 0000000000..25c885048f --- /dev/null +++ b/packages/engine/src/__tests__/step-runner.test.ts @@ -0,0 +1,397 @@ +/** + * Unit tests for the U2 substrate seams (plan 2026-06-04-001, KTD-2): + * - runTaskStep — per-step driver over step-session physics. + * - resetStepToBaseline — verbatim RETHINK mechanics + blast-radius guard. + * + * Fast tests: real git / sessions / StepSessionExecutor are never touched — + * every external is injected via the explicit `deps` object (FN-5048 fake-timer + * convention is moot here since the seams take no clock). The executor's + * delegation of the legacy RETHINK block is characterized separately in + * executor-step-session.test.ts. + */ + +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + runTaskStep, + resetStepToBaseline, + makeAncestryBlastRadiusGuard, + type StepRunnerTask, + type SessionRef, +} from "../step-runner.js"; + +function makeStore() { + return { + updateStep: vi.fn().mockResolvedValue(undefined), + logEntry: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeTask(steps: Array<{ name?: string; status?: string }>): StepRunnerTask { + return { id: "FN-001", steps }; +} + +function makeSessionRef(opts?: { + navigateTree?: ReturnType; + branchWithSummary?: ReturnType; + leafId?: string; +}): SessionRef { + const navigateTree = opts?.navigateTree ?? vi.fn().mockResolvedValue(undefined); + const branchWithSummary = opts?.branchWithSummary ?? vi.fn(); + return { + current: { + navigateTree, + sessionManager: { + branchWithSummary, + getLeafId: vi.fn().mockReturnValue(opts?.leafId ?? "leaf-pre-step"), + }, + } as unknown as SessionRef["current"], + }; +} + +describe("runTaskStep", () => { + beforeEach(() => vi.clearAllMocks()); + + it("marks the step in-progress then done on success, capturing baseline + checkpoint", async () => { + const store = makeStore(); + const task = makeTask([{ name: "Implement", status: "pending" }]); + const gitRevParse = vi.fn().mockResolvedValue("baseSHA123"); + const captureCheckpointId = vi.fn().mockReturnValue("leaf-pre-step"); + const runStep = vi.fn().mockResolvedValue({ success: true }); + + const result = await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId }, + task, + 0, + ); + + expect(result).toEqual({ outcome: "success", baselineSha: "baseSHA123", checkpointId: "leaf-pre-step" }); + // Baseline is captured BEFORE the step runs. + expect(gitRevParse).toHaveBeenCalledWith("/wt"); + expect(runStep).toHaveBeenCalledWith(0); + // Projection ordering: in-progress before done. + expect(store.updateStep.mock.calls).toEqual([ + ["FN-001", 0, "in-progress"], + ["FN-001", 0, "done"], + ]); + }); + + it("captures the baseline before running the step (order check)", async () => { + const store = makeStore(); + const order: string[] = []; + const gitRevParse = vi.fn().mockImplementation(async () => { + order.push("baseline"); + return "sha"; + }); + const runStep = vi.fn().mockImplementation(async () => { + order.push("run"); + return { success: true }; + }); + + await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId: () => "leaf" }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(order).toEqual(["baseline", "run"]); + }); + + it("leaves the step non-done on failure (no 'done'/'skipped' write)", async () => { + const store = makeStore(); + const runStep = vi.fn().mockResolvedValue({ success: false, error: "boom" }); + + const result = await runTaskStep( + { + store, + worktreePath: "/wt", + runStep, + gitRevParse: async () => "baseSHA", + captureCheckpointId: () => "leaf", + }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(result).toEqual({ outcome: "failure", baselineSha: "baseSHA", checkpointId: "leaf" }); + // Only the in-progress write happened — the failed step is left non-done. + expect(store.updateStep.mock.calls).toEqual([["FN-001", 0, "in-progress"]]); + expect(store.updateStep).not.toHaveBeenCalledWith("FN-001", 0, "done"); + expect(store.updateStep).not.toHaveBeenCalledWith("FN-001", 0, "skipped"); + }); + + it("still returns a result when baseline capture fails (best-effort)", async () => { + const store = makeStore(); + const runStep = vi.fn().mockResolvedValue({ success: true }); + const gitRevParse = vi.fn().mockRejectedValue(new Error("not a git repo")); + + const result = await runTaskStep( + { store, worktreePath: "/wt", runStep, gitRevParse, captureCheckpointId: () => "leaf" }, + makeTask([{ status: "pending" }]), + 0, + ); + + expect(result.outcome).toBe("success"); + expect(result.baselineSha).toBeUndefined(); + expect(result.checkpointId).toBe("leaf"); + }); + + it("uses the default checkpoint capture from the session ref when none injected", async () => { + const store = makeStore(); + const sessionRef = makeSessionRef({ leafId: "leaf-xyz" }); + const result = await runTaskStep( + { + store, + worktreePath: "/wt", + runStep: async () => ({ success: true }), + gitRevParse: async () => "sha", + }, + makeTask([{ status: "pending" }]), + 0, + { sessionRef }, + ); + expect(result.checkpointId).toBe("leaf-xyz"); + }); +}); + +describe("resetStepToBaseline", () => { + beforeEach(() => vi.clearAllMocks()); + + it("does git reset + session rewind + step→pending with baseline and checkpoint (code review)", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + // We can't observe the real git command without mocking child_process; verify + // the session rewind + projection happen. (The git path is exercised through + // the executor characterization test.) + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", summary: "rejected" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result).toEqual({ ok: true }); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-001", + expect.stringContaining("git reset to baseSHA"), + "rejected", + ); + }); + + it("skips the session rewind when no checkpoint is provided (partial path)", async () => { + const store = makeStore(); + const navigateTree = vi.fn(); + const branchWithSummary = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree, branchWithSummary }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + undefined, + ); + + expect(result.ok).toBe(true); + expect(navigateTree).not.toHaveBeenCalled(); + expect(branchWithSummary).not.toHaveBeenCalled(); + // Step still flips to pending. + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); + + it("plan review skips git reset, logs the plan-rewound line, still flips pending", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "plan", summary: "plan rejected" }, + makeTask([{ status: "in-progress" }]), + 2, + undefined, + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 2, "pending"); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-001", + // 0-indexed step 2 → 1-indexed "Step 3" + expect.stringContaining("Step 3 plan rewound"), + "plan rejected", + ); + }); + + it("falls back to branchWithSummary when navigateTree throws", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockRejectedValue(new Error("navigate failed")); + const branchWithSummary = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree, branchWithSummary }); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", summary: "why" }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(branchWithSummary).toHaveBeenCalledWith("leaf-checkpoint", "RETHINK: why"); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); + + // ── KTD-2 blast-radius guard refusal cases ────────────────────────────── + + it("REFUSES and mutates nothing when the guard reports a violation", async () => { + const store = makeStore(); + const navigateTree = vi.fn(); + const sessionRef = makeSessionRef({ navigateTree }); + const audit = { database: vi.fn().mockResolvedValue(undefined) }; + const blastRadiusGuard = vi.fn().mockResolvedValue("baseSHA is not an ancestor of HEAD"); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", audit, blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result).toEqual({ ok: false, reason: "baseSHA is not an ancestor of HEAD" }); + // No mutation: no rewind, no updateStep, no RETHINK logEntry. + expect(navigateTree).not.toHaveBeenCalled(); + expect(store.updateStep).not.toHaveBeenCalled(); + expect(store.logEntry).not.toHaveBeenCalled(); + // Audit warning emitted (task:integrity-warning, database domain). + expect(audit.database).toHaveBeenCalledWith( + expect.objectContaining({ + type: "task:integrity-warning", + target: "FN-001", + metadata: expect.objectContaining({ + guard: "step-reset-blast-radius", + reason: "baseSHA is not an ancestor of HEAD", + }), + }), + ); + }); + + it("fails closed (refuses) when the guard itself throws", async () => { + const store = makeStore(); + const sessionRef = makeSessionRef(); + const blastRadiusGuard = vi.fn().mockRejectedValue(new Error("git exploded")); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf", + ); + + expect(result.ok).toBe(false); + expect(result.reason).toContain("git exploded"); + expect(store.updateStep).not.toHaveBeenCalled(); + }); + + it("proceeds with the reset when the guard returns null (safe)", async () => { + const store = makeStore(); + const navigateTree = vi.fn().mockResolvedValue(undefined); + const sessionRef = makeSessionRef({ navigateTree }); + const blastRadiusGuard = vi.fn().mockResolvedValue(null); + + const result = await resetStepToBaseline( + { store, worktreePath: "/wt", sessionRef, reviewType: "code", blastRadiusGuard }, + makeTask([{ status: "in-progress" }]), + 0, + "baseSHA", + "leaf-checkpoint", + ); + + expect(result.ok).toBe(true); + expect(navigateTree).toHaveBeenCalledWith("leaf-checkpoint", { summarize: false }); + expect(store.updateStep).toHaveBeenCalledWith("FN-001", 0, "pending"); + }); +}); + +describe("makeAncestryBlastRadiusGuard", () => { + beforeEach(() => vi.clearAllMocks()); + + it("refuses when a LATER step is already done", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }, { status: "done" }]), + stepIndex: 0, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("later step 1 is done"); + }); + + it("refuses when a LATER step is already skipped", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }, { status: "skipped" }]), + stepIndex: 0, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("later step 1 is skipped"); + }); + + it("refuses when the baseline is NOT an ancestor of HEAD", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }]), + stepIndex: 0, + isAncestor: async () => false, + }); + const reason = await guard("baseSHA"); + expect(reason).toContain("not an ancestor of HEAD"); + }); + + it("allows when baseline is an ancestor and no later step is terminal", async () => { + const isAncestor = vi.fn().mockResolvedValue(true); + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([ + { status: "pending" }, + { status: "in-progress" }, + { status: "pending" }, + ]), + stepIndex: 1, + isAncestor, + }); + const reason = await guard("baseSHA"); + expect(reason).toBeNull(); + expect(isAncestor).toHaveBeenCalledWith("baseSHA", "/wt"); + }); + + it("allows (skipping ancestry) when no baseline is supplied", async () => { + const isAncestor = vi.fn(); + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "in-progress" }]), + stepIndex: 0, + isAncestor, + }); + const reason = await guard(undefined); + expect(reason).toBeNull(); + expect(isAncestor).not.toHaveBeenCalled(); + }); + + it("treats an earlier done step as harmless (only LATER steps matter)", async () => { + const guard = makeAncestryBlastRadiusGuard({ + worktreePath: "/wt", + task: makeTask([{ status: "done" }, { status: "in-progress" }]), + stepIndex: 1, + isAncestor: async () => true, + }); + const reason = await guard("baseSHA"); + expect(reason).toBeNull(); + }); +}); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index c0013945d0..c94f134d78 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -103,6 +103,7 @@ import type { StuckTaskDetector, StuckTaskEvent } from "./stuck-task-detector.js import type { PluginRunner } from "./plugin-runner.js"; import { isContextLimitError } from "./context-limit-detector.js"; import { StepSessionExecutor } from "./step-session-executor.js"; +import { resetStepToBaseline } from "./step-runner.js"; import { acquireTaskWorktree } from "./worktree-acquisition.js"; import { resolveCapturedBaseCommitSha } from "./base-commit-capture.js"; import { installTaskWorktreeIdentityGuard } from "./worktree-hooks.js"; @@ -7453,61 +7454,31 @@ export class TaskExecutor { } break; case "RETHINK": { - // For code reviews: git reset to baseline to revert file changes - // For plan reviews: skip git reset (no code has been written yet) - if (reviewType === "code" && baseline) { - try { - await execAsync(`git reset --hard ${baseline}`, { cwd: worktreePath }); - executorLog.log(`${taskId}: RETHINK — git reset --hard ${baseline}`); - } catch (gitErr: unknown) { - const gitErrMessage = gitErr instanceof Error ? gitErr.message : String(gitErr); - executorLog.error(`${taskId}: RETHINK git reset failed: ${gitErrMessage}`); - } - } else if (reviewType === "code") { - executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`); - } - - // Rewind conversation to pre-step checkpoint + // RETHINK mechanics (git reset to baseline + session rewind + + // step→pending + RETHINK log entry) are the U2 substrate seam. + // The legacy in-session path delegates to the single extracted + // implementation in step-runner.ts so there is exactly one copy. + // No blast-radius guard here: this path is intra-session with an + // agent-supplied baseline (KTD-2 — the guard is for graph-owned + // shared-isolation resets), so behavior stays byte-identical. const checkpointId = stepCheckpoints.get(stepIndex); - if (checkpointId && sessionRef.current) { - try { - await sessionRef.current.navigateTree(checkpointId, { summarize: false }); - executorLog.log(`${taskId}: RETHINK — session rewound to checkpoint ${checkpointId}`); - } catch (rewindErr: unknown) { - const msg = rewindErr instanceof Error ? rewindErr.message : String(rewindErr); - executorLog.warn(`${taskId}: RETHINK navigateTree rewind failed, falling back to branchWithSummary: ${msg}`); - // Fallback to branchWithSummary - try { - sessionRef.current.sessionManager.branchWithSummary( - checkpointId, - `RETHINK: ${result.summary || "Approach rejected by reviewer"}`, - ); - executorLog.log(`${taskId}: RETHINK — branched from checkpoint ${checkpointId}`); - } catch (branchErr: unknown) { - const branchErrMessage = branchErr instanceof Error ? branchErr.message : String(branchErr); - executorLog.error(`${taskId}: RETHINK session rewind failed: ${branchErrMessage}`); - } - } - } else { - executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`); - } - - // Reset step status to pending - await store.updateStep(taskId, stepIndex, "pending"); + await resetStepToBaseline( + { + store, + worktreePath, + sessionRef, + reviewType: reviewType === "plan" ? "plan" : "code", + summary: result.summary, + }, + { id: taskId, steps: taskSteps }, + stepIndex, + reviewType === "code" ? baseline : undefined, + checkpointId, + ); if (reviewType === "plan") { - await store.logEntry( - taskId, - `RETHINK: Step ${step} plan rewound — session checkpoint ${checkpointId || "N/A"}`, - result.summary, - ); text = `RETHINK\n\nYour plan was rejected. Here is why:\n\n${result.review}\n\nTake a different approach to planning this step. Do NOT repeat the rejected strategy.`; } else { - await store.logEntry( - taskId, - `RETHINK: Step ${step} rewound — git reset to ${baseline || "N/A"}, session checkpoint ${checkpointId || "N/A"}`, - result.summary, - ); text = `RETHINK\n\nYour previous approach was rejected. Here is why:\n\n${result.review}\n\nTake a different approach. Do NOT repeat the rejected strategy. Re-read the step requirements and find an alternative solution.`; } break; diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 9eb88b2391..b60c72874e 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -554,6 +554,21 @@ export { } from "./hold-release.js"; export { StepSessionExecutor } from "./step-session-executor.js"; export type { StepResult, ParallelWave, StepSessionExecutorOptions } from "./step-session-executor.js"; +export { + runTaskStep, + resetStepToBaseline, + makeAncestryBlastRadiusGuard, +} from "./step-runner.js"; +export type { + RunTaskStepDeps, + RunTaskStepOptions, + RunTaskStepResult, + ResetStepDeps, + ResetStepResult, + RunSingleStep, + SessionRef, + StepRunnerTask, +} from "./step-runner.js"; // Multi-project runtime types export { type ProjectRuntime, diff --git a/packages/engine/src/step-runner.ts b/packages/engine/src/step-runner.ts new file mode 100644 index 0000000000..d6d6d54da9 --- /dev/null +++ b/packages/engine/src/step-runner.ts @@ -0,0 +1,375 @@ +/** + * step-runner — the two substrate seams for graph-owned stepwise execution + * (plan 2026-06-04-001, KTD-2 / U2). + * + * This module exposes exactly two capabilities that the workflow-graph executor + * (U3/U5) will drive — it does NOT wire itself into any graph path here: + * + * - {@link runTaskStep} — run exactly step `i` of a task inside its + * session/worktree and return the outcome plus + * the per-step `baselineSha` / `checkpointId` + * that a later RETHINK needs. + * - {@link resetStepToBaseline} — the RETHINK mechanics, extracted verbatim + * from `executor.ts`'s `fn_review_step` RETHINK + * block (`git reset --hard ` + session + * rewind via `navigateTree`/`branchWithSummary` + * fallback + `store.updateStep(..., "pending")`), + * plus a defensive blast-radius guard (KTD-2). + * + * Both are parameterized via an explicit `deps` object (the DI style used by + * `hold-release.ts` / `merge-trait.ts`) so they stay unit-testable without real + * git, real sessions, or a real `StepSessionExecutor`. Production callers (U3/U5) + * pass thin adapters over the existing engine machinery; the legacy in-session + * `fn_review_step` path is untouched and keeps its own copy's behavior — this + * extraction is the single implementation the executor's RETHINK block now + * delegates to (see `TaskExecutor.applyStepRethink`). + */ + +import { exec } from "node:child_process"; +import { promisify } from "node:util"; + +import type { TaskStore } from "@fusion/core"; + +const execAsync = promisify(exec); + +import type { AgentSession as PiAgentSession } from "@earendil-works/pi-coding-agent"; +import { executorLog } from "./logger.js"; +import type { RunAuditor } from "./run-audit.js"; + +// ── Shared minimal shapes ─────────────────────────────────────────────── + +/** The slice of `Task` the step runner reads. */ +export interface StepRunnerTask { + id: string; + steps: Array<{ name?: string; status?: string }>; +} + +/** A minimal session ref mirroring the executor's `{ current: AgentSession }`. */ +export interface SessionRef { + current: PiAgentSession | null; +} + +/** + * Run exactly one step inside the task's session/worktree. Production wires this + * to a {@link import("./step-session-executor.js").StepSessionExecutor} configured + * for a single step (graph-owned runs force step-session physics, KTD-2/KTD-8); + * tests inject a fake. Returns whether the step's session completed successfully. + */ +export type RunSingleStep = (stepIndex: number) => Promise<{ success: boolean; error?: string }>; + +// ── runTaskStep ───────────────────────────────────────────────────────── + +/** Dependencies for {@link runTaskStep}. */ +export interface RunTaskStepDeps { + /** Step-state projection sink (KTD-7). */ + store: Pick; + /** Absolute path to the task's worktree (where `git rev-parse HEAD` runs). */ + worktreePath: string; + /** Run exactly step `i` (step-session physics). */ + runStep: RunSingleStep; + /** + * Capture HEAD in the worktree before step work begins (the per-step baseline, + * KTD-2 documented behavior change). Defaults to + * `git rev-parse HEAD` in {@link RunTaskStepDeps.worktreePath}; inject in tests. + */ + gitRevParse?: (worktreePath: string) => Promise; + /** + * Capture the session checkpoint (leaf) id for the step — observed the same way + * the legacy `stepCheckpoints` map is populated (`session.sessionManager.getLeafId()`). + * Defaults to reading {@link RunTaskStepOptions.sessionRef}; inject in tests. + */ + captureCheckpointId?: () => string | undefined; +} + +/** Options for {@link runTaskStep}. */ +export interface RunTaskStepOptions { + /** Session ref used for the default checkpoint capture. */ + sessionRef?: SessionRef; +} + +/** Result of {@link runTaskStep}. */ +export interface RunTaskStepResult { + outcome: "success" | "failure"; + baselineSha?: string; + checkpointId?: string; +} + +/** + * Drive execution of exactly step `stepIndex` of `task`. + * + * Order of operations (matches the legacy step-session lifecycle the + * characterization tests pin): + * 1. mark the step `in-progress` via `store.updateStep` (projection sink); + * 2. capture `baselineSha` = HEAD in the worktree, BEFORE any step work; + * 3. run exactly step `i` as a step-session (the agent authors its own + * `complete Step N` commit — this driver only observes); + * 4. capture `checkpointId` (session leaf) for a later RETHINK rewind; + * 5. on success, mark the step `done`; on failure, leave the step non-done + * (the graph decides routing — KTD-4). + */ +export async function runTaskStep( + deps: RunTaskStepDeps, + task: StepRunnerTask, + stepIndex: number, + opts: RunTaskStepOptions = {}, +): Promise { + const { store, worktreePath } = deps; + const gitRevParse = deps.gitRevParse ?? defaultGitRevParse; + const captureCheckpointId = + deps.captureCheckpointId ?? (() => defaultCaptureCheckpointId(opts.sessionRef)); + + // 1. Projection: step → in-progress (KTD-7). updateStep's own guards apply. + try { + await store.updateStep(task.id, stepIndex, "in-progress"); + } catch (err) { + executorLog.warn( + `${task.id}: runTaskStep failed to mark step ${stepIndex} in-progress: ${errMsg(err)}`, + ); + } + + // 2. Baseline capture at instance start, before step work (KTD-2). + let baselineSha: string | undefined; + try { + baselineSha = await gitRevParse(worktreePath); + } catch (err) { + executorLog.warn(`${task.id}: runTaskStep baseline capture failed: ${errMsg(err)}`); + } + + // 3. Run exactly step i. The agent authors the commit; we observe only. + const result = await deps.runStep(stepIndex); + + // 4. Capture the session checkpoint (leaf) for a later RETHINK rewind. + let checkpointId: string | undefined; + try { + checkpointId = captureCheckpointId() ?? undefined; + } catch (err) { + executorLog.warn(`${task.id}: runTaskStep checkpoint capture failed: ${errMsg(err)}`); + } + + // 5. Projection: success → done; failure leaves the step non-done. + if (result.success) { + try { + await store.updateStep(task.id, stepIndex, "done"); + } catch (err) { + executorLog.warn( + `${task.id}: runTaskStep failed to mark step ${stepIndex} done: ${errMsg(err)}`, + ); + } + return { outcome: "success", baselineSha, checkpointId }; + } + + return { outcome: "failure", baselineSha, checkpointId }; +} + +// ── resetStepToBaseline ────────────────────────────────────────────────── + +/** Dependencies for {@link resetStepToBaseline}. */ +export interface ResetStepDeps { + /** Step-state projection sink (KTD-7). */ + store: Pick; + /** Absolute path to the task's worktree (where `git reset --hard` runs). */ + worktreePath: string; + /** Session ref for the conversation rewind (`navigateTree` / `branchWithSummary`). */ + sessionRef: SessionRef; + /** + * Review type — `code` reverts file changes via git reset; `plan` skips the + * git reset (no code was written), matching the legacy RETHINK branch. + */ + reviewType?: "code" | "plan"; + /** Optional reviewer summary used as the `branchWithSummary` fallback label. */ + summary?: string; + /** Optional auditor for the blast-radius guard refusal warning (KTD-2). */ + audit?: Pick; + /** + * Blast-radius guard hook (KTD-2, shared isolation). Returns `null` when the + * reset is safe, or a refusal `reason` string when it would destroy other + * steps' approved work (baseline not an ancestor of HEAD, or a later step is + * already done/skipped past the baseline). When omitted the guard is skipped + * (worktree isolation makes it structural — KTD-11). Tests inject a fake; + * production wires {@link makeAncestryBlastRadiusGuard}. + */ + blastRadiusGuard?: (baselineSha: string | undefined) => Promise; +} + +/** Result of {@link resetStepToBaseline}. */ +export interface ResetStepResult { + ok: boolean; + reason?: string; +} + +/** + * Reset step `stepIndex` to its per-step baseline — the verbatim RETHINK + * mechanics extracted from `executor.ts` (`fn_review_step` RETHINK case): + * + * - `git reset --hard ` in the worktree (code review only; skipped + * when `baselineSha` is missing or for plan reviews — today's semantics); + * - session rewind to the pre-step checkpoint via `navigateTree`, falling back + * to `sessionManager.branchWithSummary` (skipped when `checkpointId` is + * missing — today's semantics); + * - `store.updateStep(..., "pending")`. + * + * Before any mutation, the KTD-2 blast-radius guard runs (when provided): on a + * violation it returns `{ ok: false, reason }`, emits an audit warning, and + * mutates NOTHING. + */ +export async function resetStepToBaseline( + deps: ResetStepDeps, + task: StepRunnerTask, + stepIndex: number, + baselineSha?: string, + checkpointId?: string, +): Promise { + const { store, worktreePath, sessionRef } = deps; + const reviewType = deps.reviewType ?? "code"; + const taskId = task.id; + const step = stepIndex + 1; // legacy log lines are 1-indexed + + // ── KTD-2 blast-radius guard — assert BEFORE mutating anything. ────────── + if (deps.blastRadiusGuard) { + let refusal: string | null = null; + try { + refusal = await deps.blastRadiusGuard(baselineSha); + } catch (err) { + // A guard that itself fails is treated as a refusal — fail closed. + refusal = `blast-radius guard error: ${errMsg(err)}`; + } + if (refusal) { + executorLog.warn( + `${taskId}: RETHINK reset for step ${step} REFUSED by blast-radius guard: ${refusal}`, + ); + await deps.audit?.database({ + type: "task:integrity-warning", + target: taskId, + metadata: { + guard: "step-reset-blast-radius", + stepIndex, + baselineSha: baselineSha ?? null, + reason: refusal, + }, + }); + return { ok: false, reason: refusal }; + } + } + + // ── git reset --hard (code reviews only). ───────────────────── + if (reviewType === "code" && baselineSha) { + try { + await execAsync(`git reset --hard ${baselineSha}`, { cwd: worktreePath }); + executorLog.log(`${taskId}: RETHINK — git reset --hard ${baselineSha}`); + } catch (gitErr: unknown) { + executorLog.error(`${taskId}: RETHINK git reset failed: ${errMsg(gitErr)}`); + } + } else if (reviewType === "code") { + executorLog.log(`${taskId}: RETHINK — no baseline SHA, skipping git reset`); + } + + // ── Rewind conversation to the pre-step checkpoint. ────────────────────── + if (checkpointId && sessionRef.current) { + try { + await sessionRef.current.navigateTree(checkpointId, { summarize: false }); + executorLog.log(`${taskId}: RETHINK — session rewound to checkpoint ${checkpointId}`); + } catch (rewindErr: unknown) { + executorLog.warn( + `${taskId}: RETHINK navigateTree rewind failed, falling back to branchWithSummary: ${errMsg(rewindErr)}`, + ); + try { + sessionRef.current.sessionManager.branchWithSummary( + checkpointId, + `RETHINK: ${deps.summary || "Approach rejected by reviewer"}`, + ); + executorLog.log(`${taskId}: RETHINK — branched from checkpoint ${checkpointId}`); + } catch (branchErr: unknown) { + executorLog.error(`${taskId}: RETHINK session rewind failed: ${errMsg(branchErr)}`); + } + } + } else { + executorLog.log(`${taskId}: RETHINK — no session checkpoint for step ${step}, skipping rewind`); + } + + // ── Reset step status to pending (projection sink). ────────────────────── + await store.updateStep(taskId, stepIndex, "pending"); + + if (reviewType === "plan") { + await store.logEntry( + taskId, + `RETHINK: Step ${step} plan rewound — session checkpoint ${checkpointId || "N/A"}`, + deps.summary, + ); + } else { + await store.logEntry( + taskId, + `RETHINK: Step ${step} rewound — git reset to ${baselineSha || "N/A"}, session checkpoint ${checkpointId || "N/A"}`, + deps.summary, + ); + } + + return { ok: true }; +} + +// ── Blast-radius guard factory (shared isolation, KTD-2) ───────────────── + +/** + * Build the shared-isolation blast-radius guard: a reset for step `stepIndex` is + * legal only when (a) `baselineSha` is an ancestor of HEAD in the worktree + * (`git merge-base --is-ancestor`), and (b) no LATER step is already + * `done`/`skipped` (which would postdate the baseline). On violation it returns + * the refusal reason; otherwise `null`. A missing baseline is allowed (the reset + * simply skips its git portion — today's partial-recovery semantics). + */ +export function makeAncestryBlastRadiusGuard(opts: { + worktreePath: string; + task: StepRunnerTask; + stepIndex: number; + isAncestor?: (baselineSha: string, worktreePath: string) => Promise; +}): (baselineSha: string | undefined) => Promise { + const isAncestor = opts.isAncestor ?? defaultIsAncestorOfHead; + return async (baselineSha: string | undefined): Promise => { + // (b) No later step may already be terminal-done past this baseline. + const laterDone = opts.task.steps.findIndex( + (s, i) => i > opts.stepIndex && (s.status === "done" || s.status === "skipped"), + ); + if (laterDone !== -1) { + return `later step ${laterDone} is ${opts.task.steps[laterDone]?.status} — reset would destroy approved work`; + } + // (a) Baseline must be an ancestor of HEAD (skipped when no baseline). + if (baselineSha) { + let ancestor = false; + try { + ancestor = await isAncestor(baselineSha, opts.worktreePath); + } catch (err) { + return `ancestry check failed: ${errMsg(err)}`; + } + if (!ancestor) { + return `baseline ${baselineSha} is not an ancestor of HEAD`; + } + } + return null; + }; +} + +// ── Defaults (production adapters over real git/session) ───────────────── + +async function defaultGitRevParse(worktreePath: string): Promise { + const { stdout } = await execAsync("git rev-parse HEAD", { cwd: worktreePath }); + const sha = stdout.trim(); + return sha.length > 0 ? sha : undefined; +} + +function defaultCaptureCheckpointId(sessionRef?: SessionRef): string | undefined { + const leaf = sessionRef?.current?.sessionManager?.getLeafId?.(); + return leaf ?? undefined; +} + +async function defaultIsAncestorOfHead(baselineSha: string, worktreePath: string): Promise { + try { + await execAsync(`git merge-base --is-ancestor ${baselineSha} HEAD`, { cwd: worktreePath }); + return true; + } catch { + // Non-zero exit → not an ancestor. + return false; + } +} + +function errMsg(err: unknown): string { + return err instanceof Error ? err.message : String(err); +} From 0a3cc50f6ffee74f44c2c21f901f2af11fbf97a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 11:50:45 -0700 Subject: [PATCH 03/22] =?UTF-8?q?feat(core):=20U4-core=20=E2=80=94=20schem?= =?UTF-8?q?a=20v108=20(workflow=5Frun=5Fstep=5Finstances=20+=20tasks.custo?= =?UTF-8?q?mFields),=20instance=20CRUD=20trio,=20literal=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/src/__tests__/db-migrate.test.ts | 74 +++++- packages/core/src/__tests__/db.test.ts | 42 ++-- .../core/src/__tests__/goals-schema.test.ts | 2 +- .../core/src/__tests__/insight-store.test.ts | 10 +- .../__tests__/merge-request-record.test.ts | 2 +- .../core/src/__tests__/mission-store.test.ts | 2 +- packages/core/src/__tests__/run-audit.test.ts | 2 +- .../src/__tests__/store-merge-queue.test.ts | 2 +- .../core/src/__tests__/task-documents.test.ts | 2 +- .../__tests__/workflow-step-instances.test.ts | 222 ++++++++++++++++++ packages/core/src/db.ts | 66 +++++- packages/core/src/store.ts | 117 ++++++++- packages/core/src/types.ts | 63 +++++ 13 files changed, 560 insertions(+), 46 deletions(-) create mode 100644 packages/core/src/__tests__/workflow-step-instances.test.ts diff --git a/packages/core/src/__tests__/db-migrate.test.ts b/packages/core/src/__tests__/db-migrate.test.ts index 8a989400d8..b36c04e128 100644 --- a/packages/core/src/__tests__/db-migrate.test.ts +++ b/packages/core/src/__tests__/db-migrate.test.ts @@ -715,7 +715,7 @@ describe("schema migration", () => { const row = db.prepare("SELECT deletedAt FROM tasks WHERE id = 'FN-legacy'").get() as { deletedAt: string | null }; expect(row.deletedAt).toBeNull(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -748,7 +748,7 @@ describe("schema migration", () => { { id: "WS-001", mode: "prompt", gateMode: "advisory" }, { id: "WS-002", mode: "script", gateMode: "advisory" }, ]); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -798,7 +798,7 @@ describe("schema migration", () => { reviewerContextRetryCount: 0, reviewerFallbackRetryCount: 0, }); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -827,7 +827,7 @@ describe("schema migration", () => { const columns = db.prepare("PRAGMA table_info(milestones)").all() as Array<{ name: string }>; expect(columns.map((column) => column.name)).toContain("acceptanceCriteria"); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -868,7 +868,7 @@ describe("schema migration", () => { const missionColumns = db.prepare("PRAGMA table_info(missions)").all() as Array<{ name: string }>; expect(missionColumns.map((column) => column.name)).toContain("autoMerge"); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -902,7 +902,7 @@ describe("schema migration", () => { { id: "WS-002", mode: "script", enabled: 1, gateMode: "advisory" }, { id: "WS-003", mode: "prompt", enabled: 0, gateMode: "advisory" }, ]); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -939,8 +939,68 @@ describe("schema migration", () => { const indexes = db.prepare("PRAGMA index_list(mission_goals)").all() as Array<{ name: string }>; expect(indexes.some((index) => index.name === "idxMissionGoalsGoalId")).toBe(true); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); + + it("adds workflow_run_step_instances table + tasks.customFields when migrating from schema version 107", () => { + const db = new Database(fusionDir); + db.exec("CREATE TABLE IF NOT EXISTS __meta (key TEXT PRIMARY KEY, value TEXT)"); + db.exec("INSERT INTO __meta (key, value) VALUES ('schemaVersion', '107')"); + db.exec("INSERT INTO __meta (key, value) VALUES ('lastModified', '1000')"); + db.exec(` + CREATE TABLE IF NOT EXISTS tasks ( + id TEXT PRIMARY KEY, + description TEXT NOT NULL, + "column" TEXT NOT NULL, + createdAt TEXT NOT NULL, + updatedAt TEXT NOT NULL + ) + `); + + db.init(); + + // The new per-step-instance run-state table exists with its index. + const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table'").all() as Array<{ name: string }>; + expect(tables.map((row) => row.name)).toContain("workflow_run_step_instances"); + + const stepInstanceColumns = db + .prepare("PRAGMA table_info(workflow_run_step_instances)") + .all() as Array<{ name: string }>; + expect(stepInstanceColumns.map((column) => column.name)).toEqual([ + "taskId", + "runId", + "foreachNodeId", + "stepIndex", + "pinnedStepCount", + "currentNodeId", + "status", + "baselineSha", + "checkpointId", + "reworkCount", + "branchName", + "integratedAt", + "updatedAt", + ]); + + const stepInstanceIndexes = db + .prepare("PRAGMA index_list(workflow_run_step_instances)") + .all() as Array<{ name: string }>; + expect( + stepInstanceIndexes.some((index) => index.name === "idx_workflow_run_step_instances_task_run"), + ).toBe(true); + + // tasks.customFields column is added with a default-'{}' definition. + const taskColumns = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ + name: string; + dflt_value: string | null; + }>; + const customFieldsColumn = taskColumns.find((column) => column.name === "customFields"); + expect(customFieldsColumn).toBeDefined(); + expect(customFieldsColumn?.dflt_value).toBe("'{}'"); + + expect(db.getSchemaVersion()).toBe(108); + db.close(); + }); }); diff --git a/packages/core/src/__tests__/db.test.ts b/packages/core/src/__tests__/db.test.ts index 7ef8601b31..8a618bd583 100644 --- a/packages/core/src/__tests__/db.test.ts +++ b/packages/core/src/__tests__/db.test.ts @@ -334,7 +334,7 @@ describe("Database", () => { }); it("seeds schema version", () => { - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); it("includes tokenUsageCacheWriteTokens on freshly initialized tasks table", () => { @@ -393,7 +393,7 @@ describe("Database", () => { it("is idempotent - calling init() twice does not fail", () => { expect(() => db.init()).not.toThrow(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); it("does not overwrite existing config on re-init", () => { // Update the config @@ -1463,7 +1463,7 @@ describe("schema migrations", () => { db.init(); // Verify version bumped to 29 (includes v1→v2 through v26→v29) - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); // Verify new columns exist and existing data is intact const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; @@ -1488,11 +1488,11 @@ describe("schema migrations", () => { const db = new Database(fusionDir); db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); // Re-init should not fail db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); db.close(); }); @@ -1527,7 +1527,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; expect(cols.map((col) => col.name)).toContain("priority"); @@ -1568,7 +1568,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const colNames = cols.map((col) => col.name); @@ -1640,7 +1640,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const colNames = cols.map((col) => col.name); @@ -1880,7 +1880,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const cols = db.prepare("PRAGMA table_info(chat_messages)").all() as Array<{ name: string }>; expect(cols.map((col) => col.name)).toContain("attachments"); @@ -1954,7 +1954,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'agentRatings'").all() as Array<{ name: string }>; expect(tables).toEqual([{ name: "agentRatings" }]); @@ -1978,7 +1978,7 @@ describe("schema migrations", () => { db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const tables = db.prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'mission_events'").all() as Array<{ name: string }>; expect(tables).toEqual([{ name: "mission_events" }]); @@ -2082,7 +2082,7 @@ describe("schema migrations", () => { db.init(); // Verify version bumped to 29 - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); // Verify new columns exist and existing data is intact const cols = db.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; @@ -2301,7 +2301,7 @@ describe("schema migrations", () => { localDb.init(); - expect(localDb.getSchemaVersion()).toBe(107); + expect(localDb.getSchemaVersion()).toBe(108); const columns = localDb.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; expect(columns.map((column) => column.name)).toContain("tokenUsageCacheWriteTokens"); @@ -2612,7 +2612,7 @@ describe("createDatabase factory", () => { const db = createDatabase(fusionDir); db.init(); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); expect(db.getLastModified()).toBeGreaterThan(0); db.close(); @@ -2766,7 +2766,7 @@ describe("migration v77 task token budget columns", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(107); + expect(migrated.getSchemaVersion()).toBe(108); const rows = migrated.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>; const names = new Set(rows.map((row) => row.name)); expect(names.has("tokenBudgetSoftAlertedAt")).toBe(true); @@ -2797,7 +2797,7 @@ describe("migration v106 adds tasks.transitionPending (FN-1417)", () => { const fresh = new Database(fusion); try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(107); + expect(fresh.getSchemaVersion()).toBe(108); const names = new Set( (fresh.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>).map((r) => r.name), ); @@ -2825,7 +2825,7 @@ describe("migration v106 adds tasks.transitionPending (FN-1417)", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(107); + expect(migrated.getSchemaVersion()).toBe(108); const names = new Set( (migrated.prepare("PRAGMA table_info(tasks)").all() as Array<{ name: string }>).map((r) => r.name), ); @@ -2851,7 +2851,7 @@ describe("migration v107 adds workflow_run_branches + index (FN-1417)", () => { const fresh = new Database(fusion); try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(107); + expect(fresh.getSchemaVersion()).toBe(108); const table = fresh .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'workflow_run_branches'") .get() as { name: string } | undefined; @@ -2885,7 +2885,7 @@ describe("migration v107 adds workflow_run_branches + index (FN-1417)", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(107); + expect(migrated.getSchemaVersion()).toBe(108); const table = migrated .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name = 'workflow_run_branches'") .get() as { name: string } | undefined; @@ -2926,7 +2926,7 @@ describe("migration v67 drops orphan project auth tables", () => { migrated = new Database(fusion); migrated.init(); - expect(migrated.getSchemaVersion()).toBe(107); + expect(migrated.getSchemaVersion()).toBe(108); const tables = migrated .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name LIKE 'project_auth_%'") .all() as Array<{ name: string }>; @@ -2953,7 +2953,7 @@ describe("migration v67 drops orphan project auth tables", () => { try { fresh.init(); - expect(fresh.getSchemaVersion()).toBe(107); + expect(fresh.getSchemaVersion()).toBe(108); const tables = fresh .prepare("SELECT name FROM sqlite_master WHERE type='table' AND name LIKE 'project_auth_%'") .all() as Array<{ name: string }>; diff --git a/packages/core/src/__tests__/goals-schema.test.ts b/packages/core/src/__tests__/goals-schema.test.ts index 1e449be32b..333a68e551 100644 --- a/packages/core/src/__tests__/goals-schema.test.ts +++ b/packages/core/src/__tests__/goals-schema.test.ts @@ -91,6 +91,6 @@ describe("goals schema", () => { }); it("reports schema version 101", () => { - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); }); diff --git a/packages/core/src/__tests__/insight-store.test.ts b/packages/core/src/__tests__/insight-store.test.ts index 447411ad52..7aa0e611ad 100644 --- a/packages/core/src/__tests__/insight-store.test.ts +++ b/packages/core/src/__tests__/insight-store.test.ts @@ -1000,7 +1000,7 @@ describe("Migration: pre-33 DB upgrade", () => { // Step 1: Create a fresh database at v33 (runs all migrations up to 33) const db1 = createDatabase(legacyDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(107); + expect(db1.getSchemaVersion()).toBe(108); db1.close(); // Step 2: Manually downgrade to version 32 and drop insight tables @@ -1035,7 +1035,7 @@ describe("Migration: pre-33 DB upgrade", () => { expect(tableNamesBefore).not.toContain("project_insight_runs"); // Now run init — this triggers the v32→v33 migration db3.init(); - expect(db3.getSchemaVersion()).toBe(107); + expect(db3.getSchemaVersion()).toBe(108); // Step 4: Verify insight tables exist after migration const tablesAfter = db3.prepare( @@ -1066,12 +1066,12 @@ describe("Migration: pre-33 DB upgrade", () => { try { const db1 = createDatabase(testDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(107); + expect(db1.getSchemaVersion()).toBe(108); db1.close(); const db2 = createDatabase(testDir); expect(() => db2.init()).not.toThrow(); - expect(db2.getSchemaVersion()).toBe(107); + expect(db2.getSchemaVersion()).toBe(108); db2.close(); } finally { rmSync(testDir, { recursive: true, force: true }); @@ -1085,7 +1085,7 @@ describe("Migration: pre-33 DB upgrade", () => { // Step 1: Create a fresh DB and run migrations const db1 = createDatabase(compatDir); db1.init(); - expect(db1.getSchemaVersion()).toBe(107); + expect(db1.getSchemaVersion()).toBe(108); // Step 2: Strip lifecycle and cancelledAt columns by recreating the // table without them. This simulates a DB that was created before the diff --git a/packages/core/src/__tests__/merge-request-record.test.ts b/packages/core/src/__tests__/merge-request-record.test.ts index fd508a69be..ef2a10d136 100644 --- a/packages/core/src/__tests__/merge-request-record.test.ts +++ b/packages/core/src/__tests__/merge-request-record.test.ts @@ -38,7 +38,7 @@ describe("TaskStore merge request record + completion handoff marker", () => { .all() as Array<{ name: string }>; expect(tableRows).toEqual([{ name: "completion_handoff_markers" }, { name: "merge_requests" }]); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); it("upserts merge request records", async () => { diff --git a/packages/core/src/__tests__/mission-store.test.ts b/packages/core/src/__tests__/mission-store.test.ts index fbff57d141..7410346c67 100644 --- a/packages/core/src/__tests__/mission-store.test.ts +++ b/packages/core/src/__tests__/mission-store.test.ts @@ -3746,7 +3746,7 @@ describe("MissionStore", () => { describe("Loop State & Validator Run Schema (v31)", () => { it("schema version is 101 after migration", () => { - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); it("mission_features table has loop state columns", () => { diff --git a/packages/core/src/__tests__/run-audit.test.ts b/packages/core/src/__tests__/run-audit.test.ts index c64b9a50f3..3be684ea9a 100644 --- a/packages/core/src/__tests__/run-audit.test.ts +++ b/packages/core/src/__tests__/run-audit.test.ts @@ -584,7 +584,7 @@ describe("Run Audit", () => { }); it("schema version is bumped to 40", () => { - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); }); }); }); diff --git a/packages/core/src/__tests__/store-merge-queue.test.ts b/packages/core/src/__tests__/store-merge-queue.test.ts index 6d2b141a97..6c09641156 100644 --- a/packages/core/src/__tests__/store-merge-queue.test.ts +++ b/packages/core/src/__tests__/store-merge-queue.test.ts @@ -60,7 +60,7 @@ describe("TaskStore merge queue", () => { expect.arrayContaining(["idx_mergeQueue_lease_ready", "idx_mergeQueue_leaseExpiresAt"]), ); - expect(store.getDatabase().getSchemaVersion()).toBe(107); + expect(store.getDatabase().getSchemaVersion()).toBe(108); }); it("migrates a legacy v88 database and preserves task rows", async () => { diff --git a/packages/core/src/__tests__/task-documents.test.ts b/packages/core/src/__tests__/task-documents.test.ts index ccde60a72e..7352ec08bf 100644 --- a/packages/core/src/__tests__/task-documents.test.ts +++ b/packages/core/src/__tests__/task-documents.test.ts @@ -51,7 +51,7 @@ describe("TaskStore task documents", () => { expect(tableNames.has("task_documents")).toBe(true); expect(tableNames.has("task_document_revisions")).toBe(true); - expect(db.getSchemaVersion()).toBe(107); + expect(db.getSchemaVersion()).toBe(108); const index = db .prepare( diff --git a/packages/core/src/__tests__/workflow-step-instances.test.ts b/packages/core/src/__tests__/workflow-step-instances.test.ts new file mode 100644 index 0000000000..1d301698b9 --- /dev/null +++ b/packages/core/src/__tests__/workflow-step-instances.test.ts @@ -0,0 +1,222 @@ +import { describe, it, expect, beforeEach, afterEach } from "vitest"; + +import type { WorkflowRunStepInstance } from "../types.js"; +import { createTaskStoreTestHarness } from "./store-test-helpers.js"; + +/** + * Step-inversion U4 (KTD-6/KTD-13): persistence groundwork for the foreach + * step-instance region. Covers the workflow_run_step_instances CRUD trio + * (save/load/clear) — upsert-on-conflict, per-run pruning, load ordering — plus + * the raw tasks.customFields JSON round-trip through create/update/get. + * + * The CRUD trio mirrors workflow_run_branches: a `save` is an idempotent UPSERT + * keyed by (taskId, runId, foreachNodeId, stepIndex); `load` returns the run's + * rows ordered by stepIndex; `clear` prunes either everything-but-a-kept-run + * (per-run prune) or, with no runId, every row for the task. + */ + +describe("workflow_run_step_instances CRUD (U4, KTD-6)", () => { + const harness = createTaskStoreTestHarness(); + let store: ReturnType; + + beforeEach(async () => { + await harness.beforeEach(); + store = harness.store(); + }); + afterEach(async () => { + await harness.afterEach(); + }); + + type StepInstanceStore = { + saveWorkflowRunStepInstance(state: WorkflowRunStepInstance): void; + loadWorkflowRunStepInstances(taskId: string, runId: string): WorkflowRunStepInstance[]; + clearWorkflowRunStepInstances(taskId: string, keepRunId?: string): void; + }; + const sis = (): StepInstanceStore => store as unknown as StepInstanceStore; + + function rawCount(taskId: string): number { + const db = (store as unknown as { db: { prepare: (s: string) => { get: (...a: unknown[]) => unknown } } }).db; + const row = db + .prepare("SELECT COUNT(*) AS c FROM workflow_run_step_instances WHERE taskId = ?") + .get(taskId) as { c: number }; + return row.c; + } + + function makeInstance(overrides: Partial = {}): WorkflowRunStepInstance { + return { + taskId: "T-1", + runId: "r1", + foreachNodeId: "fe", + stepIndex: 0, + pinnedStepCount: 3, + currentNodeId: "n1", + status: "in-progress", + baselineSha: "abc123", + checkpointId: "ckpt-1", + reworkCount: 0, + branchName: null, + integratedAt: null, + updatedAt: "2026-06-04T00:00:00.000Z", + ...overrides, + }; + } + + it("round-trips a full instance row through save → load", async () => { + const t = await store.createTask({ description: "stepped" }); + const inst = makeInstance({ + taskId: t.id, + branchName: "step/0", + integratedAt: "2026-06-04T01:00:00.000Z", + status: "completed", + reworkCount: 2, + }); + sis().saveWorkflowRunStepInstance(inst); + + const [loaded] = sis().loadWorkflowRunStepInstances(t.id, "r1"); + expect(loaded.taskId).toBe(t.id); + expect(loaded.runId).toBe("r1"); + expect(loaded.foreachNodeId).toBe("fe"); + expect(loaded.stepIndex).toBe(0); + expect(loaded.pinnedStepCount).toBe(3); + expect(loaded.currentNodeId).toBe("n1"); + expect(loaded.status).toBe("completed"); + expect(loaded.baselineSha).toBe("abc123"); + expect(loaded.checkpointId).toBe("ckpt-1"); + expect(loaded.reworkCount).toBe(2); + expect(loaded.branchName).toBe("step/0"); + expect(loaded.integratedAt).toBe("2026-06-04T01:00:00.000Z"); + expect(typeof loaded.updatedAt).toBe("string"); + }); + + it("save UPSERTS on (taskId, runId, foreachNodeId, stepIndex) conflict", async () => { + const t = await store.createTask({ description: "upsert" }); + sis().saveWorkflowRunStepInstance( + makeInstance({ taskId: t.id, stepIndex: 0, currentNodeId: "n1", status: "in-progress", reworkCount: 0 }), + ); + // Same PK — overwrites in place, not a second row. + sis().saveWorkflowRunStepInstance( + makeInstance({ taskId: t.id, stepIndex: 0, currentNodeId: "n5", status: "completed", reworkCount: 1 }), + ); + // Different stepIndex — a new row. + sis().saveWorkflowRunStepInstance( + makeInstance({ taskId: t.id, stepIndex: 1, currentNodeId: "n2", status: "pending" }), + ); + + expect(rawCount(t.id)).toBe(2); + const loaded = sis().loadWorkflowRunStepInstances(t.id, "r1"); + const step0 = loaded.find((row) => row.stepIndex === 0); + expect(step0?.currentNodeId).toBe("n5"); + expect(step0?.status).toBe("completed"); + expect(step0?.reworkCount).toBe(1); + }); + + it("persists nullable anchors as null and reads them back as null", async () => { + const t = await store.createTask({ description: "nulls" }); + sis().saveWorkflowRunStepInstance( + makeInstance({ + taskId: t.id, + currentNodeId: null, + baselineSha: null, + checkpointId: null, + branchName: null, + integratedAt: null, + status: "pending", + }), + ); + const [loaded] = sis().loadWorkflowRunStepInstances(t.id, "r1"); + expect(loaded.currentNodeId).toBeNull(); + expect(loaded.baselineSha).toBeNull(); + expect(loaded.checkpointId).toBeNull(); + expect(loaded.branchName).toBeNull(); + expect(loaded.integratedAt).toBeNull(); + }); + + it("loadWorkflowRunStepInstances returns the run ordered by stepIndex", async () => { + const t = await store.createTask({ description: "ordered" }); + // Insert out of order. + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, stepIndex: 2, currentNodeId: "n2" })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, stepIndex: 0, currentNodeId: "n0" })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, stepIndex: 1, currentNodeId: "n1" })); + + const loaded = sis().loadWorkflowRunStepInstances(t.id, "r1"); + expect(loaded.map((row) => row.stepIndex)).toEqual([0, 1, 2]); + }); + + it("loadWorkflowRunStepInstances scopes to the requested run only", async () => { + const t = await store.createTask({ description: "scoped" }); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "r1", stepIndex: 0 })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "r2", stepIndex: 0 })); + expect(sis().loadWorkflowRunStepInstances(t.id, "r1").length).toBe(1); + expect(sis().loadWorkflowRunStepInstances(t.id, "r2").length).toBe(1); + }); + + it("clear with keepRunId prunes every other run, keeps the kept run", async () => { + const t = await store.createTask({ description: "prune" }); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "old", stepIndex: 0 })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "old", stepIndex: 1 })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "cur", stepIndex: 0 })); + + sis().clearWorkflowRunStepInstances(t.id, "cur"); + + expect(rawCount(t.id)).toBe(1); + expect(sis().loadWorkflowRunStepInstances(t.id, "old").length).toBe(0); + expect(sis().loadWorkflowRunStepInstances(t.id, "cur").length).toBe(1); + }); + + it("clear with no keepRunId prunes all rows for the task", async () => { + const t = await store.createTask({ description: "wipe" }); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "r1", stepIndex: 0 })); + sis().saveWorkflowRunStepInstance(makeInstance({ taskId: t.id, runId: "r2", stepIndex: 0 })); + + sis().clearWorkflowRunStepInstances(t.id); + + expect(rawCount(t.id)).toBe(0); + }); +}); + +describe("tasks.customFields raw JSON round-trip (U4 groundwork for KTD-13)", () => { + const harness = createTaskStoreTestHarness(); + let store: ReturnType; + + beforeEach(async () => { + await harness.beforeEach(); + store = harness.store(); + }); + afterEach(async () => { + await harness.afterEach(); + }); + + it("a freshly created task has no customFields (legacy-shape default)", async () => { + const t = await store.createTask({ description: "no fields" }); + const got = await store.getTask(t.id); + // Stored default is '{}' which parses to an empty object; the row→Task map + // surfaces that as an empty object, distinguishable from later writes. + expect(got?.customFields).toEqual({}); + }); + + it("round-trips a customFields object through updateTask → getTask", async () => { + const t = await store.createTask({ description: "fielded" }); + await store.updateTask(t.id, { + customFields: { severity: "high", points: 3, flagged: true, tags: ["a", "b"] }, + }); + const got = await store.getTask(t.id); + expect(got?.customFields).toEqual({ severity: "high", points: 3, flagged: true, tags: ["a", "b"] }); + }); + + it("updateTask treats customFields as a whole-object opaque patch (replaces, not merges)", async () => { + const t = await store.createTask({ description: "replace" }); + await store.updateTask(t.id, { customFields: { a: 1, b: 2 } }); + await store.updateTask(t.id, { customFields: { a: 9 } }); + const got = await store.getTask(t.id); + // Whole-object replacement: `b` is gone. (Merge/validation is a later unit.) + expect(got?.customFields).toEqual({ a: 9 }); + }); + + it("leaves customFields untouched when an unrelated field is updated", async () => { + const t = await store.createTask({ description: "untouched" }); + await store.updateTask(t.id, { customFields: { keep: "me" } }); + await store.updateTask(t.id, { summary: "an unrelated change" }); + const got = await store.getTask(t.id); + expect(got?.customFields).toEqual({ keep: "me" }); + }); +}); diff --git a/packages/core/src/db.ts b/packages/core/src/db.ts index 0b9d5a0dba..6403a186f1 100644 --- a/packages/core/src/db.ts +++ b/packages/core/src/db.ts @@ -149,7 +149,7 @@ export function probeFts5(db: DatabaseSync): boolean { // ── Schema Definition ──────────────────────────────────────────────── -const SCHEMA_VERSION = 107; +const SCHEMA_VERSION = 108; export { SCHEMA_VERSION }; @@ -323,7 +323,8 @@ CREATE TABLE IF NOT EXISTS tasks ( checkoutLeaseEpoch INTEGER DEFAULT 0, deletedAt TEXT, allowResurrection INTEGER DEFAULT 0, - transitionPending TEXT + transitionPending TEXT, + customFields TEXT DEFAULT '{}' ); -- Config table (single row with project settings) @@ -589,6 +590,32 @@ CREATE TABLE IF NOT EXISTS workflow_run_branches ( ); CREATE INDEX IF NOT EXISTS idx_workflow_run_branches_task_run ON workflow_run_branches(taskId, runId); +-- Per-step-instance run state for the step-inversion foreach region (step-inversion +-- U4, KTD-6). One row per expanded step instance inside a foreach region; resume +-- reconstructs the instance set from pinnedStepCount + persisted currentNodeId/ +-- reworkCount without re-running completed instances. baselineSha/checkpointId +-- persist the RETHINK reset anchors (previously in-memory, lost on restart). +-- branchName/integratedAt and the "awaiting-integration" status serve parallel +-- mode (KTD-11) and are null/unused at concurrency 1. Additive-only, reconstructible. +-- status ∈ "pending" | "in-progress" | "awaiting-integration" | "completed" | "failed". +CREATE TABLE IF NOT EXISTS workflow_run_step_instances ( + taskId TEXT NOT NULL REFERENCES tasks(id) ON DELETE CASCADE, + runId TEXT NOT NULL, + foreachNodeId TEXT NOT NULL, + stepIndex INTEGER NOT NULL, + pinnedStepCount INTEGER NOT NULL, + currentNodeId TEXT, + status TEXT NOT NULL, + baselineSha TEXT, + checkpointId TEXT, + reworkCount INTEGER NOT NULL DEFAULT 0, + branchName TEXT, + integratedAt TEXT, + updatedAt TEXT NOT NULL, + PRIMARY KEY (taskId, runId, foreachNodeId, stepIndex) +); +CREATE INDEX IF NOT EXISTS idx_workflow_run_step_instances_task_run ON workflow_run_step_instances(taskId, runId); + -- Task documents (key-value store per task with revision tracking) CREATE TABLE IF NOT EXISTS task_documents ( id TEXT PRIMARY KEY, @@ -4229,6 +4256,41 @@ export class Database { }); } + // Migration 108: Step-inversion persistence (step-inversion U4, KTD-6/KTD-13). + // Adds workflow_run_step_instances — one row per expanded step instance inside a + // foreach region — so a crashed/restarted run reconstructs the instance set from + // pinnedStepCount + persisted currentNodeId/reworkCount, and the RETHINK reset + // anchors (baselineSha/checkpointId) survive restart (previously in-memory Maps). + // branchName/integratedAt + "awaiting-integration" status serve parallel mode + // (KTD-11; null/unused at concurrency 1). Also adds tasks.customFields (KTD-13), + // the JSON store for workflow-defined custom task field values. Additive-only, + // idempotent (table-exists / addColumnIfMissing guards); no backfill. + // status ∈ "pending" | "in-progress" | "awaiting-integration" | "completed" | "failed". + if (version < 108) { + this.applyMigration(108, () => { + this.db.exec(` + CREATE TABLE IF NOT EXISTS workflow_run_step_instances ( + taskId TEXT NOT NULL REFERENCES tasks(id) ON DELETE CASCADE, + runId TEXT NOT NULL, + foreachNodeId TEXT NOT NULL, + stepIndex INTEGER NOT NULL, + pinnedStepCount INTEGER NOT NULL, + currentNodeId TEXT, + status TEXT NOT NULL, + baselineSha TEXT, + checkpointId TEXT, + reworkCount INTEGER NOT NULL DEFAULT 0, + branchName TEXT, + integratedAt TEXT, + updatedAt TEXT NOT NULL, + PRIMARY KEY (taskId, runId, foreachNodeId, stepIndex) + ); + CREATE INDEX IF NOT EXISTS idx_workflow_run_step_instances_task_run ON workflow_run_step_instances(taskId, runId); + `); + this.addColumnIfMissing("tasks", "customFields", "TEXT DEFAULT '{}'"); + }); + } + } /** diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index a477d46f3e..f4ddcf46e7 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -207,6 +207,7 @@ interface TaskRow { executionCompletedAt: string | null; dependencies: string | null; steps: string | null; + customFields: string | null; log: string | null; attachments: string | null; steeringComments: string | null; @@ -1778,6 +1779,7 @@ export class TaskStore extends EventEmitter { executionCompletedAt: row.executionCompletedAt || undefined, dependencies: fromJson(row.dependencies) || [], steps: fromJson(row.steps) || [], + customFields: fromJson>(row.customFields) ?? undefined, log: fromJson(row.log) || [], tokenBudgetSoftAlertedAt: row.tokenBudgetSoftAlertedAt || undefined, tokenBudgetHardAlertedAt: row.tokenBudgetHardAlertedAt || undefined, @@ -1925,6 +1927,7 @@ export class TaskStore extends EventEmitter { dependencies: entry.dependencies ?? [], steps: entry.steps ?? [], currentStep: entry.currentStep ?? 0, + customFields: entry.customFields ?? undefined, size: entry.size, reviewLevel: entry.reviewLevel, prInfo: slim ? undefined : entry.prInfo, @@ -2057,6 +2060,7 @@ export class TaskStore extends EventEmitter { dependencies: task.dependencies, steps: task.steps, currentStep: task.currentStep, + customFields: task.customFields, size: task.size, reviewLevel: task.reviewLevel, prInfo: task.prInfo, @@ -2265,7 +2269,7 @@ export class TaskStore extends EventEmitter { "error", "summary", "thinkingLevel", "executionMode", "tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride", "createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt", - "dependencies", "steps", "comments", "review", "reviewState", "workflowStepResults", "steeringComments", + "dependencies", "steps", "customFields", "comments", "review", "reviewState", "workflowStepResults", "steeringComments", "attachments", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "mergeDetails", "breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles", "missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource", @@ -2314,7 +2318,7 @@ export class TaskStore extends EventEmitter { "error", "summary", "thinkingLevel", "executionMode", "tokenUsageInputTokens", "tokenUsageOutputTokens", "tokenUsageCachedTokens", "tokenUsageCacheWriteTokens", "tokenUsageTotalTokens", "tokenUsageFirstUsedAt", "tokenUsageLastUsedAt", "tokenBudgetSoftAlertedAt", "tokenBudgetHardAlertedAt", "tokenBudgetOverride", "createdAt", "updatedAt", "columnMovedAt", "firstExecutionAt", "cumulativeActiveMs", "executionStartedAt", "executionCompletedAt", - "dependencies", "steps", "attachments", "steeringComments", + "dependencies", "steps", "customFields", "attachments", "steeringComments", "comments", "review", "reviewState", "workflowStepResults", "prInfo", "prInfos", "issueInfo", "githubTracking", "sourceIssueProvider", "sourceIssueRepository", "sourceIssueExternalIssueId", "sourceIssueNumber", "sourceIssueUrl", "mergeDetails", "breakIntoSubtasks", "noCommitsExpected", "enabledWorkflowSteps", "modifiedFiles", "missionId", "sliceId", "scopeOverride", "scopeOverrideReason", "scopeAutoWiden", "assignedAgentId", "pausedByAgentId", "assigneeUserId", "nodeId", "effectiveNodeId", "effectiveNodeSource", @@ -2416,6 +2420,7 @@ export class TaskStore extends EventEmitter { task.executionCompletedAt ?? null, toJson(task.dependencies || []), toJson(task.steps || []), + toJson(task.customFields ?? {}), toJson(task.log || []), toJson(task.attachments || []), toJson(task.steeringComments || []), @@ -2483,7 +2488,7 @@ export class TaskStore extends EventEmitter { summary, thinkingLevel, executionMode, tokenUsageInputTokens, tokenUsageOutputTokens, tokenUsageCachedTokens, tokenUsageCacheWriteTokens, tokenUsageTotalTokens, tokenUsageFirstUsedAt, tokenUsageLastUsedAt, tokenBudgetSoftAlertedAt, tokenBudgetHardAlertedAt, tokenBudgetOverride, createdAt, updatedAt, columnMovedAt, firstExecutionAt, cumulativeActiveMs, executionStartedAt, executionCompletedAt, - dependencies, steps, log, attachments, steeringComments, + dependencies, steps, customFields, log, attachments, steeringComments, comments, review, reviewState, workflowStepResults, prInfo, prInfos, issueInfo, githubTracking, sourceIssueProvider, sourceIssueRepository, sourceIssueExternalIssueId, sourceIssueNumber, sourceIssueUrl, mergeDetails, breakIntoSubtasks, noCommitsExpected, autoMerge, enabledWorkflowSteps, modifiedFiles, missionId, sliceId, scopeOverride, scopeOverrideReason, scopeAutoWiden, assignedAgentId, pausedByAgentId, assigneeUserId, nodeId, effectiveNodeId, effectiveNodeSource, sourceType, sourceAgentId, sourceRunId, sourceSessionId, sourceMessageId, sourceParentTaskId, sourceMetadata, checkedOutBy, checkedOutAt, checkoutNodeId, checkoutRunId, checkoutLeaseRenewedAt, checkoutLeaseEpoch, deletedAt, allowResurrection @@ -2510,7 +2515,7 @@ export class TaskStore extends EventEmitter { summary, thinkingLevel, executionMode, tokenUsageInputTokens, tokenUsageOutputTokens, tokenUsageCachedTokens, tokenUsageCacheWriteTokens, tokenUsageTotalTokens, tokenUsageFirstUsedAt, tokenUsageLastUsedAt, tokenBudgetSoftAlertedAt, tokenBudgetHardAlertedAt, tokenBudgetOverride, createdAt, updatedAt, columnMovedAt, firstExecutionAt, cumulativeActiveMs, executionStartedAt, executionCompletedAt, - dependencies, steps, log, attachments, steeringComments, + dependencies, steps, customFields, log, attachments, steeringComments, comments, review, reviewState, workflowStepResults, prInfo, prInfos, issueInfo, githubTracking, sourceIssueProvider, sourceIssueRepository, sourceIssueExternalIssueId, sourceIssueNumber, sourceIssueUrl, mergeDetails, breakIntoSubtasks, noCommitsExpected, autoMerge, enabledWorkflowSteps, modifiedFiles, missionId, sliceId, scopeOverride, scopeOverrideReason, scopeAutoWiden, assignedAgentId, pausedByAgentId, assigneeUserId, nodeId, effectiveNodeId, effectiveNodeSource, sourceType, sourceAgentId, sourceRunId, sourceSessionId, sourceMessageId, sourceParentTaskId, sourceMetadata, checkedOutBy, checkedOutAt, checkoutNodeId, checkoutRunId, checkoutLeaseRenewedAt, checkoutLeaseEpoch, deletedAt, allowResurrection @@ -2585,6 +2590,7 @@ export class TaskStore extends EventEmitter { executionCompletedAt = excluded.executionCompletedAt, dependencies = excluded.dependencies, steps = excluded.steps, + customFields = excluded.customFields, log = excluded.log, attachments = excluded.attachments, steeringComments = excluded.steeringComments, @@ -5295,6 +5301,103 @@ export class TaskStore extends EventEmitter { } } + /** + * Persist (idempotent upsert) one step instance's run-state inside a foreach + * region (step-inversion U4, KTD-6). Keyed by (taskId, runId, foreachNodeId, + * stepIndex) — the table PK — so re-writing the same instance overwrites its + * single row with the latest currentNodeId/status/anchors. `updatedAt` is + * stamped server-side. Mirrors `saveWorkflowRunBranch`: additive, silently + * no-ops on a legacy/missing table. + */ + saveWorkflowRunStepInstance( + state: import("./types.js").WorkflowRunStepInstance, + ): void { + try { + this.db + .prepare( + `INSERT INTO workflow_run_step_instances + (taskId, runId, foreachNodeId, stepIndex, pinnedStepCount, currentNodeId, status, baselineSha, checkpointId, reworkCount, branchName, integratedAt, updatedAt) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(taskId, runId, foreachNodeId, stepIndex) DO UPDATE SET + pinnedStepCount = excluded.pinnedStepCount, + currentNodeId = excluded.currentNodeId, + status = excluded.status, + baselineSha = excluded.baselineSha, + checkpointId = excluded.checkpointId, + reworkCount = excluded.reworkCount, + branchName = excluded.branchName, + integratedAt = excluded.integratedAt, + updatedAt = excluded.updatedAt`, + ) + .run( + state.taskId, + state.runId, + state.foreachNodeId, + state.stepIndex, + state.pinnedStepCount, + state.currentNodeId ?? null, + state.status, + state.baselineSha ?? null, + state.checkpointId ?? null, + state.reworkCount ?? 0, + state.branchName ?? null, + state.integratedAt ?? null, + new Date().toISOString(), + ); + } catch { + // Legacy/missing table — persistence is additive, so degrade silently. + } + } + + /** + * Load persisted step-instance run-state for a run (crash-resume; KTD-6). + * Ordered by stepIndex so the executor can reconstruct the instance set in + * step order. Additive: returns [] on a legacy/missing table. + */ + loadWorkflowRunStepInstances( + taskId: string, + runId: string, + ): import("./types.js").WorkflowRunStepInstance[] { + try { + const rows = this.db + .prepare( + `SELECT taskId, runId, foreachNodeId, stepIndex, pinnedStepCount, currentNodeId, status, baselineSha, checkpointId, reworkCount, branchName, integratedAt, updatedAt + FROM workflow_run_step_instances + WHERE taskId = ? AND runId = ? + ORDER BY stepIndex ASC`, + ) + .all(taskId, runId) as import("./types.js").WorkflowRunStepInstance[]; + return rows; + } catch { + return []; + } + } + + /** + * Prune step-instance rows for a task (KTD-6, #1412 pattern). When `runId` is + * provided, deletes every row for `taskId` whose runId differs (bounding growth + * across a long-lived task's repeated runs — call on run start/completion). + * When `runId` is omitted, deletes all rows for the task (e.g. on archive). + * Additive: silently no-ops on a legacy/missing table. + */ + clearWorkflowRunStepInstances(taskId: string, keepRunId?: string): void { + try { + if (keepRunId === undefined) { + this.db + .prepare(`DELETE FROM workflow_run_step_instances WHERE taskId = ?`) + .run(taskId); + } else { + this.db + .prepare( + `DELETE FROM workflow_run_step_instances WHERE taskId = ? AND runId != ?`, + ) + .run(taskId, keepRunId); + } + } catch { + // Legacy/missing table — pruning is additive, so degrade silently. + } + } + async listTasksForGithubTrackingReconcile(options?: { offset?: number; limit?: number }): Promise<{ tasks: Task[]; hasMore: boolean }> { const reconcileScanLimit = 200; const offset = Math.max(0, options?.offset ?? 0); @@ -6865,7 +6968,7 @@ export class TaskStore extends EventEmitter { async updateTask( id: string, - updates: { title?: string; description?: string; priority?: TaskPriority | null; prompt?: string; worktree?: string | null; status?: string | null; dependencies?: string[]; steps?: import("./types.js").TaskStep[]; currentStep?: number; blockedBy?: string | null; overlapBlockedBy?: string | null; assignedAgentId?: string | null; pausedByAgentId?: string | null; pausedReason?: string | null; tokenBudgetSoftAlertedAt?: string | null; worktrunkFallbackAlertedAt?: string | null; worktrunkFailure?: import("./types.js").Task["worktrunkFailure"] | null; tokenBudgetHardAlertedAt?: string | null; tokenBudgetOverride?: import("./types.js").TaskTokenBudgetOverride | null; dispatchStormCount?: number | null; lastDispatchAt?: string | null; assigneeUserId?: string | null; scopeOverride?: boolean | null; scopeOverrideReason?: string | null; scopeAutoWiden?: string[] | null; nodeId?: string | null; effectiveNodeId?: string | null; effectiveNodeSource?: string | null; checkedOutBy?: string | null; checkedOutAt?: string | null; checkoutNodeId?: string | null; checkoutRunId?: string | null; checkoutLeaseRenewedAt?: string | null; checkoutLeaseEpoch?: number | null; paused?: boolean; baseBranch?: string | null; autoMerge?: boolean | null; branch?: string | null; executionStartBranch?: string | null; baseCommitSha?: string | null; size?: "S" | "M" | "L"; reviewLevel?: number; executionMode?: import("./types.js").ExecutionMode | null; mergeRetries?: number; workflowStepRetries?: number; stuckKillCount?: number | null; resumeLimboCount?: number | null; resumeLimboTipSha?: string | null; resumeLimboStepSignature?: string | null; postReviewFixCount?: number | null; recoveryRetryCount?: number | null; taskDoneRetryCount?: number | null; worktreeSessionRetryCount?: number | null; completionHandoffLimboRecoveryCount?: number | null; verificationFailureCount?: number | null; mergeConflictBounceCount?: number | null; mergeAuditBounceCount?: number | null; mergeTransientRetryCount?: number | null; branchConflictRecoveryCount?: number | null; reviewerContextRetryCount?: number | null; reviewerFallbackRetryCount?: number | null; nextRecoveryAt?: string | null; enabledWorkflowSteps?: string[]; noCommitsExpected?: boolean | null; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; planningModelProvider?: string | null; planningModelId?: string | null; thinkingLevel?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; firstExecutionAt?: string | null; cumulativeActiveMs?: number | null; executionStartedAt?: string | null; executionCompletedAt?: string | null; review?: import("./types.js").TaskReview | null; reviewState?: import("./types.js").TaskReviewState | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; mergeDetails?: import("./types.js").MergeDetails | null; sourceIssue?: import("./types.js").TaskSourceIssue | null; sourceMetadataPatch?: Record | null; githubTracking?: import("./types.js").TaskGithubTracking | null; tokenUsage?: import("./types.js").TaskTokenUsage | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null }, + updates: { title?: string; description?: string; priority?: TaskPriority | null; prompt?: string; worktree?: string | null; status?: string | null; dependencies?: string[]; steps?: import("./types.js").TaskStep[]; customFields?: Record; currentStep?: number; blockedBy?: string | null; overlapBlockedBy?: string | null; assignedAgentId?: string | null; pausedByAgentId?: string | null; pausedReason?: string | null; tokenBudgetSoftAlertedAt?: string | null; worktrunkFallbackAlertedAt?: string | null; worktrunkFailure?: import("./types.js").Task["worktrunkFailure"] | null; tokenBudgetHardAlertedAt?: string | null; tokenBudgetOverride?: import("./types.js").TaskTokenBudgetOverride | null; dispatchStormCount?: number | null; lastDispatchAt?: string | null; assigneeUserId?: string | null; scopeOverride?: boolean | null; scopeOverrideReason?: string | null; scopeAutoWiden?: string[] | null; nodeId?: string | null; effectiveNodeId?: string | null; effectiveNodeSource?: string | null; checkedOutBy?: string | null; checkedOutAt?: string | null; checkoutNodeId?: string | null; checkoutRunId?: string | null; checkoutLeaseRenewedAt?: string | null; checkoutLeaseEpoch?: number | null; paused?: boolean; baseBranch?: string | null; autoMerge?: boolean | null; branch?: string | null; executionStartBranch?: string | null; baseCommitSha?: string | null; size?: "S" | "M" | "L"; reviewLevel?: number; executionMode?: import("./types.js").ExecutionMode | null; mergeRetries?: number; workflowStepRetries?: number; stuckKillCount?: number | null; resumeLimboCount?: number | null; resumeLimboTipSha?: string | null; resumeLimboStepSignature?: string | null; postReviewFixCount?: number | null; recoveryRetryCount?: number | null; taskDoneRetryCount?: number | null; worktreeSessionRetryCount?: number | null; completionHandoffLimboRecoveryCount?: number | null; verificationFailureCount?: number | null; mergeConflictBounceCount?: number | null; mergeAuditBounceCount?: number | null; mergeTransientRetryCount?: number | null; branchConflictRecoveryCount?: number | null; reviewerContextRetryCount?: number | null; reviewerFallbackRetryCount?: number | null; nextRecoveryAt?: string | null; enabledWorkflowSteps?: string[]; noCommitsExpected?: boolean | null; modelProvider?: string | null; modelId?: string | null; validatorModelProvider?: string | null; validatorModelId?: string | null; planningModelProvider?: string | null; planningModelId?: string | null; thinkingLevel?: string | null; error?: string | null; summary?: string | null; sessionFile?: string | null; firstExecutionAt?: string | null; cumulativeActiveMs?: number | null; executionStartedAt?: string | null; executionCompletedAt?: string | null; review?: import("./types.js").TaskReview | null; reviewState?: import("./types.js").TaskReviewState | null; workflowStepResults?: import("./types.js").WorkflowStepResult[] | null; mergeDetails?: import("./types.js").MergeDetails | null; sourceIssue?: import("./types.js").TaskSourceIssue | null; sourceMetadataPatch?: Record | null; githubTracking?: import("./types.js").TaskGithubTracking | null; tokenUsage?: import("./types.js").TaskTokenUsage | null; modifiedFiles?: string[] | null; missionId?: string | null; sliceId?: string | null }, runContext?: RunMutationContext, ): Promise { return this.withTaskLock(id, () => this.updateTaskUnlocked(id, updates, runContext)); @@ -6979,6 +7082,10 @@ export class TaskStore extends EventEmitter { } } if (updates.steps !== undefined) task.steps = updates.steps; + // U4/KTD-13 groundwork: round-trip customFields as an opaque whole-object + // patch. The typed validation/write authority (updateTaskCustomFields) + // lands in a later unit; for now updateTask just persists what it is given. + if (updates.customFields !== undefined) task.customFields = updates.customFields; if (updates.currentStep !== undefined) task.currentStep = updates.currentStep; if (updates.status === null) { task.status = undefined; diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index fbf8f693da..a856b620fb 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -681,6 +681,59 @@ export interface WorkflowStepResult { completedAt?: string; } +/** + * Lifecycle status of one persisted step instance (step-inversion U4, KTD-6). + * - `pending` — expanded but not yet started. + * - `in-progress` — actively executing inside its foreach sub-walk. + * - `awaiting-integration` — work complete on a parallel-mode branch, waiting + * for the ordered integration stage (KTD-11; unused at concurrency 1). + * - `completed` — terminal success (integrated in parallel mode). + * - `failed` — terminal failure. + */ +export type WorkflowRunStepInstanceStatus = + | "pending" + | "in-progress" + | "awaiting-integration" + | "completed" + | "failed"; + +/** + * Persisted run-state for one expanded step instance inside a foreach region + * (step-inversion U4, KTD-6). One row per `(taskId, runId, foreachNodeId, + * stepIndex)`; mirrors the `workflow_run_branches` posture. Resume reconstructs + * the instance set from `pinnedStepCount` + per-instance `currentNodeId` / + * `reworkCount`. `baselineSha` / `checkpointId` are the RETHINK reset anchors + * (previously in-memory, lost on restart). `branchName` / `integratedAt` and the + * `awaiting-integration` status serve parallel mode (KTD-11); null/unused at + * concurrency 1. This is the core row shape; the engine-side instance model is + * separate and engine-owned. + */ +export interface WorkflowRunStepInstance { + taskId: string; + runId: string; + /** Node id of the foreach region that expanded this instance. */ + foreachNodeId: string; + /** Zero-based index of the step this instance runs. */ + stepIndex: number; + /** Step count pinned at expansion; resume fails on mismatch with live steps[]. */ + pinnedStepCount: number; + /** Current sub-walk node id for the in-flight instance; null when not started. */ + currentNodeId?: string | null; + status: WorkflowRunStepInstanceStatus; + /** Git sha the RETHINK reset rewinds to; null when no baseline captured. */ + baselineSha?: string | null; + /** Session checkpoint to rewind to on RETHINK; null when none captured. */ + checkpointId?: string | null; + /** Number of rework cycles consumed against the rework budget. */ + reworkCount: number; + /** Per-instance branch name in worktree-isolation mode (KTD-11); null otherwise. */ + branchName?: string | null; + /** ISO-8601 timestamp the instance branch was integrated (KTD-11); null otherwise. */ + integratedAt?: string | null; + /** ISO-8601 timestamp of the last write to this row. */ + updatedAt: string; +} + /** A built-in workflow step template for one-click creation. */ export interface WorkflowStepTemplate { /** Unique template identifier (e.g., "documentation-review") */ @@ -1825,6 +1878,14 @@ export interface Task { worktree?: string; steps: TaskStep[]; currentStep: number; + /** + * Workflow-defined custom task field values (KTD-13), keyed by field id. + * Persisted as the `tasks.customFields` JSON column. Treated as opaque by + * the core row⇄Task mapping and `updateTask`; the validation/write authority + * (type/enum/render checks against the workflow's field schema) lands in a + * later unit. Absent on legacy tasks. + */ + customFields?: Record; status?: string; /** ID of the in-progress task whose file scope overlaps with this task, * causing the scheduler to defer it. Set when the scheduler queues @@ -4037,6 +4098,8 @@ export interface ArchivedTaskEntry { dependencies: string[]; steps: TaskStep[]; currentStep: number; + /** Workflow-defined custom task field values (KTD-13) frozen at archive time. */ + customFields?: Record; size?: "S" | "M" | "L"; reviewLevel?: number; /** Execution mode for task implementation at time of archival. From ad5cd579bea6be3d270cd4c912f3c663490feca2 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 4 Jun 2026 11:50:45 -0700 Subject: [PATCH 04/22] =?UTF-8?q?feat(dashboard):=20U8=20=E2=80=94=20node?= =?UTF-8?q?=20editor=20authoring=20for=20foreach/step-review/parse-steps/c?= =?UTF-8?q?ode,=20rework=20edge=20inspector,=20template=20round-trip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.8 (1M context) --- .../app/components/WorkflowNodeEditor.css | 67 +++ .../app/components/WorkflowNodeEditor.tsx | 455 +++++++++++++++++- .../__tests__/WorkflowNodeEditor.test.tsx | 220 ++++++++- .../__tests__/workflow-flow-mapping.test.ts | 135 ++++++ .../components/nodes/WorkflowNodeTypes.tsx | 70 ++- .../app/components/workflow-flow-mapping.ts | 228 +++++++-- packages/i18n/locales/en/app.json | 29 +- packages/i18n/locales/es/app.json | 29 +- packages/i18n/locales/fr/app.json | 29 +- packages/i18n/locales/ko/app.json | 29 +- packages/i18n/locales/zh-CN/app.json | 29 +- packages/i18n/locales/zh-TW/app.json | 29 +- 12 files changed, 1303 insertions(+), 46 deletions(-) diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.css b/packages/dashboard/app/components/WorkflowNodeEditor.css index b8f2fcdee1..b25d2f9c90 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.css +++ b/packages/dashboard/app/components/WorkflowNodeEditor.css @@ -307,6 +307,73 @@ border-style: dashed; } +/* ── Step-inversion nodes (KTD-3/4/12/15, U8) ── */ + +.wf-node-step-execute { + border-color: var(--accent, var(--ws-info)); +} + +.wf-node-step-review { + border-color: var(--ws-info); +} + +.wf-node-parse-steps { + border-color: var(--ws-info); +} + +.wf-node-code { + border-color: var(--text-muted); + font-family: var(--font-mono, monospace); +} + +/* A foreach renders as a React Flow group node containing its template + * subgraph. Children are positioned inside the group's box. */ +.wf-foreach-group { + width: 100%; + height: 100%; + box-sizing: border-box; + border: 1px dashed var(--accent, var(--ws-info)); + border-radius: var(--radius-md); + background: color-mix(in srgb, var(--accent, var(--ws-info)) 6%, transparent); + padding: var(--space-xs); +} + +.wf-foreach-group.wf-node--error { + border-color: var(--ws-error); +} + +.wf-foreach-header { + display: inline-flex; + align-items: center; + gap: var(--space-xs); + font-size: 0.8rem; + color: var(--text); +} + +.wf-foreach-empty { + margin-top: var(--space-sm); + padding: var(--space-sm); + border: 1px dashed var(--border); + border-radius: var(--radius-sm); + font-size: 0.7rem; + color: var(--text-muted); + text-align: center; +} + +/* Rework edges (KTD-5): dashed accent stroke with a loop affordance. */ +.wf-edge-rework .react-flow__edge-path { + stroke: var(--accent, var(--ws-info)); + stroke-dasharray: 5 4; + stroke-width: 2; +} + +.wf-code-source { + font-family: var(--font-mono, monospace); + font-size: 0.72rem; + white-space: pre; + overflow-x: auto; +} + .wf-node-icon { display: inline-flex; color: var(--text-muted); diff --git a/packages/dashboard/app/components/WorkflowNodeEditor.tsx b/packages/dashboard/app/components/WorkflowNodeEditor.tsx index 38b08b9f4a..6d3996dadd 100644 --- a/packages/dashboard/app/components/WorkflowNodeEditor.tsx +++ b/packages/dashboard/app/components/WorkflowNodeEditor.tsx @@ -15,7 +15,7 @@ import { type Edge as FlowEdge, } from "@xyflow/react"; import { useTranslation } from "react-i18next"; -import { X, Plus, Trash2, Save, MessageSquare, Terminal, Shield, GitMerge, Loader2, HelpCircle, PauseCircle, Split, Merge } from "lucide-react"; +import { X, Plus, Trash2, Save, MessageSquare, Terminal, Shield, GitMerge, Loader2, HelpCircle, PauseCircle, Split, Merge, Repeat, ClipboardCheck, ListChecks, Code2 } from "lucide-react"; import type { WorkflowDefinition, WorkflowIrColumn, TraitViolation } from "@fusion/core"; import { getErrorMessage } from "@fusion/core"; import { @@ -46,6 +46,12 @@ import { validateColumnsClient, unplacedNodeIds, isColumnBandNode, + foreachChildFlowId, + shortConditionLabel, + FOREACH_GROUP_WIDTH, + FOREACH_GROUP_HEIGHT, + FOREACH_CHILD_X, + FOREACH_CHILD_Y, } from "./workflow-flow-mapping"; import { fetchTraits, type TraitCatalogEntry } from "../api"; import { WorkflowColumnPanel } from "./WorkflowColumnPanel"; @@ -84,6 +90,14 @@ function newNodeId(): string { return `n-${Date.now().toString(36)}-${nodeSeq}`; } +/** Built-in step parsers (KTD-12). Hardcoded for now; TODO: source from the live + * parser registry once a catalog endpoint exists (incl. plugin parsers). */ +const BUILTIN_STEP_PARSERS = ["step-headings", "json-steps"] as const; + +/** Step-review verdict outcomes (KTD-4), authored as `outcome:` edge + * conditions and displayed as short labels. */ +const STEP_REVIEW_VERDICTS = ["approve", "revise", "rethink", "unavailable"] as const; + const PALETTE: Array<{ kind: WorkflowEditorNodeKind; label: string; icon: typeof MessageSquare; presetConfig?: Record }> = [ { kind: "prompt", label: "Prompt", icon: MessageSquare }, { kind: "prompt", label: "User input", icon: HelpCircle, presetConfig: { awaitInput: true } }, @@ -93,6 +107,11 @@ const PALETTE: Array<{ kind: WorkflowEditorNodeKind; label: string; icon: typeof { kind: "hold", label: "Hold", icon: PauseCircle, presetConfig: { release: "manual" } }, { kind: "split", label: "Split", icon: Split }, { kind: "join", label: "Join", icon: Merge, presetConfig: { mode: "all", onBranchFailure: "collect" } }, + // Step-inversion (KTD-3/4/12/15). + { kind: "foreach", label: "For-each step", icon: Repeat, presetConfig: { source: "task-steps" } }, + { kind: "step-review", label: "Step review", icon: ClipboardCheck, presetConfig: { type: "code" } }, + { kind: "parse-steps", label: "Parse steps", icon: ListChecks, presetConfig: { artifact: "PROMPT.md", parser: "step-headings" } }, + { kind: "code", label: "Code", icon: Code2, presetConfig: { source: "" } }, ]; function InnerEditor({ @@ -109,6 +128,7 @@ function InnerEditor({ const [nodes, setNodes, onNodesChange] = useNodesState>([]); const [edges, setEdges, onEdgesChange] = useEdgesState([]); const [selectedNodeId, setSelectedNodeId] = useState(null); + const [selectedEdgeId, setSelectedEdgeId] = useState(null); const { t } = useTranslation("app"); // v2 columns the editor is authoring for the active workflow. const [columns, setColumns] = useState([]); @@ -172,6 +192,7 @@ function InnerEditor({ setEdges(flow.edges); setColumns(columnsOf(activeWorkflow)); setSelectedNodeId(null); + setSelectedEdgeId(null); setValidationError(null); }, [activeWorkflow, setNodes, setEdges]); @@ -219,6 +240,41 @@ function InnerEditor({ const label = nodeLabel ?? (kind === "merge" ? "Merge boundary" : kind.charAt(0).toUpperCase() + kind.slice(1)); const baseConfig = kind === "gate" ? { gateMode: "gate" } : {}; const config = presetConfig ? { ...baseConfig, ...presetConfig } : baseConfig; + + if (kind === "foreach") { + // A foreach renders as a React Flow group node. It auto-populates ONE + // step-execute child (a prompt node with seam=step-execute) so the group + // is never confusingly empty (KTD-3 / U8). The group node must precede + // its child in the array for React Flow's parent extent to apply. + const childId = foreachChildFlowId(id, newNodeId()); + setNodes((ns) => [ + ...ns, + { + id, + type: "foreach", + position: { x: 200 + ns.length * 40, y: 240 + (ns.length % 3) * 70 }, + data: { kind: "foreach", label, config, templateEmpty: false }, + style: { width: FOREACH_GROUP_WIDTH, height: FOREACH_GROUP_HEIGHT }, + deletable: true, + }, + { + id: childId, + type: "prompt", + position: { x: FOREACH_CHILD_X, y: FOREACH_CHILD_Y }, + parentId: id, + extent: "parent", + data: { + kind: "prompt", + label: t("workflowNodes.stepExecuteLabel", "Step execute"), + config: { seam: "step-execute" }, + }, + deletable: true, + }, + ]); + setSelectedNodeId(id); + return; + } + setNodes((ns) => [ ...ns, { @@ -231,7 +287,7 @@ function InnerEditor({ ]); setSelectedNodeId(id); }, - [setNodes], + [setNodes, t], ); const updateSelectedData = useCallback( @@ -269,6 +325,30 @@ function InnerEditor({ [selectedNodeId, setNodes], ); + // Edge inspector (KTD-4/5): mutate the selected edge's condition + rework + // kind, keeping its display label in sync. Rework edges render dashed/animated. + const updateSelectedEdge = useCallback( + (patch: { condition?: string; rework?: boolean }) => { + if (!selectedEdgeId) return; + setEdges((eds) => + eds.map((e) => { + if (e.id !== selectedEdgeId) return e; + const condition = patch.condition ?? (e.data?.condition as string | undefined) ?? "success"; + const rework = patch.rework ?? (e.data?.kind as string | undefined) === "rework"; + return { + ...e, + label: rework ? `${shortConditionLabel(condition)} (rework)` : shortConditionLabel(condition), + data: { ...(e.data ?? {}), condition, kind: rework ? "rework" : undefined }, + type: rework ? "step" : undefined, + animated: rework, + className: rework ? "wf-edge-rework" : undefined, + }; + }), + ); + }, + [selectedEdgeId, setEdges], + ); + const handleCreateWorkflow = useCallback(async () => { const name = window.prompt("New workflow name"); if (!name?.trim()) return; @@ -383,16 +463,54 @@ function InnerEditor({ // (WorkflowNodeErrorBadge) renders both, keyed off data.errorBadge. const nodesForRender = useMemo(() => { const unplacedSet = new Set(unplaced); + // Count current template children per foreach group so the empty-state hint + // (KTD-3 / U8) reflects live deletions even though the palette seeds one. + const childCount = new Map(); + for (const n of nodes) { + if (n.parentId) childCount.set(n.parentId, (childCount.get(n.parentId) ?? 0) + 1); + } + const emptyHint = t("workflowNodes.foreachEmptyHint", "Drag a step-execute node here"); return nodes.map((n) => { let errorBadge: string | undefined; if (unplacedSet.has(n.id)) errorBadge = t("workflowColumns.nodeUnplaced", "Not placed in a column"); if (serverNodeError?.nodeId === n.id) errorBadge = serverNodeError.message; - if (errorBadge === n.data.errorBadge) return n; - return { ...n, data: { ...n.data, errorBadge } }; + const templateEmpty = n.data.kind === "foreach" ? (childCount.get(n.id) ?? 0) === 0 : undefined; + if ( + errorBadge === n.data.errorBadge && + (n.data.kind !== "foreach" || (templateEmpty === n.data.templateEmpty && n.data.emptyHint === emptyHint)) + ) + return n; + return { + ...n, + data: { + ...n.data, + errorBadge, + ...(n.data.kind === "foreach" ? { templateEmpty, emptyHint } : {}), + }, + }; }); }, [nodes, unplaced, serverNodeError, t]); const selectedNode = nodes.find((n) => n.id === selectedNodeId) ?? null; + const selectedEdge = edges.find((e) => e.id === selectedEdgeId) ?? null; + // The edge inspector's verdict/rework controls apply only when the edge's + // source node is a step-review node (KTD-4). + const selectedEdgeSourceIsReview = useMemo(() => { + if (!selectedEdge) return false; + const src = nodes.find((n) => n.id === selectedEdge.source); + return src?.data.kind === "step-review"; + }, [selectedEdge, nodes]); + + // Artifacts the active workflow declares (KTD-12). The parse-steps inspector + // offers a select over these; when none are declared it falls back to a + // free-text input defaulting to PROMPT.md. + const declaredArtifacts = useMemo(() => { + const ir = activeWorkflow?.ir; + if (ir && ir.version === "v2" && Array.isArray(ir.artifacts)) { + return ir.artifacts.map((a) => a.key); + } + return []; + }, [activeWorkflow]); // Lazy-loaded executor resources const [models, setModels] = useState([]); @@ -402,6 +520,13 @@ function InnerEditor({ const currentExecutor = (selectedNode?.data.config?.executor as ExecutorKind | undefined) ?? "model"; useEffect(() => { + // step-review offers an optional review model picker (KTD-4). + if (selectedNode?.data.kind === "step-review" && models.length === 0) { + fetchModels().then((res) => setModels(res.models)).catch((err) => { + addToast(getErrorMessage(err) || "Failed to load models", "error"); + }); + return; + } if (!selectedNode || (selectedNode.data.kind !== "prompt" && selectedNode.data.kind !== "gate")) return; if (currentExecutor === "model" && models.length === 0) { fetchModels().then((res) => setModels(res.models)).catch((err) => { @@ -527,8 +652,18 @@ function InnerEditor({ onEdgesChange={onEdgesChange} onConnect={onConnect} onNodeDragStop={onNodeDragStop} - onNodeClick={(_, node) => setSelectedNodeId(node.id)} - onPaneClick={() => setSelectedNodeId(null)} + onNodeClick={(_, node) => { + setSelectedNodeId(node.id); + setSelectedEdgeId(null); + }} + onEdgeClick={(_, edge) => { + setSelectedEdgeId(edge.id); + setSelectedNodeId(null); + }} + onPaneClick={() => { + setSelectedNodeId(null); + setSelectedEdgeId(null); + }} fitView > @@ -837,6 +972,258 @@ function InnerEditor({

) : null} + {selectedNode.data.kind === "foreach" ? ( + (() => { + const mode = String(selectedNode.data.config?.mode ?? "sequential"); + const isParallel = mode === "parallel"; + return ( + <> + + + + + {isParallel && ( + + )} + + +

+ {t( + "workflowNodes.foreachNote", + "Expands once per planned step. Drop a step-execute node (and optional step-review) into the region.", + )} +

+ + ); + })() + ) : null} + + {selectedNode.data.kind === "step-review" ? ( + <> + + +

+ {t( + "workflowNodes.reviewNote", + "Verdicts route as outcome edges. Click an outgoing edge to set its verdict and rework behavior.", + )} +

+ + ) : null} + + {selectedNode.data.kind === "parse-steps" ? ( + <> + {declaredArtifacts.length > 0 ? ( + + ) : ( + + )} + + + ) : null} + + {selectedNode.data.kind === "code" ? ( + <> +