diff --git a/apps/backend/cmd/preview/sprite_ops.go b/apps/backend/cmd/preview/sprite_ops.go index a8933c0650..cb06befc64 100644 --- a/apps/backend/cmd/preview/sprite_ops.go +++ b/apps/backend/cmd/preview/sprite_ops.go @@ -16,7 +16,7 @@ import ( const ( spriteUploadTimeout = 10 * time.Minute // bundles can be large spriteStepTimeout = 2 * time.Minute - spriteControlRetries = 3 + spriteControlRetries = 5 spriteUploadRetries = 3 spriteBackoffInit = 700 * time.Millisecond spriteRetryMaxDelay = 30 * time.Second diff --git a/apps/backend/cmd/preview/sprite_ops_test.go b/apps/backend/cmd/preview/sprite_ops_test.go index c4e3a9a853..8dc6ad1fcf 100644 --- a/apps/backend/cmd/preview/sprite_ops_test.go +++ b/apps/backend/cmd/preview/sprite_ops_test.go @@ -55,6 +55,33 @@ func TestGetOrCreateSpriteRetriesTransientGet(t *testing.T) { } } +func TestGetOrCreateSpriteRetriesBeyondThreeTransientErrors(t *testing.T) { + getCalls := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + getCalls++ + if getCalls <= 3 { + w.WriteHeader(http.StatusServiceUnavailable) + return + } + _, _ = w.Write([]byte(`{"name":"kandev-pr-3895","status":"created"}`)) + })) + t.Cleanup(server.Close) + + client := sprites.New("token", sprites.WithBaseURL(server.URL)) + t.Cleanup(func() { _ = client.Close() }) + + sprite, err := getOrCreateSprite(t.Context(), client, "kandev-pr-3895") + if err != nil { + t.Fatalf("getOrCreateSprite() error after transient outage = %v", err) + } + if sprite.Name() != "kandev-pr-3895" { + t.Errorf("sprite.Name() = %q", sprite.Name()) + } + if getCalls != 4 { + t.Errorf("get calls = %d, want 4 after three transient failures", getCalls) + } +} + func TestGetOrCreateSpriteReconcilesTransientCreateFailure(t *testing.T) { getCalls := 0 createCalls := 0 diff --git a/apps/backend/internal/analytics/repository/sqlite/stats_aggregate_parity_test.go b/apps/backend/internal/analytics/repository/sqlite/stats_aggregate_parity_test.go index a6fff11ada..544a0514c5 100644 --- a/apps/backend/internal/analytics/repository/sqlite/stats_aggregate_parity_test.go +++ b/apps/backend/internal/analytics/repository/sqlite/stats_aggregate_parity_test.go @@ -53,7 +53,10 @@ func TestStatsRepositoryAndDailyAggregatesAvoidTurnsMessagesProduct(t *testing.T t.Fatalf("NewWithDB failed: %v", err) } - now := time.Now().UTC() + // Keep every seeded turn inside the current UTC day so the daily query's + // date range includes the complete fixture even when the test runs near UTC + // midnight. + now := time.Now().UTC().Truncate(24 * time.Hour).Add(12 * time.Hour) execOrFatal(t, dbConn, `INSERT INTO workspaces (id, name, created_at, updated_at) VALUES ('ws-heavy', 'Heavy', ?, ?)`, now, now) execOrFatal(t, dbConn, `INSERT INTO repositories (id, workspace_id, name, created_at, updated_at) VALUES ('repo-heavy', 'ws-heavy', 'Heavy', ?, ?)`, now, now) execOrFatal(t, dbConn, `INSERT INTO tasks (id, workspace_id, board_id, title, is_ephemeral, created_at, updated_at) VALUES ('task-heavy', 'ws-heavy', 'board-heavy', 'Heavy', 0, ?, ?)`, now, now) diff --git a/apps/backend/internal/gateway/websocket/lsp_lease.go b/apps/backend/internal/gateway/websocket/lsp_lease.go index 80232842a7..18fba51fa6 100644 --- a/apps/backend/internal/gateway/websocket/lsp_lease.go +++ b/apps/backend/internal/gateway/websocket/lsp_lease.go @@ -76,6 +76,7 @@ type lspLease struct { detachedAt time.Time expiryTimer *time.Timer closed bool + expectedUpstreamClose bool readDone chan struct{} ready bool readyStatus map[string]any @@ -615,6 +616,12 @@ func (l *lspLease) readUpstream() { for { messageType, message, err := l.upstream.ReadMessage() if err != nil { + l.mu.Lock() + expectedClose := l.expectedUpstreamClose + l.mu.Unlock() + if expectedClose { + return + } code, text, reason := classifyLSPUpstreamClose(err) l.terminate(code, text, reason) return diff --git a/apps/backend/internal/gateway/websocket/lsp_lease_broker.go b/apps/backend/internal/gateway/websocket/lsp_lease_broker.go index b20fa0e048..cbea86a2d6 100644 --- a/apps/backend/internal/gateway/websocket/lsp_lease_broker.go +++ b/apps/backend/internal/gateway/websocket/lsp_lease_broker.go @@ -31,6 +31,9 @@ func (l *lspLease) gracefulRelease(generation uint64, reason, requestID string) if _, err := l.sendBrokerRequest(ctx, "shutdown", nil); err != nil { l.manager.logger.Debug("LSP shutdown request did not complete before release", zap.String("language", l.language), zap.Error(err)) } + l.mu.Lock() + l.expectedUpstreamClose = true + l.mu.Unlock() _ = l.writeUpstream(jsonRPCNotification("exit", nil)) if err := l.writeBrowser(generation, map[string]any{ lspControlField: lspControlKind, diff --git a/apps/backend/internal/gateway/websocket/lsp_lease_test.go b/apps/backend/internal/gateway/websocket/lsp_lease_test.go index 0e395c9499..698a249bc8 100644 --- a/apps/backend/internal/gateway/websocket/lsp_lease_test.go +++ b/apps/backend/internal/gateway/websocket/lsp_lease_test.go @@ -648,9 +648,16 @@ func TestLSPContinuityReconnectsToSameTaskHostStream(t *testing.T) { logger: testLogger(), } var fenceHeld atomic.Bool + fenceReleased := make(chan struct{}, 1) handler.EnableContinuity(func(string) func() { fenceHeld.Store(true) - return func() { fenceHeld.Store(false) } + return func() { + fenceHeld.Store(false) + select { + case fenceReleased <- struct{}{}: + default: + } + } }, nil) t.Cleanup(func() { _ = handler.Close() }) @@ -660,6 +667,11 @@ func TestLSPContinuityReconnectsToSameTaskHostStream(t *testing.T) { if leaseID == "" || ready["resumed"] != false { t.Fatalf("first ready status = %v, want an initial lease ID and resumed=false", ready) } + select { + case <-fenceReleased: + case <-time.After(wsTestTimeout): + t.Fatal("session lifecycle fence was not released after lease admission") + } if fenceHeld.Load() { t.Fatal("session lifecycle fence remained held after lease admission") } diff --git a/apps/backend/internal/orchestrator/event_handlers_workflow.go b/apps/backend/internal/orchestrator/event_handlers_workflow.go index 29b0f50cf5..fc2de7476d 100644 --- a/apps/backend/internal/orchestrator/event_handlers_workflow.go +++ b/apps/backend/internal/orchestrator/event_handlers_workflow.go @@ -4135,35 +4135,69 @@ func (s *Service) preflightWorkflowStepCredentials( taskID string, currentSession *models.TaskSession, targetStep *wfmodels.WorkflowStep, +) error { + return s.preflightWorkflowStepCredentialsWithCandidate(ctx, taskID, nil, currentSession, targetStep) +} + +func (s *Service) preflightWorkflowStepCredentialsWithCandidate( + ctx context.Context, + taskID string, + candidate *models.Task, + currentSession *models.TaskSession, + targetStep *wfmodels.WorkflowStep, ) error { if currentSession == nil || targetStep == nil { return nil } ctx = withWorkflowMetaCache(ctx) if targetStep.SessionTarget != nil { - resolution, err := s.resolveWorkflowSessionTarget(ctx, taskID, targetStep) - if err != nil { - return err - } - targetSession := currentSession - if s.resolveStepProfileSessionStartPolicy(targetStep) == models.WorkflowProfileSessionStartPolicyReuse && resolution.session != nil { - targetSession = resolution.session - } - task, err := s.repo.GetTask(ctx, taskID) - if err != nil { - return fmt.Errorf("get task for explicit credential preflight: %w", err) - } - if task == nil { - return fmt.Errorf("task %s not found for explicit credential preflight", taskID) - } - return s.executor.PreflightManagedGitCredentials( - ctx, task.WorkspaceID, taskID, targetSession.ExecutorID, targetSession.ExecutorProfileID, - ) + return s.preflightExplicitWorkflowStepCredentials(ctx, taskID, candidate, currentSession, targetStep) } - effectiveProfile, err := s.resolveStepAgentProfileForTaskID(ctx, taskID, targetStep) + effectiveProfile, err := s.resolveWorkflowMovePreflightProfile(ctx, taskID, candidate, targetStep) if err != nil { return err } + return s.preflightFixedWorkflowStepCredentials(ctx, taskID, candidate, currentSession, targetStep, effectiveProfile) +} + +func (s *Service) preflightExplicitWorkflowStepCredentials( + ctx context.Context, + taskID string, + candidate *models.Task, + currentSession *models.TaskSession, + targetStep *wfmodels.WorkflowStep, +) error { + var resolution workflowSessionTargetResolution + var err error + if candidate != nil { + resolution, err = s.resolveWorkflowSessionTargetWithTask(ctx, taskID, targetStep, candidate) + } else { + resolution, err = s.resolveWorkflowSessionTarget(ctx, taskID, targetStep) + } + if err != nil { + return err + } + targetSession := currentSession + if s.resolveStepProfileSessionStartPolicy(targetStep) == models.WorkflowProfileSessionStartPolicyReuse && resolution.session != nil { + targetSession = resolution.session + } + task, err := s.preflightTaskProjection(ctx, taskID, candidate) + if err != nil { + return fmt.Errorf("get task for explicit credential preflight: %w", err) + } + return s.executor.PreflightManagedGitCredentials( + ctx, task.WorkspaceID, taskID, targetSession.ExecutorID, targetSession.ExecutorProfileID, + ) +} + +func (s *Service) preflightFixedWorkflowStepCredentials( + ctx context.Context, + taskID string, + candidate *models.Task, + currentSession *models.TaskSession, + targetStep *wfmodels.WorkflowStep, + effectiveProfile string, +) error { startPolicy := s.resolveStepProfileSessionStartPolicy(targetStep) if shouldKeepCurrentWorkflowStepSession(effectiveProfile, currentSession.AgentProfileID, startPolicy) { if effectiveProfile == "" || startPolicy != models.WorkflowProfileSessionStartPolicyReuse { @@ -4187,18 +4221,41 @@ func (s *Service) preflightWorkflowStepCredentials( targetSession = existing } } - task, err := s.repo.GetTask(ctx, taskID) + task, err := s.preflightTaskProjection(ctx, taskID, candidate) if err != nil { return fmt.Errorf("get task for credential preflight: %w", err) } - if task == nil { - return fmt.Errorf("task %s not found for credential preflight", taskID) - } return s.executor.PreflightManagedGitCredentials( ctx, task.WorkspaceID, taskID, targetSession.ExecutorID, targetSession.ExecutorProfileID, ) } +func (s *Service) preflightTaskProjection(ctx context.Context, taskID string, candidate *models.Task) (*models.Task, error) { + if candidate != nil { + return candidate, nil + } + task, err := s.repo.GetTask(ctx, taskID) + if err != nil { + return nil, err + } + if task == nil { + return nil, fmt.Errorf("task %s not found for credential preflight", taskID) + } + return task, nil +} + +func (s *Service) resolveWorkflowMovePreflightProfile( + ctx context.Context, + taskID string, + candidate *models.Task, + targetStep *wfmodels.WorkflowStep, +) (string, error) { + if candidate != nil { + return s.resolveStepAgentProfileForTask(ctx, candidate, targetStep), nil + } + return s.resolveStepAgentProfileForTaskID(ctx, taskID, targetStep) +} + // PreflightWorkflowStepMove exposes the destination lifecycle preflight to // the task service, which must run it before committing a manual move. func (s *Service) PreflightWorkflowStepMove( @@ -4210,6 +4267,20 @@ func (s *Service) PreflightWorkflowStepMove( return s.preflightWorkflowStepCredentials(ctx, taskID, currentSession, targetStep) } +// PreflightWorkflowStepChange validates credentials against a candidate task +// carrying the form's normalized destination profile choices. +func (s *Service) PreflightWorkflowStepChange( + ctx context.Context, + candidate *models.Task, + currentSession *models.TaskSession, + targetStep *wfmodels.WorkflowStep, +) error { + if candidate == nil { + return fmt.Errorf("workflow change candidate is required") + } + return s.preflightWorkflowStepCredentialsWithCandidate(ctx, candidate.ID, candidate, currentSession, targetStep) +} + // maybySwitchSessionForProfile preserves the legacy processOnEnter failure // handling while sharing the error-returning step-entry preflight with direct // workflow-engine dispatch. diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index f4bc66b2d2..f2f3141a95 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -224,7 +224,8 @@ var ( // concurrent terminal session transition won the persistence race. Callers // must not start the process and must arbitrate exact-execution teardown // ownership before deciding whether to force-stop the registered runtime. - ErrSessionStateSuperseded = errors.New("session state superseded by terminal transition") + ErrSessionStateSuperseded = errors.New("session state superseded by terminal transition") + errSessionAdvancedToRunning = errors.New("session state advanced to RUNNING before runtime persistence") // ErrOrphanRecoveryIncomplete means StopByTaskID stopped every session it // found but could not load at least one registry-only orphan's row, so the // task-scoped stop is not fully confirmed. Callers that already observed a diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index 008447c245..87a1cf9d0c 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -849,6 +849,15 @@ func (e *Executor) persistSessionFullRowIfCurrentState( if isStopTerminalSessionState(current.State) { return &SessionStateSupersededError{SessionID: session.ID, State: current.State} } + if expected == models.TaskSessionStateStarting && current.State == models.TaskSessionStateRunning { + return fmt.Errorf( + "%w: session %s state changed from %s to %s before runtime persistence", + errSessionAdvancedToRunning, + session.ID, + expected, + current.State, + ) + } return fmt.Errorf( "session %s state changed from %s to %s before runtime persistence", session.ID, @@ -1753,10 +1762,6 @@ func (e *Executor) LaunchPreparedSession(ctx context.Context, task *v1.Task, ses return nil, err } - if startAgent { - e.observeSessionCoresidency(launchCtx, sessionCoresidencySiteLaunch, task.ID, sessionID) - } - // Fast path: workspace already launched (executors_running row exists). // The selected-environment recovery gate above must run first, including // when this path only starts an agent process on an existing workspace. diff --git a/apps/backend/internal/orchestrator/executor/executor_launch_state_test.go b/apps/backend/internal/orchestrator/executor/executor_launch_state_test.go new file mode 100644 index 0000000000..50b47869c9 --- /dev/null +++ b/apps/backend/internal/orchestrator/executor/executor_launch_state_test.go @@ -0,0 +1,48 @@ +package executor + +import ( + "context" + "testing" + "time" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle" + "github.com/kandev/kandev/internal/task/models" + "github.com/stretchr/testify/require" +) + +func TestPersistLaunchStateKeepsRunningSessionAndPrepareResult(t *testing.T) { + ctx := context.Background() + repo := newMockRepository() + repo.sessions["session-1"] = &models.TaskSession{ + ID: "session-1", + TaskID: "task-1", + State: models.TaskSessionStateRunning, + Metadata: map[string]interface{}{"existing": "preserved"}, + } + exec := newTestExecutor(t, &mockAgentManager{}, repo) + launchSession := &models.TaskSession{ + ID: "session-1", + TaskID: "task-1", + State: models.TaskSessionStateStarting, + } + prepareResult := &lifecycle.EnvPrepareResult{Success: true, WorkspacePath: "/workspace/task-1"} + + err := exec.persistLaunchState( + ctx, + "task-1", + "session-1", + launchSession, + &LaunchAgentResponse{PrepareResult: prepareResult}, + true, + time.Now(), + ) + require.NoError(t, err) + + updated, err := repo.GetTaskSession(ctx, "session-1") + require.NoError(t, err) + require.Equal(t, models.TaskSessionStateRunning, updated.State) + require.Equal(t, "preserved", updated.Metadata["existing"]) + prepareMetadata, ok := updated.Metadata["prepare_result"].(map[string]interface{}) + require.True(t, ok) + require.Equal(t, "completed", prepareMetadata["status"]) +} diff --git a/apps/backend/internal/orchestrator/executor/executor_resume.go b/apps/backend/internal/orchestrator/executor/executor_resume.go index 5365926d89..44237aa449 100644 --- a/apps/backend/internal/orchestrator/executor/executor_resume.go +++ b/apps/backend/internal/orchestrator/executor/executor_resume.go @@ -985,6 +985,17 @@ func (e *Executor) persistLaunchState(ctx context.Context, taskID, sessionID str updateErr = e.persistSessionFullRowIfCurrentState(ctx, session, expectedState) } if updateErr != nil { + if startAgent && errors.Is(updateErr, errSessionAdvancedToRunning) { + if resp.PrepareResult == nil || !resp.PrepareResult.Success { + return nil + } + return e.repo.SetSessionMetadataKey( + ctx, + sessionID, + "prepare_result", + buildPrepareResultMetadata(resp.PrepareResult), + ) + } e.logger.Error("failed to update agent session after launch", zap.String("task_id", taskID), zap.String("session_id", sessionID), diff --git a/apps/backend/internal/orchestrator/executor/session_coresidency_test.go b/apps/backend/internal/orchestrator/executor/session_coresidency_test.go index a6fbb16113..c940afb921 100644 --- a/apps/backend/internal/orchestrator/executor/session_coresidency_test.go +++ b/apps/backend/internal/orchestrator/executor/session_coresidency_test.go @@ -149,10 +149,9 @@ func TestObserveSessionCoresidency_SiblingReadFailureRecordsSkipNotAbsence(t *te } // TestLaunchPreparedSession_ObservesWorkingSiblingOnAgentStart pins the -// wiring half of AC-004.1: LaunchPreparedSession calls the observation -// before the agent process starts, for a real launch that otherwise -// succeeds, not just the extracted helper. Ordering is enforced by -// production code structure, not a synchronized assertion here. +// LaunchPreparedSession wiring through the shared process-start hook. It waits +// until StartAgentProcess is called, after the observation has run, and checks +// that this launch recorded exactly one local warning. func TestLaunchPreparedSession_ObservesWorkingSiblingOnAgentStart(t *testing.T) { repo := newMockRepository() repo.sessions["session-123"] = &models.TaskSession{ @@ -170,6 +169,7 @@ func TestLaunchPreparedSession_ObservesWorkingSiblingOnAgentStart(t *testing.T) if err != nil { t.Fatalf("NewFromZap: %v", err) } + startCalled := make(chan struct{}, 1) agentManager := &mockAgentManager{ launchAgentFunc: func(ctx context.Context, req *LaunchAgentRequest) (*LaunchAgentResponse, error) { return &LaunchAgentResponse{ @@ -177,6 +177,10 @@ func TestLaunchPreparedSession_ObservesWorkingSiblingOnAgentStart(t *testing.T) ContainerID: "container-123", }, nil }, + startAgentProcessFunc: func(context.Context, string) error { + startCalled <- struct{}{} + return nil + }, } executor := NewExecutor(agentManager, repo, log, ExecutorConfig{ShellPrefs: &mockShellPrefs{}}) executor.SetCapabilities(&mockCapabilities{}) @@ -196,13 +200,24 @@ func TestLaunchPreparedSession_ObservesWorkingSiblingOnAgentStart(t *testing.T) }); err != nil { t.Fatalf("LaunchPreparedSession failed: %v", err) } - - if after := counterValue(sessionCoresidencyAdmittedTotalVar, sessionCoresidencySiteLaunch); after != before+1 { - t.Fatalf("admitted[launch] counter = %d, want %d", after, before+1) + select { + case <-startCalled: + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for StartAgentProcess") + } + + if after := counterValue(sessionCoresidencyAdmittedTotalVar, sessionCoresidencySiteLaunch); after < before+1 { + t.Fatalf("admitted[launch] counter = %d, want at least %d", after, before+1) + } + const observationMessage = "starting an agent while another session of this task is already working in the shared worktree" + var warnings []observer.LoggedEntry + for _, entry := range logs.All() { + if strings.HasPrefix(entry.Message, observationMessage) { + warnings = append(warnings, entry) + } } - warnings := logs.FilterLevelExact(zapcore.WarnLevel).All() if len(warnings) != 1 { - t.Fatalf("warning entries = %d, want 1; all=%v", len(warnings), logs.All()) + t.Fatalf("co-residency warning entries = %d, want 1; all=%v", len(warnings), logs.All()) } fields := warnings[0].ContextMap() if fields["site"] != sessionCoresidencySiteLaunch { diff --git a/apps/backend/internal/orchestrator/workflow_move_preview.go b/apps/backend/internal/orchestrator/workflow_move_preview.go index f55942722a..07f3d63b3e 100644 --- a/apps/backend/internal/orchestrator/workflow_move_preview.go +++ b/apps/backend/internal/orchestrator/workflow_move_preview.go @@ -10,6 +10,8 @@ import ( "github.com/kandev/kandev/internal/agent/agents" "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + taskservice "github.com/kandev/kandev/internal/task/service" wfmodels "github.com/kandev/kandev/internal/workflow/models" workflowmove "github.com/kandev/kandev/internal/workflow/move" ) @@ -19,10 +21,12 @@ import ( // source session field: the server resolves the current session exactly as // the move path does. type WorkflowMovePreviewRequest struct { - TaskID string - WorkflowID string - WorkflowStepID string - EntryOptions *workflowmove.EntryOptions + TaskID string + WorkflowID string + WorkflowStepID string + EntryOptions *workflowmove.EntryOptions + WorkflowChange *models.WorkflowChangeRequest + CandidateWorkflowOverrides *models.WorkflowAgentOverrides } type WorkflowMovePreviewOutcome string @@ -177,6 +181,16 @@ func (s *Service) PreviewWorkflowMove(ctx context.Context, request WorkflowMoveP if task.ArchivedAt != nil { return nil, fmt.Errorf("archived tasks cannot be moved") } + if request.WorkflowChange != nil { + if task.WorkflowID != request.WorkflowChange.ExpectedWorkflowID || + task.WorkflowStepID != request.WorkflowChange.ExpectedStepID || + !task.UpdatedAt.Equal(request.WorkflowChange.ExpectedUpdatedAt) { + return nil, repoerrors.ErrWorkflowChangeConflict + } + if request.CandidateWorkflowOverrides != nil && request.CandidateWorkflowOverrides.WorkflowID != request.WorkflowID { + return nil, &taskservice.WorkflowChangeValidationError{Code: taskservice.WorkflowChangeErrorInvalid} + } + } destination, err := s.workflowStepGetter.GetStep(ctx, request.WorkflowStepID) if err != nil { @@ -236,20 +250,28 @@ func (s *Service) resolveWorkflowMovePreviewInput( input.OriginalSession = previewOriginalTaskSessionFromTask(task, sessions) } - s.resolveWorkflowMovePreviewRecipient(ctx, request.TaskID, task, destination, &input) + profileTask := task + if request.WorkflowChange != nil { + candidate := *task + candidate.WorkflowID = request.WorkflowID + candidate.WorkflowStepID = request.WorkflowStepID + candidate.WorkflowAgentOverrides = request.CandidateWorkflowOverrides + profileTask = &candidate + } + s.resolveWorkflowMovePreviewRecipient(ctx, request.TaskID, profileTask, destination, &input) return input, nil } func (s *Service) resolveWorkflowMovePreviewRecipient( ctx context.Context, taskID string, - task *models.Task, + profileTask *models.Task, destination *wfmodels.WorkflowStep, input *workflowMovePreviewInput, ) { if destination.SessionTarget != nil { input.ExplicitTarget = true - target, profileID, err := s.resolvePreviewWorkflowSessionTarget(ctx, taskID, task, destination) + target, profileID, err := s.resolvePreviewWorkflowSessionTarget(ctx, taskID, profileTask, destination) if err != nil { input.Notices = append(input.Notices, workflowMovePreviewNotice("target_unavailable", nil)) } else { @@ -257,7 +279,7 @@ func (s *Service) resolveWorkflowMovePreviewRecipient( input.TargetProfileID = profileID } } else { - profileID, err := s.previewStepAgentProfile(ctx, destination, task, input.SourceSession == nil) + profileID, err := s.previewStepAgentProfile(ctx, destination, profileTask, input.SourceSession == nil) input.TargetProfileID = profileID if err != nil { input.Notices = append(input.Notices, workflowMovePreviewNotice("profile_unavailable", nil)) @@ -333,11 +355,13 @@ func (s *Service) resolvePreviewWorkflowSessionTarget( if err := validateWorkflowSessionTargetSource(step, sourceStep); err != nil { return nil, "", err } - session, err := s.resolveBoundSourceWorkflowSession(ctx, taskID, step.WorkflowID, sourceStep) + profileID, err := s.previewStepAgentProfile(ctx, sourceStep, task, true) if err != nil { return nil, "", err } - profileID, err := s.previewStepAgentProfile(ctx, sourceStep, task, true) + session, err := s.resolveBoundSourceWorkflowSessionWithProfile( + ctx, taskID, step.WorkflowID, sourceStep, profileID, + ) if err != nil { return nil, "", err } diff --git a/apps/backend/internal/orchestrator/workflow_move_preview_test.go b/apps/backend/internal/orchestrator/workflow_move_preview_test.go index aaa3eb9981..3b468223f6 100644 --- a/apps/backend/internal/orchestrator/workflow_move_preview_test.go +++ b/apps/backend/internal/orchestrator/workflow_move_preview_test.go @@ -9,6 +9,7 @@ import ( "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/orchestrator/watcher" "github.com/kandev/kandev/internal/task/models" + taskservice "github.com/kandev/kandev/internal/task/service" wfmodels "github.com/kandev/kandev/internal/workflow/models" workflowmove "github.com/kandev/kandev/internal/workflow/move" v1 "github.com/kandev/kandev/pkg/api/v1" @@ -33,6 +34,96 @@ func TestPreviewStepAgentProfileUsesTaskWorkflowOverride(t *testing.T) { } } +func TestPreviewWorkflowMoveUsesDraftOverrideWithoutWriting(t *testing.T) { + ctx := context.Background() + repo := setupTestRepo(t) + now := time.Now().UTC() + if err := repo.CreateWorkspace(ctx, &models.Workspace{ID: "workspace-preview-change", Name: "Preview change", CreatedAt: now, UpdatedAt: now}); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + for _, workflow := range []*models.Workflow{ + {ID: "workflow-preview-source", WorkspaceID: "workspace-preview-change", Name: "Source"}, + {ID: "workflow-preview-target", WorkspaceID: "workspace-preview-change", Name: "Target"}, + } { + if err := repo.CreateWorkflow(ctx, workflow); err != nil { + t.Fatalf("CreateWorkflow(%s): %v", workflow.ID, err) + } + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-preview-change", WorkspaceID: "workspace-preview-change", + WorkflowID: "workflow-preview-source", WorkflowStepID: "preview-source-step", + Title: "Preview task", State: v1.TaskStateCreated, + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + task, err := repo.GetTask(ctx, "task-preview-change") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + steps := newMockStepGetter() + steps.steps["preview-source-step"] = &wfmodels.WorkflowStep{ID: "preview-source-step", WorkflowID: task.WorkflowID} + steps.steps["preview-target-step"] = &wfmodels.WorkflowStep{ + ID: "preview-target-step", WorkflowID: "workflow-preview-target", AgentProfileID: "workflow-default-profile", + Events: wfmodels.StepEvents{OnEnter: []wfmodels.OnEnterAction{{Type: wfmodels.OnEnterAutoStartAgent}}}, + } + svc := createTestServiceWithAgent(repo, steps, newMockTaskRepo(), &mockAgentManager{ + resolveProfileInfo: &executor.AgentProfileInfo{ProfileID: "replacement-profile", ProfileName: "Replacement", Model: "model-b"}, + }) + draftOverrides, err := models.NewWorkflowAgentOverrides("workflow-preview-target", []models.WorkflowAgentOverrideBinding{{ + StepID: "preview-target-step", SourceProfileID: "workflow-default-profile", ReplacementProfileID: "replacement-profile", + }}) + if err != nil { + t.Fatalf("NewWorkflowAgentOverrides: %v", err) + } + preview, err := svc.PreviewWorkflowMove(ctx, WorkflowMovePreviewRequest{ + TaskID: "task-preview-change", WorkflowID: "workflow-preview-target", WorkflowStepID: "preview-target-step", + WorkflowChange: &models.WorkflowChangeRequest{ + ExpectedWorkflowID: task.WorkflowID, ExpectedStepID: task.WorkflowStepID, ExpectedUpdatedAt: task.UpdatedAt, + AgentOverrides: map[string]string{"workflow-default-profile": "replacement-profile"}, + }, + CandidateWorkflowOverrides: draftOverrides, + }) + if err != nil { + t.Fatalf("PreviewWorkflowMove: %v", err) + } + if preview.Recipient == nil || preview.Recipient.ProfileID != "replacement-profile" || preview.Recipient.ProfileName != "Replacement" { + t.Fatalf("draft preview recipient = %#v, want replacement profile", preview.Recipient) + } + stored, err := repo.GetTask(ctx, task.ID) + if err != nil { + t.Fatalf("GetTask after preview: %v", err) + } + if stored.WorkflowID != task.WorkflowID || stored.WorkflowStepID != task.WorkflowStepID || stored.WorkflowAgentOverrides != nil { + t.Fatalf("preview persisted draft state: %+v", stored) + } +} + +func TestPreviewWorkflowMoveRejectsCandidateOverridesForAnotherWorkflowAsValidation(t *testing.T) { + fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) + task, err := fixture.repo.GetTask(context.Background(), "t1") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + wrongWorkflowOverrides, err := models.NewWorkflowAgentOverrides("other-workflow", []models.WorkflowAgentOverrideBinding{{ + StepID: "some-step", SourceProfileID: "profile-a", ReplacementProfileID: "profile-b", + }}) + if err != nil { + t.Fatalf("NewWorkflowAgentOverrides: %v", err) + } + _, err = fixture.svc.PreviewWorkflowMove(context.Background(), WorkflowMovePreviewRequest{ + TaskID: "t1", WorkflowID: "wf1", WorkflowStepID: "step-a", + WorkflowChange: &models.WorkflowChangeRequest{ + ExpectedWorkflowID: task.WorkflowID, ExpectedStepID: task.WorkflowStepID, + ExpectedUpdatedAt: task.UpdatedAt, AgentOverrides: map[string]string{}, + }, + CandidateWorkflowOverrides: wrongWorkflowOverrides, + }) + var validationErr *taskservice.WorkflowChangeValidationError + if !errors.As(err, &validationErr) || validationErr.Code != taskservice.WorkflowChangeErrorInvalid { + t.Fatalf("PreviewWorkflowMove error = %#v, want invalid workflow-change validation", err) + } +} + func TestPreviewWorkflowSessionTargetUsesTaskWorkflowOverride(t *testing.T) { fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) overrides, err := models.NewWorkflowAgentOverrides("wf1", []models.WorkflowAgentOverrideBinding{ @@ -69,6 +160,133 @@ func TestPreviewWorkflowSessionTargetUsesTaskWorkflowOverride(t *testing.T) { } } +func TestPreviewWorkflowMoveUsesDraftProfileForExplicitStepTarget(t *testing.T) { + for _, tc := range []struct { + name string + bindingProfileID string + wantSessionID string + wantPreviewOutcome WorkflowMovePreviewOutcome + }{ + {name: "no existing binding", wantPreviewOutcome: WorkflowMovePreviewOutcomeCreateNew}, + {name: "candidate profile binding", bindingProfileID: "profile-b", wantSessionID: "session-b", wantPreviewOutcome: WorkflowMovePreviewOutcomeReuseOther}, + {name: "stale source profile binding", bindingProfileID: "profile-a", wantPreviewOutcome: WorkflowMovePreviewOutcomeCreateNew}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) + now := time.Now().UTC() + if err := fixture.repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-destination", WorkspaceID: "ws1", Name: "Destination", CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + task, err := fixture.repo.GetTask(ctx, "t1") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + draftOverrides, err := models.NewWorkflowAgentOverrides("wf-destination", []models.WorkflowAgentOverrideBinding{{ + StepID: "implement", SourceProfileID: "profile-a", ReplacementProfileID: "profile-b", + }}) + if err != nil { + t.Fatalf("NewWorkflowAgentOverrides: %v", err) + } + source := &wfmodels.WorkflowStep{ + ID: "implement", WorkflowID: "wf-destination", Position: 0, AgentProfileID: "profile-a", + } + destination := &wfmodels.WorkflowStep{ + ID: "pr", WorkflowID: "wf-destination", Position: 1, + ProfileSessionStartPolicy: models.WorkflowProfileSessionStartPolicyReuse, + SessionTarget: &wfmodels.WorkflowSessionTarget{Kind: wfmodels.WorkflowSessionTargetStep, StepID: source.ID}, + } + fixture.stepGetter.steps[source.ID] = source + fixture.stepGetter.steps[destination.ID] = destination + fixture.agentMgr.resolveProfileInfo = &executor.AgentProfileInfo{ + ProfileID: "profile-b", ProfileName: "Agent B", Model: "model-b", + } + + if tc.bindingProfileID != "" { + session := fixture.current + if tc.bindingProfileID == "profile-b" { + session = &models.TaskSession{ + ID: "session-b", TaskID: task.ID, AgentProfileID: "profile-b", + State: models.TaskSessionStateWaitingForInput, + AgentProfileSnapshot: map[string]interface{}{"name": "Agent B", "model": "model-b"}, + } + if err := fixture.repo.CreateTaskSession(ctx, session); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + } + if _, err := fixture.repo.UpsertWorkflowSessionBinding(ctx, &models.WorkflowSessionBinding{ + TaskID: task.ID, TargetKey: workflowSessionBindingTargetKey(source.ID), + WorkflowID: "wf-destination", AgentProfileID: tc.bindingProfileID, + SessionID: session.ID, OperationID: "preview-test-binding", UpdatedAt: now, + }); err != nil { + t.Fatalf("UpsertWorkflowSessionBinding: %v", err) + } + } + + preview, err := fixture.svc.PreviewWorkflowMove(ctx, WorkflowMovePreviewRequest{ + TaskID: task.ID, WorkflowID: "wf-destination", WorkflowStepID: destination.ID, + WorkflowChange: &models.WorkflowChangeRequest{ + ExpectedWorkflowID: task.WorkflowID, ExpectedStepID: task.WorkflowStepID, + ExpectedUpdatedAt: task.UpdatedAt, + AgentOverrides: map[string]string{"profile-a": "profile-b"}, + }, + CandidateWorkflowOverrides: draftOverrides, + }) + if err != nil { + t.Fatalf("PreviewWorkflowMove: %v", err) + } + storedBeforeCommit, err := fixture.repo.GetTask(ctx, task.ID) + if err != nil { + t.Fatalf("GetTask after read-only preview: %v", err) + } + if storedBeforeCommit.WorkflowID != task.WorkflowID || storedBeforeCommit.WorkflowAgentOverrides != nil { + t.Fatalf("preview persisted candidate state: %+v", storedBeforeCommit) + } + + committedTask := *task + committedTask.WorkflowID = "wf-destination" + committedTask.WorkflowStepID = destination.ID + committedTask.WorkflowAgentOverrides = draftOverrides + if err := fixture.repo.UpdateTask(ctx, &committedTask); err != nil { + t.Fatalf("UpdateTask to committed candidate: %v", err) + } + routed, err := fixture.svc.resolveWorkflowSessionTarget(ctx, task.ID, destination) + if err != nil { + t.Fatalf("resolve committed workflow target: %v", err) + } + + if preview.Outcome != tc.wantPreviewOutcome { + t.Errorf("preview outcome = %q, want %q", preview.Outcome, tc.wantPreviewOutcome) + } + if preview.Recipient == nil || preview.Recipient.ProfileID != routed.profileID { + t.Errorf("preview recipient = %#v, committed profile = %q", preview.Recipient, routed.profileID) + } + if preview.Model.After.ID != "model-b" { + t.Errorf("preview destination model = %q, want model-b", preview.Model.After.ID) + } + gotSessionID := "" + if preview.Recipient != nil { + gotSessionID = preview.Recipient.SessionID + } + if routed.session != nil && routed.session.ID != tc.wantSessionID { + t.Errorf("committed target session = %q, want %q", routed.session.ID, tc.wantSessionID) + } + if gotSessionID != sessionID(routed.session) { + t.Errorf("preview target session = %q, committed target session = %q", gotSessionID, sessionID(routed.session)) + } + stored, err := fixture.repo.GetTask(ctx, task.ID) + if err != nil { + t.Fatalf("GetTask after preview and simulated commit: %v", err) + } + if stored.WorkflowID != "wf-destination" || stored.WorkflowAgentOverrides.WorkflowID != "wf-destination" { + t.Errorf("committed task candidate was not stored as expected: %+v", stored) + } + }) + } +} + func TestBuildWorkflowMovePreview_SelectsTheSameReusableSessionAsMove(t *testing.T) { now := time.Now().UTC() current := &models.TaskSession{ diff --git a/apps/backend/internal/orchestrator/workflow_session_target.go b/apps/backend/internal/orchestrator/workflow_session_target.go index b16f4554c0..1552bb2f7f 100644 --- a/apps/backend/internal/orchestrator/workflow_session_target.go +++ b/apps/backend/internal/orchestrator/workflow_session_target.go @@ -90,6 +90,18 @@ func (s *Service) resolveWorkflowSessionTarget( ctx context.Context, taskID string, step *wfmodels.WorkflowStep, +) (workflowSessionTargetResolution, error) { + return s.resolveWorkflowSessionTargetWithTask(ctx, taskID, step, nil) +} + +// resolveWorkflowSessionTargetWithTask resolves the recipient using a task +// projection when a move is being validated against an uncommitted profile map. +// Initial targets remain tied to the task's immutable initial-session record. +func (s *Service) resolveWorkflowSessionTargetWithTask( + ctx context.Context, + taskID string, + step *wfmodels.WorkflowStep, + effectiveTask *models.Task, ) (workflowSessionTargetResolution, error) { if step == nil || step.SessionTarget == nil { return workflowSessionTargetResolution{}, fmt.Errorf("workflow session target is required") @@ -105,13 +117,14 @@ func (s *Service) resolveWorkflowSessionTarget( return workflowSessionTargetResolution{session: session, profileID: profileID}, nil } - return s.resolveSourceWorkflowSessionTarget(ctx, taskID, step) + return s.resolveSourceWorkflowSessionTargetWithTask(ctx, taskID, step, effectiveTask) } -func (s *Service) resolveSourceWorkflowSessionTarget( +func (s *Service) resolveSourceWorkflowSessionTargetWithTask( ctx context.Context, taskID string, step *wfmodels.WorkflowStep, + effectiveTask *models.Task, ) (workflowSessionTargetResolution, error) { sourceStep, err := s.loadWorkflowStepForLifecycle(ctx, step.SessionTarget.StepID, "session target source") if err != nil { @@ -120,11 +133,13 @@ func (s *Service) resolveSourceWorkflowSessionTarget( if err := validateWorkflowSessionTargetSource(step, sourceStep); err != nil { return workflowSessionTargetResolution{}, err } - session, err := s.resolveBoundSourceWorkflowSession(ctx, taskID, step.WorkflowID, sourceStep) + profileID, err := s.resolveWorkflowSessionTargetProfile(ctx, taskID, effectiveTask, sourceStep) if err != nil { return workflowSessionTargetResolution{}, err } - profileID, err := s.resolveStepAgentProfileForTaskID(ctx, taskID, sourceStep) + session, err := s.resolveBoundSourceWorkflowSessionWithProfile( + ctx, taskID, step.WorkflowID, sourceStep, profileID, + ) if err != nil { return workflowSessionTargetResolution{}, err } @@ -147,11 +162,24 @@ func validateWorkflowSessionTargetSource(destination, source *wfmodels.WorkflowS return nil } -func (s *Service) resolveBoundSourceWorkflowSession( +func (s *Service) resolveWorkflowSessionTargetProfile( + ctx context.Context, + taskID string, + effectiveTask *models.Task, + sourceStep *wfmodels.WorkflowStep, +) (string, error) { + if effectiveTask != nil { + return s.resolveStepAgentProfileForTask(ctx, effectiveTask, sourceStep), nil + } + return s.resolveStepAgentProfileForTaskID(ctx, taskID, sourceStep) +} + +func (s *Service) resolveBoundSourceWorkflowSessionWithProfile( ctx context.Context, taskID string, workflowID string, sourceStep *wfmodels.WorkflowStep, + effectiveProfileID string, ) (*models.TaskSession, error) { store, ok := s.repo.(workflowSessionBindingStore) if !ok { @@ -161,10 +189,6 @@ func (s *Service) resolveBoundSourceWorkflowSession( if err != nil { return nil, err } - effectiveProfileID, err := s.resolveStepAgentProfileForTaskID(ctx, taskID, sourceStep) - if err != nil { - return nil, err - } if binding == nil || binding.TaskID != taskID || binding.WorkflowID != workflowID || binding.AgentProfileID != effectiveProfileID || binding.SessionID == "" { return nil, nil @@ -176,6 +200,9 @@ func (s *Service) resolveBoundSourceWorkflowSession( if session == nil || session.TaskID != taskID || session.AgentProfileID != effectiveProfileID { return nil, nil } + if isTerminalSessionState(session.State) { + return nil, nil + } return session, nil } diff --git a/apps/backend/internal/orchestrator/workflow_session_target_test.go b/apps/backend/internal/orchestrator/workflow_session_target_test.go index 2fd15cc6f1..0deb69ad7c 100644 --- a/apps/backend/internal/orchestrator/workflow_session_target_test.go +++ b/apps/backend/internal/orchestrator/workflow_session_target_test.go @@ -7,11 +7,18 @@ import ( "testing" "time" + "github.com/kandev/kandev/internal/gitcredentials" "github.com/kandev/kandev/internal/task/models" wfmodels "github.com/kandev/kandev/internal/workflow/models" "github.com/stretchr/testify/require" ) +type workflowTargetTestCredentialIssuer struct{} + +func (workflowTargetTestCredentialIssuer) Issue(context.Context, gitcredentials.Scope) (gitcredentials.Lease, error) { + return gitcredentials.Lease{Token: "opaque-test-lease"}, nil +} + type failWorkflowRouteMetadataRepo struct { repoStore fail bool @@ -124,6 +131,66 @@ func TestSelectExplicitWorkflowStartSessionFallsBackFromTerminalTarget(t *testin require.Equal(t, "profile-a", profileID) } +func TestCandidatePreflightIgnoresTerminalStepBinding(t *testing.T) { + ctx := context.Background() + fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) + task, err := fixture.repo.GetTask(ctx, "t1") + require.NoError(t, err) + overrides, err := models.NewWorkflowAgentOverrides("wf1", []models.WorkflowAgentOverrideBinding{{ + StepID: "implement", SourceProfileID: "profile-a", ReplacementProfileID: "profile-b", + }}) + require.NoError(t, err) + source := &wfmodels.WorkflowStep{ID: "implement", WorkflowID: "wf1", Position: 0, AgentProfileID: "profile-a"} + target := &wfmodels.WorkflowStep{ + ID: "pr", WorkflowID: "wf1", Position: 1, + ProfileSessionStartPolicy: models.WorkflowProfileSessionStartPolicyReuse, + SessionTarget: &wfmodels.WorkflowSessionTarget{Kind: wfmodels.WorkflowSessionTargetStep, StepID: source.ID}, + } + fixture.stepGetter.steps[source.ID] = source + fixture.stepGetter.steps[target.ID] = target + + require.NoError(t, fixture.repo.CreateExecutor(ctx, &models.Executor{ + ID: "exec-ssh", Name: "SSH", Type: models.ExecutorTypeSSH, + })) + require.NoError(t, fixture.repo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "executor-profile-a", ExecutorID: "exec-ssh", Name: "A with GitHub token", + Config: map[string]string{"remote_auth_secrets": `{"gh_cli_env":"test-token"}`}, + })) + require.NoError(t, fixture.repo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "executor-profile-b", ExecutorID: "exec-ssh", Name: "B without GitHub token", + })) + fixture.current.ExecutorID = "exec-ssh" + fixture.current.ExecutorProfileID = "executor-profile-a" + require.NoError(t, fixture.repo.UpdateTaskSession(ctx, fixture.current)) + terminal := &models.TaskSession{ + ID: "terminal-profile-b", TaskID: task.ID, AgentProfileID: "profile-b", + ExecutorID: "exec-ssh", ExecutorProfileID: "executor-profile-b", + State: models.TaskSessionStateCompleted, + } + require.NoError(t, fixture.repo.CreateTaskSession(ctx, terminal)) + _, err = fixture.repo.UpsertWorkflowSessionBinding(ctx, &models.WorkflowSessionBinding{ + TaskID: task.ID, TargetKey: workflowSessionBindingTargetKey(source.ID), + WorkflowID: "wf1", AgentProfileID: "profile-b", SessionID: terminal.ID, + OperationID: "candidate-terminal-binding", UpdatedAt: time.Now().UTC(), + }) + require.NoError(t, err) + require.NoError(t, fixture.repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-invalid-github", WorkspaceID: "ws1", Name: "Invalid GitHub remote", + SourceType: "local", Provider: "github", RemoteURL: "https://forge.example/acme/repo.git", + })) + require.NoError(t, fixture.repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "task-repo-invalid-github", TaskID: task.ID, RepositoryID: "repo-invalid-github", + })) + fixture.svc.executor.SetGitHubCredentialBroker( + workflowTargetTestCredentialIssuer{}, "https://kandev.example/api/v1/github/credentials/resolve", + ) + candidate := *task + candidate.WorkflowAgentOverrides = overrides + + require.NoError(t, fixture.svc.PreflightWorkflowStepChange(ctx, &candidate, fixture.current, target), + "the terminal binding is not reused; preflight must use the current session's executor profile") +} + func TestRecordWorkflowSourceBindingIgnoresDelayedEntryAfterTaskMoves(t *testing.T) { ctx := context.Background() fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) @@ -173,6 +240,73 @@ func TestWorkflowSessionTargetUsesTaskEffectiveSourceProfile(t *testing.T) { require.Equal(t, "profile-b", resolution.profileID) } +func TestWorkflowChangeCredentialPreflightUsesCandidateBoundSession(t *testing.T) { + ctx := context.Background() + fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) + now := time.Now().UTC() + require.NoError(t, fixture.repo.CreateWorkflow(ctx, &models.Workflow{ + ID: "wf-destination", WorkspaceID: "ws1", Name: "Destination", CreatedAt: now, UpdatedAt: now, + })) + task, err := fixture.repo.GetTask(ctx, "t1") + require.NoError(t, err) + candidateOverrides, err := models.NewWorkflowAgentOverrides("wf-destination", []models.WorkflowAgentOverrideBinding{{ + StepID: "implement", SourceProfileID: "profile-a", ReplacementProfileID: "profile-b", + }}) + require.NoError(t, err) + source := &wfmodels.WorkflowStep{ + ID: "implement", WorkflowID: "wf-destination", Position: 0, AgentProfileID: "profile-a", + } + target := &wfmodels.WorkflowStep{ + ID: "pr", WorkflowID: "wf-destination", Position: 1, + ProfileSessionStartPolicy: models.WorkflowProfileSessionStartPolicyReuse, + SessionTarget: &wfmodels.WorkflowSessionTarget{Kind: wfmodels.WorkflowSessionTargetStep, StepID: source.ID}, + } + fixture.stepGetter.steps[source.ID] = source + fixture.stepGetter.steps[target.ID] = target + + require.NoError(t, fixture.repo.CreateExecutor(ctx, &models.Executor{ + ID: "exec-ssh", Name: "SSH", Type: models.ExecutorTypeSSH, + })) + require.NoError(t, fixture.repo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "executor-profile-a", ExecutorID: "exec-ssh", Name: "A without GitHub token", + })) + require.NoError(t, fixture.repo.CreateExecutorProfile(ctx, &models.ExecutorProfile{ + ID: "executor-profile-b", ExecutorID: "exec-ssh", Name: "B with GitHub token", + Config: map[string]string{"remote_auth_secrets": `{"gh_cli_env":"test-token"}`}, + })) + fixture.current.ExecutorID = "exec-ssh" + fixture.current.ExecutorProfileID = "executor-profile-a" + require.NoError(t, fixture.repo.UpdateTaskSession(ctx, fixture.current)) + bound := &models.TaskSession{ + ID: "session-profile-b", TaskID: task.ID, AgentProfileID: "profile-b", + ExecutorID: "exec-ssh", ExecutorProfileID: "executor-profile-b", + State: models.TaskSessionStateWaitingForInput, + } + require.NoError(t, fixture.repo.CreateTaskSession(ctx, bound)) + _, err = fixture.repo.UpsertWorkflowSessionBinding(ctx, &models.WorkflowSessionBinding{ + TaskID: task.ID, TargetKey: workflowSessionBindingTargetKey(source.ID), + WorkflowID: "wf-destination", AgentProfileID: "profile-b", SessionID: bound.ID, + OperationID: "preflight-candidate-binding", UpdatedAt: now, + }) + require.NoError(t, err) + require.NoError(t, fixture.repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-invalid-github", WorkspaceID: "ws1", Name: "Invalid GitHub remote", + SourceType: "local", Provider: "github", RemoteURL: "https://forge.example/acme/repo.git", + })) + require.NoError(t, fixture.repo.CreateTaskRepository(ctx, &models.TaskRepository{ + ID: "task-repo-invalid-github", TaskID: task.ID, RepositoryID: "repo-invalid-github", + })) + fixture.svc.executor.SetGitHubCredentialBroker( + workflowTargetTestCredentialIssuer{}, "https://kandev.example/api/v1/github/credentials/resolve", + ) + candidate := *task + candidate.WorkflowID = "wf-destination" + candidate.WorkflowAgentOverrides = candidateOverrides + + err = fixture.svc.PreflightWorkflowStepChange(ctx, &candidate, fixture.current, target) + require.NoError(t, err, "candidate binding B supplies its own remote Git credential profile") +} + func TestRecordWorkflowSourceBindingDoesNotOverwriteRevisitedSourceEntry(t *testing.T) { ctx := context.Background() fixture := newProfileSwitchFixture(t, models.WorkflowProfileSessionStartPolicyReuse, models.WorkflowProfileSessionEndPolicyPark) diff --git a/apps/backend/internal/task/handlers/change_workflow_test.go b/apps/backend/internal/task/handlers/change_workflow_test.go new file mode 100644 index 0000000000..e4ed3809a0 --- /dev/null +++ b/apps/backend/internal/task/handlers/change_workflow_test.go @@ -0,0 +1,133 @@ +package handlers + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/require" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/service" + ws "github.com/kandev/kandev/pkg/websocket" +) + +func TestHTTPWorkflowChangeRejectsStaleSourceBeforeMoving(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + updatedAt := time.Date(2026, time.September, 23, 10, 0, 0, 0, time.UTC) + task := &models.Task{ + ID: "task-workflow-change", + WorkspaceID: "workspace-1", + WorkflowID: "wf-source", + WorkflowStepID: "step-source", + UpdatedAt: updatedAt, + } + repo := &moveTaskConflictRepo{task: task} + 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} + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Params = gin.Params{{Key: "id", Value: task.ID}} + c.Request = httptest.NewRequest(http.MethodPost, "/tasks/"+task.ID+"/move", strings.NewReader(`{ + "workflow_id": "wf-target", + "workflow_step_id": "step-target", + "workflow_change": { + "expected_workflow_id": "wf-stale", + "expected_step_id": "step-source", + "expected_updated_at": "2026-09-23T10:00:00Z", + "agent_overrides": {} + } + }`)) + c.Request.Header.Set("Content-Type", "application/json") + + h.httpMoveTask(c) + + require.Equal(t, http.StatusConflict, rec.Code, "body: %s", rec.Body.String()) + require.Equal(t, "wf-source", repo.task.WorkflowID) + require.Equal(t, "step-source", repo.task.WorkflowStepID) +} + +func TestHTTPWorkflowChangeRequiresAnAgentOverridesObject(t *testing.T) { + gin.SetMode(gin.TestMode) + log := newTestLogger(t) + updatedAt := time.Date(2026, time.September, 23, 10, 0, 0, 0, time.UTC) + task := &models.Task{ + ID: "task-workflow-change-missing-map", + WorkspaceID: "workspace-1", + WorkflowID: "wf-source", + WorkflowStepID: "step-source", + UpdatedAt: updatedAt, + } + repo := &moveTaskConflictRepo{task: task} + 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} + rec := httptest.NewRecorder() + c, _ := gin.CreateTestContext(rec) + c.Params = gin.Params{{Key: "id", Value: task.ID}} + c.Request = httptest.NewRequest(http.MethodPost, "/tasks/"+task.ID+"/move", strings.NewReader(`{ + "workflow_id": "wf-target", + "workflow_step_id": "step-target", + "workflow_change": { + "expected_workflow_id": "wf-source", + "expected_step_id": "step-source", + "expected_updated_at": "2026-09-23T10:00:00Z" + } + }`)) + c.Request.Header.Set("Content-Type", "application/json") + + h.httpMoveTask(c) + + require.Equal(t, http.StatusBadRequest, rec.Code, "body: %s", rec.Body.String()) + require.Equal(t, "wf-source", repo.task.WorkflowID) + require.Equal(t, "step-source", repo.task.WorkflowStepID) +} + +func TestWSWorkflowChangeReturnsStableConflictCode(t *testing.T) { + log := newTestLogger(t) + updatedAt := time.Date(2026, time.September, 23, 10, 0, 0, 0, time.UTC) + task := &models.Task{ + ID: "task-ws-workflow-change", WorkspaceID: "workspace-1", + WorkflowID: "wf-source", WorkflowStepID: "step-source", UpdatedAt: updatedAt, + } + repo := &moveTaskConflictRepo{task: task} + 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} + request, err := ws.NewRequest("move-1", ws.ActionTaskMove, map[string]any{ + "id": task.ID, "workflow_id": "wf-target", "workflow_step_id": "step-target", + "workflow_change": map[string]any{ + "expected_workflow_id": "wf-outdated", "expected_step_id": "step-source", + "expected_updated_at": updatedAt, "agent_overrides": map[string]string{}, + }, + }) + require.NoError(t, err) + + response, err := h.wsMoveTask(context.Background(), request) + require.NoError(t, err) + payload := wsWorkflowError(t, response) + require.Equal(t, string(ws.ErrorCodeConflict), payload.Code) + require.Equal(t, "workflow_change_conflict", payload.Message) + require.Equal(t, "wf-source", repo.task.WorkflowID) +} diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index 939f374bb7..243804b7f7 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -26,6 +26,7 @@ const ( moveConflictCodeWorkflowStep = "task_move_workflow_step" moveConflictCodeWIPLimit = "task_move_wip_limit" moveConflictCodePending = "task_move_pending" + moveConflictCodeWorkflowChange = "workflow_change_conflict" ) // errorDetailKeyErrorCode is the JSON key every machine-readable error detail @@ -148,6 +149,15 @@ func handleSelectedMoveError(c *gin.Context, log *logger.Logger, err error) { } c.JSON(http.StatusConflict, body) case isValidationError(err): + var changeErr *service.WorkflowChangeValidationError + if errors.As(err, &changeErr) { + body := gin.H{"code": changeErr.Code} + if changeErr.SourceProfileID != "" { + body["source_profile_id"] = changeErr.SourceProfileID + } + c.JSON(http.StatusBadRequest, body) + return + } c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) default: log.Error("task move failed", zap.Error(err)) @@ -197,6 +207,8 @@ func isMoveConflict(err error) bool { // error code and the safe error message (which never carries option values). func moveEntryOptionsWSError(err error) (string, string, bool) { switch { + case errors.Is(err, service.ErrWorkflowChangeConflict): + return ws.ErrorCodeConflict, moveConflictCodeWorkflowChange, true case errors.Is(err, workflowmove.ErrMoveConflict): return ws.ErrorCodeConflict, err.Error(), true case errors.Is(err, workflowmove.ErrConflictingInstructions), @@ -205,6 +217,10 @@ func moveEntryOptionsWSError(err error) (string, string, bool) { errors.Is(err, workflowmove.ErrEntryTargetUnavailable): return ws.ErrorCodeValidation, err.Error(), true default: + var changeErr *service.WorkflowChangeValidationError + if errors.As(err, &changeErr) { + return ws.ErrorCodeValidation, changeErr.Code, true + } return "", "", false } } @@ -216,6 +232,9 @@ func moveConflictCode(err error) string { if errors.Is(err, workflowmove.ErrMoveConflict) { return moveConflictCodePending } + if errors.Is(err, service.ErrWorkflowChangeConflict) { + return moveConflictCodeWorkflowChange + } msg := strings.ToLower(err.Error()) switch { case strings.Contains(msg, "active session"): @@ -240,6 +259,9 @@ func isValidationError(err error) bool { if errors.Is(err, service.ErrInvalidParent) || errors.Is(err, service.ErrAutoTitleUnsupportedForOffice) { return true } + if errors.Is(err, service.ErrInvalidWorkflowChange) { + return true + } if errors.Is(err, service.ErrTaskTitleTooLong) { return true } diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index 6f2ca774d4..1e3f9e4559 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -1779,10 +1779,11 @@ func (h *TaskHandlers) httpUpdateTaskRepository(c *gin.Context) { } type httpMoveTaskRequest struct { - WorkflowID string `json:"workflow_id"` - WorkflowStepID string `json:"workflow_step_id"` - Position int `json:"position"` - EntryOptions *workflowmove.EntryOptions `json:"entry_options,omitempty"` + WorkflowID string `json:"workflow_id"` + WorkflowStepID string `json:"workflow_step_id"` + Position int `json:"position"` + EntryOptions *workflowmove.EntryOptions `json:"entry_options,omitempty"` + WorkflowChange *models.WorkflowChangeRequest `json:"workflow_change,omitempty"` } // httpReorderStepTasksRequest is the frozen reorder request contract @@ -1862,6 +1863,7 @@ func (h *TaskHandlers) httpMoveTask(c *gin.Context) { AllowActivePrimarySession: true, StepHistoryActor: wfmodels.StepTransitionActorHuman, EntryOptions: body.EntryOptions, + WorkflowChange: body.WorkflowChange, }, ) if err != nil { diff --git a/apps/backend/internal/task/handlers/task_move_preview_handlers.go b/apps/backend/internal/task/handlers/task_move_preview_handlers.go index ead1733753..cd494e9699 100644 --- a/apps/backend/internal/task/handlers/task_move_preview_handlers.go +++ b/apps/backend/internal/task/handlers/task_move_preview_handlers.go @@ -6,6 +6,7 @@ import ( "github.com/gin-gonic/gin" "github.com/kandev/kandev/internal/orchestrator" + "github.com/kandev/kandev/internal/task/models" workflowmove "github.com/kandev/kandev/internal/workflow/move" ) @@ -58,12 +59,24 @@ func (h *TaskHandlers) httpMoveTaskPreview(c *gin.Context) { handleSelectedMoveError(c, h.logger, err) return } + var candidateOverrides *models.WorkflowAgentOverrides + if body.WorkflowChange != nil { + candidateOverrides, err = h.service.ValidateWorkflowChange( + c.Request.Context(), taskID, body.WorkflowID, body.WorkflowStepID, body.WorkflowChange, + ) + if err != nil { + handleSelectedMoveError(c, h.logger, err) + return + } + } preview, err := h.movePreviewer.PreviewWorkflowMove(c.Request.Context(), orchestrator.WorkflowMovePreviewRequest{ - TaskID: taskID, - WorkflowID: body.WorkflowID, - WorkflowStepID: body.WorkflowStepID, - EntryOptions: entryOptions, + TaskID: taskID, + WorkflowID: body.WorkflowID, + WorkflowStepID: body.WorkflowStepID, + EntryOptions: entryOptions, + WorkflowChange: body.WorkflowChange, + CandidateWorkflowOverrides: candidateOverrides, }) if err != nil { handleSelectedMoveError(c, h.logger, err) diff --git a/apps/backend/internal/task/handlers/task_ws_handlers.go b/apps/backend/internal/task/handlers/task_ws_handlers.go index 5a52a121a2..163f63310e 100644 --- a/apps/backend/internal/task/handlers/task_ws_handlers.go +++ b/apps/backend/internal/task/handlers/task_ws_handlers.go @@ -460,11 +460,12 @@ func (h *TaskHandlers) wsArchiveTask(ctx context.Context, msg *ws.Message) (*ws. } type wsMoveTaskRequest struct { - ID string `json:"id"` - WorkflowID string `json:"workflow_id"` - WorkflowStepID string `json:"workflow_step_id"` - Position int `json:"position"` - EntryOptions *workflowmove.EntryOptions `json:"entry_options,omitempty"` + ID string `json:"id"` + WorkflowID string `json:"workflow_id"` + WorkflowStepID string `json:"workflow_step_id"` + Position int `json:"position"` + EntryOptions *workflowmove.EntryOptions `json:"entry_options,omitempty"` + WorkflowChange *models.WorkflowChangeRequest `json:"workflow_change,omitempty"` } func (h *TaskHandlers) wsMoveTask(ctx context.Context, msg *ws.Message) (*ws.Message, error) { @@ -492,6 +493,7 @@ func (h *TaskHandlers) wsMoveTask(ctx context.Context, msg *ws.Message) (*ws.Mes AllowActivePrimarySession: true, StepHistoryActor: wfmodels.StepTransitionActorHuman, EntryOptions: req.EntryOptions, + WorkflowChange: req.WorkflowChange, }, ) if err != nil { diff --git a/apps/backend/internal/task/models/workflow_agent_overrides.go b/apps/backend/internal/task/models/workflow_agent_overrides.go index 0c9f8965ad..f6d32eba88 100644 --- a/apps/backend/internal/task/models/workflow_agent_overrides.go +++ b/apps/backend/internal/task/models/workflow_agent_overrides.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "time" ) var ( @@ -32,6 +33,24 @@ type WorkflowAgentOverrides struct { Steps []WorkflowAgentOverrideBinding `json:"steps"` } +// WorkflowChangeRequest is the optimistic source guard and grouped profile +// choice accepted by the explicit change-workflow action. A non-nil empty +// AgentOverrides map means use every destination workflow default. +type WorkflowChangeRequest struct { + ExpectedWorkflowID string `json:"expected_workflow_id"` + ExpectedStepID string `json:"expected_step_id"` + ExpectedUpdatedAt time.Time `json:"expected_updated_at"` + AgentOverrides map[string]string `json:"agent_overrides"` +} + +// WorkflowChangeSource pins a guarded workflow-change write to the source +// task row observed by the user before submitting the form. +type WorkflowChangeSource struct { + WorkflowID string + StepID string + UpdatedAt time.Time +} + // NewWorkflowAgentOverrides validates and canonicalizes bindings. A self // replacement is equivalent to no override and is omitted from the record. func NewWorkflowAgentOverrides(workflowID string, bindings []WorkflowAgentOverrideBinding) (*WorkflowAgentOverrides, error) { diff --git a/apps/backend/internal/task/repository/repoerrors/errors.go b/apps/backend/internal/task/repository/repoerrors/errors.go index 2ecbd2bfc6..01c451ab80 100644 --- a/apps/backend/internal/task/repository/repoerrors/errors.go +++ b/apps/backend/internal/task/repository/repoerrors/errors.go @@ -98,6 +98,10 @@ var ErrTaskCleanupInProgress = errors.New("task cleanup in progress") // contract. var ErrWorkflowResolutionConflict = errors.New("task workflow changed since resolution") +// ErrWorkflowChangeConflict reports that an explicit change-workflow request +// no longer matches the task source or task version it was opened against. +var ErrWorkflowChangeConflict = errors.New("task changed since workflow change was opened") + // ErrRunnerMutabilityConflict wraps one of the ten ordered mutability reason // codes rejecting a runner switch. Reason is always a member of the same // closed vocabulary the projection uses, never "eligible" and never empty. diff --git a/apps/backend/internal/task/repository/sqlite/change_workflow_postgres_test.go b/apps/backend/internal/task/repository/sqlite/change_workflow_postgres_test.go new file mode 100644 index 0000000000..04fb9c60f9 --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/change_workflow_postgres_test.go @@ -0,0 +1,58 @@ +package sqlite + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +func TestPostgresWorkflowChangeAtomicAdmissionGuardsSourceVersion(t *testing.T) { + repoA, repoB, _ := newTaskPostgresRepoPair(t) + ctx := context.Background() + source := seedWorkflowChangeRows(t, repoA, "task-workflow-change-postgres") + guard := &models.WorkflowChangeSource{WorkflowID: source.WorkflowID, StepID: source.WorkflowStepID, UpdatedAt: source.UpdatedAt} + candidate := workflowChangeCandidate(t, source) + + concurrent, err := repoB.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask before concurrent edit: %v", err) + } + concurrent.Description = "Concurrent PostgreSQL edit" + if err := repoB.UpdateTask(ctx, concurrent); err != nil { + t.Fatalf("UpdateTask concurrent edit: %v", err) + } + if _, err := repoA.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, candidate, source.WorkflowStepID, candidate.WorkflowStepID, 0, nil, true, guard, + ); !errors.Is(err, repoerrors.ErrWorkflowChangeConflict) { + t.Fatalf("stale guarded write error = %v, want ErrWorkflowChangeConflict", err) + } + + current, err := repoA.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask after stale change: %v", err) + } + if current.WorkflowID != source.WorkflowID || current.WorkflowStepID != source.WorkflowStepID || current.Description != "Concurrent PostgreSQL edit" { + t.Fatalf("stale change overwrote current task: %+v", current) + } + + currentGuard := &models.WorkflowChangeSource{WorkflowID: current.WorkflowID, StepID: current.WorkflowStepID, UpdatedAt: current.UpdatedAt} + currentCandidate := workflowChangeCandidate(t, current) + if _, err := repoA.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, currentCandidate, current.WorkflowStepID, currentCandidate.WorkflowStepID, 0, nil, true, currentGuard, + ); err != nil { + t.Fatalf("current guarded write: %v", err) + } + stored, err := repoB.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask after successful change: %v", err) + } + if stored.WorkflowID != currentCandidate.WorkflowID || stored.WorkflowStepID != currentCandidate.WorkflowStepID { + t.Fatalf("stored destination = %s/%s", stored.WorkflowID, stored.WorkflowStepID) + } + if stored.WorkflowAgentOverrides == nil || stored.WorkflowAgentOverrides.WorkflowID != currentCandidate.WorkflowID { + t.Fatalf("stored destination overrides = %+v", stored.WorkflowAgentOverrides) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/change_workflow_test.go b/apps/backend/internal/task/repository/sqlite/change_workflow_test.go new file mode 100644 index 0000000000..f3f5093c3f --- /dev/null +++ b/apps/backend/internal/task/repository/sqlite/change_workflow_test.go @@ -0,0 +1,147 @@ +package sqlite + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" +) + +func seedWorkflowChangeRows(t *testing.T, repo *Repository, taskID string) *models.Task { + t.Helper() + ctx := context.Background() + seedWorkspace(t, repo, "workspace-workflow-change") + for _, workflow := range []*models.Workflow{ + {ID: "workflow-change-source", WorkspaceID: "workspace-workflow-change", Name: "Source"}, + {ID: "workflow-change-target", WorkspaceID: "workspace-workflow-change", Name: "Target"}, + } { + if err := repo.CreateWorkflow(ctx, workflow); err != nil { + t.Fatalf("CreateWorkflow(%s): %v", workflow.ID, err) + } + } + seedCASWorkflowStep(t, repo, "workflow-change-source", "workflow-change-source-step", 0) + seedCASWorkflowStep(t, repo, "workflow-change-target", "workflow-change-target-step", 0) + oldOverrides, err := models.NewWorkflowAgentOverrides("workflow-change-source", []models.WorkflowAgentOverrideBinding{{ + StepID: "workflow-change-source-step", SourceProfileID: "source-old", ReplacementProfileID: "replacement-old", + }}) + if err != nil { + t.Fatalf("NewWorkflowAgentOverrides: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: taskID, WorkspaceID: "workspace-workflow-change", + WorkflowID: "workflow-change-source", WorkflowStepID: "workflow-change-source-step", + Title: "Original title", Description: "Original description", Priority: "medium", + Metadata: map[string]interface{}{"sentinel": "keep"}, WorkflowAgentOverrides: oldOverrides, + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + task, err := repo.GetTask(ctx, taskID) + if err != nil { + t.Fatalf("GetTask: %v", err) + } + return task +} + +func workflowChangeCandidate(t *testing.T, task *models.Task) *models.Task { + t.Helper() + candidate := *task + candidate.WorkflowID = "workflow-change-target" + candidate.WorkflowStepID = "workflow-change-target-step" + candidate.WorkflowAgentOverrides, _ = models.NewWorkflowAgentOverrides("workflow-change-target", []models.WorkflowAgentOverrideBinding{{ + StepID: "workflow-change-target-step", SourceProfileID: "source-new", ReplacementProfileID: "replacement-new", + }}) + return &candidate +} + +func TestWorkflowChangeAtomicAdmissionPersistsMembershipAndOverridesTogether(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + source := seedWorkflowChangeRows(t, repo, "task-workflow-change-atomic") + guard := &models.WorkflowChangeSource{WorkflowID: source.WorkflowID, StepID: source.WorkflowStepID, UpdatedAt: source.UpdatedAt} + candidate := workflowChangeCandidate(t, source) + if _, err := repo.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, candidate, source.WorkflowStepID, candidate.WorkflowStepID, 0, nil, true, guard, + ); err != nil { + t.Fatalf("UpdateTaskWithWorkflowChangeAdmissionAndState: %v", err) + } + stored, err := repo.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask after change: %v", err) + } + if stored.WorkflowID != candidate.WorkflowID || stored.WorkflowStepID != candidate.WorkflowStepID { + t.Fatalf("stored destination = %s/%s", stored.WorkflowID, stored.WorkflowStepID) + } + if stored.WorkflowAgentOverrides == nil || stored.WorkflowAgentOverrides.WorkflowID != candidate.WorkflowID { + t.Fatalf("stored overrides = %+v", stored.WorkflowAgentOverrides) + } + if replacement, ok := stored.WorkflowAgentOverrides.ReplacementFor(candidate.WorkflowID, candidate.WorkflowStepID); !ok || replacement != "replacement-new" { + t.Fatalf("stored destination replacement = %q, %v", replacement, ok) + } + if stored.Title != "Original title" || stored.Description != "Original description" || stored.Metadata["sentinel"] != "keep" { + t.Fatalf("task context changed: %+v", stored) + } +} + +func TestWorkflowChangeAtomicAdmissionRejectsConcurrentTaskEdit(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + source := seedWorkflowChangeRows(t, repo, "task-workflow-change-stale") + staleGuard := &models.WorkflowChangeSource{WorkflowID: source.WorkflowID, StepID: source.WorkflowStepID, UpdatedAt: source.UpdatedAt} + candidate := workflowChangeCandidate(t, source) + + current, err := repo.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask before concurrent edit: %v", err) + } + current.Description = "Concurrent edit" + if err := repo.UpdateTask(ctx, current); err != nil { + t.Fatalf("UpdateTask concurrent edit: %v", err) + } + if _, err := repo.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, candidate, source.WorkflowStepID, candidate.WorkflowStepID, 0, nil, true, staleGuard, + ); !errors.Is(err, repoerrors.ErrWorkflowChangeConflict) { + t.Fatalf("stale guarded write error = %v, want ErrWorkflowChangeConflict", err) + } + stored, err := repo.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask after conflict: %v", err) + } + if stored.WorkflowID != source.WorkflowID || stored.WorkflowStepID != source.WorkflowStepID || stored.Description != "Concurrent edit" { + t.Fatalf("stale change overwrote current task: %+v", stored) + } + if stored.WorkflowAgentOverrides == nil || stored.WorkflowAgentOverrides.WorkflowID != source.WorkflowID { + t.Fatalf("stale change replaced source overrides: %+v", stored.WorkflowAgentOverrides) + } +} + +func TestWorkflowChangeAtomicAdmissionRollsBackOnTaskWriteFailure(t *testing.T) { + repo := newRepoForEntityTests(t) + ctx := context.Background() + source := seedWorkflowChangeRows(t, repo, "task-workflow-change-write-failure") + guard := &models.WorkflowChangeSource{WorkflowID: source.WorkflowID, StepID: source.WorkflowStepID, UpdatedAt: source.UpdatedAt} + candidate := workflowChangeCandidate(t, source) + if _, err := repo.db.Exec(`CREATE TRIGGER fail_workflow_change BEFORE UPDATE ON tasks + WHEN OLD.id = 'task-workflow-change-write-failure' + BEGIN SELECT RAISE(ABORT, 'injected workflow change failure'); END`); err != nil { + t.Fatalf("create injected failure trigger: %v", err) + } + _, err := repo.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, candidate, source.WorkflowStepID, candidate.WorkflowStepID, 0, nil, true, guard, + ) + if err == nil { + t.Fatal("guarded write succeeded despite injected task update failure") + } + if _, err := repo.db.Exec(`DROP TRIGGER fail_workflow_change`); err != nil { + t.Fatalf("drop injected failure trigger: %v", err) + } + stored, err := repo.GetTask(ctx, source.ID) + if err != nil { + t.Fatalf("GetTask after rollback: %v", err) + } + if stored.WorkflowID != source.WorkflowID || stored.WorkflowStepID != source.WorkflowStepID || + stored.WorkflowAgentOverrides == nil || stored.WorkflowAgentOverrides.WorkflowID != source.WorkflowID { + t.Fatalf("failed change partially committed: %+v", stored) + } +} diff --git a/apps/backend/internal/task/repository/sqlite/task.go b/apps/backend/internal/task/repository/sqlite/task.go index 3f6fc74d5c..1ec8977621 100644 --- a/apps/backend/internal/task/repository/sqlite/task.go +++ b/apps/backend/internal/task/repository/sqlite/task.go @@ -22,6 +22,7 @@ import ( "github.com/kandev/kandev/internal/steptelemetry" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/task/recoveryclaim" + "github.com/kandev/kandev/internal/task/repository/repoerrors" usermodels "github.com/kandev/kandev/internal/user/models" wfmodels "github.com/kandev/kandev/internal/workflow/models" workflowmove "github.com/kandev/kandev/internal/workflow/move" @@ -1043,7 +1044,7 @@ func (r *Repository) updateTaskCommit(ctx context.Context, task *models.Task, ex } defer func() { _ = tx.Rollback() }() - entryID, markerEntryID, err := r.updateTaskTx(ctx, tx, task, metadata, expectedWorkflowID, preservePosition, protectDeferredLaunch) + entryID, markerEntryID, err := r.updateTaskTx(ctx, tx, task, metadata, expectedWorkflowID, preservePosition, protectDeferredLaunch, nil) if err != nil { return err } @@ -1102,6 +1103,40 @@ func (r *Repository) readTaskPositionInTx(ctx context.Context, tx *sql.Tx, taskI return position, true, nil } +func (r *Repository) readTaskUpdatedAtInTx(ctx context.Context, tx *sql.Tx, taskID string) (time.Time, error) { + query := `SELECT updated_at FROM tasks WHERE id = ?` + if dialect.IsPostgres(r.db.DriverName()) { + query += forUpdateClause + } + var updatedAt time.Time + if err := tx.QueryRowContext(ctx, r.db.Rebind(query), taskID).Scan(&updatedAt); err != nil { + return time.Time{}, err + } + return updatedAt, nil +} + +func (r *Repository) validateWorkflowChangeSourceInTx( + ctx context.Context, + tx *sql.Tx, + taskID, workflowID, stepID string, + source *models.WorkflowChangeSource, +) error { + if source == nil || workflowID != source.WorkflowID || stepID != source.StepID { + return repoerrors.ErrWorkflowChangeConflict + } + updatedAt, err := r.readTaskUpdatedAtInTx(ctx, tx, taskID) + if errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("%w: %s", ErrTaskNotFound, taskID) + } + if err != nil { + return err + } + if !updatedAt.Equal(source.UpdatedAt) { + return repoerrors.ErrWorkflowChangeConflict + } + return nil +} + // applyPreservedPositionInTx overwrites task.Position with the value read // fresh inside this transaction when preservePosition is set, so a caller's // task object read before the transaction began does not clobber a @@ -1195,32 +1230,21 @@ func encodeWorkflowAgentOverridesValue(overrides *models.WorkflowAgentOverrides) // set only by UpdateTaskPreservingDeferredLaunch, re-merges the row's own // current deferred_launch value into the write so a stale in-memory snapshot // can never resurrect or clobber a concurrent session-ceiling CAS write. -func (r *Repository) updateTaskTx(ctx context.Context, tx *sql.Tx, task *models.Task, metadata []byte, expectedWorkflowID string, preservePosition, protectDeferredLaunch bool) (entryID string, markerEntryID int64, err error) { - fromWorkflowID, fromStepID, found, err := r.readTaskStepInTx(ctx, tx, task.ID) +func (r *Repository) updateTaskTx( + ctx context.Context, + tx *sql.Tx, + task *models.Task, + metadata []byte, + expectedWorkflowID string, + preservePosition, protectDeferredLaunch bool, + workflowChangeSource *models.WorkflowChangeSource, +) (entryID string, markerEntryID int64, err error) { + fromWorkflowID, fromStepID, err := r.readAndValidateTaskUpdateSourceInTx( + ctx, tx, task, expectedWorkflowID, preservePosition, workflowChangeSource, + ) if err != nil { return "", 0, err } - if !found { - // A concurrently deleted task must surface as ErrTaskNotFound, not - // fall through to the CAS comparison below: with fromWorkflowID="" - // (never equal to a non-empty expectedWorkflowID) that branch would - // misreport the deletion as a workflow-resolution conflict. NotFound - // is reserved for the addressed resource and wins the precedence - // ladder over every other case (design's error-mapping table). - return "", 0, fmt.Errorf("%w: %s", ErrTaskNotFound, task.ID) - } - if err := r.applyPreservedPositionInTx(ctx, tx, task, preservePosition); err != nil { - return "", 0, err - } - if expectedWorkflowID != "" && fromWorkflowID != expectedWorkflowID { - // Checked here, immediately before the UPDATE below and using the - // same in-transaction, lock-protected read the ledger's "from" value - // already comes from — this is the narrowest possible point to close - // the race a caller-side pre-read (GetTask, well before this write) - // cannot rule out on its own. See ErrWorkflowResolutionConflict (errors.go). - return "", 0, fmt.Errorf("%w: expected %q, task is now in %q", - ErrWorkflowResolutionConflict, expectedWorkflowID, fromWorkflowID) - } metadata, err = r.preserveLiveHandoffProvenance(ctx, tx, task.ID, metadata) if err != nil { return "", 0, err @@ -1239,12 +1263,20 @@ func (r *Repository) updateTaskTx(ctx context.Context, tx *sql.Tx, task *models. if err != nil { return "", 0, err } - result, err := tx.ExecContext(ctx, r.db.Rebind(updateQuery), task.WorkspaceID, task.WorkflowID, task.WorkflowStepID, workflowAgentOverrides, task.Title, task.Description, task.State, task.Priority, task.Position, dialect.BoolToInt(task.WIPAdmitted), task.QueuedForStepID, task.QueuedAt, string(metadata), task.ParentID, task.UpdatedAt, task.Origin, task.ProjectID, task.Labels, task.Identifier, task.AssigneeUserID, task.ID) + args := []interface{}{task.WorkspaceID, task.WorkflowID, task.WorkflowStepID, workflowAgentOverrides, task.Title, task.Description, task.State, task.Priority, task.Position, dialect.BoolToInt(task.WIPAdmitted), task.QueuedForStepID, task.QueuedAt, string(metadata), task.ParentID, task.UpdatedAt, task.Origin, task.ProjectID, task.Labels, task.Identifier, task.AssigneeUserID, task.ID} + if workflowChangeSource != nil { + updateQuery += ` AND workflow_id = ? AND workflow_step_id = ? AND updated_at = ?` + args = append(args, workflowChangeSource.WorkflowID, workflowChangeSource.StepID, workflowChangeSource.UpdatedAt) + } + result, err := tx.ExecContext(ctx, r.db.Rebind(updateQuery), args...) if err != nil { return "", 0, err } rows, _ := result.RowsAffected() if rows == 0 { + if workflowChangeSource != nil { + return "", 0, repoerrors.ErrWorkflowChangeConflict + } return "", 0, fmt.Errorf("%w: %s", ErrTaskNotFound, task.ID) } @@ -1286,6 +1318,36 @@ func (r *Repository) updateTaskTx(ctx context.Context, tx *sql.Tx, task *models. return entryID, markerEntryID, nil } +func (r *Repository) readAndValidateTaskUpdateSourceInTx( + ctx context.Context, + tx *sql.Tx, + task *models.Task, + expectedWorkflowID string, + preservePosition bool, + workflowChangeSource *models.WorkflowChangeSource, +) (string, string, error) { + fromWorkflowID, fromStepID, found, err := r.readTaskStepInTx(ctx, tx, task.ID) + if err != nil { + return "", "", err + } + if !found { + return "", "", fmt.Errorf("%w: %s", ErrTaskNotFound, task.ID) + } + if err := r.applyPreservedPositionInTx(ctx, tx, task, preservePosition); err != nil { + return "", "", err + } + if expectedWorkflowID != "" && fromWorkflowID != expectedWorkflowID { + return "", "", fmt.Errorf("%w: expected %q, task is now in %q", + ErrWorkflowResolutionConflict, expectedWorkflowID, fromWorkflowID) + } + if workflowChangeSource != nil { + if err := r.validateWorkflowChangeSourceInTx(ctx, tx, task.ID, fromWorkflowID, fromStepID, workflowChangeSource); err != nil { + return "", "", err + } + } + return fromWorkflowID, fromStepID, nil +} + // UpdateTaskWithWorkflowStepAdmission atomically moves a task into a workflow // step. A limited full target stores the task in that destination as queued; // it never rejects the move for WIP capacity. sourceStepID is the step the @@ -1298,7 +1360,7 @@ func (r *Repository) UpdateTaskWithWorkflowStepAdmission( targetStepID string, limit int, ) (bool, error) { - admitted, _, err := r.updateTaskWithWorkflowStepAdmission(ctx, task, sourceStepID, targetStepID, limit, nil, false, "", "", nil) + admitted, _, err := r.updateTaskWithWorkflowStepAdmission(ctx, task, sourceStepID, targetStepID, limit, nil, false, "", "", nil, nil) return admitted, err } @@ -1323,7 +1385,28 @@ func (r *Repository) UpdateTaskWithWorkflowStepAdmissionAndState( expectedWorkflowID string, ) (bool, error) { admitted, _, err := r.updateTaskWithWorkflowStepAdmission( - ctx, task, sourceStepID, targetStepID, limit, admittedState, queueExitPending, "", expectedWorkflowID, nil, + ctx, task, sourceStepID, targetStepID, limit, admittedState, queueExitPending, "", expectedWorkflowID, nil, nil, + ) + return admitted, err +} + +// UpdateTaskWithWorkflowChangeAdmissionAndState writes a change-workflow +// transition only while the persisted source identity and version still match. +func (r *Repository) UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx context.Context, + task *models.Task, + sourceStepID string, + targetStepID string, + limit int, + admittedState *v1.TaskState, + queueExitPending bool, + source *models.WorkflowChangeSource, +) (bool, error) { + if source == nil { + return false, fmt.Errorf("workflow change source guard is required") + } + admitted, _, err := r.updateTaskWithWorkflowStepAdmission( + ctx, task, sourceStepID, targetStepID, limit, admittedState, queueExitPending, "", "", nil, source, ) return admitted, err } @@ -1346,7 +1429,7 @@ func (r *Repository) UpdateTaskWithWorkflowStepAdmissionIfAtStep( ) (applied bool, err error) { // expectedStepID doubles as the source step to lock: it is, by // construction, the step this task is expected to currently occupy. - _, applied, err = r.updateTaskWithWorkflowStepAdmission(ctx, task, expectedStepID, targetStepID, limit, nil, false, expectedStepID, "", nil) + _, applied, err = r.updateTaskWithWorkflowStepAdmission(ctx, task, expectedStepID, targetStepID, limit, nil, false, expectedStepID, "", nil, nil) return applied, err } @@ -1360,7 +1443,7 @@ func (r *Repository) UpdateTaskWithWorkflowStepAdmissionForDeferredMove( // expectedStepID doubles as the source step to lock: it is, by // construction, the step this task is expected to currently occupy. return r.updateTaskWithWorkflowStepAdmission( - ctx, task, expectedStepID, targetStepID, limit, nil, false, expectedStepID, "", &record, + ctx, task, expectedStepID, targetStepID, limit, nil, false, expectedStepID, "", &record, nil, ) } @@ -1404,7 +1487,7 @@ func (r *Repository) MarkDeferredMoveAppliedForSession( if err != nil { return false, err } - if _, _, err := r.updateTaskTx(ctx, tx, task, metadata, "", true, false); err != nil { + if _, _, err := r.updateTaskTx(ctx, tx, task, metadata, "", true, false, nil); err != nil { return false, err } if err := r.deleteDeferredMoveGuardTx(ctx, tx, record); err != nil { @@ -1600,6 +1683,7 @@ func (r *Repository) updateTaskWithWorkflowStepAdmission( expectedStepID string, expectedWorkflowID string, deferredMove *messagequeue.PendingMoveRecord, + workflowChangeSource *models.WorkflowChangeSource, ) (admitted bool, applied bool, err error) { currentSourceStepID := sourceStepID for attempt := 0; attempt < admissionSourceRetryLimit; attempt++ { @@ -1615,6 +1699,7 @@ func (r *Repository) updateTaskWithWorkflowStepAdmission( expectedStepID, expectedWorkflowID, deferredMove, + workflowChangeSource, ) unlock() @@ -1622,6 +1707,9 @@ func (r *Repository) updateTaskWithWorkflowStepAdmission( if !errors.As(err, &changed) { return admitted, applied, err } + if workflowChangeSource != nil { + return false, false, repoerrors.ErrWorkflowChangeConflict + } if expectedStepID != "" { // CAS callers preserve their existing applied=false contract when // the task has left the expected step. They must not retry against @@ -1645,6 +1733,7 @@ func (r *Repository) updateTaskWithWorkflowStepAdmissionAttempt( expectedStepID string, expectedWorkflowID string, deferredMove *messagequeue.PendingMoveRecord, + workflowChangeSource *models.WorkflowChangeSource, ) (admitted bool, applied bool, err error) { now := time.Now().UTC() task.UpdatedAt = now @@ -1691,7 +1780,7 @@ func (r *Repository) updateTaskWithWorkflowStepAdmissionAttempt( // source so it acquires the complete sorted lock set before assigning an // arrival position. This closes the single-move TOCTOU window without // acquiring a newly discovered source beneath an already-held lock. - _, actualSourceStepID, found, err := r.readTaskStepInTx(ctx, tx, task.ID) + actualWorkflowID, actualSourceStepID, found, err := r.readTaskStepInTx(ctx, tx, task.ID) if err != nil { return false, false, err } @@ -1699,8 +1788,16 @@ func (r *Repository) updateTaskWithWorkflowStepAdmissionAttempt( return false, false, fmt.Errorf("%w: %s", ErrTaskNotFound, task.ID) } if actualSourceStepID != sourceStepID { + if workflowChangeSource != nil { + return false, false, repoerrors.ErrWorkflowChangeConflict + } return false, false, &admissionSourceChangedError{stepID: actualSourceStepID} } + if workflowChangeSource != nil { + if err := r.validateWorkflowChangeSourceInTx(ctx, tx, task.ID, actualWorkflowID, actualSourceStepID, workflowChangeSource); err != nil { + return false, false, err + } + } // AC-46/48 compare-and-swap precondition, only for CAS callers (see // rebaseTaskForStepAdmissionCAS for the full rationale). A mismatch means @@ -1773,7 +1870,7 @@ func (r *Repository) updateTaskWithWorkflowStepAdmissionAttempt( if err != nil { metadata = []byte("{}") } - entryID, markerEntryID, err := r.updateTaskTx(ctx, tx, task, metadata, expectedWorkflowID, false, false) + entryID, markerEntryID, err := r.updateTaskTx(ctx, tx, task, metadata, expectedWorkflowID, false, false, workflowChangeSource) if err != nil { return false, false, err } diff --git a/apps/backend/internal/task/service/change_workflow_test.go b/apps/backend/internal/task/service/change_workflow_test.go new file mode 100644 index 0000000000..e55b1e7f7a --- /dev/null +++ b/apps/backend/internal/task/service/change_workflow_test.go @@ -0,0 +1,326 @@ +package service + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "github.com/jmoiron/sqlx" + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/task/models" + sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" + wfmodels "github.com/kandev/kandev/internal/workflow/models" + "github.com/kandev/kandev/internal/workflow/repository" +) + +type workflowChangeExecutorValidator struct { + executorID string + profiles []string +} + +func (v *workflowChangeExecutorValidator) ValidateAgentProfileForExecutor( + _ context.Context, + profile *settingsmodels.AgentProfile, + executor *models.Executor, + _ *models.ExecutorProfile, +) error { + v.profiles = append(v.profiles, profile.ID) + if executor == nil || executor.ID != v.executorID { + return errors.New("unexpected executor") + } + return nil +} + +type workflowChangePreflightRecorder struct { + candidate *models.Task +} + +func (*workflowChangePreflightRecorder) PreflightWorkflowStepMove( + context.Context, + string, + *models.TaskSession, + *wfmodels.WorkflowStep, +) error { + return nil +} + +func (r *workflowChangePreflightRecorder) PreflightWorkflowStepChange( + _ context.Context, + candidate *models.Task, + _ *models.TaskSession, + _ *wfmodels.WorkflowStep, +) error { + copy := *candidate + r.candidate = © + return nil +} + +type workflowChangeFixture struct { + svc *Service + repo *sqliterepo.Repository + task *models.Task + request *models.WorkflowChangeRequest + validator *workflowChangeExecutorValidator +} + +func newWorkflowChangeFixture(t *testing.T) workflowChangeFixture { + t.Helper() + ctx := context.Background() + svc, _, repo := createTestService(t) + workspaceDefaultExecutor := "executor-workspace-default" + for _, executorID := range []string{workspaceDefaultExecutor, "executor-task"} { + if err := repo.CreateExecutor(ctx, &models.Executor{ + ID: executorID, Name: executorID, Type: models.ExecutorTypeLocal, + Status: models.ExecutorStatusActive, Resumable: true, + }); err != nil { + t.Fatalf("CreateExecutor(%s): %v", executorID, err) + } + } + if err := repo.CreateWorkspace(ctx, &models.Workspace{ + ID: "workspace-change", Name: "Change workflow", + DefaultExecutorID: &workspaceDefaultExecutor, + }); err != nil { + t.Fatalf("CreateWorkspace: %v", err) + } + for _, workflow := range []*models.Workflow{ + {ID: "workflow-source", WorkspaceID: "workspace-change", Name: "Source"}, + {ID: "workflow-target", WorkspaceID: "workspace-change", Name: "Target"}, + } { + if err := repo.CreateWorkflow(ctx, workflow); err != nil { + t.Fatalf("CreateWorkflow(%s): %v", workflow.ID, err) + } + } + wfDB := sqlx.NewDb(repo.DB(), "sqlite3") + wfRepo, err := repository.NewWithDB(wfDB, wfDB, nil) + if err != nil { + t.Fatalf("New workflow repository: %v", err) + } + steps := []*wfmodels.WorkflowStep{ + {ID: "source-step", WorkflowID: "workflow-source", Name: "Review", Position: 0, AgentProfileID: "old-source-profile"}, + {ID: "target-analysis", WorkflowID: "workflow-target", Name: "Analysis", Position: 0, AgentProfileID: "source-profile"}, + {ID: "target-implement", WorkflowID: "workflow-target", Name: "Implement", Position: 1, AgentProfileID: "source-profile"}, + {ID: "target-review", WorkflowID: "workflow-target", Name: "Review", Position: 2, AgentProfileID: "source-profile", SessionTarget: &wfmodels.WorkflowSessionTarget{Kind: wfmodels.WorkflowSessionTargetInitial}}, + } + for _, step := range steps { + if err := wfRepo.CreateStep(ctx, step); err != nil { + t.Fatalf("CreateStep(%s): %v", step.ID, err) + } + } + stepMap := make(map[string]*wfmodels.WorkflowStep, len(steps)) + for _, step := range steps { + stepMap[step.ID] = step + } + svc.SetWorkflowStepGetter(&fakeWorkflowStepGetter{steps: stepMap}) + oldOverrides, err := models.NewWorkflowAgentOverrides("workflow-source", []models.WorkflowAgentOverrideBinding{{ + StepID: "source-step", SourceProfileID: "old-source-profile", ReplacementProfileID: "old-replacement-profile", + }}) + if err != nil { + t.Fatalf("NewWorkflowAgentOverrides: %v", err) + } + if err := repo.CreateTask(ctx, &models.Task{ + ID: "task-workflow-change", WorkspaceID: "workspace-change", + WorkflowID: "workflow-source", WorkflowStepID: "source-step", + Title: "Preserved title", Description: "Preserved description", + State: "TODO", Priority: "medium", Metadata: map[string]interface{}{"sentinel": "keep"}, + WorkflowAgentOverrides: oldOverrides, + }); err != nil { + t.Fatalf("CreateTask: %v", err) + } + now := time.Now().UTC() + if err := repo.CreateTaskSession(ctx, &models.TaskSession{ + ID: "session-workflow-change", TaskID: "task-workflow-change", + ExecutorID: "executor-task", State: models.TaskSessionStateWaitingForInput, + StartedAt: now, UpdatedAt: now, IsPrimary: true, + }); err != nil { + t.Fatalf("CreateTaskSession: %v", err) + } + task, err := repo.GetTask(ctx, "task-workflow-change") + if err != nil { + t.Fatalf("GetTask: %v", err) + } + validator := &workflowChangeExecutorValidator{executorID: "executor-task"} + svc.agentProfiles = workflowAgentOverrideProfilesStub{profiles: map[string]*settingsmodels.AgentProfile{ + "source-profile": {ID: "source-profile", Enabled: true, WorkspaceID: "workspace-change"}, + "replacement-profile": {ID: "replacement-profile", Enabled: true, WorkspaceID: "workspace-change"}, + "old-source-profile": {ID: "old-source-profile", Enabled: true, WorkspaceID: "workspace-change"}, + "old-replacement-profile": {ID: "old-replacement-profile", Enabled: true, WorkspaceID: "workspace-change"}, + }} + svc.agentProfileExecutorValidator = validator + return workflowChangeFixture{ + svc: svc, repo: repo, task: task, validator: validator, + request: &models.WorkflowChangeRequest{ + ExpectedWorkflowID: task.WorkflowID, + ExpectedStepID: task.WorkflowStepID, + ExpectedUpdatedAt: task.UpdatedAt, + AgentOverrides: map[string]string{"source-profile": "replacement-profile"}, + }, + } +} + +func TestMoveTaskWithWorkflowChangeReplacesOverridesAndPreservesTaskContext(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + preflight := &workflowChangePreflightRecorder{} + fixture.svc.SetWorkflowMovePreflight(preflight) + + result, err := fixture.svc.MoveTaskWithOptions( + context.Background(), fixture.task.ID, "workflow-target", "target-analysis", 0, + MoveTaskOptions{AllowActivePrimarySession: true, WorkflowChange: fixture.request}, + ) + if err != nil { + t.Fatalf("MoveTaskWithOptions: %v", err) + } + if result.Task.ID != fixture.task.ID || result.Task.WorkflowID != "workflow-target" || result.Task.WorkflowStepID != "target-analysis" { + t.Fatalf("moved task identity = %+v", result.Task) + } + if result.Task.Title != "Preserved title" || result.Task.Description != "Preserved description" || result.Task.Metadata["sentinel"] != "keep" { + t.Fatalf("task context changed: %+v", result.Task) + } + if result.Task.WorkflowAgentOverrides == nil || result.Task.WorkflowAgentOverrides.WorkflowID != "workflow-target" { + t.Fatalf("destination overrides = %+v", result.Task.WorkflowAgentOverrides) + } + for _, stepID := range []string{"target-analysis", "target-implement"} { + if replacement, ok := result.Task.WorkflowAgentOverrides.ReplacementFor("workflow-target", stepID); !ok || replacement != "replacement-profile" { + t.Fatalf("replacement for %s = %q, %v", stepID, replacement, ok) + } + } + if _, ok := result.Task.WorkflowAgentOverrides.ReplacementFor("workflow-target", "target-review"); ok { + t.Fatal("explicit initial-session target received a fixed-profile replacement") + } + if _, ok := result.Task.WorkflowAgentOverrides.ReplacementFor("workflow-source", "source-step"); ok { + t.Fatal("source workflow override remained active after explicit change") + } + if preflight.candidate == nil || preflight.candidate.WorkflowID != "workflow-target" || + preflight.candidate.WorkflowAgentOverrides == nil || + preflight.candidate.WorkflowAgentOverrides.WorkflowID != "workflow-target" { + t.Fatalf("preflight did not receive candidate destination state: %+v", preflight.candidate) + } + if len(fixture.validator.profiles) != 1 || fixture.validator.profiles[0] != "replacement-profile" { + t.Fatalf("validated profiles = %v, want replacement on task executor", fixture.validator.profiles) + } +} + +func TestMoveTaskWithWorkflowChangeAcceptsEquivalentOffsetTimestamp(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + fixture.request.ExpectedUpdatedAt = fixture.request.ExpectedUpdatedAt.In(time.FixedZone("EET", 2*60*60)) + + result, err := fixture.svc.MoveTaskWithOptions( + context.Background(), fixture.task.ID, "workflow-target", "target-analysis", 0, + MoveTaskOptions{AllowActivePrimarySession: true, WorkflowChange: fixture.request}, + ) + if err != nil { + t.Fatalf("MoveTaskWithOptions with equivalent offset timestamp: %v", err) + } + if result.Task.WorkflowID != "workflow-target" || result.Task.WorkflowStepID != "target-analysis" { + t.Fatalf("moved assignment = %s/%s, want workflow-target/target-analysis", result.Task.WorkflowID, result.Task.WorkflowStepID) + } +} + +func TestValidateWorkflowChangeValidatesDefaultsAndRejectsUnavailableAgent(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + fixture.request.AgentOverrides = map[string]string{} + overrides, err := fixture.svc.ValidateWorkflowChange(context.Background(), fixture.task.ID, "workflow-target", "target-analysis", fixture.request) + if err != nil { + t.Fatalf("ValidateWorkflowChange with defaults: %v", err) + } + if overrides != nil { + t.Fatalf("default selections produced overrides: %+v", overrides) + } + if len(fixture.validator.profiles) != 1 || fixture.validator.profiles[0] != "source-profile" { + t.Fatalf("default profile validation = %v, want destination workflow default", fixture.validator.profiles) + } + + fixture.request.AgentOverrides = map[string]string{"source-profile": "disabled-profile"} + _, err = fixture.svc.ValidateWorkflowChange(context.Background(), fixture.task.ID, "workflow-target", "target-analysis", fixture.request) + var validationErr *WorkflowChangeValidationError + if !errors.As(err, &validationErr) || validationErr.Code != WorkflowChangeErrorAgentInvalid || validationErr.SourceProfileID != "source-profile" { + t.Fatalf("unavailable replacement error = %#v, want row validation for source-profile", err) + } +} + +func TestValidateWorkflowChangeRejectsStaleTaskSource(t *testing.T) { + tests := map[string]func(*models.WorkflowChangeRequest){ + "workflow": func(request *models.WorkflowChangeRequest) { request.ExpectedWorkflowID = "another-workflow" }, + "step": func(request *models.WorkflowChangeRequest) { request.ExpectedStepID = "another-step" }, + "version": func(request *models.WorkflowChangeRequest) { + request.ExpectedUpdatedAt = request.ExpectedUpdatedAt.Add(-time.Second) + }, + } + for name, alter := range tests { + t.Run(name, func(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + alter(fixture.request) + _, err := fixture.svc.ValidateWorkflowChange( + context.Background(), fixture.task.ID, "workflow-target", "target-analysis", fixture.request, + ) + if !errors.Is(err, ErrWorkflowChangeConflict) { + t.Fatalf("ValidateWorkflowChange error = %v, want source conflict", err) + } + stored, loadErr := fixture.repo.GetTask(context.Background(), fixture.task.ID) + if loadErr != nil { + t.Fatalf("GetTask after source conflict: %v", loadErr) + } + if stored.WorkflowID != fixture.task.WorkflowID || stored.WorkflowStepID != fixture.task.WorkflowStepID { + t.Fatalf("stale form changed task assignment: %s/%s", stored.WorkflowID, stored.WorkflowStepID) + } + }) + } +} + +func TestValidateWorkflowChangeRejectsProjectOwnedTask(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + officeTask := *fixture.task + officeTask.ProjectID = "office-project" + + _, err := fixture.svc.prepareWorkflowChange( + context.Background(), &officeTask, "workflow-target", "target-analysis", fixture.request, + ) + var validationErr *WorkflowChangeValidationError + if !errors.As(err, &validationErr) || validationErr.Code != WorkflowChangeErrorInvalid { + t.Fatalf("project-owned task change error = %#v, want invalid workflow change", err) + } +} + +func TestMoveTaskWithWorkflowChangePreservesSessionGuards(t *testing.T) { + t.Run("running primary remains allowed", func(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + repo := fixture.repo + if err := repo.UpdateTaskSessionState(context.Background(), "session-workflow-change", models.TaskSessionStateRunning, ""); err != nil { + t.Fatalf("UpdateTaskSessionState: %v", err) + } + if _, err := fixture.svc.MoveTaskWithOptions( + context.Background(), fixture.task.ID, "workflow-target", "target-analysis", 0, + MoveTaskOptions{AllowActivePrimarySession: true, WorkflowChange: fixture.request}, + ); err != nil { + t.Fatalf("MoveTaskWithOptions with running primary: %v", err) + } + }) + + t.Run("running sibling remains blocked", func(t *testing.T) { + fixture := newWorkflowChangeFixture(t) + repo := fixture.repo + now := time.Now().UTC() + if err := repo.CreateTaskSession(context.Background(), &models.TaskSession{ + ID: "session-workflow-change-sibling", TaskID: fixture.task.ID, + ExecutorID: "executor-task", State: models.TaskSessionStateRunning, + StartedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("CreateTaskSession sibling: %v", err) + } + _, err := fixture.svc.MoveTaskWithOptions( + context.Background(), fixture.task.ID, "workflow-target", "target-analysis", 0, + MoveTaskOptions{AllowActivePrimarySession: true, WorkflowChange: fixture.request}, + ) + if err == nil || !strings.Contains(err.Error(), "active session") { + t.Fatalf("MoveTaskWithOptions error = %v, want active sibling conflict", err) + } + stored, loadErr := repo.GetTask(context.Background(), fixture.task.ID) + if loadErr != nil { + t.Fatalf("GetTask after blocked move: %v", loadErr) + } + if stored.WorkflowID != fixture.task.WorkflowID || stored.WorkflowStepID != fixture.task.WorkflowStepID { + t.Fatalf("blocked move changed assignment: %s/%s", stored.WorkflowID, stored.WorkflowStepID) + } + }) +} diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index c28fd52f9c..c255b9d050 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -343,6 +343,12 @@ type WorkflowMovePreflight interface { PreflightWorkflowStepMove(ctx context.Context, taskID string, currentSession *models.TaskSession, targetStep *wfmodels.WorkflowStep) error } +// WorkflowChangeMovePreflight accepts the candidate task projection so +// destination routing checks see the draft override map before it is persisted. +type WorkflowChangeMovePreflight interface { + PreflightWorkflowStepChange(ctx context.Context, candidate *models.Task, currentSession *models.TaskSession, targetStep *wfmodels.WorkflowStep) error +} + // workflowStepLister is an optional extension used to find WIP steps that // pull work from a feeder when new work arrives in that feeder. type workflowStepLister interface { diff --git a/apps/backend/internal/task/service/service_workflow.go b/apps/backend/internal/task/service/service_workflow.go index 1e04414c2a..c47c6b0219 100644 --- a/apps/backend/internal/task/service/service_workflow.go +++ b/apps/backend/internal/task/service/service_workflow.go @@ -469,6 +469,9 @@ type MoveTaskOptions struct { // target workflow step. They are persisted privately on a transient task // marker and are never included in task.moved event payloads. EntryOptions *workflowmove.EntryOptions + // WorkflowChange opts this single-task move into source/version checks and + // atomically replaces the task's destination workflow agent overrides. + WorkflowChange *models.WorkflowChangeRequest } // ErrWorkflowResolutionConflict indicates a caller's pre-resolved "current @@ -514,6 +517,19 @@ type workflowMoveAdmissionWithStateRepository interface { ) (bool, error) } +type workflowChangeAdmissionRepository interface { + UpdateTaskWithWorkflowChangeAdmissionAndState( + context.Context, + *models.Task, + string, + string, + int, + *v1.TaskState, + bool, + *models.WorkflowChangeSource, + ) (bool, error) +} + // workflowMoveConflictRepository is the same-step counterpart of // workflowMoveAdmissionWithStateRepository's expectedWorkflowID guard: when a // move does not change workflow step, updateMovedTask writes through plain @@ -582,11 +598,23 @@ func (s *Service) MoveTaskWithOptions( return nil, fmt.Errorf("%w: resolved %q, task is now in %q", ErrWorkflowResolutionConflict, *opts.ExpectedWorkflowID, task.WorkflowID) } + if opts.WorkflowChange != nil { + if err := validateWorkflowChangeSource(task, workflowID, workflowStepID, opts.WorkflowChange); err != nil { + return nil, err + } + } targetStep, err := s.validateTaskMove(ctx, task, workflowID, workflowStepID, opts) if err != nil { return nil, err } + var candidateOverrides *models.WorkflowAgentOverrides + if opts.WorkflowChange != nil { + candidateOverrides, err = s.prepareWorkflowChange(ctx, task, workflowID, workflowStepID, opts.WorkflowChange) + if err != nil { + return nil, err + } + } oldWorkflowID := task.WorkflowID oldStepID := task.WorkflowStepID @@ -628,8 +656,22 @@ func (s *Service) MoveTaskWithOptions( if stepChanged && targetStep != nil && s.workflowMovePreflight != nil { currentSession := s.resolvePrimaryOrActiveSession(ctx, id) - if err := s.workflowMovePreflight.PreflightWorkflowStepMove(ctx, id, currentSession, targetStep); err != nil { - return nil, fmt.Errorf("failed to preflight workflow move: %w", err) + var preflightErr error + if opts.WorkflowChange != nil { + preflight, ok := s.workflowMovePreflight.(WorkflowChangeMovePreflight) + if !ok { + return nil, fmt.Errorf("workflow change preflight does not accept candidate task state") + } + candidate := *task + candidate.WorkflowID = workflowID + candidate.WorkflowStepID = workflowStepID + candidate.WorkflowAgentOverrides = candidateOverrides + preflightErr = preflight.PreflightWorkflowStepChange(ctx, &candidate, currentSession, targetStep) + } else { + preflightErr = s.workflowMovePreflight.PreflightWorkflowStepMove(ctx, id, currentSession, targetStep) + } + if preflightErr != nil { + return nil, fmt.Errorf("failed to preflight workflow move: %w", preflightErr) } } stateAfterAdmission := *task @@ -641,6 +683,9 @@ func (s *Service) MoveTaskWithOptions( task.WorkflowID = workflowID task.WorkflowStepID = workflowStepID + if opts.WorkflowChange != nil { + task.WorkflowAgentOverrides = candidateOverrides + } // A move naming the task's current step is not an arrival // (REQ-TASKS-KANBAN-TASK-REORDERING-001.28): it keeps the position it // already holds rather than the caller-supplied literal, which the @@ -1357,6 +1402,16 @@ func (s *Service) updateMovedTaskCrossStep( admittedState *v1.TaskState, opts MoveTaskOptions, ) (bool, error) { + if opts.WorkflowChange != nil { + changeRepo, ok := s.tasks.(workflowChangeAdmissionRepository) + if !ok { + return false, fmt.Errorf("workflow change admission repository unavailable for step %s", targetStep.ID) + } + return changeRepo.UpdateTaskWithWorkflowChangeAdmissionAndState( + ctx, task, oldStepID, targetStep.ID, targetStep.WIPLimit, + admittedState, true, workflowChangeGuard(opts.WorkflowChange), + ) + } admissionRepo, ok := s.tasks.(workflowMoveAdmissionRepository) if !ok { return false, fmt.Errorf("workflow step admission repository unavailable for step %s", targetStep.ID) diff --git a/apps/backend/internal/task/service/workflow_change.go b/apps/backend/internal/task/service/workflow_change.go new file mode 100644 index 0000000000..d5e08a2824 --- /dev/null +++ b/apps/backend/internal/task/service/workflow_change.go @@ -0,0 +1,265 @@ +package service + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + + settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" + "github.com/kandev/kandev/internal/task/models" + "github.com/kandev/kandev/internal/task/repository/repoerrors" + wfmodels "github.com/kandev/kandev/internal/workflow/models" +) + +const ( + WorkflowChangeErrorInvalid = "invalid_workflow_change" + WorkflowChangeErrorAgentInvalid = "workflow_agent_unavailable" + WorkflowChangeErrorAgentIncompat = "workflow_agent_incompatible" +) + +var ErrWorkflowChangeConflict = repoerrors.ErrWorkflowChangeConflict + +type WorkflowChangeValidationError struct { + Code string + SourceProfileID string +} + +func (e *WorkflowChangeValidationError) Error() string { + return "invalid workflow change" +} + +func (e *WorkflowChangeValidationError) Unwrap() error { + return ErrInvalidWorkflowChange +} + +var ErrInvalidWorkflowChange = errors.New("invalid workflow change") + +func invalidWorkflowChange(code, sourceProfileID string) error { + return &WorkflowChangeValidationError{Code: code, SourceProfileID: sourceProfileID} +} + +// ValidateWorkflowChange checks a read-only change form submission and returns +// its normalized destination override record. MoveTaskWithOptions repeats this +// validation and adds the transactional source guard before committing. +func (s *Service) ValidateWorkflowChange( + ctx context.Context, + taskID, workflowID, workflowStepID string, + request *models.WorkflowChangeRequest, +) (*models.WorkflowAgentOverrides, error) { + task, err := s.GetTask(ctx, taskID) + if err != nil { + return nil, err + } + return s.prepareWorkflowChange(ctx, task, workflowID, workflowStepID, request) +} + +func (s *Service) prepareWorkflowChange( + ctx context.Context, + task *models.Task, + workflowID, workflowStepID string, + request *models.WorkflowChangeRequest, +) (*models.WorkflowAgentOverrides, error) { + if err := validateWorkflowChangeSource(task, workflowID, workflowStepID, request); err != nil { + return nil, err + } + // Project-backed tasks remain Office-owned if the IsFromOffice projection is missing. + if task.ArchivedAt != nil || task.IsEphemeral || task.IsFromOffice || task.ProjectID != "" { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + workflow, err := s.workflows.GetWorkflow(ctx, workflowID) + if err != nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + if workflow == nil || workflow.Hidden || workflow.WorkspaceID != task.WorkspaceID { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + lister, ok := s.workflowStepGetter.(workflowStepLister) + if !ok || lister == nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + steps, err := lister.ListStepsByWorkflow(ctx, workflowID) + if err != nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + if !containsWorkflowChangeStep(steps, workflowStepID) { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + return s.normalizeWorkflowChangeAgentOverrides(ctx, task, workflowID, steps, request.AgentOverrides) +} + +func validateWorkflowChangeSource( + task *models.Task, + workflowID, workflowStepID string, + request *models.WorkflowChangeRequest, +) error { + if request == nil || task == nil || workflowID == "" || workflowStepID == "" || + strings.TrimSpace(request.ExpectedWorkflowID) == "" || + strings.TrimSpace(request.ExpectedStepID) == "" || request.ExpectedUpdatedAt.IsZero() || request.AgentOverrides == nil { + return invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + if task.WorkflowID != request.ExpectedWorkflowID || task.WorkflowStepID != request.ExpectedStepID || + !task.UpdatedAt.Equal(request.ExpectedUpdatedAt) { + return ErrWorkflowChangeConflict + } + if workflowID == task.WorkflowID { + return invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + return nil +} + +func containsWorkflowChangeStep(steps []*wfmodels.WorkflowStep, stepID string) bool { + for _, step := range steps { + if step != nil && step.ID == stepID { + return true + } + } + return false +} + +func (s *Service) normalizeWorkflowChangeAgentOverrides( + ctx context.Context, + task *models.Task, + workflowID string, + steps []*wfmodels.WorkflowStep, + requested map[string]string, +) (*models.WorkflowAgentOverrides, error) { + fixedSteps := fixedWorkflowAgentOverrideSteps(workflowID, steps) + requestedBySource, err := validateWorkflowChangeSources(requested, fixedSteps) + if err != nil { + return nil, err + } + if len(fixedSteps) == 0 { + return nil, nil + } + executor, executorProfile, err := s.workflowChangeExecutor(ctx, task) + if err != nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + bindings := make([]models.WorkflowAgentOverrideBinding, 0, len(requested)) + validatedProfiles := make(map[string]struct{}) + sources := sortedWorkflowChangeSources(fixedSteps) + for _, sourceID := range sources { + replacementID := sourceID + if requestedID, ok := requestedBySource[sourceID]; ok { + replacementID = requestedID + } + profile, err := s.loadWorkflowChangeProfile(ctx, task.WorkspaceID, replacementID, sourceID) + if err != nil { + return nil, err + } + if _, ok := validatedProfiles[replacementID]; !ok { + if s.agentProfileExecutorValidator == nil || s.agentProfileExecutorValidator.ValidateAgentProfileForExecutor(ctx, profile, executor, executorProfile) != nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorAgentIncompat, sourceID) + } + validatedProfiles[replacementID] = struct{}{} + } + if replacementID == sourceID { + continue + } + for _, step := range fixedSteps[sourceID] { + bindings = append(bindings, models.WorkflowAgentOverrideBinding{ + StepID: step.ID, SourceProfileID: sourceID, ReplacementProfileID: replacementID, + }) + } + } + overrides, err := models.NewWorkflowAgentOverrides(workflowID, bindings) + if err != nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorInvalid, "") + } + return overrides, nil +} + +func validateWorkflowChangeSources( + requested map[string]string, + fixedSteps map[string][]*wfmodels.WorkflowStep, +) (map[string]string, error) { + requestedBySource := make(map[string]string, len(requested)) + for rawSource, rawReplacement := range requested { + source := strings.TrimSpace(rawSource) + replacement := strings.TrimSpace(rawReplacement) + if source == "" || replacement == "" || len(fixedSteps[source]) == 0 { + return nil, invalidWorkflowChange(WorkflowChangeErrorAgentInvalid, source) + } + if _, exists := requestedBySource[source]; exists { + return nil, invalidWorkflowChange(WorkflowChangeErrorAgentInvalid, source) + } + requestedBySource[source] = replacement + } + return requestedBySource, nil +} + +func sortedWorkflowChangeSources(fixedSteps map[string][]*wfmodels.WorkflowStep) []string { + sources := make([]string, 0, len(fixedSteps)) + for source := range fixedSteps { + sources = append(sources, source) + } + sort.Strings(sources) + return sources +} + +func (s *Service) loadWorkflowChangeProfile( + ctx context.Context, + workspaceID, profileID, sourceID string, +) (*settingsmodels.AgentProfile, error) { + if s.agentProfiles == nil { + return nil, invalidWorkflowChange(WorkflowChangeErrorAgentInvalid, sourceID) + } + profile, err := s.agentProfiles.GetAgentProfile(ctx, profileID) + if err != nil || profile == nil || profile.DeletedAt != nil || !profile.Enabled || + (profile.WorkspaceID != "" && profile.WorkspaceID != workspaceID) { + return nil, invalidWorkflowChange(WorkflowChangeErrorAgentInvalid, sourceID) + } + return profile, nil +} + +func (s *Service) workflowChangeExecutor( + ctx context.Context, + task *models.Task, +) (*models.Executor, *models.ExecutorProfile, error) { + executorID := "" + executorProfileID := "" + if s.sessions != nil { + if session, _ := s.sessions.GetPrimarySessionByTaskID(ctx, task.ID); session != nil { + executorID = session.ExecutorID + executorProfileID = session.ExecutorProfileID + } else if session, _ := s.sessions.GetActiveTaskSessionByTaskID(ctx, task.ID); session != nil { + executorID = session.ExecutorID + executorProfileID = session.ExecutorProfileID + } + } + if task.Metadata != nil { + executorID = firstNonEmptyWorkflowAgentOverrideValue(executorID, models.StringFromAny(task.Metadata[models.MetaKeyExecutorID])) + executorProfileID = firstNonEmptyWorkflowAgentOverrideValue(executorProfileID, models.StringFromAny(task.Metadata[models.MetaKeyExecutorProfileID])) + } + if s.executors == nil { + return nil, nil, fmt.Errorf("executor repository unavailable") + } + executorProfile, executorID, err := s.resolveWorkflowAgentOverrideExecutorProfile(ctx, executorID, executorProfileID) + if err != nil { + return nil, nil, err + } + if executorID == "" { + executorID, err = s.resolveWorkflowAgentOverrideWorkspaceExecutor(ctx, task.WorkspaceID) + if err != nil { + return nil, nil, err + } + } + executor, err := s.executors.GetExecutor(ctx, executorID) + if err != nil || executor == nil || executor.DeletedAt != nil || executor.Status == models.ExecutorStatusDisabled { + return nil, nil, fmt.Errorf("task executor is unavailable") + } + return executor, executorProfile, nil +} + +func workflowChangeGuard(request *models.WorkflowChangeRequest) *models.WorkflowChangeSource { + if request == nil { + return nil + } + return &models.WorkflowChangeSource{ + WorkflowID: request.ExpectedWorkflowID, + StepID: request.ExpectedStepID, + UpdatedAt: request.ExpectedUpdatedAt.UTC(), + } +} diff --git a/apps/web/components/kanban-card-menu-entry-renderers.tsx b/apps/web/components/kanban-card-menu-entry-renderers.tsx new file mode 100644 index 0000000000..e5fabb75bc --- /dev/null +++ b/apps/web/components/kanban-card-menu-entry-renderers.tsx @@ -0,0 +1,121 @@ +"use client"; + +import { + ContextMenuItem, + ContextMenuSeparator, + ContextMenuSub, + ContextMenuSubContent, + ContextMenuSubTrigger, +} from "@kandev/ui/context-menu"; +import { + DropdownMenuItem, + DropdownMenuPortal, + DropdownMenuSeparator, + DropdownMenuSub, + DropdownMenuSubContent, + DropdownMenuSubTrigger, +} from "@kandev/ui/dropdown-menu"; +import type { KanbanCardMenuEntry } from "./kanban-card-menu-items"; + +function ContextEntry({ entry }: { entry: KanbanCardMenuEntry }) { + if (entry.kind === "separator") return ; + if (entry.kind === "submenu") { + return ( + + + {entry.icon} + {entry.label} + + + {entry.children.map((child) => ( + + ))} + + + ); + } + + return ( + event.stopPropagation()} + onSelect={() => { + if (!entry.disabled) entry.onSelect?.(); + }} + > + {entry.icon} + {entry.leading} + {entry.label} + {entry.trailing} + + ); +} + +function DropdownEntry({ entry }: { entry: KanbanCardMenuEntry }) { + if (entry.kind === "separator") return ; + if (entry.kind === "submenu") { + return ( + + event.stopPropagation()} + onPointerDown={(event) => event.stopPropagation()} + > + {entry.icon} + {entry.label} + + + + {entry.children.map((child) => ( + + ))} + + + + ); + } + + return ( + event.stopPropagation()} + onPointerDown={(event) => event.stopPropagation()} + onSelect={(event) => { + event.stopPropagation(); + if (!entry.disabled) entry.onSelect?.(); + }} + > + {entry.icon} + {entry.leading} + {entry.label} + {entry.trailing} + + ); +} + +export function KanbanCardContextMenuItems({ entries }: { entries: KanbanCardMenuEntry[] }) { + return ( + <> + {entries.map((entry) => ( + + ))} + + ); +} + +export function KanbanCardDropdownMenuItems({ entries }: { entries: KanbanCardMenuEntry[] }) { + return ( + <> + {entries.map((entry) => ( + + ))} + + ); +} diff --git a/apps/web/components/kanban-card-menu-grouping.test.tsx b/apps/web/components/kanban-card-menu-grouping.test.tsx index d652c46968..646e8586fa 100644 --- a/apps/web/components/kanban-card-menu-grouping.test.tsx +++ b/apps/web/components/kanban-card-menu-grouping.test.tsx @@ -5,6 +5,7 @@ import { pluginRegistry } from "@/lib/plugins/registry"; const PLUGIN_ID = "kandev-plugin-tags"; const ACTION_ID = "quick-tag"; const ACTION_LABEL = "Quick tag"; +const CHANGE_WORKFLOW_ENTRY_KEY = "change-workflow"; const WORKFLOW_ONE = "wf-1"; const WORKFLOW_TWO = "wf-2"; @@ -52,20 +53,20 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => const entries = buildKanbanCardMenuEntries({ ...movementArgs(), - onSendToWorkflow: vi.fn(), + onChangeWorkflow: vi.fn(), onLinkPullRequest: vi.fn(), }); const keys = entryKeys(entries); - const sendToIndex = keys.indexOf("send-to-workflow"); + const changeWorkflowIndex = keys.indexOf(CHANGE_WORKFLOW_ENTRY_KEY); const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}:${ACTION_ID}`); const linkIndex = keys.indexOf("link"); - expect(sendToIndex).toBeGreaterThanOrEqual(0); + expect(changeWorkflowIndex).toBeGreaterThanOrEqual(0); expect(primaryIndex).toBeGreaterThanOrEqual(0); expect(linkIndex).toBeGreaterThanOrEqual(0); - expect(linkIndex).toBeLessThan(sendToIndex); - expect(sendToIndex).toBeLessThan(primaryIndex); + expect(linkIndex).toBeLessThan(changeWorkflowIndex); + expect(changeWorkflowIndex).toBeLessThan(primaryIndex); const archiveIndex = keys.indexOf("archive"); expect(primaryIndex).toBeLessThan(archiveIndex); @@ -91,7 +92,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => parentTaskId: "parent-1", onDetach: vi.fn(), onMoveToStep: vi.fn(), - onSendToWorkflow: vi.fn(), + onChangeWorkflow: vi.fn(), onArchive: vi.fn(), onDelete: vi.fn(), }); @@ -105,7 +106,7 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => "detach", "move-separator", "move-to", - "send-to-workflow", + CHANGE_WORKFLOW_ENTRY_KEY, "plugins-separator", `plugin-primary-${PLUGIN_ID}:${ACTION_ID}`, "remove-separator", @@ -114,6 +115,23 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => ]); }); + it("offers one single-task Change workflow action instead of a workflow submenu", () => { + const onChangeWorkflow = vi.fn(); + const entries = buildKanbanCardMenuEntries({ + ...movementArgs(), + currentStepId: "s1", + onMoveToStep: vi.fn(), + onChangeWorkflow, + }); + + expect(entryKeys(entries)).toContain(CHANGE_WORKFLOW_ENTRY_KEY); + expect(entryKeys(entries)).not.toContain("send-to-workflow"); + const entry = entries.find((candidate) => candidate.key === CHANGE_WORKFLOW_ENTRY_KEY); + expect(entry?.kind).toBe("item"); + if (entry?.kind === "item") entry.onSelect?.(); + expect(onChangeWorkflow).toHaveBeenCalledOnce(); + }); + it("does not add a 'primary' entry when visible(context) returns false", () => { registerPrimaryAction(() => false); diff --git a/apps/web/components/kanban-card-menu-items.test.tsx b/apps/web/components/kanban-card-menu-items.test.tsx index 1e7d12d644..0217088a3f 100644 --- a/apps/web/components/kanban-card-menu-items.test.tsx +++ b/apps/web/components/kanban-card-menu-items.test.tsx @@ -323,20 +323,20 @@ describe("buildKanbanCardMenuEntries — 'primary' group plugin actions", () => ], "wf-2": [{ id: "s3", title: "Step 3" }], }, - onSendToWorkflow: vi.fn(), + onChangeWorkflow: vi.fn(), onLinkPullRequest: vi.fn(), onArchive: vi.fn(), }); const keys = entryKeys(entries); - const sendToIndex = keys.indexOf("send-to-workflow"); + const changeWorkflowIndex = keys.indexOf("change-workflow"); const primaryIndex = keys.indexOf(`plugin-primary-${PLUGIN_ID}:quick-tag`); const archiveIndex = keys.indexOf("archive"); - expect(sendToIndex).toBeGreaterThanOrEqual(0); + expect(changeWorkflowIndex).toBeGreaterThanOrEqual(0); expect(primaryIndex).toBeGreaterThanOrEqual(0); expect(archiveIndex).toBeGreaterThanOrEqual(0); - expect(sendToIndex).toBeLessThan(primaryIndex); + expect(changeWorkflowIndex).toBeLessThan(primaryIndex); expect(primaryIndex).toBeLessThan(archiveIndex); const primaryEntry = entries[primaryIndex]; @@ -666,7 +666,7 @@ describe("buildKanbanCardMenuEntries — move-only disabled state", () => { onEdit: vi.fn(), onSelectPriority: vi.fn(), onMoveToStep: vi.fn(), - onSendToWorkflow: vi.fn(), + onChangeWorkflow: vi.fn(), onLinkPullRequest: vi.fn(), onArchive: vi.fn(), onDelete: vi.fn(), @@ -682,7 +682,7 @@ describe("buildKanbanCardMenuEntries — move-only disabled state", () => { expect(disabled("edit")).toBe(false); expect(disabled("priority")).toBe(false); expect(disabled("move-to")).toBe(true); - expect(disabled("send-to-workflow")).toBe(true); + expect(disabled("change-workflow")).toBe(true); expect(disabled("link")).toBe(false); expect(disabled("archive")).toBe(false); expect(disabled("delete")).toBe(false); diff --git a/apps/web/components/kanban-card-menu-items.tsx b/apps/web/components/kanban-card-menu-items.tsx index 151ea34b54..0109bf542f 100644 --- a/apps/web/components/kanban-card-menu-items.tsx +++ b/apps/web/components/kanban-card-menu-items.tsx @@ -10,21 +10,6 @@ import { IconTrash, IconUnlink, } from "@tabler/icons-react"; -import { - ContextMenuItem, - ContextMenuSeparator, - ContextMenuSub, - ContextMenuSubContent, - ContextMenuSubTrigger, -} from "@kandev/ui/context-menu"; -import { - DropdownMenuItem, - DropdownMenuPortal, - DropdownMenuSeparator, - DropdownMenuSub, - DropdownMenuSubContent, - DropdownMenuSubTrigger, -} from "@kandev/ui/dropdown-menu"; import { stepHasAutoStart, type TaskMoveStep, @@ -43,6 +28,11 @@ import { buildEditMenuEntry } from "./kanban-card-edit-submenu"; import { buildPrimaryPluginEntries } from "./plugins/task-menu-actions"; import { useTranslation } from "react-i18next"; import { t } from "@/lib/i18n"; +import { buildSingleTaskWorkflowEntry } from "./kanban-card-single-workflow-entry"; +export { + KanbanCardContextMenuItems, + KanbanCardDropdownMenuItems, +} from "./kanban-card-menu-entry-renderers"; export { useKanbanCardMoveTargets } from "@/hooks/use-kanban-card-move-targets"; type ItemEntry = { @@ -113,7 +103,7 @@ export type BuildKanbanCardMenuEntriesArgs = { currentStepId?: string | null; workflows: TaskMoveWorkflow[]; stepsByWorkflowId: Record; - /** Disables only move and send-to-workflow entries while a row-local move runs. */ + /** Disables only move and change-workflow entries while a row-local move runs. */ moveDisabled?: boolean; disabled?: boolean; isDeleting?: boolean; @@ -135,7 +125,9 @@ export type BuildKanbanCardMenuEntriesArgs = { onLinkSentryIssue?: () => void; pluginLinkActions?: KanbanPluginLinkAction[]; onMoveToStep?: (stepId: string) => void; + onChangeWorkflow?: () => void; onSendToWorkflow?: (workflowId: string, stepId: string) => void; + isBulkSelection?: boolean; /** Defaults to an empty-id context (no visible plugin actions match it in practice). */ pluginMenuContext?: PluginTaskMenuContext; /** @@ -372,10 +364,10 @@ function buildSendToWorkflowSubmenu({ if (!onSendToWorkflow || !currentWorkflowId || targets.length === 0) return null; return { kind: "submenu", - key: "send-to-workflow", - testId: "task-context-send-to-workflow", + key: "change-workflow-selection", + testId: "task-context-change-workflow-selection", icon: , - label: t("kanban:sendToWorkflow"), + label: t("task:changeWorkflowForSelectedTasks"), disabled, className: "w-56", children: targets.map((workflow) => @@ -389,6 +381,72 @@ function buildSendToWorkflowSubmenu({ }; } +function buildWorkflowMenuEntry({ + currentWorkflowId, + workflows, + stepsByWorkflowId, + disabled, + onChangeWorkflow, + onSendToWorkflow, + isBulkSelection, +}: Pick< + BuildKanbanCardMenuEntriesArgs, + | "currentWorkflowId" + | "workflows" + | "stepsByWorkflowId" + | "onChangeWorkflow" + | "onSendToWorkflow" + | "isBulkSelection" +> & { disabled: boolean }): KanbanCardMenuEntry | null { + const visibleWorkflows = workflows.filter((workflow) => !workflow.hidden); + if (isBulkSelection) { + return buildSendToWorkflowSubmenu({ + currentWorkflowId, + workflows: visibleWorkflows, + stepsByWorkflowId, + disabled, + onSendToWorkflow, + }); + } + return buildSingleTaskWorkflowEntry({ + currentWorkflowId, + hasOtherWorkflows: visibleWorkflows.some((workflow) => workflow.id !== currentWorkflowId), + disabled, + onChangeWorkflow, + }); +} + +function buildCurrentWorkflowMoveEntry( + currentSteps: TaskMoveStep[], + currentStepId: string | null | undefined, + disabled: boolean, + onMoveToStep: ((stepId: string) => void) | undefined, +) { + return buildMoveToCurrentWorkflowSubmenu({ + steps: currentSteps, + currentStepId, + disabled, + onMoveToStep, + }); +} + +// A flat Edit surface cannot reuse plugin entries that include the card-only Edit group. +function resolveCardPluginEntries({ + pluginEntries, + forceFlatEdit, + disabled, + onEdit, + pluginMenuContext, +}: Pick< + BuildKanbanCardMenuEntriesArgs, + "pluginEntries" | "forceFlatEdit" | "onEdit" | "pluginMenuContext" +> & { + disabled: boolean; +}): CardPluginEntries { + if (pluginEntries && !forceFlatEdit) return pluginEntries; + return buildCardPluginEntries({ disabled, onEdit, forceFlatEdit, pluginMenuContext }); +} + export function buildKanbanCardMenuEntries({ currentWorkflowId, currentStepId, @@ -414,12 +472,13 @@ export function buildKanbanCardMenuEntries({ onLinkSentryIssue, pluginLinkActions, onMoveToStep, + onChangeWorkflow, onSendToWorkflow, + isBulkSelection, pluginMenuContext, pluginEntries, forceFlatEdit, }: BuildKanbanCardMenuEntriesArgs): KanbanCardMenuEntry[] { - const visibleWorkflows = workflows.filter((workflow) => !workflow.hidden); const currentSteps = currentWorkflowId ? (stepsByWorkflowId[currentWorkflowId] ?? []) : []; const isProcessing = Boolean(disabled || isDeleting || isArchiving || isDetaching); const priorityEntry = buildPriorityMenuEntry({ @@ -428,30 +487,29 @@ export function buildKanbanCardMenuEntries({ onSelectPriority, }); - const moveToEntry = buildMoveToCurrentWorkflowSubmenu({ - steps: currentSteps, + const moveToEntry = buildCurrentWorkflowMoveEntry( + currentSteps, currentStepId, - disabled: moveDisabled || isProcessing, + Boolean(moveDisabled || isProcessing), onMoveToStep, - }); - const sendToEntry = buildSendToWorkflowSubmenu({ + ); + const sendToEntry = buildWorkflowMenuEntry({ currentWorkflowId, - workflows: visibleWorkflows, + workflows, stepsByWorkflowId, - disabled: moveDisabled || isProcessing, + disabled: Boolean(moveDisabled || isProcessing), + onChangeWorkflow, onSendToWorkflow, + isBulkSelection, }); - // `forceFlatEdit` outranks a prebuilt bundle: those surfaces must never show the submenu form. - const pluginContributions = - pluginEntries && !forceFlatEdit - ? pluginEntries - : buildCardPluginEntries({ - disabled: isProcessing, - onEdit, - forceFlatEdit, - pluginMenuContext, - }); + const pluginContributions = resolveCardPluginEntries({ + pluginEntries, + forceFlatEdit, + disabled: isProcessing, + onEdit, + pluginMenuContext, + }); const linkEntry = buildLinkSubmenu({ disabled: isProcessing, @@ -565,106 +623,3 @@ function buildDetachEntry({ onSelect: onDetach, }; } - -function ContextEntry({ entry }: { entry: KanbanCardMenuEntry }) { - if (entry.kind === "separator") return ; - if (entry.kind === "submenu") { - return ( - - - {entry.icon} - {entry.label} - - - {entry.children.map((child) => ( - - ))} - - - ); - } - - return ( - event.stopPropagation()} - onSelect={() => { - if (!entry.disabled) entry.onSelect?.(); - }} - > - {entry.icon} - {entry.leading} - {entry.label} - {entry.trailing} - - ); -} - -function DropdownEntry({ entry }: { entry: KanbanCardMenuEntry }) { - if (entry.kind === "separator") return ; - if (entry.kind === "submenu") { - return ( - - event.stopPropagation()} - onPointerDown={(event) => event.stopPropagation()} - > - {entry.icon} - {entry.label} - - - - {entry.children.map((child) => ( - - ))} - - - - ); - } - - return ( - event.stopPropagation()} - onPointerDown={(event) => event.stopPropagation()} - onSelect={(event) => { - event.stopPropagation(); - if (!entry.disabled) entry.onSelect?.(); - }} - > - {entry.icon} - {entry.leading} - {entry.label} - {entry.trailing} - - ); -} - -export function KanbanCardContextMenuItems({ entries }: { entries: KanbanCardMenuEntry[] }) { - return ( - <> - {entries.map((entry) => ( - - ))} - - ); -} - -export function KanbanCardDropdownMenuItems({ entries }: { entries: KanbanCardMenuEntry[] }) { - return ( - <> - {entries.map((entry) => ( - - ))} - - ); -} diff --git a/apps/web/components/kanban-card-menu.tsx b/apps/web/components/kanban-card-menu.tsx index 94088bb62d..27a18f962d 100644 --- a/apps/web/components/kanban-card-menu.tsx +++ b/apps/web/components/kanban-card-menu.tsx @@ -9,6 +9,7 @@ import { import { useTaskPluginLinkActions } from "@/components/task/task-session-sidebar-link-actions"; import { cleanupSharesParentWorkspace } from "@/components/task/task-cleanup-summary"; import { TaskDeleteConfirmDialog } from "@/components/task/task-delete-confirm-dialog"; +import { ChangeWorkflowDialog } from "@/components/task/change-workflow-dialog"; import { TaskExternalLinkDialog, type ExternalLinkProvider, @@ -36,7 +37,7 @@ export interface TaskCardMenuParams { steps?: WorkflowStep[]; isDeleting?: boolean; isArchiving?: boolean; - /** Row-local in-flight move guard: disables move/send-to-workflow entries. */ + /** Row-local in-flight move guard: disables move/change-workflow entries. */ isMoving?: boolean; isSelected?: boolean; selectedIds?: Set; @@ -217,6 +218,9 @@ export function useKanbanCardMenus({ onDelete: onDelete ? () => dialogs.setShowDeleteConfirm(true) : undefined, onDetach: task.parentTaskId && !actingOnMultiSelection ? requestDetachConfirmation : undefined, ...buildLinkDialogHandlers(externalLinkAvailability, dialogs), + onChangeWorkflow: () => { + window.setTimeout(() => dialogs.setShowChangeWorkflow(true), 300); + }, pluginLinkActions, }; @@ -239,14 +243,15 @@ export function useKanbanCardMenus({ dropdownMenuEntries: buildKanbanCardMenuEntries({ ...menuBase, onMoveToStep: moveMenu.moveToStepFromDropdown, - onSendToWorkflow: moveMenu.sendTaskToWorkflow, pluginMenuContext, pluginEntries, }), contextMenuEntries: buildKanbanCardMenuEntries({ ...menuBase, onMoveToStep: moveMenu.moveSelectedToStep, - onSendToWorkflow: moveMenu.sendSelectionToWorkflow, + onChangeWorkflow: actingOnMultiSelection ? undefined : menuBase.onChangeWorkflow, + onSendToWorkflow: actingOnMultiSelection ? moveMenu.sendSelectionToWorkflow : undefined, + isBulkSelection: actingOnMultiSelection, pluginMenuContext, pluginEntries, }), @@ -279,6 +284,13 @@ export function KanbanCardDialogs({ }) { return ( <> + void; +}): KanbanCardMenuEntry | null { + if (!currentWorkflowId || !hasOtherWorkflows || !onChangeWorkflow) return null; + return { + kind: "item", + key: "change-workflow", + testId: "task-context-change-workflow", + icon: , + label: t("task:changeWorkflow"), + disabled, + onSelect: onChangeWorkflow, + }; +} diff --git a/apps/web/components/task-command-choices.test.tsx b/apps/web/components/task-command-choices.test.tsx index 065a11d31b..bbb8cbd79f 100644 --- a/apps/web/components/task-command-choices.test.tsx +++ b/apps/web/components/task-command-choices.test.tsx @@ -4,10 +4,10 @@ import { pluginCommandChoices, useTaskMoveChoices } from "./task-command-choices afterEach(cleanup); -it("omits workflows without destinations while preserving both destination actions", () => { +it("offers one task-scoped workflow-change command for every other workflow", () => { const openMoveOptions = vi.fn(); const moveImmediately = vi.fn(); - const target = { id: "review", title: "Review", color: "bg-blue-500" }; + const onChangeWorkflow = vi.fn(); const { result } = renderHook(() => useTaskMoveChoices({ task: { id: "task", title: "Task", workflowId: "current" }, @@ -17,17 +17,21 @@ it("omits workflows without destinations while preserving both destination actio { id: "missing", name: "Missing" }, { id: "target", name: "Target" }, ], - stepsByWorkflowId: { empty: [], target: [target] }, + stepsByWorkflowId: { empty: [], target: [{ id: "review", title: "Review" }] }, openMoveOptions, moveImmediately, + onChangeWorkflow, }), ); - expect(result.current.workflows.map((item) => item.label)).toEqual(["Target"]); - const destination = result.current.workflows[0].children![0]; - destination.action?.(); - destination.immediateAction?.(); - expect(openMoveOptions).toHaveBeenCalledWith({ ...target, workflow_id: "target" }); - expect(moveImmediately).toHaveBeenCalledWith({ ...target, workflow_id: "target" }); + expect(result.current.workflows).toHaveLength(1); + expect(result.current.workflows[0]).toMatchObject({ + id: "task-change-workflow", + label: "Change workflow...", + }); + result.current.workflows[0].action?.(); + expect(onChangeWorkflow).toHaveBeenCalledOnce(); + expect(openMoveOptions).not.toHaveBeenCalled(); + expect(moveImmediately).not.toHaveBeenCalled(); }); /** diff --git a/apps/web/components/task-command-choices.tsx b/apps/web/components/task-command-choices.tsx index 62f423f3f0..9fd647fc9f 100644 --- a/apps/web/components/task-command-choices.tsx +++ b/apps/web/components/task-command-choices.tsx @@ -1,4 +1,5 @@ import { createElement } from "react"; +import { IconLogicBuffer } from "@tabler/icons-react"; import { useTranslation } from "react-i18next"; import type { CommandItem } from "@/lib/commands/types"; import { useSetTaskColor, useTaskColor } from "@/hooks/use-task-color"; @@ -33,6 +34,7 @@ type ChoiceOptions = { linkHandlers: TaskLinkHandlers; openMoveOptions: (step: TaskMoveStep) => void; moveImmediately: (step: TaskMoveStep) => void; + onChangeWorkflow?: () => void; }; /** @@ -175,6 +177,7 @@ export function useTaskMoveChoices({ stepsByWorkflowId, openMoveOptions, moveImmediately, + onChangeWorkflow, }: Omit) { const { t } = useTranslation(); const { currentSteps, targets } = taskMoveOptions(task.workflowId, workflows, stepsByWorkflowId); @@ -198,15 +201,16 @@ export function useTaskMoveChoices({ const steps = currentSteps .filter((step) => step.id !== task.workflowStepId) .map((step) => choice(step, task.workflowId!, group)); - const workflowChoices: CommandItem[] = targets - .filter((workflow) => (stepsByWorkflowId[workflow.id]?.length ?? 0) > 0) - .map((workflow) => ({ - id: `task-workflow-${workflow.id}`, - label: workflow.name, - group: t("task:sendToWorkflow"), - children: (stepsByWorkflowId[workflow.id] ?? []).map((step) => - choice(step, workflow.id, workflow.name), - ), - })); + const workflowChoices: CommandItem[] = targets.length + ? [ + { + id: "task-change-workflow", + label: t("task:changeWorkflow"), + group, + icon: , + action: onChangeWorkflow, + }, + ] + : []; return { steps, workflows: workflowChoices }; } diff --git a/apps/web/components/task-command-items.tsx b/apps/web/components/task-command-items.tsx index 4e8cb9cd7c..16a5fcc525 100644 --- a/apps/web/components/task-command-items.tsx +++ b/apps/web/components/task-command-items.tsx @@ -77,7 +77,16 @@ export function buildSidebarTaskCommands(ctx: TaskCommandContext): CommandItem[] items.push(command("task-detach", t("task:detachFromParent"), IconUnlink, ctx.onDetach)); if (available.move) { choices("task-move", t("task:moveTo"), IconArrowRight, ctx.steps); - choices("task-send-workflow", t("task:sendToWorkflow"), IconLogicBuffer, ctx.workflows); + if (ctx.workflows.length) { + items.push( + command( + "task-change-workflow", + t("task:changeWorkflow"), + IconLogicBuffer, + ctx.workflows[0]?.action, + ), + ); + } } items.push(...ctx.plugins); items.push({ diff --git a/apps/web/components/task-commands.test.tsx b/apps/web/components/task-commands.test.tsx index f5b4c80965..8d83737233 100644 --- a/apps/web/components/task-commands.test.tsx +++ b/apps/web/components/task-commands.test.tsx @@ -43,7 +43,7 @@ describe("sidebar task command parity", () => { "task-link", "task-detach", "task-move", - "task-send-workflow", + "task-change-workflow", PLUGIN_ACTION, "task-delete", ]); @@ -94,4 +94,17 @@ describe("sidebar task command parity", () => { expect(buildSidebarTaskCommands(ctx).map((c) => c.id)).not.toContain(PLUGIN_ACTION); expect(buildSidebarTaskCommands(ctx).map((c) => c.id)).not.toContain("task-move"); }); + + it("does not offer workflow moves for Office-owned tasks", () => { + const commands = buildSidebarTaskCommands( + context({ + task: { + ...context().task, + isFromOffice: true, + }, + }), + ); + expect(commands.map((command) => command.id)).not.toContain("task-move"); + expect(commands.map((command) => command.id)).not.toContain("task-change-workflow"); + }); }); diff --git a/apps/web/components/task-commands.tsx b/apps/web/components/task-commands.tsx index 26a1221ac2..5f76c5ebc4 100644 --- a/apps/web/components/task-commands.tsx +++ b/apps/web/components/task-commands.tsx @@ -1,6 +1,6 @@ "use client"; -import { useMemo } from "react"; +import { useMemo, useState } from "react"; import { useTranslation } from "react-i18next"; import { useAppStore, useAppStoreApi } from "./state-provider"; import { useRegisterCommands } from "@/hooks/use-register-commands"; @@ -13,6 +13,7 @@ import { buildSidebarTaskCommands } from "./task-command-items"; import { useTaskCommandChoices } from "./task-command-choices"; import type { TaskSwitcherItem } from "./task/task-switcher-types"; import type { CommandItem } from "@/lib/commands/types"; +import { ChangeWorkflowDialog } from "@/components/task/change-workflow-dialog"; /** One task action host, shared by desktop and phone and independent of sessions. */ export function TaskCommands() { @@ -43,6 +44,7 @@ function TaskCommandsForTask({ task, workspaceId, data }: TaskCommandsForTaskPro const prefs = useSidebarTaskPrefs(); const linkHandlers = useSidebarTaskLinking(workspaceId, actions); const repositories = useAppStore((state) => state.repositories.itemsByWorkspaceId[workspaceId]); + const [changeWorkflowOpen, setChangeWorkflowOpen] = useState(false); const move = useTaskMoveOptions({ taskId: task.id, workflowId: task.workflowId, @@ -54,6 +56,7 @@ function TaskCommandsForTask({ task, workspaceId, data }: TaskCommandsForTaskPro stepsByWorkflowId: data.stepsByWorkflowId, linkHandlers, openMoveOptions: move.openMoveOptionsStep, + onChangeWorkflow: () => window.setTimeout(() => setChangeWorkflowOpen(true), 300), moveImmediately: (step) => { void move.moveImmediately(step); }, @@ -101,6 +104,12 @@ function TaskCommandsForTask({ task, workspaceId, data }: TaskCommandsForTaskPro return move.submitMoveOptions(options); }} /> + ); } diff --git a/apps/web/components/task/change-workflow-dialog.test.tsx b/apps/web/components/task/change-workflow-dialog.test.tsx new file mode 100644 index 0000000000..8840172c1a --- /dev/null +++ b/apps/web/components/task/change-workflow-dialog.test.tsx @@ -0,0 +1,223 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import type { useChangeWorkflow } from "@/hooks/domains/kanban/use-change-workflow"; +import { ChangeWorkflowDialog } from "./change-workflow-dialog"; + +const CURRENT_AGENT_LABEL = "Current agent"; +const CURRENT_PROFILE_ID = "profile-current"; +const SUBMIT_BUTTON_TEST_ID = "change-workflow-submit"; + +const { useChangeWorkflowMock, responsiveMock } = vi.hoisted(() => ({ + useChangeWorkflowMock: vi.fn(), + responsiveMock: { isMobile: false, isFinePointer: true }, +})); + +vi.mock("@/hooks/domains/kanban/use-change-workflow", () => ({ + useChangeWorkflow: useChangeWorkflowMock, +})); + +vi.mock("@/hooks/use-responsive-breakpoint", () => ({ + useResponsiveBreakpoint: () => responsiveMock, +})); + +vi.mock("@/components/task/workflow-move-preview-footer", () => ({ + WorkflowMovePreviewFooter: () =>
, +})); + +type HookState = ReturnType; + +function makeHookState(): HookState { + const task = { + id: "task-1", + title: "Keep the thread", + workflow_id: "source-workflow", + workflow_step_id: "source-step", + updated_at: "2026-09-22T12:00:00Z", + workflow_agent_overrides: { + workflow_id: "source-workflow", + steps: [{ source_profile_id: "profile-old", replacement_profile_id: "profile-retired" }], + }, + } as unknown as HookState["task"]; + const step = ( + id: string, + name: string, + sessionTarget?: { kind: "initial" } | { kind: "step"; step_id: string }, + ) => ({ + id, + name, + position: id === "build" ? 0 : 1, + color: "#abcdef", + session_target: sessionTarget, + }); + return { + task, + taskStatus: "success", + taskError: null, + workflows: [], + workflowsStatus: "success", + workflowsError: null, + destinations: [{ id: "destination", name: "Product", workspace_id: "workspace-1" }], + selectedWorkflowId: "destination", + selectedWorkflow: { id: "destination", name: "Product", workspace_id: "workspace-1" }, + changeWorkflow: vi.fn(), + snapshot: { + workflow: { id: "destination", name: "Product" }, + steps: [ + step("build", "Build"), + step("review", "Review", { kind: "step", step_id: "build" }), + step("wrap-up", "Wrap-up", { kind: "initial" }), + ], + tasks: [], + } as unknown as HookState["snapshot"], + snapshotStatus: "success", + snapshotError: null, + retrySnapshot: vi.fn(), + profiles: [], + profilesStatus: "success", + profilesError: null, + profileOptions: [ + { + value: CURRENT_PROFILE_ID, + label: CURRENT_AGENT_LABEL, + renderLabel: () => CURRENT_AGENT_LABEL, + }, + ], + rows: [ + { + sourceProfileId: CURRENT_PROFILE_ID, + sourceLabel: CURRENT_AGENT_LABEL, + sourceAgentName: "agent", + stepIds: ["build", "review"], + stepNames: ["Build", "Review"], + replacementProfileId: "profile-retired", + replacementLabel: "Retired agent", + replacementAvailable: false, + }, + ], + setOverride: vi.fn(), + overrides: {}, + selectedStepId: "build", + setSelectedStepId: vi.fn(), + sourceWorkflowName: "Backlog", + sourceStepName: "Todo", + sourceChanged: false, + uncertainResult: false, + submitError: null, + sourceProfileErrorId: undefined, + isSubmitting: false, + canSubmit: false, + submit: vi.fn(), + retryTask: vi.fn(), + retryWorkflows: vi.fn(), + retryProfiles: vi.fn(), + workflowChange: { + expected_workflow_id: "source-workflow", + expected_step_id: "source-step", + expected_updated_at: "2026-09-22T12:00:00Z", + agent_overrides: {}, + }, + } as unknown as HookState; +} + +afterEach(() => { + cleanup(); +}); + +beforeEach(() => { + responsiveMock.isMobile = false; + responsiveMock.isFinePointer = true; + useChangeWorkflowMock.mockReset(); + useChangeWorkflowMock.mockReturnValue(makeHookState()); +}); + +describe("ChangeWorkflowDialog", () => { + it("shows the destination form, task-scoped agent mapping, and conversation relationships", () => { + render( + , + ); + + expect(screen.getByRole("dialog", { name: "Change workflow..." })).toBeTruthy(); + expect(screen.getByText("Backlog · Todo")).toBeTruthy(); + expect(screen.getByTestId("change-workflow-destination")).toBeTruthy(); + expect(screen.getByTestId("change-workflow-step")).toBeTruthy(); + expect( + screen.getByText( + "Changing workflows replaces this task's existing workflow agent overrides.", + ), + ).toBeTruthy(); + expect(screen.getByText("Review uses the conversation from Build.")).toBeTruthy(); + expect(screen.getByText("Wrap-up uses the initial conversation.")).toBeTruthy(); + expect(screen.getByTestId("workflow-entry-preview")).toBeTruthy(); + expect(screen.getByRole("alert").textContent).toContain("This profile is unavailable"); + expect((screen.getByTestId(SUBMIT_BUTTON_TEST_ID) as HTMLButtonElement).disabled).toBe(true); + }); + + it("uses touch sizing on coarse-pointer tablets and always discloses override replacement", () => { + responsiveMock.isFinePointer = false; + const state = makeHookState(); + state.task = { ...state.task!, workflow_agent_overrides: undefined }; + useChangeWorkflowMock.mockReturnValue(state); + render( + , + ); + + expect(screen.getByRole("dialog", { name: "Change workflow..." })).toBeTruthy(); + expect(screen.queryByTestId("change-workflow-drawer")).toBeNull(); + expect(screen.getByTestId("change-workflow-close").className).toContain("size-11"); + expect(screen.getByTestId(SUBMIT_BUTTON_TEST_ID).className).toContain("min-h-12"); + expect( + screen.getByText( + "Changing workflows replaces this task's existing workflow agent overrides.", + ), + ).toBeTruthy(); + }); + + it("resets a replacement to the workflow profile and returns cancel to the caller", () => { + const state = makeHookState(); + useChangeWorkflowMock.mockReturnValue(state); + const onOpenChange = vi.fn(); + render( + , + ); + + fireEvent.click(screen.getByTestId(`change-workflow-profile-reset-${CURRENT_PROFILE_ID}`)); + expect(state.setOverride).toHaveBeenCalledWith(CURRENT_PROFILE_ID, CURRENT_PROFILE_ID); + fireEvent.click(screen.getByTestId("change-workflow-cancel")); + expect(onOpenChange).toHaveBeenCalledWith(false); + }); + + it("uses a full-height phone surface with one scroll area and safe-area action space", () => { + responsiveMock.isMobile = true; + render( + , + ); + + const drawer = screen.getByTestId("change-workflow-drawer"); + const body = screen.getByTestId("change-workflow-scroll"); + const footer = screen.getByTestId(SUBMIT_BUTTON_TEST_ID).parentElement; + expect(drawer.className).toContain("100dvh"); + expect(body.className).toContain("overflow-y-auto"); + expect(footer?.className).toContain("safe-area-inset-bottom"); + expect(screen.getByTestId(SUBMIT_BUTTON_TEST_ID).className).toContain("min-h-12"); + }); +}); diff --git a/apps/web/components/task/change-workflow-dialog.tsx b/apps/web/components/task/change-workflow-dialog.tsx new file mode 100644 index 0000000000..5fe2004de6 --- /dev/null +++ b/apps/web/components/task/change-workflow-dialog.tsx @@ -0,0 +1,180 @@ +"use client"; + +import { useId, type RefObject } from "react"; +import { IconX } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { + Dialog, + DialogContent, + DialogDescription, + DialogHeader, + DialogTitle, +} from "@kandev/ui/dialog"; +import { + Drawer, + DrawerContent, + DrawerDescription, + DrawerHeader, + DrawerTitle, +} from "@kandev/ui/drawer"; +import { + ChangeWorkflowActionFooter, + ChangeWorkflowAgentSection, + ChangeWorkflowCurrentTask, + ChangeWorkflowEntryPreview, + ChangeWorkflowSelectors, + ChangeWorkflowSubmitError, +} from "@/components/task/change-workflow-form-sections"; +import { useResponsiveBreakpoint } from "@/hooks/use-responsive-breakpoint"; +import { createFocusReturnHandler } from "@/lib/dialog-focus-return"; +import { cn } from "@/lib/utils"; +import { useTranslation } from "react-i18next"; +import { useChangeWorkflow } from "@/hooks/domains/kanban/use-change-workflow"; + +type ChangeWorkflowDialogProps = { + open: boolean; + onOpenChange: (open: boolean) => void; + taskId: string | null; + workspaceId: string | null; + focusReturnRef?: RefObject; + onSuccess?: () => void; +}; + +function ChangeWorkflowForm({ + taskId, + workspaceId, + open, + onOpenChange, + onSuccess, + isTouchSurface, +}: { + taskId: string | null; + workspaceId: string | null; + open: boolean; + onOpenChange: (open: boolean) => void; + onSuccess?: () => void; + isTouchSurface: boolean; +}) { + const workflowSelectId = useId(); + const stepSelectId = useId(); + const state = useChangeWorkflow({ open, taskId, workspaceId, onOpenChange, onSuccess }); + const sectionClass = cn("grid gap-3", isTouchSurface && "gap-4"); + + return ( +
{ + event.preventDefault(); + void state.submit(); + }} + > +
+
+ + + + + +
+
+ + + ); +} + +export function ChangeWorkflowDialog({ + open, + onOpenChange, + taskId, + workspaceId, + focusReturnRef, + onSuccess, +}: ChangeWorkflowDialogProps) { + const { t } = useTranslation(); + const { isFinePointer, isMobile } = useResponsiveBreakpoint(); + const isTouchSurface = isMobile || !isFinePointer; + const closeFocus = createFocusReturnHandler(focusReturnRef); + const surfaceContent = ( + <> +
+
+ {isMobile ? ( + + {t("task:changeWorkflowTitle")} + {t("task:changeWorkflowDescription")} + + ) : ( + + {t("task:changeWorkflowTitle")} + {t("task:changeWorkflowDescription")} + + )} +
+ +
+ + + ); + + if (isMobile) { + return ( + + +
+ {surfaceContent} +
+ + + ); + } + + return ( + + + {surfaceContent} + + + ); +} diff --git a/apps/web/components/task/change-workflow-form-sections.tsx b/apps/web/components/task/change-workflow-form-sections.tsx new file mode 100644 index 0000000000..10e17b0a93 --- /dev/null +++ b/apps/web/components/task/change-workflow-form-sections.tsx @@ -0,0 +1,521 @@ +import { IconLoader2, IconRefresh } from "@tabler/icons-react"; +import { Button } from "@kandev/ui/button"; +import { Combobox } from "@/components/combobox"; +import type { ComboboxOption } from "@/components/combobox"; +import { WorkflowMovePreviewFooter } from "@/components/task/workflow-move-preview-footer"; +import { cn } from "@/lib/utils"; +import { useTranslation } from "react-i18next"; +import { useChangeWorkflow } from "@/hooks/domains/kanban/use-change-workflow"; +import type { WorkflowStepDTO } from "@/lib/types/http"; + +export type ChangeWorkflowState = ReturnType; +type TouchProps = { isTouchSurface: boolean }; + +function RetryButton({ + onClick, + label, + testId, +}: { + onClick: () => void; + label: string; + testId?: string; +}) { + return ( + + ); +} + +export function ChangeWorkflowCurrentTask({ state }: { state: ChangeWorkflowState }) { + const { t } = useTranslation(); + return ( + <> + {state.taskStatus === "loading" && ( +
+
+ )} + {state.taskStatus === "error" && ( +
+ {t("task:changeWorkflowLoadTaskFailed")} + +
+ )} + {state.task && ( +
+ + {t("task:changeWorkflowCurrentLabel")} + + + {t("task:changeWorkflowCurrentAssignment", { + workflow: state.sourceWorkflowName ?? state.task.workflow_id, + step: state.sourceStepName ?? state.task.workflow_step_id, + })} + + {state.task.title} +
+ )} + {state.sourceChanged && ( +
+ {t("task:changeWorkflowSourceChanged")} +
+ )} + + ); +} + +function stepOptions(steps: readonly WorkflowStepDTO[]): ComboboxOption[] { + return [...steps] + .sort((left, right) => left.position - right.position || left.id.localeCompare(right.id)) + .map((step) => ({ + value: step.id, + label: step.name, + renderLabel: () => ( + + + {step.name} + + ), + })); +} + +function WorkflowPickerSection({ + state, + isTouchSurface, + workflowSelectId, +}: { + state: ChangeWorkflowState; + isTouchSurface: boolean; + workflowSelectId: string; +}) { + const { t } = useTranslation(); + const rowClass = cn("grid gap-1.5", isTouchSurface && "gap-2"); + const controlClass = isTouchSurface ? "h-11 w-full" : "h-7 w-full"; + const workflows: ComboboxOption[] = state.destinations.map((workflow) => ({ + value: workflow.id, + label: workflow.name, + })); + return ( + <> + {state.workflowsStatus === "loading" && ( + + )} + {state.workflowsStatus === "error" && ( + + )} + {state.workflowsStatus === "success" && state.destinations.length === 0 && ( +

+ {t("task:changeWorkflowNoDestinations")} +

+ )} + {state.workflowsStatus === "success" && state.destinations.length > 0 && ( +
+ {t("task:changeWorkflowDestinationLabel")} + +
+ )} + + ); +} + +function StepPickerSection({ + state, + isTouchSurface, + stepSelectId, +}: { + state: ChangeWorkflowState; + isTouchSurface: boolean; + stepSelectId: string; +}) { + const { t } = useTranslation(); + const steps = state.snapshot?.steps ?? []; + const rowClass = cn("grid gap-1.5", isTouchSurface && "gap-2"); + const controlClass = isTouchSurface ? "h-11 w-full" : "h-7 w-full"; + return ( + <> + {state.selectedWorkflowId && state.snapshotStatus === "loading" && ( + + )} + {state.selectedWorkflowId && state.snapshotStatus === "error" && ( + + )} + {state.snapshotStatus === "success" && steps.length === 0 && ( +

+ {t("task:changeWorkflowNoSteps")} +

+ )} + {state.snapshotStatus === "success" && steps.length > 0 && ( +
+ {t("task:changeWorkflowStepLabel")} + +
+ )} + + ); +} + +export function ChangeWorkflowSelectors({ + state, + isTouchSurface, + workflowSelectId, + stepSelectId, +}: { + state: ChangeWorkflowState; + isTouchSurface: boolean; + workflowSelectId: string; + stepSelectId: string; +}) { + return ( + <> + + + + ); +} + +function LoadingStatus({ label }: { label: string }) { + return ( +
+
+ ); +} + +function LoadError({ label, onRetry }: { label: string; onRetry: () => void }) { + const { t } = useTranslation(); + return ( +
+ {label} + +
+ ); +} + +type ConversationRelationship = + | { id: string; stepName: string; kind: "initial" } + | { id: string; stepName: string; kind: "step"; targetName: string }; + +function conversationRelationships(steps: readonly WorkflowStepDTO[]): ConversationRelationship[] { + return steps.flatMap((step) => { + const target = step.session_target; + if (target?.kind === "initial") return [{ id: step.id, stepName: step.name, kind: "initial" }]; + if (target?.kind !== "step") return []; + const targetStep = steps.find((candidate) => candidate.id === target.step_id); + return [ + { + id: step.id, + stepName: step.name, + kind: "step", + targetName: targetStep?.name ?? "", + }, + ]; + }); +} + +function WorkflowAgentRow({ + state, + row, + isTouchSurface, +}: { + state: ChangeWorkflowState; + row: ChangeWorkflowState["rows"][number]; + isTouchSurface: boolean; +}) { + const { t } = useTranslation(); + const invalid = !row.replacementAvailable || state.sourceProfileErrorId === row.sourceProfileId; + const options: ComboboxOption[] = state.profileOptions.map((option) => ({ + ...option, + disabled: false, + })); + if (!options.some((option) => option.value === row.replacementProfileId)) { + options.unshift({ + value: row.replacementProfileId, + label: row.replacementLabel ?? row.sourceLabel, + disabled: true, + disabledReason: t("task:changeWorkflowProfileUnavailable"), + }); + } + return ( +
+
+ {row.sourceLabel} + + {t("task:changeWorkflowProfileSteps", { steps: row.stepNames.join(", ") })} + +
+
+ state.setOverride(row.sourceProfileId, value)} + ariaLabel={t("task:changeWorkflowReplacementLabel", { profile: row.sourceLabel })} + placeholder={t("task:changeWorkflowProfilePlaceholder")} + searchPlaceholder={t("task:searchAgents")} + emptyMessage={t("task:noAgentFound")} + dropdownLabel={t("task:agentProfile2")} + disabled={state.isSubmitting || state.profilesStatus !== "success"} + touchTarget={isTouchSurface} + triggerClassName={cn(isTouchSurface ? "h-11 w-full" : "h-7 w-full", "min-w-0 flex-1")} + testId={`change-workflow-profile-selector-${row.sourceProfileId}`} + popoverPortal + /> + +
+ {invalid && ( +

+ {t("task:changeWorkflowProfileUnavailable")} +

+ )} +
+ ); +} + +function ConversationSection({ steps }: { steps: readonly WorkflowStepDTO[] }) { + const { t } = useTranslation(); + const relationships = conversationRelationships(steps); + if (relationships.length === 0) return null; + return ( +
+

+ {t("task:changeWorkflowConversationSection")} +

+ {relationships.map((relationship) => ( +

+ {relationship.kind === "initial" + ? t("task:changeWorkflowInitialConversation", { step: relationship.stepName }) + : t("task:changeWorkflowStepConversation", { + step: relationship.stepName, + target: relationship.targetName, + })} +

+ ))} +
+ ); +} + +export function ChangeWorkflowAgentSection({ + state, + isTouchSurface, +}: { state: ChangeWorkflowState } & TouchProps) { + const { t } = useTranslation(); + if (!state.selectedWorkflowId) return null; + const steps = state.snapshot?.steps ?? []; + return ( +
+
+

+ {t("task:changeWorkflowAgentSection")} +

+

{t("task:changeWorkflowAgentScope")}

+
+ {state.profilesStatus === "loading" && ( + + )} + {state.profilesStatus === "error" && ( + + )} + {state.profilesStatus === "success" && state.rows.length === 0 && ( +

{t("task:changeWorkflowNoFixedProfiles")}

+ )} + {state.profilesStatus === "success" && + state.rows.map((row) => ( + + ))} + {state.snapshotStatus === "success" && } +

+ {t("task:changeWorkflowReplacePreviousOverrides")} +

+
+ ); +} + +export function ChangeWorkflowEntryPreview({ + state, + isTouchSurface, +}: { state: ChangeWorkflowState } & TouchProps) { + const { t } = useTranslation(); + if ( + state.snapshotStatus !== "success" || + !state.selectedStepId || + !state.task || + !state.workflowChange + ) + return null; + return ( +
+

+ {t("task:changeWorkflowEntryPreview")} +

+ +
+ ); +} + +export function ChangeWorkflowSubmitError({ state }: { state: ChangeWorkflowState }) { + const { t } = useTranslation(); + const error = state.submitError; + if (!error) return null; + let message = t("task:changeWorkflowFailed"); + if (error.code === "workflow_change_conflict") message = t("task:changeWorkflowSourceChanged"); + if (error.code === "workflow_change_uncertain") message = t("task:changeWorkflowUncertain"); + return ( +
+ {message} + {error.code === "workflow_change_uncertain" && ( + + )} +
+ ); +} + +export function ChangeWorkflowActionFooter({ + state, + isTouchSurface, + onOpenChange, +}: TouchProps & { state: ChangeWorkflowState; onOpenChange: (open: boolean) => void }) { + const { t } = useTranslation(); + return ( +
+ + +
+ ); +} diff --git a/apps/web/components/task/task-actions-menu-dialogs.tsx b/apps/web/components/task/task-actions-menu-dialogs.tsx index 1b52eee297..9ecf168cc2 100644 --- a/apps/web/components/task/task-actions-menu-dialogs.tsx +++ b/apps/web/components/task/task-actions-menu-dialogs.tsx @@ -11,6 +11,7 @@ import { TaskExternalLinkDialog } from "@/components/task/task-external-link-dia import { TaskGitHubIssueDialog } from "@/components/task/task-github-issue-dialog"; import { TaskGitHubPRDialog } from "@/components/task/task-github-pr-dialog"; import { TaskMRLinkDialog } from "@/components/gitlab/task-mr-link-dialog"; +import { ChangeWorkflowDialog } from "@/components/task/change-workflow-dialog"; import type { Task } from "@/components/kanban-card"; import type { useTaskActionsMenu, TaskActionsMenuBoardRow } from "@/hooks/use-task-actions-menu"; import { hydrateEditedTask } from "@/hooks/domains/kanban/use-kanban-actions"; @@ -170,6 +171,7 @@ export function TaskActionsMenuDialogs({ menu={menu} /> )} + ); } + +function ChangeWorkflowDialogSurface({ + taskId, + workspaceId, + menu, +}: Pick) { + return ( + + ); +} diff --git a/apps/web/components/task/task-management-drawer.test.tsx b/apps/web/components/task/task-management-drawer.test.tsx index b7fe8edf30..f07a21ff51 100644 --- a/apps/web/components/task/task-management-drawer.test.tsx +++ b/apps/web/components/task/task-management-drawer.test.tsx @@ -6,8 +6,9 @@ import { TaskManagementDrawer } from "./task-management-drawer"; afterEach(cleanup); describe("task drawer navigation", () => { // @covers AC-TASKS-THREADS-ACTIONS-004.3, AC-TASKS-THREADS-ACTIONS-004.6, AC-TASKS-THREADS-ACTIONS-001.3 - it("chooses a workflow step in one surface and backs through nested pages", () => { + it("opens the change form directly and keeps same-workflow step navigation separate", () => { const onMove = vi.fn(); + const onChangeWorkflow = vi.fn(); render( { two: [{ id: "s3", title: "Build" }], }} onMove={onMove} + onChangeWorkflow={onChangeWorkflow} onPriority={vi.fn()} onArchive={vi.fn()} onDelete={vi.fn()} @@ -33,15 +35,11 @@ describe("task drawer navigation", () => { /> , ); - fireEvent.click(screen.getByRole("button", { name: "Send to workflow" })); - fireEvent.click(screen.getByRole("button", { name: "Two" })); - expect(screen.getAllByRole("dialog")).toHaveLength(1); - fireEvent.click(screen.getByRole("button", { name: "Back" })); - expect(screen.getByRole("button", { name: "Two" })).toBeTruthy(); - expect(document.activeElement).toBe(screen.getByRole("button", { name: "Two" })); - fireEvent.click(screen.getByRole("button", { name: "Two" })); - fireEvent.click(screen.getByRole("button", { name: "Build" })); - expect(onMove).toHaveBeenCalledWith("two", "s3"); + fireEvent.click(screen.getByRole("button", { name: "Change workflow..." })); + expect(onChangeWorkflow).toHaveBeenCalledOnce(); + fireEvent.click(screen.getByTestId("task-management-page-steps")); + fireEvent.click(screen.getByRole("button", { name: "Review" })); + expect(onMove).toHaveBeenCalledWith("one", "s2"); }); it("keeps current lifecycle status outside the disabled step choice", () => { @@ -64,6 +62,7 @@ describe("task drawer navigation", () => { ], }} onMove={vi.fn()} + onChangeWorkflow={vi.fn()} onPriority={vi.fn()} onArchive={vi.fn()} onDelete={vi.fn()} diff --git a/apps/web/components/task/task-management-drawer.tsx b/apps/web/components/task/task-management-drawer.tsx index 997115190e..e619af0314 100644 --- a/apps/web/components/task/task-management-drawer.tsx +++ b/apps/web/components/task/task-management-drawer.tsx @@ -34,7 +34,7 @@ import { export type TaskManagementDrawerProps = TaskManagementMenuProps & { onCloseAutoFocus: (event: Event) => void; }; -type Page = "root" | "priority" | "steps" | "workflows" | "links" | { workflowId: string }; +type Page = "root" | "priority" | "steps" | "links"; export function TaskManagementSheet({ title, @@ -256,7 +256,7 @@ function DrawerChoices({ }) { const { t } = useTranslation(); const { task, workflows, stepsByWorkflowId, disabled, onMove, linkActions, closeMenu } = props; - const { currentSteps, targets } = taskMoveOptions(task.workflowId, workflows, stepsByWorkflowId); + const { currentSteps } = taskMoveOptions(task.workflowId, workflows, stepsByWorkflowId); const links = taskLinkMenuOptions(linkActions); const plugins = useTaskPluginLinkActions(task.id, task.repositoryLinks ?? []); if (page === "priority") return ; @@ -273,31 +273,6 @@ function DrawerChoices({ }} /> ); - if (typeof page === "object") - return ( - onMove(page.workflowId, stepId)} - /> - ); - if (page === "workflows") - return targets.map((workflow) => ( - setPage({ workflowId: workflow.id })} - > - {workflow.name} - {!stepsByWorkflowId[workflow.id]?.length && ( - {t("task:noSteps")} - )} - - )); if (page === "links") return ( <> @@ -345,7 +320,8 @@ function RootChoices({ hasLinks: boolean; }) { const { t } = useTranslation(); - const { task, workflows, stepsByWorkflowId, disabled, onArchive, onDelete } = props; + const { task, workflows, stepsByWorkflowId, disabled, onArchive, onDelete, onChangeWorkflow } = + props; const { canMove, canSend } = taskMoveOptions(task.workflowId, workflows, stepsByWorkflowId); return ( <> @@ -372,12 +348,11 @@ function RootChoices({ {canSend && ( } - onClick={() => setPage("workflows")} + onClick={onChangeWorkflow} > - {t("task:sendToWorkflow")} + {t("task:changeWorkflow")} )} {hasLinks && ( @@ -430,26 +405,17 @@ export function TaskManagementDrawer(props: TaskManagementDrawerProps) { setPage(next); }; const back = () => { - setFocusChoiceId( - typeof page === "object" - ? `task-context-workflow-${page.workflowId}` - : `task-management-page-${page}`, - ); - setPage(typeof page === "object" ? "workflows" : "root"); + setFocusChoiceId(`task-management-page-${page}`); + setPage("root"); }; - const title = - typeof page === "object" - ? (props.workflows.find((workflow) => workflow.id === page.workflowId)?.name ?? - t("task:sendToWorkflow")) - : t( - { - root: "task:taskActions", - priority: "kanban:priority", - steps: "task:moveTo", - workflows: "task:sendToWorkflow", - links: "task:link", - }[page], - ); + const title = t( + { + root: "task:taskActions", + priority: "kanban:priority", + steps: "task:moveTo", + links: "task:link", + }[page], + ); return ( { ], }} onMove={vi.fn()} + onChangeWorkflow={vi.fn()} onPriority={onPriority} onArchive={vi.fn()} onDelete={vi.fn()} @@ -44,7 +45,7 @@ describe("focused shared task actions", () => { , ); fireEvent.contextMenu(screen.getByText("Open")); - expect(screen.queryByText("Send to workflow")).toBeNull(); + expect(screen.queryByText("Change workflow...")).toBeNull(); fireEvent.pointerMove(screen.getByTestId("task-context-move-to"), { pointerType: "mouse" }); const current = await screen.findByTestId("task-context-step-s1"); expect(current.getAttribute("aria-disabled")).toBe("true"); diff --git a/apps/web/components/task/task-management-menu.tsx b/apps/web/components/task/task-management-menu.tsx index d2abf5447f..9b2354e6de 100644 --- a/apps/web/components/task/task-management-menu.tsx +++ b/apps/web/components/task/task-management-menu.tsx @@ -16,6 +16,7 @@ export type TaskManagementMenuProps = { disabled?: boolean; onPriority: (priority: TaskPriority) => void; onMove: (workflowId: string, stepId: string) => void; + onChangeWorkflow?: () => void; onArchive: () => void; onDelete: () => void; closeMenu: () => void; @@ -30,6 +31,7 @@ export function TaskManagementMenu({ disabled, onPriority, onMove, + onChangeWorkflow, onArchive, onDelete, closeMenu, @@ -52,7 +54,7 @@ export function TaskManagementMenu({ onMoveToStep={(stepId) => { if (task.workflowId) onMove(task.workflowId, stepId); }} - onSendToWorkflow={onMove} + onChangeWorkflow={onChangeWorkflow} />