fix: Existing Folder workflow reuses local repo instead of bare clone

[PureWeen]

Problem

When a user adds an existing repo via Existing Folder (e.g. their dotnet/maui clone), PolyPilot:

1. Created a bare clone of the entire repo at ~/.polypilot/repos/ (redundant - user already has it)
2. Created nested worktree checkouts inside the user's repo at .polypilot/worktrees/{branch}/
3. The user's existing checkout was never used for sessions

For huge repos like MAUI (~5GB), this wasted significant disk space and time.

Additionally, when both a URL-based repo and a local folder pointed to the same remote, both incorrectly used the same BareClonePath, causing the local folder to be ignored.

Changes

Skip bare clone for Existing Folder repos

• AddRepositoryFromLocalAsync now points BareClonePath directly at the user's existing repo instead of creating a redundant bare clone
• EnsureRepoCloneInCurrentRootAsync detects non-bare repos (.git dir/file) and skips clone management

Separate repos for URL vs local folder

• When a URL-based repo already exists, adding the same repo from a local folder creates a separate RepositoryInfo with a distinct ID ({baseId}-local-{pathHash})
• The original URL-based repo's BareClonePath is never overwritten
• Idempotent: adding the same local folder twice returns the same repo

Validate bare clone before reuse

• Corrupt or partial bare clone directories are detected and removed before re-cloning

Reuse existing checkout for same-branch sessions

• CreateWorktreeAsync checks for an existing registered worktree on the requested branch before creating a new one
• Only matches centralized worktrees (under ~/.polypilot/worktrees/), never external user checkouts

Remove nested worktree strategy

• All worktrees now go to the centralized ~/.polypilot/worktrees/ directory
• Removed localPath parameter from CreateWorktreeAsync, CreateSessionWithWorktreeAsync, and all UI callers

Bug fixes

• Fixed Path.Combine producing backslashes for Unix-style path in BuildContinuationTranscript
• Fixed FindActiveLockPid process name filter to include testhost
• Fixed repo picker to show full repo name instead of last dash-segment ([Bug Report] When trying to select a new session from a repo, the picker will show just the l #570)
• Fixed existing repo and group name migration to URL-derived format ([Bug Report] When trying to select a new session from a repo, the picker will show just the l #570)

Orchestrator image dispatch fix (cherry-picked from PR #590)

• Orchestrator messages now always route through the dispatch pipeline even with image attachments
• Previously, image attachments caused the dispatch pipeline to be bypassed entirely

Testing

All 3433 tests pass (12 pre-existing ExternalSessionScannerTests failures due to missing node).

Test coverage includes:

• 11 AddExistingRepoTests (local folder behavior, URL vs local separation, idempotency, validation errors, ID format, reconciliation)
• 43 RepoManagerTests (URL/ID parsing, clone management, worktree reuse, git exclude entries)
• 34 WorktreeStrategyTests (strategy behaviors, worktree creation)

[PureWeen]

PR #527 Code Review

Focus: regressions, bugs, data loss, race conditions. No style comments.

---

🔴 Critical — RemoveRepositoryAsync can delete the user's real project

File: RepoManager.cs (the deleteFromDisk path in RemoveRepositoryAsync)

if (deleteFromDisk && Directory.Exists(repo.BareClonePath))
    try { Directory.Delete(repo.BareClonePath, recursive: true); } catch { }

When a repo was added via "Existing Folder", BareClonePath now points to the user's actual working directory (e.g. ~/projects/my-app). This will recursively delete the user's entire project. The guard in RemoveWorktreeAsync that checks against managed paths does NOT protect this separate deletion site.

Fix: Before deleting, check whether BareClonePath is under the managed ReposDir:

bool isManagedBareClone = repo.BareClonePath.StartsWith(ReposDir, StringComparison.OrdinalIgnoreCase);
if (deleteFromDisk && isManagedBareClone && Directory.Exists(repo.BareClonePath))
    try { Directory.Delete(repo.BareClonePath, recursive: true); } catch { }

---

🔴 High — Existing bare clone silently orphaned when same repo added via folder

File: RepoManager.cs, AddRepositoryFromLocalAsync

existing.BareClonePath = localPath;  // overwrites e.g. ~/.polypilot/repos/owner-repo.git

If the user previously added the same repo via URL (creating a managed bare clone), then adds it again via "Existing Folder", the bare clone at ~/.polypilot/repos/ is orphaned and never cleaned up. Worse, all existing worktrees created from that bare clone become disconnected — their .git files still reference the old bare clone's worktrees/ directory, but future git fetch, git worktree list, and git branch -D calls now run against the local path instead.

Fix: When existing.BareClonePath is non-null and points to a managed path (under ReposDir), either keep using it or explicitly clean up the old bare clone before overwriting.

---

🟠 High — BackfillWorktreeClonePaths propagates non-bare path to pre-existing worktrees

File: RepoManager.cs, called from AddRepositoryFromLocalAsync after setting BareClonePath = localPath

BackfillWorktreeClonePaths overwrites BareClonePath on every worktree that has a blank value. Worktrees created before this re-add get their BareClonePath silently redirected to the user's non-bare repo. When RemoveWorktreeAsync subsequently runs git worktree remove using this path, it operates on the user's local repo — potentially affecting worktrees the user manages outside of PolyPilot.

---

🟠 High — git worktree add on non-bare repo fails when branch is already checked out

File: RepoManager.cs, CreateWorktreeAsync

git worktree add errors with fatal: '<branch>' is already checked out at '<path>' if the user's repo has the same branch checked out. This is most likely to happen for the default branch (main). The worktree reuse code mitigates this for the registered local path, but any new branch creation that matches the parent's current branch will fail.

---

🟡 Medium — testhost in production FindActiveLockPid risks false positives

File: ExternalSessionScanner.cs

testhost is the .NET test runner. If a user runs dotnet test and PID recycling causes a stale lock file to contain the testhost PID, FindActiveLockPid returns a false positive — claiming a session is active when it's not, blocking session cleanup.

This is a test-only concern that shouldn't be in production code. Fix: Make the process name filter injectable, then add testhost only in the test. Or simply assert matchesFilter || myName.Contains("testhost") in the test without touching production code.

---

🟡 Medium — Worktree reuse returns stale/corrupted worktrees without validation

File: RepoManager.cs, CreateWorktreeAsync (new reuse check)

&& Directory.Exists(w.Path)

Directory.Exists returns true even if the worktree is corrupted (missing/broken .git file), locked, or has uncommitted changes from a previous session that will contaminate the new session's workspace.

Fix: Run git -C <path> rev-parse --git-dir before reusing, and optionally warn if the working tree is dirty.

---

🟡 Medium — GetDefaultBranch returns wrong branch for non-bare repos not on default branch

File: RepoManager.cs, GetDefaultBranch

git symbolic-ref HEAD on a non-bare repo returns the currently checked-out branch, not the repo's canonical default. If the user's repo is on feature/my-thing, the new worktree branches from feature/my-thing instead of main.

Fix: Use git rev-parse --abbrev-ref origin/HEAD to get the canonical default branch.

---

🔵 Low — RunGhAsync GIT_DIR guard stale after non-bare BareClonePath

File: RepoManager.cs, RunGhAsync

The guard if (workDir.EndsWith(".git")) setting GIT_DIR was written assuming BareClonePath always ends in .git. It's now a plain directory path. gh still discovers the remote, so it's not broken today — but the assumption is now wrong and the comment is misleading for future callers.

---

Summary

Severity	Issue
🔴 Critical	RemoveRepositoryAsync recursively deletes user's real project on deleteFromDisk
🔴 High	Existing bare clone silently orphaned when same repo re-added via folder
🟠 High	BackfillWorktreeClonePaths corrupts clone paths of pre-existing worktrees
🟠 High	git worktree add fails when non-bare repo has same branch checked out
🟡 Medium	testhost in production process filter — false positive risk
🟡 Medium	Worktree reuse skips corruption/dirty-state validation
🟡 Medium	GetDefaultBranch returns wrong branch for non-default-branch checkouts
🔵 Low	RunGhAsync GIT_DIR guard stale assumption

The Critical issue is a data-loss bug that should block merge. The three High issues reflect structural concerns with the "BareClonePath → non-bare repo" approach that need explicit guards.

[PureWeen]

🔍 Multi-Model Code Review — PR #527 (v4)

Reviewed by: 3 independent reviewers with adversarial consensus
Commits reviewed: 3 (16a8048, d40e4f6, 9e21b09)
CI Status: ⚠️ No checks configured for this branch

---

New Findings

🔴 CRITICAL: Missing EnsureLoaded() in AddRepositoryFromLocalAsync (3/3)

File: PolyPilot/Services/RepoManager.cs:550-639
Flagged by: All 3 reviewers (1 initial + 2 adversarial confirmation)

AddRepositoryFromLocalAsync accesses _state.Repositories (line 595) without calling EnsureLoaded() first. Every other public method that reads or mutates _state (18+ call sites) calls EnsureLoaded() as its first action. The old implementation safely delegated to AddRepositoryAsync which does call EnsureLoaded().

Data-loss scenario: If AddRepositoryFromLocalAsync is called before any other method triggers Load():

1. _state is the default empty new RepositoryState()
2. The method creates a new repo in the empty list (line 621)
3. Save() at line 624 passes the empty-state guard (guard only skips when state is empty AND not loaded, but Repositories.Count == 1 at this point)
4. repos.json is overwritten with only 1 repo — all previously registered repositories are silently destroyed

Fix: Add EnsureLoaded(); immediately before the lock (_stateLock) block at line 593.

---

🔴 CRITICAL: Orphaned bare clone deletion breaks active worktrees (2/3) — 4th consecutive round

File: PolyPilot/Services/RepoManager.cs:607-631
Flagged by: 2/3 reviewers independently

When a user adds a local folder for a repo already registered via URL, AddRepositoryFromLocalAsync:

1. Overwrites existing.BareClonePath = localPath (line 607)
2. Calls BackfillWorktreeClonePaths (line 608) — but this only updates worktrees with null/empty BareClonePath
3. Deletes the old managed bare clone (lines 628-631)

Worktrees created by CreateWorktreeAsync always have BareClonePath set (line 787), so they are silently skipped by BackfillWorktreeClonePaths. After deletion:

• wt.BareClonePath in state still points at the deleted directory
• Each worktree's .git file contains gitdir: <deleted-bare-clone>/worktrees/<id> — all git operations fail immediately

Scenario: User adds https://github.com/org/repo via URL → creates sessions (worktrees) → later adds same repo via "Existing Folder" → all existing worktrees are permanently broken.

Suggested fix (safest): When the existing repo already has a managed bare clone AND active worktrees exist, preserve the existing repo as-is — don't overwrite BareClonePath, don't delete. Only register the local folder as an external worktree. This eliminates the entire class of orphan bugs.

---

Previous Findings Status

#	Finding	v1	v2	v3	v4
1	RemoveRepositoryAsync data-loss guard	🔴	✅	✅	✅ FIXED
2	Worktree reuse scoping	🔴	✅	✅	✅ FIXED
3	Race in concurrent CreateWorktreeAsync	—	—	⚠️	⚠️ PARTIALLY FIXED
4	Worktree health check	—	—	⚠️	⚠️ PARTIALLY FIXED
5	Orphaned bare clone deletion	🔴	🔴	🔴	🔴 STILL PRESENT
6	Safety tests structural-only	—	🟡	🟡	🟡 STILL PRESENT
7	XML doc misattached	—	—	🟢	✅ FIXED (overload removed)
8	Missing EnsureLoaded()	—	—	—	🔴 NEW

---

Test Coverage Assessment

• ✅ AddRepositoryFromLocal_PointsBareClonePathAtLocalRepo — core behavioral test
• ✅ AddRepositoryFromLocal_NoBareCloneCreatedInReposDir — source-parsing test
• ❌ No behavioral test for orphan scenario (URL-based repo + worktrees + local folder add)
• ❌ AddRepositoryFromLocal_DoesNotOverwriteExistingUrlBasedRepo mentioned in earlier review but never added
• 🟡 Safety tests (delete guard, worktree reuse) are structural/source-parsing only

---

Recommendation

⚠️ Request changes — 2 critical issues must be fixed:

1. Add EnsureLoaded(); before the lock block in AddRepositoryFromLocalAsync (1-line fix)
2. Preserve existing URL-based repos — when AddRepositoryFromLocalAsync finds an existing repo with a managed bare clone, don't overwrite BareClonePath or delete the bare clone. Only register the local folder as an external worktree. Add a behavioral test for this scenario.

Items 3, 4, and 6 are lower-severity and acceptable as follow-up work.