Skip to content

[WAT-1777] Guard Paperclip worktree cleanup lifecycle#4437

Open
willblanchard wants to merge 3 commits intopaperclipai:masterfrom
willblanchard:paperclip/wat-1777-large-worktree-lifecycle
Open

[WAT-1777] Guard Paperclip worktree cleanup lifecycle#4437
willblanchard wants to merge 3 commits intopaperclipai:masterfrom
willblanchard:paperclip/wat-1777-large-worktree-lifecycle

Conversation

@willblanchard
Copy link
Copy Markdown

@willblanchard willblanchard commented Apr 24, 2026

Thinking Path

  • Paperclip orchestrates AI agents for zero-human companies.
  • Paperclip issue execution can create isolated git worktrees so agents can work without mutating the primary checkout.
  • Large repositories make worktree lifecycle policy important because stale or unsafe deletion can consume disk or destroy active work.
  • Existing cleanup paths removed runtime-created worktrees, but did not fully report why unsafe worktrees should be left in place.
  • This pull request documents the lifecycle policy and makes close/cleanup report dirty, untracked, unmerged, or active worktrees instead of deleting them.
  • A Greptile follow-up tightened branch cleanup so branches are deleted only after the git worktree removal actually succeeds.
  • The benefit is safer Paperclip workspace cleanup with clearer operator evidence for large-repo workspaces.

What Changed

  • Documented large-repo worktree lifecycle policy for Paperclip issue/review workspaces.
  • Added close-readiness blocking reasons for dirty, untracked, or unmerged isolated git worktrees.
  • Made runtime cleanup report dirty/uninspectable git worktrees instead of deleting them.
  • Gated runtime-created branch deletion so it only runs after the associated git worktree was actually removed.
  • Added regression coverage for blocked close behavior, cleanup reporting, and the dirty-worktree branch-deletion warning case.

Verification

  • pnpm vitest run server/src/__tests__/execution-workspaces-service.test.ts server/src/__tests__/workspace-runtime.test.ts
  • pnpm --filter @paperclipai/server typecheck
  • git diff --check origin/master...HEAD
  • Red/green regression check: pnpm vitest run server/src/__tests__/workspace-runtime.test.ts -t "reports dirty git worktrees instead of removing them during cleanup" failed before the branch-deletion gate and passed after the fix.

Risks

  • Low risk: the cleanup behavior is more conservative for git worktrees and only suppresses branch deletion when worktree removal did not happen.
  • Unmerged runtime-created branches are still preserved and reported through the existing git branch -d warning path after a successful worktree removal.
  • No schema, API, or UI changes.

For core feature work, check ROADMAP.md first and discuss it in #dev before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See CONTRIBUTING.md.

Model Used

  • OpenAI Codex based on GPT-5 via the Paperclip local agent adapter, with shell/git/GitHub CLI tool use and medium reasoning effort.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have specified the model used (with version and capability details)
  • I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots
  • I have updated relevant documentation to reflect my changes
  • I have considered and documented any risks above
  • I will address all Greptile and reviewer comments before requesting merge

Paperclip

  • WAT-1777
  • WAT-1842

@willblanchard willblanchard force-pushed the paperclip/wat-1777-large-worktree-lifecycle branch from e44101d to 5f18885 Compare April 24, 2026 21:07
@willblanchard willblanchard marked this pull request as ready for review April 24, 2026 23:54
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 24, 2026

Greptile Summary

This PR guards the isolated git worktree close/cleanup lifecycle by adding blockingReasons for dirty, untracked, or unmerged worktrees in getCloseReadiness, and by checking git status --porcelain before removing a worktree in cleanupExecutionWorkspaceArtifacts. Regression tests and lifecycle policy documentation are included.

  • The git branch -d block in cleanupExecutionWorkspaceArtifacts runs unconditionally even when the worktree removal was skipped due to dirty files — this produces a spurious "Cannot delete branch checked out at…" warning alongside the correctly-generated dirty-worktree warning (see inline comment).
  • The PR description does not follow the required template from CONTRIBUTING.md — it is missing the Thinking Path, What Changed, Risks, Model Used, and Checklist sections. Please fill out the full template before merging.

Confidence Score: 4/5

Safe to merge after addressing the branch-deletion ordering bug and completing the PR description template.

One P1 logic defect exists: the branch deletion runs unconditionally after a skipped worktree removal, producing a misleading second warning. The PR also doesn't follow the required CONTRIBUTING.md template. Both should be resolved before merge.

server/src/services/workspace-runtime.ts — branch deletion gating after dirty-worktree skip.

Important Files Changed

Filename Overview
server/src/services/workspace-runtime.ts Adds git-status guard before worktree removal (correct), but branch deletion is not gated on worktree removal success, producing spurious warnings for dirty worktrees.
server/src/services/execution-workspaces.ts Cleanly adds blockingReasons for dirty/untracked/unmerged git worktrees, correctly drives isDestructiveCloseAllowed=false and state="blocked".
server/src/tests/execution-workspaces-service.test.ts Test expectations correctly updated to assert state="blocked", isDestructiveCloseAllowed=false, and the new blockingReasons messages.
server/src/tests/workspace-runtime.test.ts New regression test for dirty-worktree report-only behavior; verifies cleanup.cleaned=false and the file is not removed. Solid coverage.
doc/experimental/issue-worktree-support.md Adds comprehensive lifecycle policy documentation for large-repo worktrees; well-structured and consistent with the implementation.

