From 06d72a121564b5adcabcc9efae9ca68eeb3129d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Mon, 27 Jul 2026 16:14:13 +0100 Subject: [PATCH] fix: enforce workflow-step WIP limits on task creation (#1980) --- .../backend/internal/mcp/handlers/handlers.go | 25 ++- .../internal/mcp/handlers/handlers_test.go | 7 + .../orchestrator/event_handlers_github.go | 37 +++- .../event_handlers_github_review_test.go | 90 ++++++++ .../orchestrator/event_handlers_workflow.go | 63 +++--- .../event_handlers_workflow_test.go | 88 ++++++++ .../internal/orchestrator/watcher_dispatch.go | 11 +- .../orchestrator/watcher_dispatch_test.go | 21 ++ .../internal/orchestrator/workflow_store.go | 22 -- apps/backend/internal/task/handlers/errors.go | 4 + .../internal/task/handlers/errors_test.go | 15 ++ .../task/handlers/task_http_handlers_test.go | 36 ++++ .../task/handlers/task_ws_handlers.go | 3 + .../internal/task/repository/interface.go | 2 + .../internal/task/repository/sqlite/task.go | 56 ++++- .../internal/task/repository/task_wip_test.go | 99 +++++++++ apps/backend/internal/task/service/service.go | 1 + .../internal/task/service/service_tasks.go | 30 ++- .../task/service/service_tasks_wip_test.go | 121 +++++++++++ .../internal/task/service/service_workflow.go | 5 +- .../internal/workflow/models/errors.go | 30 +++ .../e2e/tests/kanban/mobile-kanban.spec.ts | 4 +- docs/plans/wip-limited-task-creation/plan.md | 199 ++++++++++++++++++ .../task-01-atomic-repository-admission.md | 65 ++++++ .../task-02-task-service-enforcement.md | 63 ++++++ ...-conflict-adapters-and-watcher-deferral.md | 78 +++++++ .../task-04-public-documentation.md | 56 +++++ docs/public/tasks-and-workflows.md | 19 ++ docs/public/workflow-tips.md | 15 ++ docs/specs/tasks/wip-limit-pull-system.md | 61 +++++- 30 files changed, 1239 insertions(+), 87 deletions(-) create mode 100644 apps/backend/internal/task/repository/task_wip_test.go create mode 100644 apps/backend/internal/task/service/service_tasks_wip_test.go create mode 100644 apps/backend/internal/workflow/models/errors.go create mode 100644 docs/plans/wip-limited-task-creation/plan.md create mode 100644 docs/plans/wip-limited-task-creation/task-01-atomic-repository-admission.md create mode 100644 docs/plans/wip-limited-task-creation/task-02-task-service-enforcement.md create mode 100644 docs/plans/wip-limited-task-creation/task-03-conflict-adapters-and-watcher-deferral.md create mode 100644 docs/plans/wip-limited-task-creation/task-04-public-documentation.md diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index 2c72dd833a..2894e83a20 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -651,14 +651,12 @@ func (h *Handlers) handleCreateTask(ctx context.Context, msg *ws.Message) (*ws.M }) if err != nil { h.logger.Error("failed to create task", zap.Error(err)) - // Defense-in-depth: resolveTaskRepositories already catches this for the - // MCP path, but non-MCP callers (UI, internal engine) reach here directly. - if errors.Is(err, service.ErrSubtaskDepthExceeded) || - errors.Is(err, service.ErrInvalidTaskWorkflow) || - isMCPWorkflowNotFoundError(err) { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) + code := classifyCreateTaskError(err) + message := "Failed to create task" + if code != ws.ErrorCodeInternalError { + message = err.Error() } - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "Failed to create task", nil) + return ws.NewError(msg.ID, msg.Action, code, message, nil) } if h.handoffSvc != nil && workspacePolicy.NeedsAttachment() { @@ -681,6 +679,19 @@ func (h *Handlers) handleCreateTask(ctx context.Context, msg *ws.Message) (*ws.M return ws.NewResponse(msg.ID, msg.Action, dto.FromTask(task)) } +func classifyCreateTaskError(err error) string { + switch { + case errors.Is(err, service.ErrWIPLimitExceeded): + return ws.ErrorCodeConflict + case errors.Is(err, service.ErrSubtaskDepthExceeded), + errors.Is(err, service.ErrInvalidTaskWorkflow), + isMCPWorkflowNotFoundError(err): + return ws.ErrorCodeValidation + default: + return ws.ErrorCodeInternalError + } +} + // taskRepoResult holds the output of resolveTaskRepositories. type taskRepoResult struct { Repos []service.TaskRepositoryInput diff --git a/apps/backend/internal/mcp/handlers/handlers_test.go b/apps/backend/internal/mcp/handlers/handlers_test.go index c789d26acf..a6d49e94b7 100644 --- a/apps/backend/internal/mcp/handlers/handlers_test.go +++ b/apps/backend/internal/mcp/handlers/handlers_test.go @@ -196,6 +196,13 @@ func TestClassifyAddBranchError_UnresolvedBaseBranchIsValidation(t *testing.T) { } } +func TestClassifyCreateTaskErrorMapsWIPLimitToConflict(t *testing.T) { + err := fmt.Errorf("create task failed: %w", workflowmodels.NewWIPLimitError("review", 2, 2)) + if got := classifyCreateTaskError(err); got != ws.ErrorCodeConflict { + t.Fatalf("expected ErrorCodeConflict, got %q", got) + } +} + // TestHandleAddBranchToTask_RejectsMultipleLocators pins the mutual-exclusion // check at the WS handler tier: supplying two of repository_id / // repository_url / local_path is an agent mistake that previously got diff --git a/apps/backend/internal/orchestrator/event_handlers_github.go b/apps/backend/internal/orchestrator/event_handlers_github.go index 19d84ca34e..8c56dc2b1b 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github.go +++ b/apps/backend/internal/orchestrator/event_handlers_github.go @@ -2,6 +2,7 @@ package orchestrator import ( "context" + "errors" "fmt" "strconv" "strings" @@ -19,6 +20,10 @@ import ( wfmodels "github.com/kandev/kandev/internal/workflow/models" ) +func isWatcherCapacityRejection(err error) bool { + return errors.Is(err, wfmodels.ErrWIPLimitExceeded) +} + const ( issueWatchIDKey = "issue_watch_id" issueNumberKey = "issue_number" @@ -329,10 +334,18 @@ func (s *Service) createReviewTask(ctx context.Context, evt *github.NewReviewPRE repositories := s.resolveReviewRepository(ctx, evt.WorkspaceID, pr) task, err := s.reviewTaskCreator.CreateReviewTask(ctx, buildReviewTaskRequest(evt, repositories, repoSlug)) if err != nil { - s.logger.Error("failed to create review task", - zap.String("review_watch_id", evt.ReviewWatchID), - zap.Int("pr_number", pr.Number), - zap.Error(err)) + if isWatcherCapacityRejection(err) { + s.logger.Info("review workflow step at capacity; deferring review task", + zap.String("review_watch_id", evt.ReviewWatchID), + zap.Int("pr_number", pr.Number), + zap.String("workflow_step_id", evt.WorkflowStepID), + zap.Error(err)) + } else { + s.logger.Error("failed to create review task", + zap.String("review_watch_id", evt.ReviewWatchID), + zap.Int("pr_number", pr.Number), + zap.Error(err)) + } s.releaseReviewPR(ctx, evt) return } @@ -1343,10 +1356,18 @@ func (s *Service) createIssueTask(ctx context.Context, evt *github.NewIssueEvent task, err := s.issueTaskCreator.CreateIssueTask(ctx, req) if err != nil { - s.logger.Error("failed to create issue task", - zap.String(issueWatchIDKey, evt.IssueWatchID), - zap.Int(issueNumberKey, issue.Number), - zap.Error(err)) + if isWatcherCapacityRejection(err) { + s.logger.Info("issue workflow step at capacity; deferring issue task", + zap.String(issueWatchIDKey, evt.IssueWatchID), + zap.Int(issueNumberKey, issue.Number), + zap.String("workflow_step_id", evt.WorkflowStepID), + zap.Error(err)) + } else { + s.logger.Error("failed to create issue task", + zap.String(issueWatchIDKey, evt.IssueWatchID), + zap.Int(issueNumberKey, issue.Number), + zap.Error(err)) + } s.releaseIssueWatch(ctx, evt) return } diff --git a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go index 3de9ccd88f..9a16c5a494 100644 --- a/apps/backend/internal/orchestrator/event_handlers_github_review_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_github_review_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/kandev/kandev/internal/github" + "github.com/kandev/kandev/internal/orchestrator/watcher" "github.com/kandev/kandev/internal/task/models" wfmodels "github.com/kandev/kandev/internal/workflow/models" ) @@ -13,11 +14,17 @@ import ( type countingReviewTaskCreator struct { calls int err error + errs []error taskID string } func (c *countingReviewTaskCreator) CreateReviewTask(_ context.Context, _ *ReviewTaskRequest) (*models.Task, error) { c.calls++ + if len(c.errs) > 0 { + err := c.errs[0] + c.errs = c.errs[1:] + return nil, err + } if c.err != nil { return nil, c.err } @@ -137,3 +144,86 @@ func TestCreateReviewTask_ReleasesOnTaskCreationFailure(t *testing.T) { t.Errorf("expected no Assign call after failure, got %d", ghSvc.assignCalls) } } + +func TestGitHubWatcherTaskCreateError_ClassifiesWIPAsDeferred(t *testing.T) { + err := wfmodels.NewWIPLimitError("step1", 2, 2) + if !isWatcherCapacityRejection(err) { + t.Fatal("expected WIP-limit rejection to be classified as deferred capacity") + } +} + +func TestCreateReviewTask_WIPRejectionIsRetriedOnLaterPoll(t *testing.T) { + svc, _ := setupReviewTaskTest(t) + ghSvc := &mockGitHubService{reserveReturn: true} + svc.SetGitHubService(ghSvc) + creator := &countingReviewTaskCreator{ + errs: []error{wfmodels.NewWIPLimitError("step1", 2, 2)}, + taskID: "task-after-capacity", + } + svc.SetReviewTaskCreator(creator) + + svc.createReviewTask(context.Background(), newReviewEvent()) + svc.createReviewTask(context.Background(), newReviewEvent()) + + if creator.calls != 2 { + t.Fatalf("expected one rejected attempt and one retry, got %d calls", creator.calls) + } + if ghSvc.releaseCalls != 1 { + t.Fatalf("expected only the rejected attempt to release its reservation, got %d", ghSvc.releaseCalls) + } + if ghSvc.assignCalls != 1 || ghSvc.assignedTaskID != "task-after-capacity" { + t.Fatalf("expected retry to assign task ID, calls=%d task=%q", ghSvc.assignCalls, ghSvc.assignedTaskID) + } +} + +func TestReviewWatchLifecycle_BootReadyStaysOnReviewUntilTurnComplete(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "review-task", "review-session", "review") + + steps := newMockStepGetter() + steps.steps["review"] = &wfmodels.WorkflowStep{ + ID: "review", WorkflowID: "wf1", Name: "Review", Position: 0, + Events: wfmodels.StepEvents{ + OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterAutoStartAgent}}, + OnTurnComplete: []wfmodels.OnTurnCompleteAction{{Type: wfmodels.OnTurnCompleteMoveToNext}}, + }, + } + steps.steps["done"] = &wfmodels.WorkflowStep{ID: "done", WorkflowID: "wf1", Name: "Done", Position: 1} + agentMgr := &mockAgentManager{repoForExecutionLookup: repo} + svc := createTestServiceWithAgent(repo, steps, newMockTaskRepo(), agentMgr) + + if !svc.shouldAutoStartStep(ctx, "review") { + t.Fatal("review step should auto-start an admitted watcher task") + } + svc.handleAgentBootReady(ctx, watcher.AgentEventData{TaskID: "review-task", SessionID: "review-session"}) + bootTask, err := repo.GetTask(ctx, "review-task") + if err != nil { + t.Fatalf("load task after boot-ready: %v", err) + } + if bootTask.WorkflowStepID != "review" { + t.Fatalf("boot-ready must not advance review task, got step %q", bootTask.WorkflowStepID) + } + + if err := repo.UpdateTaskSessionState(ctx, "review-session", models.TaskSessionStateRunning, ""); err != nil { + t.Fatalf("mark review turn running: %v", err) + } + task, err := repo.GetTask(ctx, "review-task") + if err != nil { + t.Fatalf("reload task before turn completion: %v", err) + } + session, err := repo.GetTaskSession(ctx, "review-session") + if err != nil { + t.Fatalf("reload session before turn completion: %v", err) + } + if !svc.processOnTurnComplete(ctx, task, session) { + t.Fatal("first genuine turn completion should advance the review task") + } + completedTask, err := repo.GetTask(ctx, "review-task") + if err != nil { + t.Fatalf("load task after turn completion: %v", err) + } + if completedTask.WorkflowStepID != "done" { + t.Fatalf("expected turn completion to move task to done, got %q", completedTask.WorkflowStepID) + } +} diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index ba78565909..e082ee6462 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -255,30 +255,12 @@ func (s *Service) executeStepTransition(ctx context.Context, taskID, sessionID s s.setSessionWaitingForInput(ctx, taskID, sessionID) return } - if err := s.validateTransitionWIPLimit(ctx, task, targetStep); err != nil { - s.logger.Warn("workflow transition rejected by WIP limit", - zap.String("task_id", taskID), - zap.String("to_step_id", toStepID), - zap.Error(err)) - s.setSessionWaitingForInput(ctx, taskID, sessionID) - return - } - - // Process on_exit actions for the step we're leaving (before the step change). - // Freshly load the session since the caller may not have it (legacy path). - exitSession, exitErr := s.repo.GetTaskSession(ctx, sessionID) - if exitErr != nil { - s.logger.Warn("failed to load session for on_exit", - zap.String("session_id", sessionID), zap.Error(exitErr)) - } else { - s.processOnExit(ctx, taskID, exitSession, fromStep) - } - - // Update the task's workflow step + // Atomically admit the target step before exit side effects. A capacity + // rejection must leave both the task and its source-step session state intact. task.WorkflowStepID = toStepID task.UpdatedAt = time.Now().UTC() - if err := s.repo.UpdateTask(ctx, task); err != nil { - s.logger.Error("failed to move task to next workflow step", + if err := s.updateTransitionTaskWithCapacity(ctx, task, targetStep); err != nil { + s.logger.Warn("workflow transition rejected or failed", zap.String("task_id", taskID), zap.String("from_step", fromStep.Name), zap.String("to_step", targetStep.Name), @@ -287,6 +269,16 @@ func (s *Service) executeStepTransition(ctx context.Context, taskID, sessionID s return } + // Process on_exit only after the transition is durably admitted. Freshly + // load the session since the caller may not have it (legacy path). + exitSession, exitErr := s.repo.GetTaskSession(ctx, sessionID) + if exitErr != nil { + s.logger.Warn("failed to load session for on_exit", + zap.String("session_id", sessionID), zap.Error(exitErr)) + } else { + s.processOnExit(ctx, taskID, exitSession, fromStep) + } + // Publish task updated event via the task service so the payload carries // the full context (session counts, primary session, repositories). s.publishTaskUpdated(ctx, task) @@ -346,22 +338,25 @@ func (s *Service) executeStepTransition(ctx context.Context, taskID, sessionID s } } -func (s *Service) validateTransitionWIPLimit(ctx context.Context, task *models.Task, targetStep *wfmodels.WorkflowStep) error { - if targetStep == nil || targetStep.WIPLimit <= 0 || task.WorkflowStepID == targetStep.ID { - return nil +func (s *Service) updateTransitionTaskWithCapacity( + ctx context.Context, + task *models.Task, + targetStep *wfmodels.WorkflowStep, +) error { + if targetStep == nil || targetStep.WIPLimit <= 0 { + return s.repo.UpdateTask(ctx, task) } - limitsRepo, ok := s.repo.(workflowMoveLimitsRepository) + limitedRepo, ok := s.repo.(workflowLimitedMoveRepository) if !ok { return fmt.Errorf("WIP limit cannot be checked for workflow step %s", targetStep.ID) } - occupants, err := limitsRepo.CountTasksByWorkflowStepExcludingTask(ctx, targetStep.ID, task.ID) - if err != nil { - return fmt.Errorf("count target workflow step tasks: %w", err) - } - if occupants >= targetStep.WIPLimit { - return fmt.Errorf("WIP limit exceeded for workflow step %s: limit %d already occupied", targetStep.ID, targetStep.WIPLimit) - } - return nil + return limitedRepo.UpdateTaskIfWorkflowStepHasCapacity( + ctx, + task, + targetStep.ID, + task.ID, + targetStep.WIPLimit, + ) } // handleTaskMoved handles manual task step changes (drag-and-drop, stepper "Move here"). diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow_test.go b/apps/backend/internal/orchestrator/event_handlers_workflow_test.go index b7bdb3c30a..90c3c4ef8d 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow_test.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow_test.go @@ -2,6 +2,7 @@ package orchestrator import ( "context" + "errors" "sync" "testing" "time" @@ -9,6 +10,7 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" "github.com/kandev/kandev/internal/task/models" + wfmodels "github.com/kandev/kandev/internal/workflow/models" ) // mockEventBus captures published events for assertion. @@ -17,6 +19,92 @@ type mockEventBus struct { events []publishedEvent } +func TestUpdateTransitionTaskWithCapacity_RejectsFullLimitedStep(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + if err := repo.CreateTask(ctx, &models.Task{ + ID: "occupant", + WorkspaceID: "ws1", + WorkflowID: "wf1", + WorkflowStepID: "step2", + Title: "Occupant", + State: "TODO", + Priority: "medium", + }); err != nil { + t.Fatalf("create occupant: %v", err) + } + + svc := createTestService(repo, newMockStepGetter(), newMockTaskRepo()) + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("load task: %v", err) + } + task.WorkflowStepID = "step2" + target := &wfmodels.WorkflowStep{ID: "step2", WorkflowID: "wf1", WIPLimit: 1} + + err = svc.updateTransitionTaskWithCapacity(ctx, task, target) + if !errors.Is(err, wfmodels.ErrWIPLimitExceeded) { + t.Fatalf("error=%v, want typed WIP rejection", err) + } + stored, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("reload task: %v", err) + } + if stored.WorkflowStepID != "step1" { + t.Fatalf("task moved despite full target, got %q", stored.WorkflowStepID) + } +} + +func TestExecuteStepTransition_FullTargetLeavesOnExitStateIntact(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + seedSession(t, repo, "t1", "s1", "step1") + if err := repo.SetSessionMetadataKey(ctx, "s1", "plan_mode", true); err != nil { + t.Fatalf("enable plan mode: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "occupant", + WorkspaceID: "ws1", + WorkflowID: "wf1", + WorkflowStepID: "step2", + Title: "Occupant", + State: "TODO", + Priority: "medium", + }); err != nil { + t.Fatalf("create occupant: %v", err) + } + + steps := newMockStepGetter() + fromStep := &wfmodels.WorkflowStep{ + ID: "step1", WorkflowID: "wf1", Name: "Source", + Events: wfmodels.StepEvents{OnExit: []wfmodels.OnExitAction{{ + Type: wfmodels.OnExitDisablePlanMode, + }}}, + } + steps.steps["step2"] = &wfmodels.WorkflowStep{ + ID: "step2", WorkflowID: "wf1", Name: "Limited", WIPLimit: 1, + } + svc := createTestService(repo, steps, newMockTaskRepo()) + + svc.executeStepTransition(ctx, "t1", "s1", fromStep, "step2", false) + + task, err := repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("load task: %v", err) + } + if task.WorkflowStepID != "step1" { + t.Fatalf("task moved despite full target, got %q", task.WorkflowStepID) + } + session, err := repo.GetTaskSession(ctx, "s1") + if err != nil { + t.Fatalf("load session: %v", err) + } + if enabled, _ := session.Metadata["plan_mode"].(bool); !enabled { + t.Fatal("capacity rejection must not run source step's on_exit actions") + } +} + type publishedEvent struct { Subject string Event *bus.Event diff --git a/apps/backend/internal/orchestrator/watcher_dispatch.go b/apps/backend/internal/orchestrator/watcher_dispatch.go index a4e818ba34..70dc8debcc 100644 --- a/apps/backend/internal/orchestrator/watcher_dispatch.go +++ b/apps/backend/internal/orchestrator/watcher_dispatch.go @@ -2,11 +2,13 @@ package orchestrator import ( "context" + "errors" "sync" "go.uber.org/zap" "github.com/kandev/kandev/internal/common/logger" + workflowmodels "github.com/kandev/kandev/internal/workflow/models" ) // WatcherDispatchCoordinator owns the cross-integration pipeline that turns @@ -357,8 +359,13 @@ func (c *WatcherDispatchCoordinator) Dispatch(ctx context.Context, src WatcherSo task, err := c.getTaskCreator().CreateIssueTask(ctx, req) if err != nil { - c.logger.Error("watcher dispatch: create issue task failed", - zap.String("source", src.Name()), zap.Error(err)) + if errors.Is(err, workflowmodels.ErrWIPLimitExceeded) { + c.logger.Info("watcher dispatch: workflow step at capacity; deferring issue task", + zap.String("source", src.Name()), zap.String("workflow_step_id", req.WorkflowStepID), zap.Error(err)) + } else { + c.logger.Error("watcher dispatch: create issue task failed", + zap.String("source", src.Name()), zap.Error(err)) + } src.Release(ctx, evt) return } diff --git a/apps/backend/internal/orchestrator/watcher_dispatch_test.go b/apps/backend/internal/orchestrator/watcher_dispatch_test.go index 57e1f46b08..6375127f65 100644 --- a/apps/backend/internal/orchestrator/watcher_dispatch_test.go +++ b/apps/backend/internal/orchestrator/watcher_dispatch_test.go @@ -9,6 +9,7 @@ import ( "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/task/models" + workflowmodels "github.com/kandev/kandev/internal/workflow/models" ) // nopLogger returns a *logger.Logger backed by zap.NewNop() for use in tests. @@ -247,6 +248,26 @@ func TestCoordinator_Dispatch_CreateError_Releases(t *testing.T) { } } +func TestCoordinator_Dispatch_WIPLimitRejection_ReleasesWithoutErrorSemantics(t *testing.T) { + src := &fakeWatcherSource{ + name: "linear", + reserveOK: true, + buildReq: &IssueTaskRequest{WorkspaceID: "ws-1", WorkflowStepID: "step-1"}, + } + tc := &fakeTaskCreator{createErr: workflowmodels.NewWIPLimitError("step-1", 2, 2)} + starter := &fakeTaskStarter{} + c := newTestCoordinator(t, tc, true, starter) + + c.Dispatch(context.Background(), src, "evt") + + if src.recordedRelease != 1 { + t.Fatalf("expected release after WIP rejection, got %d", src.recordedRelease) + } + if src.recordedAttach != 0 || starter.called != 0 { + t.Fatal("expected WIP rejection to stop before attach and auto-start") + } +} + func TestCoordinator_Dispatch_NoAutoStart(t *testing.T) { src := &fakeWatcherSource{ name: "linear", diff --git a/apps/backend/internal/orchestrator/workflow_store.go b/apps/backend/internal/orchestrator/workflow_store.go index 35b3512c9f..9224961f48 100644 --- a/apps/backend/internal/orchestrator/workflow_store.go +++ b/apps/backend/internal/orchestrator/workflow_store.go @@ -127,10 +127,6 @@ func (s *workflowStore) ApplyTransition(ctx context.Context, taskID, sessionID, if err != nil { return fmt.Errorf("load target step for transition: %w", err) } - if err := s.validateWIPLimit(ctx, task, targetStep); err != nil { - 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 @@ -180,24 +176,6 @@ func (s *workflowStore) updateTransitionTask(ctx context.Context, task *models.T return limitedRepo.UpdateTaskIfWorkflowStepHasCapacity(ctx, task, targetStep.ID, task.ID, targetStep.WIPLimit) } -func (s *workflowStore) validateWIPLimit(ctx context.Context, task *models.Task, targetStep *wfmodels.WorkflowStep) error { - if targetStep == nil || targetStep.WIPLimit <= 0 || task.WorkflowStepID == targetStep.ID { - return nil - } - limitsRepo, ok := s.repo.(workflowMoveLimitsRepository) - if !ok { - return fmt.Errorf("WIP limit cannot be checked for workflow step %s", targetStep.ID) - } - occupants, err := limitsRepo.CountTasksByWorkflowStepExcludingTask(ctx, targetStep.ID, task.ID) - if err != nil { - return fmt.Errorf("count target workflow step tasks: %w", err) - } - if occupants >= targetStep.WIPLimit { - return fmt.Errorf("WIP limit exceeded for workflow step %s: limit %d already occupied", targetStep.ID, targetStep.WIPLimit) - } - return nil -} - func (s *workflowStore) pullNextTaskOnVacate(ctx context.Context, vacatedStepID, excludeTaskID string) { vacatedStep := s.pullEnabledStep(ctx, vacatedStepID) if vacatedStep == nil { diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index b13e4add11..18ae1a8326 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -17,6 +17,10 @@ func handleNotFound(c *gin.Context, log *logger.Logger, err error, fallback stri c.JSON(http.StatusNotFound, gin.H{"error": fallback}) return } + if errors.Is(err, service.ErrWIPLimitExceeded) { + c.JSON(http.StatusConflict, gin.H{"error": err.Error()}) + return + } if isValidationError(err) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return diff --git a/apps/backend/internal/task/handlers/errors_test.go b/apps/backend/internal/task/handlers/errors_test.go index 4a645c2f91..7207b4fa04 100644 --- a/apps/backend/internal/task/handlers/errors_test.go +++ b/apps/backend/internal/task/handlers/errors_test.go @@ -5,11 +5,15 @@ import ( "errors" "fmt" "net" + "net/http" + "net/http/httptest" "testing" + "github.com/gin-gonic/gin" "github.com/kandev/kandev/internal/agent/runtime/lifecycle" taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" "github.com/kandev/kandev/internal/task/service" + wfmodels "github.com/kandev/kandev/internal/workflow/models" ) // TestErrorsAreClassifiable verifies that the package's error-classification @@ -42,6 +46,16 @@ func TestErrorsAreClassifiable(t *testing.T) { t.Errorf("untyped lookalike must no longer classify") } }) + + t.Run("WIP limit is a conflict for task creation", func(t *testing.T) { + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(rec) + handleNotFound(ctx, newTestLogger(t), fmt.Errorf("create task: %w", wfmodels.NewWIPLimitError("review", 2, 2)), "task not created") + if rec.Code != http.StatusConflict { + t.Fatalf("status=%d, want %d", rec.Code, http.StatusConflict) + } + }) } // TestIsTimeoutError pins the UX-classification contract for the prompt @@ -75,6 +89,7 @@ func TestIsTimeoutError(t *testing.T) { } }) } + } type timeoutNetErr struct{} diff --git a/apps/backend/internal/task/handlers/task_http_handlers_test.go b/apps/backend/internal/task/handlers/task_http_handlers_test.go index 1c8c13ba7c..2af07aeaa5 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers_test.go +++ b/apps/backend/internal/task/handlers/task_http_handlers_test.go @@ -320,6 +320,14 @@ type missingWorkflowCreateTaskRepo struct { captureCreateTaskRepo } +type wipCreateTaskRepo struct { + captureCreateTaskRepo +} + +func (*wipCreateTaskRepo) CreateTask(_ context.Context, _ *models.Task) error { + return wfmodels.NewWIPLimitError("review", 2, 2) +} + func (*missingWorkflowCreateTaskRepo) GetWorkflow(_ context.Context, id string) (*models.Workflow, error) { return nil, fmt.Errorf("workflow not found: %s", id) } @@ -703,6 +711,34 @@ func TestWSCreateTaskReturnsValidationErrorForMissingWorkflow(t *testing.T) { assert.Contains(t, payload.Message, "workflow not found") } +func TestWSCreateTaskReturnsConflictForWIPLimit(t *testing.T) { + log := newTestLogger(t) + repo := &wipCreateTaskRepo{} + svc := service.NewService(service.Repos{ + Workspaces: repo, Tasks: repo, TaskRepos: repo, + Workflows: repo, Messages: repo, Turns: repo, + Sessions: repo, GitSnapshots: repo, RepoEntities: repo, + Executors: repo, Environments: repo, TaskEnvironments: repo, + Reviews: repo, + }, nil, log, service.RepositoryDiscoveryConfig{}) + h := &TaskHandlers{service: svc, logger: log} + + msg, err := ws.NewRequest("msg-1", ws.ActionTaskCreate, map[string]any{ + "workspace_id": "ws-1", + "workflow_id": "wf-1", + "title": "Full review step", + }) + require.NoError(t, err) + + resp, err := h.wsCreateTask(context.Background(), msg) + require.NoError(t, err) + require.Equal(t, ws.MessageTypeError, resp.Type) + var payload ws.ErrorPayload + require.NoError(t, json.Unmarshal(resp.Payload, &payload)) + assert.Equal(t, ws.ErrorCodeConflict, payload.Code) + assert.Contains(t, payload.Message, "WIP limit") +} + func TestWSCreateTaskReturnsValidationErrorForStepOutsideWorkflow(t *testing.T) { log := newTestLogger(t) repo := &captureCreateTaskRepo{} diff --git a/apps/backend/internal/task/handlers/task_ws_handlers.go b/apps/backend/internal/task/handlers/task_ws_handlers.go index b289d0ff59..3ed2f49646 100644 --- a/apps/backend/internal/task/handlers/task_ws_handlers.go +++ b/apps/backend/internal/task/handlers/task_ws_handlers.go @@ -171,6 +171,9 @@ func (h *TaskHandlers) wsCreateTask(ctx context.Context, msg *ws.Message) (*ws.M }) if err != nil { h.logger.Error("failed to create task", zap.Error(err)) + if errors.Is(err, service.ErrWIPLimitExceeded) { + return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeConflict, err.Error(), nil) + } if isTaskCreateValidationError(err) { return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) } diff --git a/apps/backend/internal/task/repository/interface.go b/apps/backend/internal/task/repository/interface.go index 4bf6bb6ca2..af61895fd0 100644 --- a/apps/backend/internal/task/repository/interface.go +++ b/apps/backend/internal/task/repository/interface.go @@ -7,6 +7,7 @@ import ( agentdto "github.com/kandev/kandev/internal/agent/dto" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/repository/repoerrors" + wfmodels "github.com/kandev/kandev/internal/workflow/models" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -15,6 +16,7 @@ var ErrWorkspaceNotFound = repoerrors.ErrWorkspaceNotFound var ErrTaskNotFound = repoerrors.ErrTaskNotFound var ErrTaskPlanNotFound = repoerrors.ErrTaskPlanNotFound var ErrRepositoryNotFound = repoerrors.ErrRepositoryNotFound +var ErrWIPLimitExceeded = wfmodels.ErrWIPLimitExceeded // WorkspaceRepository handles workspace CRUD. type WorkspaceRepository interface { diff --git a/apps/backend/internal/task/repository/sqlite/task.go b/apps/backend/internal/task/repository/sqlite/task.go index 461299f020..56fbc11258 100644 --- a/apps/backend/internal/task/repository/sqlite/task.go +++ b/apps/backend/internal/task/repository/sqlite/task.go @@ -16,6 +16,7 @@ import ( "github.com/kandev/kandev/internal/db/dialect" "github.com/kandev/kandev/internal/task/models" usermodels "github.com/kandev/kandev/internal/user/models" + wfmodels "github.com/kandev/kandev/internal/workflow/models" v1 "github.com/kandev/kandev/pkg/api/v1" ) @@ -139,6 +140,20 @@ func runnerProjection(alias string) string { // (ADR 0005 Wave F); when the request carries AssigneeAgentProfileID we // upsert a 'runner' row in workflow_step_participants instead. func (r *Repository) CreateTask(ctx context.Context, task *models.Task) error { + return r.createTask(ctx, task, "", 0) +} + +// CreateTaskIfWorkflowStepHasCapacity atomically admits a task into a +// WIP-limited workflow step. The occupancy check and insert share one writer +// transaction, so concurrent watcher events cannot overfill the step. +func (r *Repository) CreateTaskIfWorkflowStepHasCapacity(ctx context.Context, task *models.Task, targetStepID string, limit int) error { + if limit <= 0 { + return r.CreateTask(ctx, task) + } + return r.createTask(ctx, task, targetStepID, limit) +} + +func (r *Repository) createTask(ctx context.Context, task *models.Task, targetStepID string, limit int) error { if task.ID == "" { task.ID = uuid.New().String() } @@ -163,6 +178,11 @@ func (r *Repository) CreateTask(ctx context.Context, task *models.Task) error { if err != nil { return err } + defer func() { _ = tx.Rollback() }() + + if err := r.ensureWorkflowStepCapacity(ctx, tx, targetStepID, limit); err != nil { + return err + } _, err = tx.ExecContext(ctx, r.db.Rebind(` INSERT INTO tasks (id, workspace_id, workflow_id, workflow_step_id, title, description, state, priority, position, metadata, is_ephemeral, parent_id, created_at, updated_at, origin, project_id, labels, identifier) @@ -187,6 +207,36 @@ func (r *Repository) CreateTask(ctx context.Context, task *models.Task) error { return tx.Commit() } +func (r *Repository) ensureWorkflowStepCapacity(ctx context.Context, tx *sql.Tx, targetStepID string, limit int) error { + if limit <= 0 { + return nil + } + if err := lockWorkflowStepForCapacity(ctx, tx, r.db.DriverName(), r.db.Rebind, targetStepID); err != nil { + return err + } + var occupants int + if err := tx.QueryRowContext(ctx, r.db.Rebind(` + SELECT COUNT(*) FROM tasks + WHERE workflow_step_id = ? + AND archived_at IS NULL + AND is_ephemeral = 0 + `), targetStepID).Scan(&occupants); err != nil { + return err + } + if occupants >= limit { + return wfmodels.NewWIPLimitError(targetStepID, limit, occupants) + } + return nil +} + +func lockWorkflowStepForCapacity(ctx context.Context, tx *sql.Tx, driver string, rebind func(string) string, stepID string) error { + if !dialect.IsPostgres(driver) { + return nil + } + var lockedID string + return tx.QueryRowContext(ctx, rebind(`SELECT id FROM workflow_steps WHERE id = ? FOR UPDATE`), stepID).Scan(&lockedID) +} + // upsertRunnerInTx writes (or replaces) a 'runner' participant row for // (stepID, taskID) inside the provided transaction. Mirrors // workflow.Repository.SetTaskRunner but reuses the caller's tx. @@ -367,6 +417,10 @@ func (r *Repository) UpdateTaskIfWorkflowStepHasCapacity(ctx context.Context, ta } defer func() { _ = tx.Rollback() }() + if err := lockWorkflowStepForCapacity(ctx, tx, r.db.DriverName(), r.db.Rebind, targetStepID); err != nil { + return err + } + var occupants int if err := tx.QueryRowContext(ctx, r.db.Rebind(` SELECT COUNT(*) FROM tasks @@ -378,7 +432,7 @@ func (r *Repository) UpdateTaskIfWorkflowStepHasCapacity(ctx context.Context, ta return err } if occupants >= limit { - return fmt.Errorf("WIP limit exceeded for workflow step %s: limit %d already occupied", targetStepID, limit) + return wfmodels.NewWIPLimitError(targetStepID, limit, occupants) } result, err := tx.ExecContext(ctx, r.db.Rebind(` diff --git a/apps/backend/internal/task/repository/task_wip_test.go b/apps/backend/internal/task/repository/task_wip_test.go new file mode 100644 index 0000000000..360c7ee396 --- /dev/null +++ b/apps/backend/internal/task/repository/task_wip_test.go @@ -0,0 +1,99 @@ +package repository + +import ( + "context" + "errors" + "fmt" + "strings" + "sync" + "testing" + + "github.com/kandev/kandev/internal/task/models" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +type workflowStepCapacityCreator interface { + CreateTaskIfWorkflowStepHasCapacity(context.Context, *models.Task, string, int) error +} + +func TestUpdateTaskIfWorkflowStepHasCapacity_ReturnsTypedWIPError(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + ctx := context.Background() + + if err := repo.CreateTask(ctx, &models.Task{ + ID: "wip-existing", WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", + WorkflowStepID: "wip-step", Title: "Existing", State: v1.TaskStateCreated, + }); err != nil { + t.Fatalf("seed existing task: %v", err) + } + candidate := &models.Task{ + ID: "wip-candidate", WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", + WorkflowStepID: "other-step", Title: "Candidate", State: v1.TaskStateCreated, + } + err := repo.UpdateTaskIfWorkflowStepHasCapacity(ctx, candidate, "wip-step", "wip-candidate", 1) + if err == nil || !errors.Is(err, wfmodels.ErrWIPLimitExceeded) { + t.Fatalf("error=%v, want typed WIP limit error", err) + } +} + +func TestCreateTaskIfWorkflowStepHasCapacity_Concurrent(t *testing.T) { + repo, cleanup := createTestSQLiteRepo(t) + defer cleanup() + + creator, ok := any(repo).(workflowStepCapacityCreator) + if !ok { + t.Fatal("task repository does not implement atomic workflow-step capacity creation") + } + + const ( + workerCount = 8 + stepID = "wip-step" + ) + ctx := context.Background() + start := make(chan struct{}) + results := make(chan error, workerCount) + var wg sync.WaitGroup + for i := 0; i < workerCount; i++ { + wg.Add(1) + go func(index int) { + defer wg.Done() + <-start + results <- creator.CreateTaskIfWorkflowStepHasCapacity(ctx, &models.Task{ + ID: fmt.Sprintf("wip-task-%d", index), + WorkspaceID: "wip-workspace", + WorkflowID: "wip-workflow", + WorkflowStepID: stepID, + Title: fmt.Sprintf("Task %d", index), + State: v1.TaskStateCreated, + }, stepID, 2) + }(i) + } + close(start) + wg.Wait() + close(results) + + created, rejected := 0, 0 + for err := range results { + if err == nil { + created++ + continue + } + if !strings.Contains(strings.ToLower(err.Error()), "wip limit exceeded") { + t.Fatalf("unexpected create error: %v", err) + } + rejected++ + } + if created != 2 || rejected != workerCount-2 { + t.Fatalf("created=%d rejected=%d, want created=2 rejected=%d", created, rejected, workerCount-2) + } + + occupants, err := repo.CountTasksByWorkflowStep(ctx, stepID) + if err != nil { + t.Fatalf("count occupants: %v", err) + } + if occupants != 2 { + t.Fatalf("occupants=%d, want 2", occupants) + } +} diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index 05cb2602a6..8df4fc8ced 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -145,6 +145,7 @@ type StartStepResolver interface { var ( ErrActiveTaskSessions = errors.New("active agent sessions exist") + ErrWIPLimitExceeded = wfmodels.ErrWIPLimitExceeded ErrInvalidRepositorySettings = errors.New("invalid repository settings") ErrInvalidExecutorConfig = errors.New("invalid executor config") // Workspace-source sentinels are the service boundary consumed by the HTTP diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index 20d94cd6a1..e3a4e3a77e 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -79,6 +79,10 @@ type taskEnvironmentOwnerTransferer interface { TransferTaskEnvironmentToTask(ctx context.Context, envID, taskID string) error } +type workflowStepCapacityTaskCreator interface { + CreateTaskIfWorkflowStepHasCapacity(ctx context.Context, task *models.Task, targetStepID string, limit int) error +} + // Task operations // isOfficeRequest returns true if the request should create an office task. @@ -133,7 +137,7 @@ func (s *Service) CreateTask(ctx context.Context, req *CreateTaskRequest) (*mode } } - if err := s.tasks.CreateTask(ctx, task); err != nil { + if err := s.createTaskWithCapacity(ctx, task); err != nil { s.logger.Error("failed to create task", zap.Error(err)) return nil, err } @@ -163,6 +167,30 @@ func (s *Service) CreateTask(ctx context.Context, req *CreateTaskRequest) (*mode return task, nil } +func (s *Service) createTaskWithCapacity(ctx context.Context, task *models.Task) error { + if task.IsEphemeral || task.WorkflowStepID == "" { + return s.tasks.CreateTask(ctx, task) + } + if s.workflowStepGetter == nil { + return s.tasks.CreateTask(ctx, task) + } + step, err := s.workflowStepGetter.GetStep(ctx, task.WorkflowStepID) + if err != nil { + return fmt.Errorf("load workflow step %s for task creation: %w", task.WorkflowStepID, err) + } + if step == nil { + return fmt.Errorf("%w: workflow step not found: %s", ErrInvalidTaskWorkflow, task.WorkflowStepID) + } + if step.WIPLimit <= 0 { + return s.tasks.CreateTask(ctx, task) + } + creator, ok := s.tasks.(workflowStepCapacityTaskCreator) + if !ok { + return fmt.Errorf("workflow step %s has WIP limit %d but capacity-aware task persistence is unavailable", step.ID, step.WIPLimit) + } + return creator.CreateTaskIfWorkflowStepHasCapacity(ctx, task, step.ID, step.WIPLimit) +} + // inheritParentRepositories fills req.Repositories from the parent task when a // subtask is created without explicit repositories. This applies to any // repo-less subtask (not only inherit_parent ones), matching the MCP diff --git a/apps/backend/internal/task/service/service_tasks_wip_test.go b/apps/backend/internal/task/service/service_tasks_wip_test.go new file mode 100644 index 0000000000..f39cbb2df3 --- /dev/null +++ b/apps/backend/internal/task/service/service_tasks_wip_test.go @@ -0,0 +1,121 @@ +package service + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + v1 "github.com/kandev/kandev/pkg/api/v1" +) + +type fixedStartStepResolver struct { + stepID string +} + +func (r fixedStartStepResolver) ResolveStartStep(context.Context, string) (string, error) { + return r.stepID, nil +} + +func (r fixedStartStepResolver) ResolveFirstStep(context.Context, string) (string, error) { + return r.stepID, nil +} + +func TestCreateTask_RejectsFullWIPStepBeforePersistence(t *testing.T) { + svc, events, repo := createTestService(t) + ctx := context.Background() + seedWIPWorkflow(t, ctx, repo) + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: map[string]*wfmodels.WorkflowStep{ + "review-step": {ID: "review-step", WorkflowID: "wip-workflow", Name: "Review", WIPLimit: 1}, + }}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "wip-occupant", WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", + WorkflowStepID: "review-step", Title: "Occupant", State: v1.TaskStateCreated, + }); err != nil { + t.Fatalf("seed occupant: %v", err) + } + + _, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", WorkflowStepID: "review-step", + Title: "Rejected", Description: "must not persist", + }) + if err == nil || !errors.Is(err, wfmodels.ErrWIPLimitExceeded) { + t.Fatalf("error=%v, want typed WIP limit rejection", err) + } + if _, err := repo.GetTask(ctx, "wip-occupant"); err != nil { + t.Fatalf("occupant disappeared: %v", err) + } + tasks, err := repo.ListTasksByWorkflowStep(ctx, "review-step") + if err != nil { + t.Fatalf("list step tasks: %v", err) + } + if len(tasks) != 1 { + t.Fatalf("step task count=%d, want 1", len(tasks)) + } + if len(events.GetPublishedEvents()) != 0 { + t.Fatalf("published events=%d, want none", len(events.GetPublishedEvents())) + } +} + +func TestCreateTask_ResolvedStartStepUsesWIPAdmission(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + seedWIPWorkflow(t, ctx, repo) + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: map[string]*wfmodels.WorkflowStep{ + "review-step": {ID: "review-step", WorkflowID: "wip-workflow", Name: "Review", WIPLimit: 1}, + }}) + svc.SetStartStepResolver(fixedStartStepResolver{stepID: "review-step"}) + if err := repo.CreateTask(ctx, &models.Task{ + ID: "wip-occupant-resolved", WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", + WorkflowStepID: "review-step", Title: "Occupant", State: v1.TaskStateCreated, + }); err != nil { + t.Fatalf("seed occupant: %v", err) + } + + _, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", + Title: "Rejected resolved start", Description: "must not persist", + }) + if err == nil || !errors.Is(err, wfmodels.ErrWIPLimitExceeded) { + t.Fatalf("error=%v, want typed WIP limit rejection", err) + } +} + +func TestCreateTask_UnlimitedWIPStepPreservesCreation(t *testing.T) { + svc, _, repo := createTestService(t) + ctx := context.Background() + seedWIPWorkflow(t, ctx, repo) + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: map[string]*wfmodels.WorkflowStep{ + "unlimited-step": {ID: "unlimited-step", WorkflowID: "wip-workflow", Name: "Unlimited"}, + }}) + + for i := 0; i < 3; i++ { + if _, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "wip-workspace", WorkflowID: "wip-workflow", WorkflowStepID: "unlimited-step", + Title: "Unlimited task", Description: "allowed", + }); err != nil { + t.Fatalf("create task %d: %v", i, err) + } + } + occupants, err := repo.CountTasksByWorkflowStep(ctx, "unlimited-step") + if err != nil { + t.Fatalf("count occupants: %v", err) + } + if occupants != 3 { + t.Fatalf("occupants=%d, want 3", occupants) + } +} + +func seedWIPWorkflow(t *testing.T, ctx context.Context, repo interface { + CreateWorkspace(context.Context, *models.Workspace) error + CreateWorkflow(context.Context, *models.Workflow) error +}) { + t.Helper() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "wip-workspace", Name: "WIP Workspace"}); err != nil { + t.Fatalf("seed workspace: %v", err) + } + if err := repo.CreateWorkflow(ctx, &models.Workflow{ID: "wip-workflow", WorkspaceID: "wip-workspace", Name: "WIP Workflow"}); err != nil { + t.Fatalf("seed workflow: %v", err) + } +} diff --git a/apps/backend/internal/task/service/service_workflow.go b/apps/backend/internal/task/service/service_workflow.go index cdf36e527b..d316ce8530 100644 --- a/apps/backend/internal/task/service/service_workflow.go +++ b/apps/backend/internal/task/service/service_workflow.go @@ -677,7 +677,7 @@ func (s *Service) validateMoveWIPLimit(ctx context.Context, task *models.Task, t return fmt.Errorf("failed to count target workflow step tasks: %w", err) } if occupants >= targetStep.WIPLimit { - return fmt.Errorf("WIP limit exceeded for workflow step %s: limit %d already occupied", targetStep.ID, targetStep.WIPLimit) + return wfmodels.NewWIPLimitError(targetStep.ID, targetStep.WIPLimit, occupants) } return nil } @@ -839,8 +839,7 @@ func (s *Service) validateBulkMoveWIPCapacity(ctx context.Context, tasks []*mode return fmt.Errorf("failed to count target workflow step tasks: %w", err) } if occupants+incoming > targetStep.WIPLimit { - return fmt.Errorf("WIP limit exceeded for workflow step %s: limit %d, occupied %d, moving %d", - targetStep.ID, targetStep.WIPLimit, occupants, incoming) + return fmt.Errorf("%w: moving %d task(s)", wfmodels.NewWIPLimitError(targetStep.ID, targetStep.WIPLimit, occupants), incoming) } return nil } diff --git a/apps/backend/internal/workflow/models/errors.go b/apps/backend/internal/workflow/models/errors.go new file mode 100644 index 0000000000..0fc7830b80 --- /dev/null +++ b/apps/backend/internal/workflow/models/errors.go @@ -0,0 +1,30 @@ +package models + +import ( + "errors" + "fmt" +) + +// ErrWIPLimitExceeded identifies a workflow-step capacity rejection. +var ErrWIPLimitExceeded = errors.New("workflow step WIP limit exceeded") + +// WIPLimitError carries workflow-step capacity details while preserving +// errors.Is(err, ErrWIPLimitExceeded) classification at service boundaries. +type WIPLimitError struct { + StepID string + Limit int + Occupied int +} + +func (e *WIPLimitError) Error() string { + return fmt.Sprintf("WIP limit exceeded for workflow step %s: limit %d already occupied (%d)", e.StepID, e.Limit, e.Occupied) +} + +func (e *WIPLimitError) Unwrap() error { + return ErrWIPLimitExceeded +} + +// NewWIPLimitError constructs a typed capacity conflict for a workflow step. +func NewWIPLimitError(stepID string, limit, occupied int) error { + return &WIPLimitError{StepID: stepID, Limit: limit, Occupied: occupied} +} diff --git a/apps/web/e2e/tests/kanban/mobile-kanban.spec.ts b/apps/web/e2e/tests/kanban/mobile-kanban.spec.ts index eacea47822..2b516d17d7 100644 --- a/apps/web/e2e/tests/kanban/mobile-kanban.spec.ts +++ b/apps/web/e2e/tests/kanban/mobile-kanban.spec.ts @@ -574,7 +574,6 @@ test.describe("Mobile kanban view", () => { is_start_step: true, }); await apiClient.createWorkflowStep(workflow.id, "Done", 1); - await apiClient.updateWorkflowStep(limitedStep.id, { wip_limit: 1 }); await apiClient.createTask(seedData.workspaceId, "Mobile WIP One", { workflow_id: workflow.id, workflow_step_id: limitedStep.id, @@ -583,6 +582,9 @@ test.describe("Mobile kanban view", () => { workflow_id: workflow.id, workflow_step_id: limitedStep.id, }); + // Seed a legacy over-limit state by applying the limit after task creation; + // creation itself must now reject capacity overflow. + await apiClient.updateWorkflowStep(limitedStep.id, { wip_limit: 1 }); await apiClient.saveUserSettings({ workspace_id: seedData.workspaceId, workflow_filter_id: workflow.id, diff --git a/docs/plans/wip-limited-task-creation/plan.md b/docs/plans/wip-limited-task-creation/plan.md new file mode 100644 index 0000000000..047d40e3b6 --- /dev/null +++ b/docs/plans/wip-limited-task-creation/plan.md @@ -0,0 +1,199 @@ +--- +spec: docs/specs/tasks/wip-limit-pull-system.md +created: 2026-07-27 +status: complete +--- + +# Implementation Plan: WIP-Limited Task Creation + +## Overview + +Task moves already enforce workflow-step WIP limits, but `CreateTask` inserts a +task directly into its resolved step without capacity admission. GitHub review +watches fan out one goroutine per newly observed PR and then auto-start every +successfully created task, so a WIP-2 start step can receive and start all eight +tasks from one poll. + +The fix adds a typed, transactionally enforced WIP admission primitive, uses it +from task creation for both explicit and resolved start steps, maps capacity +rejections consistently across HTTP, WebSocket, MCP, and watcher dispatch, and +documents the corrected behavior. Watcher integration coverage also locks in +the existing lifecycle contract: an admitted task remains in its configured +step while the agent is starting and working, and advances only on a genuine +turn-complete event. + +## Confirmed Root Cause + +- `github.Service.TriggerReviewWatch` publishes an event for every new PR. +- `orchestrator.Service.handleNewReviewPR` starts one background + `createReviewTask` goroutine per event. +- `task/service.Service.CreateTask` calls `TaskRepository.CreateTask` without + reading the resolved step's `WIPLimit`. +- WIP validation exists only on move/transition paths. +- `createReviewTask` calls `StartTask` for every task created in an + `auto_start_agent` step. + +The smallest reproduction is an empty auto-start workflow start step with +`wip_limit: 2` receiving eight concurrent non-ephemeral task creations. Current +behavior persists eight tasks; the required behavior admits exactly two. + +--- + +## Backend + +### Typed WIP capacity contract + +- Add `ErrWIPLimitExceeded` to + `apps/backend/internal/workflow/models/errors.go` so task repository, task + service, HTTP/WS/MCP adapters, and orchestrator code classify capacity + without parsing error strings. +- Preserve user-facing context by wrapping the sentinel with the step ID, + configured limit, and current occupancy. + +### Atomic repository admission + +- Extend the narrow task-placement repository contract with + `CreateTaskIfWorkflowStepHasCapacity(ctx, task, targetStepID, limit)`. +- Implement it in + `apps/backend/internal/task/repository/sqlite/task.go` using the same task + insert and runner-participant transaction as `CreateTask`. +- Serialize admission on the target `workflow_steps` row. PostgreSQL uses + `SELECT ... FOR UPDATE`; SQLite already uses a single writer connection, so + the count and insert remain in one serialized write transaction. +- Count only non-archived, non-ephemeral occupants, matching existing WIP move + semantics. +- Reuse the step-capacity lock and typed sentinel from + `UpdateTaskIfWorkflowStepHasCapacity` so creation, moves, and pulls share one + concurrency rule. + +### Task service enforcement + +- In `task/service.Service.CreateTask`, resolve and validate the final workflow + step before persistence. +- For a positive `WIPLimit`, call the capacity-aware repository method instead + of the unconditional insert. This includes requests that omit + `workflow_step_id` and resolve to the workflow start step. +- Fail closed when a positive WIP limit is configured but the repository does + not implement capacity-aware creation. +- Leave `wip_limit: 0`, workflow-less ephemeral tasks, and existing unlimited + task creation unchanged. +- Ensure a capacity rejection occurs before blockers, repositories, + `task.created`, task sessions, or auto-start side effects. + +### Caller error behavior + +- HTTP task creation returns `409 Conflict` with the WIP error. +- WebSocket and MCP task creation return `ErrorCodeConflict` with the same + message. +- GitHub review dispatch recognizes `ErrWIPLimitExceeded` as a deferral: + release the review-PR reservation, do not attach a task ID or auto-start, and + log at a non-error level so later polls can retry without false failure + noise. +- Other task-creation errors retain their current handling. + +--- + +## Tests + +- **What:** concurrent repository admission never exceeds WIP capacity. + **File:** `apps/backend/internal/task/repository/task_wip_test.go`. + **How:** seed a WIP-2 step, synchronize eight goroutines, assert exactly two + inserts and six `errors.Is(err, ErrWIPLimitExceeded)` results; run with + `-race`. +- **What:** explicit and resolved start-step creation obey WIP, while + `wip_limit: 0` remains unlimited and rejected creation emits no + `task.created`. + **File:** new + `apps/backend/internal/task/service/service_tasks_wip_test.go`. + **How:** SQLite-backed service integration tests using real workflow/task + repositories. +- **What:** HTTP, WebSocket, and MCP task creation classify WIP rejection as a + conflict. + **Files:** task handler and MCP handler tests. + **How:** focused adapter tests asserting status/error code and message. +- **What:** review-watch capacity rejection releases the PR reservation and + never assigns a task ID or auto-starts. + **File:** + `apps/backend/internal/orchestrator/event_handlers_github_review_test.go`. + **How:** deterministic fake creator returning the typed WIP error, followed + by a retry that succeeds. +- **What:** an admitted watcher task does not advance its workflow during + startup or active work. + **File:** + `apps/backend/internal/orchestrator/event_handlers_github_review_test.go`. + **How:** create directly in an auto-start `Review` step, assert the task + remains there after creation and boot-ready, then emit a real turn-complete + event and assert one transition to `Done`. + +## Frontend + +No frontend code changes are required. Existing task-creation surfaces already +display backend errors; the backend adapters must return a conflict instead of +an internal error. + +## E2E Tests + +No browser E2E is planned because there is no visual or interaction change. +The concurrency invariant and every external error boundary are covered by +targeted Go tests. + +## Public Documentation + +- Update `docs/public/tasks-and-workflows.md` and + `docs/public/workflow-tips.md` to state that WIP applies to initial task + creation, including integration-created tasks, and that full-step creation + is rejected for later retry. +- Validate the public docs after the wording changes. + +## Implementation Waves And Parallel Candidates + +All tasks are sequential because each consumes the typed contract or behavior +introduced by the preceding task. + +- [x] [Task 01: Atomic repository admission](task-01-atomic-repository-admission.md) +- [x] [Task 02: Task-service WIP enforcement](task-02-task-service-enforcement.md) +- [x] [Task 03: Conflict adapters and review-watch deferral](task-03-conflict-adapters-and-watcher-deferral.md) +- [x] [Task 04: Public documentation](task-04-public-documentation.md) + +## Completion Evidence + +- Repository admission is transactionally serialized, with PostgreSQL step-row + locking and SQLite writer serialization; focused repository tests pass. +- Task creation enforces explicit and resolved workflow-step WIP limits, while + unlimited and ephemeral creation remain compatible; focused service tests + pass. +- HTTP returns `409 Conflict`; WebSocket and MCP return conflict error codes; + watcher capacity failures release reservations and skip assignment/start. +- Review watcher lifecycle coverage confirms boot-ready does not advance the + task and the first genuine turn completion moves Review to Done once. +- Public documentation validation: `node scripts/validate-public-docs.test.mjs` + (58/58 passed). +- Backend verification: `go test -tags fts5 ./internal/task/repository + ./internal/task/service ./internal/task/handlers ./internal/mcp/handlers + ./internal/orchestrator -count=1` (2,562 tests passed). + +No task is marked `parallel-safe`; waves do not authorize subagent execution. + +## Risks + +- A pre-insert count outside the write transaction would preserve the original + race. Admission must be serialized per target step in both SQLite and + PostgreSQL. +- Rejected review PRs must release their dedup reservations or they will never + retry. +- Watcher auto-start coverage must distinguish `agent.boot_ready` from a real + turn-complete event; treating startup readiness as completion would move a + newly admitted task out of the limited step before its review runs. +- The resolved start-step path must be covered explicitly; checking only + request-supplied `workflow_step_id` would leave the reported configuration + broken. +- Existing tests intentionally seed over-limit UI states through task creation. + Those fixtures must use repository-level setup or another explicit legacy + state mechanism rather than weakening the production invariant. + +## Out of Scope + +- A GitHub review-watch `max_inflight_tasks` setting. +- Profile-wide session concurrency limits. +- Automatic feeder-step creation or watcher configuration changes. +- UI redesign or new WIP controls. diff --git a/docs/plans/wip-limited-task-creation/task-01-atomic-repository-admission.md b/docs/plans/wip-limited-task-creation/task-01-atomic-repository-admission.md new file mode 100644 index 0000000000..3693412fe0 --- /dev/null +++ b/docs/plans/wip-limited-task-creation/task-01-atomic-repository-admission.md @@ -0,0 +1,65 @@ +--- +id: "01-atomic-repository-admission" +title: "Atomic repository admission" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/tasks/wip-limit-pull-system.md" +--- + +# Task 01: Atomic Repository Admission + +## Acceptance + +- The task repository can create a task only when the target workflow step has + capacity, counting active non-ephemeral occupants with the same semantics as + WIP-limited moves. +- Eight synchronized creates against an empty WIP-2 step persist exactly two + tasks; every rejected call matches the typed WIP sentinel. +- PostgreSQL admission locks the target workflow-step row for the transaction; + SQLite admission remains serialized through its single writer. +- Existing capacity-aware updates return the same typed WIP error. + +## Verification + +```bash +cd apps/backend +go test -tags fts5 -race ./internal/task/repository/... -run 'Test.*WorkflowStep.*Capacity|Test.*WIP.*Concurrent' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/workflow/models/errors.go` +- `apps/backend/internal/task/repository/interface.go` +- `apps/backend/internal/task/repository/sqlite/task.go` +- `apps/backend/internal/task/repository/task_repository_test.go` +- `apps/backend/internal/task/repository/sqlite/task_order_test.go` or a focused + dialect test if the PostgreSQL lock query is extracted + +## Dependencies + +None. + +## Parallelism + +`sequential` + +## Inputs + +- Spec sections `What`, `Failure Modes`, and `Scenarios`. +- Existing `UpdateTaskIfWorkflowStepHasCapacity` transaction. +- SQLite writer configuration in `apps/backend/internal/db/sqlite.go`. +- PostgreSQL dialect detection in `apps/backend/internal/db/dialect`. + +## Output contract + +Mark this task `in_progress` before the RED test and `done` after GREEN and +refactor. Update `plan.md` in the same conversation and report the failing-test +evidence, repository API/transaction changes, files changed, exact test result, +and remaining database portability risks. + +## Evidence + +`go test ./internal/task/repository -run 'Test(CreateTaskIfWorkflowStepHasCapacity|UpdateTaskIfWorkflowStepHasCapacity)' -count=1` +passed (2 tests). diff --git a/docs/plans/wip-limited-task-creation/task-02-task-service-enforcement.md b/docs/plans/wip-limited-task-creation/task-02-task-service-enforcement.md new file mode 100644 index 0000000000..8652830393 --- /dev/null +++ b/docs/plans/wip-limited-task-creation/task-02-task-service-enforcement.md @@ -0,0 +1,63 @@ +--- +id: "02-task-service-enforcement" +title: "Task-service WIP enforcement" +status: done +wave: 2 +depends_on: ["01-atomic-repository-admission"] +plan: "plan.md" +spec: "../../specs/tasks/wip-limit-pull-system.md" +--- + +# Task 02: Task-Service WIP Enforcement + +## Acceptance + +- `Service.CreateTask` uses atomic capacity admission for a positive WIP limit + after resolving the final workflow step. +- Explicit and automatically resolved start-step creation reject a full step + with the typed WIP error and persist no task, repository, blocker, event, or + session side effect. +- Unlimited workflow steps and workflow-less ephemeral tasks preserve current + behavior. +- A positive WIP limit fails closed if capacity-aware persistence is + unavailable. + +## Verification + +```bash +cd apps/backend +go test -tags fts5 ./internal/task/service -run 'TestCreateTask.*WIP|TestCreateTask.*StartStep|TestCreateTask.*Unlimited' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/task/service/service_tasks.go` +- `apps/backend/internal/task/service/service_tasks_wip_test.go` +- `apps/backend/internal/task/service/service.go` only if a narrow shared + interface or helper belongs there + +## Dependencies + +Task 01. + +## Parallelism + +`sequential` + +## Inputs + +- Task 01's typed error and capacity-aware repository method. +- `resolveWorkflowStep`, `validateTaskWorkflow`, and `buildTask` in + `service_tasks.go`. +- Existing task-event test helpers in `service_events_test.go`. + +## Output contract + +Mark this task `in_progress` before the RED test and `done` after GREEN and +refactor. Update `plan.md` and report explicit/resolved-step coverage, +side-effect assertions, files changed, exact test result, blockers, and risks. + +## Evidence + +Explicit-step, resolved-start-step, unlimited, and no-event rejection tests +pass in `internal/task/service`. diff --git a/docs/plans/wip-limited-task-creation/task-03-conflict-adapters-and-watcher-deferral.md b/docs/plans/wip-limited-task-creation/task-03-conflict-adapters-and-watcher-deferral.md new file mode 100644 index 0000000000..e443e8a144 --- /dev/null +++ b/docs/plans/wip-limited-task-creation/task-03-conflict-adapters-and-watcher-deferral.md @@ -0,0 +1,78 @@ +--- +id: "03-conflict-adapters-and-watcher-deferral" +title: "Conflict adapters and review-watch deferral" +status: done +wave: 3 +depends_on: ["02-task-service-enforcement"] +plan: "plan.md" +spec: "../../specs/tasks/wip-limit-pull-system.md" +--- + +# Task 03: Conflict Adapters and Review-Watch Deferral + +## Acceptance + +- HTTP task creation returns `409 Conflict` for the typed WIP error. +- WebSocket and MCP task creation return `ErrorCodeConflict` and preserve the + actionable step/limit message. +- GitHub review task creation treats WIP rejection as deferred capacity: + releases the PR reservation, assigns no task ID, starts no task, and permits a + later retry to succeed. +- An admitted GitHub review task created directly in an auto-start `Review` + step remains in that step during startup and active work; `agent.boot_ready` + does not advance it, and the first genuine turn completion moves it to the + configured next step exactly once. +- Non-capacity task-creation failures retain their current error behavior. + +## Verification + +```bash +cd apps/backend +go test -tags fts5 ./internal/task/handlers -run 'Test.*CreateTask.*WIP' -count=1 +go test -tags fts5 ./internal/mcp/handlers -run 'TestHandleCreateTask.*WIP' -count=1 +go test -tags fts5 ./internal/orchestrator -run 'Test(CreateReviewTask_.*|GitHubWatcherTaskCreateError|ReviewWatchLifecycle_.*)' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/task/handlers/errors.go` +- `apps/backend/internal/task/handlers/task_http_handlers.go` +- `apps/backend/internal/task/handlers/task_http_handlers_test.go` +- `apps/backend/internal/task/handlers/task_ws_handlers.go` +- `apps/backend/internal/task/handlers/task_ws_handlers_test.go` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/handlers/handlers_test.go` +- `apps/backend/internal/orchestrator/event_handlers_github.go` +- `apps/backend/internal/orchestrator/event_handlers_github_review_test.go` + +## Dependencies + +Task 02. + +## Evidence + +HTTP, WebSocket, and MCP focused tests pass. GitHub review capacity rejection +coverage confirms reservation release without task assignment or auto-start; +the lifecycle regression confirms boot-ready stays in Review and the first +genuine turn completion advances once. + +## Parallelism + +`sequential` + +## Inputs + +- Task 02's service error behavior. +- Existing `isMoveConflict`, `isTaskCreateValidationError`, and WebSocket error + mappings. +- Existing review reservation release test in + `event_handlers_github_review_test.go`. +- Existing boot-ready/turn-complete separation in + `event_handlers_agent.go` and its regression tests. + +## Output contract + +Mark this task `in_progress` before the RED tests and `done` after GREEN and +refactor. Update `plan.md` and report each adapter's observed error contract, +review reservation/retry evidence, watcher lifecycle-ordering evidence, files +changed, exact test results, blockers, and risks. diff --git a/docs/plans/wip-limited-task-creation/task-04-public-documentation.md b/docs/plans/wip-limited-task-creation/task-04-public-documentation.md new file mode 100644 index 0000000000..b4eebb181c --- /dev/null +++ b/docs/plans/wip-limited-task-creation/task-04-public-documentation.md @@ -0,0 +1,56 @@ +--- +id: "04-public-documentation" +title: "Public WIP documentation" +status: done +wave: 4 +depends_on: ["03-conflict-adapters-and-watcher-deferral"] +plan: "plan.md" +spec: "../../specs/tasks/wip-limit-pull-system.md" +--- + +# Task 04: Public WIP Documentation + +## Acceptance + +- Public workflow documentation states that WIP capacity applies to initial + task creation as well as moves and transitions. +- Documentation explains that integration-created tasks rejected for capacity + remain deferred for later retry and do not auto-start. +- Public-doc validation succeeds. + +## Evidence + +Updated `docs/public/tasks-and-workflows.md` and +`docs/public/workflow-tips.md`. `node scripts/validate-public-docs.test.mjs` +passed all 58 tests. + +## Verification + +```bash +node --test scripts/validate-public-docs.test.mjs +node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `docs/public/tasks-and-workflows.md` +- `docs/public/workflow-tips.md` + +## Dependencies + +Task 03. + +## Parallelism + +`sequential` + +## Inputs + +- Amended WIP spec. +- Final typed conflict and review-watch deferral behavior from Tasks 01-03. + +## Output contract + +Mark this task `in_progress` before editing and `done` after validation. Update +`plan.md` and report the public wording changed, files changed, exact validation +results, blockers, and residual documentation risks. diff --git a/docs/public/tasks-and-workflows.md b/docs/public/tasks-and-workflows.md index ef2318bea1..224cfb6c25 100644 --- a/docs/public/tasks-and-workflows.md +++ b/docs/public/tasks-and-workflows.md @@ -182,6 +182,19 @@ New steps allow manual moves by default. **Show in command panel** also defaults | **WIP limit** | Maximum active, non-archived, non-ephemeral tasks accepted by the step. `0` means unlimited. A move into a full step is rejected; reordering within the same step does not consume another slot. | | **Pull from** | Optional feeder step. When capacity opens, Kandev pulls eligible work from that step. It requires a nonzero WIP limit. | +The WIP check also applies when a task is created. It runs for an explicit +`workflow_step_id` and for the workflow's resolved start step, and the +admission check is atomic so concurrent creates cannot collectively exceed the +limit. A full step returns a conflict that includes the step and limit; the +rejected task, session, and task-created event are not persisted. Ephemeral +tasks are not counted. + +Integration watchers use the same admission rule. For example, a GitHub review +watch targeting a `Review` step with a limit of two admits at most two newly +observed pull requests at a time. Pull requests that lose the capacity race +remain eligible for a later poll; Kandev releases their temporary watch +reservation and does not start an agent for them. + Auto-archive is checked on a five-minute background interval and uses the task's last update time. Any task update postpones eligibility, so the archive is not guaranteed at the exact configured minute. Auto-archive affects the task itself, not its children. Pull configuration rejects self-references, cycles, and cross-workflow feeders. Pulling runs when a task vacates the limited step and fills each newly open slot. Candidates are ordered by board position, then priority (`critical`, `high`, `medium`, `low`, `none`), creation time, and ID. A candidate whose move fails—for example because its session is running or starting—is skipped for that pull pass. @@ -200,6 +213,12 @@ Generic comment, blocker-resolution, approval, heartbeat, budget, and error trig When **On Turn Complete** moves a task, **Wait for agent completion signal** is available. With it enabled, a bare turn end leaves the task waiting; the agent must call `step_complete_kandev`. The call requires a summary and can include a handoff or blockers. It is idempotent within the step, runs asynchronously, and a user message sent before the transition is applied cancels that pending signal. Without the option, turn end counts as completion. +An auto-started task stays in its current step while the agent session boots and +while its first turn is running. A boot-ready event is not a turn completion. +For example, a review step with `on_enter: auto_start_agent` and +`on_turn_complete: move_to_next` moves to the next step only after the genuine +review turn completes, not during startup. + Plan mode can be disabled when the turn completes and/or when the task exits the step. A step prompt is Markdown and can include `{{task_prompt}}` to insert the original task description. ### Build a human gate diff --git a/docs/public/workflow-tips.md b/docs/public/workflow-tips.md index f3afdd7590..b4fd2bdfd9 100644 --- a/docs/public/workflow-tips.md +++ b/docs/public/workflow-tips.md @@ -70,6 +70,21 @@ Choose this for a deliberate multi-pass delivery process. It is excessive for sm Choose this for a local first-pass review. For repository-provider watch automation, use the relevant integration instead. +### GitHub review watches and WIP limits + +A GitHub Review Watch creates each matching pull request in its configured +workflow step. If that step has a WIP limit, admission is atomic: only tasks +that obtain a slot are created. An admitted task auto-starts only when its +configured workflow step enables `auto_start_agent`. A pull request rejected +because the step is full is deferred for a later poll; its temporary watch +reservation is released, so it is not lost or permanently marked as handled. + +If the target step has `auto_start_agent` on entry and `move_to_next` on turn +completion, the task remains in the target step during agent startup and active +review. `agent.boot_ready` only makes the session ready; the first real turn +completion performs the configured move once. Use a human-gate transition when +reviews must wait for approval instead of advancing automatically. + ## Build a custom workflow Choose **Add Workflow**, give it a name, select **Custom**, and save it. Expand each step to edit its behavior. Reorder steps by dragging them; transition actions that say “next” or “previous” follow the saved position order. diff --git a/docs/specs/tasks/wip-limit-pull-system.md b/docs/specs/tasks/wip-limit-pull-system.md index 32a75219d1..389e42989a 100644 --- a/docs/specs/tasks/wip-limit-pull-system.md +++ b/docs/specs/tasks/wip-limit-pull-system.md @@ -1,5 +1,5 @@ --- -status: building +status: shipped --- # WIP Limit Pull System @@ -11,9 +11,23 @@ Workflow steps can define a work-in-progress limit and an optional feeder step. and preserves existing behavior. `pull_from_step_id` is empty by default; when it is set, it must reference another step in the same workflow. -When a limited step is full, manual moves, drag/drop moves, API moves, MCP moves, -bulk moves, and workflow-engine transitions into that step are rejected instead -of overfilling the step. Same-step reordering is allowed. +When a limited step is full, new task creation, manual moves, drag/drop moves, +API moves, MCP moves, bulk moves, and workflow-engine transitions into that +step are rejected instead of overfilling the step. The creation rule applies +whether the caller names the step explicitly or Kandev resolves it as the +workflow's start step. Same-step reordering is allowed. + +Capacity admission is atomic per workflow step. Concurrent task creation and +move attempts cannot collectively admit more active, non-archived, +non-ephemeral tasks than the configured limit. + +Integration watchers use the same admission rule as interactive and API task +creation. If a GitHub review watch observes more pull requests than its target +step can accept, Kandev creates only the tasks that obtain capacity and +auto-starts those tasks only when the configured step enables +`auto_start_agent`. Pull requests rejected for capacity remain eligible for a +later poll; Kandev does not leave a task, session, repository association, or +permanent review-watch reservation for a rejected attempt. When a task leaves a limited step that has a feeder configured, Kandev attempts to pull queued tasks from the feeder into the vacated step until the step reaches @@ -52,13 +66,44 @@ ID. Moving into a full limited step returns a conflict with a user-visible message that includes the target step and limit. Optimistic UI moves must roll back. +Creating a task in a full limited step returns the same conflict classification +and does not persist any part of the rejected task. Internal automation treats +the conflict as deferred work rather than an integration failure. + If a pull attempt races with another actor that fills the slot, the pull attempt stops without overfilling the target step. Deleting a step clears any `pull_from_step_id` that points at it. -## Scope Note +## Scenarios -`agent_profiles.max_concurrent_sessions` is related but separate. This feature -adds per-step WIP limits and pull behavior. Kanban enforcement of profile-level -session caps remains tracked separately unless implemented in the same change. +- **GIVEN** a workflow step with `wip_limit: 2` and two active tasks, **WHEN** a + user, API client, MCP client, or integration watcher creates another + non-ephemeral task directly in that step, **THEN** creation is rejected as a + WIP-capacity conflict and no task or session is persisted. +- **GIVEN** a workflow whose limited start step has `wip_limit: 2`, **WHEN** a + caller creates a task without specifying `workflow_step_id` and the start + step is full, **THEN** creation is rejected by the same capacity rule. +- **GIVEN** an empty auto-start step with `wip_limit: 2`, **WHEN** a GitHub + review watch concurrently dispatches eight newly observed pull requests, + **THEN** exactly two review tasks obtain capacity and auto-start, while the + other six pull requests retain no task or reservation and remain eligible for + a later poll. +- **GIVEN** a review watch targets a `Review` step with + `on_enter: auto_start_agent` and `on_turn_complete: move_to_next`, **WHEN** an + admitted review task starts, **THEN** it remains in `Review` throughout agent + startup and the active turn, ignores boot-ready events for workflow + advancement, and moves to the next step exactly once only after the real + agent turn completes. +- **GIVEN** the review-watch scenario above and one accepted review task later + leaves the limited step, **WHEN** the watch polls again, **THEN** one + previously deferred pull request can obtain the newly available capacity. +- **GIVEN** a step with `wip_limit: 0`, **WHEN** callers create tasks + concurrently in that step, **THEN** creation remains unlimited. + +## Out of scope + +- WIP limits do not replace `agent_profiles.max_concurrent_sessions` or impose + a profile-wide execution budget. +- This change does not add a separate GitHub review-watch + `max_inflight_tasks` setting.