diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 3f1878be44..63ef87b3d4 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -880,7 +880,7 @@ func registerSecondaryRoutes( planService *taskservice.PlanService, handoffSvc *taskservice.HandoffService, ) { - workflowhandlers.RegisterRoutes(p.router, p.gateway.Dispatcher, workflowCtrl, p.log) + workflowhandlers.RegisterRoutes(p.router, p.gateway.Dispatcher, workflowCtrl, p.eventBus, p.log) p.log.Info("Registered Workflow handlers (HTTP + WebSocket)") agentsettingshandlers.RegisterRoutes(p.router, p.agentSettingsController, p.gateway.Hub, p.log) diff --git a/apps/backend/internal/integration/integration_test.go b/apps/backend/internal/integration/integration_test.go index 08875af0fe..6accdde9b6 100644 --- a/apps/backend/internal/integration/integration_test.go +++ b/apps/backend/internal/integration/integration_test.go @@ -119,7 +119,7 @@ func NewTestServer(t *testing.T) *TestServer { taskhandlers.RegisterRepositoryRoutes(router, gateway.Dispatcher, taskSvc, log) taskhandlers.RegisterExecutorRoutes(router, gateway.Dispatcher, taskSvc, log) taskhandlers.RegisterEnvironmentRoutes(router, gateway.Dispatcher, taskSvc, log) - workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, log) + workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, eventBus, log) // Create test server server := httptest.NewServer(router) diff --git a/apps/backend/internal/integration/test_server_test.go b/apps/backend/internal/integration/test_server_test.go index 771e2ff1a3..9fcf97cfb0 100644 --- a/apps/backend/internal/integration/test_server_test.go +++ b/apps/backend/internal/integration/test_server_test.go @@ -348,7 +348,7 @@ func NewOrchestratorTestServer(t *testing.T) *OrchestratorTestServer { taskhandlers.RegisterRepositoryRoutes(router, gateway.Dispatcher, taskSvc, log) taskhandlers.RegisterExecutorRoutes(router, gateway.Dispatcher, taskSvc, log) taskhandlers.RegisterEnvironmentRoutes(router, gateway.Dispatcher, taskSvc, log) - workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, log) + workflowhandlers.RegisterRoutes(router, gateway.Dispatcher, workflowCtrl, eventBus, log) // Create test server server := httptest.NewServer(router) diff --git a/apps/backend/internal/mcp/handlers/config_handlers_test.go b/apps/backend/internal/mcp/handlers/config_handlers_test.go index 680e5a1940..8471c40cb2 100644 --- a/apps/backend/internal/mcp/handlers/config_handlers_test.go +++ b/apps/backend/internal/mcp/handlers/config_handlers_test.go @@ -12,6 +12,7 @@ import ( "github.com/kandev/kandev/internal/task/dto" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/service" + workflowmodels "github.com/kandev/kandev/internal/workflow/models" v1 "github.com/kandev/kandev/pkg/api/v1" ws "github.com/kandev/kandev/pkg/websocket" "github.com/stretchr/testify/assert" @@ -619,6 +620,188 @@ func TestHandleMoveTask_ActiveSessionWithoutPrompt_DefersMove(t *testing.T) { assert.Equal(t, "step-work", task.WorkflowStepID, "deferred move must not apply immediately") } +// seedRunningTask creates a workspace, workflow, task, and a RUNNING session so +// handleMoveTask takes the deferred path. Returns the task ID. +func seedRunningTask(t *testing.T, repo interface { + CreateWorkspace(context.Context, *models.Workspace) error + CreateWorkflow(context.Context, *models.Workflow) error + CreateTask(context.Context, *models.Task) error + CreateTaskSession(context.Context, *models.TaskSession) error +}, wsID, wfID, taskID, sessionID, currentStepID string) { + t.Helper() + ctx := context.Background() + now := time.Now().UTC() + require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ID: wsID, Name: wsID, CreatedAt: now, UpdatedAt: now})) + require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ID: wfID, WorkspaceID: wsID, Name: wfID, CreatedAt: now, UpdatedAt: now})) + require.NoError(t, repo.CreateTask(ctx, &models.Task{ + ID: taskID, WorkspaceID: wsID, WorkflowID: wfID, WorkflowStepID: currentStepID, + Title: "task", State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, + })) + require.NoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: sessionID, TaskID: taskID, State: models.TaskSessionStateRunning, + IsPrimary: true, StartedAt: now, UpdatedAt: now, + })) +} + +// TestDeferMoveTask_RejectsNonExistentStep ensures deferMoveTask returns a +// validation error when the requested workflow_step_id does not exist in the +// workflow service, preventing a stale step ID from being stored as a pending move. +func TestDeferMoveTask_RejectsNonExistentStep(t *testing.T) { + svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t) + ctx := context.Background() + seedRunningTask(t, repo, "ws-defer1", "wf-defer1", "task-defer1", "sess-defer1", "src-step") + + // Only insert the source step — target "ghost-step" does not exist. + now := time.Now().UTC() + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "src-step", WorkflowID: "wf-defer1", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + + queue := &pendingMoveRecordingQueuer{} + h := &Handlers{ + taskSvc: svc, + workflowCtrl: wfCtrl, + messageQueue: queue, + logger: testLogger(t).WithFields(), + } + + msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{ + "task_id": "task-defer1", + "workflow_id": "wf-defer1", + "workflow_step_id": "ghost-step", // does not exist + "position": 0, + }) + + resp, err := h.handleMoveTask(ctx, msg) + require.NoError(t, err) + assertWSError(t, resp, ws.ErrorCodeValidation) + assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a non-existent step") +} + +// TestDeferMoveTask_RejectsStepFromDifferentWorkflow ensures deferMoveTask +// returns a validation error when the step exists but belongs to another workflow. +func TestDeferMoveTask_RejectsStepFromDifferentWorkflow(t *testing.T) { + svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t) + ctx := context.Background() + seedRunningTask(t, repo, "ws-defer2", "wf-defer2", "task-defer2", "sess-defer2", "src-step2") + + now := time.Now().UTC() + // source step (belongs to the task's workflow) + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "src-step2", WorkflowID: "wf-defer2", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + // A step that belongs to a different workflow — not wf-defer2 + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "other-wf-step", WorkflowID: "wf-other", Name: "Other", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + + queue := &pendingMoveRecordingQueuer{} + h := &Handlers{ + taskSvc: svc, + workflowCtrl: wfCtrl, + messageQueue: queue, + logger: testLogger(t).WithFields(), + } + + msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{ + "task_id": "task-defer2", + "workflow_id": "wf-defer2", + "workflow_step_id": "other-wf-step", // exists but in another workflow + "position": 0, + }) + + resp, err := h.handleMoveTask(ctx, msg) + require.NoError(t, err) + assertWSError(t, resp, ws.ErrorCodeValidation) + assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a step from another workflow") +} + +// TestDeferMoveTask_RejectsWorkspaceMismatch ensures deferMoveTask returns a +// validation error when the target step's workflow exists and matches +// req.WorkflowID, but that workflow lives in a different workspace than the +// task being moved. The deferred path bypasses task.Service.MoveTask (see +// applyPendingMove's doc comment) so it must validate workspace ownership +// independently, mirroring the immediate-apply path's validateTaskMove check. +func TestDeferMoveTask_RejectsWorkspaceMismatch(t *testing.T) { + svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t) + ctx := context.Background() + seedRunningTask(t, repo, "ws-defer4", "wf-defer4", "task-defer4", "sess-defer4", "src-step4") + + now := time.Now().UTC() + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "src-step4", WorkflowID: "wf-defer4", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + + // A separate workspace with its own workflow/step — the step exists and + // req.WorkflowID matches it, but its workspace differs from the task's. + require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ + ID: "ws-defer4-other", Name: "Other", CreatedAt: now, UpdatedAt: now, + })) + require.NoError(t, repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-defer4-other", WorkspaceID: "ws-defer4-other", Name: "Other Board", CreatedAt: now, UpdatedAt: now, + })) + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "dst-step4-other", WorkflowID: "wf-defer4-other", Name: "Dest", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + + queue := &pendingMoveRecordingQueuer{} + h := &Handlers{ + taskSvc: svc, + workflowCtrl: wfCtrl, + messageQueue: queue, + logger: testLogger(t).WithFields(), + } + + msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{ + "task_id": "task-defer4", + "workflow_id": "wf-defer4-other", + "workflow_step_id": "dst-step4-other", + "position": 0, + }) + + resp, err := h.handleMoveTask(ctx, msg) + require.NoError(t, err) + assertWSError(t, resp, ws.ErrorCodeValidation) + assert.Empty(t, queue.pendingMoves, "pending move must not be recorded for a cross-workspace target") +} + +// TestDeferMoveTask_AcceptsValidStep ensures deferMoveTask succeeds when the +// target step exists and belongs to the requested workflow. +func TestDeferMoveTask_AcceptsValidStep(t *testing.T) { + svc, repo, wfCtrl, wfRepo := newTestTaskServiceWithWorkflow(t) + ctx := context.Background() + seedRunningTask(t, repo, "ws-defer3", "wf-defer3", "task-defer3", "sess-defer3", "src-step3") + + now := time.Now().UTC() + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "src-step3", WorkflowID: "wf-defer3", Name: "Source", Position: 0, CreatedAt: now, UpdatedAt: now, + })) + require.NoError(t, wfRepo.CreateStep(ctx, &workflowmodels.WorkflowStep{ + ID: "dst-step3", WorkflowID: "wf-defer3", Name: "Dest", Position: 1, CreatedAt: now, UpdatedAt: now, + })) + + queue := &pendingMoveRecordingQueuer{} + h := &Handlers{ + taskSvc: svc, + workflowCtrl: wfCtrl, + messageQueue: queue, + logger: testLogger(t).WithFields(), + } + + msg := makeWSMessage(t, ws.ActionMCPMoveTask, map[string]interface{}{ + "task_id": "task-defer3", + "workflow_id": "wf-defer3", + "workflow_step_id": "dst-step3", + "position": 0, + }) + + resp, err := h.handleMoveTask(ctx, msg) + require.NoError(t, err) + assert.NotEqual(t, ws.MessageTypeError, resp.Type, "valid deferred move must succeed") + require.Len(t, queue.pendingMoves, 1) + assert.Equal(t, "dst-step3", queue.pendingMoves[0].WorkflowStepID) +} + func TestMoveTaskErrorMessage_SanitizesClassifiedErrors(t *testing.T) { assert.Equal(t, "Move task conflicts with the current task or workflow state", diff --git a/apps/backend/internal/mcp/handlers/config_task_handlers.go b/apps/backend/internal/mcp/handlers/config_task_handlers.go index fd5290a1c3..d8bc6ee499 100644 --- a/apps/backend/internal/mcp/handlers/config_task_handlers.go +++ b/apps/backend/internal/mcp/handlers/config_task_handlers.go @@ -89,6 +89,65 @@ func (h *Handlers) deferMoveTask( return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "move_task requires message queue support while the source session is active", nil) } + + // Validate the target step exists and belongs to the requested workflow + // before committing the deferred move. Without this check a stale or + // foreign step_id would be stored and silently fail at turn-end, leaving + // the task orphaned on the board. + if h.workflowCtrl != nil { + stepResp, err := h.workflowCtrl.GetStep(ctx, req.WorkflowStepID) + if err != nil || stepResp == nil || stepResp.Step == nil { + h.logger.Error("move_task: target step not found", + zap.String("task_id", req.TaskID), + zap.String("workflow_step_id", req.WorkflowStepID), + zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, + "target workflow_step_id does not exist", nil) + } + if stepResp.Step.WorkflowID != req.WorkflowID { + h.logger.Error("move_task: target step belongs to a different workflow", + zap.String("task_id", req.TaskID), + zap.String("workflow_step_id", req.WorkflowStepID), + zap.String("step_workflow_id", stepResp.Step.WorkflowID), + zap.String("requested_workflow_id", req.WorkflowID)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, + "target workflow_step_id does not belong to the requested workflow_id", nil) + } + } + + // Validate the target workflow lives in the same workspace as the task. + // The immediate-apply path (task.Service.MoveTask) already enforces this + // via validateTaskMove; the deferred path bypasses that service entirely + // (see applyPendingMove's doc comment), so it must check independently or + // a cross-workspace move_task_kandev call would silently succeed. + if h.taskSvc != nil { + task, err := h.taskSvc.GetTask(ctx, req.TaskID) + if err != nil { + h.logger.Error("move_task: failed to load task for workspace validation", + zap.String("task_id", req.TaskID), zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, + "failed to load task for move validation", nil) + } + targetWorkflow, err := h.taskSvc.GetWorkflow(ctx, req.WorkflowID) + if err != nil { + h.logger.Error("move_task: target workflow not found", + zap.String("task_id", req.TaskID), + zap.String("workflow_id", req.WorkflowID), + zap.Error(err)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, + "target workflow_id does not exist", nil) + } + if targetWorkflow.WorkspaceID != task.WorkspaceID { + h.logger.Error("move_task: target workflow is in a different workspace", + zap.String("task_id", req.TaskID), + zap.String("workflow_id", req.WorkflowID), + zap.String("task_workspace_id", task.WorkspaceID), + zap.String("target_workspace_id", targetWorkflow.WorkspaceID)) + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, + "target workflow is in a different workspace", nil) + } + } + if req.Prompt != "" { wrapped := "You were moved to this step with the following message: " + req.Prompt if err := h.queueMoveTaskPrompt(ctx, req.TaskID, session.ID, wrapped); err != nil { diff --git a/apps/backend/internal/orchestrator/event_handlers_pending_move_test.go b/apps/backend/internal/orchestrator/event_handlers_pending_move_test.go index 51cc6770f3..92aaae2719 100644 --- a/apps/backend/internal/orchestrator/event_handlers_pending_move_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_pending_move_test.go @@ -117,6 +117,50 @@ func TestPendingMove_OutOfTerminalStepReopensCompletedTask(t *testing.T) { } } +func TestPendingMove_DropsForeignWorkflowStepWithoutMovingTask(t *testing.T) { + sc := buildPendingMoveScenario(t) + sc.stepGetter.steps["foreign-step"] = &wfmodels.WorkflowStep{ + ID: "foreign-step", + WorkflowID: "wf-other", + Name: "Foreign", + Position: 1, + } + + session, err := sc.repo.GetTaskSession(sc.ctx, sc.reviewSessionID) + if err != nil { + t.Fatalf("load review session: %v", err) + } + sc.svc.applyPendingMove(sc.ctx, "task-1", sc.reviewSessionID, session, &messagequeue.PendingMove{ + TaskID: "task-1", + WorkflowID: "wf1", + WorkflowStepID: "foreign-step", + }) + + task, err := sc.repo.GetTask(sc.ctx, "task-1") + if err != nil { + t.Fatalf("load task: %v", err) + } + if task.WorkflowStepID != stepInReviewID { + t.Fatalf("workflow_step_id = %q, want unchanged %q", task.WorkflowStepID, stepInReviewID) + } + session, err = sc.repo.GetTaskSession(sc.ctx, sc.reviewSessionID) + if err != nil { + t.Fatalf("reload review session: %v", err) + } + if session.State != models.TaskSessionStateRunning { + t.Fatalf("session state = %q, want unchanged RUNNING", session.State) + } + + // Regression: the workflow-mismatch drop must clean up any hand-off prompt + // queued by handleMoveTask before the deferred move was applied. Without + // this cleanup, the queued prompt (authored for the foreign-workflow + // target step) would still be sitting in the queue and could be + // misdelivered to the review session's agent on a future turn. + if status := sc.svc.messageQueue.GetStatus(sc.ctx, sc.reviewSessionID); status.Count != 0 { + t.Fatalf("queued message count = %d, want 0 after workflow-mismatch drop", status.Count) + } +} + // --- Pending-move scenario builder & assertions --- const ( diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index ce19e9dec6..21b8e222b9 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -1067,6 +1067,29 @@ func (s *Service) applyPendingMove(ctx context.Context, taskID, sessionID string reinsertPendingMove() return } + if targetStep.WorkflowID != move.WorkflowID { + s.logger.Error("pending move target step belongs to a different workflow; dropping move", + zap.String("task_id", taskID), + zap.String("target_step_id", move.WorkflowStepID), + zap.String("step_workflow_id", targetStep.WorkflowID), + zap.String("move_workflow_id", move.WorkflowID)) + // Do NOT reinsert: the move is invalid and retrying would keep failing. + // The hand-off prompt was queued by handleMoveTask before the move was + // applied. Since the move is being dropped, the on_enter path that would + // have drained the queue won't run. Drop the orphan so it can't be + // misdelivered to the source step's agent on a future turn (it was + // authored for the move's *target* step) — mirrors the cleanup done on + // the ApplyTransition failure path below. + if s.messageQueue != nil { + if _, ok := s.messageQueue.TakeQueued(ctx, sessionID); ok { + s.publishQueueStatusEvent(ctx, sessionID) + s.logger.Warn("dropped hand-off prompt after pending-move workflow mismatch", + zap.String("task_id", taskID), + zap.String("session_id", sessionID)) + } + } + return + } // Mark the session WAITING_FOR_INPUT before processOnEnter runs. The agent // just finished its turn; the active-turn guard in processOnEnter would diff --git a/apps/backend/internal/orchestrator/service.go b/apps/backend/internal/orchestrator/service.go index 179409f314..634b3afd7b 100644 --- a/apps/backend/internal/orchestrator/service.go +++ b/apps/backend/internal/orchestrator/service.go @@ -109,7 +109,7 @@ type TurnService interface { // metadata, parent_id). The orchestrator does not construct task.updated // payloads itself. type TaskEventPublisher interface { - PublishTaskUpdated(ctx context.Context, task *models.Task) + PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) PublishTaskStateChanged(ctx context.Context, task *models.Task, oldState v1.TaskState) } @@ -690,12 +690,14 @@ func (s *Service) SetTaskEventPublisher(publisher TaskEventPublisher) { // publishTaskUpdated forwards to the configured TaskEventPublisher. // No-op when the publisher isn't wired (tests, or before SetTaskEventPublisher -// has been called during startup). -func (s *Service) publishTaskUpdated(ctx context.Context, task *models.Task) { +// has been called during startup). Pass the pre-move workflow ID when the +// task's workflow changed (e.g. a cross-workflow transition) so the event +// carries old_workflow_id. +func (s *Service) publishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) { if s.taskEvents == nil || task == nil { return } - s.taskEvents.PublishTaskUpdated(ctx, task) + s.taskEvents.PublishTaskUpdated(ctx, task, oldWorkflowIDs...) } func (s *Service) publishTaskStateChanged(ctx context.Context, task *models.Task, oldState v1.TaskState) { diff --git a/apps/backend/internal/orchestrator/workflow_store.go b/apps/backend/internal/orchestrator/workflow_store.go index 3d8826418a..35b3512c9f 100644 --- a/apps/backend/internal/orchestrator/workflow_store.go +++ b/apps/backend/internal/orchestrator/workflow_store.go @@ -19,7 +19,7 @@ import ( // taskUpdatedPublisher is the minimal hook the workflow store needs to emit // task.updated events. The orchestrator Service binds this to its shared // publishTaskUpdated helper so the publisher wiring stays in one place. -type taskUpdatedPublisher func(ctx context.Context, task *models.Task) +type taskUpdatedPublisher func(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) type taskMovedPublisher func(ctx context.Context, task *models.Task, fromWorkflowID, fromStepID, toStepID, sessionID string) @@ -131,13 +131,26 @@ func (s *workflowStore) ApplyTransition(ctx context.Context, taskID, sessionID, return err } + // Keep WorkflowID in sync with the target step's owning workflow. Most + // callers transition within the same workflow (targetStep.WorkflowID == + // task.WorkflowID already), but applyPendingMove uses this path for + // cross-workflow move_task_kandev hand-offs too — without this, the task + // would end up with a step ID from a workflow its WorkflowID doesn't match. + oldWorkflowID := task.WorkflowID + if targetStep != nil { + task.WorkflowID = targetStep.WorkflowID + } task.WorkflowStepID = toStepID task.UpdatedAt = time.Now().UTC() if err := s.updateTransitionTask(ctx, task, targetStep); err != nil { return fmt.Errorf("update task workflow step: %w", err) } - s.publishTaskUpdated(ctx, task) + // Pass the pre-move workflow ID through so cross-workflow transitions + // carry old_workflow_id on the task.updated payload — the frontend uses + // that field to remove the task from its previous workflow's snapshot + // instead of leaving a stale duplicate until reload. + s.publishTaskUpdated(ctx, task, oldWorkflowID) if err := s.repo.UpdateSessionReviewStatus(ctx, sessionID, ""); err != nil { s.logger.Warn("failed to clear session review status", diff --git a/apps/backend/internal/orchestrator/workflow_store_test.go b/apps/backend/internal/orchestrator/workflow_store_test.go index bd88230c63..e48e525b47 100644 --- a/apps/backend/internal/orchestrator/workflow_store_test.go +++ b/apps/backend/internal/orchestrator/workflow_store_test.go @@ -11,7 +11,23 @@ import ( // noopPublisher satisfies the taskUpdatedPublisher contract without touching // an event bus. Workflow-store unit tests don't exercise the event path. -func noopPublisher(_ context.Context, _ *models.Task) {} +func noopPublisher(_ context.Context, _ *models.Task, _ ...string) {} + +// capturingPublisher records each publishTaskUpdated call's task and +// old-workflow-ID argument so tests can assert on what ApplyTransition +// passed through. +type capturingPublisher struct { + calls []capturedTaskUpdate +} + +type capturedTaskUpdate struct { + task *models.Task + oldWorkflowIDs []string +} + +func (c *capturingPublisher) publish(_ context.Context, task *models.Task, oldWorkflowIDs ...string) { + c.calls = append(c.calls, capturedTaskUpdate{task: task, oldWorkflowIDs: oldWorkflowIDs}) +} func TestWorkflowStore_LoadState(t *testing.T) { ctx := context.Background() @@ -143,6 +159,86 @@ func TestWorkflowStore_ApplyTransition(t *testing.T) { } } +func TestWorkflowStore_ApplyTransitionSyncsWorkflowIDAcrossWorkflows(t *testing.T) { + // Regression test: applyPendingMove (deferred cross-workflow move_task_kandev + // hand-off for tasks with an active/starting session) calls ApplyTransition + // directly, bypassing task.Service.MoveTask. Without syncing WorkflowID here, + // a cross-workflow move would leave the task pointing at a step ID that + // belongs to a different workflow than task.WorkflowID records. + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") // seedSession puts the task in wf1 + + // Create a second workflow the task is moving into. + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf2", WorkspaceID: "ws1", Name: "Other Workflow", + }); err != nil { + t.Fatalf("CreateWorkflow wf2: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step-wf2"] = &wfmodels.WorkflowStep{ + ID: "step-wf2", WorkflowID: "wf2", Name: "Target", Position: 0, + } + store := newWorkflowStore(repo, stepGetter, nil, noopPublisher, testLogger()) + + if err := store.ApplyTransition(ctx, "t1", "s1", "step1", "step-wf2", "manual_move"); err != nil { + t.Fatalf("ApplyTransition: %v", err) + } + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("failed to get task: %v", err) + } + if task.WorkflowStepID != "step-wf2" { + t.Errorf("expected task WorkflowStepID %q, got %q", "step-wf2", task.WorkflowStepID) + } + if task.WorkflowID != "wf2" { + t.Errorf("expected task WorkflowID to sync to the target step's workflow %q, got %q", + "wf2", task.WorkflowID) + } +} + +func TestWorkflowStore_ApplyTransitionPublishesOldWorkflowIDOnCrossWorkflowMove(t *testing.T) { + // Regression test for PR review feedback: ApplyTransition syncs + // task.WorkflowID to the target step's workflow, but the task.updated + // event must also carry the pre-move workflow ID (old_workflow_id) — + // same as the normal task.Service.MoveTask path — so the frontend can + // remove the task from its previous workflow's snapshot instead of + // leaving a stale duplicate until reload. + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") // seedSession puts the task in wf1 + + if err := repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf2", WorkspaceID: "ws1", Name: "Other Workflow", + }); err != nil { + t.Fatalf("CreateWorkflow wf2: %v", err) + } + + stepGetter := newMockStepGetter() + stepGetter.steps["step-wf2"] = &wfmodels.WorkflowStep{ + ID: "step-wf2", WorkflowID: "wf2", Name: "Target", Position: 0, + } + pub := &capturingPublisher{} + store := newWorkflowStore(repo, stepGetter, nil, pub.publish, testLogger()) + + if err := store.ApplyTransition(ctx, "t1", "s1", "step1", "step-wf2", "manual_move"); err != nil { + t.Fatalf("ApplyTransition: %v", err) + } + + if len(pub.calls) != 1 { + t.Fatalf("expected exactly 1 publishTaskUpdated call, got %d", len(pub.calls)) + } + call := pub.calls[0] + if call.task.WorkflowID != "wf2" { + t.Errorf("expected published task WorkflowID %q, got %q", "wf2", call.task.WorkflowID) + } + if len(call.oldWorkflowIDs) != 1 || call.oldWorkflowIDs[0] != "wf1" { + t.Errorf("expected old_workflow_id %q, got %v", "wf1", call.oldWorkflowIDs) + } +} + func TestWorkflowStore_ApplyTransitionRejectsFullWIPLimitedTarget(t *testing.T) { ctx := context.Background() repo := setupTestRepo(t) diff --git a/apps/backend/internal/task/repository/sqlite/base_migrations.go b/apps/backend/internal/task/repository/sqlite/base_migrations.go index a02e91f8dd..f1da8a94a0 100644 --- a/apps/backend/internal/task/repository/sqlite/base_migrations.go +++ b/apps/backend/internal/task/repository/sqlite/base_migrations.go @@ -981,3 +981,13 @@ func (r *Repository) backfillTaskEnvironmentRepos() error { } return tx.Commit() } + +// Startup healing of orphaned workflow_step_id values was removed: a raw SQL +// UPDATE reassigning tasks to a workflow's start step bypasses every +// domain-level invariant the task service enforces on a move (WIP limits, +// task-state sync, position bookkeeping, session/on_exit/on_enter handling, +// transition history, and event publication). The Kanban/Pipeline "Needs +// Reassignment" fallback column now keeps orphaned tasks visible without any +// automatic mutation, and any real repair should go through +// task.Service.MoveTask (or a dedicated, explicit reassignment operation) +// rather than a migration-time SQL statement. diff --git a/apps/backend/internal/task/service/handoff_cascade_test.go b/apps/backend/internal/task/service/handoff_cascade_test.go index 0af6e6f9bf..8717b5bc46 100644 --- a/apps/backend/internal/task/service/handoff_cascade_test.go +++ b/apps/backend/internal/task/service/handoff_cascade_test.go @@ -618,7 +618,7 @@ type fakeEventPublisher struct { archived []bool // archivedAt nil/non-nil per updated entry } -func (f *fakeEventPublisher) PublishTaskUpdated(_ context.Context, task *models.Task) { +func (f *fakeEventPublisher) PublishTaskUpdated(_ context.Context, task *models.Task, _ ...string) { f.mu.Lock() defer f.mu.Unlock() f.updated = append(f.updated, task.ID) diff --git a/apps/backend/internal/task/service/handoff_service.go b/apps/backend/internal/task/service/handoff_service.go index 3f8a9a2658..5aa9a6546f 100644 --- a/apps/backend/internal/task/service/handoff_service.go +++ b/apps/backend/internal/task/service/handoff_service.go @@ -198,7 +198,7 @@ type HandoffService struct { // Optional — when nil the cascade still completes, it just won't emit // WS events (matches the pre-handoff fallback in the HTTP handler). type TaskEventPublisher interface { - PublishTaskUpdated(ctx context.Context, task *models.Task) + PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) PublishTaskDeleted(ctx context.Context, task *models.Task) } diff --git a/apps/backend/internal/task/service/service_events.go b/apps/backend/internal/task/service/service_events.go index abbcdc8ed3..4c88150889 100644 --- a/apps/backend/internal/task/service/service_events.go +++ b/apps/backend/internal/task/service/service_events.go @@ -16,9 +16,13 @@ import ( // PublishTaskUpdated publishes a task.updated event for the given task. // Used when task metadata changes (e.g., primary session assignment) that -// don't go through the normal UpdateTask path. -func (s *Service) PublishTaskUpdated(ctx context.Context, task *models.Task) { - s.publishTaskEvent(ctx, events.TaskUpdated, task, nil) +// don't go through the normal UpdateTask path. Callers that changed the +// task's workflow (e.g. a cross-workflow transition) should pass the +// pre-move workflow ID so the payload carries old_workflow_id — the +// frontend uses that field to remove the task from its previous +// workflow's snapshot instead of leaving a stale duplicate until reload. +func (s *Service) PublishTaskUpdated(ctx context.Context, task *models.Task, oldWorkflowIDs ...string) { + s.publishTaskEvent(ctx, events.TaskUpdated, task, nil, oldWorkflowIDs...) } // PublishTaskStateChanged publishes a task.state_changed event for callers diff --git a/apps/backend/internal/workflow/handlers/handlers.go b/apps/backend/internal/workflow/handlers/handlers.go index d9ace2fb7c..d918a547d0 100644 --- a/apps/backend/internal/workflow/handlers/handlers.go +++ b/apps/backend/internal/workflow/handlers/handlers.go @@ -12,6 +12,8 @@ import ( "gopkg.in/yaml.v3" "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/workflow/controller" "github.com/kandev/kandev/internal/workflow/models" "github.com/kandev/kandev/internal/workflow/service" @@ -21,20 +23,22 @@ import ( // Handlers manages workflow HTTP and WebSocket handlers type Handlers struct { controller *controller.Controller + eventBus bus.EventBus logger *logger.Logger } // NewHandlers creates new workflow handlers -func NewHandlers(ctrl *controller.Controller, log *logger.Logger) *Handlers { +func NewHandlers(ctrl *controller.Controller, eventBus bus.EventBus, log *logger.Logger) *Handlers { return &Handlers{ controller: ctrl, + eventBus: eventBus, logger: log.WithFields(zap.String("component", "workflow-handlers")), } } // RegisterRoutes registers workflow HTTP and WebSocket handlers -func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, ctrl *controller.Controller, log *logger.Logger) { - handlers := NewHandlers(ctrl, log) +func RegisterRoutes(router *gin.Engine, dispatcher *ws.Dispatcher, ctrl *controller.Controller, eventBus bus.EventBus, log *logger.Logger) { + handlers := NewHandlers(ctrl, eventBus, log) handlers.registerHTTP(router) handlers.registerWS(dispatcher) } @@ -210,14 +214,59 @@ func isStepValidationError(msg string) bool { } func (h *Handlers) httpDeleteStep(c *gin.Context) { - if err := h.controller.DeleteStep(c.Request.Context(), c.Param("id")); err != nil { + ctx := c.Request.Context() + stepID := c.Param("id") + stepResp, getErr := h.controller.GetStep(ctx, stepID) + if getErr != nil { + h.logger.Warn("failed to fetch step before delete; workflow_step.deleted event will not be published", + zap.String("step_id", stepID), zap.Error(getErr)) + } + + if err := h.controller.DeleteStep(ctx, stepID); err != nil { h.logger.Error("failed to delete step", zap.Error(err)) h.writeStepMutationError(c, err) return } + if stepResp != nil { + h.publishWorkflowStepEvent(ctx, events.WorkflowStepDeleted, stepResp.Step) + } c.JSON(http.StatusOK, gin.H{"success": true}) } +func (h *Handlers) publishWorkflowStepEvent(ctx context.Context, eventType string, step *models.WorkflowStep) { + if step == nil { + return + } + data := map[string]interface{}{ + "step": map[string]interface{}{ + "id": step.ID, + "workflow_id": step.WorkflowID, + "name": step.Name, + "position": step.Position, + "color": step.Color, + "prompt": step.Prompt, + "events": step.Events, + "show_in_command_panel": step.ShowInCommandPanel, + "allow_manual_move": step.AllowManualMove, + "is_start_step": step.IsStartStep, + "auto_archive_after_hours": step.AutoArchiveAfterHours, + "wip_limit": step.WIPLimit, + "pull_from_step_id": step.PullFromStepID, + "agent_profile_id": step.AgentProfileID, + "stage_type": string(step.StageType), + "auto_advance_requires_signal": step.AutoAdvanceRequiresSignal, + "created_at": step.CreatedAt, + "updated_at": step.UpdatedAt, + }, + } + if err := h.eventBus.Publish(ctx, eventType, bus.NewEvent(eventType, "workflow-handlers", data)); err != nil { + h.logger.Error("failed to publish workflow step event", + zap.String("event_type", eventType), + zap.String("step_id", step.ID), + zap.Error(err)) + } +} + type httpReorderStepsRequest struct { StepIDs []string `json:"step_ids"` } diff --git a/apps/backend/internal/workflow/handlers/handlers_test.go b/apps/backend/internal/workflow/handlers/handlers_test.go new file mode 100644 index 0000000000..31cde47e66 --- /dev/null +++ b/apps/backend/internal/workflow/handlers/handlers_test.go @@ -0,0 +1,72 @@ +package handlers + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/events" + "github.com/kandev/kandev/internal/events/bus" + "github.com/kandev/kandev/internal/workflow/models" +) + +type recordingWorkflowEventBus struct { + subject string + event *bus.Event +} + +func (b *recordingWorkflowEventBus) Publish(_ context.Context, subject string, event *bus.Event) error { + b.subject = subject + b.event = event + return nil +} + +func (b *recordingWorkflowEventBus) Subscribe(string, bus.EventHandler) (bus.Subscription, error) { + return nil, nil +} + +func (b *recordingWorkflowEventBus) QueueSubscribe(string, string, bus.EventHandler) (bus.Subscription, error) { + return nil, nil +} + +func (b *recordingWorkflowEventBus) Request(context.Context, string, *bus.Event, time.Duration) (*bus.Event, error) { + return nil, nil +} + +func (b *recordingWorkflowEventBus) Close() {} + +func (b *recordingWorkflowEventBus) IsConnected() bool { return true } + +func TestPublishWorkflowStepEventPublishesDeletedStepPayload(t *testing.T) { + eventBus := &recordingWorkflowEventBus{} + h := NewHandlers(nil, eventBus, logger.Default()) + + h.publishWorkflowStepEvent(context.Background(), events.WorkflowStepDeleted, &models.WorkflowStep{ + ID: "step-deleted", + WorkflowID: "workflow-1", + Name: "Deleted", + Position: 2, + }) + + if eventBus.subject != events.WorkflowStepDeleted { + t.Fatalf("subject = %q, want %q", eventBus.subject, events.WorkflowStepDeleted) + } + if eventBus.event == nil { + t.Fatal("expected event to be published") + } + if eventBus.event.Source != "workflow-handlers" { + t.Fatalf("source = %q, want workflow-handlers", eventBus.event.Source) + } + data, ok := eventBus.event.Data.(map[string]interface{}) + if !ok { + t.Fatalf("event data type = %T, want map", eventBus.event.Data) + } + step, ok := data["step"].(map[string]interface{}) + if !ok { + t.Fatalf("step data type = %T, want map", data["step"]) + } + if step["id"] != "step-deleted" || step["workflow_id"] != "workflow-1" { + t.Fatalf("unexpected step payload: %#v", step) + } +} diff --git a/apps/backend/internal/workflow/service/service.go b/apps/backend/internal/workflow/service/service.go index 50a94bdd4b..25f27e1da2 100644 --- a/apps/backend/internal/workflow/service/service.go +++ b/apps/backend/internal/workflow/service/service.go @@ -311,6 +311,13 @@ func (s *Service) UpdateStepWithStartStepUpdates(ctx context.Context, step *mode } // DeleteStep deletes a workflow step and clears any references to it from other steps. +// Tasks currently on the step are NOT auto-reassigned: doing so through the normal +// move path would run on_exit/on_enter (auto-start agents, session hand-offs) as a +// side effect of an administrative delete, and any failed move would be silently +// ignored while the step is deleted anyway (a partial, non-atomic cascade). Instead, +// affected tasks keep their now-dangling workflow_step_id and are surfaced via the +// Kanban/Pipeline "Needs Reassignment" fallback column for explicit manual +// reassignment by a user (which goes through the normal, fully-validated move path). func (s *Service) DeleteStep(ctx context.Context, stepID string) error { // First, get the step to find its workflow ID step, err := s.repo.GetStep(ctx, stepID) diff --git a/apps/backend/internal/workflow/service/service_test.go b/apps/backend/internal/workflow/service/service_test.go index 195b2b933e..7326294db1 100644 --- a/apps/backend/internal/workflow/service/service_test.go +++ b/apps/backend/internal/workflow/service/service_test.go @@ -881,3 +881,99 @@ func TestImportWorkflows(t *testing.T) { assert.Empty(t, steps[0].AgentProfileID, "no matcher means no profile ID set") }) } + +// --- DeleteStep tests --- +// +// DeleteStep intentionally does NOT auto-reassign tasks off the deleted step: doing +// so through the normal move path would trigger on_exit/on_enter side effects +// (auto-start agents, session hand-offs) as a byproduct of an administrative delete, +// and any failed move would silently leave the step deleted anyway (a partial, +// non-atomic cascade). Affected tasks keep their dangling workflow_step_id and are +// surfaced via the Kanban/Pipeline "Needs Reassignment" fallback for explicit manual +// reassignment through the normal, fully-validated move path. + +func TestDeleteStep_RemovesStepWithoutReassigningTasks(t *testing.T) { + svc, db := setupTestService(t) + ctx := context.Background() + insertWorkflow(t, db, "wf-1", "Test") + + createStep(t, svc, &models.WorkflowStep{ID: "start", WorkflowID: "wf-1", Name: "Start", Position: 0, IsStartStep: true}) + createStep(t, svc, &models.WorkflowStep{ID: "mid", WorkflowID: "wf-1", Name: "Mid", Position: 1}) + createStep(t, svc, &models.WorkflowStep{ID: "end", WorkflowID: "wf-1", Name: "End", Position: 2}) + + require.NoError(t, svc.DeleteStep(ctx, "mid")) + + // Deleted step must not exist anymore. + steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-1") + require.NoError(t, err) + require.Len(t, steps, 2) + for _, s := range steps { + assert.NotEqual(t, "mid", s.ID, "deleted step must be removed") + } +} + +func TestDeleteStep_SucceedsWhenDeletingOnlyStep(t *testing.T) { + svc, db := setupTestService(t) + ctx := context.Background() + insertWorkflow(t, db, "wf-3", "Test") + + createStep(t, svc, &models.WorkflowStep{ID: "only", WorkflowID: "wf-3", Name: "Only", Position: 0}) + + require.NoError(t, svc.DeleteStep(ctx, "only")) + + steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-3") + require.NoError(t, err) + assert.Empty(t, steps) +} + +// TestDeleteStep_WIPLimitedAutoStartTargetIsUnaffected is a regression test +// requested during maintainer review: deleting a step must not run tasks +// through the normal move path (which could trip a neighboring step's WIP +// limit or trigger its auto-start-agent on_enter action as a side effect of +// an administrative delete). Since DeleteStep no longer reassigns tasks at +// all, this asserts the delete succeeds cleanly and is fully independent of +// what limits/automations are configured on the surviving steps. +func TestDeleteStep_WIPLimitedAutoStartTargetIsUnaffected(t *testing.T) { + svc, db := setupTestService(t) + ctx := context.Background() + insertWorkflow(t, db, "wf-5", "Test") + + createStep(t, svc, &models.WorkflowStep{ID: "start", WorkflowID: "wf-5", Name: "Start", Position: 0, IsStartStep: true}) + createStep(t, svc, &models.WorkflowStep{ID: "mid", WorkflowID: "wf-5", Name: "Mid", Position: 1}) + createStep(t, svc, &models.WorkflowStep{ + ID: "auto-start-limited", WorkflowID: "wf-5", Name: "Auto Start", Position: 2, + WIPLimit: 1, + Events: models.StepEvents{ + OnEnter: []models.OnEnterAction{{Type: models.OnEnterAutoStartAgent}}, + }, + }) + + require.NoError(t, svc.DeleteStep(ctx, "mid")) + + steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-5") + require.NoError(t, err) + require.Len(t, steps, 2) + for _, s := range steps { + assert.NotEqual(t, "mid", s.ID, "deleted step must be removed") + if s.ID == "auto-start-limited" { + // The WIP-limited/auto-start step itself must remain untouched. + assert.Equal(t, 1, s.WIPLimit) + } + } +} + +func TestDeleteStep_ClearsStepReferences(t *testing.T) { + svc, db := setupTestService(t) + ctx := context.Background() + insertWorkflow(t, db, "wf-4", "Test") + + createStep(t, svc, &models.WorkflowStep{ID: "s1", WorkflowID: "wf-4", Name: "S1", Position: 0}) + createStep(t, svc, &models.WorkflowStep{ID: "s2", WorkflowID: "wf-4", Name: "S2", Position: 1}) + + require.NoError(t, svc.DeleteStep(ctx, "s1")) + + steps, err := svc.repo.ListStepsByWorkflow(ctx, "wf-4") + require.NoError(t, err) + require.Len(t, steps, 1) + assert.Equal(t, "s2", steps[0].ID) +} diff --git a/apps/web/components/kanban-board-grid.tsx b/apps/web/components/kanban-board-grid.tsx index 1af8723399..91aefea7d4 100644 --- a/apps/web/components/kanban-board-grid.tsx +++ b/apps/web/components/kanban-board-grid.tsx @@ -123,6 +123,7 @@ function MobileLayout({ /> 0 && !isOrphanMoveTarget(steps[index - 1].id); + const hasNext = index < steps.length - 1 && !isOrphanMoveTarget(steps[index + 1].id); + return { + hasPrev, + prevStepId: hasPrev ? steps[index - 1].id : undefined, + hasNext, + nextStepId: hasNext ? steps[index + 1].id : undefined, + }; +} + function PipelineStepNodes({ steps, currentStepIndex, @@ -92,16 +118,18 @@ function PipelineStepNodes({ ? getConnectorType(phase, getStepPhase(index + 1, currentStepIndex)) : null; + const { hasPrev, prevStepId, hasNext, nextStepId } = getStepAdjacency(steps, index); + return (
0} - hasNext={index < steps.length - 1} - prevStepId={index > 0 ? steps[index - 1].id : undefined} - nextStepId={index < steps.length - 1 ? steps[index + 1].id : undefined} + hasPrev={hasPrev} + hasNext={hasNext} + prevStepId={prevStepId} + nextStepId={nextStepId} onMoveTask={onMoveTask} onPreviewTask={onPreviewTask} isMoving={isMoving} diff --git a/apps/web/components/kanban/orphan-move-target.test.ts b/apps/web/components/kanban/orphan-move-target.test.ts new file mode 100644 index 0000000000..4d0234a32b --- /dev/null +++ b/apps/web/components/kanban/orphan-move-target.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import { ORPHAN_STEP_ID, isOrphanMoveTarget } from "./swimlane-kanban-content"; +import { getStepAdjacency } from "./graph2-task-pipeline"; +import type { WorkflowStep } from "@/components/kanban-column"; + +describe("isOrphanMoveTarget", () => { + it("identifies the synthetic Needs Reassignment sentinel", () => { + expect(isOrphanMoveTarget(ORPHAN_STEP_ID)).toBe(true); + }); + + it("treats real step ids as valid move targets", () => { + expect(isOrphanMoveTarget("todo")).toBe(false); + expect(isOrphanMoveTarget("")).toBe(false); + }); +}); + +describe("getStepAdjacency", () => { + const realSteps: WorkflowStep[] = [ + { id: "todo", title: "Todo", color: "#64748b" }, + { id: "in-progress", title: "In Progress", color: "#3b82f6" }, + { id: "done", title: "Done", color: "#22c55e" }, + ]; + const stepsWithOrphan: WorkflowStep[] = [ + ...realSteps, + { id: ORPHAN_STEP_ID, title: "Needs Reassignment", color: "#f59e0b" }, + ]; + + it("offers both neighbors for a middle real step with no orphan present", () => { + const adjacency = getStepAdjacency(realSteps, 1); + expect(adjacency).toEqual({ + hasPrev: true, + prevStepId: "todo", + hasNext: true, + nextStepId: "done", + }); + }); + + it("does not offer the orphan sentinel as the next move target", () => { + // "done" (index 2) is immediately followed by the orphan node (index 3). + const adjacency = getStepAdjacency(stepsWithOrphan, 2); + expect(adjacency.hasNext).toBe(false); + expect(adjacency.nextStepId).toBeUndefined(); + // The real, backward neighbor is still a valid target. + expect(adjacency.hasPrev).toBe(true); + expect(adjacency.prevStepId).toBe("in-progress"); + }); + + it("still allows an orphaned task to move back onto the last real step", () => { + // The orphan node itself (index 3) — its prev neighbor is a real step. + const adjacency = getStepAdjacency(stepsWithOrphan, 3); + expect(adjacency.hasPrev).toBe(true); + expect(adjacency.prevStepId).toBe("done"); + expect(adjacency.hasNext).toBe(false); + }); +}); diff --git a/apps/web/components/kanban/swimlane-graph2-content.tsx b/apps/web/components/kanban/swimlane-graph2-content.tsx index 8514526907..743f420d95 100644 --- a/apps/web/components/kanban/swimlane-graph2-content.tsx +++ b/apps/web/components/kanban/swimlane-graph2-content.tsx @@ -3,7 +3,22 @@ import { useMemo, useState } from "react"; import { useSwimlaneMove } from "@/hooks/domains/kanban/use-swimlane-move"; import { Graph2TaskPipeline } from "./graph2-task-pipeline"; +import { ORPHAN_STEP, ORPHAN_STEP_ID, remapOrphanTasks } from "./swimlane-kanban-content"; import type { ViewContentProps } from "@/lib/kanban/view-registry"; +import type { Task } from "@/components/kanban-card"; +import type { WorkflowStep } from "@/components/kanban-column"; + +export function getGraph2DisplayState( + tasks: Task[], + steps: WorkflowStep[], +): { displayTasks: Task[]; displaySteps: WorkflowStep[] } { + const stepIds = new Set(steps.map((step) => step.id)); + const { tasks: displayTasks, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_STEP_ID); + return { + displayTasks, + displaySteps: hasOrphans ? [...steps, ORPHAN_STEP] : steps, + }; +} export function SwimlaneGraph2Content({ workflowId, @@ -24,16 +39,20 @@ export function SwimlaneGraph2Content({ onMoveError, }); const [movingTaskId, setMovingTaskId] = useState(null); + const { displayTasks, displaySteps } = useMemo( + () => getGraph2DisplayState(tasks, steps), + [tasks, steps], + ); const sortedTasks = useMemo( () => - [...tasks].sort((a, b) => { - const aStepIdx = steps.findIndex((c) => c.id === a.workflowStepId); - const bStepIdx = steps.findIndex((c) => c.id === b.workflowStepId); + [...displayTasks].sort((a, b) => { + const aStepIdx = displaySteps.findIndex((c) => c.id === a.workflowStepId); + const bStepIdx = displaySteps.findIndex((c) => c.id === b.workflowStepId); if (aStepIdx !== bStepIdx) return aStepIdx - bStepIdx; return (a.position ?? 0) - (b.position ?? 0); }), - [tasks, steps], + [displayTasks, displaySteps], ); const handleMoveTask = async (task: (typeof tasks)[number], targetStepId: string) => { @@ -45,7 +64,7 @@ export function SwimlaneGraph2Content({ } }; - if (tasks.length === 0) { + if (displayTasks.length === 0) { return (
No tasks
@@ -60,7 +79,7 @@ export function SwimlaneGraph2Content({ { + it("adds a Needs Reassignment pipeline step for tasks on deleted workflow steps", () => { + const { displayTasks, displaySteps } = getGraph2DisplayState( + [makeTask("valid", "todo"), makeTask("orphan", "deleted-step")], + steps, + ); + + expect(displaySteps.map((step) => step.title)).toEqual(["Todo", "Done", "Needs Reassignment"]); + expect(displayTasks.find((task) => task.id === "valid")?.workflowStepId).toBe("todo"); + expect(displayTasks.find((task) => task.id === "orphan")?.workflowStepId).toBe(ORPHAN_STEP_ID); + }); + + it("keeps the pipeline unchanged when every task references a rendered step", () => { + const { displayTasks, displaySteps } = getGraph2DisplayState( + [makeTask("valid", "todo"), makeTask("finished", "done")], + steps, + ); + + expect(displaySteps).toBe(steps); + expect(displayTasks.map((task) => task.workflowStepId)).toEqual(["todo", "done"]); + }); +}); diff --git a/apps/web/components/kanban/swimlane-kanban-content.tsx b/apps/web/components/kanban/swimlane-kanban-content.tsx index 72afafe1d3..c6d184e6a5 100644 --- a/apps/web/components/kanban/swimlane-kanban-content.tsx +++ b/apps/web/components/kanban/swimlane-kanban-content.tsx @@ -31,6 +31,28 @@ import { useKanbanExternalLinkAvailability, } from "@/components/kanban-external-link-availability"; +/** + * Sentinel step ID used to collect tasks whose workflow_step_id no longer + * matches any rendered column. Tasks remapped here are visible as a + * "Needs Reassignment" fallback column so they are never silently hidden. + */ +export const ORPHAN_STEP_ID = "__kandev_orphan__"; + +export const ORPHAN_STEP: WorkflowStep = { + id: ORPHAN_STEP_ID, + title: "Needs Reassignment", + color: "#f59e0b", +}; + +/** + * The "Needs Reassignment" column is a display-only fallback, not a real + * workflow step — it must never be offered as a manual move destination + * (drag-and-drop, "Move to" menus, or Pipeline navigation). + */ +export function isOrphanMoveTarget(targetStepId: string): boolean { + return targetStepId === ORPHAN_STEP_ID; +} + export type SwimlaneKanbanContentProps = { workflowId: string; steps: WorkflowStep[]; @@ -82,7 +104,7 @@ function useSwimlaneKanbanDnd({ tasks, workflowId, onMoveError }: SwimlaneKanban const taskId = active.id as string; const targetStepId = over.id as string; const task = tasks.find((t) => t.id === taskId); - if (!task || task.workflowStepId === targetStepId) return; + if (!task || task.workflowStepId === targetStepId || isOrphanMoveTarget(targetStepId)) return; const state = store.getState(); const snapshot = state.kanbanMulti.snapshots[workflowId]; @@ -176,6 +198,46 @@ function useMobileColumnIndex(workflowId: string, steps: WorkflowStep[], tasks: return { activeIndex, setActiveIndex }; } +/** + * remapOrphanTasks re-keys any task whose workflowStepId matches no step in + * `stepIds` to `orphanStepId`. Returns the remapped task list and whether + * any orphans were found. Pure function — safe to call outside React. + */ +export function remapOrphanTasks( + tasks: Task[], + stepIds: Set, + orphanStepId: string, +): { tasks: Task[]; hasOrphans: boolean } { + let hasOrphans = false; + const remapped = tasks.map((t) => { + if (!t.workflowStepId || stepIds.has(t.workflowStepId)) return t; + hasOrphans = true; + return { ...t, workflowStepId: orphanStepId }; + }); + return { tasks: remapped, hasOrphans }; +} + +/** + * useOrphanDisplay remaps tasks with an unknown workflowStepId to the sentinel + * ORPHAN_STEP so they appear in a visible fallback column instead of being + * silently dropped from the board. + * + * Returns: + * displayTasks – all tasks, with orphaned ones keyed to ORPHAN_STEP_ID + * displaySteps – original steps plus ORPHAN_STEP when orphans are present + */ +function useOrphanDisplay( + tasks: Task[], + steps: WorkflowStep[], +): { displayTasks: Task[]; displaySteps: WorkflowStep[] } { + return useMemo(() => { + const stepIds = new Set(steps.map((s) => s.id)); + const { tasks: displayTasks, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_STEP_ID); + const displaySteps = hasOrphans ? [...steps, ORPHAN_STEP] : steps; + return { displayTasks, displaySteps }; + }, [tasks, steps]); +} + function useTasksByStep(tasks: Task[]) { return useCallback( (stepId: string) => @@ -186,6 +248,7 @@ function useTasksByStep(tasks: Task[]) { function MobileKanbanLayout({ steps, + moveTargetSteps, tasks, activeIndex, onIndexChange, @@ -205,26 +268,10 @@ function MobileKanbanLayout({ isMultiSelectMode, externalLinkAvailability, mobileWorkflowNavigation, -}: { - steps: WorkflowStep[]; - tasks: Task[]; +}: SharedKanbanLayoutProps & { activeIndex: number; onIndexChange: (index: number) => void; - onPreviewTask: (task: Task) => void; - onOpenTask: (task: Task) => void; - onEditTask: (task: Task) => void; - onDeleteTask: (task: Task) => void; - onArchiveTask?: (task: Task) => void; - moveTaskToStep: (task: Task, targetStepId: string) => Promise; activeTask: Task | null; - showMaximizeButton?: boolean; - deletingTaskId?: string | null; - archivingTaskId?: string | null; - selectedIds?: Set; - onToggleSelect?: (taskId: string) => void; - onSelectRange?: (taskId: string, orderedIds: string[]) => void; - isMultiSelectMode?: boolean; - externalLinkAvailability: KanbanExternalLinkAvailability; mobileWorkflowNavigation?: MobileWorkflowNavigation; }) { const taskCounts = useMemo(() => { @@ -261,6 +308,7 @@ function MobileKanbanLayout({ ) : ( )} - +
); } -function TabletKanbanLayout({ - steps, - tasks, - onPreviewTask, - onOpenTask, - onEditTask, - onDeleteTask, - onArchiveTask, - moveTaskToStep, - showMaximizeButton, - deletingTaskId, - archivingTaskId, - selectedIds, - onToggleSelect, - onSelectRange, - isMultiSelectMode, - externalLinkAvailability, -}: { +type SharedKanbanLayoutProps = { steps: WorkflowStep[]; + // Real workflow steps only (excludes the synthetic "Needs Reassignment" + // sentinel) — used wherever a step is offered as a move destination + // (move menus, drop targets), since that sentinel is display-only. + moveTargetSteps: WorkflowStep[]; tasks: Task[]; onPreviewTask: (task: Task) => void; onOpenTask: (task: Task) => void; @@ -319,7 +358,27 @@ function TabletKanbanLayout({ onSelectRange?: (taskId: string, orderedIds: string[]) => void; isMultiSelectMode?: boolean; externalLinkAvailability: KanbanExternalLinkAvailability; -}) { +}; + +function TabletKanbanLayout({ + steps, + moveTargetSteps, + tasks, + onPreviewTask, + onOpenTask, + onEditTask, + onDeleteTask, + onArchiveTask, + moveTaskToStep, + showMaximizeButton, + deletingTaskId, + archivingTaskId, + selectedIds, + onToggleSelect, + onSelectRange, + isMultiSelectMode, + externalLinkAvailability, +}: SharedKanbanLayoutProps) { const getTasksForStep = useTasksByStep(tasks); return ( @@ -338,7 +397,7 @@ function TabletKanbanLayout({ onDeleteTask={onDeleteTask} onArchiveTask={onArchiveTask} onMoveTask={moveTaskToStep} - steps={steps} + steps={moveTargetSteps} showMaximizeButton={showMaximizeButton} deletingTaskId={deletingTaskId} archivingTaskId={archivingTaskId} @@ -356,6 +415,7 @@ function TabletKanbanLayout({ function DesktopKanbanLayout({ steps, + moveTargetSteps, tasks, onPreviewTask, onOpenTask, @@ -372,24 +432,8 @@ function DesktopKanbanLayout({ isMultiSelectMode, isCompactDesktop, externalLinkAvailability, -}: { - steps: WorkflowStep[]; - tasks: Task[]; - onPreviewTask: (task: Task) => void; - onOpenTask: (task: Task) => void; - onEditTask: (task: Task) => void; - onDeleteTask: (task: Task) => void; - onArchiveTask?: (task: Task) => void; - moveTaskToStep: (task: Task, targetStepId: string) => Promise; - showMaximizeButton?: boolean; - deletingTaskId?: string | null; - archivingTaskId?: string | null; - selectedIds?: Set; - onToggleSelect?: (taskId: string) => void; - onSelectRange?: (taskId: string, orderedIds: string[]) => void; - isMultiSelectMode?: boolean; +}: SharedKanbanLayoutProps & { isCompactDesktop: boolean; - externalLinkAvailability: KanbanExternalLinkAvailability; }) { const getTasksForStep = useTasksByStep(tasks); @@ -412,7 +456,7 @@ function DesktopKanbanLayout({ onDeleteTask={onDeleteTask} onArchiveTask={onArchiveTask} onMoveTask={moveTaskToStep} - steps={steps} + steps={moveTargetSteps} deletingTaskId={deletingTaskId} archivingTaskId={archivingTaskId} showMaximizeButton={showMaximizeButton} @@ -427,6 +471,50 @@ function DesktopKanbanLayout({ ); } +/** + * Picks the responsive layout to render for the swimlane. Extracted so + * `SwimlaneKanbanContent` stays under the max-lines-per-function limit. + */ +function renderKanbanLayout({ + isMobile, + isTablet, + isCompactDesktop, + sharedProps, + activeIndex, + setActiveIndex, + activeTask, + mobileWorkflowNavigation, +}: { + isMobile: boolean; + isTablet: boolean; + isCompactDesktop: boolean; + sharedProps: SharedKanbanLayoutProps; + activeIndex: number; + setActiveIndex: (index: number) => void; + activeTask: Task | null; + mobileWorkflowNavigation?: MobileWorkflowNavigation; +}): React.ReactNode { + if (isMobile) { + return ( + + ); + } + if (isTablet) { + return ; + } + return ( +
+ +
+ ); +} + export function SwimlaneKanbanContent({ workflowId, steps, @@ -449,17 +537,27 @@ export function SwimlaneKanbanContent({ const { isMobile, isTablet, isCompactDesktop } = useResponsiveBreakpoint(); const activeWorkspaceId = useAppStore((state) => state.workspaces.activeId); const externalLinkAvailability = useKanbanExternalLinkAvailability(activeWorkspaceId); - const { activeIndex, setActiveIndex } = useMobileColumnIndex(workflowId, steps, tasks); + + // Remap tasks with a dead workflowStepId to the orphan sentinel so they + // always appear in a visible column rather than being silently dropped. + const { displayTasks, displaySteps } = useOrphanDisplay(tasks, steps); + + const { activeIndex, setActiveIndex } = useMobileColumnIndex( + workflowId, + displaySteps, + displayTasks, + ); const { sensors, handleDragStart, handleDragEnd, handleDragCancel, moveTaskToStep, activeTask } = - useSwimlaneKanbanDnd({ tasks, workflowId, onMoveError }); + useSwimlaneKanbanDnd({ tasks: displayTasks, workflowId, onMoveError }); // Memoized so the layout components don't re-render from a fresh props object // on every parent render. Declared before the early return to keep hook order // stable. const sharedProps = useMemo( () => ({ - steps, - tasks, + steps: displaySteps, + moveTargetSteps: steps, + tasks: displayTasks, onPreviewTask, onOpenTask, onEditTask, @@ -476,8 +574,9 @@ export function SwimlaneKanbanContent({ externalLinkAvailability, }), [ + displaySteps, steps, - tasks, + displayTasks, onPreviewTask, onOpenTask, onEditTask, @@ -495,28 +594,18 @@ export function SwimlaneKanbanContent({ ], ); - if (steps.length === 0 && !isMobile) return null; + if (displaySteps.length === 0 && !isMobile) return null; - let layoutContent: React.ReactNode; - if (isMobile) { - layoutContent = ( - - ); - } else if (isTablet) { - layoutContent = ; - } else { - layoutContent = ( -
- -
- ); - } + const layoutContent = renderKanbanLayout({ + isMobile, + isTablet, + isCompactDesktop, + sharedProps, + activeIndex, + setActiveIndex, + activeTask, + mobileWorkflowNavigation, + }); return ( [0][number]; +} + +describe("remapOrphanTasks", () => { + it("leaves tasks on known steps unchanged", () => { + const stepIds = new Set(["s1", "s2"]); + const tasks = [makeTask("t1", "s1"), makeTask("t2", "s2")]; + const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID); + expect(hasOrphans).toBe(false); + expect(out[0].workflowStepId).toBe("s1"); + expect(out[1].workflowStepId).toBe("s2"); + }); + + it("remaps a task with a dead step to the orphan sentinel", () => { + const stepIds = new Set(["s1"]); + const tasks = [makeTask("t-orphan", "deleted-step")]; + const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID); + expect(hasOrphans).toBe(true); + expect(out[0].workflowStepId).toBe(ORPHAN_ID); + }); + + it("does not mutate the original task object", () => { + const stepIds = new Set(["s1"]); + const original = makeTask("t-orig", "dead"); + const { tasks: out } = remapOrphanTasks([original], stepIds, ORPHAN_ID); + // Original must not change. + expect(original.workflowStepId).toBe("dead"); + // Returned task is a new object. + expect(out[0]).not.toBe(original); + }); + + it("handles a mix of valid and orphaned tasks", () => { + const stepIds = new Set(["s1", "s2"]); + const tasks = [makeTask("t-ok1", "s1"), makeTask("t-orphan", "ghost"), makeTask("t-ok2", "s2")]; + const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID); + expect(hasOrphans).toBe(true); + expect(out[0].workflowStepId).toBe("s1"); + expect(out[1].workflowStepId).toBe(ORPHAN_ID); + expect(out[2].workflowStepId).toBe("s2"); + }); + + it("does not remap tasks with an empty workflowStepId", () => { + const stepIds = new Set(["s1"]); + const tasks = [makeTask("t-empty", "")]; + const { tasks: out, hasOrphans } = remapOrphanTasks(tasks, stepIds, ORPHAN_ID); + expect(hasOrphans).toBe(false); + expect(out[0].workflowStepId).toBe(""); + }); + + it("returns hasOrphans=false for an empty task list", () => { + const { hasOrphans } = remapOrphanTasks([], new Set(["s1"]), ORPHAN_ID); + expect(hasOrphans).toBe(false); + }); +}); diff --git a/apps/web/components/kanban/swipeable-columns.tsx b/apps/web/components/kanban/swipeable-columns.tsx index 82c29e8d17..3d5df65b7d 100644 --- a/apps/web/components/kanban/swipeable-columns.tsx +++ b/apps/web/components/kanban/swipeable-columns.tsx @@ -9,6 +9,9 @@ import type { KanbanExternalLinkAvailability } from "../kanban-external-link-ava type SwipeableColumnsProps = { steps: WorkflowStep[]; + // Real workflow steps only (excludes the "Needs Reassignment" sentinel) — + // passed to KanbanColumn purely for move-menu targets. + moveTargetSteps: WorkflowStep[]; tasks: Task[]; activeIndex: number; onIndexChange: (index: number) => void; @@ -66,6 +69,7 @@ function useEmblaIndexSync( export function SwipeableColumns({ steps, + moveTargetSteps, tasks, activeIndex, onIndexChange, @@ -126,7 +130,7 @@ export function SwipeableColumns({ onDeleteTask={onDeleteTask} onArchiveTask={onArchiveTask} onMoveTask={onMoveTask} - steps={steps} + steps={moveTargetSteps} showMaximizeButton={showMaximizeButton} deletingTaskId={deletingTaskId} archivingTaskId={archivingTaskId}