diff --git a/context/workspace-runtime-lifecycle.md b/context/workspace-runtime-lifecycle.md index 1caecf343..7dfa6f897 100644 --- a/context/workspace-runtime-lifecycle.md +++ b/context/workspace-runtime-lifecycle.md @@ -62,6 +62,10 @@ Workspace deletion is intentionally conservative. - Only after a clean preflight may runtime sessions and shells be stopped. - Only after runtime shutdown succeeds should destructive worktree and DB teardown continue. +- A live worktree registration at the persisted path in the resolved repository + authorizes cleanup without an ownership marker; a same-repository replacement + is the workspace, but a symlink to another worktree is not. + (`internal/workspace/manager.go::gitDirOwnsCleanupWorktree`) - A confirmed delete publishes workspace absence from the application workflow before presenter-specific navigation or failure UI; releasing the initiating presenter must not suppress tombstones, hosted-session cleanup, or route invalidation (`frontend/src/lib/components/terminal/workspace-runtime-workflow.ts::executeMutation`). diff --git a/docs/adr/0004-live-worktree-registration-authorizes-cleanup.md b/docs/adr/0004-live-worktree-registration-authorizes-cleanup.md new file mode 100644 index 000000000..cfc208963 --- /dev/null +++ b/docs/adr/0004-live-worktree-registration-authorizes-cleanup.md @@ -0,0 +1,36 @@ +# ADR 0004: Live Worktree Registration Authorizes Cleanup + +## Status + +Accepted + +## Context + +kenn-forge writes an ownership marker into linked-worktree metadata when it +creates a workspace. Workspaces created before that marker was introduced have +valid persisted records and live Git registrations but cannot be deleted or +retried because cleanup treats the missing marker as unproven ownership. + +## Decision + +An exact live linked-worktree registration in the repository resolved for the +persisted workspace is sufficient authority for cleanup. The ownership marker +is not required for workspace deletion or retry cleanup. + +Cleanup will still preserve a checkout that belongs to another repository, a +standalone replacement clone, or a path no longer registered by the resolved +repository. A same-repository live registration at the exact persisted path is +treated as the workspace even if it appeared after cleanup planning. + +Ownership markers remain part of worktree creation and rollback, where they +distinguish the generation kenn-forge just created from a replacement that may +have appeared during a failed setup. + +## Consequences + +- Pre-marker workspaces are deletable without migration or manual metadata + repair. +- A manually created linked worktree at the exact persisted workspace path in + the expected repository is treated as the workspace and can be deleted. +- Replacement repositories remain protected by repository identity; a + same-repository replacement at the persisted path is deleted. diff --git a/internal/server/workspacetest/workspace_clone_safety_test.go b/internal/server/workspacetest/workspace_clone_safety_test.go index 651914b75..99dcb4a40 100644 --- a/internal/server/workspacetest/workspace_clone_safety_test.go +++ b/internal/server/workspacetest/workspace_clone_safety_test.go @@ -358,7 +358,7 @@ func TestWorkspaceForceDeletePreservesForeignLinkedWorktreeAfterManagedPruneE2E( ) } -func TestWorkspaceForceDeleteRejectsSameRepoReplacementAfterManagedPruneE2E( +func TestWorkspaceForceDeleteRemovesSameRepoReplacementAfterManagedPruneE2E( t *testing.T, ) { t.Parallel() @@ -394,23 +394,14 @@ func TestWorkspaceForceDeleteRejectsSameRepoReplacementAfterManagedPruneE2E( ) require.NoError(err) require.Equal( - http.StatusConflict, deleteResp.StatusCode(), string(deleteResp.Body), + http.StatusNoContent, deleteResp.StatusCode(), string(deleteResp.Body), ) stored, err := fixture.database.GetWorkspace(ctx, ws.Id) require.NoError(err) - require.NotNil(stored) - assert.Equal(ws.Id, stored.ID) - require.FileExists(filepath.Join(worktreePath, ".git")) - contents, err := os.ReadFile(filepath.Join(worktreePath, "base.txt")) - require.NoError(err) - assert.Equal("foreign dirty data\n", string(contents)) - assert.Equal(foreignHead, testGitSHA(t, worktreePath, "HEAD")) - assert.Contains( - workspaceGitOutput(t, worktreePath, "status", "--porcelain"), - "M base.txt", - ) - assert.Contains( + assert.Nil(stored) + require.NoDirExists(worktreePath) + assert.NotContains( workspaceGitOutput(t, fixture.bare, "worktree", "list", "--porcelain"), worktreePath, ) @@ -420,7 +411,7 @@ func TestWorkspaceForceDeleteRejectsSameRepoReplacementAfterManagedPruneE2E( ) } -func TestWorkspaceForceDeleteRejectsPreMarkerWorkspaceAfterUpgradeE2E( +func TestWorkspaceForceDeleteRemovesPreMarkerWorkspaceAfterUpgradeE2E( t *testing.T, ) { t.Parallel() @@ -450,26 +441,18 @@ func TestWorkspaceForceDeleteRejectsPreMarkerWorkspaceAfterUpgradeE2E( ) require.NoError(err) require.Equal( - http.StatusConflict, deleteResp.StatusCode(), string(deleteResp.Body), + http.StatusNoContent, deleteResp.StatusCode(), string(deleteResp.Body), ) stored, err := fixture.database.GetWorkspace(ctx, ws.Id) require.NoError(err) - require.NotNil(stored) - assert.Equal(ws.Id, stored.ID) - require.FileExists(filepath.Join(worktreePath, ".git")) - assert.Equal( - "kenn-forge/pr-1", - workspaceGitOutput(t, worktreePath, "branch", "--show-current"), - ) - assert.Contains( + assert.Nil(stored) + require.NoDirExists(worktreePath) + assert.NotContains( workspaceGitOutput(t, fixture.bare, "worktree", "list", "--porcelain"), worktreePath, ) - assert.Equal( - testGitSHA(t, worktreePath, "HEAD"), - testGitSHA(t, fixture.bare, "refs/heads/kenn-forge/pr-1"), - ) + requireGitRefMissing(t, fixture.bare, "refs/heads/kenn-forge/pr-1") } func TestWorkspaceForceDeleteRetainsLockedWorktreeE2E(t *testing.T) { @@ -528,7 +511,7 @@ func TestWorkspaceForceDeleteRetainsLockedWorktreeE2E(t *testing.T) { assert.Nil(stored) } -func TestWorkspaceForceDeleteRejectsSymlinkToSameRepoWorktreeE2E( +func TestWorkspaceForceDeleteForgetsSymlinkToSameRepoWorktreeE2E( t *testing.T, ) { t.Parallel() @@ -561,13 +544,12 @@ func TestWorkspaceForceDeleteRejectsSymlinkToSameRepoWorktreeE2E( ) require.NoError(err) require.Equal( - http.StatusConflict, deleteResp.StatusCode(), string(deleteResp.Body), + http.StatusNoContent, deleteResp.StatusCode(), string(deleteResp.Body), ) stored, err := fixture.database.GetWorkspace(ctx, ws.Id) require.NoError(err) - require.NotNil(stored) - assert.Equal(ws.Id, stored.ID) + assert.Nil(stored) pathInfo, err := os.Lstat(worktreePath) require.NoError(err) assert.NotZero(pathInfo.Mode() & os.ModeSymlink) @@ -644,7 +626,7 @@ func TestWorkspaceCreateRejectsSymlinkedReusableWorktreeE2E(t *testing.T) { assert.ErrorIs(err, os.ErrNotExist) } -func TestWorkspaceRetryRejectsPreMarkerWorkspaceAfterUpgradeE2E( +func TestWorkspaceRetryAcceptsPreMarkerWorkspaceAfterUpgradeE2E( t *testing.T, ) { t.Parallel() @@ -656,6 +638,7 @@ func TestWorkspaceRetryRejectsPreMarkerWorkspaceAfterUpgradeE2E( ctx := t.Context() ws := createReadyWorkspace(t, ctx, fixture.client) + branch := workspaceGitOutput(t, ws.WorktreePath, "branch", "--show-current") metadataDir := workspaceGitOutput( t, ws.WorktreePath, "rev-parse", "--path-format=absolute", "--git-dir", @@ -669,20 +652,23 @@ func TestWorkspaceRetryRejectsPreMarkerWorkspaceAfterUpgradeE2E( retryResp, err := fixture.client.HTTP.RetryWorkspaceWithResponse(ctx, ws.Id) require.NoError(err) require.Equal( - http.StatusConflict, retryResp.StatusCode(), string(retryResp.Body), + http.StatusAccepted, retryResp.StatusCode(), string(retryResp.Body), ) - stored, err := fixture.database.GetWorkspace(ctx, ws.Id) - require.NoError(err) - require.NotNil(stored) - assert.Equal("error", stored.Status) - require.NotNil(stored.ErrorMessage) - assert.NotEmpty(*stored.ErrorMessage) - require.FileExists(filepath.Join(ws.WorktreePath, ".git")) - assert.Contains( - workspaceGitOutput(t, fixture.bare, "worktree", "list", "--porcelain"), - ws.WorktreePath, + ready := waitForWorkspaceReady(t, ctx, fixture.client, ws.Id) + assert.Equal(branch, workspaceGitOutput( + t, ready.WorktreePath, "branch", "--show-current", + )) + require.FileExists(filepath.Join(ready.WorktreePath, ".git")) + newMetadataDir := workspaceGitOutput( + t, ready.WorktreePath, + "rev-parse", "--path-format=absolute", "--git-dir", ) + marker, err := os.ReadFile(filepath.Join( + newMetadataDir, "kenn-forge-workspace-id", + )) + require.NoError(err) + assert.Equal(ws.Id+"\n", string(marker)) } func TestWorkspaceRetryCleansStalePreMarkerRegistrationE2E(t *testing.T) { diff --git a/internal/workspace/manager.go b/internal/workspace/manager.go index 3c59831d3..f478892ad 100644 --- a/internal/workspace/manager.go +++ b/internal/workspace/manager.go @@ -3375,6 +3375,13 @@ func worktreeRegistrationMetadataDir( func gitDirHasStaleWorktreeRegistration( ctx context.Context, gitDir, worktreePath string, ) (bool, error) { + info, err := os.Lstat(worktreePath) + if err == nil && info.Mode()&os.ModeSymlink != 0 { + return false, nil + } + if err != nil && !errors.Is(err, os.ErrNotExist) { + return false, fmt.Errorf("stat workspace path: %w", err) + } tracked, err := gitDirTracksWorktreePath(ctx, gitDir, worktreePath) if err != nil || !tracked { return false, err @@ -3396,18 +3403,7 @@ func gitDirHasLiveWorktree( if err != nil { return false, fmt.Errorf("stat workspace path: %w", err) } - isDirectory := info.IsDir() - if info.Mode()&os.ModeSymlink != 0 { - targetInfo, targetErr := os.Stat(worktreePath) - if errors.Is(targetErr, os.ErrNotExist) { - return false, nil - } - if targetErr != nil { - return false, fmt.Errorf("stat workspace symlink target: %w", targetErr) - } - isDirectory = targetInfo.IsDir() - } - if !isDirectory { + if !info.IsDir() || info.Mode()&os.ModeSymlink != 0 { return false, nil } isRoot, err := worktreePathIsRoot(ctx, worktreePath) @@ -3486,13 +3482,7 @@ func gitDirOwnsCleanupWorktree( if err != nil || owned { return owned, err } - live, err := gitDirHasLiveWorktree(ctx, gitDir, worktreePath) - if err != nil || !live { - return false, err - } - return false, fmt.Errorf( - "%w: %s", ErrWorkspaceOwnershipUnproven, worktreePath, - ) + return gitDirHasLiveWorktree(ctx, gitDir, worktreePath) } func gitDirOwnsLinkedWorktree( diff --git a/internal/workspace/manager_test.go b/internal/workspace/manager_test.go index 042b83f4e..a2bd4fd67 100644 --- a/internal/workspace/manager_test.go +++ b/internal/workspace/manager_test.go @@ -2741,23 +2741,21 @@ func TestCleanupPreservesExistingWorktreeWhenConfiguredBaseChanges(t *testing.T) assert.True(wrongExists, "cleanup must not delete branch from current settings repo") } -func TestCleanupDoesNotTrustReplacementCloneAtWorkspacePath(t *testing.T) { - assert := assert.New(t) +func TestCleanupDeletesLiveRegisteredWorktreeWithoutOwnershipMarker(t *testing.T) { require := require.New(t) const branch = "kenn-forge/pr-42" - _, remote := setupLocalWorktreeBaseWithRemoteForWorkspaceGitTest(t, "feature/thing") + localRepo := setupLocalWorktreeBaseForWorkspaceGitTest(t, "feature/thing") worktreePath := filepath.Join(t.TempDir(), "workspace") - runWorkspaceTestGit(t, t.TempDir(), "clone", remote, worktreePath) runWorkspaceTestGit( - t, worktreePath, "remote", "set-url", "origin", - "https://github.com/acme/widget.git", + t, localRepo, + "worktree", "add", worktreePath, "-b", branch, "HEAD", ) - runWorkspaceTestGit(t, worktreePath, "branch", branch, "HEAD") mgr := NewManager(openTestDB(t), t.TempDir()) + mgr.SetWorktreeBasePathResolver(staticBaseResolver(localRepo)) ws := &Workspace{ - ID: "ws-replaced-clone", + ID: "ws-unmarked-live-registration", Platform: "github", PlatformHost: "github.com", RepoOwner: "acme", @@ -2769,30 +2767,20 @@ func TestCleanupDoesNotTrustReplacementCloneAtWorkspacePath(t *testing.T) { WorktreePath: worktreePath, } - gitDir, ok, err := mgr.workspaceCleanupGitDir(t.Context(), ws) - - require.NoError(err) - assert.False(ok) - assert.Empty(gitDir) - branchExists, err := localBranchExists(t.Context(), worktreePath, branch) + require.NoError(mgr.cleanupWorkspaceArtifactsForDelete(t.Context(), ws)) + require.NoDirExists(worktreePath) + branchExists, err := localBranchExists(t.Context(), localRepo, branch) require.NoError(err) - assert.True(branchExists) + require.False(branchExists) } -func TestCleanupDoesNotTrustStaleLocalBaseRegistrationForReplacementClone(t *testing.T) { +func TestCleanupDoesNotTrustReplacementCloneAtWorkspacePath(t *testing.T) { assert := assert.New(t) require := require.New(t) const branch = "kenn-forge/pr-42" - localRepo, remote := setupLocalWorktreeBaseWithRemoteForWorkspaceGitTest( - t, "feature/thing", - ) + _, remote := setupLocalWorktreeBaseWithRemoteForWorkspaceGitTest(t, "feature/thing") worktreePath := filepath.Join(t.TempDir(), "workspace") - runWorkspaceTestGit( - t, localRepo, - "worktree", "add", worktreePath, "-b", branch, "HEAD", - ) - require.NoError(os.RemoveAll(worktreePath)) runWorkspaceTestGit(t, t.TempDir(), "clone", remote, worktreePath) runWorkspaceTestGit( t, worktreePath, "remote", "set-url", "origin", @@ -2801,9 +2789,8 @@ func TestCleanupDoesNotTrustStaleLocalBaseRegistrationForReplacementClone(t *tes runWorkspaceTestGit(t, worktreePath, "branch", branch, "HEAD") mgr := NewManager(openTestDB(t), t.TempDir()) - mgr.SetWorktreeBasePathResolver(staticBaseResolver(localRepo)) ws := &Workspace{ - ID: "ws-stale-local-base-replaced-clone", + ID: "ws-replaced-clone", Platform: "github", PlatformHost: "github.com", RepoOwner: "acme", @@ -2823,16 +2810,14 @@ func TestCleanupDoesNotTrustStaleLocalBaseRegistrationForReplacementClone(t *tes branchExists, err := localBranchExists(t.Context(), worktreePath, branch) require.NoError(err) assert.True(branchExists) - _, err = os.Stat(worktreePath) - require.NoError(err) } -func TestCleanupDeleteLockedRevalidatesOwnershipAfterPlanRace(t *testing.T) { +func TestCleanupDoesNotTrustStaleLocalBaseRegistrationForReplacementClone(t *testing.T) { assert := assert.New(t) require := require.New(t) const branch = "kenn-forge/pr-42" - localRepo, _ := setupLocalWorktreeBaseWithRemoteForWorkspaceGitTest( + localRepo, remote := setupLocalWorktreeBaseWithRemoteForWorkspaceGitTest( t, "feature/thing", ) worktreePath := filepath.Join(t.TempDir(), "workspace") @@ -2840,11 +2825,18 @@ func TestCleanupDeleteLockedRevalidatesOwnershipAfterPlanRace(t *testing.T) { t, localRepo, "worktree", "add", worktreePath, "-b", branch, "HEAD", ) + require.NoError(os.RemoveAll(worktreePath)) + runWorkspaceTestGit(t, t.TempDir(), "clone", remote, worktreePath) + runWorkspaceTestGit( + t, worktreePath, "remote", "set-url", "origin", + "https://github.com/acme/widget.git", + ) + runWorkspaceTestGit(t, worktreePath, "branch", branch, "HEAD") mgr := NewManager(openTestDB(t), t.TempDir()) mgr.SetWorktreeBasePathResolver(staticBaseResolver(localRepo)) ws := &Workspace{ - ID: "ws-delete-lock-race", + ID: "ws-stale-local-base-replaced-clone", Platform: "github", PlatformHost: "github.com", RepoOwner: "acme", @@ -2855,45 +2847,17 @@ func TestCleanupDeleteLockedRevalidatesOwnershipAfterPlanRace(t *testing.T) { WorkspaceBranch: branch, WorktreePath: worktreePath, } - require.NoError(writeWorkspaceOwnershipMarker(t.Context(), localRepo, ws)) - gitDir, owned, err := mgr.workspaceCleanupGitDir(t.Context(), ws) - require.NoError(err) - require.True(owned) - require.NotEmpty(gitDir) - require.NoError(os.RemoveAll(worktreePath)) - runWorkspaceTestGit(t, localRepo, "worktree", "prune") - runWorkspaceTestGit( - t, localRepo, - "worktree", "add", worktreePath, - "-b", "foreign/race-replacement", "HEAD", - ) - require.NoError(os.WriteFile( - filepath.Join(worktreePath, "foreign.txt"), []byte("keep me\n"), 0o644, - )) - foreignHead := strings.TrimSpace(string( - runWorkspaceTestGit(t, worktreePath, "rev-parse", "HEAD"), - )) - - err = mgr.cleanupWorkspaceArtifactsForDeleteLocked( - t.Context(), gitDir, ws, - ) + gitDir, ok, err := mgr.workspaceCleanupGitDir(t.Context(), ws) - require.ErrorIs(err, ErrWorkspaceOwnershipUnproven) - require.FileExists(filepath.Join(worktreePath, ".git")) - contents, err := os.ReadFile(filepath.Join(worktreePath, "foreign.txt")) require.NoError(err) - assert.Equal("keep me\n", string(contents)) - assert.Equal( - foreignHead, - strings.TrimSpace(string( - runWorkspaceTestGit(t, worktreePath, "rev-parse", "HEAD"), - )), - ) - assert.Contains( - string(runWorkspaceTestGit(t, localRepo, "worktree", "list", "--porcelain")), - worktreePath, - ) + assert.False(ok) + assert.Empty(gitDir) + branchExists, err := localBranchExists(t.Context(), worktreePath, branch) + require.NoError(err) + assert.True(branchExists) + _, err = os.Stat(worktreePath) + require.NoError(err) } func TestCleanupIgnoresInvalidConfiguredBaseWhenWorktreeAbsent(t *testing.T) {