From 34a031f9b9fe3e714e2db4e9838cc5c8e1bca2cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 17 Jul 2026 18:47:27 +0100 Subject: [PATCH] feat(web): warn about workflow replay cycles (#1720) --- .../settings/workflow-card-actions.test.ts | 735 ++++++++++++++---- .../settings/workflow-card-actions.ts | 231 +++--- .../web/components/settings/workflow-card.tsx | 86 +- .../workflow-cycle-diagnostic.test.tsx | 174 +++++ .../settings/workflow-cycle-diagnostic.tsx | 188 +++++ .../settings/workflow-mutation-guard.ts | 118 +++ .../settings/workflow-pipeline-editor.tsx | 40 + .../workflow-read-only-actions.test.ts | 160 ++++ .../settings/workflow-reorder-actions.test.ts | 128 +++ .../settings/workflow-step-mutations.ts | 95 +++ apps/web/e2e/pages/workflow-settings-page.ts | 67 +- .../mobile-workflow-cycle-guardrails.spec.ts | 113 +++ .../workflow-cycle-guardrails.spec.ts | 124 +++ .../workflows/replay-cycle-analysis.test.ts | 554 +++++++++++++ .../lib/workflows/replay-cycle-analysis.ts | 503 ++++++++++++ docs/plans/workflow-cycle-guardrails/plan.md | 277 +++++++ .../task-01-cycle-analyzer.md | 54 ++ .../task-02-mutation-guard.md | 54 ++ .../task-03-diagnostic-ui.md | 55 ++ .../task-04-e2e-verification.md | 55 ++ docs/specs/INDEX.md | 1 + docs/specs/workflow-cycle-guardrails/spec.md | 197 +++++ 22 files changed, 3667 insertions(+), 342 deletions(-) create mode 100644 apps/web/components/settings/workflow-cycle-diagnostic.test.tsx create mode 100644 apps/web/components/settings/workflow-cycle-diagnostic.tsx create mode 100644 apps/web/components/settings/workflow-mutation-guard.ts create mode 100644 apps/web/components/settings/workflow-read-only-actions.test.ts create mode 100644 apps/web/components/settings/workflow-reorder-actions.test.ts create mode 100644 apps/web/components/settings/workflow-step-mutations.ts create mode 100644 apps/web/e2e/tests/workflow/mobile-workflow-cycle-guardrails.spec.ts create mode 100644 apps/web/e2e/tests/workflow/workflow-cycle-guardrails.spec.ts create mode 100644 apps/web/lib/workflows/replay-cycle-analysis.test.ts create mode 100644 apps/web/lib/workflows/replay-cycle-analysis.ts create mode 100644 docs/plans/workflow-cycle-guardrails/plan.md create mode 100644 docs/plans/workflow-cycle-guardrails/task-01-cycle-analyzer.md create mode 100644 docs/plans/workflow-cycle-guardrails/task-02-mutation-guard.md create mode 100644 docs/plans/workflow-cycle-guardrails/task-03-diagnostic-ui.md create mode 100644 docs/plans/workflow-cycle-guardrails/task-04-e2e-verification.md create mode 100644 docs/specs/workflow-cycle-guardrails/spec.md diff --git a/apps/web/components/settings/workflow-card-actions.test.ts b/apps/web/components/settings/workflow-card-actions.test.ts index 41508559a3..b32a0bb432 100644 --- a/apps/web/components/settings/workflow-card-actions.test.ts +++ b/apps/web/components/settings/workflow-card-actions.test.ts @@ -1,20 +1,18 @@ import { act, renderHook } from "@testing-library/react"; -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, expect, it, vi } from "vitest"; import { createWorkflowAction, createWorkflowStepAction, deleteWorkflowStepAction, - getWorkflowTaskCount, + getStepTaskCount, listWorkflowStepsAction, reorderWorkflowStepsAction, updateWorkflowStepAction, } from "@/app/actions/workspaces"; +import type { OnTurnCompleteAction } from "@/lib/types/workflow-actions"; import type { Workflow, WorkflowStep } from "@/lib/types/http"; -import { - useWorkflowDeleteHandlers, - useWorkflowSaveActions, - useWorkflowStepActions, -} from "./workflow-card-actions"; +import { useWorkflowSaveActions, useWorkflowStepActions } from "./workflow-card-actions"; +import { useWorkflowMutationGuard } from "./workflow-mutation-guard"; vi.mock("@/app/actions/workspaces", () => ({ createWorkflowAction: vi.fn(), @@ -29,6 +27,18 @@ vi.mock("@/app/actions/workspaces", () => ({ bulkMoveTasks: vi.fn(), })); +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(createWorkflowStepAction).mockImplementation(async (payload) => + step(`created-${payload.position}`, payload.name, payload.position, false), + ); + vi.mocked(updateWorkflowStepAction).mockImplementation(async (stepId, updates) => ({ + ...step(stepId, stepId, updates.position ?? 0, updates.is_start_step ?? false), + ...updates, + })); + vi.mocked(reorderWorkflowStepsAction).mockResolvedValue({ steps: [], total: 0 }); +}); + const workflow = { id: "wf-1", workspace_id: "ws-1", @@ -37,7 +47,13 @@ const workflow = { updated_at: "", } as Workflow; -function step(id: string, name: string, position: number, isStartStep: boolean): WorkflowStep { +function step( + id: string, + name: string, + position: number, + isStartStep: boolean, + options: { autoStart?: boolean; onTurnComplete?: OnTurnCompleteAction[] } = {}, +): WorkflowStep { return { id, workflow_id: workflow.id, @@ -46,6 +62,10 @@ function step(id: string, name: string, position: number, isStartStep: boolean): color: "bg-slate-500", allow_manual_move: true, is_start_step: isStartStep, + events: { + on_enter: options.autoStart ? [{ type: "auto_start_agent" }] : [], + on_turn_complete: options.onTurnComplete, + }, created_at: "", updated_at: "", }; @@ -58,214 +78,587 @@ function renderNewWorkflowStepActions(initialSteps: WorkflowStep[]) { steps = typeof updater === "function" ? updater(steps) : updater; }, ); - const view = renderHook(() => - useWorkflowStepActions({ + const view = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(steps); + return useWorkflowStepActions({ workflow, isNewWorkflow: true, workflowSteps: steps, setWorkflowSteps, - refreshWorkflowSteps: vi.fn(), setStepToDelete: vi.fn(), setStepTaskCount: vi.fn(), setTargetStepForMigration: vi.fn(), setStepDeleteOpen: vi.fn(), toast: vi.fn(), - }), - ); + mutationGuard, + }); + }); return { ...view, getSteps: () => steps }; } -function renderReadOnlyWorkflowStepActions(initialSteps: WorkflowStep[]) { - let steps = initialSteps; +function renderPersistedWorkflowStepActions(steps: WorkflowStep[]) { + let currentSteps = steps; const setWorkflowSteps = vi.fn( (updater: ((prev: WorkflowStep[]) => WorkflowStep[]) | WorkflowStep[]) => { - steps = typeof updater === "function" ? updater(steps) : updater; + currentSteps = typeof updater === "function" ? updater(currentSteps) : updater; }, ); - const refreshWorkflowSteps = vi.fn(); - const view = renderHook(() => - useWorkflowStepActions({ + const setStepDeleteOpen = vi.fn(); + const view = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(steps); + const actions = useWorkflowStepActions({ workflow, isNewWorkflow: false, - readOnly: true, workflowSteps: steps, setWorkflowSteps, - refreshWorkflowSteps, + setStepToDelete: vi.fn(), + setStepTaskCount: vi.fn(), + setTargetStepForMigration: vi.fn(), + setStepDeleteOpen, + toast: vi.fn(), + mutationGuard, + }); + return { actions, mutationGuard }; + }); + return { + ...view, + setWorkflowSteps, + setStepDeleteOpen, + getSteps: () => currentSteps, + }; +} + +function deferredPromise() { + let resolve!: (value: T) => void; + const promise = new Promise((resolvePromise) => { + resolve = resolvePromise; + }); + return { promise, resolve }; +} + +it("keeps one start step while editing a new workflow locally", async () => { + const { result, getSteps } = renderNewWorkflowStepActions([ + step("step-1", "Todo", 0, true), + step("step-2", "Plan", 1, false), + ]); + + await act(async () => { + await result.current.handleUpdateWorkflowStep("step-2", { is_start_step: true }); + }); + + expect( + getSteps() + .filter((s) => s.is_start_step) + .map((s) => s.id), + ).toEqual(["step-2"]); +}); + +it("does not update a persisted step when the proposal introduces a blocking cycle", async () => { + const steps = [ + step("build", "Build", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { autoStart: true }), + ]; + const { result } = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(steps); + return useWorkflowStepActions({ + workflow, + isNewWorkflow: false, + workflowSteps: steps, + setWorkflowSteps: vi.fn(), setStepToDelete: vi.fn(), setStepTaskCount: vi.fn(), setTargetStepForMigration: vi.fn(), setStepDeleteOpen: vi.fn(), toast: vi.fn(), + mutationGuard, + }); + }); + + await act(async () => { + await result.current.handleUpdateWorkflowStep("review", { + events: { + ...steps[1].events, + on_turn_complete: [{ type: "move_to_previous" }], + }, + }); + }); + + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); +}); + +it("does not persist a second topology edit before the first mutation completes", async () => { + const steps = [ + step("work", "Work", 0, true, { autoStart: true }), + step("review", "Review", 1, false), + ]; + const update = deferredPromise(); + vi.mocked(updateWorkflowStepAction).mockReturnValueOnce(update.promise); + const { result } = renderPersistedWorkflowStepActions(steps); + + let firstEdit!: Promise; + await act(async () => { + firstEdit = result.current.actions.handleUpdateWorkflowStep("work", { + events: { on_turn_complete: [{ type: "move_to_next" }] }, + }); + await vi.waitFor(() => expect(updateWorkflowStepAction).toHaveBeenCalledTimes(1)); + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { on_turn_complete: [{ type: "move_to_previous" }] }, + }); + }); + + expect(updateWorkflowStepAction).toHaveBeenCalledTimes(1); + expect(result.current.mutationGuard.isMutationPending).toBe(true); + + update.resolve({ + ...steps[0], + events: { on_turn_complete: [{ type: "move_to_next" }] }, + }); + await act(async () => { + await firstEdit; + }); + expect(result.current.mutationGuard.isMutationPending).toBe(false); +}); + +it("does not replace a pending warning proposal with a second edit", async () => { + const steps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], }), - ); - return { ...view, getSteps: () => steps, refreshWorkflowSteps, setWorkflowSteps }; -} + step("review", "Review", 1, false), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); -describe("useWorkflowStepActions", () => { - it("keeps one start step while editing a new workflow locally", async () => { - const { result, getSteps } = renderNewWorkflowStepActions([ - step("step-1", "Todo", 0, true), - step("step-2", "Plan", 1, false), + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { + ...steps[1].events, + on_turn_complete: [{ type: "move_to_previous" }], + }, + }); + }); + const proposal = result.current.mutationGuard.proposal; + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("work", { name: "Building" }); + }); + + expect(result.current.mutationGuard.proposal).toBe(proposal); + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); +}); + +it("does not replace a pending blocking proposal with a second edit", async () => { + const steps = [ + step("build", "Build", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { autoStart: true }), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { on_turn_complete: [{ type: "move_to_previous" }] }, + }); + }); + const proposal = result.current.mutationGuard.proposal; + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("build", { name: "Building" }); + }); + + expect(result.current.mutationGuard.proposal).toBe(proposal); + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); +}); + +it("releases a blocking proposal if confirmation is invoked defensively", async () => { + const steps = [ + step("build", "Build", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { autoStart: true }), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { + ...steps[1].events, + on_turn_complete: [{ type: "move_to_previous" }], + }, + }); + }); + expect(result.current.mutationGuard.proposal?.severity).toBe("blocking"); + + await act(async () => { + await result.current.mutationGuard.confirmProposal(); + }); + + expect(result.current.mutationGuard.proposal).toBeNull(); + expect(result.current.mutationGuard.isMutationPending).toBe(false); + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); +}); + +it("holds a warning update until confirmation and executes it exactly once", async () => { + const steps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { on_turn_complete: [{ type: "move_to_previous" }] }, + }); + }); + + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.proposal).toMatchObject({ + intent: "apply", + severity: "warning", + }); + + await act(async () => { + await result.current.mutationGuard.confirmProposal(); + await result.current.mutationGuard.confirmProposal(); + }); + + expect(updateWorkflowStepAction).toHaveBeenCalledTimes(1); +}); + +it("discards a warning update when confirmation is cancelled", async () => { + const steps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { on_turn_complete: [{ type: "move_to_previous" }] }, + }); + result.current.mutationGuard.cancelProposal(); + }); + + expect(updateWorkflowStepAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.proposal).toBeNull(); +}); + +it("does not gate an unchanged diagnostic identity or an edit that removes a cycle", async () => { + const steps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { + onTurnComplete: [{ type: "move_to_previous" }], + }), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("work", { prompt: "New prompt" }); + await result.current.actions.handleUpdateWorkflowStep("review", { + events: { on_turn_complete: [] }, + }); + }); + + expect(updateWorkflowStepAction).toHaveBeenCalledTimes(2); + expect(result.current.mutationGuard.proposal).toBeNull(); +}); + +it("preflights a delete shape before any side effect", async () => { + const work = step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }); + const middle = step("middle", "Middle", 1, false); + const review = step("review", "Review", 2, false, { + autoStart: true, + onTurnComplete: [{ type: "move_to_previous" }], + }); + const { result, setWorkflowSteps } = renderPersistedWorkflowStepActions([work, middle, review]); + + await act(async () => { + await result.current.actions.handleRemoveWorkflowStep("middle"); + }); + + expect(getStepTaskCount).not.toHaveBeenCalled(); + expect(deleteWorkflowStepAction).not.toHaveBeenCalled(); + expect(setWorkflowSteps).not.toHaveBeenCalled(); +}); + +it("preflights a reorder shape with an idle guard before any side effect", async () => { + const work = step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }); + const middle = step("middle", "Middle", 1, false); + const review = step("review", "Review", 2, false, { + autoStart: true, + onTurnComplete: [{ type: "move_to_previous" }], + }); + const { result, setWorkflowSteps } = renderPersistedWorkflowStepActions([work, middle, review]); + + await act(async () => { + await result.current.actions.handleReorderWorkflowSteps([ + work, + { ...review, position: 1 }, + { ...middle, position: 2 }, ]); - - await act(async () => { - await result.current.handleUpdateWorkflowStep("step-2", { is_start_step: true }); - }); - - expect( - getSteps() - .filter((s) => s.is_start_step) - .map((s) => s.id), - ).toEqual(["step-2"]); }); - it("refuses to add, update, remove, or reorder steps when readOnly", async () => { - const initialSteps = [step("step-1", "Todo", 0, true), step("step-2", "Plan", 1, false)]; - const { result, getSteps, refreshWorkflowSteps } = - renderReadOnlyWorkflowStepActions(initialSteps); - - await act(async () => { - await result.current.handleAddWorkflowStep(); - }); - await act(async () => { - await result.current.handleUpdateWorkflowStep("step-2", { name: "Renamed" }); - }); - await act(async () => { - await result.current.handleRemoveWorkflowStep("step-1"); - }); - await act(async () => { - await result.current.handleReorderWorkflowSteps([initialSteps[1], initialSteps[0]]); - }); - - expect(getSteps()).toEqual(initialSteps); - expect(createWorkflowStepAction).not.toHaveBeenCalled(); - expect(updateWorkflowStepAction).not.toHaveBeenCalled(); - expect(deleteWorkflowStepAction).not.toHaveBeenCalled(); - expect(reorderWorkflowStepsAction).not.toHaveBeenCalled(); - expect(refreshWorkflowSteps).not.toHaveBeenCalled(); - }); + expect(result.current.mutationGuard.proposal?.severity).toBe("blocking"); + expect(reorderWorkflowStepsAction).not.toHaveBeenCalled(); + expect(setWorkflowSteps).not.toHaveBeenCalled(); }); -describe("useWorkflowDeleteHandlers", () => { - it("refuses to open the delete-workflow dialog when readOnly", async () => { - const wfDel = { - setDeleteOpen: vi.fn(), - setWorkflowTaskCount: vi.fn(), - setWorkflowDeleteLoading: vi.fn(), - setTargetWorkflowId: vi.fn(), - setTargetWorkflowSteps: vi.fn(), - setTargetStepId: vi.fn(), - targetWorkflowId: "", - targetStepId: "", - setMigrateLoading: vi.fn(), - }; - const { result } = renderHook(() => - useWorkflowDeleteHandlers({ - workflow, - isNewWorkflow: false, - readOnly: true, - otherWorkflows: [], - wfDel, - deleteWorkflowRun: vi.fn(), - toast: vi.fn(), - }), - ); +it("reconciles a successful update from its authoritative response", async () => { + const steps = [step("work", "Work", 0, true)]; + const updated = { ...steps[0], name: "Building" }; + vi.mocked(updateWorkflowStepAction).mockResolvedValue(updated); + const { result, getSteps } = renderPersistedWorkflowStepActions(steps); - await act(async () => { - await result.current.handleDeleteWorkflowClick(); - }); - - expect(getWorkflowTaskCount).not.toHaveBeenCalled(); - expect(wfDel.setDeleteOpen).not.toHaveBeenCalled(); + await act(async () => { + await result.current.actions.handleUpdateWorkflowStep("work", { name: "Building" }); }); + + expect(getSteps()).toEqual([updated]); + expect(listWorkflowStepsAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.isMutationPending).toBe(false); }); -describe("useWorkflowSaveActions", () => { - it("remaps template workflow pull sources between backend-created and added steps", async () => { - const createdWorkflowId = "wf-created" as Workflow["id"]; - const templateName = "Template Step"; - const addedName = "Added Step"; - const backendTemplateId = "backend-template"; - const backendAddedId = "backend-added"; - const draftTemplateId = "draft-template"; +it("appends a successful add from its authoritative response", async () => { + const steps = [step("work", "Work", 0, true)]; + const created = step("created", "Server Step", 1, false); + vi.mocked(createWorkflowStepAction).mockResolvedValue(created); + const { result, getSteps } = renderPersistedWorkflowStepActions(steps); - vi.mocked(createWorkflowAction).mockResolvedValue({ - ...workflow, - id: createdWorkflowId, - workflow_template_id: "template-1", - }); - vi.mocked(listWorkflowStepsAction).mockResolvedValue({ - steps: [ - { - ...step(backendTemplateId, templateName, 0, true), - id: backendTemplateId, - workflow_id: createdWorkflowId, - }, - ], - total: 1, - }); - vi.mocked(createWorkflowStepAction).mockResolvedValue({ - ...step(backendAddedId, addedName, 1, false), - id: backendAddedId, - workflow_id: createdWorkflowId, - }); - vi.mocked(updateWorkflowStepAction).mockResolvedValue( - step(backendAddedId, addedName, 1, false), - ); + await act(async () => { + await result.current.actions.handleAddWorkflowStep(); + }); - const templateStep = { - ...step(draftTemplateId, templateName, 0, true), - pull_from_step_id: "draft-added", - }; - const addedStep = { - ...step("draft-added", addedName, 1, false), - pull_from_step_id: draftTemplateId, - }; - const { result } = renderHook(() => - useWorkflowSaveActions({ - workflow: { ...workflow, workflow_template_id: "template-1" }, - isNewWorkflow: true, - workflowSteps: [templateStep, addedStep], - templateStepCount: 1, - onSaveWorkflow: vi.fn(), - onWorkflowCreated: vi.fn(), - toast: vi.fn(), - }), - ); + expect(getSteps()).toEqual([...steps, created]); + expect(listWorkflowStepsAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.isMutationPending).toBe(false); +}); - await act(async () => { - await result.current.handleSaveWorkflow(); - }); +it("reconciles a successful reorder from its authoritative response", async () => { + const work = step("work", "Work", 0, true); + const review = step("review", "Review", 1, false); + const reordered = [ + { ...review, position: 0 }, + { ...work, position: 1 }, + ]; + vi.mocked(reorderWorkflowStepsAction).mockResolvedValue({ steps: reordered, total: 2 }); + const { result, getSteps } = renderPersistedWorkflowStepActions([work, review]); - expect(createWorkflowStepAction).toHaveBeenCalledWith( - expect.objectContaining({ pull_from_step_id: "" }), - ); - expect(updateWorkflowStepAction).toHaveBeenCalledWith(backendAddedId, { - pull_from_step_id: backendTemplateId, - }); - expect(updateWorkflowStepAction).toHaveBeenCalledWith(backendTemplateId, { - pull_from_step_id: backendAddedId, + await act(async () => { + await result.current.actions.handleReorderWorkflowSteps([review, work]); + }); + + expect(getSteps()).toEqual(reordered); + expect(listWorkflowStepsAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.isMutationPending).toBe(false); +}); + +it("holds the task migration delete path until a warning is confirmed", async () => { + vi.mocked(getStepTaskCount).mockResolvedValue({ task_count: 2 }); + const work = step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }); + const middle = step("middle", "Middle", 1, false); + const review = step("review", "Review", 2, false, { + onTurnComplete: [{ type: "move_to_previous" }], + }); + const { result, setStepDeleteOpen } = renderPersistedWorkflowStepActions([work, middle, review]); + + await act(async () => { + await result.current.actions.handleRemoveWorkflowStep("middle"); + }); + expect(getStepTaskCount).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.proposal?.severity).toBe("warning"); + + await act(async () => { + await result.current.mutationGuard.confirmProposal(); + }); + + expect(getStepTaskCount).toHaveBeenCalledTimes(1); + expect(setStepDeleteOpen).toHaveBeenCalledWith(true); + expect(deleteWorkflowStepAction).not.toHaveBeenCalled(); +}); + +it("adds a step without reconfirming an existing diagnostic", async () => { + const steps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_step", config: { step_id: "work" } }], + }), + ]; + const { result } = renderPersistedWorkflowStepActions(steps); + + await act(async () => { + await result.current.actions.handleAddWorkflowStep(); + }); + + expect(createWorkflowStepAction).toHaveBeenCalledTimes(1); + expect(result.current.mutationGuard.proposal).toBeNull(); +}); + +it("does not create a draft workflow with a blocking cycle", async () => { + const draftSteps = [ + step("build", "Build", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { + autoStart: true, + onTurnComplete: [{ type: "move_to_previous" }], + }), + ]; + const { result } = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(draftSteps); + return useWorkflowSaveActions({ + workflow: { ...workflow, id: "temp-workflow" as Workflow["id"] }, + isNewWorkflow: true, + workflowSteps: draftSteps, + templateStepCount: 0, + onSaveWorkflow: vi.fn(), + toast: vi.fn(), + mutationGuard, }); }); - it("refuses to save changes to an existing workflow when readOnly", async () => { - const onSaveWorkflow = vi.fn().mockResolvedValue(undefined); - const { result } = renderHook(() => - useWorkflowSaveActions({ - workflow, - isNewWorkflow: false, - readOnly: true, - workflowSteps: [], - templateStepCount: 0, - onSaveWorkflow, - toast: vi.fn(), - }), - ); + await act(async () => { + await result.current.handleSaveWorkflow(); + }); - await act(async () => { - await result.current.handleSaveWorkflow(); + expect(createWorkflowAction).not.toHaveBeenCalled(); + expect(result.current.mutationGuard.proposal?.severity).toBe("blocking"); +}); + +it("cancels a warning draft with no requests and creates it once after confirmation", async () => { + const createdWorkflow = { ...workflow, id: "created-workflow" as Workflow["id"] }; + vi.mocked(createWorkflowAction).mockResolvedValue(createdWorkflow); + vi.mocked(createWorkflowStepAction).mockImplementation(async (payload) => ({ + ...step(`created-${payload.position}`, payload.name, payload.position, false), + workflow_id: createdWorkflow.id, + })); + const draftSteps = [ + step("work", "Work", 0, true, { + autoStart: true, + onTurnComplete: [{ type: "move_to_next" }], + }), + step("review", "Review", 1, false, { + onTurnComplete: [{ type: "move_to_previous" }], + }), + ]; + const { result } = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(draftSteps); + return useWorkflowSaveActions({ + workflow: { ...workflow, id: "temp-workflow" as Workflow["id"] }, + isNewWorkflow: true, + workflowSteps: draftSteps, + templateStepCount: 0, + onSaveWorkflow: vi.fn(), + toast: vi.fn(), + mutationGuard, }); + }); - expect(onSaveWorkflow).not.toHaveBeenCalled(); + await act(async () => { + await result.current.handleSaveWorkflow(); + result.current.mutationGuard.cancelProposal(); + }); + expect(createWorkflowAction).not.toHaveBeenCalled(); + expect(createWorkflowStepAction).not.toHaveBeenCalled(); + + await act(async () => { + await result.current.handleSaveWorkflow(); + await result.current.mutationGuard.confirmProposal(); + await result.current.mutationGuard.confirmProposal(); + }); + + expect(createWorkflowAction).toHaveBeenCalledTimes(1); + expect(createWorkflowStepAction).toHaveBeenCalledTimes(2); +}); + +it("remaps template workflow pull sources between backend-created and added steps", async () => { + const createdWorkflowId = "wf-created" as Workflow["id"]; + const templateName = "Template Step"; + const addedName = "Added Step"; + const backendTemplateId = "backend-template"; + const backendAddedId = "backend-added"; + const draftTemplateId = "draft-template"; + + vi.mocked(createWorkflowAction).mockResolvedValue({ + ...workflow, + id: createdWorkflowId, + workflow_template_id: "template-1", + }); + vi.mocked(listWorkflowStepsAction).mockResolvedValue({ + steps: [ + { + ...step(backendTemplateId, templateName, 0, true), + id: backendTemplateId, + workflow_id: createdWorkflowId, + }, + ], + total: 1, + }); + vi.mocked(createWorkflowStepAction).mockResolvedValue({ + ...step(backendAddedId, addedName, 1, false), + id: backendAddedId, + workflow_id: createdWorkflowId, + }); + vi.mocked(updateWorkflowStepAction).mockResolvedValue(step(backendAddedId, addedName, 1, false)); + + const templateStep = { + ...step(draftTemplateId, templateName, 0, true), + pull_from_step_id: "draft-added", + }; + const addedStep = { + ...step("draft-added", addedName, 1, false), + pull_from_step_id: draftTemplateId, + }; + const workflowSteps = [templateStep, addedStep]; + const { result } = renderHook(() => { + const mutationGuard = useWorkflowMutationGuard(workflowSteps); + return useWorkflowSaveActions({ + workflow: { ...workflow, workflow_template_id: "template-1" }, + isNewWorkflow: true, + workflowSteps, + templateStepCount: 1, + onSaveWorkflow: vi.fn(), + onWorkflowCreated: vi.fn(), + toast: vi.fn(), + mutationGuard, + }); + }); + + await act(async () => { + await result.current.handleSaveWorkflow(); + }); + + expect(createWorkflowStepAction).toHaveBeenCalledWith( + expect.objectContaining({ pull_from_step_id: "" }), + ); + expect(updateWorkflowStepAction).toHaveBeenCalledWith(backendAddedId, { + pull_from_step_id: backendTemplateId, + }); + expect(updateWorkflowStepAction).toHaveBeenCalledWith(backendTemplateId, { + pull_from_step_id: backendAddedId, }); }); diff --git a/apps/web/components/settings/workflow-card-actions.ts b/apps/web/components/settings/workflow-card-actions.ts index 9dede74e4c..b6dcccfc10 100644 --- a/apps/web/components/settings/workflow-card-actions.ts +++ b/apps/web/components/settings/workflow-card-actions.ts @@ -4,7 +4,6 @@ import { useEffect } from "react"; import type { Workflow, WorkflowStep } from "@/lib/types/http"; import { useToast } from "@/components/toast-provider"; import { useRequest } from "@/lib/http/use-request"; -import { generateUUID } from "@/lib/utils"; import { createWorkflowAction, createWorkflowStepAction, @@ -17,33 +16,35 @@ import { exportWorkflowAction, bulkMoveTasks, } from "@/app/actions/workspaces"; +import type { WorkflowMutationGuardController } from "./workflow-mutation-guard"; +import { + addLocalStep, + addRemoteStep, + applyWorkflowStepUpdates, + newWorkflowStep, + removeLocalStep, + updateRemoteWorkflowStep, +} from "./workflow-step-mutations"; const FALLBACK_ERROR_MESSAGE = "Request failed"; - type WorkflowStepActionsParams = { workflow: Workflow; isNewWorkflow: boolean; - /** - * Workflows synced from GitHub (`workflow.source === "github"`) are - * read-only in the UI; the backend also rejects step mutations with a 409. - * Gating here is defense-in-depth in case a disabled control is somehow - * still triggered. - */ readOnly?: boolean; workflowSteps: WorkflowStep[]; setWorkflowSteps: (updater: ((prev: WorkflowStep[]) => WorkflowStep[]) | WorkflowStep[]) => void; - refreshWorkflowSteps: () => Promise; setStepToDelete: (id: string | null) => void; setStepTaskCount: (count: number | null) => void; setTargetStepForMigration: (id: string) => void; setStepDeleteOpen: (open: boolean) => void; toast: ReturnType["toast"]; + mutationGuard: WorkflowMutationGuardController; }; type RemoveStepParams = { stepId: string; workflowSteps: WorkflowStep[]; - refreshWorkflowSteps: () => Promise; + setWorkflowSteps: (updater: (prev: WorkflowStep[]) => WorkflowStep[]) => void; setStepToDelete: (id: string | null) => void; setStepTaskCount: (count: number | null) => void; setTargetStepForMigration: (id: string) => void; @@ -54,7 +55,7 @@ type RemoveStepParams = { async function removeWorkflowStep({ stepId, workflowSteps, - refreshWorkflowSteps, + setWorkflowSteps, setStepToDelete, setStepTaskCount, setTargetStepForMigration, @@ -65,7 +66,11 @@ async function removeWorkflowStep({ const { task_count } = await getStepTaskCount(stepId); if (task_count === 0) { await deleteWorkflowStepAction(stepId); - await refreshWorkflowSteps(); + setWorkflowSteps((previous) => + previous + .filter((step) => step.id !== stepId) + .map((step, position) => ({ ...step, position })), + ); return; } setStepToDelete(stepId); @@ -82,73 +87,18 @@ async function removeWorkflowStep({ } } -const NEW_STEP_DEFAULTS = { name: "New Step", color: "bg-slate-500" } as const; - -function addLocalStep( - workflow: Workflow, - setWorkflowSteps: WorkflowStepActionsParams["setWorkflowSteps"], -) { - setWorkflowSteps((prev) => [ - ...prev, - { - id: `temp-step-${generateUUID()}`, - workflow_id: workflow.id, - ...NEW_STEP_DEFAULTS, - position: prev.length, - allow_manual_move: true, - created_at: "", - updated_at: "", - }, - ]); -} - -async function addRemoteStep( - workflow: Workflow, - stepCount: number, - refreshWorkflowSteps: () => Promise, - toast: WorkflowStepActionsParams["toast"], -) { - try { - await createWorkflowStepAction({ - workflow_id: workflow.id, - ...NEW_STEP_DEFAULTS, - position: stepCount, - }); - await refreshWorkflowSteps(); - } catch (error) { - toast({ - title: "Failed to add workflow step", - description: error instanceof Error ? error.message : FALLBACK_ERROR_MESSAGE, - variant: "error", - }); - } -} - -function applyWorkflowStepUpdates( - steps: WorkflowStep[], - stepId: string, - updates: Partial, -): WorkflowStep[] { - const isSettingStartStep = updates.is_start_step === true; - return steps.map((step) => { - if (step.id === stepId) return { ...step, ...updates }; - if (isSettingStartStep) return { ...step, is_start_step: false }; - return step; - }); -} - export function useWorkflowStepActions({ workflow, isNewWorkflow, readOnly = false, workflowSteps, setWorkflowSteps, - refreshWorkflowSteps, setStepToDelete, setStepTaskCount, setTargetStepForMigration, setStepDeleteOpen, toast, + mutationGuard, }: WorkflowStepActionsParams) { const handleUpdateWorkflowStep = async (stepId: string, updates: Partial) => { if (readOnly) return; @@ -156,66 +106,79 @@ export function useWorkflowStepActions({ setWorkflowSteps((prev) => applyWorkflowStepUpdates(prev, stepId, updates)); return; } - try { - await updateWorkflowStepAction(stepId, updates); - await refreshWorkflowSteps(); - } catch (error) { - toast({ - title: "Failed to update workflow step", - description: error instanceof Error ? error.message : FALLBACK_ERROR_MESSAGE, - variant: "error", - }); - } + const proposedSteps = applyWorkflowStepUpdates(workflowSteps, stepId, updates); + await mutationGuard.guardMutation({ + proposedSteps, + operation: () => updateRemoteWorkflowStep({ stepId, updates, setWorkflowSteps, toast }), + }); }; - const handleAddWorkflowStep = async () => { if (readOnly) return; if (isNewWorkflow) { addLocalStep(workflow, setWorkflowSteps); return; } - await addRemoteStep(workflow, workflowSteps.length, refreshWorkflowSteps, toast); + const proposedSteps = [ + ...workflowSteps, + newWorkflowStep(workflow, workflowSteps.length, `proposed-step-${workflow.id}`), + ]; + await mutationGuard.guardMutation({ + proposedSteps, + operation: () => addRemoteStep(workflow, workflowSteps.length, setWorkflowSteps, toast), + }); }; - const handleRemoveWorkflowStep = async (stepId: string) => { if (readOnly) return; if (isNewWorkflow) { - setWorkflowSteps((prev) => - prev.filter((s) => s.id !== stepId).map((s, i) => ({ ...s, position: i })), - ); + removeLocalStep(stepId, setWorkflowSteps); return; } - await removeWorkflowStep({ - stepId, - workflowSteps, - refreshWorkflowSteps, - setStepToDelete, - setStepTaskCount, - setTargetStepForMigration, - setStepDeleteOpen, - toast, + const proposedSteps = workflowSteps + .filter((step) => step.id !== stepId) + .map((step, position) => ({ ...step, position })); + await mutationGuard.guardMutation({ + proposedSteps, + operation: () => + removeWorkflowStep({ + stepId, + workflowSteps, + setWorkflowSteps, + setStepToDelete, + setStepTaskCount, + setTargetStepForMigration, + setStepDeleteOpen, + toast, + }), }); }; - const handleReorderWorkflowSteps = async (reorderedSteps: WorkflowStep[]) => { if (readOnly) return; - setWorkflowSteps(reorderedSteps); - if (isNewWorkflow) return; - try { - await reorderWorkflowStepsAction( - workflow.id, - reorderedSteps.map((s) => s.id), - ); - } catch (error) { - toast({ - title: "Failed to reorder workflow steps", - description: error instanceof Error ? error.message : FALLBACK_ERROR_MESSAGE, - variant: "error", - }); - await refreshWorkflowSteps(); + const proposedSteps = reorderedSteps.map((step, position) => ({ ...step, position })); + if (isNewWorkflow) { + setWorkflowSteps(proposedSteps); + return; } + await mutationGuard.guardMutation({ + proposedSteps, + operation: async () => { + setWorkflowSteps(proposedSteps); + try { + const response = await reorderWorkflowStepsAction( + workflow.id, + proposedSteps.map((step) => step.id), + ); + setWorkflowSteps(response.steps); + } catch (error) { + toast({ + title: "Failed to reorder workflow steps", + description: error instanceof Error ? error.message : FALLBACK_ERROR_MESSAGE, + variant: "error", + }); + setWorkflowSteps(workflowSteps); + } + }, + }); }; - return { handleUpdateWorkflowStep, handleAddWorkflowStep, @@ -227,7 +190,6 @@ export function useWorkflowStepActions({ type WorkflowDeleteHandlersParams = { workflow: Workflow; isNewWorkflow: boolean; - /** See `WorkflowStepActionsParams.readOnly` — defense-in-depth for synced workflows. */ readOnly?: boolean; otherWorkflows: Workflow[]; wfDel: { @@ -303,8 +265,6 @@ export function useWorkflowDeleteHandlers({ }; const handleDeleteWorkflow = async () => { - // A background sync can flip the workflow read-only while the delete - // dialog is already open; re-check at confirm time. if (readOnly) return; try { await deleteWorkflowRun(); @@ -343,7 +303,6 @@ export function useWorkflowDeleteHandlers({ return { handleDeleteWorkflowClick, handleDeleteWorkflow, handleMigrateAndDeleteWorkflow }; } - type StepDeleteHandlersParams = { workflow: Workflow; stepDel: { @@ -353,28 +312,33 @@ type StepDeleteHandlersParams = { setStepDeleteOpen: (v: boolean) => void; setStepToDelete: (v: string | null) => void; }; - refreshWorkflowSteps: () => Promise; + setWorkflowSteps: (updater: (prev: WorkflowStep[]) => WorkflowStep[]) => void; toast: ReturnType["toast"]; }; export function useStepDeleteHandlers({ workflow, stepDel, - refreshWorkflowSteps, + setWorkflowSteps, toast, }: StepDeleteHandlersParams) { const handleMigrateAndDeleteStep = async () => { if (!stepDel.stepToDelete || !stepDel.targetStepForMigration) return; + const stepId = stepDel.stepToDelete; stepDel.setStepMigrateLoading(true); try { await bulkMoveTasks({ source_workflow_id: workflow.id, - source_step_id: stepDel.stepToDelete, + source_step_id: stepId, target_workflow_id: workflow.id, target_step_id: stepDel.targetStepForMigration, }); - await deleteWorkflowStepAction(stepDel.stepToDelete); - await refreshWorkflowSteps(); + await deleteWorkflowStepAction(stepId); + setWorkflowSteps((previous) => + previous + .filter((step) => step.id !== stepId) + .map((step, position) => ({ ...step, position })), + ); stepDel.setStepDeleteOpen(false); stepDel.setStepToDelete(null); } catch (error) { @@ -390,10 +354,15 @@ export function useStepDeleteHandlers({ const handleDeleteStepAndTasks = async () => { if (!stepDel.stepToDelete) return; + const stepId = stepDel.stepToDelete; stepDel.setStepMigrateLoading(true); try { - await deleteWorkflowStepAction(stepDel.stepToDelete); - await refreshWorkflowSteps(); + await deleteWorkflowStepAction(stepId); + setWorkflowSteps((previous) => + previous + .filter((step) => step.id !== stepId) + .map((step, position) => ({ ...step, position })), + ); stepDel.setStepDeleteOpen(false); stepDel.setStepToDelete(null); } catch (error) { @@ -525,13 +494,13 @@ async function reconcileTemplateSteps( type WorkflowSaveActionsParams = { workflow: Workflow; isNewWorkflow: boolean; - /** See `WorkflowStepActionsParams.readOnly` — defense-in-depth for synced workflows. */ readOnly?: boolean; workflowSteps: WorkflowStep[]; templateStepCount: number; onSaveWorkflow: () => Promise; onWorkflowCreated?: (created: Workflow) => void; toast: ReturnType["toast"]; + mutationGuard: WorkflowMutationGuardController; }; export function useWorkflowSaveActions({ @@ -543,6 +512,7 @@ export function useWorkflowSaveActions({ onSaveWorkflow, onWorkflowCreated, toast, + mutationGuard, }: WorkflowSaveActionsParams) { const saveWorkflowRequest = useRequest(onSaveWorkflow); @@ -567,8 +537,7 @@ export function useWorkflowSaveActions({ const activeSaveRequest = isNewWorkflow ? saveNewWorkflowRequest : saveWorkflowRequest; - const handleSaveWorkflow = async () => { - if (readOnly) return; + const runSaveWorkflow = async () => { try { if (isNewWorkflow) await saveNewWorkflowRequest.run(); else await saveWorkflowRequest.run(); @@ -581,7 +550,21 @@ export function useWorkflowSaveActions({ } }; - return { activeSaveRequest, handleSaveWorkflow }; + const handleSaveWorkflow = async () => { + if (readOnly) return; + if (!isNewWorkflow) { + await runSaveWorkflow(); + return; + } + await mutationGuard.guardMutation({ + baselineSteps: [], + proposedSteps: workflowSteps, + intent: "create", + operation: runSaveWorkflow, + }); + }; + + return { activeSaveRequest, handleSaveWorkflow, mutationGuard }; } type WorkflowExportActionsParams = { diff --git a/apps/web/components/settings/workflow-card.tsx b/apps/web/components/settings/workflow-card.tsx index 77217b2ed4..04f37ea80d 100644 --- a/apps/web/components/settings/workflow-card.tsx +++ b/apps/web/components/settings/workflow-card.tsx @@ -3,13 +3,14 @@ import { useState, useEffect } from "react"; import { IconDownload, IconTrash } from "@tabler/icons-react"; import { Card, CardContent } from "@kandev/ui/card"; -import { Badge } from "@kandev/ui/badge"; import { Button } from "@kandev/ui/button"; +import { Badge } from "@kandev/ui/badge"; import { Input } from "@kandev/ui/input"; import { Label } from "@kandev/ui/label"; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@kandev/ui/select"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; import type { Workflow, WorkflowStep } from "@/lib/types/http"; +import type { WorkflowReplayCycleDiagnostic } from "@/lib/workflows/replay-cycle-analysis"; import { useHealthyAgentProfiles } from "@/hooks/domains/settings/use-healthy-agent-profiles"; import { useRequest } from "@/lib/http/use-request"; import { useToast } from "@/components/toast-provider"; @@ -26,6 +27,8 @@ import { useWorkflowSaveActions, handleExportWorkflow, } from "./workflow-card-actions"; +import { useWorkflowMutationGuard } from "./workflow-mutation-guard"; +import { WorkflowCycleGuardDialog } from "./workflow-cycle-diagnostic"; type WorkflowCardProps = { workflow: Workflow; @@ -76,16 +79,7 @@ function useWorkflowSteps( }; }, [workflowId, initialSteps, isNewWorkflow, toast]); - const refreshWorkflowSteps = async () => { - try { - const res = await listWorkflowStepsAction(workflowId); - setWorkflowSteps(res.steps ?? []); - } catch { - /* ignore */ - } - }; - - return { workflowSteps, setWorkflowSteps, workflowLoading, refreshWorkflowSteps }; + return { workflowSteps, setWorkflowSteps, workflowLoading }; } type WorkflowDeleteState = { @@ -176,17 +170,13 @@ type WorkflowCardActionsProps = { }; const SYNCED_READ_ONLY_REASON = - "Managed by workflow sync — edit or remove it in the synced repository"; + "Managed by workflow sync - edit or remove it in the synced repository"; function DeleteWorkflowButton({ onDeleteClick, deleteDisabled, readOnly, -}: { - onDeleteClick: () => Promise; - deleteDisabled: boolean; - readOnly: boolean; -}) { +}: Pick) { const button = (