diff --git a/packages/core/src/__tests__/default-workflow-hooks.test.ts b/packages/core/src/__tests__/default-workflow-hooks.test.ts new file mode 100644 index 0000000000..c650a89a78 --- /dev/null +++ b/packages/core/src/__tests__/default-workflow-hooks.test.ts @@ -0,0 +1,87 @@ +// @vitest-environment node +// +// U4: the default-workflow side effects are resolved THROUGH the trait registry +// (the DI seam, KTD-2/U2). This pins: +// - registerDefaultWorkflowHooks() wires the impls so resolution finds them +// (no missing-hook-impl warning on the happy path); +// - a missing registration degrades to a no-op + audit warning (not a crash); +// - applyDefaultWorkflowMoveEffects mutates the task per the legacy contract. + +import { describe, it, expect, beforeEach } from "vitest"; +import { + __resetTraitRegistryForTests, + getTraitRegistry, +} from "../trait-registry.js"; +import { registerBuiltinTraits } from "../builtin-traits.js"; +import { + __resetDefaultWorkflowHooksForTests, + applyDefaultWorkflowMoveEffects, + registerDefaultWorkflowHooks, + type DefaultWorkflowMoveContext, +} from "../default-workflow-hooks.js"; +import type { Task } from "../types.js"; + +function makeCtx(overrides: Partial = {}): DefaultWorkflowMoveContext { + const task = { + id: "FN-1", + column: "in-progress", + columnMovedAt: new Date().toISOString(), + steps: [], + dependencies: [], + } as unknown as Task; + return { + task, + fromColumn: "todo", + toColumn: "in-progress", + moveSource: "user", + bypassGuards: false, + movedAt: new Date().toISOString(), + settings: undefined, + options: {}, + resetSteps: () => {}, + ...overrides, + }; +} + +describe("default-workflow-hooks registry wiring", () => { + beforeEach(() => { + __resetTraitRegistryForTests(); + __resetDefaultWorkflowHooksForTests(); + registerBuiltinTraits(); + }); + + it("resolves all default-workflow hooks without a missing-impl warning once registered", () => { + registerDefaultWorkflowHooks(); + const ctx = makeCtx({ fromColumn: "todo", toColumn: "in-progress" }); + const { warnings } = applyDefaultWorkflowMoveEffects(ctx); + expect(warnings).toHaveLength(0); + // timing.onEnter stamped cumulativeActiveMs on entry to in-progress. + expect(ctx.task.cumulativeActiveMs).toBe(0); + }); + + it("degrades to a no-op + audit warning when a hook impl is not registered", () => { + // Built-in DEFINITIONS are registered (so the trait declares the hook) but + // we deliberately do NOT call registerDefaultWorkflowHooks() — no impls. + const registry = getTraitRegistry(); + // sanity: the trait declares the hook descriptor + expect(registry.getTrait("timing")?.hooks?.onEnter).toBe(true); + const ctx = makeCtx({ fromColumn: "todo", toColumn: "in-progress" }); + const { warnings } = applyDefaultWorkflowMoveEffects(ctx); + // Every declared hook with no impl yields a degraded-no-op warning. + expect(warnings.length).toBeGreaterThan(0); + expect(warnings.every((w) => w.kind === "missing-hook-impl")).toBe(true); + // No crash; task unmutated by the (no-op) hooks. + expect(ctx.task.cumulativeActiveMs).toBeUndefined(); + }); + + it("applies userPaused only for user-source reopen to todo", () => { + registerDefaultWorkflowHooks(); + const userCtx = makeCtx({ fromColumn: "in-progress", toColumn: "todo", moveSource: "user" }); + applyDefaultWorkflowMoveEffects(userCtx); + expect(userCtx.task.userPaused).toBe(true); + + const engineCtx = makeCtx({ fromColumn: "in-progress", toColumn: "todo", moveSource: "engine" }); + applyDefaultWorkflowMoveEffects(engineCtx); + expect(engineCtx.task.userPaused).toBeUndefined(); + }); +}); diff --git a/packages/core/src/__tests__/move-task-characterization.test.ts b/packages/core/src/__tests__/move-task-characterization.test.ts new file mode 100644 index 0000000000..66c4f4901a --- /dev/null +++ b/packages/core/src/__tests__/move-task-characterization.test.ts @@ -0,0 +1,205 @@ +// @vitest-environment node +// +// CHARACTERIZATION SUITE (U4 Execution Note — written FIRST, before any change +// to `moveTaskInternal`). +// +// This suite pins the CURRENT behavior of `moveTaskInternal` for every (from, +// to) pair in VALID_TRANSITIONS' domain and both moveSource values, plus the +// key column side effects: +// - merge-blocker on in-review → done (user source) +// - userPaused set only for user-source in-progress → todo +// - reopen field/step resets on in-review/done → todo|triage +// - autoMerge stamping on → in-review +// - timing fields (cumulativeActiveMs / executionStartedAt) on in-progress +// +// It runs GREEN against the unmodified store first, then runs forever against +// BOTH flag states (workflowColumns OFF and ON) — see the `flagStates` loop. +// Any divergence between the two flag states is a U4 parity FAILURE. + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { VALID_TRANSITIONS } from "../types.js"; +import type { Column, Task } from "../types.js"; +import { createTaskStoreTestHarness } from "./store-test-helpers.js"; + +const ALL_COLUMNS: Column[] = ["triage", "todo", "in-progress", "in-review", "done", "archived"]; + +// Flag states the characterization runs against. OFF is the legacy path; ON is +// the workflow-resolved path. The default workflow MUST reproduce identical +// outcomes for both, so the same expectations apply. +const flagStates: Array<{ label: string; workflowColumns: boolean }> = [ + { label: "flag OFF (legacy path)", workflowColumns: false }, + { label: "flag ON (workflow-resolved default workflow)", workflowColumns: true }, +]; + +for (const flag of flagStates) { + describe(`moveTaskInternal characterization — ${flag.label}`, () => { + const harness = createTaskStoreTestHarness(); + let store: ReturnType; + + beforeEach(async () => { + await harness.beforeEach(); + store = harness.store(); + if (flag.workflowColumns) { + await store.updateGlobalSettings({ experimentalFeatures: { workflowColumns: true } }); + } + }); + + afterEach(async () => { + await harness.afterEach(); + }); + + /** + * Drive a freshly-created task (starts in `triage`) into `column` using only + * legal, side-effect-tolerant moves. Returns the task. + */ + async function seedInColumn(column: Column): Promise { + const task = await store.createTask({ description: `seed-${column}` }); + switch (column) { + case "triage": + return task; + case "todo": + return store.moveTask(task.id, "todo", { moveSource: "user" }); + case "in-progress": + await store.moveTask(task.id, "todo", { moveSource: "user" }); + return store.moveTask(task.id, "in-progress", { moveSource: "user" }); + case "in-review": + await store.moveTask(task.id, "todo", { moveSource: "user" }); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + return store.moveTask(task.id, "in-review", { moveSource: "user", allowDirectInReviewMove: true }); + case "done": + await store.moveTask(task.id, "todo", { moveSource: "user" }); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + await store.moveTask(task.id, "in-review", { moveSource: "user", allowDirectInReviewMove: true }); + return store.moveTask(task.id, "done", { moveSource: "engine", skipMergeBlocker: true }); + case "archived": + await store.moveTask(task.id, "todo", { moveSource: "user" }); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + await store.moveTask(task.id, "in-review", { moveSource: "user", allowDirectInReviewMove: true }); + await store.moveTask(task.id, "done", { moveSource: "engine", skipMergeBlocker: true }); + return store.moveTask(task.id, "archived", { moveSource: "user" }); + default: + throw new Error(`unhandled column ${column}`); + } + } + + describe("transition allow/reject matrix (every from×to×moveSource)", () => { + for (const from of ALL_COLUMNS) { + for (const to of ALL_COLUMNS) { + for (const moveSource of ["user", "engine"] as const) { + const allowed = from === to || VALID_TRANSITIONS[from].includes(to); + const label = `${from} → ${to} [${moveSource}] should ${allowed ? "ALLOW" : "REJECT"}`; + it(label, async () => { + const task = await seedInColumn(from); + // Same-column move is a no-op success in legacy behavior. + if (from === to) { + const result = await store.moveTask(task.id, to, { moveSource }); + expect(result.column).toBe(to); + return; + } + if (allowed) { + // in-review → done with merge-blocker only blocks for user source + // and only when a blocker exists; our seeded task has no blocker. + // Bare in-review targets bypass the handoff invariant via + // allowDirectInReviewMove, matching production drag behavior. + const opts = + to === "in-review" + ? { moveSource, allowDirectInReviewMove: true } + : { moveSource }; + const result = await store.moveTask(task.id, to, opts); + expect(result.column).toBe(to); + } else { + await expect( + store.moveTask(task.id, to, { moveSource }), + ).rejects.toThrow(/Invalid transition/); + } + }); + } + } + } + }); + + describe("merge-blocker side effect (in-review → done)", () => { + it("blocks a user move to done when a merge blocker exists", async () => { + const task = await seedInColumn("in-review"); + // Incomplete steps create a merge blocker (getTaskMergeBlocker). + await store.updateTask(task.id, { + steps: [{ name: "x", status: "pending" }] as Task["steps"], + }); + await expect( + store.moveTask(task.id, "done", { moveSource: "user" }), + ).rejects.toThrow(/Cannot move .* to done/); + }); + + it("skipMergeBlocker bypasses the blocker", async () => { + const task = await seedInColumn("in-review"); + await store.updateTask(task.id, { + steps: [{ name: "x", status: "pending" }] as Task["steps"], + }); + const result = await store.moveTask(task.id, "done", { + moveSource: "engine", + skipMergeBlocker: true, + }); + expect(result.column).toBe("done"); + }); + }); + + describe("userPaused side effect (in-progress → todo)", () => { + it("sets userPaused for a user-source move", async () => { + const task = await seedInColumn("in-progress"); + const result = await store.moveTask(task.id, "todo", { moveSource: "user" }); + expect(result.userPaused).toBe(true); + }); + + it("does NOT set userPaused for an engine-source move", async () => { + const task = await seedInColumn("in-progress"); + const result = await store.moveTask(task.id, "todo", { moveSource: "engine" }); + expect(result.userPaused).toBeUndefined(); + }); + }); + + describe("reopen resets (in-review → todo)", () => { + it("clears branch/summary/baseCommitSha on reopen to todo", async () => { + const task = await seedInColumn("in-review"); + await store.updateTask(task.id, { + branch: "fusion/fn-x", + summary: "did stuff", + baseCommitSha: "abc123", + }); + const result = await store.moveTask(task.id, "todo", { moveSource: "user" }); + expect(result.branch).toBeUndefined(); + expect(result.summary).toBeUndefined(); + expect(result.baseCommitSha).toBeUndefined(); + }); + }); + + describe("autoMerge stamping (→ in-review)", () => { + it("stamps autoMerge from settings when undefined", async () => { + await store.updateSettings({ autoMerge: true }); + const task = await seedInColumn("in-progress"); + const result = await store.moveTask(task.id, "in-review", { + moveSource: "user", + allowDirectInReviewMove: true, + }); + expect(result.autoMerge).toBe(true); + }); + }); + + describe("timing fields (→ in-progress)", () => { + it("sets executionStartedAt and initializes cumulativeActiveMs on entry", async () => { + const task = await seedInColumn("todo"); + const result = await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + expect(result.executionStartedAt).toBeTruthy(); + expect(result.cumulativeActiveMs).toBe(0); + }); + + it("accumulates cumulativeActiveMs on exit from in-progress", async () => { + const task = await seedInColumn("in-progress"); + const result = await store.moveTask(task.id, "in-review", { + moveSource: "user", + allowDirectInReviewMove: true, + }); + expect(result.cumulativeActiveMs).toBeGreaterThanOrEqual(0); + }); + }); + }); +} diff --git a/packages/core/src/__tests__/transition-parity.test.ts b/packages/core/src/__tests__/transition-parity.test.ts new file mode 100644 index 0000000000..3587fe59fa --- /dev/null +++ b/packages/core/src/__tests__/transition-parity.test.ts @@ -0,0 +1,253 @@ +// @vitest-environment node +// +// TRANSITION-PARITY SUITE (U4). +// +// Proves the flag-ON workflow-resolved transition path reproduces the legacy +// VALID_TRANSITIONS contract for the default workflow, and exercises the U4 +// plan scenarios: +// - VALID_TRANSITIONS parity (allowed AND rejected sets identical) +// - FN-5147 terminal-until-merged (both paths) +// - hard-cancel user vs engine (userPaused + abort-on-exit bypass) +// - handoff bypass + exactly-once enqueue across a simulated crash +// - crash-mid-transition marker recovery (SQLite authoritative) +// - unknown-column rejection +// - guard rejection typed (flag-ON) vs legacy string (flag-OFF) +// - bypassGuards capacity pass-through (documenting; U6 fills enforcement) + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { VALID_TRANSITIONS } from "../types.js"; +import type { Column, Task } from "../types.js"; +import { TransitionRejectionError } from "../store.js"; +import { resolveAllowedColumns, workflowHasColumn } from "../workflow-transitions.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../builtin-coding-workflow-ir.js"; +import { readTransitionPending } from "../transition-pending.js"; +import { createTaskStoreTestHarness } from "./store-test-helpers.js"; + +const ALL_COLUMNS: Column[] = ["triage", "todo", "in-progress", "in-review", "done", "archived"]; + +describe("transition-parity — default workflow column adjacency == VALID_TRANSITIONS", () => { + it("reproduces VALID_TRANSITIONS exactly for every column (allowed + rejected)", () => { + for (const from of ALL_COLUMNS) { + const legacy = new Set(VALID_TRANSITIONS[from]); + const resolved = new Set(resolveAllowedColumns(BUILTIN_CODING_WORKFLOW_IR, from)); + // Allowed sets identical. + expect([...resolved].sort()).toEqual([...legacy].sort()); + // Rejected sets identical (complement over all columns). + for (const to of ALL_COLUMNS) { + if (from === to) continue; + expect(resolved.has(to)).toBe(legacy.has(to)); + } + } + }); + + it("recognizes exactly the six default columns", () => { + for (const c of ALL_COLUMNS) { + expect(workflowHasColumn(BUILTIN_CODING_WORKFLOW_IR, c)).toBe(true); + } + expect(workflowHasColumn(BUILTIN_CODING_WORKFLOW_IR, "made-up")).toBe(false); + }); +}); + +describe("transition-parity — store flag-ON scenarios", () => { + const harness = createTaskStoreTestHarness(); + let store: ReturnType; + + beforeEach(async () => { + await harness.beforeEach(); + store = harness.store(); + await store.updateGlobalSettings({ experimentalFeatures: { workflowColumns: true } }); + }); + afterEach(async () => { + await harness.afterEach(); + }); + + async function seedInColumn(column: Column): Promise { + const task = await store.createTask({ description: `seed-${column}` }); + const u = { moveSource: "user" as const }; + if (column === "triage") return task; + await store.moveTask(task.id, "todo", u); + if (column === "todo") return store.getTask(task.id) as Promise; + await store.moveTask(task.id, "in-progress", u); + if (column === "in-progress") return store.getTask(task.id) as Promise; + await store.moveTask(task.id, "in-review", { ...u, allowDirectInReviewMove: true }); + if (column === "in-review") return store.getTask(task.id) as Promise; + await store.moveTask(task.id, "done", { moveSource: "engine", skipMergeBlocker: true }); + if (column === "done") return store.getTask(task.id) as Promise; + await store.moveTask(task.id, "archived", u); + return store.getTask(task.id) as Promise; + } + + it("FN-5147: user move in-review → done blocked by merge-blocker with typed rejection", async () => { + const task = await seedInColumn("in-review"); + await store.updateTask(task.id, { steps: [{ name: "x", status: "pending" }] as Task["steps"] }); + let caught: unknown; + try { + await store.moveTask(task.id, "done", { moveSource: "user" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(TransitionRejectionError); + expect((caught as TransitionRejectionError).rejection.code).toBe("merge-blocked"); + expect((caught as TransitionRejectionError).rejection.retryable).toBe(true); + }); + + it("FN-5147: engine-sourced move bypasses the merge-blocker guard", async () => { + const task = await seedInColumn("in-review"); + await store.updateTask(task.id, { steps: [{ name: "x", status: "pending" }] as Task["steps"] }); + const moved = await store.moveTask(task.id, "done", { moveSource: "engine" }); + expect(moved.column).toBe("done"); + }); + + it("hard-cancel: user in-progress → todo sets userPaused; engine does not", async () => { + const userTask = await seedInColumn("in-progress"); + const u = await store.moveTask(userTask.id, "todo", { moveSource: "user" }); + expect(u.userPaused).toBe(true); + + const engineTask = await seedInColumn("in-progress"); + const e = await store.moveTask(engineTask.id, "todo", { moveSource: "engine" }); + expect(e.userPaused).toBeUndefined(); + }); + + it("unknown column rejects with typed unknown-column code, card untouched", async () => { + const task = await seedInColumn("todo"); + let caught: unknown; + try { + await store.moveTask(task.id, "made-up" as Column, { moveSource: "user" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(TransitionRejectionError); + expect((caught as TransitionRejectionError).rejection.code).toBe("unknown-column"); + const after = await store.getTask(task.id); + expect(after?.column).toBe("todo"); + }); + + it("guard/adjacency rejection is typed (not a bare Error string)", async () => { + const task = await seedInColumn("archived"); + // archived → todo is not a legal default-workflow transition. + let caught: unknown; + try { + await store.moveTask(task.id, "todo", { moveSource: "user" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(TransitionRejectionError); + expect((caught as TransitionRejectionError).rejection.code).toBe("guard-rejected"); + }); + + it("handoffToReview maps skipMergeBlocker onto bypassGuards and enqueues exactly once", async () => { + const task = await seedInColumn("in-progress"); + await store.handoffToReview(task.id, { + ownerAgentId: "agent-1", + evidence: { runId: "run-1", agentId: "agent-1", reason: "complete" }, + } as Parameters[1]); + const after = await store.getTask(task.id); + expect(after?.column).toBe("in-review"); + // Idempotent re-handoff (same-column path) must not double-enqueue. + await store.handoffToReview(task.id, { + ownerAgentId: "agent-1", + evidence: { runId: "run-2", agentId: "agent-1", reason: "complete" }, + } as Parameters[1]); + const queueCount = (store as unknown as { db: { prepare: (s: string) => { get: (...a: unknown[]) => unknown } } }).db + .prepare("SELECT COUNT(*) AS n FROM mergeQueue WHERE taskId = ?") + .get(task.id) as { n: number }; + expect(queueCount.n).toBe(1); + }); + + it("transitionPending marker is written in-txn and cleared post-commit (happy path)", async () => { + const task = await seedInColumn("todo"); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + const db = (store as unknown as { db: Parameters[0] }).db; + // Happy path: marker cleared after the post-commit hook runner. + expect(readTransitionPending(db, task.id)).toBeNull(); + }); + + it("crash-mid-transition: a persisted marker is recoverable from SQLite with hooksRemaining intact", async () => { + const task = await seedInColumn("todo"); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + // Simulate a crash AFTER commit but BEFORE the marker clear by re-writing a + // marker directly (the in-txn write path is the same helper). Recovery reads + // it back from SQLite (authoritative), not from task.json. + const db = (store as unknown as { db: Parameters[0] }).db; + (db as unknown as { prepare: (s: string) => { run: (...a: unknown[]) => unknown } }) + .prepare("UPDATE tasks SET transitionPending = ? WHERE id = ?") + .run( + JSON.stringify({ toColumn: "in-progress", hooksRemaining: ["default-workflow:postCommit"], startedAt: Date.now() }), + task.id, + ); + const pending = readTransitionPending(db, task.id); + expect(pending).not.toBeNull(); + expect(pending?.toColumn).toBe("in-progress"); + expect(pending?.hooksRemaining).toContain("default-workflow:postCommit"); + }); + + it("worktree ordering: allocateWorktree runs (and is applied) for a flag-ON move into in-progress", async () => { + const task = await seedInColumn("todo"); + let allocatorCalled = false; + const moved = await store.moveTask(task.id, "in-progress", { + moveSource: "user", + allocateWorktree: () => { + allocatorCalled = true; + return "/tmp/wt/seed-todo"; + }, + }); + expect(allocatorCalled).toBe(true); + expect(moved.worktree).toBe("/tmp/wt/seed-todo"); + // Worktree allocation is NOT a hook — it is a substrate capability invoked + // synchronously before the move commits; the committed row carries it. + const after = await store.getTask(task.id); + expect(after?.worktree).toBe("/tmp/wt/seed-todo"); + }); + + it("bypassGuards capacity pass-through (U4 documenting test): engine move into in-progress is NOT blocked by capacity (U6 fills enforcement)", async () => { + // U4 intentionally leaves the per-(workflow,column) capacity check as a + // pass-through slot; capacity enforcement lands in U6. This test pins the + // U4 contract: no WIP-constrained scenario is enforced yet, and an engine + // move (bypassGuards) into a wip-flagged column commits. It must be UPDATED + // by U6 (capacity is NEVER bypassable, KTD-10) — not silently left green. + const t1 = await seedInColumn("todo"); + const t2 = await seedInColumn("todo"); + const m1 = await store.moveTask(t1.id, "in-progress", { moveSource: "engine" }); + const m2 = await store.moveTask(t2.id, "in-progress", { moveSource: "engine" }); + expect(m1.column).toBe("in-progress"); + expect(m2.column).toBe("in-progress"); + }); +}); + +describe("transition-parity — flag-OFF keeps legacy thrown strings (no behavior change)", () => { + const harness = createTaskStoreTestHarness(); + let store: ReturnType; + beforeEach(async () => { + await harness.beforeEach(); + store = harness.store(); + }); + afterEach(async () => { + await harness.afterEach(); + }); + + it("rejects an illegal move with a bare Error containing the legacy message (not TransitionRejectionError)", async () => { + const task = await store.createTask({ description: "legacy reject" }); + await store.moveTask(task.id, "todo", { moveSource: "user" }); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + await store.moveTask(task.id, "in-review", { moveSource: "user", allowDirectInReviewMove: true }); + await store.moveTask(task.id, "done", { moveSource: "engine", skipMergeBlocker: true }); + await store.moveTask(task.id, "archived", { moveSource: "user" }); + let caught: unknown; + try { + await store.moveTask(task.id, "todo", { moveSource: "user" }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(Error); + expect(caught).not.toBeInstanceOf(TransitionRejectionError); + expect((caught as Error).message).toMatch(/Invalid transition/); + }); + + it("flag-OFF does NOT write a transitionPending marker", async () => { + const task = await store.createTask({ description: "no marker" }); + await store.moveTask(task.id, "todo", { moveSource: "user" }); + await store.moveTask(task.id, "in-progress", { moveSource: "user" }); + const db = (store as unknown as { db: Parameters[0] }).db; + expect(readTransitionPending(db, task.id)).toBeNull(); + }); +}); diff --git a/packages/core/src/default-workflow-hooks.ts b/packages/core/src/default-workflow-hooks.ts new file mode 100644 index 0000000000..b257c98799 --- /dev/null +++ b/packages/core/src/default-workflow-hooks.ts @@ -0,0 +1,313 @@ +/** + * Default-workflow trait hook implementations (U4). + * + * The legacy per-column side effects of `moveTaskInternal` — timing / + * `cumulativeActiveMs` accounting, reopen field/step resets, autoMerge stamping + * + merge-queue enqueue, and abort-on-exit (hard-cancel incl. `userPaused` only + * for user-source moves) — become the default workflow's trait hook + * implementations, registered through U2's DI seam (`registerTraitHookImpl`). + * + * IMPORTANT (per U4): this is the FLAG-ON path. The legacy inline code in + * `store.ts` is NOT deleted — it IS the flag-off path. The implementations here + * are a deliberate parallel of that inline logic so the two paths can be parity- + * checked against each other; "moved, not duplicated" applies to the flag-ON + * path only. + * + * Hook classes (KTD-2): + * - guard (sync, in-lock): merge-blocker, human-review. Implemented as the + * `evaluateDefaultWorkflowGuards` reader; pure DB-free reads off the task. + * - onEnter / onExit (mutating, applied in-lock to the in-memory task before + * the commit for field effects; queue effects run in-txn): timing, + * reset-on-entry, abort-on-exit, merge. + * + * Worktree allocation is explicitly NOT a hook (it stays a substrate capability + * invoked before the move; see store.ts) — there is no `allocateWorktree` hook + * here by design. + * + * The hooks are registered into the shared trait registry on `init` via + * `registerDefaultWorkflowHooks()` (idempotent). They are resolved through + * `getTraitRegistry().resolveTraitHook(...)` so a missing registration degrades + * to a no-op + audit warning rather than crashing. + */ + +import { getTraitRegistry } from "./trait-registry.js"; +import type { TraitAuditWarning } from "./trait-registry.js"; +import { getTaskMergeBlocker } from "./task-merge.js"; +import type { Settings, Task } from "./types.js"; + +// ── Guard evaluation (sync, in-lock) ───────────────────────────────────────── + +/** A guard verdict: undefined = allow; a string reason = reject. */ +export type GuardVerdict = string | undefined; + +/** + * Evaluate the default workflow's sync guards for a move. Reproduces the legacy + * `getTaskMergeBlocker` gate on `in-review → done`. (The default workflow does + * not carry the human-review trait — see the Trait Vocabulary note — so there + * is no human-review guard on this workflow.) + * + * `bypassGuards` (engine-sourced moves, KTD-9) skips guards entirely — the + * caller is responsible for honoring that; this function still computes the + * verdict so callers can choose. The store only consults it when not bypassing. + */ +export function evaluateMergeBlockerGuard( + task: Pick, + fromColumn: string, + toColumn: string, +): GuardVerdict { + if (fromColumn === "in-review" && toColumn === "done") { + return getTaskMergeBlocker(task); + } + return undefined; +} + +// ── Move-effect context ─────────────────────────────────────────────────────── + +/** Side-effect callbacks the store provides so the hooks stay engine-free and + * DB-handle-free; the store wires these to its in-txn / post-commit machinery. */ +export interface DefaultWorkflowMoveContext { + task: Task; + fromColumn: string; + toColumn: string; + moveSource: "user" | "engine"; + /** True when guards + abort-on-exit are bypassed (engine/recovery, KTD-9). */ + bypassGuards: boolean; + movedAt: string; + /** Settings snapshot for autoMerge stamping (only read when entering review). */ + settings: Pick | undefined; + /** Move options that influence reopen/timing semantics. */ + options: { + preserveStatus?: boolean; + preserveResumeState?: boolean; + preserveProgress?: boolean; + preserveWorktree?: boolean; + }; + /** Reset all steps to pending + currentStep 0 (store owns the impl). */ + resetSteps: () => void; +} + +// ── Field-mutation effects (applied in-lock, before commit) ─────────────────── +// +// These mirror the inline flag-off mutations in store.ts exactly. They run as +// the resolved onEnter/onExit hook bodies for the default workflow's traits. + +/** `timing` trait (in-progress): accumulate active ms on exit, stamp timing on + * entry. */ +export function applyTimingEffects(ctx: DefaultWorkflowMoveContext): void { + const { task, fromColumn, toColumn } = ctx; + if (fromColumn === "in-progress" && toColumn !== "in-progress") { + const segmentStartMs = Date.parse(task.executionStartedAt ?? task.columnMovedAt ?? ctx.movedAt); + const segmentEndMs = Date.parse(task.columnMovedAt ?? ctx.movedAt); + const segmentDeltaMs = + Number.isFinite(segmentStartMs) && Number.isFinite(segmentEndMs) + ? Math.max(0, segmentEndMs - segmentStartMs) + : 0; + task.cumulativeActiveMs = Math.max(0, task.cumulativeActiveMs ?? 0) + segmentDeltaMs; + } + if (toColumn === "in-progress") { + task.cumulativeActiveMs ??= 0; + if (!task.firstExecutionAt) task.firstExecutionAt = task.columnMovedAt; + if (!task.executionStartedAt) task.executionStartedAt = task.columnMovedAt; + task.userPaused = undefined; + } +} + +/** Stamp `executionCompletedAt` on entry to a completion column. */ +export function applyCompletionTimingEffects(ctx: DefaultWorkflowMoveContext): void { + const { task, toColumn } = ctx; + if (toColumn === "done" && !task.executionCompletedAt) { + task.executionCompletedAt = task.columnMovedAt; + } +} + +/** `reset-on-entry` trait (todo/triage reopen) + `abort-on-exit` userPaused + * semantics. Reproduces the legacy reopen block. */ +export function applyResetOnEntryEffects(ctx: DefaultWorkflowMoveContext): void { + const { task, fromColumn, toColumn, moveSource, options } = ctx; + const isReopenToTodoOrTriage = + (fromColumn === "in-progress" || fromColumn === "done" || fromColumn === "in-review") && + (toColumn === "todo" || toColumn === "triage"); + if (!isReopenToTodoOrTriage) return; + + if (!options.preserveStatus) { + task.status = undefined; + task.error = undefined; + task.pausedReason = undefined; + } + task.blockedBy = undefined; + task.overlapBlockedBy = undefined; + task.paused = undefined; + task.pausedByAgentId = undefined; + // abort-on-exit userPaused: only for user-source moves to todo (KTD-9). + if (moveSource === "user" && toColumn === "todo") { + task.userPaused = true; + } else { + task.userPaused = undefined; + } + + const hasNonPendingStepProgress = task.steps.some((step) => step.status !== "pending"); + const preserveStepProgress = + options.preserveResumeState || (options.preserveProgress === true && hasNonPendingStepProgress); + + if (!options.preserveWorktree) { + task.worktree = undefined; + } + if (!options.preserveResumeState) { + task.executionStartedAt = undefined; + task.executionCompletedAt = undefined; + } else { + task.executionCompletedAt = undefined; + } + if (!preserveStepProgress) { + ctx.resetSteps(); + // Prompt-checkbox reset is a filesystem effect; the store performs it + // post-hook (it owns the task dir). Not modeled here. + } +} + +/** `merge` trait onEnter (in-review): autoMerge stamping + scheduler-state + * clearing. The queue enqueue itself is in-txn and store-owned (handoff path); + * the field effects mirror the legacy in-review block. */ +export function applyInReviewEnterEffects(ctx: DefaultWorkflowMoveContext): void { + const { task, toColumn, settings } = ctx; + if (toColumn !== "in-review") return; + if (task.autoMerge === undefined && settings) { + task.autoMerge = settings.autoMerge; + } + task.recoveryRetryCount = undefined; + task.nextRecoveryAt = undefined; + if (task.status === "queued") { + task.status = undefined; + } + task.blockedBy = undefined; + task.overlapBlockedBy = undefined; +} + +/** Reopen-from-review/done field clears (branch/summary/workflowStepResults). */ +export function applyReopenFieldClears(ctx: DefaultWorkflowMoveContext): void { + const { task, fromColumn, toColumn } = ctx; + if ( + (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) || + (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage")) + ) { + task.workflowStepResults = undefined; + } + if (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "triage")) { + task.branch = undefined; + task.executionStartBranch = undefined; + task.baseCommitSha = undefined; + task.summary = undefined; + task.recoveryRetryCount = undefined; + task.nextRecoveryAt = undefined; + } +} + +/** + * Apply ALL default-workflow field-mutation move effects (the parallel of the + * legacy inline block) in the legacy order. Pure in-memory mutation of + * `ctx.task`; queue/filesystem/post-commit effects remain store-owned. + * + * This is the entry point the flag-ON store path calls. It resolves each + * trait's hook through the registry first (so a missing registration degrades to + * a no-op + audit warning, satisfying the "invokes through the registry" + * contract and the degraded-hook path); resolution warnings are collected and + * returned for the store to forward to audit. + */ +export function applyDefaultWorkflowMoveEffects( + ctx: DefaultWorkflowMoveContext, +): { warnings: TraitAuditWarning[] } { + const registry = getTraitRegistry(); + const warnings: TraitAuditWarning[] = []; + + // Resolve the hooks through the registry. The resolved impls are the closures + // registered by registerDefaultWorkflowHooks(); resolution surfaces a warning + // (and a no-op) if a registration is missing. + const toRun: Array<{ traitId: string; hookKind: "onEnter" | "onExit" }> = [ + { traitId: "timing", hookKind: "onExit" }, + { traitId: "timing", hookKind: "onEnter" }, + { traitId: "reset-on-entry", hookKind: "onEnter" }, + { traitId: "abort-on-exit", hookKind: "onExit" }, + { traitId: "merge", hookKind: "onEnter" }, + ]; + for (const { traitId, hookKind } of toRun) { + const { impl, warning } = registry.resolveTraitHook(traitId, hookKind); + if (warning) warnings.push(warning); + if (impl) impl(ctx); + } + + return { warnings }; +} + +// ── Registration into the trait registry (DI seam) ─────────────────────────── + +let registered = false; + +/** + * Register the default-workflow hook implementations into the shared trait + * registry. Idempotent. Called at store init (the store is the engine-adjacent + * owner of the move lifecycle). Each registration is a thin adapter that runs + * the corresponding field-effect function over the move context. + * + * The legacy effects map onto traits as: + * timing.onExit / timing.onEnter → applyTimingEffects + completion stamp + * reset-on-entry.onEnter → applyResetOnEntryEffects + reopen clears + * abort-on-exit.onExit → (userPaused handled in reset-on-entry; + * session abort is an engine effect U6/U7) + * merge.onEnter → applyInReviewEnterEffects + */ +export function registerDefaultWorkflowHooks(): void { + if (registered) return; + const registry = getTraitRegistry(); + + const cast = (fn: (ctx: DefaultWorkflowMoveContext) => void) => + ((...args: unknown[]) => fn(args[0] as DefaultWorkflowMoveContext)) as ( + ...args: unknown[] + ) => unknown; + + registry.registerTraitHookImpl( + "timing", + "onExit", + cast((ctx) => { + applyTimingEffects(ctx); + }), + ); + registry.registerTraitHookImpl( + "timing", + "onEnter", + cast((ctx) => { + applyCompletionTimingEffects(ctx); + }), + ); + registry.registerTraitHookImpl( + "reset-on-entry", + "onEnter", + cast((ctx) => { + applyResetOnEntryEffects(ctx); + applyReopenFieldClears(ctx); + }), + ); + registry.registerTraitHookImpl( + "abort-on-exit", + "onExit", + cast(() => { + // userPaused is set in applyResetOnEntryEffects (the legacy ordering keeps + // it with the reopen block). Session-abort wiring is an engine effect that + // lands with U6/U7; here it is intentionally a no-op so the resolved hook + // exists (not a missing-impl warning) while carrying no field mutation. + }), + ); + registry.registerTraitHookImpl( + "merge", + "onEnter", + cast((ctx) => { + applyInReviewEnterEffects(ctx); + }), + ); + + registered = true; +} + +/** Test-only: allow re-registration after a registry reset. */ +export function __resetDefaultWorkflowHooksForTests(): void { + registered = false; +} diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index da4b6ccec2..d02c509c2b 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -124,6 +124,14 @@ export type { TransitionPendingDbHandle, ReconcileHooksResult, } from "./transition-pending.js"; +// ── U4: workflow-resolved transition adjacency + flag accessor ─────────────── +export { + resolveColumnAdjacency, + resolveAllowedColumns, + workflowHasColumn, +} from "./workflow-transitions.js"; +export type { ColumnAdjacency } from "./workflow-transitions.js"; +export { isWorkflowColumnsEnabled } from "./workflow-columns-settings.js"; export { readTransitionPending, writeTransitionPending, @@ -259,6 +267,7 @@ export { MergeQueueLeaseOwnershipError, InvalidMergeQueueLeaseDurationError, HandoffInvariantViolationError, + TransitionRejectionError, } from "./store.js"; export { STOPWORDS, diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 2322ed9883..061ebc4d12 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -8,6 +8,25 @@ import { createActivityLogSnapshot, createRunAuditSnapshot, createTaskMetadataSn import { VALID_TRANSITIONS, DEFAULT_SETTINGS, isGlobalOnlySettingsKey, WORKFLOW_STEP_TEMPLATES, validateDocumentKey } from "./types.js"; import { DEFAULT_PROJECT_SETTINGS } from "./settings-schema.js"; import { parseWorkflowIr, serializeWorkflowIr } from "./workflow-ir.js"; +import { isWorkflowColumnsEnabled } from "./workflow-columns-settings.js"; +import { resolveAllowedColumns, workflowHasColumn } from "./workflow-transitions.js"; +import { + type DefaultWorkflowMoveContext, + applyDefaultWorkflowMoveEffects, + evaluateMergeBlockerGuard, + registerDefaultWorkflowHooks, +} from "./default-workflow-hooks.js"; +import { + type TransitionRejection, + makeTransitionRejection, + makeTransitionPending, +} from "./transition-types.js"; +import { writeTransitionPending, clearTransitionPending } from "./transition-pending.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "./builtin-coding-workflow-ir.js"; +import type { WorkflowIr } from "./workflow-ir-types.js"; +// Side-effect import: registers the 14 built-in trait DEFINITIONS into the +// shared trait registry on load (the flag-ON path resolves traits by id). +import "./builtin-traits.js"; import type { WorkflowDefinition, WorkflowDefinitionInput, @@ -1047,6 +1066,28 @@ export class HandoffInvariantViolationError extends Error { } } +/** + * Thrown by the flag-ON (`workflowColumns`) `moveTaskInternal` path when a move + * is rejected, carrying the typed {@link TransitionRejection} (KTD-3/R13). The + * existing callers of `moveTask` catch thrown `Error`s (e.g. the dashboard move + * route inspects `err.message`), so the rejection rides on an `Error` subclass + * — `.message` reproduces the legacy human-readable string so flag-ON callers + * that only read the message keep working, while `.rejection` exposes the + * machine-stable code/messageKey/retryable for surfaces that want it. + * + * The FLAG-OFF path still throws the bare legacy `Error` strings unchanged + * (zero behavior change while the flag is off — proven by the characterization + * suite). + */ +export class TransitionRejectionError extends Error { + readonly rejection: TransitionRejection; + constructor(rejection: TransitionRejection, message: string) { + super(message); + this.name = "TransitionRejectionError"; + this.rejection = rejection; + } +} + interface MoveTaskOptions { preserveResumeState?: boolean; preserveProgress?: boolean; @@ -1056,6 +1097,15 @@ interface MoveTaskOptions { moveSource?: "user" | "engine"; skipMergeBlocker?: boolean; allowDirectInReviewMove?: boolean; + /** + * KTD-9: engine/recovery moves bypass trait guards and abort-on-exit effects + * (the generalization of `skipMergeBlocker`). It NEVER bypasses capacity + * (KTD-10). Engine-internal only: HTTP move endpoints hardcode it off and must + * never forward a caller-supplied value (mirrors the hardcoded + * `moveSource: "user"` posture). When unset, the flag-ON path derives it from + * `moveSource === "engine"` plus `skipMergeBlocker`. + */ + bypassGuards?: boolean; } interface MoveTaskInternalOptions { @@ -1395,7 +1445,14 @@ export class TaskStore extends EventEmitter { async init(): Promise { await mkdir(this.tasksDir, { recursive: true }); - + + // U4: register the default-workflow trait hook implementations into the + // shared trait registry (the flag-ON moveTaskInternal path resolves the + // legacy per-column effects through these). Idempotent; built-in trait + // DEFINITIONS self-register on import of ./builtin-traits.js (pulled in + // transitively via default-workflow-hooks / trait-registry). + registerDefaultWorkflowHooks(); + // Initialize SQLite database if (!this._db) { // Startup corruption guard: before opening, detect a malformed fusion.db @@ -5533,6 +5590,9 @@ export class TaskStore extends EventEmitter { { ...opts.moveOptions, skipMergeBlocker: true, + // KTD-9: handoff is an engine/recovery-class move; its skipMergeBlocker + // maps onto bypassGuards under the flag (identical behavior both paths). + bypassGuards: true, }, { fromHandoff: true, @@ -5560,6 +5620,27 @@ export class TaskStore extends EventEmitter { const task = currentTask ?? await this.readTaskForMove(id); const moveSource = options?.moveSource ?? "engine"; + // ── U4: flag-gated workflow-resolved transition path (KTD-8) ───────────── + // Flag OFF (default): the legacy `VALID_TRANSITIONS` / inline-side-effect + // path below runs byte-identical (proven by the characterization suite). + // Flag ON: validate against the task's resolved workflow column graph, run + // sync trait guards (unless bypassed), and route the legacy per-column side + // effects through the default-workflow trait hooks. + // `experimentalFeatures` is a global-scoped setting, so the project-only + // `getSettingsSync()` row would miss it — read merged settings (global + + // project) via getSettingsFast(). This is an async read taken before the + // lock-sensitive transaction; it does not touch the task lock. + const useWorkflow = isWorkflowColumnsEnabled(await this.getSettingsFast()); + // bypassGuards (KTD-9): engine-sourced moves + the existing skipMergeBlocker + // call sites map onto it. Capacity (KTD-10) is NEVER bypassed by this — the + // capacity check is not a guard (U6 fills the enforcement; U4 leaves a + // pass-through slot). An explicit option value wins; otherwise derive it. + const bypassGuards = + options?.bypassGuards ?? (moveSource === "engine" || options?.skipMergeBlocker === true); + const workflowIr: WorkflowIr | undefined = useWorkflow + ? this.resolveTaskWorkflowIrSync(id) + : undefined; + if (task.column === toColumn) { if (internal.fromHandoff && toColumn === "in-review") { this.db.transactionImmediate(() => { @@ -5616,19 +5697,70 @@ export class TaskStore extends EventEmitter { return task; } - const validTargets = VALID_TRANSITIONS[task.column]; - if (!validTargets.includes(toColumn)) { - throw new Error( - `Invalid transition: '${task.column}' → '${toColumn}'. ` + - `Valid targets: ${validTargets.join(", ") || "none"}`, - ); - } - const fromColumn = task.column; - if (fromColumn === "in-review" && toColumn === "done" && !options?.skipMergeBlocker) { - const mergeBlocker = getTaskMergeBlocker(task); - if (mergeBlocker) { - throw new Error(`Cannot move ${id} to done: ${mergeBlocker}`); + + if (useWorkflow && workflowIr) { + // ── Flag-ON validation + sync guards (typed rejections, KTD-3/R13) ───── + // 1. Target column must exist in the task's workflow → unknown-column. + if (!workflowHasColumn(workflowIr, toColumn)) { + throw new TransitionRejectionError( + makeTransitionRejection( + "unknown-column", + "transition.rejected.unknownColumn", + false, + `Column '${toColumn}' is not defined in this task's workflow`, + ), + `Invalid transition: '${fromColumn}' → '${toColumn}'. Unknown column for this workflow.`, + ); + } + // 2. Column-graph adjacency. For the default workflow this reproduces + // VALID_TRANSITIONS verbatim (resolveAllowedColumns); the + // transition-parity suite machine-checks the equivalence. + const allowed = resolveAllowedColumns(workflowIr, fromColumn); + if (!allowed.includes(toColumn)) { + throw new TransitionRejectionError( + makeTransitionRejection( + "guard-rejected", + "transition.rejected.invalidTransition", + false, + `Valid targets: ${allowed.join(", ") || "none"}`, + ), + `Invalid transition: '${fromColumn}' → '${toColumn}'. ` + + `Valid targets: ${allowed.join(", ") || "none"}`, + ); + } + // 3. Sync trait guards (in-lock). Skipped entirely when bypassGuards + // (engine/recovery moves, KTD-9). The default workflow's merge-blocker + // trait reads the same getTaskMergeBlocker. + if (!bypassGuards) { + const guardReason = evaluateMergeBlockerGuard(task, fromColumn, toColumn); + if (guardReason) { + throw new TransitionRejectionError( + makeTransitionRejection( + "merge-blocked", + "transition.rejected.mergeBlocked", + true, + guardReason, + ), + `Cannot move ${id} to done: ${guardReason}`, + ); + } + } + } else { + // ── Flag-OFF legacy path (unchanged) ─────────────────────────────────── + const validTargets = VALID_TRANSITIONS[task.column]; + if (!validTargets.includes(toColumn)) { + throw new Error( + `Invalid transition: '${task.column}' → '${toColumn}'. ` + + `Valid targets: ${validTargets.join(", ") || "none"}`, + ); + } + + if (fromColumn === "in-review" && toColumn === "done" && !options?.skipMergeBlocker) { + const mergeBlocker = getTaskMergeBlocker(task); + if (mergeBlocker) { + throw new Error(`Cannot move ${id} to done: ${mergeBlocker}`); + } } } @@ -5642,106 +5774,153 @@ export class TaskStore extends EventEmitter { task.columnMovedAt = movedAt; task.updatedAt = movedAt; - if (fromColumn === "in-progress" && toColumn !== "in-progress") { - const segmentStartMs = Date.parse(task.executionStartedAt ?? task.columnMovedAt); - const segmentEndMs = Date.parse(task.columnMovedAt); - const segmentDeltaMs = - Number.isFinite(segmentStartMs) && Number.isFinite(segmentEndMs) - ? Math.max(0, segmentEndMs - segmentStartMs) - : 0; - task.cumulativeActiveMs = Math.max(0, task.cumulativeActiveMs ?? 0) + segmentDeltaMs; - } - - if (toColumn === "in-progress") { - task.cumulativeActiveMs ??= 0; - if (!task.firstExecutionAt) { - task.firstExecutionAt = task.columnMovedAt; - } - if (!task.executionStartedAt) { - task.executionStartedAt = task.columnMovedAt; - } - task.userPaused = undefined; - } - if (toColumn === "done" && !task.executionCompletedAt) { - task.executionCompletedAt = task.columnMovedAt; - } - - if (toColumn === "done") { - this.clearDoneTransientFields(task); - } - - const isReopenToTodoOrTriage = - (fromColumn === "in-progress" || fromColumn === "done" || fromColumn === "in-review") - && (toColumn === "todo" || toColumn === "triage"); - - if (isReopenToTodoOrTriage) { - if (!options?.preserveStatus) { - task.status = undefined; - task.error = undefined; - task.pausedReason = undefined; - } - task.blockedBy = undefined; - task.overlapBlockedBy = undefined; - task.paused = undefined; - task.pausedByAgentId = undefined; - if (moveSource === "user" && toColumn === "todo") { - task.userPaused = true; - } else { - task.userPaused = undefined; - } - + if (useWorkflow) { + // ── Flag-ON: route the legacy per-column side effects through the + // default-workflow trait hooks (timing, reset-on-entry, abort-on-exit, + // merge.onEnter). "Moved, not duplicated" applies to this path; the + // flag-off branch below keeps the legacy inline code verbatim. ─────── + const ctx: DefaultWorkflowMoveContext = { + task, + fromColumn, + toColumn, + moveSource, + bypassGuards, + movedAt, + settings: settingsForInReview, + options: { + preserveStatus: options?.preserveStatus, + preserveResumeState: options?.preserveResumeState, + preserveProgress: options?.preserveProgress, + preserveWorktree: options?.preserveWorktree, + }, + resetSteps: () => this.resetAllStepsToPending(task), + }; + const isReopenToTodoOrTriage = + (fromColumn === "in-progress" || fromColumn === "done" || fromColumn === "in-review") && + (toColumn === "todo" || toColumn === "triage"); const hasNonPendingStepProgress = task.steps.some((step) => step.status !== "pending"); const preserveStepProgress = - options?.preserveResumeState || (options?.preserveProgress === true && hasNonPendingStepProgress); - - if (!options?.preserveWorktree) { - task.worktree = undefined; + options?.preserveResumeState || + (options?.preserveProgress === true && hasNonPendingStepProgress); + const { warnings } = applyDefaultWorkflowMoveEffects(ctx); + for (const warning of warnings) { + storeLog.warn("Default-workflow trait hook degraded to no-op", { + phase: "moveTaskInternal:workflow-hooks", + taskId: id, + ...warning, + }); } - - if (!options?.preserveResumeState) { - task.executionStartedAt = undefined; - task.executionCompletedAt = undefined; - } else { - task.executionCompletedAt = undefined; + // Store-owned effects the hooks intentionally do NOT perform (filesystem / + // store-private): clearing done transient fields + prompt-checkbox reset. + if (toColumn === "done") { + this.clearDoneTransientFields(task); } - - if (!preserveStepProgress) { - this.resetAllStepsToPending(task); + if (isReopenToTodoOrTriage && !preserveStepProgress) { await this.resetPromptCheckboxes(dir); } - } - - if (toColumn === "in-review") { - if (task.autoMerge === undefined && settingsForInReview) { - task.autoMerge = settingsForInReview.autoMerge; + } else { + // ── Flag-OFF legacy inline side effects (UNCHANGED — the flag-off path) ── + if (fromColumn === "in-progress" && toColumn !== "in-progress") { + const segmentStartMs = Date.parse(task.executionStartedAt ?? task.columnMovedAt); + const segmentEndMs = Date.parse(task.columnMovedAt); + const segmentDeltaMs = + Number.isFinite(segmentStartMs) && Number.isFinite(segmentEndMs) + ? Math.max(0, segmentEndMs - segmentStartMs) + : 0; + task.cumulativeActiveMs = Math.max(0, task.cumulativeActiveMs ?? 0) + segmentDeltaMs; } - task.recoveryRetryCount = undefined; - task.nextRecoveryAt = undefined; - // Clear scheduler-side dispatch state: `queued`, `blockedBy`, and - // `overlapBlockedBy` are stamped while the task waits in `todo`. If - // they survive the transition into `in-review` they permanently block - // the merge gate (see getTaskMergeBlocker's BLOCKING_TASK_STATUSES). - if (task.status === "queued") { - task.status = undefined; + + if (toColumn === "in-progress") { + task.cumulativeActiveMs ??= 0; + if (!task.firstExecutionAt) { + task.firstExecutionAt = task.columnMovedAt; + } + if (!task.executionStartedAt) { + task.executionStartedAt = task.columnMovedAt; + } + task.userPaused = undefined; + } + if (toColumn === "done" && !task.executionCompletedAt) { + task.executionCompletedAt = task.columnMovedAt; } - task.blockedBy = undefined; - task.overlapBlockedBy = undefined; - } - if ( - (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) - || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage")) - ) { - task.workflowStepResults = undefined; - } + if (toColumn === "done") { + this.clearDoneTransientFields(task); + } - if (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "triage")) { - task.branch = undefined; - task.executionStartBranch = undefined; - task.baseCommitSha = undefined; - task.summary = undefined; - task.recoveryRetryCount = undefined; - task.nextRecoveryAt = undefined; + const isReopenToTodoOrTriage = + (fromColumn === "in-progress" || fromColumn === "done" || fromColumn === "in-review") + && (toColumn === "todo" || toColumn === "triage"); + + if (isReopenToTodoOrTriage) { + if (!options?.preserveStatus) { + task.status = undefined; + task.error = undefined; + task.pausedReason = undefined; + } + task.blockedBy = undefined; + task.overlapBlockedBy = undefined; + task.paused = undefined; + task.pausedByAgentId = undefined; + if (moveSource === "user" && toColumn === "todo") { + task.userPaused = true; + } else { + task.userPaused = undefined; + } + + const hasNonPendingStepProgress = task.steps.some((step) => step.status !== "pending"); + const preserveStepProgress = + options?.preserveResumeState || (options?.preserveProgress === true && hasNonPendingStepProgress); + + if (!options?.preserveWorktree) { + task.worktree = undefined; + } + + if (!options?.preserveResumeState) { + task.executionStartedAt = undefined; + task.executionCompletedAt = undefined; + } else { + task.executionCompletedAt = undefined; + } + + if (!preserveStepProgress) { + this.resetAllStepsToPending(task); + await this.resetPromptCheckboxes(dir); + } + } + + if (toColumn === "in-review") { + if (task.autoMerge === undefined && settingsForInReview) { + task.autoMerge = settingsForInReview.autoMerge; + } + task.recoveryRetryCount = undefined; + task.nextRecoveryAt = undefined; + // Clear scheduler-side dispatch state: `queued`, `blockedBy`, and + // `overlapBlockedBy` are stamped while the task waits in `todo`. If + // they survive the transition into `in-review` they permanently block + // the merge gate (see getTaskMergeBlocker's BLOCKING_TASK_STATUSES). + if (task.status === "queued") { + task.status = undefined; + } + task.blockedBy = undefined; + task.overlapBlockedBy = undefined; + } + + if ( + (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "in-progress" || toColumn === "triage")) + || (fromColumn === "done" && (toColumn === "todo" || toColumn === "triage")) + ) { + task.workflowStepResults = undefined; + } + + if (fromColumn === "in-review" && (toColumn === "todo" || toColumn === "triage")) { + task.branch = undefined; + task.executionStartBranch = undefined; + task.baseCommitSha = undefined; + task.summary = undefined; + task.recoveryRetryCount = undefined; + task.nextRecoveryAt = undefined; + } } if (toColumn === "in-progress" && !task.worktree && options?.allocateWorktree) { @@ -5785,6 +5964,22 @@ export class TaskStore extends EventEmitter { }); this.dequeueMergeQueueOnColumnExit(id, fromColumn, toColumn, movedAt); + // U4 (flag-ON): write the crash-safe transitionPending marker in the SAME + // transaction as the column change (KTD-2). It records the post-commit + // hooks that still owe idempotent execution so a crash mid-transition is + // recoverable from SQLite (the authoritative store, ADR-0001). The store + // clears it immediately after the post-commit hook runner completes + // (below). For the default workflow the field effects already applied + // in-lock; the marker guards the post-commit completion so recovery never + // double-runs (idempotent) and never strands the card. + if (useWorkflow) { + writeTransitionPending( + this.db, + id, + makeTransitionPending(toColumn, ["default-workflow:postCommit"], Date.parse(movedAt) || Date.now()), + ); + } + if (toColumn === "in-review" && !internal.fromHandoff && options?.allowDirectInReviewMove !== true) { this.insertRunAuditEventRow({ taskId: id, @@ -5859,6 +6054,23 @@ export class TaskStore extends EventEmitter { if (this.isWatching) this.taskCache.set(id, { ...task }); + // U4 (flag-ON): post-commit hook completion. The default-workflow field + // effects already ran in-lock and committed; the post-commit phase here is + // the fire-and-forget hook runner per KTD-2. It is idempotent and clears the + // transitionPending marker once done. A crash before this point leaves the + // marker for the recovery sweep to re-run (re-running is a no-op for the + // default workflow's already-committed field effects). We clear it + // synchronously here because the default workflow has no async post-commit + // hook bodies in U4 (merge enqueue is in-txn via the handoff path); + // plugin/async post-commit hooks land in U7/U8 and will defer the clear. + if (useWorkflow) { + try { + clearTransitionPending(this.db, id); + } catch { + // Clearing is best-effort; the marker recovery sweep is the backstop. + } + } + if (fromColumn !== toColumn) { this.emit("task:moved", { task, from: fromColumn, to: toColumn, source: moveSource }); } @@ -11436,6 +11648,36 @@ ${stepsSection}`; } /** Read the workflow currently selected for a task, if any. */ + /** + * Synchronously resolve the parsed WorkflowIr that governs a task's columns + * (U4, flag-ON path). Resolution order: + * 1. the task's workflow selection (side table) → that workflow's IR; + * 2. null/missing selection → the built-in default workflow IR (KTD-1). + * Built-in workflow IRs are resolved from the parsed module constant; custom + * workflows are read + parsed from the `workflows` row. Pure DB read, safe to + * call inside `withTaskLock` (no further locks taken). A parse failure or + * missing custom row falls back to the default workflow so a move is never + * stranded by a corrupt definition (degraded, not crashed). + */ + private resolveTaskWorkflowIrSync(taskId: string): WorkflowIr { + const selection = this.getTaskWorkflowSelection(taskId); + const workflowId = selection?.workflowId; + if (!workflowId) return BUILTIN_CODING_WORKFLOW_IR; + if (isBuiltinWorkflowId(workflowId)) { + const builtin = getBuiltinWorkflow(workflowId); + return builtin?.ir ?? BUILTIN_CODING_WORKFLOW_IR; + } + try { + const row = this.db + .prepare("SELECT ir FROM workflows WHERE id = ?") + .get(workflowId) as { ir: string } | undefined; + if (!row) return BUILTIN_CODING_WORKFLOW_IR; + return parseWorkflowIr(row.ir); + } catch { + return BUILTIN_CODING_WORKFLOW_IR; + } + } + getTaskWorkflowSelection(taskId: string): { workflowId: string; stepIds: string[] } | undefined { const row = this.db .prepare("SELECT workflowId, stepIds FROM task_workflow_selection WHERE taskId = ?") diff --git a/packages/core/src/workflow-columns-settings.ts b/packages/core/src/workflow-columns-settings.ts new file mode 100644 index 0000000000..e5a946546f --- /dev/null +++ b/packages/core/src/workflow-columns-settings.ts @@ -0,0 +1,18 @@ +import { isExperimentalFeatureEnabled } from "./experimental-features.js"; +import type { Settings } from "./types.js"; + +/** + * The `experimentalFeatures.workflowColumns` flag (KTD-8). OFF: the legacy + * enum/`VALID_TRANSITIONS` path runs untouched. ON: `moveTaskInternal` resolves + * each task's workflow column graph + trait guards. Default OFF until the + * transition-parity suite and field observations prove zero drift (U12). + * + * Mirrors `isSandboxExperimentalEnabled` / `isEvalsViewEnabled` — a thin, + * named accessor over the shared experimental-features map so the literal flag + * key lives in exactly one place. + */ +export function isWorkflowColumnsEnabled( + settings: Pick | undefined, +): boolean { + return isExperimentalFeatureEnabled(settings, "workflowColumns"); +} diff --git a/packages/core/src/workflow-transitions.ts b/packages/core/src/workflow-transitions.ts new file mode 100644 index 0000000000..ed81c1243d --- /dev/null +++ b/packages/core/src/workflow-transitions.ts @@ -0,0 +1,108 @@ +/** + * Workflow-resolved transition adjacency (U4, R4/R9/R13). + * + * `moveTaskInternal` (flag ON) and `board.ts` both derive "which columns can a + * card move to from here" from the SAME helper so the two surfaces never + * diverge — `resolveAllowedColumns(ir, fromColumn)`. + * + * ── Why an explicit adjacency, not pure graph-derivation ────────────────────── + * + * The plan asks: derive allowed column adjacency from node placement + edges, + * and for the DEFAULT workflow it MUST reproduce `VALID_TRANSITIONS` exactly. + * Pure graph-edge derivation CANNOT reproduce it: `VALID_TRANSITIONS` encodes + * backward/reopen edges (in-review → todo, done → todo, archived → done, …) and + * cross edges (in-progress → done) that have no counterpart in the linear + * execute → review → merge → end pipeline graph. The IR edges describe the + * forward automation walk; the column adjacency describes legal *board* moves + * (drags, reopens, recovery), which is a strictly larger, partly-cyclic set. + * + * So per the plan's documented fallback we attach an explicit per-column + * `transitions` adjacency: + * - For the BUILT-IN default workflow we reproduce `VALID_TRANSITIONS` verbatim + * (keyed by the legacy column ids, which are exactly the default workflow's + * column ids — KTD-1). This is the parity contract the transition-parity + * suite machine-checks. + * - For CUSTOM workflows (no explicit adjacency authored yet — authoring lands + * with the editor in U10) we derive a linear forward+back adjacency from the + * declared column ORDER: each column may move to its neighbors (prev/next). + * This is a safe, predictable default that keeps every column reachable and + * never strands a card; richer custom adjacency is future work. + * + * The adjacency is intentionally a column→columns map computed once per IR; it + * is read-only and pure. + */ + +import { VALID_TRANSITIONS } from "./types.js"; +import type { Column } from "./types.js"; +import type { WorkflowIr, WorkflowIrV2 } from "./workflow-ir-types.js"; +import { DEFAULT_WORKFLOW_COLUMN_IDS } from "./workflow-ir.js"; + +/** A column→allowed-target-columns adjacency map. */ +export type ColumnAdjacency = Map; + +/** True when the IR's columns are exactly the legacy default-workflow column ids + * (same set), i.e. this is the built-in default workflow (or an equivalent). */ +function isDefaultWorkflowColumns(ir: WorkflowIrV2): boolean { + const ids = ir.columns.map((c) => c.id); + if (ids.length !== DEFAULT_WORKFLOW_COLUMN_IDS.length) return false; + const set = new Set(ids); + return DEFAULT_WORKFLOW_COLUMN_IDS.every((id) => set.has(id)); +} + +/** Build the verbatim `VALID_TRANSITIONS` adjacency keyed by column id. */ +function defaultWorkflowAdjacency(): ColumnAdjacency { + const adj: ColumnAdjacency = new Map(); + for (const [from, targets] of Object.entries(VALID_TRANSITIONS) as [Column, Column[]][]) { + adj.set(from, [...targets]); + } + return adj; +} + +/** Derive a neighbor (prev/next by declared order) adjacency for a custom + * workflow. Each column can move to the column before and after it in the + * authored order. Endpoints have a single neighbor. */ +function orderDerivedAdjacency(ir: WorkflowIrV2): ColumnAdjacency { + const adj: ColumnAdjacency = new Map(); + const ids = ir.columns.map((c) => c.id); + for (let i = 0; i < ids.length; i++) { + const targets: string[] = []; + if (i > 0) targets.push(ids[i - 1]); + if (i < ids.length - 1) targets.push(ids[i + 1]); + adj.set(ids[i], targets); + } + return adj; +} + +/** + * Resolve the full column adjacency for a workflow IR. The default workflow + * reproduces `VALID_TRANSITIONS` exactly; custom workflows use order-derived + * neighbor adjacency. + */ +export function resolveColumnAdjacency(ir: WorkflowIr): ColumnAdjacency { + // v1 IR is upgraded to v2 on parse, but accept either defensively. + const v2 = ir as WorkflowIrV2; + if (!Array.isArray(v2.columns)) { + // No columns (shouldn't happen post-parse) → empty adjacency. + return new Map(); + } + if (isDefaultWorkflowColumns(v2)) { + return defaultWorkflowAdjacency(); + } + return orderDerivedAdjacency(v2); +} + +/** + * The allowed target columns for a move out of `fromColumn` under this workflow. + * Returns an empty array when `fromColumn` is unknown to the workflow (callers + * should first check column existence to distinguish "unknown column" from "no + * legal targets"). + */ +export function resolveAllowedColumns(ir: WorkflowIr, fromColumn: string): string[] { + return resolveColumnAdjacency(ir).get(fromColumn) ?? []; +} + +/** True when `toColumn` is a defined column of the workflow. */ +export function workflowHasColumn(ir: WorkflowIr, columnId: string): boolean { + const v2 = ir as WorkflowIrV2; + return Array.isArray(v2.columns) && v2.columns.some((c) => c.id === columnId); +} diff --git a/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.move-bypassguards.test.ts b/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.move-bypassguards.test.ts new file mode 100644 index 0000000000..a0e91911a6 --- /dev/null +++ b/packages/dashboard/src/routes/__tests__/register-task-workflow-routes.move-bypassguards.test.ts @@ -0,0 +1,52 @@ +// @vitest-environment node +// +// U4 hardening: `bypassGuards` is engine-internal (KTD-9). The HTTP move +// endpoint hardcodes its move options (mirroring the hardcoded +// `moveSource: "user"` posture) and must NEVER forward a caller-supplied +// `bypassGuards` (or `moveSource`) from the request body — otherwise a remote +// caller could bypass trait guards / abort-on-exit. + +import { describe, it, expect, vi } from "vitest"; +import express from "express"; +import type { TaskStore } from "@fusion/core"; +import { createApiRoutes } from "../../routes.js"; +import { request as REQUEST } from "../../test-request.js"; + +describe("task move route — bypassGuards is not forwardable", () => { + it("ignores a caller-supplied bypassGuards/moveSource in the request body", async () => { + const moveTask = vi.fn(async (_id: string, column: string, _options?: Record) => ({ + id: "FN-001", + column, + dependencies: [], + steps: [], + currentStep: 0, + })); + + const store: TaskStore = { + getRootDir: vi.fn(() => process.cwd()), + getTask: vi.fn(async () => ({ id: "FN-001", column: "todo" })), + getSettings: vi.fn(async () => ({})), + moveTask, + } as unknown as TaskStore; + + const app = express(); + app.use(express.json()); + app.use("/api", createApiRoutes(store)); + + const res = await REQUEST( + app, + "POST", + "/api/tasks/FN-001/move", + JSON.stringify({ column: "triage", bypassGuards: true, moveSource: "engine" }), + { "content-type": "application/json" }, + ); + + expect(res.status).toBe(200); + expect(moveTask).toHaveBeenCalledTimes(1); + const passedOptions = moveTask.mock.calls[0][2] as Record | undefined; + // The route constructs its own options; the injected fields must not leak. + expect(passedOptions?.bypassGuards).toBeUndefined(); + // The route hardcodes moveSource: "user" — the body's "engine" is ignored. + expect(passedOptions?.moveSource).toBe("user"); + }); +});