From bf4e0cbb34e1962840e3220f02a8ee0b3895d9fe Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Sun, 9 Aug 2026 12:31:10 -0400 Subject: [PATCH 1/2] docs: let live registrations authorize workspace cleanup Workspaces created before ownership markers remain valid Git worktrees but cannot be deleted or retried after upgrading. The cleanup contract now treats an exact live registration in the persisted workspace repository as sufficient authority while retaining repository-identity and locked recheck protections. Generated with Codex (gpt-5.6-sol) Co-authored-by: Codex <198982749+openai-codex@users.noreply.github.com> --- ...orktree-registration-authorizes-cleanup.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 docs/adr/0004-live-worktree-registration-authorizes-cleanup.md 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..e1b1d5245 --- /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. The cleanup operation will recheck registration under the +repository lock before removing the worktree and managed branches. + +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 and ownership changes during cleanup remain + protected by repository identity and the locked registration recheck. From 225eb08a42ce6512c2f8a5153359279171c039da Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Sun, 9 Aug 2026 13:11:48 -0400 Subject: [PATCH 2/2] fix: delete workspaces created before ownership markers Workspaces created before ownership metadata was introduced remained valid Git worktrees but became impossible to delete or retry after upgrading. Treating the persisted repository's live registration as cleanup authority restores those lifecycle operations without requiring a migration or manual marker repair.\n\nSame-repository replacements at the persisted path follow the workspace lifecycle. Foreign replacements and symlink targets remain preserved; symlinked paths can be forgotten without stripping the target worktree's Git metadata.\n\nValidation: make test (6,294 tests, 11 skipped); make lint-check (0 issues); full internal/workspace and internal/server/workspacetest packages. Generated with Codex (gpt-5.6-sol) Co-authored-by: Codex <198982749+openai-codex@users.noreply.github.com> --- context/workspace-runtime-lifecycle.md | 4 + ...orktree-registration-authorizes-cleanup.md | 8 +- .../workspace_clone_safety_test.go | 74 ++++++-------- internal/workspace/manager.go | 28 ++---- internal/workspace/manager_test.go | 98 ++++++------------- 5 files changed, 78 insertions(+), 134 deletions(-) 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 index e1b1d5245..cfc208963 100644 --- a/docs/adr/0004-live-worktree-registration-authorizes-cleanup.md +++ b/docs/adr/0004-live-worktree-registration-authorizes-cleanup.md @@ -19,8 +19,8 @@ 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. The cleanup operation will recheck registration under the -repository lock before removing the worktree and managed branches. +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 @@ -32,5 +32,5 @@ have appeared during a failed setup. 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 and ownership changes during cleanup remain - protected by repository identity and the locked registration recheck. +- 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) {