Comments Outside Diff (1)

  1. server/src/services/workspace-runtime.ts, line 1414-1437 (link)

    P1 Branch deletion runs unconditionally when worktree removal is skipped

    When gitStatus is non-empty (dirty) or null (inspection failed), the worktree removal is correctly skipped. However, the git branch -d block sits outside the dirty-check and runs unconditionally for any git_worktree workspace with createdByRuntime && branchName. When the branch is still checked out in the un-removed dirty worktree, git branch -d will fail with "Cannot delete branch 'X' checked out at '...'" — producing a second, misleading warning on top of the already-correct dirty-worktree warning. Callers inspecting cleanup.warnings see two warnings where the branch-deletion one obscures the real cause (dirty files, not an unmerged branch).

    Fix: track whether the worktree was actually removed and gate branch deletion on it:

    let worktreeWasRemoved = false;
    if (gitStatus && gitStatus.length > 0) {
      warnings.push(`Skipped removing git worktree "${workspacePath}" because it has uncommitted or untracked files.`);
    } else if (gitStatus === "") {
      try {
        await recordGitOperation(...);
        worktreeWasRemoved = true;
      } catch (err) {
        warnings.push(err instanceof Error ? err.message : String(err));
      }
    }
    // further down:
    if (createdByRuntime && input.workspace.branchName && worktreeWasRemoved) {
      // try git branch -d
    }
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: server/src/services/workspace-runtime.ts
    Line: 1414-1437
    
    Comment:
    **Branch deletion runs unconditionally when worktree removal is skipped**
    
    When `gitStatus` is non-empty (dirty) or `null` (inspection failed), the worktree removal is correctly skipped. However, the `git branch -d` block sits outside the dirty-check and runs unconditionally for any `git_worktree` workspace with `createdByRuntime && branchName`. When the branch is still checked out in the un-removed dirty worktree, `git branch -d` will fail with "Cannot delete branch 'X' checked out at '...'" — producing a second, misleading warning on top of the already-correct dirty-worktree warning. Callers inspecting `cleanup.warnings` see two warnings where the branch-deletion one obscures the real cause (dirty files, not an unmerged branch).
    
    Fix: track whether the worktree was actually removed and gate branch deletion on it:
    ```typescript
    let worktreeWasRemoved = false;
    if (gitStatus && gitStatus.length > 0) {
      warnings.push(`Skipped removing git worktree "${workspacePath}" because it has uncommitted or untracked files.`);
    } else if (gitStatus === "") {
      try {
        await recordGitOperation(...);
        worktreeWasRemoved = true;
      } catch (err) {
        warnings.push(err instanceof Error ? err.message : String(err));
      }
    }
    // further down:
    if (createdByRuntime && input.workspace.branchName && worktreeWasRemoved) {
      // try git branch -d
    }
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: server/src/services/workspace-runtime.ts
Line: 1414-1437

Comment:
**Branch deletion runs unconditionally when worktree removal is skipped**

When `gitStatus` is non-empty (dirty) or `null` (inspection failed), the worktree removal is correctly skipped. However, the `git branch -d` block sits outside the dirty-check and runs unconditionally for any `git_worktree` workspace with `createdByRuntime && branchName`. When the branch is still checked out in the un-removed dirty worktree, `git branch -d` will fail with "Cannot delete branch 'X' checked out at '...'" — producing a second, misleading warning on top of the already-correct dirty-worktree warning. Callers inspecting `cleanup.warnings` see two warnings where the branch-deletion one obscures the real cause (dirty files, not an unmerged branch).

Fix: track whether the worktree was actually removed and gate branch deletion on it:
```typescript
let worktreeWasRemoved = false;
if (gitStatus && gitStatus.length > 0) {
  warnings.push(`Skipped removing git worktree "${workspacePath}" because it has uncommitted or untracked files.`);
} else if (gitStatus === "") {
  try {
    await recordGitOperation(...);
    worktreeWasRemoved = true;
  } catch (err) {
    warnings.push(err instanceof Error ? err.message : String(err));
  }
}
// further down:
if (createdByRuntime && input.workspace.branchName && worktreeWasRemoved) {
  // try git branch -d
}
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "docs: cover non-execution worktree lifec..." | Re-trigger Greptile

@willblanchard
Copy link
Copy Markdown
Author

@Praviwwattshift please review PR #4437 for WAT-1777 when you have a slot. Current state: all visible checks are green (policy, verify, e2e, Snyk, Greptile), the PR is mergeable against master, and it is no longer draft.

One thing to call out before merge: Greptile flagged a P1 branch-deletion ordering issue in server/src/services/workspace-runtime.ts plus a PR-template gap. I do not want to merge this until the owner either fixes those or you explicitly accept them.

@willblanchard
Copy link
Copy Markdown
Author

Addressed the Greptile P1 branch-deletion ordering issue in commit 9f6cca49: branch deletion now only runs after git worktree remove succeeds, and the dirty-worktree regression asserts only the intended warning is emitted. Also replaced the PR body with the required CONTRIBUTING template sections.\n\nLocal verification run on the pushed head:\n- pnpm vitest run server/src/__tests__/execution-workspaces-service.test.ts server/src/__tests__/workspace-runtime.test.ts\n- pnpm --filter @paperclipai/server typecheck\n- git diff --check origin/master...HEAD\n\nFresh GitHub checks restarted for 9f6cca49; policy and Snyk have passed, verify/e2e were still running at last readback.

@willblanchard
Copy link
Copy Markdown
Author

@Praviwwattshift update: the Greptile P1/template blocker called out above has been fixed on current head 9f6cca496d51fe48c8e041bbb766eb67e77b8589. Current readback: PR is non-draft, mergeable/CLEAN, and policy, verify, e2e, and Snyk are green.

I attempted the formal review request and merge from this adapter, but GitHub denies both RequestReviewsByLogin and MergePullRequest for the active account/integration. Please review and merge PR #4437 when you can, or confirm the maintainer path you want us to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant