fix: add existing repo clones locally and avoids duplicate sidebar groups

[rmarinho]

Bug Report

[Session: MAUI.Sherpa] Adding MAUI.Sherpa as existing repository still cloned it from the network, and created duplicate entries (both a folder and a repo) in the sidebar treeview.

Root Cause

Two bugs in the 'Add Existing Folder' flow:

Bug 1: Network clone for local repos

\AddRepositoryFromLocalAsync\ extracted the remote URL from the local repo's \origin\ remote, then called \AddRepositoryAsync(remoteUrl)\ which does \git clone --bare \ from the network — even though the local repo is right there.

Fix: Added \localCloneSource\ parameter to \AddRepositoryAsync\ that clones from the local path and then sets
emote.origin.url\ to the real remote URL (so future fetches still go to the network).

Bug 2: Duplicate sidebar groups

\ReconcileOrganization\ auto-creates URL-based repo groups when it links sessions to worktrees. But \GetOrCreateRepoGroup\ explicitly skips \IsLocalFolder\ groups, so it created a second group even when a local folder group already existed.

Fix: Before calling \GetOrCreateRepoGroup, check if a local folder group with the same \RepoId\ exists and use it instead.

Testing

• 6 new tests in \AddExistingRepoTests.cs\ covering both bugs and regression scenarios
• All 3315 tests pass (0 failures)

[PureWeen]

🔍 Multi-Model Code Review — PR #533

3 independent reviewers analyzed this PR. Findings below reflect adversarial consensus (2+ reviewers must agree for inclusion).

---

CI Status

⚠️ No CI checks reported on this branch.

---

Original Findings (Round 1)

🟡 MODERATE — Second ReconcileOrganization block had the same bug

File: CopilotService.Organization.cs ~lines 607–627
Flagged by: 1/3 initially → confirmed 3/3 after adversarial round

✅ FIXED in 387a73e — same local-folder-group-first check applied to both blocks + new test added.

🟡 MODERATE — Source-file-reading tests didn't verify runtime behavior

File: AddExistingRepoTests.cs
Flagged by: All 3 reviewers

✅ FIXED in 387a73e — replaced with behavioral integration tests that create real git repos.

🟡 MODERATE — Missing network fetch after local clone

File: RepoManager.cs lines 509–512
Flagged by: 2/3 reviewers

✅ FIXED in 387a73e — documented as intentional design choice with inline comment.

🟡 MODERATE — Multi-local-folder-group edge case

File: CopilotService.Organization.cs line 576
Flagged by: 2/3 reviewers

➖ N/A — theoretical edge case. PolyPilot deduplicates by RepoId in the "Add Existing" flow. All 3 reviewers confirmed still present in re-review but agreed it's low-risk.

🟢 MINOR — localCloneSource had no validation

File: RepoManager.cs line 484
Flagged by: All 3 reviewers

✅ FIXED in 387a73e — added Directory.Exists guard with ArgumentException.

🟢 MINOR — GetUninitializedObject test fragility

File: AddExistingRepoTests.cs
Flagged by: All 3 reviewers

➖ N/A — consistent with existing test patterns across codebase. Accepted as tech debt.

---

Re-Review Findings (Round 2)

After fixes were applied, all 3 reviewers re-reviewed the updated diff.

🟡 NEW — changed = true unconditional in second reconcile block (regression)

File: CopilotService.Organization.cs line 624
Flagged by: 1/3 reviewers — independently verified as real regression

The fix commit moved changed = true outside the if/else in the second reconcile block, making it fire even when no group assignment happened (e.g., when GetOrCreateRepoGroup returns null). Original code had it inside if (repoGroup != null).

✅ FIXED in df413ae — restored conditional changed = true inside each branch.

🟢 MINOR — Non-async test uses .Result on ThrowsAsync

File: AddExistingRepoTests.cs line 251
Flagged by: 2/3 reviewers

AddRepositoryAsync_LocalCloneSource_InvalidPath_Throws was a void method calling Assert.ThrowsAsync without await, then accessing .Result. Works in xUnit but non-idiomatic and risks deadlocks in environments with a SynchronizationContext.

✅ FIXED in df413ae — changed to async Task with await.

🟢 MINOR — RunGitOutput leaks Process handle

File: AddExistingRepoTests.cs line 338
Flagged by: 1/3 reviewers — verified as real

Process.Start without using — if ReadToEndAsync or WaitForExitAsync throws, the Process handle leaks.

✅ FIXED in df413ae — added using var p.

---

Test Results

Passed!  - Failed: 0, Passed: 3315, Skipped: 0, Total: 3315

All tests pass after both fix rounds.

---

Recommendation

✅ Approve — All findings addressed across two review rounds. Both core bugs fixed correctly with proper test coverage, the introduced regression caught and fixed, and all minor issues resolved. Full test suite green.