Skip to content

[T-185C2E] Git Worktree#63

Open
weekly-daniel wants to merge 9 commits into
mainfrom
feature/t-185c2e-git-worktree
Open

[T-185C2E] Git Worktree#63
weekly-daniel wants to merge 9 commits into
mainfrom
feature/t-185c2e-git-worktree

Conversation

@weekly-daniel

Copy link
Copy Markdown
Collaborator

Summary

Replace per-task repository cloning with per-task Git worktrees and branches, while keeping planning in the configured repository checkout and updating cleanup/docs/tests around the new lifecycle.

Key Changes

  • server/src/orchestrator.js (replace clone-based workspace setup with git worktree creation/reuse/removal and move planning off task workspaces)
  • server/src/index.js (adjust startup and task cleanup logic so it treats the workspace root as a worktree root and only removes safe orphan worktrees)
  • server/src/config.js (keep existing configurable workspaceRoot behavior but update prompt/help text assumptions if they still describe cloned workspaces)
  • server/src/orchestrator.pr.test.js (cover PR flow against worktree-backed repositories and any new git raw calls)
  • server/src/orchestrator.delete.test.js (cover deletion/cleanup behavior for worktree-backed task directories)
  • server/src/orchestrator.test.js (add focused unit coverage for branch/worktree preparation and planner cwd changes)

Validation

  • server/src/orchestrator.test.js: planning uses task.repoPath directly and does not create a workspace before approval
  • server/src/orchestrator.test.js: implementation creates a branch-backed worktree under the configured workspace root and reuses it when valid
  • server/src/orchestrator.test.js: stale/mismatched task directories are removed and recreated as worktrees
  • server/src/orchestrator.pr.test.js: PR creation still fetches/rebases/pushes from the task worktree and manual-PR fallback remains intact
  • server/src/orchestrator.delete.test.js: deleting/completing/resetting tasks removes the worktree safely and clears workspacePath
  • server/src/index.test.js: startup orphan cleanup preserves active task worktrees and removes orphan task worktrees only
  • README.md / docs/task-flow.md: no automated test, but verify wording matches actual lifecycle

Review

  • Verdict: PASS
  • Summary: Changed files: server/src/orchestrator.js, server/src/index.js, server/src/config.js, server/src/orchestrator.test.js, server/src/orchestrator.pr.test.js, server/src/orchestrator.delete.test.js,
  • Minor issues: (Confidence 82) cleanupOrphanTaskWorktrees in server/src/index.js:140-153 deletes orphan directories with rmSync without attempting git worktree remove first. If git worktree list --porcelain failed for a; (Confidence 78) parseWorktreeList is duplicated with slightly different return types: index.js:89 returns a Set of paths, orchestrator.js:640 returns an Array of {path, branchRef} objects. Both parse the

Risks

  • git worktree add fails if the target branch already exists and is checked out in another worktree; the implementation needs a clear reuse/error path instead of forcing branch deletion.
  • Cleanup is more delicate than today because removing a worktree by deleting the directory can leave stale Git metadata behind; the plan should prefer git worktree remove and only force-delete as a fallback.
  • Planning in task.repoPath assumes configured repos are valid local Git checkouts; if remote URLs are still allowed anywhere in the UI, the implementation may need an explicit validation/error path instead of relying on legacy blocker recovery.
  • PR flow currently hardcodes main as the base branch; the worktree change should not expand scope into dependency-aware branching yet, but tests should make that constraint explicit.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates task execution from per-task repository clones to per-task Git worktrees + branches, while keeping planning in the configured repository checkout and updating cleanup logic, docs, and tests to match the new lifecycle.

Changes:

  • Replace clone-based workspace provisioning with Git worktree creation/reuse/removal in the orchestrator.
  • Update server startup + manual approval cleanup to treat the workspace root as a worktree root and remove only safe orphan task directories.
  • Expand/adjust tests and docs to reflect planning-in-repo and implementation-in-worktree behavior.

Reviewed changes

