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.

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

Reuse existing checkout for same-branch sessions

• CreateWorktreeAsync checks for an existing registered worktree on the requested branch before creating a new one
• Avoids duplicating huge repos when the user just wants a session on their current branch

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

Testing

All 3269 tests pass.

[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]

PR #527 Multi-Model Code Review (Re-Review v2)

CI status: ⚠️ no checks reported on this branch.
Tests: ✅ All 3319 tests pass locally.
Commits reviewed: 3 on main (16a8048, d40e4f6, 9e21b09)

---

Previous Findings — Status

#	Finding	Status
1	🔴 RemoveRepositoryAsync deletes user's real repo	✅ FIXED — managed-path guard at RemoveRepositoryAsync only deletes if BareClonePath is under ReposDir (3/3 reviewers confirm)
2	🟡 Worktree reuse returns user's primary checkout	✅ FIXED — reuse logic requires Path.StartsWith(managedWorktreePrefix), excluding external worktrees (3/3)
3	🟡 Race condition in concurrent CreateWorktreeAsync	⚠️ PARTIALLY FIXED — guid8 naming prevents directory collision. However, two concurrent calls for the same branch both pass the reuse check, then the second git worktree add -b fails. Low severity — intermittent error, no data loss. (3/3)
4	🟢 No worktree health check before reuse	⚠️ PARTIALLY FIXED — Directory.Exists check prevents reuse of deleted directories. No deeper .git validation. (2/3)
5	🟡 Orphaned bare clone cleanup breaks existing worktrees	❌ STILL PRESENT — see critical finding below (3/3)
6	🟡 New tests are structural-only	❌ STILL PRESENT — reinforced in commit 3 (3/3)
7	🟢 DevFlow.Blazor hardcodes package version	➖ N/A — not part of this diff's scope

---

Current Findings

🔴 CRITICAL: Orphaned bare clone deletion breaks active worktrees (3/3 reviewers)

File: RepoManager.cs — AddRepositoryFromLocalAsync (lines 593-632)

When a repo previously added by URL is re-added via "Existing Folder":

1. BareClonePath is updated to the user's local repo (line 607)
2. BackfillWorktreeClonePaths only updates worktrees with empty BareClonePath — worktrees that already stored the old bare clone path are not migrated (line 399 filter: string.IsNullOrWhiteSpace(w.BareClonePath))
3. Old bare clone is deleted from disk (line 630)

But existing git worktrees have .git files containing gitdir: pointers to {oldBareClone}/worktrees/{name}. After deletion, every git operation in those worktrees fails with fatal: not a git repository.

Compare with RemoveRepositoryAsync (lines 1097-1101), which correctly removes all worktrees first before deleting the bare clone. The orphan cleanup path skips this step.

Fix: Before deleting oldBareClonePath, either:

• (a) Check if any worktrees reference it and skip deletion if so, or
• (b) Remove/re-link those worktrees first (similar to RemoveRepositoryAsync's approach)

---

🟡 MODERATE: Safety tests are structural-only — zero production code coverage (3/3 reviewers)

Files: RepoManagerTests.cs (lines 1025-1079), AddExistingRepoTests.cs (lines 240-271)

The three safety "regression" tests never call any production code:

• RemoveRepository_DeleteFromDisk_SkipsNonManagedBareClonePath — creates directories, asserts path prefixes, never calls RemoveRepositoryAsync
• WorktreeReuse_OnlyMatchesCentralizedWorktrees — creates directories, asserts prefix relationships, never calls CreateWorktreeAsync
• AddRepositoryFromLocal_NoBareCloneCreatedInReposDir — parses source code as text, asserts string contains "BareClonePath = localPath"

Removing the guards from production code would leave all these tests green. The critical data-loss guard (RemoveRepositoryAsync managed-path check) has zero behavioral test coverage.

Fix: Add behavioral tests that:

1. Create a RepoManager, add an "Existing Folder" repo
2. Call RemoveRepositoryAsync(deleteFromDisk: true)
3. Assert the user's directory still exists afterward

---

🟡 MODERATE: Worktree reuse returns broken worktrees without validation (2/3 reviewers)

File: RepoManager.cs — CreateWorktreeAsync (lines 731-739)

Directory.Exists returns true even if the worktree's .git file is broken (orphaned by the bare clone deletion issue above, or by manual user action). A reused broken worktree gives the session a non-functional working directory.

Fix: Add a minimal health check before reuse: verify .git file exists inside the worktree directory. On failure, prune the stale entry from state and fall through to create a new worktree.

---

Informational (single-reviewer, discarded after adversarial round)

• Directory.Exists called inside _stateLock: One reviewer flagged disk I/O inside the synchronous lock. Adversarial check found this negligible — ≤5 local-path stat() calls totaling ~10-50μs. The codebase has heavier I/O under the same lock (Save() → File.WriteAllText). Not actionable.

---

Test Coverage Assessment

• ✅ Existing nested-strategy tests correctly replaced with centralized-strategy tests
• ✅ AddRepositoryFromLocal_PointsBareClonePathAtLocalRepo — good behavioral test of the core flow
• ⚠️ Safety tests (RemoveRepository_DeleteFromDisk_*, WorktreeReuse_*) are structural-only — need behavioral equivalents
• ⚠️ No test for the orphaned bare clone cleanup path (the critical finding)
• ⚠️ No test for concurrent same-branch worktree creation
• 🟢 AddRepositoryFromLocal_NoBareCloneCreatedInReposDir source-code parsing test is fragile but provides a guardrail against accidentally re-introducing the bare clone call

---

Recommendation

⚠️ Request changes — the two critical fixes from v1 (data-loss guard + reuse scoping) are solid, but:

1. 🔴 The orphaned bare clone cleanup can break existing worktrees — this is the same finding from the previous review, still unaddressed. It's a data-integrity issue that affects the URL→Existing Folder upgrade path.
2. 🟡 The regression tests don't exercise production code — the data-loss guard's correctness is untested.

After fixing #1 (add worktree drain/skip before bare clone deletion) and adding at least one behavioral test for #2, this PR is ready to merge.