fix: harden done cleanup identity and path safety (#3195)

Co-authored-by: ayattara <alassane.yattara@savoirfairelinux.com>
Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>

apps/backend/internal/worktree/manager_cleanup.go

@@ -275,6 +275,11 @@ func (m *Manager) removeWorktree(ctx context.Context, wt *Worktree, removeBranch
 	if wt == nil {
 		return errors.New("worktree cleanup requires a worktree")
 	}
+	// Cleanup runs after the inventory snapshot has been selected. Keep a
+	// private copy so a later session/path projection update cannot redirect
+	// the destructive operation or its reference release.
+	worktreeSnapshot := *wt
+	wt = &worktreeSnapshot
 	if strings.TrimSpace(wt.RepositoryPath) == "" || strings.TrimSpace(wt.Path) == "" {
 		return errors.New("worktree cleanup requires repository and worktree paths")
 	}
@@ -572,6 +577,12 @@ func (m *Manager) removeWorktreeDir(
 		if err := pathHandles[0].RemoveDirectory(ctx); err != nil {
 			return fmt.Errorf("remove audited worktree directory: %w", err)
 		}
+		// The parent cleanup is a separate path-based fallback. Revalidate the
+		// audited target immediately before it so a replacement cannot turn an
+		// otherwise safe cleanup into recursive deletion of another directory.
+		if err := pathHandles[0].VerifyPath(worktreePath); err != nil && !errors.Is(err, os.ErrNotExist) {
+			return fmt.Errorf("worktree path changed before parent cleanup: %w", err)
+		}
 		pruneCmd := newGitCommand(ctx, "worktree", "prune")
 		pruneCmd.Dir = repoPath
 		if err := runGitCmd(ctx, pruneCmd); err != nil {

apps/backend/internal/worktree/manager_cleanup_audit.go

@@ -43,7 +43,7 @@ func (m *Manager) auditWorktreeCleanup(
 			_ = pathHandle.Close()
 		}
 	}()
-	branchRef, branchOID, err := m.cleanupBranchIdentity(ctx, wt)
+	branchRef, branchOID, err := m.cleanupBranchIdentity(ctx, wt, pathPresent, removeBranch)
 	if err != nil {
 		return worktreeCleanupAudit{}, err
 	}
@@ -119,12 +119,24 @@ func (m *Manager) openCleanupPathHandle(
 	return handle, nil
 }
 
-func (m *Manager) cleanupBranchIdentity(ctx context.Context, wt *Worktree) (string, string, error) {
+func (m *Manager) cleanupBranchIdentity(
+	ctx context.Context, wt *Worktree, pathPresent, requireImmutableIdentity bool,
+) (string, string, error) {
 	branchRef := ""
 	if wt.Branch != "" {
 		branchRef = "refs/heads/" + wt.Branch
 	}
 	expectedOID := strings.TrimSpace(wt.CleanupHeadOID)
+	if expectedOID == "" && pathPresent {
+		output, err := m.runBoundedGitInspect(ctx, wt.Path, "rev-parse", "--verify", "HEAD^{commit}")
+		if err != nil {
+			return "", "", fmt.Errorf("capture cleanup worktree HEAD: %w", err)
+		}
+		expectedOID = strings.TrimSpace(output)
+		if expectedOID == "" {
+			return "", "", errors.New("capture cleanup worktree HEAD returned an empty commit")
+		}
+	}
 	if branchRef == "" {
 		return branchRef, expectedOID, nil
 	}
@@ -138,6 +150,9 @@ func (m *Manager) cleanupBranchIdentity(ctx context.Context, wt *Worktree) (stri
 		// classifier will fail closed rather than deleting another checkout.
 		return branchRef, "", nil
 	}
+	if expectedOID == "" && requireImmutableIdentity {
+		return "", "", fmt.Errorf("cleanup branch %q has no immutable expected commit", wt.Branch)
+	}
 	output, err := m.runBoundedGitInspect(ctx, wt.RepositoryPath, "rev-parse", "--verify", branchRef+"^{commit}")
 	if err != nil {
 		return "", "", fmt.Errorf("resolve cleanup branch %q: %w", wt.Branch, err)

apps/backend/internal/worktree/manager_cleanup_identity_test.go

@@ -0,0 +1,160 @@
+package worktree
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/kandev/kandev/internal/task/models"
+)
+
+// blockingReferenceStore pauses cleanup after removeWorktree has copied the
+// inventory record. This is reviewer-requested contract coverage for the
+// snapshot boundary.
+type blockingReferenceStore struct {
+	*SQLiteStore
+	entered     chan struct{}
+	release     chan struct{}
+	releaseOnce sync.Once
+}
+
+func (s *blockingReferenceStore) CountActiveWorktreeReferences(
+	ctx context.Context,
+	_ string,
+	_ []string,
+) (int, error) {
+	close(s.entered)
+	select {
+	case <-s.release:
+		return 0, nil
+	case <-ctx.Done():
+		return 0, ctx.Err()
+	}
+}
+
+func (s *blockingReferenceStore) unblock() {
+	s.releaseOnce.Do(func() { close(s.release) })
+}
+
+// postRemovalSwapDirectoryHandle moves the audited directory away before the
+// second verification. It lets the test prove that parent cleanup is skipped
+// when the lexical path no longer identifies the audited directory.
+type postRemovalSwapDirectoryHandle struct {
+	path            string
+	replacementPath string
+	verifyCalls     int
+}
+
+func (h *postRemovalSwapDirectoryHandle) Close() error { return nil }
+
+func (h *postRemovalSwapDirectoryHandle) VerifyPath(string) error {
+	h.verifyCalls++
+	if h.verifyCalls == 1 {
+		return nil
+	}
+	return errors.New("pinned directory no longer matches lexical path")
+}
+
+func (h *postRemovalSwapDirectoryHandle) IsValidWorktree() bool { return true }
+
+func (h *postRemovalSwapDirectoryHandle) RemoveDirectory(context.Context) error {
+	return os.Rename(h.path, h.replacementPath)
+}
+
+func (h *postRemovalSwapDirectoryHandle) ReadFile(string) ([]byte, error) {
+	return nil, os.ErrNotExist
+}
+
+func (h *postRemovalSwapDirectoryHandle) WriteFile(string, []byte, os.FileMode) error {
+	return errors.New("fake directory handle does not support writes")
+}
+
+// Reviewer-requested contract coverage: the post-removal identity check must
+// run before the best-effort parent-directory cleanup.
+func TestRemoveWorktreeDir_RevalidatesBeforeParentCleanup(t *testing.T) {
+	mgr, _ := newReferenceCleanupTestManager(t)
+	tasksBase, err := mgr.config.ExpandedTasksBasePath()
+	if err != nil {
+		t.Fatalf("expand tasks base path: %v", err)
+	}
+	taskDir := filepath.Join(tasksBase, "task-post-removal")
+	worktreePath := filepath.Join(taskDir, "repository")
+	replacementPath := filepath.Join(t.TempDir(), "replacement")
+	if err := os.MkdirAll(worktreePath, 0o755); err != nil {
+		t.Fatalf("create worktree path: %v", err)
+	}
+
+	handle := &postRemovalSwapDirectoryHandle{
+		path:            worktreePath,
+		replacementPath: replacementPath,
+	}
+	err = mgr.removeWorktreeDir(
+		context.Background(),
+		worktreePath,
+		filepath.Join(t.TempDir(), "repository.git"),
+		handle,
+	)
+	if err == nil || !strings.Contains(err.Error(), "before parent cleanup") {
+		t.Fatalf("removeWorktreeDir error = %v, want post-removal identity failure", err)
+	}
+	if handle.verifyCalls != 2 {
+		t.Fatalf("VerifyPath calls = %d, want 2", handle.verifyCalls)
+	}
+	if _, err := os.Stat(replacementPath); err != nil {
+		t.Fatalf("renamed original worktree missing: %v", err)
+	}
+	if _, err := os.Stat(taskDir); err != nil {
+		t.Fatalf("parent task directory was removed after identity failure: %v", err)
+	}
+}
+
+// Reviewer-requested contract coverage: cleanup must use the copied record
+// even when a caller mutates its worktree projection while cleanup waits.
+func TestCleanupWorktrees_UsesSnapshotWhenCallerMutatesPath(t *testing.T) {
+	mgr, store := newReferenceCleanupTestManager(t)
+	seedReferenceCleanupSession(t, store, "task-snapshot", "session-snapshot", models.TaskSessionStateCompleted)
+	wt := createReferenceCleanupWorktree(t, mgr, "task-snapshot", "session-snapshot")
+	originalPath := wt.Path
+
+	blockingStore := &blockingReferenceStore{
+		SQLiteStore: store,
+		entered:     make(chan struct{}),
+		release:     make(chan struct{}),
+	}
+	mgr.store = blockingStore
+	t.Cleanup(blockingStore.unblock)
+
+	cleanupDone := make(chan error, 1)
+	go func() {
+		cleanupDone <- mgr.CleanupWorktrees(context.Background(), []*Worktree{wt})
+	}()
+	<-blockingStore.entered
+
+	replacementPath := originalPath + "-caller-replacement"
+	if err := os.MkdirAll(replacementPath, 0o755); err != nil {
+		blockingStore.unblock()
+		t.Fatalf("create caller replacement path: %v", err)
+	}
+	sentinel := filepath.Join(replacementPath, "sentinel.txt")
+	if err := os.WriteFile(sentinel, []byte("preserve me\n"), 0o644); err != nil {
+		blockingStore.unblock()
+		t.Fatalf("write caller replacement sentinel: %v", err)
+	}
+	wt.Path = replacementPath
+	blockingStore.unblock()
+
+	if err := <-cleanupDone; err != nil {
+		t.Fatalf("cleanup after caller projection mutation: %v", err)
+	}
+	if _, err := os.Stat(originalPath); !os.IsNotExist(err) {
+		t.Fatalf("snapshotted worktree path remains, stat error = %v", err)
+	}
+	if got, err := os.ReadFile(sentinel); err != nil || string(got) != "preserve me\n" {
+		t.Fatalf("caller replacement changed: contents=%q err=%v", got, err)
+	}
+	assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
+}

apps/backend/internal/worktree/manager_cleanup_recovery_test.go

@@ -2,6 +2,7 @@ package worktree
 
 import (
 	"context"
+	"errors"
 	"os"
 	"path/filepath"
 	"strings"
@@ -27,6 +28,35 @@ type cancellingCleanupScriptHandler struct {
 	cancel context.CancelFunc
 }
 
+type failingReleaseStore struct {
+	*SQLiteStore
+	failUpdate bool
+}
+
+func (s *failingReleaseStore) UpdateWorktree(ctx context.Context, wt *Worktree) error {
+	if s.failUpdate {
+		return errors.New("injected release failure")
+	}
+	return s.SQLiteStore.UpdateWorktree(ctx, wt)
+}
+
+type swappingCleanupScriptHandler struct{}
+
+func (h *swappingCleanupScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error {
+	return nil
+}
+
+func (h *swappingCleanupScriptHandler) ExecuteCleanupScript(_ context.Context, req ScriptExecutionRequest) error {
+	replacement := req.WorkingDir + "-replacement"
+	if err := os.Rename(req.WorkingDir, replacement); err != nil {
+		return err
+	}
+	if err := os.MkdirAll(req.WorkingDir, 0o755); err != nil {
+		return err
+	}
+	return os.WriteFile(filepath.Join(req.WorkingDir, "replacement.txt"), []byte("preserve me\n"), 0o644)
+}
+
 func (h *cancellingCleanupScriptHandler) ExecuteSetupScript(context.Context, ScriptExecutionRequest) error {
 	return nil
 }
@@ -40,6 +70,7 @@ func TestCleanupWorktrees_RecoversAfterPathOnlyRemoval(t *testing.T) {
 	mgr, store := newReferenceCleanupTestManager(t)
 	seedReferenceCleanupSession(t, store, "task-partial", "session-partial", models.TaskSessionStateCompleted)
 	wt := createReferenceCleanupWorktree(t, mgr, "task-partial", "session-partial")
+	wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
 
 	if err := os.RemoveAll(wt.Path); err != nil {
 		t.Fatalf("remove worktree path without shared Git metadata: %v", err)
@@ -53,10 +84,71 @@ func TestCleanupWorktrees_RecoversAfterPathOnlyRemoval(t *testing.T) {
 	assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
 }
 
+func TestCleanupWorktrees_RejectsPathlessRetryWithoutImmutableHead(t *testing.T) {
+	mgr, store := newReferenceCleanupTestManager(t)
+	seedReferenceCleanupSession(t, store, "task-no-snapshot", "session-no-snapshot", models.TaskSessionStateCompleted)
+	wt := createReferenceCleanupWorktree(t, mgr, "task-no-snapshot", "session-no-snapshot")
+
+	if err := os.RemoveAll(wt.Path); err != nil {
+		t.Fatalf("remove worktree path without shared Git metadata: %v", err)
+	}
+	if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil {
+		t.Fatal("cleanup accepted pathless worktree without immutable cleanup identity")
+	}
+	assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
+	assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
+}
+
+func TestCleanupWorktreesPreservingBranches_RetriesAfterReleaseFailure(t *testing.T) {
+	mgr, store := newReferenceCleanupTestManager(t)
+	seedReferenceCleanupSession(t, store, "task-preserve-retry", "session-preserve-retry", models.TaskSessionStateCompleted)
+	wt := createReferenceCleanupWorktree(t, mgr, "task-preserve-retry", "session-preserve-retry")
+
+	mgr.store = &failingReleaseStore{SQLiteStore: store, failUpdate: true}
+	if err := mgr.CleanupWorktreesPreservingBranches(context.Background(), []*Worktree{wt}); err == nil {
+		t.Fatal("cleanup succeeded despite injected reference-release failure")
+	}
+	if _, err := os.Stat(wt.Path); !os.IsNotExist(err) {
+		t.Fatalf("worktree path remains after first removal: %v", err)
+	}
+	assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
+
+	mgr.store = store
+	if err := mgr.CleanupWorktreesPreservingBranches(context.Background(), []*Worktree{wt}); err != nil {
+		t.Fatalf("retry pathless branch-preserving cleanup: %v", err)
+	}
+	assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
+	assertWorktreeReferenceStatus(t, store, wt.ID, StatusDeleted)
+}
+
+func TestCleanupWorktrees_PreservesReplacementAfterPostAuditPathSwap(t *testing.T) {
+	mgr, store := newReferenceCleanupTestManager(t)
+	seedReferenceCleanupSession(t, store, "task-path-swap", "session-path-swap", models.TaskSessionStateCompleted)
+	wt := createReferenceCleanupWorktree(t, mgr, "task-path-swap", "session-path-swap")
+	wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
+
+	mgr.SetRepositoryProvider(&fakeRepoProvider{repo: &Repository{ID: wt.RepositoryID, CleanupScript: "swap"}})
+	mgr.SetScriptMessageHandler(&swappingCleanupScriptHandler{})
+	if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err == nil {
+		t.Fatal("cleanup accepted a path replacement after audit")
+	}
+
+	replacement := filepath.Join(filepath.Dir(wt.Path), filepath.Base(wt.Path)+"-replacement")
+	if got, err := os.ReadFile(filepath.Join(wt.Path, "replacement.txt")); err != nil || string(got) != "preserve me\n" {
+		t.Fatalf("replacement path changed: contents=%q err=%v", got, err)
+	}
+	if _, err := os.Stat(replacement); err != nil {
+		t.Fatalf("renamed original worktree missing: %v", err)
+	}
+	assertCleanupBranchPresent(t, wt.RepositoryPath, wt.Branch)
+	assertWorktreeReferenceStatus(t, store, wt.ID, StatusActive)
+}
+
 func TestCleanupWorktrees_PartialFailureRemainsRetryable(t *testing.T) {
 	mgr, store := newReferenceCleanupTestManager(t)
 	seedReferenceCleanupSession(t, store, "task-retry", "session-retry", models.TaskSessionStateCompleted)
 	wt := createReferenceCleanupWorktree(t, mgr, "task-retry", "session-retry")
+	wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
 
 	if err := os.RemoveAll(wt.Path); err != nil {
 		t.Fatalf("remove worktree path without shared Git metadata: %v", err)
@@ -82,6 +174,7 @@ func TestCleanupWorktrees_IsIdempotentAfterVerifiedRemoval(t *testing.T) {
 	mgr, store := newReferenceCleanupTestManager(t)
 	seedReferenceCleanupSession(t, store, "task-idempotent", "session-idempotent", models.TaskSessionStateCompleted)
 	wt := createReferenceCleanupWorktree(t, mgr, "task-idempotent", "session-idempotent")
+	wt.CleanupHeadOID = strings.TrimSpace(runGit(t, wt.Path, "rev-parse", "HEAD"))
 
 	for attempt := 1; attempt <= 2; attempt++ {
 		if err := mgr.CleanupWorktrees(context.Background(), []*Worktree{wt}); err != nil {