From 2c4a0f425f89f4a7ca99a1d3163b87fd9c630bd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sun, 6 Sep 2026 15:19:25 +0100 Subject: [PATCH] fix(tasks): protect dirty worktree deletion (#3426) --- apps/backend/internal/backendapp/e2e_reset.go | 18 +- .../internal/backendapp/e2e_reset_test.go | 30 ++ apps/backend/internal/task/handlers/errors.go | 12 + .../internal/task/handlers/errors_test.go | 28 ++ .../task/handlers/task_http_handlers.go | 15 +- .../internal/task/service/handoff_cascade.go | 106 +++++-- .../task/service/handoff_cascade_test.go | 78 +++++ .../internal/task/service/handoff_service.go | 15 + .../task/service/resource_cleanup_jobs.go | 58 ++-- .../resource_cleanup_task_owned_test.go | 30 +- .../resource_cleanup_worktree_retry_test.go | 227 +++++++++++++-- apps/backend/internal/task/service/service.go | 22 ++ .../internal/task/service/service_tasks.go | 91 +++++- .../task/service/task_delete_errors.go | 49 ++++ .../internal/worktree/manager_cleanup.go | 41 ++- .../worktree/manager_cleanup_audit.go | 13 +- .../worktree/manager_cleanup_dirty.go | 117 ++++++++ .../worktree/manager_cleanup_dirty_test.go | 55 ++++ apps/web/app/tasks/columns.tsx | 11 +- apps/web/app/tasks/tasks-list-view.tsx | 29 +- apps/web/app/tasks/tasks-page-client.tsx | 95 +++--- apps/web/app/tasks/tasks-page-content.tsx | 5 +- apps/web/components/kanban-card.tsx | 12 +- .../kanban/graph2-task-pipeline.tsx | 14 +- .../kanban/swimlane-graph-content.test.tsx | 29 +- .../swimlane-graph2-orphan-display.test.ts | 29 +- ...e-kanban-content.render-stability.test.tsx | 39 +-- .../kanban/task-multi-select-toolbar.tsx | 10 +- .../messages/action-message-actions.test.tsx | 37 +++ .../chat/messages/action-message-actions.tsx | 152 ++++++++-- .../chat/messages/action-message.test.tsx | 4 + .../session-task-switcher-sheet-hooks.ts | 14 +- .../mobile/session-task-switcher-sheet.tsx | 4 +- .../components/task/task-cleanup-summary.ts | 4 + .../task/task-delete-confirm-dialog.test.tsx | 56 +++- .../task/task-delete-confirm-dialog.tsx | 271 +++++++++++++++--- .../task/task-session-sidebar-dialogs.tsx | 5 +- .../task/task-session-sidebar-selection.tsx | 7 +- .../components/task/task-session-sidebar.tsx | 12 +- .../web/components/task/task-top-bar.test.tsx | 7 +- apps/web/e2e/pages/session-page.ts | 9 +- .../kanban/card-menu-delete-archive.spec.ts | 1 + .../kanban/cascade-subtasks-toggle.spec.ts | 10 + .../kanban/dialog-enter-confirms.spec.ts | 2 + .../e2e/tests/kanban/pipeline-view.spec.ts | 1 + .../tests/kanban/task-multi-select.spec.ts | 2 + .../task/confirmation-text-hierarchy.spec.ts | 4 +- ...mobile-confirmation-text-hierarchy.spec.ts | 62 +++- .../tests/task/sidebar-delete-confirm.spec.ts | 6 +- .../tests/task/sidebar-multi-select.spec.ts | 1 + apps/web/hooks/use-sidebar-multi-select.ts | 8 +- apps/web/hooks/use-task-actions.test.ts | 29 +- apps/web/hooks/use-task-actions.ts | 36 ++- apps/web/hooks/use-task-crud.ts | 6 +- apps/web/hooks/use-task-multi-select.ts | 10 +- apps/web/lib/api/domains/kanban-api.test.ts | 20 ++ apps/web/lib/api/domains/kanban-api.ts | 14 +- apps/web/lib/api/task-delete-errors.ts | 7 + apps/web/src/locales/en/task.json | 4 + apps/web/src/locales/pseudo/task.json | 4 + apps/web/src/locales/pt-pt/task.json | 4 + apps/web/src/locales/zh-cn/task.json | 4 + apps/web/src/locales/zh-hk/task.json | 4 + apps/web/src/locales/zh-tw/task.json | 4 + .../plan.md | 186 ++++++++++++ .../task-01-enforce-delete-boundary.md | 122 ++++++++ .../task-02-surface-discard-consent.md | 143 +++++++++ docs/public/git-operations.md | 19 +- .../tasks/requirements/runtime-cleanup.md | 14 +- .../system-design/dirty-worktree-deletion.md | 169 +++++++++++ .../tasks/system-design/runtime-cleanup.md | 7 +- .../confirmation-warning-hierarchy.md | 20 +- .../confirmation-warning-hierarchy.md | 68 +++-- 73 files changed, 2486 insertions(+), 365 deletions(-) create mode 100644 apps/backend/internal/task/service/task_delete_errors.go create mode 100644 apps/backend/internal/worktree/manager_cleanup_dirty.go create mode 100644 apps/backend/internal/worktree/manager_cleanup_dirty_test.go create mode 100644 apps/web/components/task/chat/messages/action-message-actions.test.tsx create mode 100644 apps/web/lib/api/task-delete-errors.ts create mode 100644 docs/plans/task-delete-dirty-worktree-consent/plan.md create mode 100644 docs/plans/task-delete-dirty-worktree-consent/task-01-enforce-delete-boundary.md create mode 100644 docs/plans/task-delete-dirty-worktree-consent/task-02-surface-discard-consent.md create mode 100644 docs/specs/tasks/system-design/dirty-worktree-deletion.md diff --git a/apps/backend/internal/backendapp/e2e_reset.go b/apps/backend/internal/backendapp/e2e_reset.go index 1ae8f60598..1d0013cc7c 100644 --- a/apps/backend/internal/backendapp/e2e_reset.go +++ b/apps/backend/internal/backendapp/e2e_reset.go @@ -284,7 +284,7 @@ func handleE2EReset( deletedTaskIDSet[taskID] = struct{}{} } for _, t := range tasks { - if err := taskSvc.DeleteTask(ctx, t.ID); err != nil { + if err := deleteTaskForE2EReset(ctx, taskSvc, t.ID); err != nil { // Abort: leaving an undeleted task with its workflow gone // would create orphan rows visible to subsequent tests. log.Error("e2e reset: failed to delete task", @@ -329,6 +329,22 @@ func handleE2EReset( } } +type e2eResetTaskDeleter interface { + DeleteTaskWithOptions(context.Context, string, taskservice.DeleteTaskOptions) error +} + +func deleteTaskForE2EReset( + ctx context.Context, + taskDeleter e2eResetTaskDeleter, + taskID string, +) error { + return taskDeleter.DeleteTaskWithOptions(ctx, taskID, taskservice.DeleteTaskOptions{ + // E2E reset is an explicit test cleanup boundary. It must remove + // disposable local changes left by the test that created the task. + DiscardWorktreeChanges: true, + }) +} + func waitForE2ETaskCleanup(ctx context.Context, database *sql.DB, taskIDs []string) error { if database == nil || len(taskIDs) == 0 { return nil diff --git a/apps/backend/internal/backendapp/e2e_reset_test.go b/apps/backend/internal/backendapp/e2e_reset_test.go index 0662703d52..6f1647f449 100644 --- a/apps/backend/internal/backendapp/e2e_reset_test.go +++ b/apps/backend/internal/backendapp/e2e_reset_test.go @@ -14,8 +14,38 @@ import ( "github.com/kandev/kandev/internal/db" taskmodels "github.com/kandev/kandev/internal/task/models" + taskservice "github.com/kandev/kandev/internal/task/service" ) +type e2eResetTaskDeleterStub struct { + taskID string + options taskservice.DeleteTaskOptions +} + +func (s *e2eResetTaskDeleterStub) DeleteTaskWithOptions( + _ context.Context, + taskID string, + options taskservice.DeleteTaskOptions, +) error { + s.taskID = taskID + s.options = options + return nil +} + +func TestDeleteTaskForE2EResetDiscardsWorktreeChanges(t *testing.T) { + deleter := &e2eResetTaskDeleterStub{} + + if err := deleteTaskForE2EReset(context.Background(), deleter, "task-1"); err != nil { + t.Fatalf("deleteTaskForE2EReset: %v", err) + } + if deleter.taskID != "task-1" { + t.Fatalf("deleted task ID = %q, want task-1", deleter.taskID) + } + if !deleter.options.DiscardWorktreeChanges { + t.Fatal("E2E reset must discard disposable worktree changes") + } +} + func TestE2EResetDeletesWorkspaceGitHubAuthentication(t *testing.T) { raw, err := db.OpenSQLite(filepath.Join(t.TempDir(), "e2e-reset.db")) if err != nil { diff --git a/apps/backend/internal/task/handlers/errors.go b/apps/backend/internal/task/handlers/errors.go index 0f5425c427..b730fe62cd 100644 --- a/apps/backend/internal/task/handlers/errors.go +++ b/apps/backend/internal/task/handlers/errors.go @@ -55,6 +55,11 @@ func handleNotFound(c *gin.Context, log *logger.Logger, err error, fallback stri c.JSON(http.StatusConflict, gin.H{"error": err.Error()}) return } + var dirtyWorktreeErr *service.TaskDeleteDirtyWorktreeError + if errors.As(err, &dirtyWorktreeErr) { + c.JSON(http.StatusConflict, taskErrorBody(err)) + return + } if isValidationError(err) { c.JSON(http.StatusBadRequest, taskErrorBody(err)) return @@ -79,6 +84,13 @@ func taskErrorDetails(err error) map[string]interface{} { if errors.Is(err, service.ErrRepositoryBranchPolicyStale) { return map[string]interface{}{"error_code": service.BranchPolicyStaleErrorCode} } + var dirtyWorktreeErr *service.TaskDeleteDirtyWorktreeError + if errors.As(err, &dirtyWorktreeErr) { + return map[string]interface{}{ + "error_code": service.TaskDeleteDirtyWorktreeErrorCode, + "dirty_worktrees": dirtyWorktreeErr.DirtyWorktrees, + } + } return nil } diff --git a/apps/backend/internal/task/handlers/errors_test.go b/apps/backend/internal/task/handlers/errors_test.go index 2ba851aae9..6e9fa54594 100644 --- a/apps/backend/internal/task/handlers/errors_test.go +++ b/apps/backend/internal/task/handlers/errors_test.go @@ -16,6 +16,7 @@ import ( taskrepo "github.com/kandev/kandev/internal/task/repository/sqlite" "github.com/kandev/kandev/internal/task/service" wfmodels "github.com/kandev/kandev/internal/workflow/models" + "github.com/kandev/kandev/internal/worktree" ws "github.com/kandev/kandev/pkg/websocket" ) @@ -97,6 +98,33 @@ func TestErrorsAreClassifiable(t *testing.T) { } }) + t.Run("dirty worktree delete returns a typed conflict", func(t *testing.T) { + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(rec) + err := &service.TaskDeleteDirtyWorktreeError{DirtyWorktrees: []worktree.DirtyWorktree{{ + WorktreeID: "wt-1", RepositoryID: "repo-1", Path: "/tasks/task-1/repo", + DirtyFiles: []string{"notes.txt"}, + }}} + handleNotFound(ctx, newTestLogger(t), fmt.Errorf("delete task: %w", err), "task not deleted") + if rec.Code != http.StatusConflict { + t.Fatalf("status=%d, want %d", rec.Code, http.StatusConflict) + } + var payload struct { + ErrorCode string `json:"error_code"` + DirtyWorktrees []worktree.DirtyWorktree `json:"dirty_worktrees"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatalf("decode response: %v", err) + } + if payload.ErrorCode != service.TaskDeleteDirtyWorktreeErrorCode { + t.Fatalf("error_code=%q, want %q", payload.ErrorCode, service.TaskDeleteDirtyWorktreeErrorCode) + } + if len(payload.DirtyWorktrees) != 1 || payload.DirtyWorktrees[0].DirtyFiles[0] != "notes.txt" { + t.Fatalf("dirty_worktrees=%+v, want the inspected file", payload.DirtyWorktrees) + } + }) + t.Run("stale branch policies include a stable error code", func(t *testing.T) { gin.SetMode(gin.TestMode) rec := httptest.NewRecorder() diff --git a/apps/backend/internal/task/handlers/task_http_handlers.go b/apps/backend/internal/task/handlers/task_http_handlers.go index 4528d9a75d..17b049cd64 100644 --- a/apps/backend/internal/task/handlers/task_http_handlers.go +++ b/apps/backend/internal/task/handlers/task_http_handlers.go @@ -1762,18 +1762,25 @@ func (h *TaskHandlers) httpDeleteTask(c *gin.Context) { defer cancel() taskID := c.Param("id") cascade := cascadeQueryParam(c) + discardWorktreeChanges := discardWorktreeChangesQueryParam(c) // Office task-handoffs phase 6: route through HandoffService.DeleteTaskTree // when wired so descendant runs are cancelled, group memberships are // released with reason=deleted, and the cleanup state machine fires. if h.handoffSvc != nil { - if _, err := h.handoffSvc.DeleteTaskTree(deleteCtx, taskID, cascade); err != nil { + if _, err := h.handoffSvc.DeleteTaskTreeWithOptions( + deleteCtx, taskID, cascade, service.DeleteTaskOptions{ + DiscardWorktreeChanges: discardWorktreeChanges, + }, + ); err != nil { handleNotFound(c, h.logger, err, "task not deleted") return } c.JSON(http.StatusOK, dto.SuccessResponse{Success: true}) return } - if err := h.service.DeleteTask(deleteCtx, taskID); err != nil { + if err := h.service.DeleteTaskWithOptions(deleteCtx, taskID, service.DeleteTaskOptions{ + DiscardWorktreeChanges: discardWorktreeChanges, + }); err != nil { handleNotFound(c, h.logger, err, "task not deleted") return } @@ -1810,6 +1817,10 @@ func cascadeQueryParam(c *gin.Context) bool { return strings.EqualFold(c.Query("cascade"), "true") } +func discardWorktreeChangesQueryParam(c *gin.Context) bool { + return strings.EqualFold(c.Query("discard_worktree_changes"), "true") +} + // httpTaskSubtaskCount returns the count of direct, non-archived, // non-ephemeral subtasks for a task. Used by the frontend's archive / // delete confirmation dialogs to decide whether to render the diff --git a/apps/backend/internal/task/service/handoff_cascade.go b/apps/backend/internal/task/service/handoff_cascade.go index 8a0f2928ea..242b9482e8 100644 --- a/apps/backend/internal/task/service/handoff_cascade.go +++ b/apps/backend/internal/task/service/handoff_cascade.go @@ -219,7 +219,9 @@ func (s *HandoffService) ArchiveTaskTree(ctx context.Context, rootID string, cas if err != nil { return out, err } - cleanupOps, err := s.prepareCascadeResourceCleanup(ctx, all, cascadeID, models.TaskResourceCleanupTriggerCascadeArchive) + cleanupOps, err := s.prepareCascadeResourceCleanup( + ctx, all, cascadeID, models.TaskResourceCleanupTriggerCascadeArchive, false, + ) if err != nil { return out, s.rollbackWorkspaceEnvironmentOwnershipAfterFailure(ctx, ownershipTransfers, err) } @@ -313,6 +315,15 @@ func (s *HandoffService) ArchiveTaskTree(ctx context.Context, rootID string, cas // because deletion is unconditional; the cascade ID is stamped only // for symmetry with archive. func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, cascade bool) (*CascadeOutcome, error) { + return s.DeleteTaskTreeWithOptions(ctx, rootID, cascade, DeleteTaskOptions{}) +} + +// DeleteTaskTreeWithOptions deletes a task tree after all owned worktrees have +// passed the dirty-worktree admission check. Consent is persisted in each +// cleanup snapshot before any task row is mutated. +func (s *HandoffService) DeleteTaskTreeWithOptions( + ctx context.Context, rootID string, cascade bool, options DeleteTaskOptions, +) (*CascadeOutcome, error) { if err := s.authorizeTask(ctx, rootID); err != nil { return nil, err } @@ -325,18 +336,12 @@ func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, casc cascadeID := uuid.New().String() out := &CascadeOutcome{CascadeID: cascadeID} - all, err := s.resolveDeleteSet(ctx, rootID, cascade) - if err != nil { - return nil, err - } - ownershipTransfers, err := s.transferSharedWorkspaceEnvironmentOwnership(ctx, all) + all, ownershipTransfers, cleanupOps, err := s.prepareDeleteTaskTree( + ctx, rootID, cascade, cascadeID, options, + ) if err != nil { return out, err } - cleanupOps, err := s.prepareCascadeResourceCleanup(ctx, all, cascadeID, models.TaskResourceCleanupTriggerCascadeDelete) - if err != nil { - return out, s.rollbackWorkspaceEnvironmentOwnershipAfterFailure(ctx, ownershipTransfers, err) - } s.cancelActiveRuns(ctx, all, "task tree deleted") @@ -354,6 +359,61 @@ func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, casc // Delete deepest first; failures abort the cascade and surface so // the caller can retry. We do NOT roll back partial deletions — // delete is destructive by design and re-running is idempotent. + postDeleteCtx, err := s.deleteTaskTreeRows( + ctx, all, cleanupOps, ownershipTransfers, out, + ) + if err != nil { + return out, err + } + + for _, gid := range groupIDs { + if err := s.evaluateWorkspaceGroupCleanup(postDeleteCtx, gid); err != nil { + s.logf().Error("evaluate workspace group cleanup", + zap.String("group_id", gid), zap.Error(err)) + } + } + return out, nil +} + +func (s *HandoffService) prepareDeleteTaskTree( + ctx context.Context, + rootID string, + cascade bool, + cascadeID string, + options DeleteTaskOptions, +) ([]string, []workspaceEnvironmentOwnershipTransfer, map[string]string, error) { + all, err := s.resolveDeleteSet(ctx, rootID, cascade) + if err != nil { + return nil, nil, nil, err + } + if checker, ok := s.resourceCleaner.(taskDeleteWorktreeAdmissionChecker); ok { + if err := checker.ValidateTaskDeleteWorktrees(ctx, all, options.DiscardWorktreeChanges); err != nil { + return nil, nil, nil, err + } + } + ownershipTransfers, err := s.transferSharedWorkspaceEnvironmentOwnership(ctx, all) + if err != nil { + return nil, nil, nil, err + } + cleanupOps, err := s.prepareCascadeResourceCleanup( + ctx, all, cascadeID, models.TaskResourceCleanupTriggerCascadeDelete, + options.DiscardWorktreeChanges, + ) + if err != nil { + return nil, nil, nil, s.rollbackWorkspaceEnvironmentOwnershipAfterFailure( + ctx, ownershipTransfers, err, + ) + } + return all, ownershipTransfers, cleanupOps, nil +} + +func (s *HandoffService) deleteTaskTreeRows( + ctx context.Context, + all []string, + cleanupOps map[string]string, + ownershipTransfers []workspaceEnvironmentOwnershipTransfer, + out *CascadeOutcome, +) (context.Context, error) { postDeleteCtx := ctx vacatedStepIDs := make(map[string]struct{}) defer func() { @@ -377,9 +437,11 @@ func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, casc s.cancelCascadeResourceCleanupRange(postDeleteCtx, all[:i+1], cleanupOps) deleteErr := fmt.Errorf("delete %s: %w", all[i], err) if len(out.ArchivedTaskIDs) == 0 { - deleteErr = s.rollbackWorkspaceEnvironmentOwnershipAfterFailure(ctx, ownershipTransfers, deleteErr) + deleteErr = s.rollbackWorkspaceEnvironmentOwnershipAfterFailure( + ctx, ownershipTransfers, deleteErr, + ) } - return out, deleteErr + return postDeleteCtx, deleteErr } postDeleteCtx = context.WithoutCancel(ctx) // The delete mutation committed, so it is now safe to finalize @@ -396,14 +458,7 @@ func (s *HandoffService) DeleteTaskTree(ctx context.Context, rootID string, casc s.eventPublisher.PublishTaskDeleted(postDeleteCtx, snapshot) } } - - for _, gid := range groupIDs { - if err := s.evaluateWorkspaceGroupCleanup(postDeleteCtx, gid); err != nil { - s.logf().Error("evaluate workspace group cleanup", - zap.String("group_id", gid), zap.Error(err)) - } - } - return out, nil + return postDeleteCtx, nil } func (s *HandoffService) archiveTaskWithVacatedStep( @@ -656,6 +711,7 @@ func (s *HandoffService) prepareCascadeResourceCleanup( taskIDs []string, cascadeID string, trigger models.TaskResourceCleanupTrigger, + discardWorktreeChanges bool, ) (map[string]string, error) { coordinator, ok := s.resourceCleaner.(taskResourceCleanupCoordinator) if !ok { @@ -665,7 +721,15 @@ func (s *HandoffService) prepareCascadeResourceCleanup( for _, taskID := range taskIDs { operationID := string(trigger) + ":" + cascadeID + ":" + taskID deleteEnvironmentRow := trigger == models.TaskResourceCleanupTriggerCascadeDelete - if err := coordinator.PrepareTaskResourceCleanup(ctx, taskID, trigger, operationID, deleteEnvironmentRow); err != nil { + var err error + if withOptions, supportsOptions := s.resourceCleaner.(taskResourceCleanupCoordinatorWithOptions); supportsOptions { + err = withOptions.PrepareTaskResourceCleanupWithOptions( + ctx, taskID, trigger, operationID, deleteEnvironmentRow, discardWorktreeChanges, + ) + } else { + err = coordinator.PrepareTaskResourceCleanup(ctx, taskID, trigger, operationID, deleteEnvironmentRow) + } + if err != nil { s.cancelCascadeResourceCleanupRange(ctx, taskIDs, operations) return nil, fmt.Errorf("prepare cleanup %s: %w", taskID, err) } diff --git a/apps/backend/internal/task/service/handoff_cascade_test.go b/apps/backend/internal/task/service/handoff_cascade_test.go index 5865536d0f..af9cc9138b 100644 --- a/apps/backend/internal/task/service/handoff_cascade_test.go +++ b/apps/backend/internal/task/service/handoff_cascade_test.go @@ -135,6 +135,33 @@ type recordingCleanupCoordinator struct { cleaned []string } +type deleteAdmissionCall struct { + taskIDs []string + discard bool +} + +type recordingDeleteAdmissionCleaner struct { + mu sync.Mutex + calls []deleteAdmissionCall + dirtyErr error +} + +func (c *recordingDeleteAdmissionCleaner) CleanupTaskResources(context.Context, string, bool) {} + +func (c *recordingDeleteAdmissionCleaner) ValidateTaskDeleteWorktrees( + _ context.Context, taskIDs []string, discardWorktreeChanges bool, +) error { + c.mu.Lock() + c.calls = append(c.calls, deleteAdmissionCall{ + taskIDs: append([]string(nil), taskIDs...), discard: discardWorktreeChanges, + }) + c.mu.Unlock() + if !discardWorktreeChanges { + return c.dirtyErr + } + return nil +} + func (c *recordingCleanupCoordinator) CleanupTaskResources(_ context.Context, taskID string, _ bool) { c.mu.Lock() defer c.mu.Unlock() @@ -172,6 +199,57 @@ func TestDeleteTaskTreePreparedCleanupDeletesEnvironmentRow(t *testing.T) { } } +func TestDeleteTaskTreeWithOptionsPreflightsWholeTreeAndPropagatesDiscardConsent(t *testing.T) { + tasks := newFakeTaskRepo() + tasks.addTask("root", "", "ws-1") + tasks.addTask("child", "root", "ws-1") + tasks.addTask("grandchild", "child", "ws-1") + cleaner := &recordingDeleteAdmissionCleaner{dirtyErr: errors.New("dirty worktree")} + svc := NewHandoffService( + &fakeDeleteRepo{fakeCascadeRepo: newCascadeRepo(tasks)}, nil, nil, nil, nil, nil, + ) + svc.SetTaskResourceCleaner(cleaner) + + if _, err := svc.DeleteTaskTreeWithOptions(context.Background(), "root", true, DeleteTaskOptions{}); !errors.Is(err, cleaner.dirtyErr) { + t.Fatalf("unconsented cascade error = %v, want dirty admission error", err) + } + for _, taskID := range []string{"root", "child", "grandchild"} { + if task, err := tasks.GetTask(context.Background(), taskID); err != nil || task == nil { + t.Fatalf("unconsented cascade mutated %s: task=%#v err=%v", taskID, task, err) + } + } + + cleaner.mu.Lock() + if len(cleaner.calls) != 1 { + t.Fatalf("admission calls after refusal = %d, want 1", len(cleaner.calls)) + } + if cleaner.calls[0].discard { + t.Fatal("unconsented cascade passed discard consent") + } + if len(cleaner.calls[0].taskIDs) != 3 { + t.Fatalf("admission task IDs = %v, want complete tree", cleaner.calls[0].taskIDs) + } + cleaner.mu.Unlock() + + if _, err := svc.DeleteTaskTreeWithOptions(context.Background(), "root", true, DeleteTaskOptions{ + DiscardWorktreeChanges: true, + }); err != nil { + t.Fatalf("consented cascade: %v", err) + } + for _, taskID := range []string{"root", "child", "grandchild"} { + if task, err := tasks.GetTask(context.Background(), taskID); err != nil { + t.Fatalf("get deleted task %s: %v", taskID, err) + } else if task != nil { + t.Fatalf("consented cascade retained %s: %#v", taskID, task) + } + } + cleaner.mu.Lock() + defer cleaner.mu.Unlock() + if len(cleaner.calls) != 2 || !cleaner.calls[1].discard { + t.Fatalf("admission calls after consent = %+v, want second call with discard=true", cleaner.calls) + } +} + func TestDeleteTaskTree_NoCascadeNormalizesInheritedChildren(t *testing.T) { tasks := newFakeTaskRepo() tasks.addTask("root", "", "ws-1") diff --git a/apps/backend/internal/task/service/handoff_service.go b/apps/backend/internal/task/service/handoff_service.go index b3dfa29a27..c320777893 100644 --- a/apps/backend/internal/task/service/handoff_service.go +++ b/apps/backend/internal/task/service/handoff_service.go @@ -353,6 +353,21 @@ type taskResourceCleanupCoordinator interface { CancelPreparedTaskResourceCleanup(ctx context.Context, operationID string) error } +type taskResourceCleanupCoordinatorWithOptions interface { + PrepareTaskResourceCleanupWithOptions( + ctx context.Context, + taskID string, + trigger models.TaskResourceCleanupTrigger, + operationID string, + deleteEnvironmentRow bool, + discardWorktreeChanges bool, + ) error +} + +type taskDeleteWorktreeAdmissionChecker interface { + ValidateTaskDeleteWorktrees(ctx context.Context, taskIDs []string, discardWorktreeChanges bool) error +} + // SetTaskResourceCleaner wires the resource teardown surface invoked by // cascade archive/delete to release containers / sandboxes / worktrees. // Optional — when nil the cascade does not tear down runtime resources. diff --git a/apps/backend/internal/task/service/resource_cleanup_jobs.go b/apps/backend/internal/task/service/resource_cleanup_jobs.go index b5da4015e1..3713e30f09 100644 --- a/apps/backend/internal/task/service/resource_cleanup_jobs.go +++ b/apps/backend/internal/task/service/resource_cleanup_jobs.go @@ -40,13 +40,14 @@ type persistedTaskStopTarget struct { } type taskResourceCleanupSnapshot struct { - Sessions []*models.TaskSession `json:"sessions,omitempty"` - Worktrees []*worktree.Worktree `json:"worktrees,omitempty"` - WorktreeHeadOIDs map[string]string `json:"worktree_head_oids,omitempty"` - StopTargets []persistedTaskStopTarget `json:"stop_targets,omitempty"` - TaskEnvironment *models.TaskEnvironment `json:"task_environment,omitempty"` - DeleteEnvironmentRow bool `json:"delete_environment_row,omitempty"` - LegacyWorktreeCleanup bool `json:"legacy_worktree_cleanup,omitempty"` + Sessions []*models.TaskSession `json:"sessions,omitempty"` + Worktrees []*worktree.Worktree `json:"worktrees,omitempty"` + WorktreeHeadOIDs map[string]string `json:"worktree_head_oids,omitempty"` + DiscardWorktreeChanges bool `json:"discard_worktree_changes,omitempty"` + StopTargets []persistedTaskStopTarget `json:"stop_targets,omitempty"` + TaskEnvironment *models.TaskEnvironment `json:"task_environment,omitempty"` + DeleteEnvironmentRow bool `json:"delete_environment_row,omitempty"` + LegacyWorktreeCleanup bool `json:"legacy_worktree_cleanup,omitempty"` // SSHTaskDirs records the remote task directories this task launched into. // Additive and absent-tolerant: a job row written by an older backend // decodes with an empty list and reclaims nothing. @@ -86,10 +87,11 @@ func (s *Service) persistTaskResourceCleanup( } snapshot := taskResourceCleanupSnapshot{ Sessions: sessions, Worktrees: worktrees, WorktreeHeadOIDs: worktreeHeadOIDs, - StopTargets: persistStopTargets(stopTargets), - TaskEnvironment: envCleanup.env, - DeleteEnvironmentRow: envCleanup.deleteRow, - LegacyWorktreeCleanup: s.hasLegacyWorktreeCleanup(), + StopTargets: persistStopTargets(stopTargets), + TaskEnvironment: envCleanup.env, + DeleteEnvironmentRow: envCleanup.deleteRow, + DiscardWorktreeChanges: envCleanup.discardWorktreeChanges, + LegacyWorktreeCleanup: s.hasLegacyWorktreeCleanup(), } if !prepared { // A prepared job stores a deliberately empty placeholder snapshot that @@ -476,7 +478,8 @@ func (s *Service) executeTaskResourceCleanupJob( errs := s.performTaskCleanup(ctx, job.TaskID, snapshot.Sessions, snapshot.Worktrees, targets, taskEnvironmentCleanup{ env: snapshot.TaskEnvironment, deleteRow: snapshot.DeleteEnvironmentRow, - preserveBranches: job.IsArchive(), + preserveBranches: job.IsArchive(), + discardWorktreeChanges: snapshot.DiscardWorktreeChanges, }, taskCleanupPreserveRows(stopOutcome)) if cause := context.Cause(ctx); cause != nil { @@ -597,7 +600,7 @@ func (s *Service) resolveTaskResourceCleanupAfterMutationError(ctx context.Conte func (s *Service) retryTaskResourceCleanupJob(ctx context.Context, job *models.TaskResourceCleanupJob, cleanupErr error) error { state := models.TaskResourceCleanupStateRetryWait var nextAttempt *time.Time - if job.Attempts >= taskResourceCleanupMaxAttempts { + if isDirtyWorktreeCleanupError(cleanupErr) || job.Attempts >= taskResourceCleanupMaxAttempts { state = models.TaskResourceCleanupStateFailed } else { next := time.Now().UTC().Add(taskResourceCleanupRetryDelayForAttempt(job.Attempts)) @@ -648,13 +651,29 @@ func (s *Service) PrepareTaskResourceCleanup( trigger models.TaskResourceCleanupTrigger, operationID string, deleteEnvironmentRow bool, +) error { + return s.PrepareTaskResourceCleanupWithOptions( + ctx, taskID, trigger, operationID, deleteEnvironmentRow, false, + ) +} + +// PrepareTaskResourceCleanupWithOptions is the consent-aware cascade +// preparation path. The option is written into the durable snapshot before +// task rows are mutated so a later worker uses the same user decision. +func (s *Service) PrepareTaskResourceCleanupWithOptions( + ctx context.Context, + taskID string, + trigger models.TaskResourceCleanupTrigger, + operationID string, + deleteEnvironmentRow bool, + discardWorktreeChanges bool, ) error { // Reserve the durable lifecycle barrier BEFORE capturing the inventory. // Session and worktree creation serialize against the owning task row and // reject new ownership while this prepared barrier is active, so the // snapshot below cannot miss a resource admitted mid-preparation. job, err := s.persistTaskResourceCleanup(ctx, taskID, trigger, operationID, - nil, nil, nil, taskEnvironmentCleanup{}, true) + nil, nil, nil, taskEnvironmentCleanup{discardWorktreeChanges: discardWorktreeChanges}, true) if err != nil { return err } @@ -688,11 +707,12 @@ func (s *Service) PrepareTaskResourceCleanup( } snapshot := taskResourceCleanupSnapshot{ Sessions: sessions, Worktrees: worktrees, WorktreeHeadOIDs: worktreeHeadOIDs, - StopTargets: persistStopTargets(stopTargets), - TaskEnvironment: taskEnv, - DeleteEnvironmentRow: deleteEnvironmentRow, - LegacyWorktreeCleanup: s.hasLegacyWorktreeCleanup(), - SSHTaskDirs: sshTaskDirs, + StopTargets: persistStopTargets(stopTargets), + TaskEnvironment: taskEnv, + DeleteEnvironmentRow: deleteEnvironmentRow, + DiscardWorktreeChanges: discardWorktreeChanges, + LegacyWorktreeCleanup: s.hasLegacyWorktreeCleanup(), + SSHTaskDirs: sshTaskDirs, } encoded, err := json.Marshal(snapshot) if err != nil { diff --git a/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go b/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go index 7d47f2bf39..761cbcd568 100644 --- a/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go +++ b/apps/backend/internal/task/service/resource_cleanup_task_owned_test.go @@ -51,14 +51,17 @@ func TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart(t *test ctx := context.Background() _, _, repo := createTestService(t) seedCleanupTaskAndSession(t, repo, "task-zero-session", "session-zero-session") + repoPath := initSimpleGitRepo(t) + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-zero-session", WorkspaceID: "ws-task-zero-session", + Name: "repo-zero-session", SourceType: "local", LocalPath: repoPath, + }); err != nil { + t.Fatalf("CreateRepository: %v", err) + } + initialMgr := newCleanupTestWorktreeManager(t, repo) if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ ID: "env-zero-session", TaskID: "task-zero-session", ExecutorType: "worktree", WorkspacePath: "/tmp/zero", Status: models.TaskEnvironmentStatusReady, - Repos: []*models.TaskEnvironmentRepo{{ - ID: "env-repo-zero-session", RepositoryID: "repo-zero-session", - WorktreeID: "wt-zero-session", WorktreePath: "/tmp/zero/repo", - WorktreeBranch: "feature/zero", Status: "active", - }}, }); err != nil { t.Fatalf("CreateTaskEnvironment: %v", err) } @@ -70,6 +73,15 @@ func TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart(t *test if err := repo.UpdateTaskSession(ctx, session); err != nil { t.Fatalf("link session: %v", err) } + created, err := initialMgr.Create(ctx, worktree.CreateRequest{ + TaskID: "task-zero-session", SessionID: "session-zero-session", + TaskTitle: "Zero session cleanup", RepositoryID: "repo-zero-session", RepositoryPath: repoPath, + BaseBranch: "main", TaskDirName: "task-zero-session", RepoName: "repo-zero-session", + }) + if err != nil { + t.Fatalf("create worktree: %v", err) + } + worktreeID := created.ID // Delete the only session through the orchestrator-style deletion. if err := repo.DeleteTaskSession(ctx, "session-zero-session"); err != nil { @@ -85,8 +97,8 @@ func TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart(t *test if err != nil { t.Fatalf("gather worktrees: %v", err) } - if len(worktrees) != 1 || worktrees[0].ID != "wt-zero-session" { - t.Fatalf("zero-session inventory = %+v, want wt-zero-session", worktrees) + if len(worktrees) != 1 || worktrees[0].ID != worktreeID { + t.Fatalf("zero-session inventory = %+v, want %s", worktrees, worktreeID) } if err := svc2.DeleteTask(ctx, "task-zero-session"); err != nil { @@ -103,8 +115,8 @@ func TestDeleteTaskCleanupFindsWorktreeAfterLastSessionDeletedAndRestart(t *test if err := json.Unmarshal([]byte(encoded), &snapshot); err != nil { t.Fatalf("decode snapshot: %v", err) } - if len(snapshot.Worktrees) != 1 || snapshot.Worktrees[0].ID != "wt-zero-session" { - t.Fatalf("snapshot worktrees = %+v, want wt-zero-session after task-row deletion", snapshot.Worktrees) + if len(snapshot.Worktrees) != 1 || snapshot.Worktrees[0].ID != worktreeID { + t.Fatalf("snapshot worktrees = %+v, want %s after task-row deletion", snapshot.Worktrees, worktreeID) } } diff --git a/apps/backend/internal/task/service/resource_cleanup_worktree_retry_test.go b/apps/backend/internal/task/service/resource_cleanup_worktree_retry_test.go index 957bb31851..c049999353 100644 --- a/apps/backend/internal/task/service/resource_cleanup_worktree_retry_test.go +++ b/apps/backend/internal/task/service/resource_cleanup_worktree_retry_test.go @@ -2,16 +2,20 @@ package service import ( "context" + "encoding/json" + "errors" + "fmt" "os" "path/filepath" - "strings" "testing" "github.com/kandev/kandev/internal/task/models" "github.com/kandev/kandev/internal/worktree" ) -func TestDeleteTaskCleanupRetriesWhenAuditedWorktreeRemovalIsUnsafe(t *testing.T) { +// @covers AC-TASKS-RUNTIME-CLEANUP-001.10 +// @covers AC-TASKS-RUNTIME-CLEANUP-001.11 +func TestDeleteTaskRejectsDirtyWorktreeBeforeMutation(t *testing.T) { ctx := context.Background() _, _, repo := createTestService(t) const taskID = "task-audited-cleanup-retry" @@ -60,8 +64,101 @@ func TestDeleteTaskCleanupRetriesWhenAuditedWorktreeRemovalIsUnsafe(t *testing.T svc := newServiceOverRepository(t, repo) svc.SetWorktreeCleanup(mgr) - if err := svc.DeleteTask(ctx, taskID); err != nil { - t.Fatalf("DeleteTask: %v", err) + if err := svc.DeleteTask(ctx, taskID); err == nil { + t.Fatal("DeleteTask succeeded without discard consent") + } + task, err := repo.GetTask(ctx, taskID) + if err != nil { + t.Fatalf("dirty delete removed task: %v", err) + } + if task == nil { + t.Fatal("dirty delete removed task") + } + var cleanupJobs int + if err := repo.DB().QueryRowContext(ctx, ` + SELECT COUNT(*) FROM task_resource_cleanup_jobs + WHERE task_id = ? AND trigger = 'delete' + `, taskID).Scan(&cleanupJobs); err != nil { + t.Fatalf("count delete cleanup jobs: %v", err) + } + if cleanupJobs != 0 { + t.Fatalf("dirty delete created %d cleanup jobs, want none", cleanupJobs) + } + if got, err := os.ReadFile(sentinel); err != nil || string(got) != "preserve me\n" { + t.Fatalf("preserved work changed: contents=%q err=%v", got, err) + } +} + +// @covers AC-TASKS-RUNTIME-CLEANUP-001.12 +func TestDeleteTaskWithDiscardConsentPersistsAndCleansDirtyWorktree(t *testing.T) { + ctx := context.Background() + _, _, repo := createTestService(t) + const taskID = "task-audited-cleanup-discard" + const sessionID = "session-audited-cleanup-discard" + seedCleanupTaskAndSession(t, repo, taskID, sessionID) + + repoPath := initSimpleGitRepo(t) + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-audited-cleanup-discard", WorkspaceID: "ws-" + taskID, + Name: "repo-audited-cleanup-discard", SourceType: "local", LocalPath: repoPath, + }); err != nil { + t.Fatalf("create repository: %v", err) + } + mgr := newCleanupTestWorktreeManager(t, repo) + wt, err := mgr.Create(ctx, worktree.CreateRequest{ + TaskID: taskID, SessionID: sessionID, TaskTitle: "Audited cleanup discard", + RepositoryID: "repo-audited-cleanup-discard", RepositoryPath: repoPath, + BaseBranch: "main", TaskDirName: taskID, RepoName: "repo-audited-cleanup-discard", + }) + if err != nil { + t.Fatalf("create worktree: %v", err) + } + if err := repo.CreateTaskEnvironment(ctx, &models.TaskEnvironment{ + ID: "env-" + taskID, TaskID: taskID, ExecutorType: "worktree", + WorkspacePath: filepath.Dir(wt.Path), Status: models.TaskEnvironmentStatusReady, + Repos: []*models.TaskEnvironmentRepo{{ + ID: "env-repo-audited-cleanup-discard", RepositoryID: "repo-audited-cleanup-discard", + WorktreeID: wt.ID, WorktreePath: wt.Path, WorktreeBranch: wt.Branch, Status: "active", + }}, + }); err != nil { + t.Fatalf("create task environment: %v", err) + } + session, err := repo.GetTaskSession(ctx, sessionID) + if err != nil { + t.Fatalf("get task session: %v", err) + } + session.TaskEnvironmentID = "env-" + taskID + session.BaseBranch = "main" + if err := repo.UpdateTaskSession(ctx, session); err != nil { + t.Fatalf("link task session: %v", err) + } + sentinel := filepath.Join(wt.Path, "untracked-discarded.txt") + if err := os.WriteFile(sentinel, []byte("discard me\n"), 0o644); err != nil { + t.Fatalf("write untracked work: %v", err) + } + + svc := newServiceOverRepository(t, repo) + svc.SetWorktreeCleanup(mgr) + svc.SetEnvironmentDestroyer(&managerEnvironmentDestroyer{mgr: mgr}) + if err := svc.DeleteTaskWithOptions(ctx, taskID, DeleteTaskOptions{ + DiscardWorktreeChanges: true, + }); err != nil { + t.Fatalf("delete task with discard consent: %v", err) + } + var encodedSnapshot string + if err := repo.DB().QueryRowContext(ctx, ` + SELECT resource_snapshot FROM task_resource_cleanup_jobs + WHERE task_id = ? AND trigger = 'delete' + ORDER BY created_at DESC LIMIT 1 + `, taskID).Scan(&encodedSnapshot); err != nil { + t.Fatalf("load delete cleanup snapshot: %v", err) + } + var snapshot taskResourceCleanupSnapshot + if err := json.Unmarshal([]byte(encodedSnapshot), &snapshot); err != nil { + t.Fatalf("decode delete cleanup snapshot: %v", err) + } + if !snapshot.DiscardWorktreeChanges { + t.Fatal("cleanup snapshot omitted discard consent") } var jobID string if err := repo.DB().QueryRowContext(ctx, ` @@ -71,21 +168,113 @@ func TestDeleteTaskCleanupRetriesWhenAuditedWorktreeRemovalIsUnsafe(t *testing.T `, taskID).Scan(&jobID); err != nil { t.Fatalf("load delete cleanup job: %v", err) } - if err := svc.processTaskResourceCleanupJob(ctx, jobID); err == nil { - t.Fatal("unsafe worktree cleanup returned nil") + if err := svc.processTaskResourceCleanupJob(ctx, jobID); err != nil { + t.Fatalf("process delete cleanup job: %v", err) } - - job, err := repo.GetTaskResourceCleanupJob(ctx, jobID) - if err != nil { - t.Fatalf("reload cleanup job: %v", err) - } - if job.State != models.TaskResourceCleanupStateRetryWait || job.NextAttemptAt == nil { - t.Fatalf("cleanup job state = %q next=%v, want retry_wait with next attempt", job.State, job.NextAttemptAt) - } - if !strings.Contains(job.LastError, "uncommitted or untracked work") { - t.Fatalf("cleanup last error = %q, want audited-work refusal", job.LastError) - } - if got, err := os.ReadFile(sentinel); err != nil || string(got) != "preserve me\n" { - t.Fatalf("preserved work changed: contents=%q err=%v", got, err) + if _, statErr := os.Stat(wt.Path); !os.IsNotExist(statErr) { + t.Fatalf("consented cleanup left worktree on disk: %s (stat err = %v)", wt.Path, statErr) + } +} + +type dirtyWorktreeCleanupFailure struct{} + +func (dirtyWorktreeCleanupFailure) OnTaskDeleted(context.Context, string) error { return nil } + +func (dirtyWorktreeCleanupFailure) GetAllByTaskID(context.Context, string) ([]*worktree.Worktree, error) { + return nil, nil +} + +func (dirtyWorktreeCleanupFailure) CleanupWorktrees(context.Context, []*worktree.Worktree) error { + return fmt.Errorf("cleanup worktrees: %w", worktree.ErrDirtyWorktreeCleanup) +} + +// @covers AC-TASKS-RUNTIME-CLEANUP-001.11 +func TestDirtyWorktreeCleanupRefusalBecomesTerminalAfterAdmission(t *testing.T) { + ctx := context.Background() + svc, _, repo := createTestService(t) + svc.StopTaskResourceCleanupWorker() + svc.SetWorktreeCleanup(dirtyWorktreeCleanupFailure{}) + snapshot, err := json.Marshal(taskResourceCleanupSnapshot{ + Worktrees: []*worktree.Worktree{{ID: "wt-race", TaskID: "task-race"}}, + }) + if err != nil { + t.Fatalf("encode snapshot: %v", err) + } + job := &models.TaskResourceCleanupJob{ + ID: "dirty-worktree-terminal", OperationID: "delete:dirty-worktree-terminal", + TaskID: "task-race", Trigger: models.TaskResourceCleanupTriggerDelete, + State: models.TaskResourceCleanupStatePending, ResourceSnapshot: string(snapshot), + } + if err := repo.CreateTaskResourceCleanupJob(ctx, job); err != nil { + t.Fatalf("create cleanup job: %v", err) + } + if err := svc.processTaskResourceCleanupJob(ctx, job.ID); !errors.Is(err, worktree.ErrDirtyWorktreeCleanup) { + t.Fatalf("process cleanup error = %v, want dirty-worktree refusal", err) + } + got, err := repo.GetTaskResourceCleanupJob(ctx, job.ID) + if err != nil { + t.Fatalf("get cleanup job: %v", err) + } + if got.State != models.TaskResourceCleanupStateFailed { + t.Fatalf("cleanup state = %q, want failed", got.State) + } + if got.NextAttemptAt != nil { + t.Fatalf("terminal cleanup scheduled retry at %v", got.NextAttemptAt) + } +} + +func TestDirtyWorktreeCleanupMixedFailureRemainsRetryable(t *testing.T) { + dirtyErr := fmt.Errorf("cleanup worktrees: %w", worktree.ErrDirtyWorktreeCleanup) + mixedErr := errors.Join(dirtyErr, errors.New("temporary environment cleanup failure")) + if isDirtyWorktreeCleanupError(mixedErr) { + t.Fatal("mixed cleanup failure was classified as a sole dirty-worktree refusal") + } + if !isDirtyWorktreeCleanupError(dirtyErr) { + t.Fatal("wrapped dirty-worktree refusal was not classified as terminal") + } +} + +func TestDirtyWorktreeCleanupBatchOrderRemainsRetryable(t *testing.T) { + ctx := context.Background() + _, _, repo := createTestService(t) + const taskID = "task-batch-cleanup-order" + const sessionID = "session-batch-cleanup-order" + seedCleanupTaskAndSession(t, repo, taskID, sessionID) + + repoPath := initSimpleGitRepo(t) + if err := repo.CreateRepository(ctx, &models.Repository{ + ID: "repo-batch-cleanup-order", WorkspaceID: "ws-" + taskID, + Name: "repo-batch-cleanup-order", SourceType: "local", LocalPath: repoPath, + }); err != nil { + t.Fatalf("create repository: %v", err) + } + mgr := newCleanupTestWorktreeManager(t, repo) + wt, err := mgr.Create(ctx, worktree.CreateRequest{ + TaskID: taskID, SessionID: sessionID, TaskTitle: "Batch cleanup order", + RepositoryID: "repo-batch-cleanup-order", RepositoryPath: repoPath, + BaseBranch: "main", TaskDirName: taskID, RepoName: "repo-batch-cleanup-order", + }) + if err != nil { + t.Fatalf("create worktree: %v", err) + } + dirtyPath := filepath.Join(wt.Path, "dirty-before-retry.txt") + if err := os.WriteFile(dirtyPath, []byte("preserve me\n"), 0o644); err != nil { + t.Fatalf("write dirty worktree file: %v", err) + } + + // The first inventory row fails for a retryable metadata reason. The second + // row refuses cleanup because it is dirty. Both failures must reach the job + // classifier; returning only the final sentinel would incorrectly terminalize + // this batch. + firstFailure := &worktree.Worktree{ID: "wt-retryable-first", TaskID: taskID} + cleanupErr := mgr.CleanupWorktrees(ctx, []*worktree.Worktree{firstFailure, wt}) + if cleanupErr == nil { + t.Fatal("batch cleanup succeeded despite two failures") + } + if !errors.Is(cleanupErr, worktree.ErrDirtyWorktreeCleanup) { + t.Fatalf("batch cleanup error = %v, want dirty refusal included", cleanupErr) + } + if isDirtyWorktreeCleanupError(cleanupErr) { + t.Fatalf("batch cleanup was classified as terminal: %v", cleanupErr) } } diff --git a/apps/backend/internal/task/service/service.go b/apps/backend/internal/task/service/service.go index 4d76c9ebec..5dd9e49ad5 100644 --- a/apps/backend/internal/task/service/service.go +++ b/apps/backend/internal/task/service/service.go @@ -73,6 +73,12 @@ type WorktreeCleanupIdentityProvider interface { CaptureCleanupHeadOIDs(ctx context.Context, worktrees []*worktree.Worktree) (map[string]string, error) } +// WorktreeDirtyInspector reports local changes before a task deletion mutates +// task rows or persists a cleanup job. +type WorktreeDirtyInspector interface { + InspectDirtyWorktrees(ctx context.Context, worktrees []*worktree.Worktree) ([]worktree.DirtyWorktree, error) +} + // WorktreeBatchCleaner extends WorktreeProvider with batch cleanup. type WorktreeBatchCleaner interface { WorktreeProvider @@ -80,6 +86,22 @@ type WorktreeBatchCleaner interface { CleanupWorktrees(ctx context.Context, worktrees []*worktree.Worktree) error } +// WorktreeBatchCleanerWithOptions is the consent-aware cleanup extension. The +// legacy batch method remains available for archive and non-consented cleanup. +type WorktreeBatchCleanerWithOptions interface { + WorktreeBatchCleaner + CleanupWorktreesWithOptions( + ctx context.Context, + worktrees []*worktree.Worktree, + options worktree.WorktreeCleanupOptions, + ) error +} + +// DeleteTaskOptions controls destructive task deletion behavior. +type DeleteTaskOptions struct { + DiscardWorktreeChanges bool +} + // WorktreeArchiveBatchCleaner removes archived task worktrees without deleting // the local branches required for later recovery. type WorktreeArchiveBatchCleaner interface { diff --git a/apps/backend/internal/task/service/service_tasks.go b/apps/backend/internal/task/service/service_tasks.go index c6266c1caf..c6b39e5d71 100644 --- a/apps/backend/internal/task/service/service_tasks.go +++ b/apps/backend/internal/task/service/service_tasks.go @@ -110,9 +110,10 @@ type taskStopTarget struct { } type taskEnvironmentCleanup struct { - env *models.TaskEnvironment - deleteRow bool - preserveBranches bool + env *models.TaskEnvironment + deleteRow bool + preserveBranches bool + discardWorktreeChanges bool } type taskEnvironmentSessionUsageChecker interface { @@ -2610,21 +2611,33 @@ func (s *Service) registerTaskRuntimeStopOwners(stopTargets []taskStopTarget, fo // For fast UI response, the DB delete and event publish happen synchronously, // while agent stopping and worktree cleanup happen asynchronously. func (s *Service) DeleteTask(ctx context.Context, id string) error { - return s.deleteTaskWithReason(ctx, id, "") + return s.deleteTaskWithReasonAndOptions(ctx, id, "", DeleteTaskOptions{}) +} + +// DeleteTaskWithOptions deletes a task with explicit consent for destructive +// worktree cleanup. +func (s *Service) DeleteTaskWithOptions(ctx context.Context, id string, options DeleteTaskOptions) error { + return s.deleteTaskWithReasonAndOptions(ctx, id, "", options) } // DeleteTaskWithReason behaves like DeleteTask but attaches a machine-readable // reason (e.g. "pr_approved_by_user") to the task.deleted event so the frontend // can explain why a focused task vanished. func (s *Service) DeleteTaskWithReason(ctx context.Context, id, reason string) error { - return s.deleteTaskWithReason(ctx, id, reason) + return s.deleteTaskWithReasonAndOptions(ctx, id, reason, DeleteTaskOptions{}) } func (s *Service) deleteTaskWithReason(ctx context.Context, id, reason string) error { + return s.deleteTaskWithReasonAndOptions(ctx, id, reason, DeleteTaskOptions{}) +} + +func (s *Service) deleteTaskWithReasonAndOptions( + ctx context.Context, id, reason string, options DeleteTaskOptions, +) error { if err := s.authorizeTaskScope(ctx, id, authz.ScopeTaskWrite); err != nil { return err } - _, err := s.deleteTaskWithReasonAndDBDelete(ctx, id, reason, models.TaskResourceCleanupTriggerDelete, func(ctx context.Context, id string) (bool, error) { + _, err := s.deleteTaskWithReasonAndDBDelete(ctx, id, reason, models.TaskResourceCleanupTriggerDelete, options, func(ctx context.Context, id string) (bool, error) { if err := s.tasks.DeleteTask(ctx, id); err != nil { return false, err } @@ -2633,8 +2646,54 @@ func (s *Service) deleteTaskWithReason(ctx context.Context, id, reason string) e return err } +// ValidateTaskDeleteWorktrees inspects every worktree in a delete set before +// the caller mutates task or environment ownership state. +func (s *Service) ValidateTaskDeleteWorktrees( + ctx context.Context, taskIDs []string, discardWorktreeChanges bool, +) error { + if s.worktreeCleanup == nil { + return nil + } + provider, ok := s.worktreeCleanup.(WorktreeProvider) + if !ok { + return nil + } + worktrees := make([]*worktree.Worktree, 0) + for _, taskID := range taskIDs { + if strings.TrimSpace(taskID) == "" { + continue + } + inventory, err := provider.GetAllByTaskID(ctx, taskID) + if err != nil { + return fmt.Errorf("list worktrees for delete admission: %w", err) + } + worktrees = append(worktrees, inventory...) + } + return s.validateTaskDeleteWorktreeInventory(ctx, worktrees, discardWorktreeChanges) +} + +func (s *Service) validateTaskDeleteWorktreeInventory( + ctx context.Context, worktrees []*worktree.Worktree, discardWorktreeChanges bool, +) error { + if len(worktrees) == 0 || s.worktreeCleanup == nil { + return nil + } + inspector, ok := s.worktreeCleanup.(WorktreeDirtyInspector) + if !ok { + return nil + } + dirty, err := inspector.InspectDirtyWorktrees(ctx, worktrees) + if err != nil { + return fmt.Errorf("inspect worktrees before delete: %w", err) + } + if discardWorktreeChanges { + return nil + } + return newTaskDeleteDirtyWorktreeError(dirty) +} + func (s *Service) deleteExpiredQuickChatTask(ctx context.Context, id string, cutoff time.Time) (bool, error) { - deleted, err := s.deleteTaskWithReasonAndDBDelete(ctx, id, "", models.TaskResourceCleanupTriggerQuickChatExpire, func(ctx context.Context, id string) (bool, error) { + deleted, err := s.deleteTaskWithReasonAndDBDelete(ctx, id, "", models.TaskResourceCleanupTriggerQuickChatExpire, DeleteTaskOptions{}, func(ctx context.Context, id string) (bool, error) { return s.tasks.DeleteExpiredQuickChatTask(ctx, id, cutoff) }) if errors.Is(err, taskrepo.ErrTaskNotFound) { @@ -2648,6 +2707,7 @@ func (s *Service) deleteTaskWithReasonAndDBDelete( id string, reason string, trigger models.TaskResourceCleanupTrigger, + options DeleteTaskOptions, deleteFromDB func(context.Context, string) (bool, error), ) (bool, error) { start := time.Now() @@ -2668,6 +2728,11 @@ func (s *Service) deleteTaskWithReasonAndDBDelete( if err != nil { return false, fmt.Errorf("list worktrees for delete: %w", err) } + if trigger == models.TaskResourceCleanupTriggerDelete { + if err := s.validateTaskDeleteWorktreeInventory(ctx, worktrees, options.DiscardWorktreeChanges); err != nil { + return false, err + } + } taskEnv, err := s.gatherTaskEnvironmentForCleanup(ctx, id) if err != nil { return false, fmt.Errorf("lookup task environment for delete: %w", err) @@ -2694,7 +2759,9 @@ func (s *Service) deleteTaskWithReasonAndDBDelete( } } - envCleanup := taskEnvironmentCleanup{env: taskEnv, deleteRow: false} + envCleanup := taskEnvironmentCleanup{ + env: taskEnv, deleteRow: false, discardWorktreeChanges: options.DiscardWorktreeChanges, + } cleanupJob, err := s.persistTaskResourceCleanup( ctx, id, trigger, "", sessions, worktrees, stopTargets, envCleanup, true, ) @@ -3578,6 +3645,14 @@ func (s *Service) cleanupDestructiveTaskResources( return append(errs, errors.New("worktree cleaner cannot preserve branches during archive cleanup")) } cleanupErr = cleaner.CleanupWorktreesPreservingBranches(ctx, worktrees) + } else if envCleanup.discardWorktreeChanges { + cleaner, ok := s.worktreeCleanup.(WorktreeBatchCleanerWithOptions) + if !ok { + return append(errs, errors.New("worktree cleaner cannot honor discard consent")) + } + cleanupErr = cleaner.CleanupWorktreesWithOptions(ctx, worktrees, worktree.WorktreeCleanupOptions{ + DiscardWorktreeChanges: true, + }) } else if cleaner, ok := s.worktreeCleanup.(WorktreeBatchCleaner); ok { cleanupErr = cleaner.CleanupWorktrees(ctx, worktrees) } diff --git a/apps/backend/internal/task/service/task_delete_errors.go b/apps/backend/internal/task/service/task_delete_errors.go new file mode 100644 index 0000000000..4ae74f0287 --- /dev/null +++ b/apps/backend/internal/task/service/task_delete_errors.go @@ -0,0 +1,49 @@ +package service + +import ( + "errors" + + "github.com/kandev/kandev/internal/worktree" +) + +const TaskDeleteDirtyWorktreeErrorCode = "task_delete_dirty_worktree" + +// TaskDeleteDirtyWorktreeError is returned before task deletion when one or +// more owned worktrees contain local changes and the caller did not grant +// discard consent. +type TaskDeleteDirtyWorktreeError struct { + DirtyWorktrees []worktree.DirtyWorktree +} + +func (e *TaskDeleteDirtyWorktreeError) Error() string { + return "task worktree contains local changes" +} + +func newTaskDeleteDirtyWorktreeError(dirty []worktree.DirtyWorktree) error { + if len(dirty) == 0 { + return nil + } + copyDirty := make([]worktree.DirtyWorktree, len(dirty)) + copy(copyDirty, dirty) + for i := range copyDirty { + copyDirty[i].DirtyFiles = append([]string(nil), copyDirty[i].DirtyFiles...) + } + return &TaskDeleteDirtyWorktreeError{DirtyWorktrees: copyDirty} +} + +func isDirtyWorktreeCleanupError(err error) bool { + // A dirty refusal is terminal only when it is the sole cleanup failure. + // Joined errors can also contain retryable environment or remote cleanup + // failures that must keep their retry schedule. + if err == nil { + return false + } + if joined, ok := err.(interface{ Unwrap() []error }); ok { + causes := joined.Unwrap() + return len(causes) == 1 && isDirtyWorktreeCleanupError(causes[0]) + } + if wrapped, ok := err.(interface{ Unwrap() error }); ok { + return isDirtyWorktreeCleanupError(wrapped.Unwrap()) + } + return errors.Is(err, worktree.ErrDirtyWorktreeCleanup) +} diff --git a/apps/backend/internal/worktree/manager_cleanup.go b/apps/backend/internal/worktree/manager_cleanup.go index d96c02a62a..fbf381ea9c 100644 --- a/apps/backend/internal/worktree/manager_cleanup.go +++ b/apps/backend/internal/worktree/manager_cleanup.go @@ -193,7 +193,7 @@ func (m *Manager) RemoveByID(ctx context.Context, worktreeID string, removeBranc return err } m.enrichCleanupWorktreeFromCache(wt) - return m.removeWorktree(ctx, wt, removeBranch) + return m.removeWorktree(ctx, wt, removeBranch, WorktreeCleanupOptions{}) } func (m *Manager) enrichCleanupWorktreeFromCache(wt *Worktree) { @@ -271,7 +271,12 @@ func (m *Manager) CaptureCleanupHeadOIDs(ctx context.Context, worktrees []*Workt } // removeWorktree performs the actual removal of a worktree. -func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch bool) error { +func (m *Manager) removeWorktree( + ctx context.Context, + wt *Worktree, + removeBranch bool, + options WorktreeCleanupOptions, +) error { if wt == nil { return errors.New("worktree cleanup requires a worktree") } @@ -320,7 +325,7 @@ func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch return nil } - audit, err := m.auditWorktreeCleanup(ctx, wt, removeBranch) + audit, err := m.auditWorktreeCleanup(ctx, wt, removeBranch, options) if err != nil { return fmt.Errorf("audit worktree cleanup %s: %w", wt.ID, err) } @@ -506,21 +511,37 @@ func (m *Manager) managedScriptEnvironment(ctx context.Context) map[string]strin // CleanupWorktrees removes provided worktrees without re-fetching from the store. func (m *Manager) CleanupWorktrees(ctx context.Context, worktrees []*Worktree) error { - return m.cleanupWorktrees(ctx, worktrees, true) + return m.cleanupWorktrees(ctx, worktrees, true, WorktreeCleanupOptions{}) +} + +// CleanupWorktreesWithOptions removes worktrees while retaining all cleanup +// identity and branch-safety audits. Discard consent only changes the clean +// checkout gate. +func (m *Manager) CleanupWorktreesWithOptions( + ctx context.Context, + worktrees []*Worktree, + options WorktreeCleanupOptions, +) error { + return m.cleanupWorktrees(ctx, worktrees, true, options) } // CleanupWorktreesPreservingBranches removes provided worktrees while retaining // their local branch refs for later archive recovery. func (m *Manager) CleanupWorktreesPreservingBranches(ctx context.Context, worktrees []*Worktree) error { - return m.cleanupWorktrees(ctx, worktrees, false) + return m.cleanupWorktrees(ctx, worktrees, false, WorktreeCleanupOptions{}) } -func (m *Manager) cleanupWorktrees(ctx context.Context, worktrees []*Worktree, removeBranch bool) error { +func (m *Manager) cleanupWorktrees( + ctx context.Context, + worktrees []*Worktree, + removeBranch bool, + options WorktreeCleanupOptions, +) error { if len(worktrees) == 0 { return nil } - var lastErr error + var errs []error for _, wt := range worktrees { if wt == nil { continue @@ -532,16 +553,16 @@ func (m *Manager) cleanupWorktrees(ctx context.Context, worktrees []*Worktree, r continue } m.enrichCleanupWorktreeFromCache(wt) - if err := m.removeWorktree(ctx, wt, removeBranch); err != nil { + if err := m.removeWorktree(ctx, wt, removeBranch, options); err != nil { m.logger.Warn("failed to remove worktree during batch cleanup", zap.String("task_id", wt.TaskID), zap.String("worktree_id", wt.ID), zap.Error(err)) - lastErr = err + errs = append(errs, fmt.Errorf("cleanup worktree %s: %w", wt.ID, err)) } } - return lastErr + return errors.Join(errs...) } // OnTaskDeleted cleans up all worktrees for a task when it is deleted. diff --git a/apps/backend/internal/worktree/manager_cleanup_audit.go b/apps/backend/internal/worktree/manager_cleanup_audit.go index 0f2a12e564..db634bbd88 100644 --- a/apps/backend/internal/worktree/manager_cleanup_audit.go +++ b/apps/backend/internal/worktree/manager_cleanup_audit.go @@ -12,6 +12,10 @@ import ( storageworkspaces "github.com/kandev/kandev/internal/system/storage/workspaces" ) +// ErrDirtyWorktreeCleanup identifies the expected refusal when cleanup reaches +// a checkout that contains local changes without explicit discard consent. +var ErrDirtyWorktreeCleanup = errors.New("worktree cleanup refused because worktree contains local changes") + type worktreeCleanupAudit struct { branchRef string branchOID string @@ -22,7 +26,7 @@ type worktreeCleanupAudit struct { } func (m *Manager) auditWorktreeCleanup( - ctx context.Context, wt *Worktree, removeBranch bool, + ctx context.Context, wt *Worktree, removeBranch bool, options WorktreeCleanupOptions, ) (audit worktreeCleanupAudit, err error) { if wt == nil || wt.RepositoryPath == "" || wt.Path == "" { return worktreeCleanupAudit{}, errors.New("worktree cleanup requires repository and worktree paths") @@ -61,7 +65,7 @@ func (m *Manager) auditWorktreeCleanup( return worktreeCleanupAudit{}, fmt.Errorf("worktree cleanup path is not registered to the recorded branch: %s", wt.Path) } deleteBranch, err := m.auditCleanupBranchDisposition( - ctx, wt, branchRef, branchOID, pathPresent, removeBranch, + ctx, wt, branchRef, branchOID, pathPresent, removeBranch, options, ) if err != nil { return worktreeCleanupAudit{}, err @@ -79,11 +83,12 @@ func (m *Manager) auditCleanupBranchDisposition( branchOID string, pathPresent bool, removeBranch bool, + options WorktreeCleanupOptions, ) (bool, error) { if !removeBranch || branchRef == "" || branchOID == "" { return false, nil } - if pathPresent { + if pathPresent && !options.DiscardWorktreeChanges { return m.verifyCleanRedundantCheckout(ctx, wt, branchRef) } return m.verifyCleanupBranchRedundant(ctx, wt, branchRef) @@ -172,7 +177,7 @@ func (m *Manager) verifyCleanRedundantCheckout( return false, fmt.Errorf("inspect worktree changes before cleanup: %w", err) } if strings.TrimSpace(status) != "" { - return false, fmt.Errorf("worktree cleanup refused because %q contains uncommitted or untracked work", wt.Path) + return false, fmt.Errorf("%w: %q contains uncommitted or untracked work", ErrDirtyWorktreeCleanup, wt.Path) } return m.verifyCleanupBranchRedundant(ctx, wt, branchRef) } diff --git a/apps/backend/internal/worktree/manager_cleanup_dirty.go b/apps/backend/internal/worktree/manager_cleanup_dirty.go new file mode 100644 index 0000000000..eebb9fb35e --- /dev/null +++ b/apps/backend/internal/worktree/manager_cleanup_dirty.go @@ -0,0 +1,117 @@ +package worktree + +import ( + "context" + "fmt" + "path/filepath" + "sort" + "strings" +) + +// DirtyWorktree describes local changes that would be lost by task deletion. +// Paths are derived from the persisted worktree record and Git status; callers +// never provide a filesystem path for this inspection. +type DirtyWorktree struct { + WorktreeID string `json:"worktree_id"` + RepositoryID string `json:"repository_id"` + Path string `json:"path"` + DirtyFiles []string `json:"dirty_files"` +} + +// WorktreeCleanupOptions controls the destructive part of worktree cleanup. +// All identity, registration, path ownership, and branch safety checks remain +// active regardless of this option. +type WorktreeCleanupOptions struct { + DiscardWorktreeChanges bool +} + +// InspectDirtyWorktrees audits the recorded paths and reports local Git +// changes without mutating the checkout. Inspection errors fail closed so a +// caller cannot treat an unknown checkout state as clean. +func (m *Manager) InspectDirtyWorktrees( + ctx context.Context, worktrees []*Worktree, +) ([]DirtyWorktree, error) { + dirty := make([]DirtyWorktree, 0) + seen := make(map[string]struct{}, len(worktrees)) + for _, wt := range worktrees { + if wt == nil || strings.TrimSpace(wt.ID) == "" { + continue + } + m.enrichCleanupWorktreeFromCache(wt) + if strings.TrimSpace(wt.RepositoryPath) == "" || strings.TrimSpace(wt.Path) == "" { + return nil, fmt.Errorf("worktree %s is missing repository or worktree path metadata", wt.ID) + } + key := wt.RepositoryPath + "\x00" + filepath.Clean(wt.Path) + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + + if err := m.validateExistingWorktreePathOwner(wt.Path, wt); err != nil { + return nil, fmt.Errorf("validate worktree cleanup path %s: %w", wt.ID, err) + } + pathPresent, err := cleanupPathPresent(wt.Path) + if err != nil { + return nil, fmt.Errorf("inspect worktree cleanup path %s: %w", wt.ID, err) + } + if !pathPresent { + continue + } + pathHandle, err := m.openCleanupPathHandle(wt.Path, true) + if err != nil { + return nil, fmt.Errorf("pin worktree cleanup path %s: %w", wt.ID, err) + } + status, statusErr := m.runBoundedGitInspect( + ctx, wt.Path, "status", "--porcelain=v1", "--untracked-files=normal", "-z", + ) + if pathHandle != nil { + _ = pathHandle.Close() + } + if statusErr != nil { + return nil, fmt.Errorf("inspect worktree changes for %s: %w", wt.ID, statusErr) + } + files := parseDirtyWorktreeFiles(status) + if len(files) == 0 { + continue + } + dirty = append(dirty, DirtyWorktree{ + WorktreeID: wt.ID, + RepositoryID: wt.RepositoryID, + Path: filepath.Clean(wt.Path), + DirtyFiles: files, + }) + } + return dirty, nil +} + +func parseDirtyWorktreeFiles(status string) []string { + seen := make(map[string]struct{}) + files := make([]string, 0) + skipNext := false + for _, record := range strings.Split(status, "\x00") { + if skipNext { + skipNext = false + continue + } + if len(record) < 4 || record[2] != ' ' { + continue + } + if record[0] == 'R' || record[0] == 'C' { + // With -z, Git emits the rename/copy origin path as the next + // NUL-delimited field without an XY status prefix. The destination + // above is the path users need to review before discarding. + skipNext = true + } + path := record[3:] + if path == "" { + continue + } + if _, ok := seen[path]; ok { + continue + } + seen[path] = struct{}{} + files = append(files, path) + } + sort.Strings(files) + return files +} diff --git a/apps/backend/internal/worktree/manager_cleanup_dirty_test.go b/apps/backend/internal/worktree/manager_cleanup_dirty_test.go new file mode 100644 index 0000000000..21fcc96adc --- /dev/null +++ b/apps/backend/internal/worktree/manager_cleanup_dirty_test.go @@ -0,0 +1,55 @@ +package worktree + +import ( + "context" + "reflect" + "strings" + "testing" +) + +func TestParseDirtyWorktreeFiles(t *testing.T) { + status := " M tracked.txt\x00?? untracked.txt\x00R renamed-new.txt\x00renamed-old.txt\x00C copied-new.txt\x00copied-old.txt\x00" + got := parseDirtyWorktreeFiles(status) + want := []string{"copied-new.txt", "renamed-new.txt", "tracked.txt", "untracked.txt"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("dirty files = %#v, want %#v", got, want) + } +} + +func TestParseDirtyWorktreeFilesPreservesPathWhitespace(t *testing.T) { + status := "?? leading and trailing.txt \x00" + got := parseDirtyWorktreeFiles(status) + want := []string{" leading and trailing.txt "} + if !reflect.DeepEqual(got, want) { + t.Fatalf("dirty files = %#v, want %#v", got, want) + } +} + +func TestInspectDirtyWorktreesFailsClosedWhenRequiredMetadataIsMissing(t *testing.T) { + mgr, _ := newReferenceCleanupTestManager(t) + tests := []struct { + name string + repositoryPath string + worktreePath string + }{ + {name: "missing repository path", worktreePath: "/tmp/worktree"}, + {name: "missing worktree path", repositoryPath: "/tmp/repository"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + id := "wt-missing-metadata" + _, err := mgr.InspectDirtyWorktrees(context.Background(), []*Worktree{{ + ID: id, + RepositoryPath: tt.repositoryPath, + Path: tt.worktreePath, + }}) + if err == nil { + t.Fatal("inspection succeeded without required worktree metadata") + } + if !strings.Contains(err.Error(), id) { + t.Fatalf("inspection error = %v, want worktree id %q", err, id) + } + }) + } +} diff --git a/apps/web/app/tasks/columns.tsx b/apps/web/app/tasks/columns.tsx index e26eedcfe2..dc763dd018 100644 --- a/apps/web/app/tasks/columns.tsx +++ b/apps/web/app/tasks/columns.tsx @@ -27,7 +27,10 @@ interface ColumnsConfig { steps: WorkflowStep[]; repositories: Repository[]; onArchive: (taskId: string, opts?: { cascade?: boolean }) => void; - onDelete: (taskId: string, opts?: { cascade?: boolean }) => void; + onDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => void; deletingTaskId: string | null; } @@ -66,7 +69,7 @@ function TitleCell({ type ActionsCtx = { onArchive: (id: string, opts?: { cascade?: boolean }) => void; - onDelete: (id: string, opts?: { cascade?: boolean }) => void; + onDelete: (id: string, opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }) => void; deletingTaskId: string | null; }; @@ -130,7 +133,9 @@ function ActionsCell({ row, ctx }: { row: Row; ctx: ActionsC taskId={task.id} executorType={task.primary_executor_type} isDeleting={isDeleting} - onConfirm={({ cascade }) => ctx.onDelete(task.id, { cascade })} + onConfirm={({ cascade, discardWorktreeChanges }) => + ctx.onDelete(task.id, { cascade, discardWorktreeChanges }) + } /> Promise; handleUnarchive: (taskId: string) => Promise; - handleDelete: (taskId: string, opts?: { cascade?: boolean }) => Promise; + handleDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; onRefresh?: () => void | Promise; }; @@ -153,7 +156,10 @@ function TaskRows({ deletingTaskId: string | null; onArchive: (taskId: string, opts?: { cascade?: boolean }) => Promise; onUnarchive: (taskId: string) => Promise; - onDelete: (taskId: string, opts?: { cascade?: boolean }) => Promise; + onDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; onRowClick: (task: Task) => void; facetValues?: Record; }) { @@ -356,7 +362,10 @@ function TaskListRow({ deletingTaskId: string | null; onArchive: (taskId: string, opts?: { cascade?: boolean }) => Promise; onUnarchive: (taskId: string) => Promise; - onDelete: (taskId: string, opts?: { cascade?: boolean }) => Promise; + onDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; onRowClick: (task: Task) => void; }) { const [showDeleteConfirm, setShowDeleteConfirm] = useState(false); @@ -426,7 +435,10 @@ function TaskListSectionView({ deletingTaskId: string | null; onArchive: (taskId: string, opts?: { cascade?: boolean }) => Promise; onUnarchive: (taskId: string) => Promise; - onDelete: (taskId: string, opts?: { cascade?: boolean }) => Promise; + onDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; onRowClick: (task: Task) => void; }) { const rows = flattenTaskTree(section.nodes); @@ -530,7 +542,10 @@ function TaskRowActions({ onArchiveOpenChange: (open: boolean) => void; onArchive: (taskId: string, opts?: { cascade?: boolean }) => Promise; onUnarchive: (taskId: string) => Promise; - onDelete: (taskId: string, opts?: { cascade?: boolean }) => Promise; + onDelete: ( + taskId: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; }) { const { t } = useTranslation(); const archiveAnchorRef = useRef(null); @@ -587,7 +602,9 @@ function TaskRowActions({ isInFlight={isTaskInFlight(task.foreground_activity)} executorType={task.primary_executor_type} isDeleting={isDeleting} - onConfirm={({ cascade }) => onDelete(task.id, { cascade })} + onConfirm={({ cascade, discardWorktreeChanges }) => + onDelete(task.id, { cascade, discardWorktreeChanges }) + } /> void) { ); const handleDelete = useCallback( - async (taskId: string, opts?: { cascade?: boolean }) => { + async (taskId: string, opts?: DeleteTaskParams) => { setDeletingTaskId(taskId); try { await deleteTask(taskId, opts); fetchTasks(); } catch (err) { toast({ - title: t("tasks:failedToDeleteTask"), - description: errorDescription(err), + title: isTaskDeleteDirtyWorktreeError(err) + ? t("task:deleteDirtyWorktreeTitle") + : t("tasks:failedToDeleteTask"), + description: isTaskDeleteDirtyWorktreeError(err) + ? t("task:deleteDirtyWorktreeDescription") + : errorDescription(err), variant: "error", }); } finally { @@ -506,6 +510,23 @@ function useTasksListPreferenceSync({ return { handleSortChange, handleGroupChange }; } +function useTasksPageClientEffects({ + setMobileSearchOpen, + setView, +}: { + setMobileSearchOpen: (open: boolean) => void; + setView: (view: "list") => void; +}) { + useEffect(() => { + setMobileSearchOpen(false); + return () => setMobileSearchOpen(false); + }, [setMobileSearchOpen]); + + useEffect(() => { + setView("list"); + }, [setView]); +} + export function TasksPageClient(props: TasksPageClientProps) { const s = useTasksPageSetup(props); const [isCreateOpen, setIsCreateOpen] = useState(false); @@ -523,8 +544,6 @@ export function TasksPageClient(props: TasksPageClientProps) { state.kanban.workflowId === s.activeWorkflowId ? state.kanban.steps : EMPTY_WORKFLOW_STEPS, ); useWorkflowSnapshot(s.activeWorkflowId); - // Task-title previews render PR/MR glyphs independently of the list-details - // preference, so both workspace caches must be hydrated here. useWorkspacePRs(s.activeWorkspaceId); useWorkspaceMRs(s.activeWorkspaceId); useForegroundRefresh(() => s.fetchTasks(true), Boolean(s.activeWorkspaceId), s.activeWorkspaceId); @@ -546,14 +565,7 @@ export function TasksPageClient(props: TasksPageClientProps) { onCoreGroupChange: handleGroupChange, }); - useEffect(() => { - setMobileSearchOpen(false); - return () => setMobileSearchOpen(false); - }, [setMobileSearchOpen]); - - useEffect(() => { - setView("list"); - }, [setView]); + useTasksPageClientEffects({ setMobileSearchOpen, setView }); return ( s.fetchTasks()} - mobileActions={mobileTaskActions({ - isMobile, - workspaceId: s.activeWorkspaceId, - workflowId: s.activeWorkflowId, - steps: activeSteps, - open: isCreateOpen, - onOpenChange: setIsCreateOpen, - onCreated: () => s.fetchTasks(true), - })} - /> - ); -} - -function mobileTaskActions({ - isMobile, - workspaceId, - workflowId, - steps, - open, - onOpenChange, - onCreated, -}: { - isMobile: boolean; - workspaceId: string | null; - workflowId: string | null; - steps: MobileTaskStep[]; - open: boolean; - onOpenChange: (open: boolean) => void; - onCreated: () => Promise; -}) { - if (!isMobile || !workspaceId) return null; - return ( - s.fetchTasks(true)} + /> + ) : null + } /> ); } diff --git a/apps/web/app/tasks/tasks-page-content.tsx b/apps/web/app/tasks/tasks-page-content.tsx index 63b26cccf4..538069e8dc 100644 --- a/apps/web/app/tasks/tasks-page-content.tsx +++ b/apps/web/app/tasks/tasks-page-content.tsx @@ -31,7 +31,10 @@ type Props = { deletingTaskId: string | null; onArchive: (id: string, opts?: { cascade?: boolean }) => Promise; onUnarchive: (id: string) => Promise; - onDelete: (id: string, opts?: { cascade?: boolean }) => Promise; + onDelete: ( + id: string, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => Promise; onRefresh: () => Promise; mobileActions: ReactNode; }; diff --git a/apps/web/components/kanban-card.tsx b/apps/web/components/kanban-card.tsx index 88a340b38a..923a8cb538 100644 --- a/apps/web/components/kanban-card.tsx +++ b/apps/web/components/kanban-card.tsx @@ -26,6 +26,7 @@ import { TaskGitHubPRDialog } from "@/components/task/task-github-pr-dialog"; import { TaskMRLinkDialog } from "@/components/gitlab/task-mr-link-dialog"; import { useTaskWorkflowMove } from "@/hooks/use-task-workflow-move"; import { useTaskMultiSelectStore } from "@/hooks/use-task-multi-select"; +import type { TaskActionOptions } from "@/hooks/use-task-actions"; import { useDetachTask } from "@/hooks/use-detach-task"; import { useUpdateTaskPriority } from "@/hooks/use-update-task-priority"; import { @@ -136,8 +137,8 @@ interface KanbanCardProps { repositoryChips?: RepositoryChip[]; onClick?: (task: Task) => void; onEdit?: (task: Task) => void; - onDelete?: (task: Task, opts?: { cascade?: boolean }) => void; - onArchive?: (task: Task, opts?: { cascade?: boolean }) => void; + onDelete?: (task: Task, opts?: TaskActionOptions) => void; + onArchive?: (task: Task, opts?: TaskActionOptions) => void; onOpenFullPage?: (task: Task) => void; onMove?: (task: Task, targetStepId: string) => void; steps?: WorkflowStep[]; @@ -174,10 +175,7 @@ function useKanbanCardMoveMenuActions({ }); }; const moveToStepFromDropdown = (stepId: string) => { - if (onMove) { - onMove(task, stepId); - return; - } + if (onMove) return onMove(task, stepId); if (moveTargets.currentWorkflowId) { runMoveTasks([task.id], moveTargets.currentWorkflowId, stepId, "step"); } @@ -416,7 +414,7 @@ function KanbanCardDialogs({ taskId={task.id} executorType={task.primaryExecutorType} isDeleting={isDeleting} - onConfirm={({ cascade }) => onDelete?.(task, { cascade })} + onConfirm={(opts) => onDelete?.(task, opts)} /> void; onPreviewTask: (task: Task) => void; onOpenTask: (task: Task) => void; - onDeleteTask: (task: Task, opts?: { cascade?: boolean }) => void; - onArchiveTask?: (task: Task, opts?: { cascade?: boolean }) => void; + onDeleteTask: ( + task: Task, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => void; + onArchiveTask?: ( + task: Task, + opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }, + ) => void; isMoving?: boolean; isDeleting?: boolean; isArchiving?: boolean; @@ -226,7 +232,9 @@ function TaskActions({ taskId={task.id} executorType={task.primaryExecutorType} isDeleting={isDeleting} - onConfirm={({ cascade }) => onDeleteTask(task, { cascade })} + onConfirm={({ cascade, discardWorktreeChanges }) => + onDeleteTask(task, { cascade, discardWorktreeChanges }) + } /> - undefined} - /> + + undefined} + /> + , ); } @@ -75,12 +78,14 @@ describe("SwimlaneGraphContent — waiting-for-input variants", () => { } as Task; return render( - undefined} - /> + + undefined} + /> + , ); } diff --git a/apps/web/components/kanban/swimlane-graph2-orphan-display.test.ts b/apps/web/components/kanban/swimlane-graph2-orphan-display.test.ts index 9496913fa5..fce1442c1b 100644 --- a/apps/web/components/kanban/swimlane-graph2-orphan-display.test.ts +++ b/apps/web/components/kanban/swimlane-graph2-orphan-display.test.ts @@ -2,6 +2,7 @@ import { cleanup, fireEvent, render, screen } from "@testing-library/react"; import { createElement } from "react"; import { afterEach, describe, expect, it } from "vitest"; import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; import { TooltipProvider } from "@kandev/ui/tooltip"; import { ORPHAN_STEP_ID } from "./swimlane-kanban-content"; import { getGraph2DisplayState, SwimlaneGraph2Content } from "./swimlane-graph2-content"; @@ -59,19 +60,23 @@ describe("SwimlaneGraph2Content auto-hidden steps", () => { createElement( StateProvider, null, - createElement(TooltipProvider, { - delayDuration: 0, - children: createElement(SwimlaneGraph2Content, { - workflowId: "wf-1", - steps: visibleSteps, - moveTargetSteps, - tasks, - onPreviewTask: () => undefined, - onOpenTask: () => undefined, - onEditTask: () => undefined, - onDeleteTask: () => undefined, + createElement( + ToastProvider, + null, + createElement(TooltipProvider, { + delayDuration: 0, + children: createElement(SwimlaneGraph2Content, { + workflowId: "wf-1", + steps: visibleSteps, + moveTargetSteps, + tasks, + onPreviewTask: () => undefined, + onOpenTask: () => undefined, + onEditTask: () => undefined, + onDeleteTask: () => undefined, + }), }), - }), + ), ), ); } diff --git a/apps/web/components/kanban/swimlane-kanban-content.render-stability.test.tsx b/apps/web/components/kanban/swimlane-kanban-content.render-stability.test.tsx index e0774ff892..f793dda02b 100644 --- a/apps/web/components/kanban/swimlane-kanban-content.render-stability.test.tsx +++ b/apps/web/components/kanban/swimlane-kanban-content.render-stability.test.tsx @@ -42,6 +42,7 @@ vi.mock("./virtualized-column-task-list", () => ({ })); import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; import type { Task } from "@/components/kanban-card"; import type { WorkflowStep } from "@/components/kanban-column"; import { defaultState } from "@/lib/state/default-state"; @@ -73,24 +74,26 @@ const HANDLERS = { function Content({ tasks }: { tasks: Task[] }) { return ( - - - + + + + + ); } diff --git a/apps/web/components/kanban/task-multi-select-toolbar.tsx b/apps/web/components/kanban/task-multi-select-toolbar.tsx index cef76399ee..99cba6779b 100644 --- a/apps/web/components/kanban/task-multi-select-toolbar.tsx +++ b/apps/web/components/kanban/task-multi-select-toolbar.tsx @@ -23,8 +23,8 @@ interface TaskMultiSelectToolbarProps { isProcessing: boolean; canMove?: boolean; onClearSelection: () => void; - onBulkDelete: (opts?: { cascade?: boolean }) => Promise; - onBulkArchive: (opts?: { cascade?: boolean }) => Promise; + onBulkDelete: (opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }) => Promise; + onBulkArchive: (opts?: { cascade?: boolean; discardWorktreeChanges?: boolean }) => Promise; onBulkMove: (targetStepId: string) => Promise; } @@ -95,7 +95,7 @@ function BulkDeleteDialog({ taskIds: string[]; executorTypes: Array; isProcessing: boolean; - onConfirm: (opts: { cascade: boolean }) => void; + onConfirm: (opts: { cascade: boolean; discardWorktreeChanges: boolean }) => void; }) { const { t } = useTranslation(); const [open, setOpen] = useState(false); @@ -201,7 +201,9 @@ export function TaskMultiSelectToolbar({ taskIds={taskIds} executorTypes={executorTypes} isProcessing={isProcessing} - onConfirm={({ cascade }) => onBulkDelete({ cascade })} + onConfirm={({ cascade, discardWorktreeChanges }) => + onBulkDelete({ cascade, discardWorktreeChanges }) + } /> + ); + const dialog = ( + void handleDeleteConfirm(opts)} + /> + ); + + if (action.tooltip) { + return ( + <> + + {button} + {action.tooltip} + + {dialog} + + ); + } + return ( + <> + {button} + {dialog} + + ); +} diff --git a/apps/web/components/task/chat/messages/action-message.test.tsx b/apps/web/components/task/chat/messages/action-message.test.tsx index 3495aa5b20..d36c8aaf41 100644 --- a/apps/web/components/task/chat/messages/action-message.test.tsx +++ b/apps/web/components/task/chat/messages/action-message.test.tsx @@ -12,6 +12,10 @@ import { } from "@/lib/types/http"; import type { AppState } from "@/lib/state/store"; +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: vi.fn() }), +})); + const requestMock = vi.fn().mockResolvedValue({}); const getWebSocketClientMock = vi.fn<() => { request: typeof requestMock } | null>(() => ({ request: requestMock, diff --git a/apps/web/components/task/mobile/session-task-switcher-sheet-hooks.ts b/apps/web/components/task/mobile/session-task-switcher-sheet-hooks.ts index 9f2a5d497f..0ca17a4169 100644 --- a/apps/web/components/task/mobile/session-task-switcher-sheet-hooks.ts +++ b/apps/web/components/task/mobile/session-task-switcher-sheet-hooks.ts @@ -5,7 +5,11 @@ import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { replaceTaskUrl } from "@/lib/links"; import { fetchWorkflowSnapshot, listWorkflows } from "@/lib/api"; import { useWorkspaceSidebarTasks } from "@/hooks/domains/kanban/use-workspace-sidebar-tasks"; -import { useTaskActions, useArchiveAndSwitchTask } from "@/hooks/use-task-actions"; +import { + useTaskActions, + useArchiveAndSwitchTask, + type TaskActionOptions, +} from "@/hooks/use-task-actions"; import { useTaskDetachDialog } from "@/hooks/use-detach-task"; import { useNestTaskByDrag } from "@/hooks/use-nest-task"; import { useTaskRemoval } from "@/hooks/use-task-removal"; @@ -367,7 +371,7 @@ function useSheetDeleteActions( ); const handleDeleteConfirm = useCallback( - async (opts?: { cascade?: boolean }) => { + async (opts?: TaskActionOptions) => { if (!deletingTask || isDeleting) return; const taskId = deletingTask.id; setIsDeleting(true); @@ -422,7 +426,7 @@ export function useSheetArchiveActions( const [archivingTaskId, setArchivingTaskId] = useState(null); const runArchive = useCallback( - async (taskId: string, opts?: { cascade?: boolean }) => { + async (taskId: string, opts?: TaskActionOptions) => { setIsArchiving(true); setArchivingTaskId(taskId); try { @@ -439,7 +443,7 @@ export function useSheetArchiveActions( ); const handleArchiveTask = useCallback( - (taskId: string, opts?: { cascade?: boolean }) => { + (taskId: string, opts?: TaskActionOptions) => { if (opts) { void runArchive(taskId, opts); return; @@ -455,7 +459,7 @@ export function useSheetArchiveActions( ); const handleArchiveConfirm = useCallback( - async (opts?: { cascade?: boolean }) => { + async (opts?: TaskActionOptions) => { if (!archivingTask) return; await runArchive(archivingTask.id, opts); }, diff --git a/apps/web/components/task/mobile/session-task-switcher-sheet.tsx b/apps/web/components/task/mobile/session-task-switcher-sheet.tsx index 9da1718089..da450e3eb7 100644 --- a/apps/web/components/task/mobile/session-task-switcher-sheet.tsx +++ b/apps/web/components/task/mobile/session-task-switcher-sheet.tsx @@ -460,7 +460,9 @@ function TaskSwitcherDialogs({ taskId={actions.deletingTask?.id} executorType={actions.deletingTask?.executorType} isDeleting={actions.isDeleting} - onConfirm={({ cascade }) => actions.handleDeleteConfirm({ cascade })} + onConfirm={({ cascade, discardWorktreeChanges }) => + actions.handleDeleteConfirm({ cascade, discardWorktreeChanges }) + } /> { mockGetSubtaskCount.mockReset(); @@ -92,8 +93,9 @@ describe("TaskDeleteConfirmDialog", () => { await waitFor(() => expect(mockGetSubtaskCount).toHaveBeenCalledWith("task-1")); expect(screen.queryByTestId("delete-cascade-checkbox")).toBeNull(); + fireEvent.click(screen.getByTestId(DISCARD_CHECKBOX_TESTID)); fireEvent.click(screen.getByTestId("confirm")); - expect(onConfirm).toHaveBeenCalledWith({ cascade: false }); + expect(onConfirm).toHaveBeenCalledWith({ cascade: false, discardWorktreeChanges: true }); }); it("shows the cascade checkbox when the task has subtasks; defaults to unchecked", async () => { @@ -112,8 +114,9 @@ describe("TaskDeleteConfirmDialog", () => { await screen.findByTestId("delete-cascade-checkbox"); expect(screen.getByText(/Also delete 3 subtasks/i)).toBeTruthy(); + fireEvent.click(screen.getByTestId(DISCARD_CHECKBOX_TESTID)); fireEvent.click(screen.getByTestId("confirm")); - expect(onConfirm).toHaveBeenCalledWith({ cascade: false }); + expect(onConfirm).toHaveBeenCalledWith({ cascade: false, discardWorktreeChanges: true }); }); it("propagates cascade=true when the user ticks the checkbox", async () => { @@ -130,9 +133,10 @@ describe("TaskDeleteConfirmDialog", () => { />, ); const checkbox = await screen.findByTestId("delete-cascade-checkbox"); + fireEvent.click(screen.getByTestId(DISCARD_CHECKBOX_TESTID)); fireEvent.click(checkbox); fireEvent.click(screen.getByTestId("confirm")); - expect(onConfirm).toHaveBeenCalledWith({ cascade: true }); + expect(onConfirm).toHaveBeenCalledWith({ cascade: true, discardWorktreeChanges: true }); }); it("sums subtask counts across taskIds for bulk delete", async () => { @@ -153,6 +157,51 @@ describe("TaskDeleteConfirmDialog", () => { }); }); +describe("TaskDeleteConfirmDialog discard consent", () => { + it("requires explicit discard consent for worktree cleanup", async () => { + mockGetSubtaskCount.mockResolvedValue({ count: 0 }); + const onConfirm = vi.fn(); + renderDialog( + {}} + taskTitle="My task" + taskId="task-1" + executorType="worktree" + onConfirm={onConfirm} + confirmTestId="confirm" + />, + ); + + const discard = await screen.findByTestId(DISCARD_CHECKBOX_TESTID); + const confirm = screen.getByTestId("confirm") as HTMLButtonElement; + expect(confirm.disabled).toBe(true); + fireEvent.click(confirm); + expect(onConfirm).not.toHaveBeenCalled(); + + fireEvent.click(discard); + expect(confirm.disabled).toBe(false); + fireEvent.click(confirm); + expect(onConfirm).toHaveBeenCalledWith({ cascade: false, discardWorktreeChanges: true }); + }); + + it("shows discard consent when a cascade can include child worktrees", async () => { + mockGetSubtaskCount.mockResolvedValue({ count: 1 }); + renderDialog( + {}} + taskTitle="Parent task" + taskId="task-1" + executorType="local" + onConfirm={() => {}} + />, + ); + + expect(await screen.findByTestId(DISCARD_CHECKBOX_TESTID)).toBeTruthy(); + }); +}); + describe("TaskDeleteConfirmDialog executor cleanup copy", () => { it("states the named delete outcome directly and separates cleanup effects", () => { mockGetSubtaskCount.mockResolvedValue({ count: 0 }); @@ -236,6 +285,7 @@ describe("TaskDeleteConfirmDialog executor cleanup copy", () => { />, ); expect(screen.getByText(/Any running agent sessions will be stopped/i)).toBeTruthy(); + expect(screen.getByTestId(DISCARD_CHECKBOX_TESTID)).toBeTruthy(); }); }); diff --git a/apps/web/components/task/task-delete-confirm-dialog.tsx b/apps/web/components/task/task-delete-confirm-dialog.tsx index d0d3a51025..fb4cf6d236 100644 --- a/apps/web/components/task/task-delete-confirm-dialog.tsx +++ b/apps/web/components/task/task-delete-confirm-dialog.tsx @@ -15,7 +15,11 @@ import { import { Checkbox } from "@kandev/ui/checkbox"; import { useSubtaskCount } from "@/hooks/use-subtask-count"; import { useTaskInFlight } from "@/hooks/use-task-in-flight"; -import { getCleanupSummary, getBulkCleanupSummary } from "./task-cleanup-summary"; +import { + getCleanupSummary, + getBulkCleanupSummary, + hasWorktreeExecutor, +} from "./task-cleanup-summary"; import { TaskCleanupConsequences } from "./task-cleanup-consequences"; import { StillWorkingWarning } from "./task-still-working-warning"; import { @@ -42,10 +46,199 @@ type TaskDeleteConfirmDialogProps = { executorType?: string | null; /** Executor types of the tasks being deleted (bulk). */ executorTypes?: Array; - onConfirm: (opts: { cascade: boolean }) => void; + /** Require discard consent when the task's retained worktree state is unknown. */ + requireDiscardConsent?: boolean; + onConfirm: (opts: { cascade: boolean; discardWorktreeChanges: boolean }) => void; confirmTestId?: string; }; +type DiscardWorktreeChangesOptionProps = { + enabled: boolean; + checked: boolean; + disabled?: boolean; + onCheckedChange: (checked: boolean) => void; +}; + +function DiscardWorktreeChangesOption({ + enabled, + checked, + disabled, + onCheckedChange, +}: DiscardWorktreeChangesOptionProps) { + const { t } = useTranslation(); + if (!enabled) return null; + return ( + + ); +} + +type CascadeOptionProps = { + count: number; + checked: boolean; + disabled?: boolean; + onCheckedChange: (checked: boolean) => void; +}; + +function CascadeOption({ count, checked, disabled, onCheckedChange }: CascadeOptionProps) { + const { t } = useTranslation(); + return ( + + ); +} + +type TaskDeleteActionProps = { + isDeleting?: boolean; + disabled: boolean; + confirmTestId?: string; + cascade: boolean; + discardWorktreeChanges: boolean; + onConfirm: (opts: { cascade: boolean; discardWorktreeChanges: boolean }) => void; + onClose: () => void; +}; + +function TaskDeleteAction({ + isDeleting, + disabled, + confirmTestId, + cascade, + discardWorktreeChanges, + onConfirm, + onClose, +}: TaskDeleteActionProps) { + const { t } = useTranslation(); + return ( + { + if (isDeleting) return; + onConfirm({ cascade, discardWorktreeChanges }); + onClose(); + }} + > + {isDeleting ? : null} + {t("task:delete")} + + ); +} + +function useTaskDeleteDialogState(onOpenChange: (open: boolean) => void) { + const [cascade, setCascade] = useState(false); + const [discardWorktreeChanges, setDiscardWorktreeChanges] = useState(false); + const handleOpenChange = (next: boolean) => { + if (!next) { + setCascade(false); + setDiscardWorktreeChanges(false); + } + onOpenChange(next); + }; + return { + cascade, + setCascade, + discardWorktreeChanges, + setDiscardWorktreeChanges, + handleOpenChange, + }; +} + +function hasPotentialWorktree( + isBulkOperation: boolean | undefined, + executorType: string | null | undefined, + executorTypes: Array | undefined, +) { + if (isBulkOperation) { + return ( + executorTypes == null || + executorTypes.some((type) => type == null || hasWorktreeExecutor(type)) + ); + } + return executorType == null || hasWorktreeExecutor(executorType); +} + +function shouldRequireDiscardConsent( + explicit: boolean, + hasWorktree: boolean, + subtaskCount: number, +) { + return explicit || hasWorktree || subtaskCount > 0; +} + +function TaskDeleteDialogOptions({ + isInFlight, + isBulkOperation, + safeCount, + storeInFlight, + requiresDiscardConsent, + discardWorktreeChanges, + setDiscardWorktreeChanges, + isDeleting, + subtaskCount, + cascade, + setCascade, +}: { + isInFlight?: boolean; + isBulkOperation?: boolean; + safeCount: number; + storeInFlight: boolean; + requiresDiscardConsent: boolean; + discardWorktreeChanges: boolean; + setDiscardWorktreeChanges: (checked: boolean) => void; + isDeleting?: boolean; + subtaskCount: number; + cascade: boolean; + setCascade: (checked: boolean) => void; +}) { + return ( + <> + {(isInFlight || storeInFlight) && ( + + )} + + {subtaskCount > 0 && ( + + )} + + ); +} + export function TaskDeleteConfirmDialog({ open, onOpenChange, @@ -58,6 +251,7 @@ export function TaskDeleteConfirmDialog({ isInFlight, executorType, executorTypes, + requireDiscardConsent = false, onConfirm, confirmTestId, }: TaskDeleteConfirmDialogProps) { @@ -73,14 +267,20 @@ export function TaskDeleteConfirmDialog({ ? getBulkCleanupSummary(executorTypes ?? []) : getCleanupSummary(executorType); - const [cascade, setCascade] = useState(false); + const { + cascade, + setCascade, + discardWorktreeChanges, + setDiscardWorktreeChanges, + handleOpenChange, + } = useTaskDeleteDialogState(onOpenChange); const subtaskCount = useSubtaskCount(open, taskId, taskIds); const storeInFlight = useTaskInFlight(taskId, taskIds, open); - - const handleOpenChange = (next: boolean) => { - if (!next) setCascade(false); - onOpenChange(next); - }; + const requiresDiscardConsent = shouldRequireDiscardConsent( + requireDiscardConsent, + hasPotentialWorktree(isBulkOperation, executorType, executorTypes), + subtaskCount, + ); return ( @@ -95,44 +295,33 @@ export function TaskDeleteConfirmDialog({ - {(isInFlight || storeInFlight) && ( - - )} - {subtaskCount > 0 && ( - - )} + {t("common:cancel")} - { - if (isDeleting) return; - onConfirm({ cascade }); - handleOpenChange(false); - }} - > - {isDeleting ? : null} - {t("task:delete")} - + handleOpenChange(false)} + /> diff --git a/apps/web/components/task/task-session-sidebar-dialogs.tsx b/apps/web/components/task/task-session-sidebar-dialogs.tsx index ff9f5a88f1..4a53aabf17 100644 --- a/apps/web/components/task/task-session-sidebar-dialogs.tsx +++ b/apps/web/components/task/task-session-sidebar-dialogs.tsx @@ -40,7 +40,10 @@ export type SidebarDialogsActions = { deletingTask: Target; setDeletingTask: (next: Target) => void; isDeleting: boolean; - handleDeleteConfirm: (opts: { cascade: boolean }) => Promise | void; + handleDeleteConfirm: (opts: { + cascade: boolean; + discardWorktreeChanges: boolean; + }) => Promise | void; detachingTask: DetachTarget; setDetachingTask: (next: DetachTarget) => void; detachingTaskId: string | null; diff --git a/apps/web/components/task/task-session-sidebar-selection.tsx b/apps/web/components/task/task-session-sidebar-selection.tsx index d74fb90685..3b1de37a76 100644 --- a/apps/web/components/task/task-session-sidebar-selection.tsx +++ b/apps/web/components/task/task-session-sidebar-selection.tsx @@ -11,6 +11,7 @@ import { } from "@/lib/sidebar/apply-view"; import { TaskArchiveConfirmDialog } from "@/components/task/task-archive-confirm-dialog"; import { TaskDeleteConfirmDialog } from "@/components/task/task-delete-confirm-dialog"; +import type { TaskActionOptions } from "@/hooks/use-task-actions"; type BulkConfirmState = { ids: string[]; executorTypes: Array }; @@ -22,7 +23,7 @@ type BulkConfirmState = { ids: string[]; executorTypes: Array, - run: (ids: string[], opts?: { cascade?: boolean }) => Promise, + run: (ids: string[], opts?: TaskActionOptions) => Promise, ) { const [state, setState] = useState(null); @@ -35,10 +36,10 @@ export function useBulkConfirmDialog( ); const confirm = useCallback( - async ({ cascade }: { cascade: boolean }) => { + async ({ cascade, discardWorktreeChanges }: TaskActionOptions & { cascade: boolean }) => { if (!state) return; try { - await run(state.ids, { cascade }); + await run(state.ids, { cascade, discardWorktreeChanges }); } catch (error) { console.error("Bulk action failed:", error); } finally { diff --git a/apps/web/components/task/task-session-sidebar.tsx b/apps/web/components/task/task-session-sidebar.tsx index c97bc7a63d..1fc7d3b5ac 100644 --- a/apps/web/components/task/task-session-sidebar.tsx +++ b/apps/web/components/task/task-session-sidebar.tsx @@ -15,7 +15,11 @@ import { PanelRoot } from "./panel-primitives"; import { TaskSidebarScrollArea } from "./task-sidebar-scroll-area"; import { useAppStore, useAppStoreApi } from "@/components/state-provider"; import { useWorkspaceSidebarTasks } from "@/hooks/domains/kanban/use-workspace-sidebar-tasks"; -import { useTaskActions, useArchiveAndSwitchTask } from "@/hooks/use-task-actions"; +import { + useTaskActions, + useArchiveAndSwitchTask, + type TaskActionOptions, +} from "@/hooks/use-task-actions"; import { useTaskDetachDialog } from "@/hooks/use-detach-task"; import { useNestTaskByDrag } from "@/hooks/use-nest-task"; import { useSidebarSelection, SidebarBulkDialogs } from "./task-session-sidebar-selection"; @@ -146,7 +150,7 @@ function useArchiveActions(store: StoreApi) { const [isArchiving, setIsArchiving] = useState(false); const runArchive = useCallback( - async (taskId: string, opts: { cascade?: boolean }) => { + async (taskId: string, opts: TaskActionOptions) => { setIsArchiving(true); setArchivingTaskId(taskId); try { @@ -163,7 +167,7 @@ function useArchiveActions(store: StoreApi) { ); const handleArchiveTask = useCallback( - (taskId: string, opts?: { cascade?: boolean }) => { + (taskId: string, opts?: TaskActionOptions) => { if (opts) { void runArchive(taskId, opts); return; @@ -224,7 +228,7 @@ function useDeleteActions( ); const handleDeleteConfirm = useCallback( - async (opts: { cascade: boolean }) => { + async (opts: TaskActionOptions & { cascade: boolean; discardWorktreeChanges: boolean }) => { if (!deletingTask || isDeleting) return; const taskId = deletingTask.id; setIsDeleting(true); diff --git a/apps/web/components/task/task-top-bar.test.tsx b/apps/web/components/task/task-top-bar.test.tsx index 0a65668d1b..39e0b4294f 100644 --- a/apps/web/components/task/task-top-bar.test.tsx +++ b/apps/web/components/task/task-top-bar.test.tsx @@ -1,6 +1,7 @@ import { cleanup, render, screen, within } from "@testing-library/react"; import { afterEach, describe, expect, it, vi } from "vitest"; import { StateProvider } from "@/components/state-provider"; +import { ToastProvider } from "@/components/toast-provider"; import { TaskTopBar } from "./task-top-bar"; afterEach(() => cleanup()); @@ -195,5 +196,9 @@ describe("TaskTopBar archived task controls", () => { }); function renderTopBar(ui: React.ReactNode) { - return render({ui}); + return render( + + {ui} + , + ); } diff --git a/apps/web/e2e/pages/session-page.ts b/apps/web/e2e/pages/session-page.ts index 2e70146e56..8090e899ce 100644 --- a/apps/web/e2e/pages/session-page.ts +++ b/apps/web/e2e/pages/session-page.ts @@ -721,9 +721,12 @@ export class SessionPage { */ async deleteTaskInSidebar(title: string): Promise { await this.openSidebarMenuAndClick(title, "Delete"); - const confirmButton = this.page - .getByRole("alertdialog") - .getByRole("button", { name: "Delete" }); + const dialog = this.page.getByRole("alertdialog"); + const discard = dialog.getByTestId("delete-discard-worktree-checkbox"); + if (await discard.isVisible()) { + await discard.click(); + } + const confirmButton = dialog.getByRole("button", { name: "Delete" }); await confirmButton.click(); } diff --git a/apps/web/e2e/tests/kanban/card-menu-delete-archive.spec.ts b/apps/web/e2e/tests/kanban/card-menu-delete-archive.spec.ts index 3f2876c5f2..02ef3792a7 100644 --- a/apps/web/e2e/tests/kanban/card-menu-delete-archive.spec.ts +++ b/apps/web/e2e/tests/kanban/card-menu-delete-archive.spec.ts @@ -137,6 +137,7 @@ test.describe("Kanban card actions menu — delete/archive in All Workflows view const dialog = testPage.getByRole("alertdialog"); await expect(dialog).toBeVisible(); + await dialog.getByTestId("delete-discard-worktree-checkbox").click(); await dialog.getByRole("button", { name: "Delete" }).click(); await expect(kanban.taskCardByTitle("All-Wf Delete Task")).not.toBeVisible({ diff --git a/apps/web/e2e/tests/kanban/cascade-subtasks-toggle.spec.ts b/apps/web/e2e/tests/kanban/cascade-subtasks-toggle.spec.ts index 79a443ab35..59e5f58b85 100644 --- a/apps/web/e2e/tests/kanban/cascade-subtasks-toggle.spec.ts +++ b/apps/web/e2e/tests/kanban/cascade-subtasks-toggle.spec.ts @@ -160,6 +160,12 @@ test.describe("Kanban card delete — cascade subtasks toggle", () => { await expect(dialog).toContainText("Also delete 1 subtask"); await expect(cascade).not.toBeChecked(); + const discard = testPage.getByTestId("delete-discard-worktree-checkbox"); + await expect(discard).toBeVisible(); + await expect(dialog.getByRole("button", { name: "Delete" })).toBeDisabled(); + await discard.click(); + await expect(dialog.getByRole("button", { name: "Delete" })).toBeEnabled(); + await dialog.getByRole("button", { name: "Delete" }).click(); // Parent is gone, child survives (now reparented to root with no @@ -199,6 +205,10 @@ test.describe("Kanban card delete — cascade subtasks toggle", () => { const dialog = testPage.getByRole("alertdialog"); const cascade = testPage.getByTestId("delete-cascade-checkbox"); await expect(cascade).toBeVisible(); + const discard = testPage.getByTestId("delete-discard-worktree-checkbox"); + await expect(discard).toBeVisible(); + await discard.click(); + await expect(discard).toBeChecked(); await cascade.click(); await expect(cascade).toBeChecked(); diff --git a/apps/web/e2e/tests/kanban/dialog-enter-confirms.spec.ts b/apps/web/e2e/tests/kanban/dialog-enter-confirms.spec.ts index 37565344df..4bb9f51b43 100644 --- a/apps/web/e2e/tests/kanban/dialog-enter-confirms.spec.ts +++ b/apps/web/e2e/tests/kanban/dialog-enter-confirms.spec.ts @@ -29,6 +29,8 @@ test.describe("Dialog Enter key — executes the semantic action", () => { const dialog = testPage.getByRole("alertdialog"); await expect(dialog).toBeVisible(); await expect(dialog).toContainText("Enter Delete Task"); + await dialog.getByTestId("delete-discard-worktree-checkbox").click(); + await dialog.getByRole("button", { name: "Delete" }).focus(); // Do NOT click Delete — Enter alone must trigger the destructive action. await testPage.keyboard.press("Enter"); diff --git a/apps/web/e2e/tests/kanban/pipeline-view.spec.ts b/apps/web/e2e/tests/kanban/pipeline-view.spec.ts index d604cc459f..35a99d62a4 100644 --- a/apps/web/e2e/tests/kanban/pipeline-view.spec.ts +++ b/apps/web/e2e/tests/kanban/pipeline-view.spec.ts @@ -103,6 +103,7 @@ test.describe("Pipeline view", () => { await kanban.bulkDeleteButton.click(); await expect(kanban.bulkDeleteConfirm).toBeVisible(); + await testPage.getByTestId("delete-discard-worktree-checkbox").click(); await kanban.bulkDeleteConfirm.click(); await expect(kanban.pipelineTask(t1.id)).toHaveCount(0, { timeout: 10000 }); diff --git a/apps/web/e2e/tests/kanban/task-multi-select.spec.ts b/apps/web/e2e/tests/kanban/task-multi-select.spec.ts index 23c3a7cf6c..888b5ff7d0 100644 --- a/apps/web/e2e/tests/kanban/task-multi-select.spec.ts +++ b/apps/web/e2e/tests/kanban/task-multi-select.spec.ts @@ -86,6 +86,7 @@ test.describe("Multi-select bulk actions", () => { await kanban.bulkDeleteButton.click(); await expect(kanban.bulkDeleteConfirm).toBeVisible(); + await testPage.getByTestId("delete-discard-worktree-checkbox").click(); await kanban.bulkDeleteConfirm.click(); await expect(kanban.taskCard(t1.id)).not.toBeVisible({ timeout: 10000 }); @@ -177,6 +178,7 @@ test.describe("Multi-select bulk actions", () => { await kanban.bulkDeleteButton.click(); await expect(kanban.bulkDeleteConfirm).toBeVisible(); + await testPage.getByTestId("delete-discard-worktree-checkbox").click(); await kanban.bulkDeleteConfirm.click(); await expect(kanban.taskCard(t1.id)).not.toBeVisible({ timeout: 10000 }); diff --git a/apps/web/e2e/tests/task/confirmation-text-hierarchy.spec.ts b/apps/web/e2e/tests/task/confirmation-text-hierarchy.spec.ts index a91c85d07b..3c2c320788 100644 --- a/apps/web/e2e/tests/task/confirmation-text-hierarchy.spec.ts +++ b/apps/web/e2e/tests/task/confirmation-text-hierarchy.spec.ts @@ -151,7 +151,9 @@ test.describe("Desktop confirmation text hierarchy", () => { expect((await apiClient.getTask(task.task_id)).id).toBe(task.task_id); await session.openSidebarMenuAndClick(LONG_TASK_TITLE, "Delete"); - await testPage.getByRole("alertdialog").locator('[data-slot="alert-dialog-action"]').click(); + const confirmedDeleteDialog = testPage.getByRole("alertdialog"); + await confirmedDeleteDialog.getByTestId("delete-discard-worktree-checkbox").click(); + await confirmedDeleteDialog.locator('[data-slot="alert-dialog-action"]').click(); await expect .poll( async () => (await apiClient.rawRequest("GET", `/api/v1/tasks/${task.task_id}`)).status, diff --git a/apps/web/e2e/tests/task/mobile-confirmation-text-hierarchy.spec.ts b/apps/web/e2e/tests/task/mobile-confirmation-text-hierarchy.spec.ts index 80ec8e148d..e594b4d221 100644 --- a/apps/web/e2e/tests/task/mobile-confirmation-text-hierarchy.spec.ts +++ b/apps/web/e2e/tests/task/mobile-confirmation-text-hierarchy.spec.ts @@ -1,3 +1,5 @@ +import fs from "node:fs"; +import path from "node:path"; import type { Locator, Page } from "@playwright/test"; import { expect, test } from "../../fixtures/test-base"; import { @@ -149,17 +151,41 @@ test.describe("Mobile confirmation text hierarchy", () => { }) => { test.setTimeout(120_000); await testPage.setViewportSize({ width: 320, height: 400 }); - const task = await apiClient.seedTask(seedData.workspaceId, LONG_TASK_TITLE, { - workflow_id: seedData.workflowId, - workflow_step_id: seedData.startStepId, - }); + const task = await apiClient.createTaskWithAgent( + seedData.workspaceId, + LONG_TASK_TITLE, + seedData.agentProfileId, + { + description: "/e2e:simple-message", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + executor_profile_id: seedData.worktreeExecutorProfileId, + }, + ); + await expect + .poll(async () => (await apiClient.getTaskEnvironment(task.id))?.status ?? null, { + timeout: 60_000, + message: "the dirty-worktree task environment did not become ready", + }) + .toBe("ready"); + const environment = await apiClient.getTaskEnvironment(task.id); + const dirtyWorktreePath = + environment?.repos?.[0]?.worktree_path ?? environment?.worktree_path ?? ""; + if (!dirtyWorktreePath) { + throw new Error("the dirty-worktree task did not expose a worktree path"); + } + const dirtyFilePath = path.join(dirtyWorktreePath, "mobile-delete-dirty.txt"); + fs.writeFileSync(dirtyFilePath, "preserve this local change\n", "utf8"); + expect(fs.existsSync(dirtyFilePath)).toBe(true); + await apiClient.seedTask(seedData.workspaceId, "Mobile cleanup confirmation child", { workflow_id: seedData.workflowId, workflow_step_id: seedData.startStepId, - parent_id: task.task_id, + parent_id: task.id, }); - await testPage.goto(`/t/${task.task_id}`); + await testPage.goto(`/t/${task.id}`); await expect(testPage.getByTestId("mobile-task-layout")).toBeVisible(); await enablePseudoLocale(testPage); @@ -171,19 +197,33 @@ test.describe("Mobile confirmation text hierarchy", () => { await expect(drawerTitle).toHaveCSS("text-wrap", "balance"); await assertPhoneDeleteDialog(dialog, width); + const discard = dialog.getByTestId("delete-discard-worktree-checkbox"); + const deleteAction = dialog.locator('[data-slot="alert-dialog-action"]'); + await expect(discard).toBeVisible(); + await expect(deleteAction).toBeDisabled(); + await dialog.locator('[data-slot="alert-dialog-cancel"]').tap(); await expect(dialog).toBeHidden(); - expect((await apiClient.getTask(task.task_id)).id).toBe(task.task_id); + expect((await apiClient.getTask(task.id)).id).toBe(task.id); + expect(fs.existsSync(dirtyFilePath)).toBe(true); } const { dialog } = await openDeleteDialog(testPage, LONG_TASK_TITLE); + await dialog.getByTestId("delete-discard-worktree-checkbox").tap(); + await expect(dialog.locator('[data-slot="alert-dialog-action"]')).toBeEnabled(); await dialog.locator('[data-slot="alert-dialog-action"]').tap(); await expect - .poll( - async () => (await apiClient.rawRequest("GET", `/api/v1/tasks/${task.task_id}`)).status, - { timeout: 15_000, message: "the mobile Delete action should remove the task" }, - ) + .poll(async () => (await apiClient.rawRequest("GET", `/api/v1/tasks/${task.id}`)).status, { + timeout: 15_000, + message: "the mobile Delete action should remove the task", + }) .toBe(404); + await expect + .poll(() => fs.existsSync(dirtyFilePath), { + timeout: 15_000, + message: "consented deletion should remove the dirty worktree", + }) + .toBe(false); }); // @covers AC-UI-SURFACE-TEXT-HIERARCHY-001.1, AC-UI-SURFACE-TEXT-HIERARCHY-001.3 diff --git a/apps/web/e2e/tests/task/sidebar-delete-confirm.spec.ts b/apps/web/e2e/tests/task/sidebar-delete-confirm.spec.ts index 0ae7ad94dd..9e3fde339e 100644 --- a/apps/web/e2e/tests/task/sidebar-delete-confirm.spec.ts +++ b/apps/web/e2e/tests/task/sidebar-delete-confirm.spec.ts @@ -66,8 +66,10 @@ test.describe("Task sidebar — delete shows confirmation", () => { // Reopen and confirm — now it should disappear. await session.openSidebarMenuAndClick("Sidebar Delete Confirm Task", "Delete"); - await expect(testPage.getByRole("alertdialog")).toBeVisible(); - await testPage.getByRole("alertdialog").getByRole("button", { name: "Delete" }).click(); + const confirmedDeleteDialog = testPage.getByRole("alertdialog"); + await expect(confirmedDeleteDialog).toBeVisible(); + await confirmedDeleteDialog.getByTestId("delete-discard-worktree-checkbox").click(); + await confirmedDeleteDialog.getByRole("button", { name: "Delete" }).click(); await expect(session.taskInSidebar("Sidebar Delete Confirm Task")).not.toBeVisible({ timeout: 15_000, diff --git a/apps/web/e2e/tests/task/sidebar-multi-select.spec.ts b/apps/web/e2e/tests/task/sidebar-multi-select.spec.ts index cfc24d1eab..ca63f987ba 100644 --- a/apps/web/e2e/tests/task/sidebar-multi-select.spec.ts +++ b/apps/web/e2e/tests/task/sidebar-multi-select.spec.ts @@ -212,6 +212,7 @@ test.describe("Sidebar selection-aware context menu", () => { const confirm = sidebar.bulkDeleteConfirm(); await expect(confirm).toBeVisible({ timeout: 5_000 }); + await testPage.getByTestId("delete-discard-worktree-checkbox").click(); await confirm.click(); await expect(sidebar.row(a.id)).not.toBeVisible({ timeout: 10_000 }); diff --git a/apps/web/hooks/use-sidebar-multi-select.ts b/apps/web/hooks/use-sidebar-multi-select.ts index afb4a728b5..1c0aad436d 100644 --- a/apps/web/hooks/use-sidebar-multi-select.ts +++ b/apps/web/hooks/use-sidebar-multi-select.ts @@ -6,14 +6,18 @@ import { multiSelectReducer, useTaskMultiSelectStore, } from "./use-task-multi-select"; -import { useTaskActions, useArchiveAndSwitchTask } from "./use-task-actions"; +import { + useTaskActions, + useArchiveAndSwitchTask, + type TaskActionOptions, +} from "./use-task-actions"; import { useTaskRemoval } from "./use-task-removal"; import { useTaskWorkflowMove } from "./use-task-workflow-move"; import { useToast } from "@/components/toast-provider"; import { useAppStoreApi } from "@/components/state-provider"; import { useTranslation } from "react-i18next"; -type BulkOpts = { cascade?: boolean }; +type BulkOpts = TaskActionOptions; /** * Bulk archive/delete/move that act on an explicit id list. Archive and delete diff --git a/apps/web/hooks/use-task-actions.test.ts b/apps/web/hooks/use-task-actions.test.ts index b46e26a743..f6c2b705d4 100644 --- a/apps/web/hooks/use-task-actions.test.ts +++ b/apps/web/hooks/use-task-actions.test.ts @@ -2,12 +2,14 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook, act } from "@testing-library/react"; const archiveTaskMock = vi.fn(); +const deleteTaskMock = vi.fn(); const removeTaskFromBoardMock = vi.fn(); const getStateMock = vi.fn(); const storeMock = { getState: getStateMock }; const replaceTaskUrlMock = vi.fn(); const setActiveSessionMock = vi.fn(); const setActiveTaskMock = vi.fn(); +const toastMock = vi.fn(); let storeState: { tasks: { activeTaskId: string | null; activeSessionId: string | null }; setActiveSession: (...args: unknown[]) => void; @@ -16,7 +18,7 @@ let storeState: { vi.mock("@/lib/api", () => ({ archiveTask: (...args: unknown[]) => archiveTaskMock(...args), - deleteTask: vi.fn(), + deleteTask: (...args: unknown[]) => deleteTaskMock(...args), moveTask: vi.fn(), updateTask: vi.fn(), })); @@ -34,8 +36,11 @@ vi.mock("@/hooks/use-task-removal", () => ({ removeTaskFromBoard: (...args: unknown[]) => removeTaskFromBoardMock(...args), }), })); +vi.mock("@/components/toast-provider", () => ({ + useToast: () => ({ toast: toastMock }), +})); -import { useArchiveAndSwitchTask } from "./use-task-actions"; +import { useArchiveAndSwitchTask, useTaskActions } from "./use-task-actions"; function deferred() { let resolve!: (value: T | PromiseLike) => void; @@ -145,3 +150,23 @@ describe("useArchiveAndSwitchTask", () => { expect(replaceTaskUrlMock).not.toHaveBeenCalled(); }); }); + +describe("useTaskActions", () => { + it("shows localized retry guidance for a dirty-worktree delete conflict", async () => { + const { ApiError } = await import("@/lib/api/client"); + deleteTaskMock.mockRejectedValueOnce( + new ApiError("task worktree contains local changes", 409, { + error_code: "task_delete_dirty_worktree", + }), + ); + const { result } = renderHook(() => useTaskActions()); + + await expect(result.current.deleteTaskById("task-1")).rejects.toMatchObject({ status: 409 }); + expect(toastMock).toHaveBeenCalledWith({ + title: "Task deletion needs your confirmation", + description: + "The task stays visible. Review the local changes, then select discard and try again.", + variant: "error", + }); + }); +}); diff --git a/apps/web/hooks/use-task-actions.ts b/apps/web/hooks/use-task-actions.ts index 50a8b92017..d4c29b62a5 100644 --- a/apps/web/hooks/use-task-actions.ts +++ b/apps/web/hooks/use-task-actions.ts @@ -1,21 +1,47 @@ import { useCallback } from "react"; import { archiveTask, deleteTask, moveTask, updateTask } from "@/lib/api"; +import type { DeleteTaskParams } from "@/lib/api/domains/kanban-api"; +import { isTaskDeleteDirtyWorktreeError } from "@/lib/api/task-delete-errors"; import { replaceTaskUrl } from "@/lib/links"; import { useAppStoreApi } from "@/components/state-provider"; +import { useToast } from "@/components/toast-provider"; import { useTaskRemoval } from "@/hooks/use-task-removal"; +import { useTranslation } from "react-i18next"; type MovePayload = { workflow_id: string; workflow_step_id: string; position: number }; +export type TaskActionOptions = { + cascade?: boolean; + discardWorktreeChanges?: boolean; +}; + export function useTaskActions() { + const { toast } = useToast(); + const { t } = useTranslation(); + const moveTaskById = useCallback(async (taskId: string, payload: MovePayload) => { return moveTask(taskId, payload); }, []); - const deleteTaskById = useCallback(async (taskId: string, opts?: { cascade?: boolean }) => { - return deleteTask(taskId, opts); - }, []); + const deleteTaskById = useCallback( + async (taskId: string, opts?: DeleteTaskParams) => { + try { + return await deleteTask(taskId, opts); + } catch (error) { + if (isTaskDeleteDirtyWorktreeError(error)) { + toast({ + title: t("task:deleteDirtyWorktreeTitle"), + description: t("task:deleteDirtyWorktreeDescription"), + variant: "error", + }); + } + throw error; + } + }, + [t, toast], + ); - const archiveTaskById = useCallback(async (taskId: string, opts?: { cascade?: boolean }) => { + const archiveTaskById = useCallback(async (taskId: string, opts?: TaskActionOptions) => { return archiveTask(taskId, opts); }, []); @@ -39,7 +65,7 @@ export function useArchiveAndSwitchTask(opts?: { useLayoutSwitch?: boolean }) { }); return useCallback( - async (taskId: string, opts?: { cascade?: boolean }) => { + async (taskId: string, opts?: TaskActionOptions) => { const { activeTaskId: wasActiveTaskId, activeSessionId: wasActiveSessionId } = store.getState().tasks; const removalOptions = opts?.cascade ? { excludeTaskTree: true } : {}; diff --git a/apps/web/hooks/use-task-crud.ts b/apps/web/hooks/use-task-crud.ts index 6377b4637e..17fe08526e 100644 --- a/apps/web/hooks/use-task-crud.ts +++ b/apps/web/hooks/use-task-crud.ts @@ -2,7 +2,7 @@ import { useCallback, useState } from "react"; import { useAppStoreApi } from "@/components/state-provider"; -import { useTaskActions } from "@/hooks/use-task-actions"; +import { useTaskActions, type TaskActionOptions } from "@/hooks/use-task-actions"; import type { Task } from "@/components/kanban-card"; import type { KanbanState } from "@/lib/state/slices"; @@ -31,7 +31,7 @@ export function useTaskCRUD() { }, []); const handleDelete = useCallback( - async (task: Task, opts?: { cascade?: boolean }) => { + async (task: Task, opts?: TaskActionOptions) => { setDeletingTaskId(task.id); try { await deleteTaskById(task.id, opts); @@ -53,7 +53,7 @@ export function useTaskCRUD() { ); const handleArchive = useCallback( - async (task: Task, opts?: { cascade?: boolean }) => { + async (task: Task, opts?: TaskActionOptions) => { setArchivingTaskId(task.id); try { await archiveTaskById(task.id, opts); diff --git a/apps/web/hooks/use-task-multi-select.ts b/apps/web/hooks/use-task-multi-select.ts index b23ed47c6b..c94fcee595 100644 --- a/apps/web/hooks/use-task-multi-select.ts +++ b/apps/web/hooks/use-task-multi-select.ts @@ -1,7 +1,7 @@ "use client"; import { useCallback, useEffect, useLayoutEffect, useReducer, useRef, type RefObject } from "react"; -import { useTaskActions } from "@/hooks/use-task-actions"; +import { useTaskActions, type TaskActionOptions } from "@/hooks/use-task-actions"; import { useAppStoreApi } from "@/components/state-provider"; import type { KanbanState } from "@/lib/state/slices"; import { sortIdsByCreatedDesc } from "@/lib/kanban/task-order"; @@ -129,9 +129,9 @@ function useBulkOperations({ }) { const runBulk = useCallback( async ( - per: (id: string, opts?: { cascade?: boolean }) => Promise, + per: (id: string, opts?: TaskActionOptions) => Promise, setBusy: (v: boolean) => void, - opts?: { cascade?: boolean }, + opts?: TaskActionOptions, ) => { const ids = selectedIdsRef.current; if (!ids || ids.size === 0) return; @@ -152,12 +152,12 @@ function useBulkOperations({ ); const bulkDelete = useCallback( - (opts?: { cascade?: boolean }) => runBulk(deleteTaskById, setIsDeleting, opts), + (opts?: TaskActionOptions) => runBulk(deleteTaskById, setIsDeleting, opts), [runBulk, deleteTaskById, setIsDeleting], ); const bulkArchive = useCallback( - (opts?: { cascade?: boolean }) => runBulk(archiveTaskById, setIsArchiving, opts), + (opts?: TaskActionOptions) => runBulk(archiveTaskById, setIsArchiving, opts), [runBulk, archiveTaskById, setIsArchiving], ); diff --git a/apps/web/lib/api/domains/kanban-api.test.ts b/apps/web/lib/api/domains/kanban-api.test.ts index 319c6d0b34..ac26ca9780 100644 --- a/apps/web/lib/api/domains/kanban-api.test.ts +++ b/apps/web/lib/api/domains/kanban-api.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { attachTaskWorkspaceSources, detachTask, + deleteTask, listTasksByWorkspace, updateTask, updateTaskPortForwarding, @@ -17,6 +18,25 @@ beforeEach(() => { afterEach(() => vi.unstubAllGlobals()); +describe("deleteTask", () => { + it("sends cascade and discard consent independently", async () => { + fetchSpy.mockResolvedValueOnce(new Response(null, { status: 204 })); + + await deleteTask( + "task-1", + { cascade: true, discardWorktreeChanges: true }, + { baseUrl: API_BASE_URL }, + ); + + expect(fetchSpy).toHaveBeenCalledOnce(); + const [url, init] = fetchSpy.mock.calls[0]; + expect(url).toBe( + `${API_BASE_URL}/api/v1/tasks/task-1?cascade=true&discard_worktree_changes=true`, + ); + expect(init?.method).toBe("DELETE"); + }); +}); + describe("detachTask", () => { it("posts without a body to the canonical detach endpoint", async () => { fetchSpy.mockResolvedValueOnce( diff --git a/apps/web/lib/api/domains/kanban-api.ts b/apps/web/lib/api/domains/kanban-api.ts index eea3417542..db532309e1 100644 --- a/apps/web/lib/api/domains/kanban-api.ts +++ b/apps/web/lib/api/domains/kanban-api.ts @@ -215,12 +215,22 @@ export async function updateTaskRepositoryBaseBranch( }); } +export type DeleteTaskParams = { + cascade?: boolean; + discardWorktreeChanges?: boolean; +}; + export async function deleteTask( taskId: string, - params?: { cascade?: boolean }, + params?: DeleteTaskParams, options?: ApiRequestOptions, ) { - const query = params?.cascade ? "?cascade=true" : ""; + const queryParams = new URLSearchParams(); + if (params?.cascade) queryParams.set("cascade", "true"); + if (params?.discardWorktreeChanges) { + queryParams.set("discard_worktree_changes", "true"); + } + const query = queryParams.toString() ? `?${queryParams.toString()}` : ""; return fetchJson(`/api/v1/tasks/${taskId}${query}`, { ...options, init: { method: "DELETE", ...(options?.init ?? {}) }, diff --git a/apps/web/lib/api/task-delete-errors.ts b/apps/web/lib/api/task-delete-errors.ts new file mode 100644 index 0000000000..f733b05174 --- /dev/null +++ b/apps/web/lib/api/task-delete-errors.ts @@ -0,0 +1,7 @@ +import { ApiError } from "./client"; + +export const TASK_DELETE_DIRTY_WORKTREE_ERROR_CODE = "task_delete_dirty_worktree"; + +export function isTaskDeleteDirtyWorktreeError(error: unknown): error is ApiError { + return error instanceof ApiError && error.errorCode === TASK_DELETE_DIRTY_WORKTREE_ERROR_CODE; +} diff --git a/apps/web/src/locales/en/task.json b/apps/web/src/locales/en/task.json index e4c6e162fd..a92c3e7242 100644 --- a/apps/web/src/locales/en/task.json +++ b/apps/web/src/locales/en/task.json @@ -332,6 +332,8 @@ "deleteLayoutConfirm": "Delete {{name}}?", "deleteSession": "Delete session?", "deleteTaskConfirm": "Delete \"{{taskTitle}}\". This action cannot be undone.", + "deleteDirtyWorktreeTitle": "Task deletion needs your confirmation", + "deleteDirtyWorktreeDescription": "The task stays visible. Review the local changes, then select discard and try again.", "deleteTaskTitle": "Delete task", "deleteTasksConfirm_one": "Delete {{count}} task. This action cannot be undone.", "deleteTasksConfirm_other": "Delete {{count}} tasks. This action cannot be undone.", @@ -341,6 +343,8 @@ "deleteTasksTitle_other": "Delete {{count}} tasks", "deletesTheCurrentTaskEnvironmentContainer": "Deletes the current task environment (container, sandbox, and/or worktree) and forces a fresh one to be created for your next run.", "deleting": "Deleting...", + "discardWorktreeChanges": "Permanently discard tracked and untracked changes", + "discardWorktreeChangesDescription": "The task worktree and its local changes will be removed.", "deliverTheseCommentsToTheCli": "Deliver these comments to the CLI agent's stdin as a single message; no need to open the chat box. The agent receives the same markdown that prepending to a typed message would produce.", "deny": "Deny", "dependencies": "Dependencies", diff --git a/apps/web/src/locales/pseudo/task.json b/apps/web/src/locales/pseudo/task.json index daaddae406..5f33062137 100644 --- a/apps/web/src/locales/pseudo/task.json +++ b/apps/web/src/locales/pseudo/task.json @@ -332,6 +332,8 @@ "deleteLayoutConfirm": "Ďēĺēţē {{name}}?", "deleteSession": "Ďēĺēţē śēśśĩōń?", "deleteTaskConfirm": "Ďēĺēţē \"{{taskTitle}}\". Ţĥĩś àćţĩōń ćàńńōţ ƀē ũńďōńē.", + "deleteDirtyWorktreeTitle": "Ţàśķ ďēĺēţĩōń ńēēďś ŷōũŕ ćōńƒĩŕḿàţĩōń", + "deleteDirtyWorktreeDescription": "Ţĥē ţàśķ śţàŷś vĩśĩƀĺē. Ŕēvĩēŵ ţĥē ĺōćàĺ ćĥàńĝēś, ţĥēń śēĺēćţ ďĩśćàŕď àńď ţŕŷ àĝàĩń.", "deleteTaskTitle": "Ďēĺēţē ţàśķ", "deleteTasksConfirm_one": "Ďēĺēţē {{count}} ţàśķ. Ţĥĩś àćţĩōń ćàńńōţ ƀē ũńďōńē.", "deleteTasksConfirm_other": "Ďēĺēţē {{count}} ţàśķś. Ţĥĩś àćţĩōń ćàńńōţ ƀē ũńďōńē.", @@ -341,6 +343,8 @@ "deleteTasksTitle_other": "Ďēĺēţē {{count}} ţàśķś", "deletesTheCurrentTaskEnvironmentContainer": "Ďēĺēţēś ţĥē ćũŕŕēńţ ţàśķ ēńvĩŕōńḿēńţ (ćōńţàĩńēŕ, śàńďƀōx, àńď/ōŕ ŵōŕķţŕēē) àńď ƒōŕćēś à ƒŕēśĥ ōńē ţō ƀē ćŕēàţēď ƒōŕ ŷōũŕ ńēxţ ŕũń.", "deleting": "Ďēĺēţĩńĝ...", + "discardWorktreeChanges": "Ƥēŕḿàńēńţĺŷ ďĩśćàŕď ţŕàćķēď àńď ũńţŕàćķēď ćĥàńĝēś", + "discardWorktreeChangesDescription": "Ţĥē ţàśķ ŵōŕķţŕēē àńď ĩţś ĺōćàĺ ćĥàńĝēś ŵĩĺĺ ƀē ŕēḿōvēď.", "deliverTheseCommentsToTheCli": "Ďēĺĩvēŕ ţĥēśē ćōḿḿēńţś ţō ţĥē ĆĹĨ àĝēńţ'ś śţďĩń àś à śĩńĝĺē ḿēśśàĝē; ńō ńēēď ţō ōƥēń ţĥē ćĥàţ ƀōx. Ţĥē àĝēńţ ŕēćēĩvēś ţĥē śàḿē ḿàŕķďōŵń ţĥàţ ƥŕēƥēńďĩńĝ ţō à ţŷƥēď ḿēśśàĝē ŵōũĺď ƥŕōďũćē.", "deny": "Ďēńŷ", "dependencies": "Ďēƥēńďēńćĩēś", diff --git a/apps/web/src/locales/pt-pt/task.json b/apps/web/src/locales/pt-pt/task.json index 552e5fedde..6280578fcc 100644 --- a/apps/web/src/locales/pt-pt/task.json +++ b/apps/web/src/locales/pt-pt/task.json @@ -331,6 +331,8 @@ "deleteLayoutConfirm": "Eliminar {{name}}?", "deleteSession": "Eliminar a sessão?", "deleteTaskConfirm": "Eliminar \"{{taskTitle}}\". Esta ação não pode ser anulada.", + "deleteDirtyWorktreeTitle": "A eliminação da tarefa precisa da sua confirmação", + "deleteDirtyWorktreeDescription": "A tarefa continua visível. Reveja as alterações locais, selecione a opção para as eliminar e tente novamente.", "deleteTaskTitle": "Eliminar tarefa", "deleteTasksConfirm_one": "Eliminar {{count}} tarefa. Esta ação não pode ser anulada.", "deleteTasksConfirm_other": "Eliminar {{count}} tarefas. Esta ação não pode ser anulada.", @@ -340,6 +342,8 @@ "deleteTasksTitle_other": "Eliminar {{count}} tarefas", "deletesTheCurrentTaskEnvironmentContainer": "Elimina o ambiente atual da tarefa (contentor, sandbox e/ou worktree) e força a criação de um novo na próxima execução.", "deleting": "A eliminar...", + "discardWorktreeChanges": "Eliminar permanentemente as alterações rastreadas e não rastreadas", + "discardWorktreeChangesDescription": "O worktree da tarefa e as respetivas alterações locais serão removidos.", "deliverTheseCommentsToTheCli": "Entrega estes comentários no stdin do agente CLI como uma única mensagem; não é preciso abrir a caixa de chat. O agente recebe o mesmo markdown que resultaria de os anteceder a uma mensagem escrita.", "deny": "Negar", "describeWhatNeedsToChange": "Descreva o que precisa de ser alterado", diff --git a/apps/web/src/locales/zh-cn/task.json b/apps/web/src/locales/zh-cn/task.json index f91067cfea..3fe83defb0 100644 --- a/apps/web/src/locales/zh-cn/task.json +++ b/apps/web/src/locales/zh-cn/task.json @@ -331,6 +331,8 @@ "deleteLayoutConfirm": "删除 {{name}}？", "deleteSession": "删除会话？", "deleteTaskConfirm": "删除“{{taskTitle}}”。此操作无法撤销。", + "deleteDirtyWorktreeTitle": "删除任务需要你的确认", + "deleteDirtyWorktreeDescription": "任务仍会显示。请检查本地更改，选择丢弃，然后重试。", "deleteTaskTitle": "删除任务", "deleteTasksConfirm_one": "删除 {{count}} 个任务。此操作无法撤销。", "deleteTasksConfirm_other": "删除 {{count}} 个任务。此操作无法撤销。", @@ -340,6 +342,8 @@ "deleteTasksTitle_other": "删除 {{count}} 个任务", "deletesTheCurrentTaskEnvironmentContainer": "删除当前任务环境（容器、沙箱和/或工作树），并在下次运行时强制创建新环境。", "deleting": "正在删除…", + "discardWorktreeChanges": "永久丢弃已跟踪和未跟踪的更改", + "discardWorktreeChangesDescription": "任务工作树及其本地更改将被移除。", "deliverTheseCommentsToTheCli": "将这些评论作为单条消息投递到 CLI 智能体的 stdin；无需打开聊天框。智能体收到的 markdown 与在输入消息前附加评论时相同。", "deny": "拒绝", "describeWhatNeedsToChange": "描述需要变更的内容", diff --git a/apps/web/src/locales/zh-hk/task.json b/apps/web/src/locales/zh-hk/task.json index abec48b75b..d6995adc6b 100644 --- a/apps/web/src/locales/zh-hk/task.json +++ b/apps/web/src/locales/zh-hk/task.json @@ -331,6 +331,8 @@ "deleteLayoutConfirm": "刪除 {{name}}？", "deleteSession": "刪除工作階段？", "deleteTaskConfirm": "刪除「{{taskTitle}}」。此操作無法撤銷。", + "deleteDirtyWorktreeTitle": "刪除任務需要你的確認", + "deleteDirtyWorktreeDescription": "任務仍會顯示。請檢查本地更改，選擇丟棄，然後重試。", "deleteTaskTitle": "刪除任務", "deleteTasksConfirm_one": "刪除 {{count}} 個任務。此操作無法撤銷。", "deleteTasksConfirm_other": "刪除 {{count}} 個任務。此操作無法撤銷。", @@ -340,6 +342,8 @@ "deleteTasksTitle_other": "刪除 {{count}} 個任務", "deletesTheCurrentTaskEnvironmentContainer": "刪除目前任務環境（容器、沙箱和/或工作樹），並在下次執行時強制建立新環境。", "deleting": "正在刪除…", + "discardWorktreeChanges": "永久丟棄已跟蹤和未跟蹤的更改", + "discardWorktreeChangesDescription": "任務工作樹及其本地更改將被移除。", "deliverTheseCommentsToTheCli": "將這些評論作為單條訊息投遞到 CLI 代理程式的 stdin；無需開啟聊天框。代理程式收到的 markdown 與在輸入訊息前附加評論時相同。", "deny": "拒絕", "describeWhatNeedsToChange": "描述需要變更的內容", diff --git a/apps/web/src/locales/zh-tw/task.json b/apps/web/src/locales/zh-tw/task.json index 5e6ba7bb93..2296b0913c 100644 --- a/apps/web/src/locales/zh-tw/task.json +++ b/apps/web/src/locales/zh-tw/task.json @@ -331,6 +331,8 @@ "deleteLayoutConfirm": "刪除 {{name}}？", "deleteSession": "刪除工作階段？", "deleteTaskConfirm": "刪除「{{taskTitle}}」。此操作無法撤銷。", + "deleteDirtyWorktreeTitle": "刪除任務需要你的確認", + "deleteDirtyWorktreeDescription": "任務仍會顯示。請檢查本地更改，選擇丟棄，然後重試。", "deleteTaskTitle": "刪除任務", "deleteTasksConfirm_one": "刪除 {{count}} 個任務。此操作無法撤銷。", "deleteTasksConfirm_other": "刪除 {{count}} 個任務。此操作無法撤銷。", @@ -340,6 +342,8 @@ "deleteTasksTitle_other": "刪除 {{count}} 個任務", "deletesTheCurrentTaskEnvironmentContainer": "刪除目前任務環境（容器、沙箱和/或工作樹），並在下次執行時強制建立新環境。", "deleting": "正在刪除…", + "discardWorktreeChanges": "永久丟棄已跟蹤和未跟蹤的更改", + "discardWorktreeChangesDescription": "任務工作樹及其本地更改將被移除。", "deliverTheseCommentsToTheCli": "將這些評論作為單條訊息投遞到 CLI 代理程式的 stdin；無需開啟聊天框。代理程式收到的 markdown 與在輸入訊息前附加評論時相同。", "deny": "拒絕", "describeWhatNeedsToChange": "描述需要變更的內容", diff --git a/docs/plans/task-delete-dirty-worktree-consent/plan.md b/docs/plans/task-delete-dirty-worktree-consent/plan.md new file mode 100644 index 0000000000..0b92a3743d --- /dev/null +++ b/docs/plans/task-delete-dirty-worktree-consent/plan.md @@ -0,0 +1,186 @@ +--- +created: 2026-09-05 +status: completed +requirements: + - REQ-TASKS-RUNTIME-CLEANUP-001 + - REQ-UI-TASK-CLEANUP-CONFIRMATION-001 +system_design: + - ../../specs/tasks/system-design/dirty-worktree-deletion.md + - ../../specs/ui/system-design/confirmation-warning-hierarchy.md +legacy_specs: [] +--- + +# Implementation Plan: Confirm Dirty Worktree Deletion + +## Overview + +Prevent task deletion from becoming an invisible, repeated cleanup failure when +an owned worktree contains local changes. Check every target before task +mutation, require explicit discard consent, and carry that consent into the +audited cleanup job. Use the shared task-delete dialog for the choice on desktop +and mobile. + +## Confirmed root cause + +- `Service.DeleteTask` prepares durable cleanup, deletes the task row, publishes + `task.deleted`, and starts cleanup after the successful response path. +- `verifyCleanRedundantCheckout` rejects tracked or untracked changes. This + safety check runs in the asynchronous cleanup worker, after the task has + disappeared from the UI. +- The cleanup refusal preserves the checkout and branch, but the deleted task no + longer gives the user a place to consent or recover. +- Current `main` does not retry forever. Cleanup is bounded to eight attempts + with backoff. Repeating a deterministic dirty-worktree refusal is still noisy + and cannot make progress. +- `TestDeleteTaskCleanupRetriesWhenAuditedWorktreeRemovalIsUnsafe` records the + current behavior: deletion succeeds, the dirty checkout remains, and the job + enters `retry_wait`. + +## Scope + +### In scope + +- Preflight direct and cascade task deletion before any selected task changes. +- A typed dirty-worktree conflict that lists every affected worktree. +- Explicit discard consent in the HTTP, service, and durable cleanup contracts. +- Forced cleanup that bypasses only the clean-checkout refusal. +- Forced cleanup that bypasses only the cleanliness check, while preserving + every other cleanup audit. +- Immediate terminal classification if an unconsented dirty checkout is found + after task mutation. +- One shared, localized discard choice for every task-delete surface. +- Desktop and phone browser proof, including bulk and cascade behavior. +- Public documentation for dirty-worktree deletion behavior. + +### Out of scope + +- Automatic commits, pushes, patches, or backups of local changes. +- Force-reclaiming uncertain paths or remote SSH worktrees. +- Deleting a branch whose commits are not safely redundant. +- A general UI for inspecting or replaying terminal cleanup jobs. +- Changes to archive, session deletion, workspace reset, or quick-chat expiry. + +## Technical approach + +### Admit deletion before mutation + +- Add one delete options value that keeps `cascade` and + `discard_worktree_changes` independent. +- Resolve the full direct or cascade target set, gather its owned worktree + snapshots, and inspect all local checkouts before task rows or cleanup jobs + change. +- Return HTTP 409 with `error_code: task_delete_dirty_worktree` and the complete + dirty-worktree list when consent is absent. Keep the service error typed for + non-HTTP callers. +- Store consent in the additive cleanup snapshot JSON. Existing snapshots decode + as unconsented. + +### Preserve fail-closed cleanup + +- Let a consented cleanup skip only the dirty-checkout rejection. +- Keep the pinned no-follow handle, exact path, owner, registration, expected + commit, shared-reference, and branch ancestry checks mandatory. +- Keep the existing rule that preserves a branch with unique commits. +- Treat an unconsented dirty refusal found after admission as terminal after its + first worker claim. Do not schedule retries for a condition that needs user + consent. + +### Surface the choice in the shared dialog + +- Extend `TaskDeleteConfirmDialog` to return `discardWorktreeChanges` with the + existing cascade value. +- Show an unchecked discard choice for a worktree executor, for a bulk selection + that contains one, and when an enabled cascade can include child worktrees. +- State that tracked and untracked local files are permanently removed. Disable + Delete until the user makes the explicit choice. +- Keep a task visible when the typed conflict is returned. Show the same + localized retry guidance at every single and bulk delete entry point. + +### Mobile design contract + +- Keep the existing centered `AlertDialog`; no new navigation or drawer is + needed for this blocking decision. +- Put the choice in the existing scrolling body and give its label a 44 CSS px + touch target. +- Keep the responsive full-width footer actions and current focus, Escape, + overflow, and safe-area behavior. +- Prove phone containment and choice reachability through the real task drawer + entry point. Prove the equivalent shared behavior on desktop. + +## Test strategy + +- Backend unit and handler tests cover preflight atomicity, direct and cascade + conflicts, consent snapshot round trips, constrained force cleanup, and + terminal handling for a post-admission race. +- Component and API tests cover choice visibility, reset, disabled actions, + query construction, and typed conflict classification. +- Desktop and mobile Playwright flows create a dirty task worktree, prove cancel + and unconsented preservation, then consent and prove task/worktree removal. +- Internationalization and public-documentation checks run with the focused + suites. + +## Work orders + +Wave 1: + +- [x] [Task 01: Enforce the dirty-worktree deletion boundary](task-01-enforce-delete-boundary.md) + +Wave 2: + +- [x] [Task 02: Add explicit discard consent to task deletion](task-02-surface-discard-consent.md) + +Execution is sequential in the primary conversation. Task 02 depends on the +backend contract from Task 01. The wave labels do not authorize subagents. + +## Risks + +- A checkout can become dirty between admission and cleanup. The worker must + still fail closed when its snapshot has no consent. +- Cascade admission must be atomic. Finding one dirty descendant cannot leave a + partially deleted subtree. +- Consent must never weaken path identity or Git redundancy checks. +- Bulk deletion uses separate requests. The UI must report partial success + accurately if another process changes one checkout between requests. + +## Validation commands + +- `(cd apps/backend && go test ./internal/worktree ./internal/task/service ./internal/task/handlers -count=1)` +- `(cd apps/web && pnpm exec vitest run components/task/task-delete-confirm-dialog.test.tsx lib/api/domains/kanban-api.test.ts hooks/use-task-actions.test.ts)` +- `(cd apps/web && pnpm run i18n:check)` +- `(cd apps/web && pnpm e2e:run tests/task/sidebar-delete-confirm.spec.ts)` +- `(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-confirmation-text-hierarchy.spec.ts)` +- `node --test scripts/validate-public-docs.test.mjs && node scripts/validate-public-docs.mjs` + +## Results + +- Added audited dirty-worktree inspection and typed service/HTTP conflicts. Direct and cascade + deletion now preflight every target before task mutation; consent is persisted in cleanup + snapshots and bypasses only the clean-checkout refusal. Legacy or raced unconsented refusals are + terminal, while owner, path, registration, commit, branch, and shared-reference checks remain + mandatory. +- Added the shared localized discard choice and propagated it through sidebar, mobile, Kanban, + graph, list, bulk, and task-message delete actions. Delete stays disabled until required consent + is selected, and typed conflicts keep client state intact while showing retry guidance. +- Added five-locale and pseudo-locale copy, public Git operations guidance, backend/component/API + regressions, and desktop/mobile browser coverage. +- Verification passed: 2,685 focused backend tests; 29 focused frontend tests; web lint and + typecheck; i18n checks; desktop sidebar E2E (1 passed); mobile confirmation E2E (4 passed); and + public-doc validation (61 tests, 45 pages). + +## Review follow-up results + +- Fixed the cleanup batch error-loss path: every per-worktree failure is now joined, so a + retryable failure remains retryable even when a later worktree reports the dirty-worktree + sentinel. +- Dirty-worktree admission now fails closed when an identified worktree still lacks repository or + worktree path metadata after cache enrichment. A cascade regression proves full-tree preflight, + atomic refusal, and discard-consent propagation. +- The shared discard option calls its translation hook in stable order, and the mobile browser + flow now creates a real dirty worktree, proves cancel/unconsented preservation, then proves + consented task and worktree removal. +- Follow-up verification passed: 2,693 backend tests across worktree, task service, and task + handlers; 58 focused frontend tests; web typecheck and targeted ESLint; specification lint; and + the mobile confirmation E2E suite (4 passed). +- PR follow-up aligned the test-only E2E reset with explicit discard consent and updated existing + browser deletion fixtures to select that consent. Focused Chromium deletion/reset coverage passed + 37 tests, the mobile dirty-worktree flow passed 4 tests, and the backend reset regression passed. diff --git a/docs/plans/task-delete-dirty-worktree-consent/task-01-enforce-delete-boundary.md b/docs/plans/task-delete-dirty-worktree-consent/task-01-enforce-delete-boundary.md new file mode 100644 index 0000000000..5115d303d5 --- /dev/null +++ b/docs/plans/task-delete-dirty-worktree-consent/task-01-enforce-delete-boundary.md @@ -0,0 +1,122 @@ +--- +id: "01-enforce-delete-boundary" +title: "Enforce the dirty-worktree deletion boundary" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/tasks/requirements/runtime-cleanup.md" +--- + +# Task 01: Enforce the Dirty-Worktree Deletion Boundary + +## Outcome + +Direct and cascade task deletion require explicit consent before they discard a +dirty owned worktree. Consented cleanup remains fail-closed for every other path +and Git identity check. + +## In scope + +- Add the typed service conflict and HTTP 409 response. +- Preflight all direct or cascade targets before task mutation. +- Carry discard consent in cleanup snapshots. +- Let consent bypass only the clean-checkout audit. +- Stop retrying an unconsented dirty refusal found after task mutation. + +## Exclusions + +- Frontend changes and localized copy. +- Changes to archive, session deletion, or workspace reset. +- Relaxing owner, path, registration, commit, branch, or shared-reference checks. + +## Requirements and design + +- `REQ-TASKS-RUNTIME-CLEANUP-001` +- `AC-TASKS-RUNTIME-CLEANUP-001.10` +- `AC-TASKS-RUNTIME-CLEANUP-001.11` +- `AC-TASKS-RUNTIME-CLEANUP-001.12` +- `docs/specs/tasks/system-design/dirty-worktree-deletion.md` +- `docs/specs/tasks/system-design/runtime-cleanup.md` + +## Acceptance + +- An unconsented direct or cascade request returns the complete typed conflict + before it changes any selected task or cleanup job. +- A consented request removes dirty owned worktrees only when every existing + non-cleanliness audit passes, and it still preserves branches with unique + commits. +- A legacy or raced unconsented dirty refusal becomes terminal after one worker + claim and does not enter `retry_wait`. + +## Verification + +```bash +(cd apps/backend && go test ./internal/worktree ./internal/task/service ./internal/task/handlers -count=1) +``` + +Write the direct-delete regression first. It must fail by showing that the task +row is currently deleted before the cleanup worker rejects the dirty checkout. + +## Files likely touched + +- `apps/backend/internal/worktree/manager_cleanup.go` +- `apps/backend/internal/worktree/manager_cleanup_audit.go` +- `apps/backend/internal/worktree/manager_cleanup_audit_test.go` +- `apps/backend/internal/task/models/resource_cleanup.go` +- `apps/backend/internal/task/service/service_tasks.go` +- `apps/backend/internal/task/service/handoff_service.go` +- `apps/backend/internal/task/service/handoff_cascade.go` +- `apps/backend/internal/task/service/resource_cleanup_jobs.go` +- `apps/backend/internal/task/service/resource_cleanup_jobs_test.go` +- `apps/backend/internal/task/service/resource_cleanup_worktree_retry_test.go` +- `apps/backend/internal/task/service/handoff_cascade_test.go` +- `apps/backend/internal/task/handlers/task_http_handlers.go` +- `apps/backend/internal/task/handlers/task_http_handlers_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. This task owns the API and cleanup contract required by Task 02. + +## Risks + +- Do not inspect a user-supplied path. Use the audited owned-worktree snapshot. +- Lock or snapshot ordering must prevent partial cascade mutation. +- Existing cleanup snapshots have no consent field and must remain fail-closed. + +## Output contract + +Report the RED test, response shape, admission ordering, force-audit boundary, +changed files, and exact test results. Update this work order and `plan.md` in +the same conversation. + +## Results + +- Added `DirtyWorktree` inspection with owned-path, no-follow, bounded Git-status, and sorted-file + safeguards. Direct and cascade deletion now resolves and preflights the complete target set + before changing task rows or creating cleanup work. +- Added the typed `task_delete_dirty_worktree` service conflict and HTTP 409 payload with every + affected worktree. Cleanup snapshots persist `discard_worktree_changes`, while old snapshots + remain unconsented and fail closed. +- Consent bypasses only the dirty-checkout audit. Path identity, ownership, registration, expected + `HEAD`, shared references, branch ancestry, and unique-commit preservation remain enforced. + A raced or legacy unconsented dirty refusal becomes terminal after its first claim. +- The direct regression first captured the old delete-before-refusal behavior, then passed with the + task row and dirty file preserved. Consent cleanup and raced terminal-state regressions also pass. +- Verification: `go test ./internal/worktree ./internal/task/service ./internal/task/handlers + -count=1` passed 2,685 tests across the three packages. +- Review follow-up joins every cleanup failure instead of retaining only the last error. The new + batch-order regression proves that a retryable metadata failure followed by a dirty-worktree + refusal remains retryable, and the missing-metadata inspection regression proves admission fails + closed after cache enrichment. +- The cascade service regression drives `DeleteTaskTreeWithOptions` through the complete root, + child, and grandchild set, proving refusal is atomic and the discard option reaches admission. +- The E2E reset boundary now explicitly opts into discarding disposable worktree changes, with a + focused regression covering the option passed to task deletion. +- Follow-up verification: `go test ./internal/worktree ./internal/task/service + ./internal/task/handlers -count=1` passed 2,693 tests across the three packages; specification + lint also passed. diff --git a/docs/plans/task-delete-dirty-worktree-consent/task-02-surface-discard-consent.md b/docs/plans/task-delete-dirty-worktree-consent/task-02-surface-discard-consent.md new file mode 100644 index 0000000000..0f113ec81e --- /dev/null +++ b/docs/plans/task-delete-dirty-worktree-consent/task-02-surface-discard-consent.md @@ -0,0 +1,143 @@ +--- +id: "02-surface-discard-consent" +title: "Add explicit discard consent to task deletion" +status: done +wave: 2 +depends_on: + - "01-enforce-delete-boundary" +plan: "plan.md" +spec: "../../specs/ui/requirements/confirmation-warning-hierarchy.md" +--- + +# Task 02: Add Explicit Discard Consent to Task Deletion + +## Outcome + +Every desktop and mobile task-delete surface gives the user an explicit choice +before local worktree changes can be discarded. A typed backend conflict keeps +the task visible and explains how to retry. + +## In scope + +- Extend the shared delete-dialog callback and API request options. +- Show and reset the worktree discard choice for single, bulk, and cascade cases. +- Disable Delete until required consent is selected. +- Handle the typed conflict across all task-delete entry points. +- Add copy in all five locales and regenerate Traditional Chinese and pseudo + catalogs with the repository commands. +- Add desktop and mobile browser proof. +- Update the public Git operations cleanup guidance. + +## Exclusions + +- A cleanup-job status or replay UI. +- Automatic commit, push, patch, or backup actions. +- Changes to compact archive confirmation. + +## Requirements and design + +- `REQ-UI-TASK-CLEANUP-CONFIRMATION-001` +- `AC-UI-TASK-CLEANUP-CONFIRMATION-001.8` +- `AC-UI-TASK-CLEANUP-CONFIRMATION-001.9` +- `AC-UI-TASK-CLEANUP-CONFIRMATION-001.10` +- `AC-TASKS-RUNTIME-CLEANUP-001.10` +- `AC-TASKS-RUNTIME-CLEANUP-001.11` +- `AC-TASKS-RUNTIME-CLEANUP-001.12` +- `docs/specs/ui/system-design/confirmation-warning-hierarchy.md` +- `docs/specs/tasks/system-design/dirty-worktree-deletion.md` +- `docs/specs/tasks/system-design/runtime-cleanup.md` + +## Acceptance + +- The shared dialog requires the explicit choice whenever the selected delete + can remove a worktree and passes both cascade and discard values unchanged. +- A typed dirty-worktree conflict preserves the task in client state and shows + localized retry guidance on every single and bulk delete surface. +- Desktop and phone E2E flows prove preservation without consent and removal + with consent; the phone choice has a 44 CSS px target inside the scroll body. + +## Verification + +```bash +(cd apps/web && pnpm exec vitest run components/task/task-delete-confirm-dialog.test.tsx lib/api/domains/kanban-api.test.ts hooks/use-task-actions.test.ts) +(cd apps/web && pnpm run i18n:zh-hant) +(cd apps/web && pnpm run i18n:pseudo) +(cd apps/web && pnpm run i18n:check) +(cd apps/web && pnpm e2e:run tests/task/sidebar-delete-confirm.spec.ts) +(cd apps/web && pnpm e2e:run --project mobile-chrome tests/task/mobile-confirmation-text-hierarchy.spec.ts) +node --test scripts/validate-public-docs.test.mjs && node scripts/validate-public-docs.mjs +``` + +Write the dialog and API tests first. They must fail because the current callback +contains only `cascade` and the current request has no discard query parameter. + +## Files likely touched + +- `apps/web/components/task/task-delete-confirm-dialog.tsx` +- `apps/web/components/task/task-delete-confirm-dialog.test.tsx` +- `apps/web/components/kanban-card.tsx` +- `apps/web/components/task/task-session-sidebar-dialogs.tsx` +- `apps/web/components/task/task-session-sidebar-selection.tsx` +- `apps/web/components/task/mobile/session-task-switcher-sheet.tsx` +- `apps/web/components/kanban/graph2-task-pipeline.tsx` +- `apps/web/components/kanban/task-multi-select-toolbar.tsx` +- `apps/web/hooks/use-task-actions.ts` +- `apps/web/hooks/use-task-actions.test.ts` +- `apps/web/hooks/use-task-multi-select.ts` +- `apps/web/hooks/use-sidebar-multi-select.ts` +- `apps/web/lib/api/domains/kanban-api.ts` +- `apps/web/lib/api/domains/kanban-api.test.ts` +- `apps/web/src/locales/en/task.json` +- `apps/web/src/locales/pt-pt/task.json` +- `apps/web/src/locales/zh-cn/task.json` +- `apps/web/src/locales/zh-hk/task.json` +- `apps/web/src/locales/zh-tw/task.json` +- `apps/web/e2e/tests/task/sidebar-delete-confirm.spec.ts` +- `apps/web/e2e/tests/task/mobile-confirmation-text-hierarchy.spec.ts` +- `docs/public/git-operations.md` + +## Dependencies + +- Task 01 must define the typed conflict and request contract. + +## Parallelism + +Sequential after Task 01. The browser proof depends on the real backend contract. + +## Risks + +- The dialog must reset consent after cancel or close. +- Bulk requests can have partial success if checkout state changes after the + shared confirmation. +- A cascade can contain worktree executors that are not visible in the root task. + +## Output contract + +Report the RED tests, every delete entry point updated, localized keys, desktop +and mobile outcomes, public documentation changes, and exact verification +results. Update this work order and `plan.md` in the same conversation. + +## Results + +- Extended the shared delete dialog and API contract with independent cascade and discard values. + The unchecked discard choice appears for worktree, bulk, and cascade-capable deletes, uses a + 44-CSS-pixel label target, resets on close, and keeps Delete disabled until consent is explicit. +- Propagated the option through sidebar, mobile, Kanban card, graph, list, bulk-selection, and + task-message delete entry points. Typed conflicts now use shared localized retry guidance without + removing the task from client state. +- Added the four new task-copy keys to `en`, `pt-pt`, `zh-cn`, `zh-hk`, `zh-tw`, and regenerated + pseudo output. Updated public Git operations documentation. +- Verification passed: 29 focused Vitest tests; web lint, typecheck, and i18n checks; desktop + sidebar E2E (1 passed); mobile confirmation/layout E2E (4 passed); and public-doc validation + (61 tests, 45 pages). The backend suite above proves physical dirty-worktree preservation and + consented cleanup; the browser flows prove the shared desktop/mobile consent state. +- Review follow-up moves `useTranslation` before the discard-option early return, preserving hook + order while the async cascade capability resolves. +- The mobile confirmation flow now materializes a real worktree, writes an uncommitted marker, + proves cancel and unconsented deletion preserve both task and file, then proves consented + deletion removes both. Follow-up verification passed 58 focused Vitest tests, web typecheck, + targeted ESLint, and `pnpm e2e:run --project mobile-chrome + tests/task/mobile-confirmation-text-hierarchy.spec.ts` (4 passed). +- PR follow-up aligned the test-only E2E reset and existing browser deletion fixtures with the + explicit discard-consent contract. Focused Chromium deletion/reset coverage passed 37 tests, + and the mobile dirty-worktree flow passed 4 tests. diff --git a/docs/public/git-operations.md b/docs/public/git-operations.md index d8d8a4eed0..0495f5b135 100644 --- a/docs/public/git-operations.md +++ b/docs/public/git-operations.md @@ -137,6 +137,21 @@ each repository, and a Git or snapshot persistence failure does not undo the acc After creation, Kandev copies any repository-configured files and runs its setup script. Setup-script failure is non-fatal: the worktree remains and the session surfaces a warning. Cleanup scripts run before worktree removal, but their failure also does not prevent removal. +### Deleting a task with local worktree changes + +Task deletion checks every owned worktree before it changes the task or starts +cleanup. If Git reports tracked or untracked changes, deletion stops and the +task remains visible. The confirmation dialog lists the affected worktrees and +requires an explicit choice to permanently discard those changes before a +retry. + +With that choice, Kandev removes the worktree and its local changes only after +it passes the normal path-ownership, Git-registration, checkout-identity, +shared-reference, and branch-safety checks. A branch with unique commits is +preserved. If the checkout becomes dirty after admission, cleanup preserves the +checkout and records a terminal failure instead of retrying the destructive +operation automatically. + ## Everyday operations All operations below run in the selected repository workspace. @@ -270,11 +285,11 @@ Read-only Git actions used by the Changes panel include `session.commit_diff`, ` Worktree cleanup audits the Git worktree and checkout before it runs the repository cleanup script. It then removes the Git worktree directory and may remove the local branch: -- Normal task deletion audits each owned worktree before mutation. Tracked or untracked changes keep the checkout in place and make durable cleanup retry. A clean branch is removed only when its current commit is already contained by the recorded base or repository default; a clean branch with unique commits is preserved after its checkout is reclaimed. Remote branches are never deleted. +- Normal task deletion audits each owned worktree before mutation. Tracked or untracked changes stop deletion and keep the task, checkout, and local branch in place until the user explicitly consents to discard them. With consent, cleanup removes the checkout only after the normal ownership and identity audits pass. A clean branch is removed only when its current commit is already contained by the recorded base or repository default; a clean branch with unique commits is preserved after its checkout is reclaimed. Remote branches are never deleted. - **Reset Environment** is allowed only when no task session is `STARTING` or `RUNNING`. It can optionally push first; a failed requested push aborts the reset. Teardown removes the worktree but deliberately preserves the local branch, then the next launch materializes a fresh environment. - Office handoff cleanup also preserves the branch when it releases a worktree. -Before deleting a task or performing a hard reset, commit and push anything you need. Kandev does not run a cleanup script when the audit finds uncommitted or untracked work. Cleanup scripts should perform transient teardown only; files they create are removed with the audited checkout. If an audited cleanup script fails, Kandev logs the failure and continues with the same recorded worktree. An audited directory is removed through its pinned no-follow handle, and un-audited fallback cleanup can remove a managed directory without following replacement links. Git metadata is then pruned. Registration pruning and local-branch deletion are verified before the durable cleanup job succeeds; a partial failure remains retryable. +Before deleting a task or performing a hard reset, commit and push anything you need. Without discard consent, Kandev does not start a cleanup job when the audit finds uncommitted or untracked work. With consent, cleanup scripts run only after the normal worktree audit and perform transient teardown; files they create are removed with the audited checkout. If an audited cleanup script fails, Kandev logs the failure and continues with the same recorded worktree. An audited directory is removed through its pinned no-follow handle, and un-audited fallback cleanup can remove a managed directory without following replacement links. Git metadata is then pruned. Registration pruning and local-branch deletion are verified before the durable cleanup job succeeds; a partial failure remains retryable. ## Troubleshooting diff --git a/docs/specs/tasks/requirements/runtime-cleanup.md b/docs/specs/tasks/requirements/runtime-cleanup.md index f32df2f351..1e82a18451 100644 --- a/docs/specs/tasks/requirements/runtime-cleanup.md +++ b/docs/specs/tasks/requirements/runtime-cleanup.md @@ -2,7 +2,7 @@ status: draft system: tasks created: 2026-06-22 -updated: 2026-08-31 +updated: 2026-09-05 owners: - cfl --- @@ -34,3 +34,15 @@ and safe when runtimes or task rows are already gone. attach-only reuse of the deleted workspace. - **AC-TASKS-RUNTIME-CLEANUP-001.8:** When dead-row repair loses its compare-and-set to a newer execution, reconciliation shall preserve the newer row without a warning. Other repair errors shall remain warnings. - **AC-TASKS-RUNTIME-CLEANUP-001.9:** When task deletion reclaims a Git worktree, the system shall verify the exact recorded path, branch, and commit before mutation; preserve a checkout with tracked or untracked changes; preserve a clean local branch with commits not contained by the recorded base or repository default; recover idempotently when the checkout path is already absent; and keep failed registration or branch cleanup retryable. +- **AC-TASKS-RUNTIME-CLEANUP-001.10:** When a task deletion targets an owned + worktree with tracked or untracked changes, the system shall require explicit + discard consent before it mutates the task or starts resource cleanup. +- **AC-TASKS-RUNTIME-CLEANUP-001.11:** When discard consent is absent, the + system shall return a typed conflict and preserve the task, worktree, branch, + and local changes without a cleanup retry. +- **AC-TASKS-RUNTIME-CLEANUP-001.12:** When discard consent is present, the + system shall remove dirty owned worktrees only after the pinned no-follow + path handle confirms the exact owned path, no shared active environment + reference exists, and all ownership, path, registration, branch, and commit + identity checks pass. The existing unique branch preservation rule shall + remain active. diff --git a/docs/specs/tasks/system-design/dirty-worktree-deletion.md b/docs/specs/tasks/system-design/dirty-worktree-deletion.md new file mode 100644 index 0000000000..96542a80a2 --- /dev/null +++ b/docs/specs/tasks/system-design/dirty-worktree-deletion.md @@ -0,0 +1,169 @@ +--- +status: draft +system: tasks +requirements: + - REQ-TASKS-RUNTIME-CLEANUP-001 +created: 2026-09-05 +updated: 2026-09-05 +owners: + - cfl +--- + +# Dirty Worktree Task Deletion System Design + +## Purpose and boundaries + +This design extends task runtime cleanup for direct and cascade deletion. It +defines how deletion detects local worktree changes, obtains explicit discard +consent, and preserves every existing ownership and Git identity safeguard. + +It does not change archive, session deletion, workspace reset, automatic commit +or push behavior, or branch redundancy rules. + +## Requirement mapping + +| Requirement | Design source | +| --- | --- | +| REQ-TASKS-RUNTIME-CLEANUP-001.10-.12 | Admission, cleanup snapshot, audited removal, and failure classification below | + +## Existing system context + +Task deletion currently prepares a durable cleanup job, deletes the task row, +publishes `task.deleted`, and then starts the cleanup worker. The worktree +manager checks `git status --porcelain=v1 --untracked-files=normal` during +audited removal. If it finds output, it rejects removal after the task is gone. + +The cleanup worker has eight bounded attempts with backoff. A dirty-checkout +refusal is deterministic, so another scheduled attempt cannot make progress +without user consent. + +## Design + +### Admission before mutation + +Direct and cascade deletion use one options value with independent `cascade` and +`discard_worktree_changes` fields. The service resolves the full target set, +captures each owned worktree through the existing audited snapshot path, and +checks every local checkout before it changes a task or cleanup job. + +If a checkout is dirty and consent is absent, the service returns one typed +conflict with all dirty worktrees. The operation does not mutate any selected +task and does not create or activate a cleanup job. + +If consent is present, the service stores it in every applicable cleanup +snapshot before it performs the existing task mutation sequence. A cascade uses +one explicit consent choice for its complete target set. + +### Audited removal with consent + +Consent bypasses only the clean-checkout rejection. Removal still requires: + +- a pinned no-follow path handle; +- exact workspace, owner, and worktree identity; +- an expected Git worktree registration; +- the expected `HEAD` commit; +- no shared active environment reference; and +- the existing branch redundancy and ancestry result. + +The worktree checkout can be removed when these checks pass. The branch remains +when it contains unique commits under the existing cleanup policy. + +### Race after admission + +A checkout can become dirty after admission. If its cleanup snapshot has no +discard consent, the worker preserves the checkout and records a terminal +`failed` result after its first claim. It does not schedule another attempt. + +A snapshot with consent can remove the changed checkout, subject to all other +audits. Existing snapshots decode without consent and remain fail-closed. + +## Components and ownership + +- `task/service` owns target resolution, admission, atomic cascade behavior, + typed errors, and the cleanup snapshot. +- `task/handlers` maps the typed service conflict to the HTTP contract. +- `worktree.Manager` owns Git status inspection and the constrained removal + audit. +- The cleanup worker owns terminal classification for a dirty refusal found + after task mutation. +- The UI system design owns how the user gives consent and receives conflict + feedback. + +## Interface contract + +`DELETE /api/v1/tasks/:id` accepts the optional +`discard_worktree_changes=true` query parameter. The existing `cascade=true` +parameter remains independent. + +Without consent, a dirty target returns HTTP 409 before task mutation: + +```json +{ + "error": "task worktree contains local changes", + "error_code": "task_delete_dirty_worktree", + "dirty_worktrees": [ + { + "worktree_id": "...", + "repository_id": "...", + "path": "...", + "dirty_files": ["relative/path"] + } + ] +} +``` + +The response lists every dirty worktree in a direct or cascade request. The +service keeps the error typed so non-HTTP callers also fail before mutation +unless they provide explicit consent. + +## Persistence + +The cleanup snapshot JSON gains an additive discard-consent field. No database +schema change is required. A missing field is `false`. + +## Failure and recovery + +- A status inspection error fails admission closed and returns no consent prompt + that could authorize an uncertain path. +- A dirty conflict preserves the task, worktree, branch, and local changes. The + user can commit the changes or retry with explicit discard consent. +- A non-cleanliness audit failure preserves the checkout even when consent is + present. Its existing retry or terminal policy remains active. +- A raced unconsented dirty refusal is terminal because retry cannot obtain + consent for an already deleted task. + +## Security and privacy + +The response contains only task-owned worktree identifiers, paths, and relative +dirty file names available to the authorized task caller. Consent never accepts +a caller-supplied filesystem target and never weakens symlink or ownership +checks. + +## Observability + +The service logs the typed admission conflict without treating it as an internal +cleanup failure. A post-admission dirty refusal records one terminal cleanup job +error. It does not emit repeated retry attempts. + +## Test scenarios + +- **GIVEN** one or more dirty owned worktrees without consent, **WHEN** direct or + cascade admission runs, **THEN** it reports every dirty target and changes no + task or cleanup job. +- **GIVEN** a dirty owned worktree with consent, **WHEN** every other audit + passes, **THEN** cleanup removes the checkout and applies the existing branch + redundancy result. +- **GIVEN** one identity audit fails, **WHEN** consent is present, **THEN** + cleanup preserves the checkout. +- **GIVEN** an unconsented checkout becomes dirty after admission, **WHEN** the + worker sees the refusal, **THEN** it records terminal failure without retry. +- **GIVEN** an older cleanup snapshot, **WHEN** it targets a dirty checkout, + **THEN** missing consent is false and cleanup fails closed. + +## Related records + +- [Task Runtime Cleanup](runtime-cleanup.md) +- [Fail-closed GC semantics](../../../decisions/0009-fail-closed-gc-semantics.md) +- [Task-owned worktree lifetime](../../../decisions/2026-08-08-task-owned-worktree-lifetime.md) +- [Confirmation Warning Hierarchy](../../ui/system-design/confirmation-warning-hierarchy.md) + diff --git a/docs/specs/tasks/system-design/runtime-cleanup.md b/docs/specs/tasks/system-design/runtime-cleanup.md index 6f3ce8a80c..6ccb522eb1 100644 --- a/docs/specs/tasks/system-design/runtime-cleanup.md +++ b/docs/specs/tasks/system-design/runtime-cleanup.md @@ -4,7 +4,7 @@ system: tasks requirements: - REQ-TASKS-RUNTIME-CLEANUP-001 created: 2026-06-22 -updated: 2026-08-31 +updated: 2026-09-05 owners: - cfl --- @@ -231,9 +231,8 @@ never holds filesystem, target-path, or repository Git locks. ## API Surface -No new user-facing HTTP or WebSocket action is required. Existing task archive, -task delete, session stop, and backend startup behavior gain stronger cleanup -guarantees. +No new action is required for the base contract. Dirty deletion admission is in +[Dirty Worktree Task Deletion](dirty-worktree-deletion.md). `session.delete` keeps its existing request and response contract. Success means the session row is gone. It does not mean the task workspace was cleaned, and it diff --git a/docs/specs/ui/requirements/confirmation-warning-hierarchy.md b/docs/specs/ui/requirements/confirmation-warning-hierarchy.md index 9502242d9a..73ab075105 100644 --- a/docs/specs/ui/requirements/confirmation-warning-hierarchy.md +++ b/docs/specs/ui/requirements/confirmation-warning-hierarchy.md @@ -2,6 +2,7 @@ status: active system: ui created: 2026-08-24 +updated: 2026-09-05 owners: - kandev --- @@ -127,13 +128,28 @@ repository resources, so that I can understand the consequence before acting. zero-descendant surface. Task-switcher/task-row surfaces shall retain their intentional row-owned coarse-pointer inline confirmation, and a disabled archive-confirmation preference shall continue to bypass the surface. +- **AC-UI-TASK-CLEANUP-CONFIRMATION-001.11:** When the task's executor + projection is absent but task-owned worktree state may remain after the last + session ends, the delete confirmation shall fail closed by showing the same + explicit discard selection. +- **AC-UI-TASK-CLEANUP-CONFIRMATION-001.12:** When delete can remove one or more + task worktrees, the confirmation shall state that tracked and untracked local + changes will be permanently discarded. The destructive action shall require + an explicit selection for this outcome. +- **AC-UI-TASK-CLEANUP-CONFIRMATION-001.13:** When the backend rejects deletion + because discard consent is absent, every task deletion surface shall keep the + task visible and show a localized explanation. The explanation shall tell the + user how to retry with explicit consent. +- **AC-UI-TASK-CLEANUP-CONFIRMATION-001.14:** At phone widths, the discard + selection shall remain inside the existing centered dialog and its scrolling + body. Its label shall provide a touch target of at least 44 CSS px. ## Out of scope - Rewriting or re-translating the still-working warning. - Widening all confirmation popovers globally. -- Changing in-flight detection, archive behavior, delete behavior, API - contracts, focus handling, Escape handling, or safe-area handling. +- Changing in-flight detection, archive behavior, focus handling, Escape + handling, or safe-area handling. - Changing compact archive popover/inline or unrelated dialog action dimensions. - Replacing the centered task alerts with a drawer, sheet, or new navigation diff --git a/docs/specs/ui/system-design/confirmation-warning-hierarchy.md b/docs/specs/ui/system-design/confirmation-warning-hierarchy.md index 2506d198c8..e39326ebd9 100644 --- a/docs/specs/ui/system-design/confirmation-warning-hierarchy.md +++ b/docs/specs/ui/system-design/confirmation-warning-hierarchy.md @@ -1,6 +1,7 @@ --- status: current system: ui +updated: 2026-09-05 requirements: - REQ-TASKS-CONFIRMATION-WARNING-001 - REQ-TASKS-CONFIRMATION-SURFACE-002 @@ -13,10 +14,9 @@ requirements: This design owns the presentation contract for the shared still-working warning, the fine-pointer archive confirmation surface, and the cleanup consequence -hierarchy used by task archive and delete workflows. It changes density, -surface-local composition, localized presentation, action semantics, and archive -mounting only; task state, cleanup rules, and callbacks remain owned by their -existing components and runtime contracts. +hierarchy used by task archive and delete workflows. It also owns the explicit +discard choice for task worktrees. Task state and cleanup rules remain owned by +the task runtime contract. ## Requirement mapping @@ -70,6 +70,14 @@ policy into a visual component. phone width remains because prose benefits from the available line length and the primitive already preserves 16px viewport insets. +`TaskDeleteConfirmDialog` shows an unchecked discard selection when the delete +can remove a worktree. This includes a worktree executor, a bulk selection with +a worktree executor, or a cascade that can include child worktrees. The label +states that tracked and untracked files will be permanently removed. The delete +action remains disabled until the user selects this outcome. When the task's +executor projection is absent, the dialog fails closed and shows the same +choice because a retained task-owned worktree may outlive its last session. + Each full surface uses an auto/minmax/auto layout: title in the first row, one `minmax(0, 1fr)` body containing description, cleanup consequences, warning, and cascade choice, then the persistent action footer. The surface is capped by @@ -152,9 +160,13 @@ yellow border/background/text classes. The compact style contract is: - existing rounded border, yellow semantic colors, and dark-mode contrast stay unchanged. -No API, WebSocket, state, or persisted-data contract changes are required. -Localization catalogs change only for task confirmation presentation; executor -cleanup semantics remain unchanged. +The dialog passes `discardWorktreeChanges` with the existing cascade choice. +The task API sends it as `discard_worktree_changes=true`. A typed HTTP 409 uses +`task_delete_dirty_worktree` when consent is absent. A shared client classifier +maps this code to localized feedback for single and bulk deletion surfaces. + +Localization catalogs change in the five real locales. The Traditional Chinese +catalogs use the existing generation command. No browser state is persisted. `forceDialog` is an optional internal presentation prop with a default of `false`. It is passed from the card-owned source to the existing routing @@ -174,26 +186,34 @@ popover or the explicitly inline row surface. This changes rendering only, not archive commands, descendant classification, preference state, or callbacks. +The delete dialog resets discard selection when it closes. On confirm, it sends +the cascade and discard choices through the existing callback. A typed dirty +conflict keeps the task in every local cache and shows one localized message. +Other deletion errors keep their existing error handling. + ## Failure and recovery -No new runtime failure path exists. If localized text is longer than the -available width, the shared surface-text contract wraps it inside the body. If -content becomes taller than the dynamic viewport, the body scrolls without -moving the title or actions. Unknown executor types retain the generic running -session effect. If no task activity is present, no warning mounts, as before. +If localized text is longer than the available width, the shared surface-text +contract wraps it inside the body. If content is taller than the dynamic +viewport, the body scrolls without moving the title or actions. + +If deletion returns `task_delete_dirty_worktree`, the task remains visible. The +localized message tells the user to reopen the confirmation and select discard. +Unknown executor types retain the generic running session effect. ## Persistence -None. This is a client-side presentation-only change. +The browser does not persist discard selection or consent. Backend +cleanup-snapshot persistence, the `discard_worktree_changes` contract, and +typed HTTP 409 handling are defined in +[Dirty worktree task deletion](../../tasks/system-design/dirty-worktree-deletion.md). ## Observability -Existing component tests continue to assert warning presence and absence for -generating, background, and idle activity. Cleanup-summary tests cover every -executor, bulk grouping, ordering, effects, and supporting notes. Dialog and -compact-surface tests assert equivalent structured content, semantic action -variants, and unchanged callbacks. Rendered desktop and phone checks inspect -computed text wrapping, viewport bounds, scroll ownership, action reachability, +Existing component tests continue to cover warning visibility and cleanup +content. Delete-dialog tests cover selection reset, disabled actions, single and +bulk callback values, cascade behavior, and typed conflict feedback. Rendered +desktop and phone checks cover viewport bounds, scroll ownership, action reach, and document overflow. ## Mobile and desktop containment @@ -219,13 +239,15 @@ The task-delete phone check enters through the real task drawer action menu and opens Delete with a long task title and a longer bundled locale. After portal animations settle, it verifies the surface's 16px viewport insets, zero document horizontal overflow, title/body wrapping, body scroll ownership when -needed, and visible persistent actions. Both actions are full-width and at -least 44px high, Delete exposes `data-variant="destructive"`, and Cancel closes -the alert without deleting the task. A desktop check retains compact row -actions and the existing deletion flow. +needed, and visible persistent actions. The discard label has a 44px touch +target and remains inside the scrolling body. Both footer actions remain +full-width and at least 44px high. A desktop check retains compact row actions. ## Related decisions - [ADR 0049: Fine-grained foreground-idle busy signal](../../../decisions/0049-fine-grained-foreground-idle-busy-signal.md) +- [ADR 0009: Fail-closed GC semantics](../../../decisions/0009-fail-closed-gc-semantics.md) +- [Task-owned worktree lifetime](../../../decisions/2026-08-08-task-owned-worktree-lifetime.md) +- [Dirty worktree task deletion](../../tasks/system-design/dirty-worktree-deletion.md) - [Mobile task navigation](../requirements/mobile-task-navigation.md) - [Surface text hierarchy](../requirements/surface-text-hierarchy.md)