feat: Add existing folder as repository

[PureWeen]

Add Existing Folder as Repository

Allows users to point PolyPilot at an already-cloned local git repo, giving it full sidebar integration with worktree-based branch creation.

Two-Path Add Flow

1. Existing repo match -- merges into existing group + shows toast
2. New repo -- creates 📁 group with Quick Branch + Named Branch menus

Nested Worktrees

For 📁 groups, worktrees created inside the repo at {localPath}/.polypilot/worktrees/{branchName}/. Auto .gitignore. Path traversal protection. URL repos use ~/.polypilot/worktrees/.

Delete Session Flow

Non-managed sessions skip worktree deletion prompt. Nested worktrees show delete option.

Migration

ReconcileOrganization backfills LocalPath/RepoId on stale groups.

Key Files

• RepoManager.cs -- AddRepositoryFromLocalAsync, CreateWorktreeAsync, EnsureGitIgnoreEntry
• CopilotService.Bridge.cs -- AddRepoFromLocalFolderAsync
• CopilotService.Organization.cs -- GetOrCreateLocalFolderGroup, ReconcileOrganization
• SessionSidebar.razor -- group menus, quickBranchLocalPath
• SessionListItem.razor -- managed worktree detection
• SessionOrganization.cs -- SessionGroup.LocalPath, IsLocalFolder

TODOs

• Migration heuristic could false-match shared folder names
• Cancel button doesnt clear quickBranchLocalPath (only Escape)
• Consider git worktree list auto-discovery

Tests

49 RepoManager tests + all WorktreeStrategy tests passing

Commits

• d5f62ad fix: path traversal guard + nested worktree delete
• fd19d2e feat: nested worktrees for local folder repos
• 185cede fix: full worktree support + auto-migrate stale groups
• 23e799b fix: smart local-folder add
• ec7c3d8 fix: ReconcileOrganization guard
• 917cc9c fix: distinct sidebar group
• 76ac48e feat: Add existing folder as repository
• ae274ae Guard empty SessionId

[PureWeen]

🔍 Multi-Model Consensus Review — PR #405 (Round 1)

PR: feat: Add existing folder as repository
CI Status: ⚠️ No CI checks configured on this branch
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex

---

Findings (consensus ≥ 2 models)

🟡 F1: RunProcess test helper — race condition (RepoManagerTests.cs:69-75)
Process.Start() is called on line 69, but p.Exited is subscribed on line 70 and p.EnableRaisingEvents = true is set on line 75. If git init completes between Start() and EnableRaisingEvents = true, the Exited event never fires and the TaskCompletionSource blocks forever — no timeout, no cancellation token. Fix: set EnableRaisingEvents = true and subscribe Exited before Start(), or use await p.WaitForExitAsync(ct).

🟡 F2: Bare catch swallows OperationCanceledException (RepoManager.cs, AddRepositoryFromLocalAsync)

catch { remoteUrl = ""; }

RunGitAsync throws InvalidOperationException on non-zero exit, but if the user cancels via CancellationToken, OperationCanceledException is also caught. The caller then throws a misleading "No 'origin' remote found" instead of propagating cancellation. Fix: catch (Exception ex) when (ex is not OperationCanceledException).

🟡 F3: Same bare catch pattern in IsGitRepositoryAsync (RepoManager.cs)

catch { return false; }

A cancelled git rev-parse reports "not a git repository" instead of propagating cancellation. Same fix as F2.

🟢 F4: Fragile state reset on form open (SessionSidebar.razor:341)
showAddRepo = true inline handler does not reset addRepoFolderMode or addRepoFolderPath. All current close paths go through CloseAddRepoForm() which resets them, but any future close path that sets showAddRepo = false directly would leak stale folder-mode state. Low risk — all current paths are correct.

---

✅ Positive Notes

• AddRepoFromLocalFolderAsync in CopilotService.Bridge.cs correctly guards remote mode, follows GetOrCreateRepoGroup pattern, and state persistence is handled (save + notify)
• AddRepositoryAsync already handles duplicate repos gracefully (returns existing)
• CloseAddRepoForm() properly resets all new state variables
• Test coverage for error paths (non-existent folder, no git, no origin) is good

Test Coverage

• ✅ Three failure-mode tests added (NonExistentFolder, NoGit, NoOrigin)
• ⚠️ No success-path test for AddRepositoryFromLocalAsync (bare clone from local)
• ⚠️ No test for AddRepoFromLocalFolderAsync remote-mode guard (InvalidOperationException)

Verdict: ⚠️ Request Changes

Required: Fix F1 (test hang risk) and F2/F3 (cancellation swallowing)
Nice-to-have: F4 state reset, success-path test

[PureWeen]

Re-Review: Add existing folder as repository (#405) — Round 2

Latest commit: 775ea1b (3 commits total)

---

Round 1 Findings Status

#	Severity	Finding	Status
F1	🟡 MODERATE	RunProcess race: EnableRaisingEvents set after Process.Start() in test helper	✅ FIXED
F2	🟡 MODERATE	Bare catch { remoteUrl = ""; } swallows OperationCanceledException in AddRepositoryFromLocalAsync	✅ FIXED
F3	🟡 MODERATE	Bare catch { return false; } swallows OperationCanceledException in IsGitRepositoryAsync	✅ FIXED
F4	🟢 MINOR	showAddRepo = true inline handler doesn't reset addRepoFolderMode/addRepoFolderPath	✅ FIXED

All 4 Round 1 findings confirmed fixed by 4/4 models (5th agent timed out).

---

New Findings (Round 2, consensus 2+ models)

Severity	Finding
🟢 MINOR	RunProcess in RepoManagerTests.cs doesn't dispose the Process object — minor resource leak in test helper (3/4 models)
🟢 MINOR	stdout/stderr redirected but never drained asynchronously — for long-running or verbose commands, stdout buffer fill could deadlock WaitForExitAsync (2/4 models; low risk given current usage of short git remote get-url and git rev-parse commands)

---

Test Coverage

New RepoManagerTests.cs covers: valid git repo detection, non-git directory, invalid path, remote URL extraction, and missing origin. Coverage is good for the added code paths.

---

Recommended Action: ✅ Approve

All blocking issues are fixed. The two remaining minor items (process leak and stdout drain) are test infrastructure concerns that don't affect production behavior.

[PureWeen]

🔍 Multi-Model Consensus Review — PR #405 (Round 3)

PR: feat: Add existing folder as repository (new commits since Round 2 approval)
CI Status: ⚠️ No CI checks configured
Tests: ✅ 70/70 pass (RepoManagerTests + WorktreeStrategyTests)
Models: claude-opus-4.6 ×2, claude-sonnet-4.6, gemini-3-pro-preview, gpt-5.3-codex

---

Round 1+2 Findings Status (all fixed ✅)

F1 EnableRaisingEvents race, F2/F3 bare catch swallowing cancellation, F4 addRepoFolderMode reset — all confirmed fixed in Round 2.

---

New Findings (Round 3, consensus 2+ models)

🔴 C1 — Data Loss: RemoveWorktreeAsync deletes user's original local repository

File: PolyPilot/Services/RepoManager.cs:893-903 + 995-1001
Flagged by: 2/5 models (gpt-5.3-codex, claude-opus-4.6)

Root cause: RegisterExternalWorktreeAsync stores the user's local folder as a WorktreeInfo with BareClonePath set (line 626). When the user removes the repository via UI → RemoveRepositoryAsync iterates all worktrees including external ones and calls RemoveWorktreeAsync on each.

In RemoveWorktreeAsync, because BareClonePath is non-null, it enters the guarded branch and tries git worktree remove <user-local-path> --force on the bare clone. This fails — the user's local path is a standalone clone, not a linked worktree of the bare clone. The catch block then unconditionally does:

if (Directory.Exists(wt.Path))
    try { Directory.Delete(wt.Path, recursive: true); } catch { }

This permanently deletes the user's original repository directory and all its contents. The safety guard at lines 909-918 (managed-location check) only applies to the else branch when bareClonePath is null — external worktrees always have BareClonePath set, so they bypass it entirely.

Scenario: User adds ~/Projects/myapp via "Add Existing Folder" → it shares a RepoId with any URL-based repo group → user deletes that repo group → ~/Projects/myapp is recursively deleted.

Fix: Before Directory.Delete in the catch block, apply the same managed-location guard (isCentralized || isNested). External worktrees should only be unregistered from state — their directory must never be deleted.

---

🔴 C2 — Data Loss: Empty sanitized branch name destroys all nested worktrees

File: PolyPilot/Components/Layout/SessionSidebar.razor:1949 → PolyPilot/Services/RepoManager.cs:694-719
Flagged by: 4/5 models (claude-opus-4.6 ×2, claude-sonnet-4.6, gpt-5.3-codex)

Root cause: The quick-branch sanitizer at SessionSidebar.razor:1949:

branchName = Regex.Replace(input, @"[^a-zA-Z0-9/_.-]", "-").Trim('-');

branchName = "" reaches CreateWorktreeAsync with a non-null localPath. In the nested strategy:

1. Path.Combine(repoWorktreesDir, "") returns repoWorktreesDir itself
2. The path traversal guard's resolved.Equals(repoWorktreesDir) condition passes (it was intended to prevent escapes, but allows equality)
3. git worktree add <repoWorktreesDir> -b "" ... fails (git rejects empty branch names)
4. Catch cleanup: Directory.Delete(worktreePath=repoWorktreesDir, recursive: true) — destroys the entire .polypilot/worktrees/ container, wiping all previously created nested worktrees for that repo

Fix (two-part):

1. After sanitization on line 1949, add: if (string.IsNullOrEmpty(branchName)) { quickBranchError = "Branch name cannot be empty."; return; }
2. In CreateWorktreeAsync, remove the resolved.Equals(repoWorktreesDir) condition from the path traversal guard — equality with the parent directory is not a valid worktree path and should throw

---

🟡 M1 — TOCTOU race: duplicate external worktree entries

File: PolyPilot/Services/RepoManager.cs:599-634
Flagged by: 5/5 models

The idempotency check (lock at line 599-605) and the Worktrees.Add (lock at line 630-634) are two separate critical sections with async RunGitAsync calls between them. Two concurrent calls for the same path both pass the first check, both complete async work, and both insert duplicate WorktreeInfo entries. Duplicates persist to disk and survive restarts.

Fix: Re-check for duplicates inside the second lock (_stateLock) block before calling _state.Worktrees.Add(wt).

---

🟡 M2 — Migration heuristic: folder-name collision assigns wrong RepoId

File: PolyPilot/Services/CopilotService.Organization.cs:583-592
Flagged by: 2/5 models (gemini-3-pro-preview, gpt-5.3-codex)

The migration FirstOrDefault matches by leaf folder name only. If the user has two different repos both named "myapp" registered as external worktrees, the migration will assign one group's LocalPath/RepoId to both — permanently wrong metadata. Unlike C1/C2, this is acknowledged as a known heuristic limitation and the impact is cosmetic (wrong sidebar grouping), not data loss.

---

Test Coverage

---

Recommended Action: ⚠️ Request Changes

C1 and C2 are data-loss bugs introduced in the newest commits. Both need fixes before merge. M1 (TOCTOU) is a low-risk fix worth doing. M2 is a known trade-off and acceptable as-is.

[PureWeen]

🔄 Re-Review Round 4 — PR #405

Reviewing commit 2c2f4e8a which the author says addresses C1, C2, and M1 from Round 3.

Previous Findings Status

• C1 (🔴 RemoveWorktreeAsync data loss): ✅ FIXED

  The catch block now has the isCentralized || isNested guard before Directory.Delete:

  var isCentralized = fullPath.StartsWith(Path.GetFullPath(WorktreesDir), ...);
  var isNested = fullPath.Contains("/.polypilot/worktrees/", ...);
  if ((isCentralized || isNested) && Directory.Exists(wt.Path))
      try { Directory.Delete(wt.Path, recursive: true); } catch { }

  External folders added via "Add Existing Folder" have paths that don't match either condition, so they're now safely skipped. The else branch (no bare clone path) also has the same guard. ✅

• C2 (🔴 Empty branch name data loss): ✅ FIXED

  Two-layer defence applied:

  1. UI layer (SessionSidebar.razor): after regex sanitization, string.IsNullOrEmpty(branchName) is checked → user-visible error message, returns early before calling CreateWorktreeAsync.
  2. RepoManager (CreateWorktreeAsync): the .Equals(repoWorktreesDir) escape hatch was removed from the path guard. An empty branch → Path.Combine(dir, "") == dir → !resolved.StartsWith(dir + Sep) is true → throws InvalidOperationException, preventing git from failing and the catch-block from deleting the container directory. ✅

• M1 (🟡 TOCTOU race): ✅ FIXED

  RegisterExternalWorktreeAsync now re-checks for a matching path inside the second lock(_stateLock) block immediately before Worktrees.Add, eliminating the race window:

  lock (_stateLock)
  {
      // Re-check for duplicates under the lock to handle concurrent registrations (TOCTOU).
      if (_state.Worktrees.Any(w => w.RepoId == repo.Id && PathsEqual(w.Path, normalizedPath)))
          return;
      _state.Worktrees.Add(wt);
      Save();
  }

• M2 (🟡 Migration folder-name collision): ⚠️ STILL PRESENT

  The ReconcileOrganization migration still uses FirstOrDefault matching by folder name only:

  var match = _repoManager.Worktrees.FirstOrDefault(wt =>
      !wt.Path.StartsWith(managedWorktreesDir, ...) &&
      string.Equals(Path.GetFileName(wt.Path.TrimEnd(...)), group.Name, OrdinalIgnoreCase));

  Two external repos with the same folder name (e.g., ~/projects/myapp and ~/work/myapp) will still cause the wrong repo group to be back-filled. Not addressed in this commit.

New Issue (Minor)

Test quality gap on C2: The new CreateWorktree_PathTraversal_InBranchName_IsRejected tests use the old two-condition guard logic in their assertions (both !StartsWith && !Equals), not the new single-condition guard that actually ships in production. This means the tests don't cover the empty-branch-name case at the RepoManager level. The fix is still correct (defended at UI layer), but a test like:

[InlineData("")]
[InlineData(".")]

would complete the regression coverage for C2's server-side guard.

Tests

2830 passed / 1 failed

The one failure (WsBridgeIntegrationTests.Organization_BroadcastsStateBack_ToClient) is a pre-existing flaky timing test — that file was not modified by this PR, and the test passes when run in isolation within the affected test class. Not a regression.

No new test failures introduced by this PR.

Verdict

⚠️ Request changes (minor)

The two critical data-loss bugs (C1, C2) and the TOCTOU race (M1) are all genuinely fixed. The only outstanding item is M2 (migration folder-name collision), which was a moderate finding from Round 3, and a minor test coverage gap for the C2 server-side guard.

Suggested before merging:

1. Address M2 — either fix the migration to match on full path (not just folder name), or acknowledge it as a known limitation with a code comment explaining why it's safe (e.g., migration only runs on old groups that predate the LocalPath field, and the window is very narrow).
2. Add [InlineData("")] and [InlineData(".")] to the path traversal theory tests to confirm the C2 RepoManager guard works end-to-end.

If M2 is intentionally left for a follow-up, I'd accept this PR — the critical data-loss issues are fixed.

[PureWeen]

🔄 Re-Review Round 5 — PR #405

Reviewing commit a92a983b (newest, pushed after Round 4).

Previous Findings Status

• C1 (🔴 RemoveWorktreeAsync data loss): ✅ FIXED (Round 4, unchanged)

• C2 (🔴 Empty branch name data loss): ✅ FIXED (Round 4, unchanged)

• M1 (🟡 TOCTOU race): ✅ FIXED (Round 4, unchanged)

• M2 (🟡 Migration folder-name collision): ✅ FIXED

  ReconcileOrganization now collects all external worktrees matching the group folder name and only back-fills when exactly one match exists — skips if ambiguous:

  var candidates = _repoManager.Worktrees.Where(wt =>
      !wt.Path.StartsWith(managedWorktreesDir, ...) &&
      string.Equals(Path.GetFileName(wt.Path.TrimEnd(...)), group.Name, OrdinalIgnoreCase)).ToList();
  
  // Skip if ambiguous (multiple repos share the same folder name)
  if (candidates.Count != 1)
      continue;

  Two repos with folder name myapp no longer cause the wrong group to be mutated. ✅

• N1 (🟢 Test gap for empty/dot branch at RepoManager level): ✅ FIXED

  Path traversal tests updated to use the single-condition production guard AND cover the new cases:

  [InlineData("")]   // empty branch → resolves to repoWorktreesDir itself → rejected
  [InlineData(".")]  // dot → also resolves to repoWorktreesDir itself → rejected

  The assertion logic now matches the actual guard in CreateWorktreeAsync. ✅

New Issues

None found.

Tests

2831 passed / 2 failed / 2833 total (+2 new tests vs Round 4)

Both failures are pre-existing flaky timing tests (TurnEndFallbackTests.FallbackTimer_NotCancelled_FiresAfterDelay and WsBridgeIntegrationTests.Organization_BroadcastsStateBack_ToClient) — neither file was modified by this PR. Not a regression.

Verdict

✅ Approve

All 5 findings (2 critical, 2 moderate, 1 minor) are now fixed. No new issues introduced. The migration guard is correct and conservative (skip-if-ambiguous is the safe choice). Test coverage is complete. Ready to merge.

[PureWeen]

🔄 Re-Review Round 6 — PR #405

No new commits since Round 5 (a92a983b). The Round 5 ✅ Approve verdict stands.

All 5 findings resolved (C1, C2, M1, M2, N1), tests at 2831/2833 passing with 2 pre-existing flaky failures unrelated to this PR. Ready to merge.

[PureWeen]

🔄 Re-Review Round 7 — PR #405

Reviewing new commit 60cce7a0 pushed since Round 6.

Previous Findings Status

• C1 (🔴 RemoveWorktreeAsync data loss): ✅ FIXED (Round 4, unchanged)
• C2 (🔴 Empty branch name data loss): ✅ FIXED (Round 4, unchanged)
• M1 (🟡 TOCTOU race): ✅ FIXED (Round 4, unchanged)
• M2 (🟡 Migration collision): ✅ FIXED (Round 5, unchanged)
• N1 (🟢 Test gap for empty/dot branch): ✅ FIXED (Round 5, unchanged)

New Commit 60cce7a0

Fix: GetOrCreateRepoGroup must not return local folder groups

When a user has both a URL-based repo group and a 📁 local folder group for the same RepoId, GetOrCreateRepoGroup was incorrectly matching the local folder group (since both have the same RepoId). Sessions created from the URL-based repo menu were landing in the local folder group instead.

Fix: Added && !g.IsLocalFolder to the FirstOrDefault predicate:

var existing = Organization.Groups.FirstOrDefault(g =>
    g.RepoId == repoId && !g.IsMultiAgent && !g.IsLocalFolder
    && !Organization.Sessions.Any(m => m.GroupId == g.Id && m.Role == MultiAgentRole.Orchestrator));

The fix is correct and minimal. GetOrCreateLocalFolderGroup already has its own IsLocalFolder-filtered predicate, so the two group types are now cleanly separated. Four new regression tests added covering:

• GetOrCreateRepoGroup_DoesNotReturnLocalFolderGroup
• GetOrCreateRepoGroup_WhenBothGroupTypesExist_ReturnsUrlBasedGroup
• GetOrCreateRepoGroup_LocalFolderGroupFirst_ThenUrlGroup_ReturnsUrlBasedGroup
• ReconcileOrganization_SessionInLocalFolderGroup_DoesNotMoveToUrlGroup

New Issues

None found.

Tests

2834 passed / 3 failed / 2837 total (+4 new tests vs Round 5)

All 3 failures are pre-existing flaky timing/isolation tests (TurnEndFallbackTests.FallbackTimer_NotCancelled_FiresAfterDelay, WsBridgeIntegrationTests.Organization_BroadcastsStateBack_ToClient, UsageStatsTests.SaveAndLoad_PreservesStats). All 3 pass when run in isolation — confirmed not regressions introduced by this PR.

Verdict

✅ Approve

All 5 previous findings remain fixed. The new commit (60cce7a0) introduces a correct, well-tested fix for a session-routing bug when both group types coexist for the same repo. Ready to merge.