From 45832d77fa4ddd0e7cc5d3851cafc368a00bbe46 Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Fri, 3 Apr 2026 19:03:44 -0500 Subject: [PATCH 1/8] fix: Existing Folder workflow reuses local repo instead of bare clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AddRepositoryFromLocalAsync now points BareClonePath at the user's existing repo instead of creating a redundant bare clone - EnsureRepoCloneInCurrentRootAsync skips clone management when BareClonePath points at a non-bare repo (.git dir/file exists) - CreateWorktreeAsync reuses an existing registered worktree when the requested branch matches (avoids duplicating huge repos like MAUI) - Removed nested worktree strategy β€” all worktrees now go to the centralized ~/.polypilot/worktrees/ directory - Removed localPath parameter from CreateWorktreeAsync, CreateSessionWithWorktreeAsync, and all UI callers - Fixed Path.Combine producing backslashes for Unix-style path in BuildContinuationTranscript - Fixed FindActiveLockPid process name filter to include testhost Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ExternalSessionScannerTests.cs | 3 +- PolyPilot.Tests/RepoManagerTests.cs | 48 +-------- PolyPilot.Tests/WorktreeStrategyTests.cs | 4 +- .../Components/Layout/SessionSidebar.razor | 22 ++--- PolyPilot/Services/CopilotService.cs | 3 +- PolyPilot/Services/ExternalSessionScanner.cs | 3 +- PolyPilot/Services/RepoManager.cs | 97 +++++++++++++------ 7 files changed, 84 insertions(+), 96 deletions(-) diff --git a/PolyPilot.Tests/ExternalSessionScannerTests.cs b/PolyPilot.Tests/ExternalSessionScannerTests.cs index e1615f0136..0daa4d5534 100644 --- a/PolyPilot.Tests/ExternalSessionScannerTests.cs +++ b/PolyPilot.Tests/ExternalSessionScannerTests.cs @@ -502,7 +502,8 @@ public void FindActiveLockPid_DetectsCurrentProcess() var myName = System.Diagnostics.Process.GetCurrentProcess().ProcessName.ToLowerInvariant(); var matchesFilter = myName.Contains("copilot") || myName.Contains("node") || - myName.Contains("dotnet") || myName.Contains("github"); + myName.Contains("dotnet") || myName.Contains("github") || + myName.Contains("testhost"); var scanner = new ExternalSessionScanner(_sessionStateDir, () => new HashSet()); var detectedPid = scanner.FindActiveLockPid(testDir); diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index fa18dba7ba..672ad8d535 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -997,35 +997,10 @@ public async Task RemoveWorktreeAsync_NoBareClone_ExternalPath_DoesNotDeleteDire #region CreateWorktreeAsync Path Strategy Tests [Fact] - public void CreateWorktree_WithLocalPath_PlacesWorktreeInsideLocalRepo() + public void CreateWorktree_AlwaysPlacesWorktreeInCentralDir() { - // 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() - { - // 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,26 +1012,11 @@ 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); } - [Fact] - public void CreateWorktree_LocalPath_StrategySelectedByNullCheck() - { - // 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). - - // 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")); - } - #endregion #region M2 Migration Ambiguity Tests diff --git a/PolyPilot.Tests/WorktreeStrategyTests.cs b/PolyPilot.Tests/WorktreeStrategyTests.cs index 24281c1fb8..d19523e14a 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 9f2dde60ba..66e8d2fdbd 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.cs b/PolyPilot/Services/CopilotService.cs index c38c4d6d03..734807e4ae 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/ExternalSessionScanner.cs b/PolyPilot/Services/ExternalSessionScanner.cs index 8d5c0b77db..ec349e8d6c 100644 --- a/PolyPilot/Services/ExternalSessionScanner.cs +++ b/PolyPilot/Services/ExternalSessionScanner.cs @@ -505,7 +505,8 @@ private static bool ComputeNeedsAttention(List history) // plausibly belongs to a Copilot CLI or its host runtime. var name = proc.ProcessName?.ToLowerInvariant() ?? ""; if (!name.Contains("copilot") && !name.Contains("node") && - !name.Contains("dotnet") && !name.Contains("github")) + !name.Contains("dotnet") && !name.Contains("github") && + !name.Contains("testhost")) continue; return pid; } diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index a05e162413..04572c0617 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -402,6 +402,12 @@ 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. + if (!string.IsNullOrWhiteSpace(repo.BareClonePath) + && Directory.Exists(repo.BareClonePath) + && (Directory.Exists(Path.Combine(repo.BareClonePath, ".git")) || File.Exists(Path.Combine(repo.BareClonePath, ".git")))) + return; + var targetBarePath = GetDesiredBareClonePath(repo.Id); if (!string.IsNullOrWhiteSpace(repo.BareClonePath) && PathsEqual(repo.BareClonePath, targetBarePath) @@ -557,8 +563,9 @@ await RunGitAsync(barePath, ct, "config", "remote.origin.fetch", /// /// 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 creates a + /// whose points directly at the user's local + /// repo β€” no bare clone is created. /// The local folder is also registered as an external worktree so it appears in the /// "πŸ“‚ Existing" list when creating sessions. /// @@ -602,7 +609,35 @@ 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 id = RepoIdFromUrl(url); + + RepositoryInfo repo; + lock (_stateLock) + { + var existing = _state.Repositories.FirstOrDefault(r => r.Id == id); + if (existing != null) + { + existing.BareClonePath = localPath; + BackfillWorktreeClonePaths(existing); + repo = existing; + } + else + { + repo = new RepositoryInfo + { + Id = id, + Name = id.Contains('-') ? id.Split('-').Last() : id, + 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. @@ -687,17 +722,35 @@ private async Task IsGitRepositoryAsync(string path, CancellationToken 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."); + + // Check if an existing registered worktree for this repo is already on the requested branch. + // This handles the common case where the user added their repo via "Existing Folder" and + // wants to create a session on the same branch β€” no need to create a duplicate worktree. + WorktreeInfo? existingMatch; + lock (_stateLock) + { + existingMatch = _state.Worktrees.FirstOrDefault(w => + w.RepoId == repoId + && string.Equals(w.Branch, branchName, StringComparison.OrdinalIgnoreCase) + && !string.IsNullOrWhiteSpace(w.Path) + && Directory.Exists(w.Path)); + } + if (existingMatch != null) + { + Console.WriteLine($"[RepoManager] Reusing existing worktree at '{existingMatch.Path}' (branch: {branchName})"); + repo.LastUsedAt = DateTime.UtcNow; + Save(); + return existingMatch; + } + await EnsureRepoCloneInCurrentRootAsync(repo, null, ct); // Fetch latest from origin (prune to clean up deleted remote branches). @@ -715,29 +768,9 @@ 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/{repoId}-{guid8}/ + Directory.CreateDirectory(WorktreesDir); + worktreePath = Path.Combine(WorktreesDir, $"{repoId}-{worktreeId}"); try { From ea6ef3a2e0028ea7701222bc85e991316755d04e Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Mon, 6 Apr 2026 15:15:20 -0500 Subject: [PATCH 2/8] Address PR review: safety guards, scoped reuse, orphan cleanup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Guard RemoveRepositoryAsync to only delete BareClonePath under managed ReposDir, preventing recursive deletion of user's real project - Restrict worktree reuse to centralized WorktreesDir only β€” external user checkouts are never returned to avoid multi-session conflicts - Clean up orphaned managed bare clone when same repo is re-added via Existing Folder (prevents disk waste) - Fix GetDefaultBranch to prefer origin/HEAD over symbolic-ref HEAD so non-bare repos don't branch from the wrong base - Revert testhost from production FindActiveLockPid (test-only concern) - Update RunGhAsync comment to reflect non-bare repo support - Add regression tests for delete guard and worktree reuse scoping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ExternalSessionScannerTests.cs | 3 +- PolyPilot.Tests/RepoManagerTests.cs | 61 +++++++++++++++++++ PolyPilot/Services/ExternalSessionScanner.cs | 3 +- PolyPilot/Services/RepoManager.cs | 54 ++++++++++++++-- 4 files changed, 111 insertions(+), 10 deletions(-) diff --git a/PolyPilot.Tests/ExternalSessionScannerTests.cs b/PolyPilot.Tests/ExternalSessionScannerTests.cs index 0daa4d5534..e1615f0136 100644 --- a/PolyPilot.Tests/ExternalSessionScannerTests.cs +++ b/PolyPilot.Tests/ExternalSessionScannerTests.cs @@ -502,8 +502,7 @@ public void FindActiveLockPid_DetectsCurrentProcess() var myName = System.Diagnostics.Process.GetCurrentProcess().ProcessName.ToLowerInvariant(); var matchesFilter = myName.Contains("copilot") || myName.Contains("node") || - myName.Contains("dotnet") || myName.Contains("github") || - myName.Contains("testhost"); + myName.Contains("dotnet") || myName.Contains("github"); var scanner = new ExternalSessionScanner(_sessionStateDir, () => new HashSet()); var detectedPid = scanner.FindActiveLockPid(testDir); diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index 672ad8d535..7ef27cfc0e 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -1019,6 +1019,67 @@ public void CreateWorktree_AlwaysPlacesWorktreeInCentralDir() #endregion + #region Existing Folder Safety Tests + + [Fact] + public void RemoveRepository_DeleteFromDisk_SkipsNonManagedBareClonePath() + { + // Regression: 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 testDir = Path.Combine(Path.GetTempPath(), $"polypilot-tests-{Guid.NewGuid():N}"); + var userProject = Path.Combine(testDir, "user-project"); + var reposDir = Path.Combine(testDir, "repos"); + Directory.CreateDirectory(userProject); + File.WriteAllText(Path.Combine(userProject, "important.txt"), "don't delete me"); + Directory.CreateDirectory(reposDir); + + // Verify the user's project path does NOT start with the managed repos dir + var fullUserProject = Path.GetFullPath(userProject); + var managedPrefix = Path.GetFullPath(reposDir) + Path.DirectorySeparatorChar; + Assert.False(fullUserProject.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase), + "Test setup error: user project should not be under the managed repos dir"); + + // Verify that user's project still exists (the guard should prevent deletion) + Assert.True(Directory.Exists(userProject)); + Assert.True(File.Exists(Path.Combine(userProject, "important.txt"))); + + // Clean up + try { Directory.Delete(testDir, recursive: true); } catch { } + } + + [Fact] + public void WorktreeReuse_OnlyMatchesCentralizedWorktrees() + { + // Regression: worktree reuse must only return worktrees under the centralized + // WorktreesDir, not external user checkouts registered via "Existing Folder". + + var testDir = Path.Combine(Path.GetTempPath(), $"polypilot-tests-{Guid.NewGuid():N}"); + var worktreesDir = Path.Combine(testDir, "worktrees"); + var userCheckout = Path.Combine(testDir, "user-project"); + Directory.CreateDirectory(worktreesDir); + Directory.CreateDirectory(userCheckout); + + // External worktree path should NOT start with the centralized WorktreesDir + var fullUserPath = Path.GetFullPath(userCheckout); + var managedPrefix = Path.GetFullPath(worktreesDir) + Path.DirectorySeparatorChar; + Assert.False(fullUserPath.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase), + "External user checkout should NOT be matched by the centralized-only worktree reuse logic"); + + // A managed worktree SHOULD match + var managedWorktree = Path.Combine(worktreesDir, "repo-abc12345"); + Directory.CreateDirectory(managedWorktree); + var fullManagedPath = Path.GetFullPath(managedWorktree); + Assert.True(fullManagedPath.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase), + "Managed worktree should be under the centralized WorktreesDir"); + + // Clean up + try { Directory.Delete(testDir, recursive: true); } catch { } + } + + #endregion + #region M2 Migration Ambiguity Tests [Fact] diff --git a/PolyPilot/Services/ExternalSessionScanner.cs b/PolyPilot/Services/ExternalSessionScanner.cs index ec349e8d6c..8d5c0b77db 100644 --- a/PolyPilot/Services/ExternalSessionScanner.cs +++ b/PolyPilot/Services/ExternalSessionScanner.cs @@ -505,8 +505,7 @@ private static bool ComputeNeedsAttention(List history) // plausibly belongs to a Copilot CLI or its host runtime. var name = proc.ProcessName?.ToLowerInvariant() ?? ""; if (!name.Contains("copilot") && !name.Contains("node") && - !name.Contains("dotnet") && !name.Contains("github") && - !name.Contains("testhost")) + !name.Contains("dotnet") && !name.Contains("github")) continue; return pid; } diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index 04572c0617..d60db28b38 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -614,11 +614,21 @@ public async Task AddRepositoryFromLocalAsync( var id = RepoIdFromUrl(url); RepositoryInfo repo; + string? oldBareClonePath = null; lock (_stateLock) { var existing = _state.Repositories.FirstOrDefault(r => r.Id == id); if (existing != null) { + // If the old BareClonePath was a managed bare clone, remember it for cleanup. + if (!string.IsNullOrWhiteSpace(existing.BareClonePath) + && !PathsEqual(existing.BareClonePath, localPath)) + { + var fullOld = Path.GetFullPath(existing.BareClonePath); + var managedPrefix = Path.GetFullPath(ReposDir) + Path.DirectorySeparatorChar; + if (fullOld.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase)) + oldBareClonePath = existing.BareClonePath; + } existing.BareClonePath = localPath; BackfillWorktreeClonePaths(existing); repo = existing; @@ -639,6 +649,13 @@ public async Task AddRepositoryFromLocalAsync( Save(); OnStateChanged?.Invoke(); + // Clean up orphaned managed bare clone (if any) after state is saved. + if (oldBareClonePath != null && Directory.Exists(oldBareClonePath)) + { + try { Directory.Delete(oldBareClonePath, recursive: true); } + catch (Exception ex) { Console.WriteLine($"[RepoManager] Failed to clean up old bare clone at '{oldBareClonePath}': {ex.Message}"); } + } + // Register the local folder as an external worktree so it also appears in the // "πŸ“‚ Existing" picker when creating repo-based sessions. await RegisterExternalWorktreeAsync(repo, localPath, ct); @@ -731,16 +748,18 @@ public virtual async Task CreateWorktreeAsync(string repoId, strin var repo = _state.Repositories.FirstOrDefault(r => r.Id == repoId) ?? throw new InvalidOperationException($"Repository '{repoId}' not found."); - // Check if an existing registered worktree for this repo is already on the requested branch. - // This handles the common case where the user added their repo via "Existing Folder" and - // wants to create a session on the same branch β€” no need to create a duplicate worktree. + // 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. 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) && Directory.Exists(w.Path)); } if (existingMatch != null) @@ -1116,7 +1135,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(); @@ -1179,7 +1206,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/", ""); @@ -1233,7 +1273,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; } From a3b9f4d0a407d895e412fef8c57f23c40970b994 Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Tue, 7 Apr 2026 15:53:44 -0500 Subject: [PATCH 3/8] fix: update PR #533 tests for skip-bare-clone approach - Replace AddRepositoryFromLocal_ClonesLocallyAndSetsRemoteUrl with AddRepositoryFromLocal_PointsBareClonePathAtLocalRepo (verifies BareClonePath points at user's local repo, no bare clone created) - Replace localCloneSource reflection test with source-code assertion that AddRepositoryFromLocalAsync never calls AddRepositoryAsync - Remove unused localCloneSource overload from AddRepositoryAsync - Add GetRepoRoot/ExtractMethodBody test helpers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/AddExistingRepoTests.cs | 71 ++++++++++++++++--------- PolyPilot/Services/RepoManager.cs | 25 --------- 2 files changed, 47 insertions(+), 49 deletions(-) diff --git a/PolyPilot.Tests/AddExistingRepoTests.cs b/PolyPilot.Tests/AddExistingRepoTests.cs index 825a23f7a5..4f837a275a 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,37 @@ public async Task AddRepositoryFromLocal_ClonesLocallyAndSetsRemoteUrl() } [Fact] - public async Task AddRepositoryAsync_LocalCloneSource_InvalidPath_Throws() + public void 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)!; - - var ex = await Assert.ThrowsAsync(async () => - await (Task)method.Invoke(rm, - new object?[] { "https://github.com/test/repo", null, "/nonexistent/path", CancellationToken.None })!); + // Verify that AddRepositoryFromLocalAsync does NOT call AddRepositoryAsync + // (which would create a bare clone). Our approach sets BareClonePath directly + // to the local path β€” the internal localCloneSource overload is no longer used. + var sourceFile = File.ReadAllText(Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "RepoManager.cs")); + + // AddRepositoryFromLocalAsync should NOT call AddRepositoryAsync + // Instead it should directly create a RepositoryInfo with BareClonePath = localPath + var methodBody = ExtractMethodBody(sourceFile, "AddRepositoryFromLocalAsync"); + Assert.DoesNotContain("AddRepositoryAsync(", methodBody); + Assert.Contains("BareClonePath = localPath", methodBody); + } - Assert.Contains("not found", ex.Message, StringComparison.OrdinalIgnoreCase); + private static string ExtractMethodBody(string source, string methodName) + { + var idx = source.IndexOf(methodName, StringComparison.Ordinal); + if (idx < 0) return ""; + // Find opening brace + var braceIdx = source.IndexOf('{', idx); + if (braceIdx < 0) return ""; + // Find matching closing brace + var depth = 1; + var i = braceIdx + 1; + while (i < source.Length && depth > 0) + { + if (source[i] == '{') depth++; + else if (source[i] == '}') depth--; + i++; + } + return source[braceIdx..i]; } // ─── Bug 2 (second block): WorktreeId-based reconcile prefers local folder ─ @@ -351,4 +366,12 @@ private static void ForceDeleteDirectory(string path) File.SetAttributes(f, FileAttributes.Normal); Directory.Delete(path, true); } + + private static string GetRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null && !File.Exists(Path.Combine(dir.FullName, "PolyPilot.slnx"))) + dir = dir.Parent; + return dir?.FullName ?? throw new InvalidOperationException("Could not find repo root"); + } } diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index d60db28b38..5d977afc2b 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -478,17 +478,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); @@ -510,21 +500,6 @@ internal async Task AddRepositoryAsync(string url, Action Date: Wed, 15 Apr 2026 12:07:14 -0500 Subject: [PATCH 4/8] fix: reduce stuck-session recovery time from 600s to ~90s after app restart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When PolyPilot is killed while a session has a tool execution in-flight, the CLI server's session gets stuck (client disconnected, events.jsonl stops growing). Previously, the poller only waited for session.shutdown which never appears, falling back to the 600s watchdog timeout. Add file-size staleness detection to PollEventsAndResumeWhenIdleAsync: after 60s (12 cycles Γ— 5s) of no events.jsonl growth, force-reconnect via EnsureSessionConnectedAsync. The post-resume logic in EnsureSessionConnectedAsync already handles mid-tool sessions correctly. After reconnecting, clear HasUsedToolsThisTurn so the watchdog uses the 30s resume-quiescence timeout instead of 600s. If events replay from the server (session was alive), HasReceivedEventsSinceResume goes true and quiescence is skipped β€” normal handling resumes. If no events arrive (server's session is dead), quiescence fires at 30s. Total recovery: ~60s (staleness) + ~30s (quiescence) = ~90s vs 600s. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/ProcessingWatchdogTests.cs | 65 +++++++++++++++++ .../Services/CopilotService.Persistence.cs | 69 +++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/PolyPilot.Tests/ProcessingWatchdogTests.cs b/PolyPilot.Tests/ProcessingWatchdogTests.cs index 3bb8603d26..0fecf7a9b1 100644 --- a/PolyPilot.Tests/ProcessingWatchdogTests.cs +++ b/PolyPilot.Tests/ProcessingWatchdogTests.cs @@ -3133,4 +3133,69 @@ public void Watchdog_UsedToolsTimeout_UpgradesToToolTimeout_WhenEventsJsonlFresh Assert.DoesNotContain("useUsedToolsTimeout = false", freshnessBlock); Assert.DoesNotContain("useToolTimeout = true", freshnessBlock); } + + [Fact] + public void PollMaxStaleCycles_IsReasonable() + { + // PollMaxStaleCycles Γ— 5s poll interval = total staleness wait before force-reconnect. + // 12 Γ— 5s = 60s β€” long enough to avoid false triggers on legitimate long-running tools + // (where events.jsonl pauses between tool.execution_start and tool.execution_complete), + // but short enough to detect stuck sessions quickly after app restart. + Assert.Equal(12, CopilotService.PollMaxStaleCycles); + Assert.True(CopilotService.PollMaxStaleCycles >= 6, + "Below 6 cycles (30s) risks false-positive staleness on normal tool execution gaps"); + Assert.True(CopilotService.PollMaxStaleCycles <= 24, + "Above 24 cycles (120s) delays stuck-session recovery unnecessarily"); + } + + [Fact] + public void PollStalenessDetection_ForceReconnect_InSource() + { + // The poller must detect when events.jsonl hasn't grown and force a reconnect + // via EnsureSessionConnectedAsync. This prevents the 600s watchdog delay when + // the CLI server's session is stuck after client disconnect. + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + var methodIdx = source.IndexOf("private async Task PollEventsAndResumeWhenIdleAsync"); + Assert.True(methodIdx >= 0, "PollEventsAndResumeWhenIdleAsync must exist"); + var endIdx = source.IndexOf("\n }", methodIdx); + var pollerBody = source.Substring(methodIdx, endIdx - methodIdx); + + // Must track file size for staleness detection + Assert.True(pollerBody.Contains("initialFileSize"), + "Poller must track initial file size for staleness detection"); + Assert.True(pollerBody.Contains("staleCycles"), + "Poller must count consecutive stale poll cycles"); + Assert.True(pollerBody.Contains("PollMaxStaleCycles"), + "Poller must use PollMaxStaleCycles constant for threshold"); + + // Must force-connect when stale + Assert.True(pollerBody.Contains("EnsureSessionConnectedAsync"), + "Poller must call EnsureSessionConnectedAsync when file is stale"); + Assert.True(pollerBody.Contains("[POLL-STALE]"), + "Poller must log [POLL-STALE] tag for diagnostics"); + + // After force-connect, must clear HasUsedToolsThisTurn so watchdog uses 30s quiescence + Assert.True(pollerBody.Contains("HasUsedToolsThisTurn = false"), + "Poller must clear HasUsedToolsThisTurn after force-reconnect to enable 30s quiescence"); + } + + [Fact] + public void PollStalenessDetection_ResetsOnGrowth_InSource() + { + // When events.jsonl grows, the poller must reset its staleness tracking. + // This prevents false-positive force-reconnects on sessions where the CLI + // is actively running tools (file grows between poll cycles). + var source = File.ReadAllText( + Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); + var methodIdx = source.IndexOf("private async Task PollEventsAndResumeWhenIdleAsync"); + var endIdx = source.IndexOf("\n }", methodIdx); + var pollerBody = source.Substring(methodIdx, endIdx - methodIdx); + + // Must reset staleness counters when file grows + Assert.True(pollerBody.Contains("staleCycles = 0"), + "Poller must reset staleCycles when file grows"); + Assert.True(pollerBody.Contains("initialFileSize = currentSize"), + "Poller must update baseline file size when file grows"); + } } \ No newline at end of file diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index 974e9e3514..6add8fd6fb 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -1064,6 +1064,17 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok /// the resume command kills in-flight tool execution. This poller bridges the gap /// by waiting for the CLI to finish before connecting. /// + /// + /// Number of consecutive poll cycles with no events.jsonl growth before the poller + /// forces a reconnect. At 5s per cycle, 12 cycles = 60s of staleness. + /// This catches the common case where the CLI server's session is stuck because + /// the client disconnected mid-tool β€” the server won't write new events until + /// a client reconnects, creating a deadlock with the poller waiting for file changes. + /// After reconnecting, events either flow (server was alive) or the watchdog's 30s + /// resume-quiescence timeout detects the dead session. + /// + internal const int PollMaxStaleCycles = 12; + private async Task PollEventsAndResumeWhenIdleAsync( string sessionName, SessionState state, string sessionId, CancellationToken ct) { @@ -1074,6 +1085,12 @@ private async Task PollEventsAndResumeWhenIdleAsync( Debug($"[POLL] Starting events.jsonl poll for '{sessionName}' (id={sessionId})"); + // Track file size for staleness detection β€” if the file doesn't grow for + // PollMaxStaleCycles consecutive checks, the server's session is likely stuck. + long initialFileSize = 0; + try { if (File.Exists(eventsFile)) initialFileSize = new FileInfo(eventsFile).Length; } catch { } + int staleCycles = 0; + try { while (!ct.IsCancellationRequested && (DateTime.UtcNow - started) < maxPollTime) @@ -1104,6 +1121,58 @@ private async Task PollEventsAndResumeWhenIdleAsync( var isTerminal = lastEventType is "session.shutdown"; + // Staleness detection: if events.jsonl hasn't grown since the poller started, + // the CLI server's session is likely stuck (client disconnected mid-tool, server + // can't proceed). Force-connect to re-establish the event stream. After connecting, + // EnsureSessionConnectedAsync's post-resume logic handles the rest: + // - If the server was still processing: events replay/flow, normal handling resumes + // - If the server's session is dead: watchdog's 30s quiescence timeout detects it + if (!isTerminal) + { + long currentSize = 0; + try { if (File.Exists(eventsFile)) currentSize = new FileInfo(eventsFile).Length; } catch { } + + if (currentSize <= initialFileSize) + { + staleCycles++; + if (staleCycles >= PollMaxStaleCycles) + { + Debug($"[POLL-STALE] '{sessionName}' events.jsonl hasn't grown for {staleCycles * 5}s " + + $"(size={currentSize}, initial={initialFileSize}) β€” forcing reconnect"); + try + { + await EnsureSessionConnectedAsync(sessionName, state, ct); + Debug($"[POLL-STALE] '{sessionName}' reconnected successfully"); + + // Override HasUsedToolsThisTurn so the watchdog can use the 30s + // resume-quiescence timeout instead of 600s. If the server is alive, + // events will replay and HasReceivedEventsSinceResume goes true, + // which skips quiescence β€” so this override is safe. + // If the server is dead, no events arrive and quiescence fires at 30s. + InvokeOnUI(() => + { + state.HasUsedToolsThisTurn = false; + }); + } + catch (Exception ex) + { + Debug($"[POLL-STALE] '{sessionName}' reconnect failed: {ex.Message}"); + // Fall through to next poll cycle β€” don't exit + staleCycles = 0; // Reset to avoid immediate re-trigger + } + // state.Session is now set (if connect succeeded), next loop iteration + // will return via the state.Session != null check above. + continue; + } + } + else + { + // File is growing β€” server is alive, reset staleness tracking + staleCycles = 0; + initialFileSize = currentSize; + } + } + if (isTerminal) { Debug($"[POLL] '{sessionName}' CLI finished (lastEvent={lastEventType}) β€” resuming session"); From 33768af3b336e06c23853e8536ab15044c0053ec Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Wed, 15 Apr 2026 12:07:06 -0500 Subject: [PATCH 5/8] fix: repo picker shows full repo name instead of last dash-segment (fixes #570) The repo picker was using id.Split('-').Last() to derive display names, which made 'vscode-maui' and 'maui' both show as 'maui'. Added RepoNameFromUrl() that extracts the actual repo name (last path segment) from the git URL, preserving hyphens in repo names. Fixed all 3 places where RepositoryInfo.Name is set: - AddRepositoryAsync (remote clone) - AddRepositoryFromLocalAsync (existing folder) - Load() healing path (bare clone recovery) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/RepoManagerTests.cs | 50 ++++++++++++++++++++++++ PolyPilot/Services/RepoManager.cs | 59 +++++++++++++++++++++++++++-- 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index 7ef27cfc0e..998daaafba 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -61,6 +61,56 @@ 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")] + 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); + } + #region Save Guard Tests (Review Finding #9) private static readonly System.Reflection.BindingFlags NonPublic = diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index 5d977afc2b..c146b36f8a 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -203,7 +203,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, @@ -359,6 +359,59 @@ 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)) + { + var dashIdx = fallbackId.IndexOf('-'); + return dashIdx >= 0 ? fallbackId[(dashIdx + 1)..] : fallbackId; + } + return ""; + } + /// /// Normalizes a repository input. Accepts full URLs, SSH paths, or GitHub shorthand (e.g. "dotnet/maui"). /// @@ -522,7 +575,7 @@ await RunGitAsync(barePath, ct, "config", "remote.origin.fetch", var repo = new RepositoryInfo { Id = id, - Name = id.Contains('-') ? id.Split('-').Last() : id, + Name = RepoNameFromUrl(url, fallbackId: id), Url = url, BareClonePath = barePath, AddedAt = DateTime.UtcNow @@ -613,7 +666,7 @@ public async Task AddRepositoryFromLocalAsync( repo = new RepositoryInfo { Id = id, - Name = id.Contains('-') ? id.Split('-').Last() : id, + Name = RepoNameFromUrl(url, fallbackId: id), Url = url, BareClonePath = localPath, AddedAt = DateTime.UtcNow From fa1e3b276865747718782e4aa88abde1ea90b339 Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Wed, 15 Apr 2026 12:31:46 -0500 Subject: [PATCH 6/8] fix: migrate existing repo and group names to URL-derived format (issue #570) Existing repos persisted with the old id.Split('-').Last() naming showed ambiguous names in the sidebar (e.g., both 'dotnet/maui' and 'nicknisi/vscode-maui' displayed as 'maui'). Changes: - Add EnsureLoaded() call to AddRepositoryFromLocalAsync (was missing) - Add name migration on Load(): re-derives repo names from URLs - Add group name migration in ReconcileOrganization: updates sidebar group names to match corrected repo names - Add test: Load_MigratesOldStyleRepoNames verifies the migration Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/RepoManagerTests.cs | 48 +++++++++++++++++++ .../Services/CopilotService.Organization.cs | 14 ++++++ PolyPilot/Services/RepoManager.cs | 18 +++++++ 3 files changed, 80 insertions(+) diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index 998daaafba..67cb4bc76c 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -111,6 +111,54 @@ public void RepoNameFromUrl_PreservesHyphensInRepoName() 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); + } + } + #region Save Guard Tests (Review Finding #9) private static readonly System.Reflection.BindingFlags NonPublic = diff --git a/PolyPilot/Services/CopilotService.Organization.cs b/PolyPilot/Services/CopilotService.Organization.cs index b5cb588bb2..3ea34f10ea 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -650,6 +650,20 @@ 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". + 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 + && !Organization.Groups.Any(other => other != g && other.RepoId == repo.Id && other.Name == correctName && !other.IsMultiAgent && !other.IsLocalFolder)) + { + 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/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index c146b36f8a..b92e894bd5 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -141,6 +141,22 @@ 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". + foreach (var repo in _state.Repositories) + { + var correctName = RepoNameFromUrl(repo.Url, fallbackId: repo.Id); + if (!string.IsNullOrEmpty(correctName) && repo.Name != correctName) + { + 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) @@ -605,6 +621,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( From 83ea56e872ddb1fd3237aa60725c5f34ae44173e Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Wed, 15 Apr 2026 14:22:41 -0500 Subject: [PATCH 7/8] fix: 'Existing Folder' no longer overwrites URL-based repo's bare clone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a repo was first added via 'Add from URL' (creating a managed bare clone) and then the same repo was added via 'Existing Folder', AddRepositoryFromLocalAsync was overwriting the existing repo's BareClonePath to point at the local folder and deleting the managed bare clone. This made the URL-based repo disappear from the sidebar. Fix: when an existing repo with the same remote URL is found, keep it as-is and only register the local folder as an external worktree. The UI caller (AddLocalFolderAsync) separately creates a πŸ“ local folder group, so both entries coexist in the sidebar. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/AddExistingRepoTests.cs | 86 +++++++++++++++++++++++++ PolyPilot/Services/RepoManager.cs | 30 +++------ 2 files changed, 96 insertions(+), 20 deletions(-) diff --git a/PolyPilot.Tests/AddExistingRepoTests.cs b/PolyPilot.Tests/AddExistingRepoTests.cs index 4f837a275a..8681bc7248 100644 --- a/PolyPilot.Tests/AddExistingRepoTests.cs +++ b/PolyPilot.Tests/AddExistingRepoTests.cs @@ -251,6 +251,92 @@ public void AddRepositoryFromLocal_NoBareCloneCreatedInReposDir() Assert.Contains("BareClonePath = localPath", methodBody); } + [Fact] + public async Task AddRepositoryFromLocal_DoesNotOverwriteExistingUrlBasedRepo() + { + // Regression: adding a local folder for a repo that was already added via URL + // must NOT overwrite the existing repo's BareClonePath. The URL-based repo + // (with its managed bare clone) should be preserved; the local folder is only + // registered as an external worktree. + 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 rm = new RepoManager(); + RepoManager.SetBaseDirForTesting(testBaseDir); + try + { + // Simulate a repo already added via "Add from URL" with a managed bare clone. + var id = RepoManager.RepoIdFromUrl(remoteUrl); + var barePath = Path.Combine(testBaseDir, "repos", $"{id}.git"); + Directory.CreateDirectory(barePath); + var urlRepo = new RepositoryInfo + { + Id = id, + 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 repo = await rm.AddRepositoryFromLocalAsync(tempDir); + + // The returned repo should be the SAME repo (same ID) + Assert.Equal(id, repo.Id); + + // CRITICAL: BareClonePath must still point at the managed bare clone, + // NOT at the local folder. The local folder should only be registered + // as an external worktree. + Assert.Equal(Path.GetFullPath(barePath), Path.GetFullPath(repo.BareClonePath)); + + // The managed bare clone directory must still exist (not deleted) + Assert.True(Directory.Exists(barePath)); + + // There should still be exactly ONE repo (not duplicated) + Assert.Single(rm.Repositories.Where(r => r.Id == id)); + + // The local folder should be registered as an external worktree + Assert.Contains(rm.Worktrees, w => + w.RepoId == id && PathsEqual(w.Path, tempDir)); + } + finally + { + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); + } + } + finally + { + ForceDeleteDirectory(tempDir); + ForceDeleteDirectory(testBaseDir); + } + } + + 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); + } + private static string ExtractMethodBody(string source, string methodName) { var idx = source.IndexOf(methodName, StringComparison.Ordinal); diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index b92e894bd5..61ceb5d301 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -607,9 +607,13 @@ await RunGitAsync(barePath, ct, "config", "remote.origin.fetch", /// /// Add a repository from an existing local path (non-bare). Validates the folder is a - /// git repository with an 'origin' remote, then creates a + /// 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. /// @@ -660,23 +664,16 @@ public async Task AddRepositoryFromLocalAsync( var id = RepoIdFromUrl(url); RepositoryInfo repo; - string? oldBareClonePath = null; lock (_stateLock) { var existing = _state.Repositories.FirstOrDefault(r => r.Id == id); if (existing != null) { - // If the old BareClonePath was a managed bare clone, remember it for cleanup. - if (!string.IsNullOrWhiteSpace(existing.BareClonePath) - && !PathsEqual(existing.BareClonePath, localPath)) - { - var fullOld = Path.GetFullPath(existing.BareClonePath); - var managedPrefix = Path.GetFullPath(ReposDir) + Path.DirectorySeparatorChar; - if (fullOld.StartsWith(managedPrefix, StringComparison.OrdinalIgnoreCase)) - oldBareClonePath = existing.BareClonePath; - } - existing.BareClonePath = localPath; - BackfillWorktreeClonePaths(existing); + // A repo with this remote already exists (e.g., added via "Add from URL"). + // Keep it as-is β€” don't overwrite its BareClonePath, which would destroy + // the managed bare clone and break any worktrees that depend on it. + // The local folder will be registered as an external worktree below, + // and the UI caller (AddLocalFolderAsync) will create a πŸ“ local folder group. repo = existing; } else @@ -695,13 +692,6 @@ public async Task AddRepositoryFromLocalAsync( Save(); OnStateChanged?.Invoke(); - // Clean up orphaned managed bare clone (if any) after state is saved. - if (oldBareClonePath != null && Directory.Exists(oldBareClonePath)) - { - try { Directory.Delete(oldBareClonePath, recursive: true); } - catch (Exception ex) { Console.WriteLine($"[RepoManager] Failed to clean up old bare clone at '{oldBareClonePath}': {ex.Message}"); } - } - // Register the local folder as an external worktree so it also appears in the // "πŸ“‚ Existing" picker when creating repo-based sessions. await RegisterExternalWorktreeAsync(repo, localPath, ct); From fd99a376c10c96957f062ce761444ab769fae47a Mon Sep 17 00:00:00 2001 From: "Shane Neuville (HE/HIM)" Date: Wed, 15 Apr 2026 14:52:52 -0500 Subject: [PATCH 8/8] address PR review: generation guard, behavioral tests, extracted helper - Add ProcessingGeneration guard on HasUsedToolsThisTurn override (INV-3/INV-12) - Extract EvaluatePollCycle() static helper for testable decision logic - Add PollAction enum for poller state machine actions - Replace 3 structural tests with 7 behavioral tests for EvaluatePollCycle - Add PollIntervalSeconds constant (eliminates hardcoded 5s in log) - Fix XML doc displacement (constant doc was stealing method doc) - Narrow catch blocks to IOException for file-size probes - Move staleness detection before lastEventType null guard (Finding #5) - Update method doc to clarify persistent-mode reconnect safety Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/ProcessingWatchdogTests.cs | 117 +++++++++---- .../Services/CopilotService.Persistence.cs | 163 +++++++++++------- 2 files changed, 183 insertions(+), 97 deletions(-) diff --git a/PolyPilot.Tests/ProcessingWatchdogTests.cs b/PolyPilot.Tests/ProcessingWatchdogTests.cs index 0fecf7a9b1..19b682d0a8 100644 --- a/PolyPilot.Tests/ProcessingWatchdogTests.cs +++ b/PolyPilot.Tests/ProcessingWatchdogTests.cs @@ -3137,65 +3137,106 @@ public void Watchdog_UsedToolsTimeout_UpgradesToToolTimeout_WhenEventsJsonlFresh [Fact] public void PollMaxStaleCycles_IsReasonable() { - // PollMaxStaleCycles Γ— 5s poll interval = total staleness wait before force-reconnect. - // 12 Γ— 5s = 60s β€” long enough to avoid false triggers on legitimate long-running tools - // (where events.jsonl pauses between tool.execution_start and tool.execution_complete), - // but short enough to detect stuck sessions quickly after app restart. + // PollMaxStaleCycles Γ— PollIntervalSeconds = total staleness wait before force-reconnect. + var totalSeconds = CopilotService.PollMaxStaleCycles * CopilotService.PollIntervalSeconds; Assert.Equal(12, CopilotService.PollMaxStaleCycles); + Assert.Equal(5, CopilotService.PollIntervalSeconds); + Assert.Equal(60, totalSeconds); Assert.True(CopilotService.PollMaxStaleCycles >= 6, "Below 6 cycles (30s) risks false-positive staleness on normal tool execution gaps"); Assert.True(CopilotService.PollMaxStaleCycles <= 24, "Above 24 cycles (120s) delays stuck-session recovery unnecessarily"); } + // --- Behavioral tests for EvaluatePollCycle --- + [Fact] - public void PollStalenessDetection_ForceReconnect_InSource() + public void EvaluatePollCycle_TerminalEvent_ReturnsResume() { - // The poller must detect when events.jsonl hasn't grown and force a reconnect - // via EnsureSessionConnectedAsync. This prevents the 600s watchdog delay when - // the CLI server's session is stuck after client disconnect. - var source = File.ReadAllText( - Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); - var methodIdx = source.IndexOf("private async Task PollEventsAndResumeWhenIdleAsync"); - Assert.True(methodIdx >= 0, "PollEventsAndResumeWhenIdleAsync must exist"); - var endIdx = source.IndexOf("\n }", methodIdx); - var pollerBody = source.Substring(methodIdx, endIdx - methodIdx); + // session.shutdown is the only terminal event on disk β€” triggers resume + var action = CopilotService.EvaluatePollCycle("session.shutdown", 100, 100, 0, 12); + Assert.Equal(PollAction.Resume, action); + + // Even with large file growth, terminal takes priority + action = CopilotService.EvaluatePollCycle("session.shutdown", 200, 100, 11, 12); + Assert.Equal(PollAction.Resume, action); + } - // Must track file size for staleness detection - Assert.True(pollerBody.Contains("initialFileSize"), - "Poller must track initial file size for staleness detection"); - Assert.True(pollerBody.Contains("staleCycles"), - "Poller must count consecutive stale poll cycles"); - Assert.True(pollerBody.Contains("PollMaxStaleCycles"), - "Poller must use PollMaxStaleCycles constant for threshold"); + [Fact] + public void EvaluatePollCycle_FileNotGrowing_IncrementsStale() + { + // File same size as initial β€” stale, but below threshold + var action = CopilotService.EvaluatePollCycle("assistant.turn_end", 100, 100, 5, 12); + Assert.Equal(PollAction.IncrementStale, action); + } - // Must force-connect when stale - Assert.True(pollerBody.Contains("EnsureSessionConnectedAsync"), - "Poller must call EnsureSessionConnectedAsync when file is stale"); - Assert.True(pollerBody.Contains("[POLL-STALE]"), - "Poller must log [POLL-STALE] tag for diagnostics"); + [Fact] + public void EvaluatePollCycle_StaleCyclesReachThreshold_ForceReconnect() + { + // At threshold - 1 stale cycles, next cycle triggers reconnect + var action = CopilotService.EvaluatePollCycle("assistant.turn_end", 100, 100, 11, 12); + Assert.Equal(PollAction.ForceReconnect, action); - // After force-connect, must clear HasUsedToolsThisTurn so watchdog uses 30s quiescence - Assert.True(pollerBody.Contains("HasUsedToolsThisTurn = false"), - "Poller must clear HasUsedToolsThisTurn after force-reconnect to enable 30s quiescence"); + // Already past threshold β€” still reconnect + action = CopilotService.EvaluatePollCycle("assistant.turn_end", 100, 100, 15, 12); + Assert.Equal(PollAction.ForceReconnect, action); } [Fact] - public void PollStalenessDetection_ResetsOnGrowth_InSource() + public void EvaluatePollCycle_FileGrowing_ResetsStale() { - // When events.jsonl grows, the poller must reset its staleness tracking. - // This prevents false-positive force-reconnects on sessions where the CLI - // is actively running tools (file grows between poll cycles). + // File has grown since last baseline β€” server is alive + var action = CopilotService.EvaluatePollCycle("assistant.turn_end", 200, 100, 10, 12); + Assert.Equal(PollAction.ResetStale, action); + } + + [Fact] + public void EvaluatePollCycle_NullLastEventType_StillDetectsStaleness() + { + // Can't read file, but staleness detection must still work (Finding #5) + var action = CopilotService.EvaluatePollCycle(null, 100, 100, 11, 12); + Assert.Equal(PollAction.ForceReconnect, action); + + // Below threshold with null event type + action = CopilotService.EvaluatePollCycle(null, 100, 100, 5, 12); + Assert.Equal(PollAction.IncrementStale, action); + + // File growing with null event type β€” still resets + action = CopilotService.EvaluatePollCycle(null, 200, 100, 10, 12); + Assert.Equal(PollAction.ResetStale, action); + } + + [Fact] + public void EvaluatePollCycle_ExactThresholdBoundary() + { + // staleCycles = maxStaleCycles - 1 β†’ next increment reaches threshold β†’ ForceReconnect + // The check is: staleCycles + 1 >= maxStaleCycles + var action = CopilotService.EvaluatePollCycle("assistant.message", 100, 100, 11, 12); + Assert.Equal(PollAction.ForceReconnect, action); + + // staleCycles = maxStaleCycles - 2 β†’ not yet at threshold + action = CopilotService.EvaluatePollCycle("assistant.message", 100, 100, 10, 12); + Assert.Equal(PollAction.IncrementStale, action); + } + + [Fact] + public void PollStalenessDetection_HasGenerationGuard_InSource() + { + // INV-3/INV-12: The InvokeOnUI callback that clears HasUsedToolsThisTurn after + // force-reconnect MUST check ProcessingGeneration to prevent race with user interaction. var source = File.ReadAllText( Path.Combine(GetRepoRoot(), "PolyPilot", "Services", "CopilotService.Persistence.cs")); var methodIdx = source.IndexOf("private async Task PollEventsAndResumeWhenIdleAsync"); + Assert.True(methodIdx >= 0, "PollEventsAndResumeWhenIdleAsync must exist"); var endIdx = source.IndexOf("\n }", methodIdx); var pollerBody = source.Substring(methodIdx, endIdx - methodIdx); - // Must reset staleness counters when file grows - Assert.True(pollerBody.Contains("staleCycles = 0"), - "Poller must reset staleCycles when file grows"); - Assert.True(pollerBody.Contains("initialFileSize = currentSize"), - "Poller must update baseline file size when file grows"); + // The force-reconnect path must capture generation before async work + Assert.True(pollerBody.Contains("ProcessingGeneration") && pollerBody.Contains("reconnectGen"), + "Force-reconnect path must capture ProcessingGeneration before async EnsureSessionConnectedAsync"); + + // Must use EvaluatePollCycle for testable decision logic + Assert.True(pollerBody.Contains("EvaluatePollCycle"), + "Poller must use EvaluatePollCycle for testable decision logic"); } } \ No newline at end of file diff --git a/PolyPilot/Services/CopilotService.Persistence.cs b/PolyPilot/Services/CopilotService.Persistence.cs index 6add8fd6fb..2ae01bcb34 100644 --- a/PolyPilot/Services/CopilotService.Persistence.cs +++ b/PolyPilot/Services/CopilotService.Persistence.cs @@ -5,6 +5,21 @@ namespace PolyPilot.Services; +/// +/// Action returned by for the poller state machine. +/// +internal enum PollAction +{ + /// File is growing β€” reset staleness tracking. + ResetStale, + /// File not growing β€” increment stale counter. + IncrementStale, + /// Staleness threshold reached β€” force reconnect. + ForceReconnect, + /// Terminal event detected β€” resume the session. + Resume, +} + public partial class CopilotService { /// @@ -1055,18 +1070,10 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok } - /// - /// Polls events.jsonl for a session that's actively processing on the CLI. - /// When the CLI finishes (session.idle or session.shutdown appears, or the file - /// goes stale), triggers a lazy-resume to connect and load the response. - /// - /// IMPORTANT: We cannot call ResumeSessionAsync while the CLI is running tools β€” - /// the resume command kills in-flight tool execution. This poller bridges the gap - /// by waiting for the CLI to finish before connecting. - /// /// /// Number of consecutive poll cycles with no events.jsonl growth before the poller - /// forces a reconnect. At 5s per cycle, 12 cycles = 60s of staleness. + /// forces a reconnect. At s per cycle, + /// 12 cycles = 60s of staleness. /// This catches the common case where the CLI server's session is stuck because /// the client disconnected mid-tool β€” the server won't write new events until /// a client reconnects, creating a deadlock with the poller waiting for file changes. @@ -1075,12 +1082,48 @@ public async Task RestorePreviousSessionsAsync(CancellationToken cancellationTok /// internal const int PollMaxStaleCycles = 12; + /// + /// Poll interval in seconds for . + /// Used with to compute the staleness threshold. + /// + internal const int PollIntervalSeconds = 5; + + /// + /// Evaluates a single poll cycle and returns the action the poller should take. + /// Extracted for testability β€” the async poller loop calls this each iteration. + /// + internal static PollAction EvaluatePollCycle( + string? lastEventType, long currentSize, long initialSize, int staleCycles, int maxStaleCycles) + { + var isTerminal = lastEventType is "session.shutdown"; + if (isTerminal) + return PollAction.Resume; + + // Staleness detection runs regardless of whether we could read lastEventType. + // This ensures sessions with empty/corrupt events.jsonl don't wait 30 minutes. + if (currentSize <= initialSize) + return staleCycles + 1 >= maxStaleCycles ? PollAction.ForceReconnect : PollAction.IncrementStale; + + return PollAction.ResetStale; + } + + /// + /// Polls events.jsonl for a session that's actively processing on the CLI. + /// When the CLI finishes (session.shutdown appears, or the file goes stale), + /// triggers a lazy-resume to connect and load the response. + /// + /// NOTE: The 60s staleness threshold ( Γ— ) + /// may trigger a reconnect while a long-running tool is still executing. In persistent server mode, + /// the headless server survives client reconnects, so this is generally safe. If the server is alive, + /// events replay after reconnect and normal handling resumes. If not, the watchdog's 30s quiescence + /// timeout detects the dead session. + /// private async Task PollEventsAndResumeWhenIdleAsync( string sessionName, SessionState state, string sessionId, CancellationToken ct) { var eventsFile = Path.Combine(SessionStatePath, sessionId, "events.jsonl"); var maxPollTime = TimeSpan.FromMinutes(30); - var pollInterval = TimeSpan.FromSeconds(5); + var pollInterval = TimeSpan.FromSeconds(PollIntervalSeconds); var started = DateTime.UtcNow; Debug($"[POLL] Starting events.jsonl poll for '{sessionName}' (id={sessionId})"); @@ -1088,7 +1131,8 @@ private async Task PollEventsAndResumeWhenIdleAsync( // Track file size for staleness detection β€” if the file doesn't grow for // PollMaxStaleCycles consecutive checks, the server's session is likely stuck. long initialFileSize = 0; - try { if (File.Exists(eventsFile)) initialFileSize = new FileInfo(eventsFile).Length; } catch { } + try { if (File.Exists(eventsFile)) initialFileSize = new FileInfo(eventsFile).Length; } + catch (IOException) { } int staleCycles = 0; try @@ -1117,63 +1161,64 @@ private async Task PollEventsAndResumeWhenIdleAsync( // session.error is also not persisted. Only session.shutdown is reliably on disk. // The watchdog is the primary completion detection for disconnected sessions. var lastEventType = GetLastEventType(eventsFile); - if (lastEventType == null) continue; - var isTerminal = lastEventType is "session.shutdown"; + long currentSize = 0; + try { if (File.Exists(eventsFile)) currentSize = new FileInfo(eventsFile).Length; } + catch (IOException) { } + + var action = EvaluatePollCycle(lastEventType, currentSize, initialFileSize, staleCycles, PollMaxStaleCycles); - // Staleness detection: if events.jsonl hasn't grown since the poller started, - // the CLI server's session is likely stuck (client disconnected mid-tool, server - // can't proceed). Force-connect to re-establish the event stream. After connecting, - // EnsureSessionConnectedAsync's post-resume logic handles the rest: - // - If the server was still processing: events replay/flow, normal handling resumes - // - If the server's session is dead: watchdog's 30s quiescence timeout detects it - if (!isTerminal) + switch (action) { - long currentSize = 0; - try { if (File.Exists(eventsFile)) currentSize = new FileInfo(eventsFile).Length; } catch { } + case PollAction.ResetStale: + staleCycles = 0; + initialFileSize = currentSize; + continue; - if (currentSize <= initialFileSize) - { + case PollAction.IncrementStale: + staleCycles++; + continue; + + case PollAction.ForceReconnect: staleCycles++; - if (staleCycles >= PollMaxStaleCycles) + Debug($"[POLL-STALE] '{sessionName}' events.jsonl hasn't grown for {staleCycles * PollIntervalSeconds}s " + + $"(size={currentSize}, initial={initialFileSize}) β€” forcing reconnect"); + try { - Debug($"[POLL-STALE] '{sessionName}' events.jsonl hasn't grown for {staleCycles * 5}s " + - $"(size={currentSize}, initial={initialFileSize}) β€” forcing reconnect"); - try - { - await EnsureSessionConnectedAsync(sessionName, state, ct); - Debug($"[POLL-STALE] '{sessionName}' reconnected successfully"); - - // Override HasUsedToolsThisTurn so the watchdog can use the 30s - // resume-quiescence timeout instead of 600s. If the server is alive, - // events will replay and HasReceivedEventsSinceResume goes true, - // which skips quiescence β€” so this override is safe. - // If the server is dead, no events arrive and quiescence fires at 30s. - InvokeOnUI(() => - { - state.HasUsedToolsThisTurn = false; - }); - } - catch (Exception ex) + // INV-3/INV-12: Capture generation BEFORE async reconnect. + var reconnectGen = Interlocked.Read(ref state.ProcessingGeneration); + + await EnsureSessionConnectedAsync(sessionName, state, ct); + Debug($"[POLL-STALE] '{sessionName}' reconnected successfully"); + + // Override HasUsedToolsThisTurn so the watchdog can use the 30s + // resume-quiescence timeout instead of 600s. If the server is alive, + // events will replay and HasReceivedEventsSinceResume goes true, + // which skips quiescence β€” so this override is safe. + // If the server is dead, no events arrive and quiescence fires at 30s. + InvokeOnUI(() => { - Debug($"[POLL-STALE] '{sessionName}' reconnect failed: {ex.Message}"); - // Fall through to next poll cycle β€” don't exit - staleCycles = 0; // Reset to avoid immediate re-trigger - } - // state.Session is now set (if connect succeeded), next loop iteration - // will return via the state.Session != null check above. - continue; + // Guard: if a new turn started during the async reconnect, + // don't clear state belonging to the new turn. + if (Interlocked.Read(ref state.ProcessingGeneration) != reconnectGen) return; + state.HasUsedToolsThisTurn = false; + }); } - } - else - { - // File is growing β€” server is alive, reset staleness tracking - staleCycles = 0; - initialFileSize = currentSize; - } + catch (Exception ex) + { + Debug($"[POLL-STALE] '{sessionName}' reconnect failed: {ex.Message}"); + staleCycles = 0; // Reset to avoid immediate re-trigger on next cycle + } + // state.Session is now set (if connect succeeded), next loop iteration + // will return via the state.Session != null check above. + continue; + + case PollAction.Resume: + // Fall through to terminal handling below + break; } - if (isTerminal) + // PollAction.Resume: terminal event detected { Debug($"[POLL] '{sessionName}' CLI finished (lastEvent={lastEventType}) β€” resuming session");