Copilot reviewed 10 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
server/src/orchestrator.js Introduces worktree lifecycle helpers, runs planning in repo checkout, and removes worktrees on task completion/cleanup paths.
server/src/index.js Makes max-review approval cleanup async via worktree removal; adds orphan task worktree cleanup at startup.
server/src/config.js Updates prompt/rules text to reflect implementation running in a task worktree.
server/src/orchestrator.test.js Adds unit coverage for planning cwd and worktree preparation/reuse behavior; checks updated prompt wording.
server/src/orchestrator.pr.test.js Verifies PR flow still pushes from the task worktree and removes the worktree on success.
server/src/orchestrator.delete.test.js Ensures deleting terminal tasks triggers worktree removal and clears workspacePath.
server/src/index.test.js Updates approval cleanup test to use orchestrator worktree removal; adds coverage for orphan cleanup behavior.
server/src/agents.test.js Adjusts path handling for URL-derived cwd strings.
docs/task-flow.md Updates lifecycle documentation to describe planning-in-repo and worktree-backed implementation/review.
README.md Updates user-facing docs to describe worktrees instead of per-task workspaces/clones.
package-lock.json Lockfile metadata updates.
server/package-lock.json Lockfile metadata updates.
client/package-lock.json Lockfile metadata updates.
Files not reviewed (2)
  • client/package-lock.json: Language not supported
  • server/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/src/orchestrator.js
Comment thread server/src/orchestrator.js
Comment thread server/src/index.js
Comment thread server/src/index.js Outdated
Comment thread docs/task-flow.md Outdated
…rage

- Move protected branch validation to top of prepareTaskWorktree (fail fast)
- Refactor removeTaskWorktree to use conditional instead of throw-sentinel
- Only clear workspacePath on successful removal (prevent orphaned worktrees)
- Add missing bus.emit('task:blocked') in startPlanning repoPath validation
- Make WebSocket handler async so approveMaxReviewBlocker is properly awaited
- Fix statusToStage mapping: workspace_setup belongs to implementation stage
- Remove unnecessary git worktree remove attempts on non-registered orphans
- Add dedicated worktree.test.js with 7 tests for shared parser
- Fix misleading test name for worktree teardown/recreation test

🐛 Daniel Stilero Eliasson
- Move workspace_setup from planning to implementation column in client
  KanbanBoard.jsx to match server statusToStage mapping and docs
- Add warning log when git worktree remove fails in prepareTaskWorktree
  instead of silently swallowing the error

🐛 Daniel Stilero Eliasson
- Update ReportsModal to count workspace_setup under implementation
- Change restartRecovery to map workspace_setup → queued (not backlog)
  since approved plans should not require re-planning after restart
- Remove redundant workspacePath: null from abortTask, resetTask, and
  buildMaxReviewBlockerApprovalUpdate — removeTaskWorktree handles this
  conditionally and callers should not override on failure
- Update tests to match new workspacePath clearing behavior

🐛 Daniel Stilero Eliasson
- Validate task.repoPath exists before using simpleGit (prevents
  silently operating on cwd when repoPath is null/empty)
- Add git worktree prune before deleteLocalBranch to clean stale
  worktree registrations left by failed git worktree remove + rm
  fallback, which would otherwise block branch deletion

🐛 Daniel Stilero Eliasson

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 20 changed files in this pull request and generated 3 comments.

Files not reviewed (2)
  • client/package-lock.json: Language not supported
  • server/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)

server/src/orchestrator.js:1231

  • deleteTask proceeds to remove the plan and delete the task record even if removeTaskWorktree fails to remove the underlying worktree directory/registration (and removeTaskWorktree does not currently return success/failure). This can leave behind an untracked worktree that startup cleanup may never remove because there’s no longer a task referencing it. Consider having removeTaskWorktree return a boolean (or throw) and making deleteTask fail/abort (or at least log and keep the task) when cleanup is unsuccessful.
  if (task.workspacePath) {
    await removeTaskWorktree(task);
  }

  store.removePlan(taskId);
  store.deleteTask(taskId);
  return true;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/src/worktree.js Outdated
Comment thread server/src/index.js
Comment thread server/src/workflow.js
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.

2 participants