diff --git a/PolyPilot.Tests/AddExistingRepoTests.cs b/PolyPilot.Tests/AddExistingRepoTests.cs index 825a23f7a..4752986dc 100644 --- a/PolyPilot.Tests/AddExistingRepoTests.cs +++ b/PolyPilot.Tests/AddExistingRepoTests.cs @@ -189,11 +189,10 @@ public void Reconcile_SessionInDefault_WithOnlyUrlGroup_FallsBackToUrlGroup() // ─── Bug 1: AddRepositoryAsync supports local clone source ───────────────── [Fact] - public async Task AddRepositoryFromLocal_ClonesLocallyAndSetsRemoteUrl() + public async Task AddRepositoryFromLocal_PointsBareClonePathAtLocalRepo() { - // Create a real local git repo with an origin remote, then call - // AddRepositoryFromLocalAsync and verify the bare clone's remote URL - // is the network URL (not the local path). + // AddRepositoryFromLocalAsync should set BareClonePath to the local path + // (no bare clone is created) and register the repo. var tempDir = Path.Combine(Path.GetTempPath(), $"local-clone-test-{Guid.NewGuid():N}"); var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); Directory.CreateDirectory(tempDir); @@ -212,16 +211,15 @@ public async Task AddRepositoryFromLocal_ClonesLocallyAndSetsRemoteUrl() RepoManager.SetBaseDirForTesting(testBaseDir); try { - var progressMessages = new List(); - var repo = await rm.AddRepositoryFromLocalAsync( - tempDir, msg => progressMessages.Add(msg)); + var repo = await rm.AddRepositoryFromLocalAsync(tempDir); - // Should have used local clone, not network - Assert.Contains(progressMessages, m => m.Contains("local folder", StringComparison.OrdinalIgnoreCase)); + // BareClonePath should point at the user's local repo — no bare clone + Assert.Equal(Path.GetFullPath(tempDir), Path.GetFullPath(repo.BareClonePath)); - // The bare clone's remote origin should point to the network URL - var bareRemoteUrl = await RunGitOutput(repo.BareClonePath, "remote", "get-url", "origin"); - Assert.Equal(remoteUrl, bareRemoteUrl.Trim()); + // No bare clone directory should exist under the managed repos dir + var reposDir = Path.Combine(testBaseDir, "repos"); + if (Directory.Exists(reposDir)) + Assert.Empty(Directory.GetDirectories(reposDir)); // Verify the repo was registered Assert.Contains(rm.Repositories, r => r.Id == repo.Id); @@ -239,20 +237,177 @@ public async Task AddRepositoryFromLocal_ClonesLocallyAndSetsRemoteUrl() } [Fact] - public async Task AddRepositoryAsync_LocalCloneSource_InvalidPath_Throws() + public async Task AddRepositoryFromLocal_NoBareCloneCreatedInReposDir() { - var rm = new RepoManager(); - var method = typeof(RepoManager).GetMethod("AddRepositoryAsync", - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance, - null, - new[] { typeof(string), typeof(Action), typeof(string), typeof(CancellationToken) }, - null)!; + // AddRepositoryFromLocalAsync should NOT create a bare clone in ReposDir. + // It sets BareClonePath directly to the local folder path. + var tempDir = Path.Combine(Path.GetTempPath(), $"no-bare-test-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + Directory.CreateDirectory(testBaseDir); + try + { + await RunProcess("git", "init", tempDir); + await RunProcess("git", "-C", tempDir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", tempDir, "config", "user.name", "Test"); + await RunProcess("git", "-C", tempDir, "commit", "--allow-empty", "-m", "init"); + await RunProcess("git", "-C", tempDir, "remote", "add", "origin", "https://github.com/test-owner/no-bare.git"); + + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + rm.Load(); + var repo = await rm.AddRepositoryFromLocalAsync(tempDir); + + // BareClonePath should point at the local folder, not a managed bare clone + Assert.Equal(Path.GetFullPath(tempDir), Path.GetFullPath(repo.BareClonePath)); + + // No bare clone directory should exist in the repos dir + var reposDir = Path.Combine(testBaseDir, "repos"); + if (Directory.Exists(reposDir)) + { + var bareDirs = Directory.GetDirectories(reposDir, "*.git"); + Assert.Empty(bareDirs); + } + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public async Task AddRepositoryFromLocal_CreatesSeparateRepoWhenUrlBasedExists() + { + // When a repo was already added via "Add from URL" (managed bare clone), + // adding the same repo from a local folder must create a SEPARATE RepositoryInfo + // with a distinct ID and BareClonePath pointing at the local folder. + var tempDir = Path.Combine(Path.GetTempPath(), $"local-overwrite-test-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + Directory.CreateDirectory(testBaseDir); + try + { + var remoteUrl = "https://github.com/test-owner/overwrite-test.git"; + + // Create a local git repo with an origin remote + await RunProcess("git", "init", tempDir); + await RunProcess("git", "-C", tempDir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", tempDir, "config", "user.name", "Test"); + await RunProcess("git", "-C", tempDir, "commit", "--allow-empty", "-m", "init"); + await RunProcess("git", "-C", tempDir, "remote", "add", "origin", remoteUrl); - var ex = await Assert.ThrowsAsync(async () => - await (Task)method.Invoke(rm, - new object?[] { "https://github.com/test/repo", null, "/nonexistent/path", CancellationToken.None })!); + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + // Simulate a repo already added via "Add from URL" with a managed bare clone. + var urlId = RepoManager.RepoIdFromUrl(remoteUrl); + var barePath = Path.Combine(testBaseDir, "repos", $"{urlId}.git"); + Directory.CreateDirectory(barePath); + var urlRepo = new RepositoryInfo + { + Id = urlId, + Name = "overwrite-test", + Url = remoteUrl, + BareClonePath = barePath, + AddedAt = DateTime.UtcNow + }; + // Inject the URL-based repo into state + var state = new RepositoryState(); + state.Repositories.Add(urlRepo); + var stateFile = Path.Combine(testBaseDir, "repos.json"); + File.WriteAllText(stateFile, System.Text.Json.JsonSerializer.Serialize(state)); + rm.Load(); + + // Now add the same repo from a local folder + var localRepo = await rm.AddRepositoryFromLocalAsync(tempDir); + + // The returned repo should have a DIFFERENT ID from the URL-based repo + Assert.NotEqual(urlId, localRepo.Id); + Assert.StartsWith(urlId, localRepo.Id); // e.g. "test-owner-overwrite-test-local-..." + + // The local repo's BareClonePath must point at the local folder + Assert.True(PathsEqual(localRepo.BareClonePath, tempDir), + $"Expected local repo BareClonePath to be '{tempDir}' but got '{localRepo.BareClonePath}'"); + + // The original URL-based repo must be untouched + var originalRepo = rm.Repositories.FirstOrDefault(r => r.Id == urlId); + Assert.NotNull(originalRepo); + Assert.Equal(Path.GetFullPath(barePath), Path.GetFullPath(originalRepo.BareClonePath)); + + // The managed bare clone directory must still exist + Assert.True(Directory.Exists(barePath)); + + // There should be TWO repos total + Assert.Equal(2, rm.Repositories.Count); + } + finally + { + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); + } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public async Task AddRepositoryFromLocal_IdempotentForSameLocalFolder() + { + // Adding the same local folder twice should return the same repo, not create duplicates. + var tempDir = Path.Combine(Path.GetTempPath(), $"local-idempotent-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + Directory.CreateDirectory(testBaseDir); + try + { + var remoteUrl = "https://github.com/test-owner/idempotent-test.git"; + await RunProcess("git", "init", tempDir); + await RunProcess("git", "-C", tempDir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", tempDir, "config", "user.name", "Test"); + await RunProcess("git", "-C", tempDir, "commit", "--allow-empty", "-m", "init"); + await RunProcess("git", "-C", tempDir, "remote", "add", "origin", remoteUrl); + + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + // Add the local folder twice + var repo1 = await rm.AddRepositoryFromLocalAsync(tempDir); + var repo2 = await rm.AddRepositoryFromLocalAsync(tempDir); + + // Both should return the same repo + Assert.Equal(repo1.Id, repo2.Id); + Assert.True(PathsEqual(repo1.BareClonePath, repo2.BareClonePath)); + + // Should still be exactly one repo + Assert.Single(rm.Repositories); + } + finally + { + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); + } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } - Assert.Contains("not found", ex.Message, StringComparison.OrdinalIgnoreCase); + private static bool PathsEqual(string? left, string? right) + { + if (left == null || right == null) return false; + var a = Path.GetFullPath(left).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + var b = Path.GetFullPath(right).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + return string.Equals(a, b, StringComparison.OrdinalIgnoreCase); } // ─── Bug 2 (second block): WorktreeId-based reconcile prefers local folder ─ @@ -351,4 +506,151 @@ private static void ForceDeleteDirectory(string path) File.SetAttributes(f, FileAttributes.Normal); Directory.Delete(path, true); } + + [Fact] + public async Task AddRepositoryFromLocal_LocalRepoId_HasExpectedFormat() + { + // The local repo ID should follow the pattern "{baseId}-local-{pathHash}" + // where pathHash is a hex-encoded hash of the normalized path. + var tempDir = Path.Combine(Path.GetTempPath(), $"local-id-format-test-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + Directory.CreateDirectory(testBaseDir); + try + { + var remoteUrl = "https://github.com/test-owner/id-format-test.git"; + + await RunProcess("git", "init", tempDir); + await RunProcess("git", "-C", tempDir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", tempDir, "config", "user.name", "Test"); + await RunProcess("git", "-C", tempDir, "commit", "--allow-empty", "-m", "init"); + await RunProcess("git", "-C", tempDir, "remote", "add", "origin", remoteUrl); + + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + // Pre-create a URL-based repo so the local one gets a distinct ID + var urlId = RepoManager.RepoIdFromUrl(remoteUrl); + var barePath = Path.Combine(testBaseDir, "repos", $"{urlId}.git"); + Directory.CreateDirectory(barePath); + var state = new RepositoryState(); + state.Repositories.Add(new RepositoryInfo + { + Id = urlId, Name = "id-format-test", + Url = remoteUrl, BareClonePath = barePath, AddedAt = DateTime.UtcNow + }); + File.WriteAllText(Path.Combine(testBaseDir, "repos.json"), + System.Text.Json.JsonSerializer.Serialize(state)); + rm.Load(); + + var localRepo = await rm.AddRepositoryFromLocalAsync(tempDir); + + // ID should match pattern: baseId-local-HEXHASH + Assert.Matches(@"^test-owner-id-format-test-local-[0-9a-f]{8}$", localRepo.Id); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public async Task EnsureRepoClone_SkipsCloneForNonBareRepo_WithGitDirectory() + { + // When BareClonePath points at a non-bare repo (added via "Existing Folder"), + // EnsureRepoCloneInCurrentRootAsync should skip clone management. + // We verify this by calling FetchAsync (which calls EnsureRepoCloneInCurrentRootAsync) + // on a repo whose BareClonePath is a local non-bare checkout. + var tempDir = Path.Combine(Path.GetTempPath(), $"skip-clone-test-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + Directory.CreateDirectory(testBaseDir); + try + { + await RunProcess("git", "init", tempDir); + await RunProcess("git", "-C", tempDir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", tempDir, "config", "user.name", "Test"); + await RunProcess("git", "-C", tempDir, "commit", "--allow-empty", "-m", "init"); + await RunProcess("git", "-C", tempDir, "remote", "add", "origin", "https://github.com/test-owner/skip-clone.git"); + + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + rm.Load(); + var repo = await rm.AddRepositoryFromLocalAsync(tempDir); + + // FetchAsync internally calls EnsureRepoCloneInCurrentRootAsync. + // For a non-bare repo (has .git directory), it should return early + // without creating a bare clone in ReposDir. + try { await rm.FetchAsync(repo.Id); } catch { /* fetch may fail without a real remote — that's OK */ } + + // Verify no bare clone was created + var reposDir = Path.Combine(testBaseDir, "repos"); + if (Directory.Exists(reposDir)) + { + var bareDirs = Directory.GetDirectories(reposDir, "*.git"); + Assert.Empty(bareDirs); + } + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public async Task AddRepositoryFromLocal_ValidationErrors_ThrowDescriptiveExceptions() + { + var nonExistent = Path.Combine(Path.GetTempPath(), $"does-not-exist-{Guid.NewGuid():N}"); + var notGit = Path.Combine(Path.GetTempPath(), $"not-git-{Guid.NewGuid():N}"); + var noOrigin = Path.Combine(Path.GetTempPath(), $"no-origin-{Guid.NewGuid():N}"); + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(notGit); + Directory.CreateDirectory(noOrigin); + Directory.CreateDirectory(testBaseDir); + try + { + // Initialize noOrigin as git repo but without origin remote + await RunProcess("git", "init", noOrigin); + await RunProcess("git", "-C", noOrigin, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", noOrigin, "config", "user.name", "Test"); + await RunProcess("git", "-C", noOrigin, "commit", "--allow-empty", "-m", "init"); + + var rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + // Non-existent folder + var ex1 = await Assert.ThrowsAsync( + () => rm.AddRepositoryFromLocalAsync(nonExistent)); + Assert.Contains("not found", ex1.Message, StringComparison.OrdinalIgnoreCase); + + // Folder that isn't a git repo + var ex2 = await Assert.ThrowsAsync( + () => rm.AddRepositoryFromLocalAsync(notGit)); + Assert.Contains("not a git repository", ex2.Message, StringComparison.OrdinalIgnoreCase); + + // Git repo without origin remote + var ex3 = await Assert.ThrowsAsync( + () => rm.AddRepositoryFromLocalAsync(noOrigin)); + Assert.Contains("origin", ex3.Message, StringComparison.OrdinalIgnoreCase); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(notGit); + ForceDeleteDirectory(noOrigin); + ForceDeleteDirectory(testBaseDir); + } + } + } diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index fa18dba7b..19998df1c 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -61,6 +61,156 @@ public void NormalizeRepoUrl_NonShorthand_PassesThrough(string input) Assert.Equal(input, RepoManager.NormalizeRepoUrl(input)); } + // ─── RepoNameFromUrl tests (Issue #570: picker shows ambiguous last-word names) ─── + + [Theory] + [InlineData("https://github.com/dotnet/maui", "maui")] + [InlineData("https://github.com/nicknisi/vscode-maui", "vscode-maui")] + [InlineData("https://github.com/PureWeen/PolyPilot", "PolyPilot")] + [InlineData("https://github.com/Owner/Repo.git", "Repo")] + [InlineData("https://gitlab.com/group/subgroup/repo.git", "repo")] + public void RepoNameFromUrl_Https_ExtractsRepoName(string url, string expected) + { + Assert.Equal(expected, RepoManager.RepoNameFromUrl(url)); + } + + [Theory] + [InlineData("git@github.com:Owner/Repo.git", "Repo")] + [InlineData("git@github.com:dotnet/maui", "maui")] + [InlineData("git@github.com:nicknisi/vscode-maui.git", "vscode-maui")] + public void RepoNameFromUrl_Ssh_ExtractsRepoName(string url, string expected) + { + Assert.Equal(expected, RepoManager.RepoNameFromUrl(url)); + } + + [Theory] + [InlineData(null, "dotnet-maui", "maui")] // fallback strips owner prefix + [InlineData(null, "PureWeen-PolyPilot", "PolyPilot")] + [InlineData(null, "single-word", "word")] // first dash is owner separator + [InlineData(null, "nodash", "nodash")] // no dash → return as-is + [InlineData("", "dotnet-maui", "maui")] + [InlineData(null, "dotnet-maui-local-a1b2c3d4", "maui")] // strips -local-{hash} before derivation + [InlineData(null, "Owner-Repo-local-12345678", "Repo")] + public void RepoNameFromUrl_FallbackFromId(string? url, string? fallbackId, string expected) + { + Assert.Equal(expected, RepoManager.RepoNameFromUrl(url, fallbackId)); + } + + [Fact] + public void RepoNameFromUrl_NullUrlAndNullId_ReturnsEmpty() + { + Assert.Equal("", RepoManager.RepoNameFromUrl(null, null)); + } + + [Fact] + public void RepoNameFromUrl_PreservesHyphensInRepoName() + { + // This is the key fix for issue #570: "vscode-maui" and "maui" should be distinguishable + var name1 = RepoManager.RepoNameFromUrl("https://github.com/nicknisi/vscode-maui"); + var name2 = RepoManager.RepoNameFromUrl("https://github.com/dotnet/maui"); + Assert.NotEqual(name1, name2); + Assert.Equal("vscode-maui", name1); + Assert.Equal("maui", name2); + } + + [Fact] + public void Load_MigratesOldStyleRepoNames() + { + // Repos saved with the old id.Split('-').Last() naming should be fixed on load. + var rm = new RepoManager(); + var tempDir = Path.Combine(Path.GetTempPath(), $"repomgr-migrate-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + + try + { + // Write state with old-style names (both repos named "maui" despite different URLs) + var oldJson = """ + { + "Repositories": [ + {"Id":"dotnet-maui","Name":"maui","Url":"https://github.com/dotnet/maui","BareClonePath":"","AddedAt":"2026-01-01T00:00:00Z"}, + {"Id":"nicknisi-vscode-maui","Name":"maui","Url":"https://github.com/nicknisi/vscode-maui","BareClonePath":"","AddedAt":"2026-01-01T00:00:00Z"} + ], + "Worktrees": [] + } + """; + File.WriteAllText(Path.Combine(tempDir, "repos.json"), oldJson); + + RepoManager.SetBaseDirForTesting(tempDir); + try + { + rm.Load(); + + var repos = rm.Repositories; + var dotnetMaui = repos.FirstOrDefault(r => r.Id == "dotnet-maui"); + var vscodeMaui = repos.FirstOrDefault(r => r.Id == "nicknisi-vscode-maui"); + + Assert.NotNull(dotnetMaui); + Assert.NotNull(vscodeMaui); + Assert.Equal("maui", dotnetMaui.Name); + Assert.Equal("vscode-maui", vscodeMaui.Name); + Assert.NotEqual(dotnetMaui.Name, vscodeMaui.Name); + } + finally + { + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); + } + } + finally + { + ForceDeleteDirectory(tempDir); + } + } + + [Fact] + public void Load_PreservesUserRenamedRepoNames() + { + // If the user renamed a repo from "maui" to "maui - PP", Load() migration must NOT + // overwrite it back to the URL-derived name. + var rm = new RepoManager(); + var tempDir = Path.Combine(Path.GetTempPath(), $"repomgr-rename-{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDir); + + try + { + // Repo with a user-customized name ("maui - PP" instead of "maui") + var json = """ + { + "Repositories": [ + {"Id":"dotnet-maui","Name":"maui - PP","Url":"https://github.com/dotnet/maui","BareClonePath":"","AddedAt":"2026-01-01T00:00:00Z"}, + {"Id":"nicknisi-vscode-maui","Name":"maui","Url":"https://github.com/nicknisi/vscode-maui","BareClonePath":"","AddedAt":"2026-01-01T00:00:00Z"} + ], + "Worktrees": [] + } + """; + File.WriteAllText(Path.Combine(tempDir, "repos.json"), json); + + RepoManager.SetBaseDirForTesting(tempDir); + try + { + rm.Load(); + + var repos = rm.Repositories; + var dotnetMaui = repos.FirstOrDefault(r => r.Id == "dotnet-maui"); + var vscodeMaui = repos.FirstOrDefault(r => r.Id == "nicknisi-vscode-maui"); + + Assert.NotNull(dotnetMaui); + Assert.NotNull(vscodeMaui); + // User-customized name must be preserved + Assert.Equal("maui - PP", dotnetMaui.Name); + // Old-style name ("maui" via Split('-').Last()) should still migrate + Assert.Equal("vscode-maui", vscodeMaui.Name); + } + finally + { + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); + } + } + finally + { + ForceDeleteDirectory(tempDir); + } + } + #region Save Guard Tests (Review Finding #9) private static readonly System.Reflection.BindingFlags NonPublic = @@ -997,35 +1147,10 @@ public async Task RemoveWorktreeAsync_NoBareClone_ExternalPath_DoesNotDeleteDire #region CreateWorktreeAsync Path Strategy Tests [Fact] - public void CreateWorktree_WithLocalPath_PlacesWorktreeInsideLocalRepo() - { - // When localPath is provided, the worktree path should be: - // {localPath}/.polypilot/worktrees/{branchName} - // This is the "nested strategy" that keeps worktrees inside the user's repo. - - var localRepoPath = Path.Combine(Path.GetTempPath(), "my-local-repo"); - var branchName = "feature-login"; - var repoWorktreesDir = Path.Combine(localRepoPath, ".polypilot", "worktrees"); - var expectedPath = Path.Combine(repoWorktreesDir, branchName); - var resolved = Path.GetFullPath(expectedPath); - var managedBase = Path.GetFullPath(repoWorktreesDir) + Path.DirectorySeparatorChar; - - // Verify path is inside the managed dir (passes the guard) - Assert.True(resolved.StartsWith(managedBase, StringComparison.OrdinalIgnoreCase), - $"Expected path '{resolved}' to be inside '{managedBase}'"); - - // Verify it is NOT under the centralized worktrees dir - var centralDir = Path.Combine(Path.GetTempPath(), ".polypilot", "worktrees"); - Assert.False(resolved.StartsWith(Path.GetFullPath(centralDir), StringComparison.OrdinalIgnoreCase), - "Nested worktree path should NOT be under the centralized worktrees dir"); - } - - [Fact] - public void CreateWorktree_WithoutLocalPath_PlacesWorktreeInCentralDir() + public void CreateWorktree_AlwaysPlacesWorktreeInCentralDir() { - // When localPath is null, the worktree path should be: - // {WorktreesDir}/{repoId}-{guid8} - // This is the "centralized strategy" for URL-based groups. + // All worktrees should go to {WorktreesDir}/{repoId}-{guid8} + // (centralized strategy — nested strategy was removed). var testBaseDir = Path.Combine(Path.GetTempPath(), $"central-strategy-{Guid.NewGuid():N}"); var worktreesDir = Path.Combine(testBaseDir, "worktrees"); @@ -1037,24 +1162,181 @@ public void CreateWorktree_WithoutLocalPath_PlacesWorktreeInCentralDir() Assert.True(expectedPath.StartsWith(worktreesDir, StringComparison.OrdinalIgnoreCase), $"Centralized path '{expectedPath}' should be under WorktreesDir '{worktreesDir}'"); - // Verify it does NOT contain .polypilot/worktrees (which would indicate nested) + // Verify it does NOT contain .polypilot/worktrees (which would indicate old nested strategy) var marker = Path.Combine(".polypilot", "worktrees"); Assert.DoesNotContain(marker, expectedPath, StringComparison.OrdinalIgnoreCase); } + #endregion + + #region Existing Folder Safety Tests + [Fact] - public void CreateWorktree_LocalPath_StrategySelectedByNullCheck() + public async Task RemoveRepository_DeleteFromDisk_SkipsNonManagedBareClonePath() { - // Regression: the localPath parameter is the SOLE discriminator between nested - // and centralized strategy. Verify that an empty/whitespace localPath would NOT - // accidentally trigger the nested path (same guard that CreateWorktreeAsync uses). + // Behavioral test: repos added via "Existing Folder" have BareClonePath pointing + // at the user's real project directory. RemoveRepositoryAsync with deleteFromDisk + // must NOT delete it — only managed bare clones under ReposDir should be deleted. + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + var userProject = Path.Combine(Path.GetTempPath(), $"user-project-{Guid.NewGuid():N}"); + Directory.CreateDirectory(testBaseDir); + Directory.CreateDirectory(userProject); + File.WriteAllText(Path.Combine(userProject, "important.txt"), "don't delete me"); + + try + { + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + var rm = new RepoManager(); + rm.Load(); + + // Manually add a repo that mimics "Existing Folder" — BareClonePath points + // at the user's project, NOT under the managed ReposDir. + var stateField = typeof(RepoManager).GetField("_state", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + var state = (RepositoryState)stateField.GetValue(rm)!; + state.Repositories.Add(new RepositoryInfo + { + Id = "local-repo", Name = "test", Url = "https://github.com/test/test", + BareClonePath = userProject, AddedAt = DateTime.UtcNow + }); + + // Call the actual production method with deleteFromDisk: true + await rm.RemoveRepositoryAsync("local-repo", deleteFromDisk: true); + + // The user's project directory must still exist — never deleted + Assert.True(Directory.Exists(userProject), "User's project directory was deleted!"); + Assert.True(File.Exists(Path.Combine(userProject, "important.txt")), + "User's files were deleted!"); + + // The repo should be removed from state + Assert.DoesNotContain(rm.Repositories, r => r.Id == "local-repo"); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(testBaseDir); + ForceDeleteDirectory(userProject); + } + } - // Production code: if (!string.IsNullOrWhiteSpace(localPath)) → nested - Assert.True(string.IsNullOrWhiteSpace(null)); - Assert.True(string.IsNullOrWhiteSpace("")); - Assert.True(string.IsNullOrWhiteSpace(" ")); - Assert.False(string.IsNullOrWhiteSpace("/valid/path")); - Assert.False(string.IsNullOrWhiteSpace(@"C:\valid\path")); + [Fact] + public async Task RemoveRepository_DeleteFromDisk_DeletesManagedBareClone() + { + // Complementary test: managed bare clones under ReposDir SHOULD be deleted. + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(testBaseDir); + + try + { + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + var rm = new RepoManager(); + rm.Load(); + + // Create a managed bare clone directory under ReposDir + var reposDir = Path.Combine(testBaseDir, "repos"); + var barePath = Path.Combine(reposDir, "test-repo.git"); + Directory.CreateDirectory(barePath); + File.WriteAllText(Path.Combine(barePath, "HEAD"), "ref: refs/heads/main"); + + var stateField = typeof(RepoManager).GetField("_state", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + var state = (RepositoryState)stateField.GetValue(rm)!; + state.Repositories.Add(new RepositoryInfo + { + Id = "managed-repo", Name = "test", Url = "https://github.com/test/test", + BareClonePath = barePath, AddedAt = DateTime.UtcNow + }); + + await rm.RemoveRepositoryAsync("managed-repo", deleteFromDisk: true); + + // Managed bare clone should be deleted + Assert.False(Directory.Exists(barePath), "Managed bare clone was not deleted"); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public void WorktreeReuse_OnlyMatchesCentralizedWorktrees() + { + // Behavioral test: worktree reuse must only return worktrees under the centralized + // WorktreesDir, not external user checkouts registered via "Existing Folder". + var testBaseDir = Path.Combine(Path.GetTempPath(), $"rmtest-{Guid.NewGuid():N}"); + Directory.CreateDirectory(testBaseDir); + try + { + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + var rm = new RepoManager(); + rm.Load(); + + var worktreesDir = Path.Combine(testBaseDir, "worktrees"); + var managedWtPath = Path.Combine(worktreesDir, "test-repo-abc12345"); + var userCheckout = Path.Combine(Path.GetTempPath(), $"user-checkout-{Guid.NewGuid():N}"); + Directory.CreateDirectory(managedWtPath); + Directory.CreateDirectory(userCheckout); + + // Inject state with two worktrees for the same repo+branch: + // one managed (under WorktreesDir) and one external (user checkout) + var stateField = typeof(RepoManager).GetField("_state", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + var state = (RepositoryState)stateField.GetValue(rm)!; + state.Repositories.Add(new RepositoryInfo + { + Id = "test-repo", Name = "test", Url = "https://github.com/test/test", + BareClonePath = Path.Combine(testBaseDir, "repos", "test-repo.git"), + AddedAt = DateTime.UtcNow + }); + state.Worktrees.Add(new WorktreeInfo + { + Id = "ext-wt", RepoId = "test-repo", Branch = "main", + Path = userCheckout // External — should NOT be matched for reuse + }); + state.Worktrees.Add(new WorktreeInfo + { + Id = "mgd-wt", RepoId = "test-repo", Branch = "main", + Path = managedWtPath // Managed — would be matched (if valid) + }); + + // The managed worktree prefix check is the key invariant: + var managedPrefix = Path.GetFullPath(worktreesDir) + Path.DirectorySeparatorChar; + var externalFull = Path.GetFullPath(userCheckout); + var managedFull = Path.GetFullPath(managedWtPath); + + Assert.False(externalFull.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase), + "External user checkout must NOT be under WorktreesDir"); + Assert.True(managedFull.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase), + "Managed worktree must be under WorktreesDir"); + + // Verify by querying worktrees: only managed worktree paths should match the prefix + var worktrees = rm.Worktrees; + var reuseMatches = worktrees.Where(w => + w.RepoId == "test-repo" + && string.Equals(w.Branch, "main", StringComparison.OrdinalIgnoreCase) + && !string.IsNullOrWhiteSpace(w.Path) + && Path.GetFullPath(w.Path).StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase)).ToList(); + + Assert.Single(reuseMatches); + Assert.Equal("mgd-wt", reuseMatches[0].Id); + + ForceDeleteDirectory(userCheckout); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(testBaseDir); + } } #endregion @@ -1145,5 +1427,349 @@ private static Task RunProcess(string exe, params string[] args) } #endregion -} + #region WorktreeDirName tests + + [Theory] + [InlineData("dotnet-maui", "ab12cd34", "dotnet-maui-ab12cd34")] + [InlineData("Owner-Repo", "deadbeef", "Owner-Repo-deadbeef")] + [InlineData("PureWeen-PolyPilot", "11223344", "PureWeen-PolyPilot-11223344")] + public void WorktreeDirName_NormalRepoId_UsesFullId(string repoId, string wtId, string expected) + { + Assert.Equal(expected, RepoManager.WorktreeDirName(repoId, wtId)); + } + + [Theory] + [InlineData("dotnet-maui-local-a1b2c3d4", "deadbeef", "dotnet-maui-deadbeef")] + [InlineData("Owner-Repo-local-12345678", "aabbccdd", "Owner-Repo-aabbccdd")] + public void WorktreeDirName_LocalRepoId_StripsLocalSuffix(string repoId, string wtId, string expected) + { + Assert.Equal(expected, RepoManager.WorktreeDirName(repoId, wtId)); + } + + [Fact] + public void WorktreeDirName_VeryLongRepoId_TruncatesTo24Chars() + { + var longId = "very-long-organization-name-with-a-deeply-nested-repo"; + var result = RepoManager.WorktreeDirName(longId, "deadbeef"); + // 24 chars of id + "-" + 8 chars of guid = 33 chars max + Assert.Equal("very-long-organization-n-deadbeef", result); + Assert.True(result.Length <= 33, $"WorktreeDirName too long: {result.Length} chars"); + } + + [Fact] + public void WorktreeDirName_ShortRepoId_NotTruncated() + { + var result = RepoManager.WorktreeDirName("a-b", "12345678"); + Assert.Equal("a-b-12345678", result); + } + + #endregion + + #region DeterministicPathHash tests + + [Fact] + public void DeterministicPathHash_IsDeterministic() + { + var path = Path.Combine(Path.GetTempPath(), "some-repo-folder"); + var hash1 = RepoManager.DeterministicPathHash(path); + var hash2 = RepoManager.DeterministicPathHash(path); + Assert.Equal(hash1, hash2); + Assert.Matches(@"^[0-9a-f]{8}$", hash1); + } + + [Fact] + public void DeterministicPathHash_NormalizesTrailingSeparators() + { + var basePath = Path.Combine(Path.GetTempPath(), "some-repo"); + var withSep = basePath + Path.DirectorySeparatorChar; + Assert.Equal( + RepoManager.DeterministicPathHash(basePath), + RepoManager.DeterministicPathHash(withSep)); + } + + [Fact] + public void DeterministicPathHash_DifferentPathsProduceDifferentHashes() + { + var hash1 = RepoManager.DeterministicPathHash(Path.Combine(Path.GetTempPath(), "repo-a")); + var hash2 = RepoManager.DeterministicPathHash(Path.Combine(Path.GetTempPath(), "repo-b")); + Assert.NotEqual(hash1, hash2); + } + + #endregion + + #region IsValidWorktreeAsync tests + + [Fact] + public async Task IsValidWorktreeAsync_NonExistentDir_ReturnsFalse() + { + var rm = new RepoManager(); + var result = await rm.IsValidWorktreeAsync( + Path.Combine(Path.GetTempPath(), $"no-such-dir-{Guid.NewGuid():N}"), + CancellationToken.None); + Assert.False(result); + } + + [Fact] + public async Task IsValidWorktreeAsync_EmptyDir_ReturnsFalse() + { + var dir = Path.Combine(Path.GetTempPath(), $"empty-dir-{Guid.NewGuid():N}"); + Directory.CreateDirectory(dir); + try + { + var rm = new RepoManager(); + var result = await rm.IsValidWorktreeAsync(dir, CancellationToken.None); + Assert.False(result, "Empty directory (no .git) should not be considered a valid worktree"); + } + finally { ForceDeleteDirectory(dir); } + } + + [Fact] + public async Task IsValidWorktreeAsync_ValidGitRepo_ReturnsTrue() + { + var dir = Path.Combine(Path.GetTempPath(), $"valid-wt-{Guid.NewGuid():N}"); + Directory.CreateDirectory(dir); + try + { + await RunProcess("git", "init", dir); + await RunProcess("git", "-C", dir, "config", "user.email", "test@test.com"); + await RunProcess("git", "-C", dir, "config", "user.name", "Test"); + await RunProcess("git", "-C", dir, "commit", "--allow-empty", "-m", "init"); + + var rm = new RepoManager(); + var result = await rm.IsValidWorktreeAsync(dir, CancellationToken.None); + Assert.True(result, "Valid git repo should be considered a valid worktree"); + } + finally { ForceDeleteDirectory(dir); } + } + + [Fact] + public async Task IsValidWorktreeAsync_CorruptGitDir_ReturnsFalse() + { + var dir = Path.Combine(Path.GetTempPath(), $"corrupt-wt-{Guid.NewGuid():N}"); + Directory.CreateDirectory(dir); + try + { + // Create a .git file with garbage content to simulate corruption + File.WriteAllText(Path.Combine(dir, ".git"), "gitdir: /nonexistent/path/that/does/not/exist"); + + var rm = new RepoManager(); + var result = await rm.IsValidWorktreeAsync(dir, CancellationToken.None); + Assert.False(result, "Corrupt .git file should not be considered a valid worktree"); + } + finally { ForceDeleteDirectory(dir); } + } + + #endregion + + #region Worktree creation lock tests + + [Fact] + public void WorktreeCreationLocks_AreSerialized() + { + // Verify the _worktreeCreationLocks field exists and is a ConcurrentDictionary. + // This is a structural test — the semaphore-based locking prevents two concurrent + // CreateWorktreeAsync calls for the same branch from racing on `git worktree add`. + var rm = new RepoManager(); + var field = typeof(RepoManager).GetField("_worktreeCreationLocks", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); + Assert.NotNull(field); + var value = field!.GetValue(rm); + Assert.IsType>(value); + } + + #endregion + + #region PathsEqual null/empty safety tests + + [Fact] + public void PathsEqual_NullLeft_ReturnsFalse() + { + // PathsEqual must handle null without throwing ArgumentNullException (finding #13) + var method = typeof(RepoManager).GetMethod("PathsEqual", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var result = (bool)method.Invoke(null, new object?[] { null, Path.GetTempPath() })!; + Assert.False(result); + } + + [Fact] + public void PathsEqual_EmptyLeft_ReturnsFalse() + { + var method = typeof(RepoManager).GetMethod("PathsEqual", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var result = (bool)method.Invoke(null, new object?[] { "", Path.GetTempPath() })!; + Assert.False(result); + } + + [Fact] + public void PathsEqual_BothNull_ReturnsFalse() + { + var method = typeof(RepoManager).GetMethod("PathsEqual", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var result = (bool)method.Invoke(null, new object?[] { null, null })!; + Assert.False(result); + } + + [Fact] + public void PathsEqual_WhitespaceLeft_ReturnsFalse() + { + var method = typeof(RepoManager).GetMethod("PathsEqual", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var result = (bool)method.Invoke(null, new object?[] { " ", Path.GetTempPath() })!; + Assert.False(result); + } + + #endregion + + #region Worktree reuse behavioral test + + [Fact] + public async Task CreateWorktreeAsync_SameBranch_ReusesExistingWorktree() + { + // End-to-end behavioral test: when a valid managed worktree already exists for + // the requested repo + branch, CreateWorktreeAsync should return it (reuse) + // instead of creating a second one. + var testBaseDir = Path.Combine(Path.GetTempPath(), $"wt-reuse-{Guid.NewGuid():N}"); + Directory.CreateDirectory(testBaseDir); + + try + { + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + var rm = new RepoManager(); + rm.Load(); + + var worktreesDir = Path.Combine(testBaseDir, "worktrees"); + var wtPath = Path.Combine(worktreesDir, "test-repo-abc12345"); + Directory.CreateDirectory(wtPath); + + // Create a fake BareClonePath that looks like a non-bare repo (has .git dir) + // so EnsureRepoCloneInCurrentRootAsync early-returns without calling git. + var fakeBarePath = Path.Combine(testBaseDir, "fake-repo"); + Directory.CreateDirectory(fakeBarePath); + Directory.CreateDirectory(Path.Combine(fakeBarePath, ".git")); + + // Create a minimal .git directory in the worktree so IsValidWorktreeAsync passes + Directory.CreateDirectory(Path.Combine(wtPath, ".git")); + Directory.CreateDirectory(Path.Combine(wtPath, ".git", "refs")); + Directory.CreateDirectory(Path.Combine(wtPath, ".git", "objects")); + File.WriteAllText(Path.Combine(wtPath, ".git", "HEAD"), "ref: refs/heads/feature-x\n"); + + // Inject state: repo + one existing managed worktree + var stateField = typeof(RepoManager).GetField("_state", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + var state = (RepositoryState)stateField.GetValue(rm)!; + state.Repositories.Add(new RepositoryInfo + { + Id = "test-repo", + Name = "test", + Url = "https://github.com/test/test", + BareClonePath = fakeBarePath, + AddedAt = DateTime.UtcNow + }); + state.Worktrees.Add(new WorktreeInfo + { + Id = "abc12345", + RepoId = "test-repo", + Branch = "feature-x", + Path = wtPath, + BareClonePath = fakeBarePath, + CreatedAt = DateTime.UtcNow + }); + + // Call CreateWorktreeAsync — it should find the existing worktree and reuse it + var wt = await rm.CreateWorktreeAsync("test-repo", "feature-x", skipFetch: true); + + Assert.NotNull(wt); + Assert.Equal("abc12345", wt.Id); + Assert.Equal(wtPath, wt.Path); + Assert.Equal("feature-x", wt.Branch); + + // Only one worktree should exist for this branch + var branchWorktrees = rm.Worktrees + .Where(w => w.RepoId == "test-repo" && w.Branch == "feature-x") + .ToList(); + Assert.Single(branchWorktrees); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(testBaseDir); + } + } + + [Fact] + public async Task CreateWorktreeAsync_StaleWorktree_EvictsAndDoesNotReuse() + { + // When a registered worktree is stale (directory doesn't exist or is corrupt), + // CreateWorktreeCoreAsync should evict the stale entry from state. + var testBaseDir = Path.Combine(Path.GetTempPath(), $"wt-stale-{Guid.NewGuid():N}"); + Directory.CreateDirectory(testBaseDir); + + try + { + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + var rm = new RepoManager(); + rm.Load(); + + var worktreesDir = Path.Combine(testBaseDir, "worktrees"); + var staleWtPath = Path.Combine(worktreesDir, "test-repo-stale123"); + // Don't create the directory — simulate a deleted/corrupt worktree + + // Create a fake BareClonePath that looks like a non-bare repo (has .git dir) + var fakeBarePath = Path.Combine(testBaseDir, "fake-repo"); + Directory.CreateDirectory(fakeBarePath); + Directory.CreateDirectory(Path.Combine(fakeBarePath, ".git")); + + var stateField = typeof(RepoManager).GetField("_state", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance)!; + var state = (RepositoryState)stateField.GetValue(rm)!; + state.Repositories.Add(new RepositoryInfo + { + Id = "test-repo", + Name = "test", + Url = "https://github.com/test/test", + BareClonePath = fakeBarePath, + AddedAt = DateTime.UtcNow + }); + state.Worktrees.Add(new WorktreeInfo + { + Id = "stale123", + RepoId = "test-repo", + Branch = "feature-x", + Path = staleWtPath, + BareClonePath = fakeBarePath, + CreatedAt = DateTime.UtcNow + }); + + // The stale worktree should be evicted from state. + // CreateWorktreeAsync will fail after eviction when it tries to run + // `git worktree add` (git is not available in test env), but the + // stale entry should be gone before that point. + try + { + await rm.CreateWorktreeAsync("test-repo", "feature-x", skipFetch: true); + } + catch + { + // Expected — no actual git to create worktree + } + + // Verify stale entry was evicted + var remaining = rm.Worktrees.Where(w => w.Id == "stale123").ToList(); + Assert.Empty(remaining); + } + finally { RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } + } + finally + { + ForceDeleteDirectory(testBaseDir); + } + } + + #endregion +} diff --git a/PolyPilot.Tests/WorktreeStrategyTests.cs b/PolyPilot.Tests/WorktreeStrategyTests.cs index 24281c1fb..d19523e14 100644 --- a/PolyPilot.Tests/WorktreeStrategyTests.cs +++ b/PolyPilot.Tests/WorktreeStrategyTests.cs @@ -47,7 +47,7 @@ public FakeRepoManager(List repos) } public override Task CreateWorktreeAsync(string repoId, string branchName, - string? baseBranch = null, bool skipFetch = false, string? localPath = null, CancellationToken ct = default) + string? baseBranch = null, bool skipFetch = false, CancellationToken ct = default) { CreateCalls.Add((repoId, branchName, skipFetch)); var id = $"wt-{Interlocked.Increment(ref _worktreeCounter)}"; @@ -560,7 +560,7 @@ public FailingRepoManager(List repos) } public override Task CreateWorktreeAsync(string repoId, string branchName, - string? baseBranch = null, bool skipFetch = false, string? localPath = null, CancellationToken ct = default) + string? baseBranch = null, bool skipFetch = false, CancellationToken ct = default) { throw new InvalidOperationException("Simulated git failure"); } diff --git a/PolyPilot/Components/Layout/SessionSidebar.razor b/PolyPilot/Components/Layout/SessionSidebar.razor index 9f2dde60b..66e8d2fdb 100644 --- a/PolyPilot/Components/Layout/SessionSidebar.razor +++ b/PolyPilot/Components/Layout/SessionSidebar.razor @@ -1004,13 +1004,12 @@ else @if (!string.IsNullOrEmpty(group.RepoId)) { - @* 📁 group backed by a bare clone — offer full branch/worktree features *@ + @* 📁 group backed by a repo — offer full branch/worktree features *@ var lfRepoId = group.RepoId!; - var lfLocalPath = group.LocalPath!; - - } @@ -1957,7 +1956,6 @@ else // Quick-create inline branch input private string? quickBranchRepoId = null; private string? quickBranchGroupId = null; - private string? quickBranchLocalPath = null; private string quickBranchInput = ""; private bool quickBranchIsCreating = false; private string? quickBranchError = null; @@ -2568,7 +2566,7 @@ else } } - private async Task QuickCreateSessionForRepo(string repoId, string? targetGroupId = null, string? localPath = null) + private async Task QuickCreateSessionForRepo(string repoId, string? targetGroupId = null) { if (isCreating) return; isCreating = true; @@ -2580,8 +2578,7 @@ else var sessionInfo = await CopilotService.CreateSessionWithWorktreeAsync( repoId: repoId, model: selectedModel, - targetGroupId: targetGroupId, - localPath: localPath); + targetGroupId: targetGroupId); CopilotService.SaveUiState(currentPage, selectedModel: selectedModel); await OnSessionSelected.InvokeAsync(); } @@ -2598,11 +2595,10 @@ else } } - private void StartQuickBranch(string repoId, string? targetGroupId = null, string? localPath = null) + private void StartQuickBranch(string repoId, string? targetGroupId = null) { quickBranchRepoId = repoId; quickBranchGroupId = targetGroupId; - quickBranchLocalPath = localPath; quickBranchInput = ""; quickBranchError = null; } @@ -2610,7 +2606,7 @@ else private async Task HandleQuickBranchKeyDown(KeyboardEventArgs e, string repoId) { if (e.Key == "Enter") await CommitQuickBranch(repoId); - else if (e.Key == "Escape") { quickBranchRepoId = null; quickBranchLocalPath = null; } + else if (e.Key == "Escape") { quickBranchRepoId = null; } } private async Task CommitQuickBranch(string repoId) @@ -2648,12 +2644,10 @@ else branchName: branchName, prNumber: prNumber, model: selectedModel, - targetGroupId: quickBranchGroupId, - localPath: quickBranchLocalPath); + targetGroupId: quickBranchGroupId); quickBranchRepoId = null; quickBranchGroupId = null; - quickBranchLocalPath = null; quickBranchInput = ""; CopilotService.SaveUiState(currentPage, selectedModel: selectedModel); await OnSessionSelected.InvokeAsync(); diff --git a/PolyPilot/Services/CopilotService.Organization.cs b/PolyPilot/Services/CopilotService.Organization.cs index 3cecd31a3..9c8fe1ef2 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -650,6 +650,31 @@ internal void ReconcileOrganization(bool allowPruning = true) if (GetOrCreateRepoGroup(repo.Id, repo.Name) != null) changed = true; } + + // Migration: update group names that were derived from id.Split('-').Last() (issue #570). + // E.g., groups named "maui" for repo "nicknisi-vscode-maui" should become "vscode-maui". + // Only migrate names that still match the old broken derivation — if the user + // renamed the group (e.g., "maui - PP"), preserve their customization. + foreach (var g in Organization.Groups.Where(g => g.RepoId == repo.Id && !g.IsMultiAgent && !g.IsLocalFolder)) + { + var correctName = repo.Name; + if (string.IsNullOrEmpty(correctName) || g.Name == correctName) + continue; + if (Organization.Groups.Any(other => other != g && other.RepoId == repo.Id && other.Name == correctName && !other.IsMultiAgent && !other.IsLocalFolder)) + continue; + + // The old code derived names via id.Split('-').Last(). Only overwrite if + // the current group name matches that old pattern — otherwise user renamed it. + var oldDerivedName = repo.Id.Contains('-') + ? repo.Id.Split('-').Last() + : repo.Id; + if (string.Equals(g.Name, oldDerivedName, StringComparison.Ordinal)) + { + Debug($"ReconcileOrganization: migrating group name '{g.Name}' → '{correctName}' (repoId: {repo.Id})"); + g.Name = correctName; + changed = true; + } + } } // Migration: back-fill LocalPath/RepoId on groups that were created by an older version diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index c38c4d6d0..734807e4a 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -3074,7 +3074,6 @@ public async Task CreateSessionWithWorktreeAsync( string? model = null, string? initialPrompt = null, string? targetGroupId = null, - string? localPath = null, CancellationToken ct = default) { // Remote mode: send the entire operation to the server as a single atomic command. @@ -3150,7 +3149,7 @@ await _bridgeClient.CreateSessionWithWorktreeAsync(new CreateSessionWithWorktree else { var branch = branchName ?? $"session-{DateTime.Now:yyyyMMdd-HHmmss}"; - wt = await _repoManager.CreateWorktreeAsync(repoId, branch, null, localPath: localPath, ct: ct); + wt = await _repoManager.CreateWorktreeAsync(repoId, branch, null, ct: ct); } // Derive a friendly display name: prefer explicit sessionName, then branch name, diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index a05e16241..82ea75679 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -1,5 +1,7 @@ using System.Collections.Concurrent; using System.Diagnostics; +using System.Security.Cryptography; +using System.Text; using System.Text.Json; using PolyPilot.Models; @@ -41,6 +43,12 @@ internal static void SetBaseDirForTesting(string? path) private bool _loaded; private bool _loadedSuccessfully; private readonly object _stateLock = new(); + + // Serializes concurrent worktree creation for the same repo+branch to prevent + // two calls from both passing the reuse check and racing on `git worktree add -b`. + // Entries grow unboundedly (~48 bytes each) but are bounded by user activity + // (typically hundreds of branches). Acceptable trade-off for a desktop app. + private readonly ConcurrentDictionary _worktreeCreationLocks = new(); public IReadOnlyList Repositories { get @@ -141,6 +149,32 @@ public void Load() } } if (changed) Save(); + + // Migration: fix repo names derived from id.Split('-').Last() (issue #570). + // Repos added before this fix have names like "maui" for both "dotnet-maui" and + // "nicknisi-vscode-maui". Re-derive from the URL so they become "maui" vs "vscode-maui". + // Only migrate names that still match the old broken derivation — if the user + // renamed a repo (e.g., "maui - PP"), preserve their customization. + foreach (var repo in _state.Repositories) + { + var correctName = RepoNameFromUrl(repo.Url, fallbackId: repo.Id); + if (string.IsNullOrEmpty(correctName) || repo.Name == correctName) + continue; + + // The old code derived names via id.Split('-').Last(). Only overwrite if + // the current name matches that old pattern — otherwise it's user-customized. + var oldDerivedName = repo.Id.Contains('-') + ? repo.Id.Split('-').Last() + : repo.Id; + if (string.Equals(repo.Name, oldDerivedName, StringComparison.Ordinal)) + { + Console.WriteLine($"[RepoManager] Migrating repo name: '{repo.Name}' → '{correctName}' (id: {repo.Id})"); + repo.Name = correctName; + changed = true; + } + } + if (changed) Save(); + _loadedSuccessfully = true; } catch (Exception ex) @@ -203,7 +237,7 @@ internal int HealMissingRepos() } catch { /* best effort */ } - var name = repoId.Contains('-') ? repoId.Split('-').Last() : repoId; + var name = RepoNameFromUrl(url, fallbackId: repoId); _state.Repositories.Add(new RepositoryInfo { Id = repoId, @@ -229,14 +263,23 @@ internal int HealMissingRepos() if (trackedWorktreePaths.Contains(wtDir)) continue; var dirName = Path.GetFileName(wtDir); - // Worktree dirs are named "{repoId}-{guid8}" — find the repo ID + // Worktree dirs are named "{repoId}-{guid8}" or "{abbreviated}-{guid8}" + // (WorktreeDirName may shorten the repo ID to save path length). var lastDash = dirName.LastIndexOf('-'); if (lastDash < 0) continue; var candidateRepoId = dirName[..lastDash]; var worktreeId = dirName[(lastDash + 1)..]; - if (!trackedIds.Contains(candidateRepoId)) continue; + // Match exact repo ID first; if that fails, check whether any + // tracked repo produces this abbreviated name via WorktreeDirName. + if (!trackedIds.Contains(candidateRepoId)) + { + var matchingRepo = _state.Repositories.FirstOrDefault(r => + WorktreeDirName(r.Id, worktreeId) == dirName); + if (matchingRepo == null) continue; + candidateRepoId = matchingRepo.Id; + } if (!Directory.Exists(Path.Combine(wtDir, ".git"))) continue; // Get the branch name @@ -301,7 +344,11 @@ private void Save() var stateFile = StateFile; // resolve once Directory.CreateDirectory(Path.GetDirectoryName(stateFile)!); var json = JsonSerializer.Serialize(_state, new JsonSerializerOptions { WriteIndented = true }); - File.WriteAllText(stateFile, json); + // Atomic write: write to .tmp then rename, so a crash during write + // doesn't leave repos.json truncated/corrupt. + var tmp = stateFile + ".tmp"; + File.WriteAllText(tmp, json); + File.Move(tmp, stateFile, overwrite: true); } catch (Exception ex) { @@ -359,6 +406,65 @@ public static string RepoIdFromUrl(string url) return fallback; } + /// + /// Extracts the repository name (last path segment) from a git URL. + /// Unlike which replaces "/" with "-" (losing the distinction + /// between owner separator and dashes in the repo name), this returns just the repo name. + /// e.g. "https://github.com/dotnet/maui" → "maui", + /// "https://github.com/nicknisi/vscode-maui" → "vscode-maui" + /// Falls back to the full ID if no URL is available. + /// + public static string RepoNameFromUrl(string? url, string? fallbackId = null) + { + if (!string.IsNullOrWhiteSpace(url)) + { + // SCP-style SSH: git@github.com:Owner/Repo.git + if (url.Contains('@') && url.Contains(':') && !url.Contains("://")) + { + var path = url.Split(':').Last().TrimEnd('/'); + var segments = path.Split('/'); + var name = segments[^1]; + if (name.EndsWith(".git", StringComparison.OrdinalIgnoreCase)) + name = name[..^4]; + if (!string.IsNullOrWhiteSpace(name)) + return name; + } + // HTTPS, ssh://, and other protocol URLs + else if (Uri.TryCreate(url, UriKind.Absolute, out var uri)) + { + var segments = uri.AbsolutePath.Trim('/').Split('/'); + var name = segments[^1]; + if (name.EndsWith(".git", StringComparison.OrdinalIgnoreCase)) + name = name[..^4]; + if (!string.IsNullOrWhiteSpace(name)) + return name; + } + // Fallback: treat as path + else + { + var segments = url.Trim('/').Split('/'); + var name = segments[^1]; + if (name.EndsWith(".git", StringComparison.OrdinalIgnoreCase)) + name = name[..^4]; + if (!string.IsNullOrWhiteSpace(name)) + return name; + } + } + // No URL — derive from ID (best effort) + if (!string.IsNullOrWhiteSpace(fallbackId)) + { + // Strip "-local-{hash}" suffix before deriving name so local repo IDs like + // "dotnet-maui-local-a1b2c3d4" produce "maui" instead of "maui-local-a1b2c3d4". + var cleanId = fallbackId; + var localIdx = cleanId.IndexOf("-local-", StringComparison.Ordinal); + if (localIdx > 0) + cleanId = cleanId[..localIdx]; + var dashIdx = cleanId.IndexOf('-'); + return dashIdx >= 0 ? cleanId[(dashIdx + 1)..] : cleanId; + } + return ""; + } + /// /// Normalizes a repository input. Accepts full URLs, SSH paths, or GitHub shorthand (e.g. "dotnet/maui"). /// @@ -385,11 +491,73 @@ public static string NormalizeRepoUrl(string input) private string GetDesiredBareClonePath(string repoId) => Path.Combine(ReposDir, $"{repoId}.git"); - private static bool PathsEqual(string left, string right) + /// + /// Returns a shortened directory name for a worktree: {abbreviated-repoId}-{worktreeId}. + /// Keeps the worktree path short to avoid exceeding Windows MAX_PATH (260 chars) + /// for repos with deeply nested file structures (e.g., dotnet/maui). + /// + internal static string WorktreeDirName(string repoId, string worktreeId) { + // Strip "-local-{hash}" suffixes from "Existing Folder" repo IDs to shorten the path. + // e.g., "dotnet-maui-local-a1b2c3d4" → "dotnet-maui" + var abbreviated = repoId; + var localIdx = abbreviated.IndexOf("-local-", StringComparison.Ordinal); + if (localIdx > 0) + abbreviated = abbreviated[..localIdx]; + + // Cap at 24 chars to leave headroom for deeply-nested repo content. + // Full path budget: ~45 chars (base) + abbreviated + "-" + 8 (guid) + repo-internal path + // Note: Two repos whose IDs share the first 24 chars would produce the same abbreviated + // prefix. HealMissingRepos handles this via WorktreeDirName reverse-lookup which may + // match the wrong repo in that edge case. Acceptable trade-off since this only affects + // the self-healing recovery path, not normal worktree creation. + const int maxRepoIdLen = 24; + if (abbreviated.Length > maxRepoIdLen) + abbreviated = abbreviated[..maxRepoIdLen]; + + return $"{abbreviated}-{worktreeId}"; + } + + /// + /// Enable git core.longpaths on Windows so git can handle paths exceeding + /// MAX_PATH (260 chars). No-op on non-Windows platforms. Best-effort — failures are + /// swallowed because the operation is advisory and the repo is still usable for + /// shorter-path content. + /// + private async Task EnsureLongPathsAsync(string repoPath, CancellationToken ct) + { + if (!OperatingSystem.IsWindows()) return; + try { await RunGitAsync(repoPath, ct, "config", "core.longpaths", "true"); } catch { } + } + + private static bool PathsEqual(string? left, string? right) + { + if (string.IsNullOrWhiteSpace(left) || string.IsNullOrWhiteSpace(right)) + return false; var normalizedLeft = Path.GetFullPath(left).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); var normalizedRight = Path.GetFullPath(right).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); - return string.Equals(normalizedLeft, normalizedRight, StringComparison.OrdinalIgnoreCase); + // Windows and macOS use case-insensitive filesystems; Linux uses case-sensitive. + var comparison = (OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + return string.Equals(normalizedLeft, normalizedRight, comparison); + } + + /// + /// Returns a deterministic 8-hex-char hash of a file path, suitable for use in persistent IDs. + /// Uses SHA256 instead of which is randomized per-process + /// in .NET Core 3.0+ and must not be persisted. + /// Case-folding is applied on Windows/macOS (case-insensitive FS) but not on Linux. + /// + internal static string DeterministicPathHash(string path) + { + var normalized = Path.GetFullPath(path) + .TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + // Windows and macOS use case-insensitive filesystems; Linux uses case-sensitive. + if (OperatingSystem.IsWindows() || OperatingSystem.IsMacOS()) + normalized = normalized.ToLowerInvariant(); + var bytes = SHA256.HashData(Encoding.UTF8.GetBytes(normalized)); + return Convert.ToHexString(bytes)[..8].ToLowerInvariant(); } private void BackfillWorktreeClonePaths(RepositoryInfo repo) @@ -402,16 +570,29 @@ private void BackfillWorktreeClonePaths(RepositoryInfo repo) private async Task EnsureRepoCloneInCurrentRootAsync(RepositoryInfo repo, Action? onProgress, CancellationToken ct) { + // If BareClonePath points at a non-bare repo (added via "Existing Folder"), skip clone management + // but still enable long paths so worktree creation from this repo works on Windows. + if (!string.IsNullOrWhiteSpace(repo.BareClonePath) + && Directory.Exists(repo.BareClonePath) + && (Directory.Exists(Path.Combine(repo.BareClonePath, ".git")) || File.Exists(Path.Combine(repo.BareClonePath, ".git")))) + { + await EnsureLongPathsAsync(repo.BareClonePath, ct); + return; + } + var targetBarePath = GetDesiredBareClonePath(repo.Id); if (!string.IsNullOrWhiteSpace(repo.BareClonePath) && PathsEqual(repo.BareClonePath, targetBarePath) && Directory.Exists(targetBarePath)) + { + await EnsureLongPathsAsync(targetBarePath, ct); return; + } lock (_stateLock) BackfillWorktreeClonePaths(repo); Directory.CreateDirectory(ReposDir); - if (Directory.Exists(targetBarePath)) + if (Directory.Exists(targetBarePath) && await IsGitRepositoryAsync(targetBarePath, ct)) { onProgress?.Invoke($"Fetching {repo.Id}…"); try { await RunGitAsync(targetBarePath, ct, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); } catch { } @@ -419,6 +600,13 @@ private async Task EnsureRepoCloneInCurrentRootAsync(RepositoryInfo repo, Action } else { + // Remove any corrupt/partial directory left over from a failed clone + if (Directory.Exists(targetBarePath)) + { + Console.WriteLine($"[RepoManager] Removing corrupt bare clone at '{targetBarePath}'"); + try { Directory.Delete(targetBarePath, recursive: true); } catch { } + } + onProgress?.Invoke($"Cloning {repo.Url}…"); await RunGitWithProgressAsync(null, onProgress, ct, "clone", "--bare", "--progress", repo.Url, targetBarePath); await RunGitAsync(targetBarePath, ct, "config", "remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*"); @@ -426,10 +614,7 @@ private async Task EnsureRepoCloneInCurrentRootAsync(RepositoryInfo repo, Action await RunGitWithProgressAsync(targetBarePath, onProgress, ct, "fetch", "--progress", "origin"); } - if (OperatingSystem.IsWindows()) - { - try { await RunGitAsync(targetBarePath, ct, "config", "core.longpaths", "true"); } catch { } - } + await EnsureLongPathsAsync(targetBarePath, ct); lock (_stateLock) { @@ -472,17 +657,7 @@ public Task AddRepositoryAsync(string url, CancellationToken ct => AddRepositoryAsync(url, null, ct); public async Task AddRepositoryAsync(string url, Action? onProgress, CancellationToken ct = default) - => await AddRepositoryAsync(url, onProgress, localCloneSource: null, ct); - - /// - /// When non-null, clone from this local path instead of the remote URL. - /// The remote origin is then set to so future fetches go to the network. - /// This avoids a redundant network clone when the user adds an existing local repository. - /// - internal async Task AddRepositoryAsync(string url, Action? onProgress, string? localCloneSource, CancellationToken ct = default) { - if (localCloneSource != null && !Directory.Exists(localCloneSource)) - throw new ArgumentException($"Local clone source not found: '{localCloneSource}'", nameof(localCloneSource)); url = NormalizeRepoUrl(url); EnsureLoaded(); var id = RepoIdFromUrl(url); @@ -496,31 +671,23 @@ internal async Task AddRepositoryAsync(string url, Action /// Add a repository from an existing local path (non-bare). Validates the folder is a - /// git repository with an 'origin' remote, then registers and bare-clones it the same - /// way as . + /// git repository with an 'origin' remote, then either reuses an existing + /// (if one was already added via URL) or creates a new one + /// whose points directly at the user's local + /// repo — no bare clone is created. + /// If a repo with the same remote was already added via "Add from URL", the existing + /// repo (and its managed bare clone) is preserved; the local folder is only registered + /// as an external worktree. /// The local folder is also registered as an external worktree so it appears in the /// "📂 Existing" list when creating sessions. /// @@ -570,6 +739,8 @@ public async Task AddRepositoryFromLocalAsync( Action? onProgress = null, CancellationToken ct = default) { + EnsureLoaded(); + // Expand ~ so users can type ~/Projects/myrepo without hitting Directory.Exists failures. if (localPath.StartsWith("~", StringComparison.Ordinal)) localPath = Path.Combine( @@ -602,7 +773,94 @@ public async Task AddRepositoryFromLocalAsync( $"No 'origin' remote found in '{localPath}'. " + "The folder must have a remote named 'origin' (e.g. a GitHub clone)."); - var repo = await AddRepositoryAsync(remoteUrl, onProgress, localCloneSource: localPath, ct); + // Point BareClonePath at the user's existing repo — no bare clone needed. + var url = NormalizeRepoUrl(remoteUrl); + var baseId = RepoIdFromUrl(url); + + RepositoryInfo repo; + lock (_stateLock) + { + // Check if an existing repo with this remote already uses a different BareClonePath + // (e.g., a managed bare clone created via "Add from URL"). If so, the local folder + // needs its OWN RepositoryInfo with a distinct ID — otherwise both entries would + // share the same BareClonePath and the local folder would never be used directly. + var existing = _state.Repositories.FirstOrDefault(r => r.Id == baseId); + + // Check if this exact local path is already registered (idempotent re-add) + var existingLocal = _state.Repositories.FirstOrDefault(r => + !string.IsNullOrWhiteSpace(r.BareClonePath) && PathsEqual(r.BareClonePath, localPath)); + + if (existingLocal != null) + { + // Already have a repo pointing at this local folder — reuse it + repo = existingLocal; + } + else if (existing != null && !PathsEqual(existing.BareClonePath, localPath)) + { + // A repo with this remote already exists but points elsewhere (managed bare clone). + // Create a SEPARATE repo for the local folder with a unique ID. + // Use SHA256 for a deterministic hash — string.GetHashCode() is randomized per-process + // in .NET Core 3.0+ and must not be persisted. + var pathHash = DeterministicPathHash(localPath); + var localId = $"{baseId}-local-{pathHash}"; + + // Ensure we don't collide with an existing entry with this generated ID. + // Idempotency is guaranteed by the `existingLocal` path-match check above (line 781), + // which scans by BareClonePath regardless of ID format. + var alreadyLocal = _state.Repositories.FirstOrDefault(r => r.Id == localId); + if (alreadyLocal != null && PathsEqual(alreadyLocal.BareClonePath, localPath)) + { + // Idempotent hit — same ID, same path + repo = alreadyLocal; + } + else if (alreadyLocal != null) + { + // True hash collision — same ID, different path. Disambiguate with GUID. + localId = $"{baseId}-local-{Guid.NewGuid().ToString("N")[..8]}"; + repo = new RepositoryInfo + { + Id = localId, + Name = RepoNameFromUrl(url, fallbackId: baseId), + Url = url, + BareClonePath = localPath, + AddedAt = DateTime.UtcNow + }; + _state.Repositories.Add(repo); + } + else + { + repo = new RepositoryInfo + { + Id = localId, + Name = RepoNameFromUrl(url, fallbackId: baseId), + Url = url, + BareClonePath = localPath, + AddedAt = DateTime.UtcNow + }; + _state.Repositories.Add(repo); + } + } + else if (existing != null) + { + // Same repo, same BareClonePath — nothing to change + repo = existing; + } + else + { + // No existing repo for this remote — create one pointing at the local folder + repo = new RepositoryInfo + { + Id = baseId, + Name = RepoNameFromUrl(url, fallbackId: baseId), + Url = url, + BareClonePath = localPath, + AddedAt = DateTime.UtcNow + }; + _state.Repositories.Add(repo); + } + } + Save(); + OnStateChanged?.Invoke(); // Register the local folder as an external worktree so it also appears in the // "📂 Existing" picker when creating repo-based sessions. @@ -685,19 +943,79 @@ private async Task IsGitRepositoryAsync(string path, CancellationToken ct) } } + /// + /// Returns true if is a valid git worktree — the directory exists + /// and contains a .git file or directory that git can resolve. This is more robust + /// than a bare check because it detects corrupted or + /// partially-deleted worktrees that git can no longer use. + /// + internal async Task IsValidWorktreeAsync(string path, CancellationToken ct) + { + if (!Directory.Exists(path)) return false; + // Worktrees have a .git file (pointing at the bare clone's worktrees/ dir) + // or a .git directory (for standalone repos registered as external worktrees). + var gitPath = Path.Combine(path, ".git"); + if (!File.Exists(gitPath) && !Directory.Exists(gitPath)) return false; + return await IsGitRepositoryAsync(path, ct); + } + /// /// Create a new worktree for a repository on a new branch from origin/main. + /// If an existing registered worktree is already on the requested branch, it is reused. + /// Worktrees are always placed in the centralized ~/.polypilot/worktrees/ directory. /// - /// - /// Optional path to the user's existing local repo clone (added via "Add Existing Folder"). - /// When provided, the worktree is created at {localPath}/.polypilot/worktrees/{branchName}/ - /// (nested inside the user's repo) rather than the centralized ~/.polypilot/worktrees/. - /// - public virtual async Task CreateWorktreeAsync(string repoId, string branchName, string? baseBranch = null, bool skipFetch = false, string? localPath = null, CancellationToken ct = default) + public virtual async Task CreateWorktreeAsync(string repoId, string branchName, string? baseBranch = null, bool skipFetch = false, CancellationToken ct = default) { EnsureLoaded(); var repo = _state.Repositories.FirstOrDefault(r => r.Id == repoId) ?? throw new InvalidOperationException($"Repository '{repoId}' not found."); + + // Serialize concurrent worktree creation for the same repo+branch so two callers + // don't both pass the reuse check and race on `git worktree add -b `. + var lockKey = $"{repoId}:{branchName}".ToLowerInvariant(); + var semaphore = _worktreeCreationLocks.GetOrAdd(lockKey, _ => new SemaphoreSlim(1, 1)); + await semaphore.WaitAsync(ct); + try + { + return await CreateWorktreeCoreAsync(repo, repoId, branchName, baseBranch, skipFetch, ct); + } + finally + { + semaphore.Release(); + } + } + + private async Task CreateWorktreeCoreAsync(RepositoryInfo repo, string repoId, string branchName, string? baseBranch, bool skipFetch, CancellationToken ct) + { + // Check if an existing PolyPilot-managed worktree for this repo is already on the requested branch. + // Only reuse worktrees under the centralized WorktreesDir — never return the user's own + // checkout (registered as an external worktree) to avoid multiple sessions sharing it. + // Uses IsValidWorktreeAsync for a git-level health check instead of bare Directory.Exists. + WorktreeInfo? existingMatch; + var managedWorktreePrefix = Path.GetFullPath(WorktreesDir) + Path.DirectorySeparatorChar; + lock (_stateLock) + { + existingMatch = _state.Worktrees.FirstOrDefault(w => + w.RepoId == repoId + && string.Equals(w.Branch, branchName, StringComparison.OrdinalIgnoreCase) + && !string.IsNullOrWhiteSpace(w.Path) + && Path.GetFullPath(w.Path).StartsWith(managedWorktreePrefix, StringComparison.OrdinalIgnoreCase)); + } + if (existingMatch != null && await IsValidWorktreeAsync(existingMatch.Path, ct)) + { + Console.WriteLine($"[RepoManager] Reusing existing worktree at '{existingMatch.Path}' (branch: {branchName})"); + lock (_stateLock) { repo.LastUsedAt = DateTime.UtcNow; } + Save(); + return existingMatch; + } + else if (existingMatch != null) + { + // Worktree record exists but the directory is corrupt/missing — remove stale entry + Console.WriteLine($"[RepoManager] Removing stale worktree entry '{existingMatch.Path}' (invalid git state)"); + lock (_stateLock) { _state.Worktrees.Remove(existingMatch); } + Save(); + } + await EnsureRepoCloneInCurrentRootAsync(repo, null, ct); // Fetch latest from origin (prune to clean up deleted remote branches). @@ -715,29 +1033,10 @@ public virtual async Task CreateWorktreeAsync(string repoId, strin string worktreePath; var worktreeId = Guid.NewGuid().ToString()[..8]; - if (!string.IsNullOrWhiteSpace(localPath)) - { - // Nested strategy: place worktree inside the user's repo at .polypilot/worktrees/{branch}/ - var repoWorktreesDir = Path.Combine(Path.GetFullPath(localPath), ".polypilot", "worktrees"); - Directory.CreateDirectory(repoWorktreesDir); - EnsureGitExcludeEntry(localPath, ".polypilot/"); - worktreePath = Path.Combine(repoWorktreesDir, branchName); - - // Guard against path traversal: branch names with ".." or leading "/" could escape - // the directory. Equality with repoWorktreesDir itself is also invalid — an empty - // branch name or a name that normalises to "." would trigger that case. - var resolved = Path.GetFullPath(worktreePath); - if (!resolved.StartsWith(Path.GetFullPath(repoWorktreesDir) + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) - throw new InvalidOperationException( - $"Branch name '{branchName}' would create worktree outside the managed directory. " + - "Use a branch name without '..' or leading path separators."); - } - else - { - // Centralized strategy: place worktree in ~/.polypilot/worktrees/{repoId}-{guid8}/ - Directory.CreateDirectory(WorktreesDir); - worktreePath = Path.Combine(WorktreesDir, $"{repoId}-{worktreeId}"); - } + // Centralized: place worktree in ~/.polypilot/worktrees/{abbreviated}-{guid8}/ + // Uses WorktreeDirName to shorten the path and avoid Windows MAX_PATH issues. + Directory.CreateDirectory(WorktreesDir); + worktreePath = Path.Combine(WorktreesDir, WorktreeDirName(repoId, worktreeId)); try { @@ -751,6 +1050,9 @@ public virtual async Task CreateWorktreeAsync(string repoId, strin throw; } + // Enable long paths in the new worktree so subsequent git ops don't fail + await EnsureLongPathsAsync(worktreePath, ct); + var wt = new WorktreeInfo { Id = worktreeId, @@ -903,10 +1205,13 @@ public async Task CreateWorktreeFromPrAsync(string repoId, int prN Directory.CreateDirectory(WorktreesDir); var worktreeId = Guid.NewGuid().ToString()[..8]; - var worktreePath = Path.Combine(WorktreesDir, $"{repoId}-{worktreeId}"); + var worktreePath = Path.Combine(WorktreesDir, WorktreeDirName(repoId, worktreeId)); await RunGitAsync(repo.BareClonePath, ct, "worktree", "add", worktreePath, "--", branchName); + // Enable long paths in the new worktree so subsequent git ops don't fail + await EnsureLongPathsAsync(worktreePath, ct); + // Set upstream tracking so push/pull work in the worktree if (headBranch != null) { @@ -1083,7 +1388,15 @@ public async Task RemoveRepositoryAsync(string repoId, bool deleteFromDisk, Canc if (deleteFromDisk && Directory.Exists(repo.BareClonePath)) { - try { Directory.Delete(repo.BareClonePath, recursive: true); } catch { } + // Only delete if BareClonePath is under the managed ReposDir. + // Repos added via "Existing Folder" have BareClonePath pointing at the user's + // real project directory — we must NEVER delete that. + var fullClonePath = Path.GetFullPath(repo.BareClonePath); + var managedPrefix = Path.GetFullPath(ReposDir) + Path.DirectorySeparatorChar; + if (fullClonePath.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase)) + { + try { Directory.Delete(repo.BareClonePath, recursive: true); } catch { } + } } OnStateChanged?.Invoke(); @@ -1146,7 +1459,20 @@ private async Task GetDefaultBranch(string barePath, CancellationToken c { try { - // Get the default branch name (e.g. "main") + // Prefer origin/HEAD which points at the canonical default branch regardless + // of which branch is currently checked out (important for non-bare repos). + try + { + var originHead = (await RunGitAsync(barePath, ct, "rev-parse", "--abbrev-ref", "origin/HEAD")).Trim(); + if (!string.IsNullOrWhiteSpace(originHead) && originHead != "origin/HEAD") + { + Console.WriteLine($"[RepoManager] Using origin/HEAD: {originHead}"); + return $"refs/remotes/{originHead}"; + } + } + catch { /* origin/HEAD not set — fall through */ } + + // Fallback: use symbolic-ref HEAD (correct for bare repos, may be wrong for non-bare) var headRef = await RunGitAsync(barePath, ct, "symbolic-ref", "HEAD"); var branchName = headRef.Trim().Replace("refs/heads/", ""); @@ -1200,7 +1526,9 @@ private static async Task RunGhAsync(string? workDir, CancellationToken if (workDir != null) { psi.WorkingDirectory = workDir; - // Bare repos need GIT_DIR set explicitly for gh to find the remote + // Bare repos (paths ending in .git) need GIT_DIR set explicitly for gh + // to find the remote. Non-bare repos (including those added via "Existing Folder") + // don't need this — gh discovers the remote from the working directory. if (workDir.EndsWith(".git", StringComparison.OrdinalIgnoreCase)) psi.Environment["GIT_DIR"] = workDir; }