From 8084bd7c32ed631ea8c93723d66291dd7fa89c20 Mon Sep 17 00:00:00 2001 From: Shane Date: Wed, 25 Feb 2026 21:24:59 -0600 Subject: [PATCH 1/2] Fix RepoManager test isolation: lock static path resolution to prevent race Root cause: RepoManager used non-atomic ??= for static path caching (StateFile, ReposDir, WorktreesDir). Tests that call SetBaseDirForTesting used Volatile.Write to clear the cache, but parallel test execution could interleave between the override write and the cache resolution, causing Save() to write to the real ~/.polypilot/repos.json instead of the test temp directory. This corrupted the user's repos.json with test data ('repo-1', 'MyRepo') and wiped all real repository registrations. Fix: - Add _pathLock around all static path resolution (getters and setter) - RepoManagerTests uses SetBaseDirForTesting instead of raw field reflection - Add [Collection('BaseDir')] to RepoManagerTests for serialization - Add guard tests verifying RepoManager paths point to test directory Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- PolyPilot.Tests/RepoManagerTests.cs | 18 ++++++------ PolyPilot.Tests/TestIsolationGuardTests.cs | 32 ++++++++++++++++++++++ PolyPilot/Services/RepoManager.cs | 18 +++++++----- 3 files changed, 51 insertions(+), 17 deletions(-) diff --git a/PolyPilot.Tests/RepoManagerTests.cs b/PolyPilot.Tests/RepoManagerTests.cs index 4eb3f6083a..0814955ca7 100644 --- a/PolyPilot.Tests/RepoManagerTests.cs +++ b/PolyPilot.Tests/RepoManagerTests.cs @@ -3,6 +3,7 @@ namespace PolyPilot.Tests; +[Collection("BaseDir")] public class RepoManagerTests { [Theory] @@ -99,10 +100,8 @@ public void Save_AfterFailedLoad_DoesNotOverwriteWithEmptyState() SetField(rm, "_loadedSuccessfully", false); SetField(rm, "_state", new RepositoryState()); - // Override StateFile to our temp path - var stateFileField = typeof(RepoManager).GetField("_stateFile", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; - var originalValue = stateFileField.GetValue(null); - stateFileField.SetValue(null, stateFile); + // Redirect RepoManager to our temp dir (safe — uses the lock-protected setter) + RepoManager.SetBaseDirForTesting(tempDir); try { // Save should be blocked — empty state after failed load @@ -114,7 +113,7 @@ public void Save_AfterFailedLoad_DoesNotOverwriteWithEmptyState() } finally { - stateFileField.SetValue(null, originalValue); + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } } finally @@ -129,7 +128,6 @@ public void Save_AfterSuccessfulLoad_PersistsEmptyState() var rm = new RepoManager(); var tempDir = Path.Combine(Path.GetTempPath(), $"repomgr-test-{Guid.NewGuid():N}"); Directory.CreateDirectory(tempDir); - var stateFile = Path.Combine(tempDir, "repos.json"); try { @@ -138,21 +136,21 @@ public void Save_AfterSuccessfulLoad_PersistsEmptyState() SetField(rm, "_loadedSuccessfully", true); SetField(rm, "_state", new RepositoryState()); - var stateFileField = typeof(RepoManager).GetField("_stateFile", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; - var originalValue = stateFileField.GetValue(null); - stateFileField.SetValue(null, stateFile); + // Redirect RepoManager to our temp dir (safe — uses the lock-protected setter) + RepoManager.SetBaseDirForTesting(tempDir); try { // Save should proceed — load was successful, intentional empty state InvokeSave(rm); + var stateFile = Path.Combine(tempDir, "repos.json"); var content = File.ReadAllText(stateFile); Assert.Contains("Repositories", content); Assert.DoesNotContain("test-1", content); } finally { - stateFileField.SetValue(null, originalValue); + RepoManager.SetBaseDirForTesting(TestSetup.TestBaseDir); } } finally diff --git a/PolyPilot.Tests/TestIsolationGuardTests.cs b/PolyPilot.Tests/TestIsolationGuardTests.cs index 1ce64308ce..2daa29eb15 100644 --- a/PolyPilot.Tests/TestIsolationGuardTests.cs +++ b/PolyPilot.Tests/TestIsolationGuardTests.cs @@ -40,6 +40,38 @@ public void BaseDir_PointsToTempDirectory() Assert.Contains("polypilot-tests-", CopilotService.BaseDir); } + [Fact] + public void RepoManager_StateFile_IsNotRealPolypilotDir() + { + // Access the static StateFile via reflection to verify it doesn't point to real path + var stateFileField = typeof(RepoManager).GetField("_stateFile", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + // Force resolution by accessing Repositories (which calls EnsureLoaded -> Load -> StateFile) + var rm = new RepoManager(); + _ = rm.Repositories; + + var stateFile = (string?)stateFileField.GetValue(null); + var realReposJson = Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), + ".polypilot", "repos.json"); + + Assert.NotNull(stateFile); + Assert.NotEqual(realReposJson, stateFile); + Assert.DoesNotContain(Path.Combine(".polypilot", "repos.json"), stateFile); + } + + [Fact] + public void RepoManager_BaseDir_MatchesTestSetupDir() + { + // Verify RepoManager resolves to the same test directory as CopilotService + var baseDirOverride = typeof(RepoManager).GetField("_baseDirOverride", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static)!; + var overrideValue = (string?)baseDirOverride.GetValue(null); + + Assert.NotNull(overrideValue); + Assert.Equal(TestSetup.TestBaseDir, overrideValue); + } + [Fact] public void TestSetup_ModuleInitializer_HasRun() { diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index 740f5d990a..4c519f6a9a 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -11,12 +11,13 @@ namespace PolyPilot.Services; public class RepoManager { private static string? _baseDirOverride; + private static readonly object _pathLock = new(); private static string? _reposDir; - private static string ReposDir => _reposDir ??= GetReposDir(); + private static string ReposDir { get { lock (_pathLock) return _reposDir ??= GetReposDir(); } } private static string? _worktreesDir; - private static string WorktreesDir => _worktreesDir ??= GetWorktreesDir(); + private static string WorktreesDir { get { lock (_pathLock) return _worktreesDir ??= GetWorktreesDir(); } } private static string? _stateFile; - private static string StateFile => _stateFile ??= GetStateFile(); + private static string StateFile { get { lock (_pathLock) return _stateFile ??= GetStateFile(); } } /// /// Redirect all RepoManager paths to a test directory. @@ -24,10 +25,13 @@ public class RepoManager /// internal static void SetBaseDirForTesting(string? path) { - Volatile.Write(ref _baseDirOverride, path); - Volatile.Write(ref _reposDir, null); - Volatile.Write(ref _worktreesDir, null); - Volatile.Write(ref _stateFile, null); + lock (_pathLock) + { + _baseDirOverride = path; + _reposDir = null; + _worktreesDir = null; + _stateFile = null; + } } private RepositoryState _state = new(); From a8cc5fc7ccec262cf907cee5c55b94655abd95a7 Mon Sep 17 00:00:00 2001 From: Shane Date: Wed, 25 Feb 2026 21:44:22 -0600 Subject: [PATCH 2/2] Address review findings: CopilotService _pathLock, TOCTOU fix, Volatile.Read cleanup Finding 1: CopilotService had the same static-path race condition as RepoManager. Added _pathLock around all static path getters and SetBaseDirForTesting. Finding 2: Save() and Load() in RepoManager accessed StateFile twice without holding the lock across both calls. Captured to local variable once. Finding 3: GetBaseDir() used Volatile.Read paired with a lock-protected write. Replaced with plain read since it's only called within _pathLock. All 1400 tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Services/CopilotService.Organization.cs | 2 +- PolyPilot/Services/CopilotService.cs | 36 ++++++++++--------- PolyPilot/Services/RepoManager.cs | 13 ++++--- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/PolyPilot/Services/CopilotService.Organization.cs b/PolyPilot/Services/CopilotService.Organization.cs index 0a55976f04..2a448edadf 100644 --- a/PolyPilot/Services/CopilotService.Organization.cs +++ b/PolyPilot/Services/CopilotService.Organization.cs @@ -1108,7 +1108,7 @@ private void AddOrchestratorSystemMessage(string sessionName, string message) #region Orchestration Persistence (relaunch resilience) private static string? _pendingOrchestrationFile; - private static string PendingOrchestrationFile => _pendingOrchestrationFile ??= Path.Combine(PolyPilotBaseDir, "pending-orchestration.json"); + private static string PendingOrchestrationFile { get { lock (_pathLock) return _pendingOrchestrationFile ??= Path.Combine(PolyPilotBaseDir, "pending-orchestration.json"); } } internal void SavePendingOrchestration(PendingOrchestration pending) { diff --git a/PolyPilot/Services/CopilotService.cs b/PolyPilot/Services/CopilotService.cs index 94a09a8229..2f88a0f9fd 100644 --- a/PolyPilot/Services/CopilotService.cs +++ b/PolyPilot/Services/CopilotService.cs @@ -40,11 +40,12 @@ public partial class CopilotService : IAsyncDisposable private string? _activeSessionName; private SynchronizationContext? _syncContext; + private static readonly object _pathLock = new(); private static string? _copilotBaseDir; - private static string CopilotBaseDir => LazyInitializer.EnsureInitialized(ref _copilotBaseDir, GetCopilotBaseDir); + private static string CopilotBaseDir { get { lock (_pathLock) return _copilotBaseDir ??= GetCopilotBaseDir(); } } private static string? _polyPilotBaseDir; - private static string PolyPilotBaseDir => LazyInitializer.EnsureInitialized(ref _polyPilotBaseDir, GetPolyPilotBaseDir); + private static string PolyPilotBaseDir { get { lock (_pathLock) return _polyPilotBaseDir ??= GetPolyPilotBaseDir(); } } internal static string BaseDir => PolyPilotBaseDir; private static string GetCopilotBaseDir() @@ -100,19 +101,19 @@ private static string GetPolyPilotBaseDir() } private static string? _sessionStatePath; - private static string SessionStatePath => _sessionStatePath ??= Path.Combine(CopilotBaseDir, "session-state"); + private static string SessionStatePath { get { lock (_pathLock) return _sessionStatePath ??= Path.Combine(CopilotBaseDir, "session-state"); } } private static string? _activeSessionsFile; - private static string ActiveSessionsFile => _activeSessionsFile ??= Path.Combine(PolyPilotBaseDir, "active-sessions.json"); + private static string ActiveSessionsFile { get { lock (_pathLock) return _activeSessionsFile ??= Path.Combine(PolyPilotBaseDir, "active-sessions.json"); } } private static string? _sessionAliasesFile; - private static string SessionAliasesFile => _sessionAliasesFile ??= Path.Combine(PolyPilotBaseDir, "session-aliases.json"); + private static string SessionAliasesFile { get { lock (_pathLock) return _sessionAliasesFile ??= Path.Combine(PolyPilotBaseDir, "session-aliases.json"); } } private static string? _uiStateFile; - private static string UiStateFile => _uiStateFile ??= Path.Combine(PolyPilotBaseDir, "ui-state.json"); + private static string UiStateFile { get { lock (_pathLock) return _uiStateFile ??= Path.Combine(PolyPilotBaseDir, "ui-state.json"); } } private static string? _organizationFile; - private static string OrganizationFile => _organizationFile ??= Path.Combine(PolyPilotBaseDir, "organization.json"); + private static string OrganizationFile { get { lock (_pathLock) return _organizationFile ??= Path.Combine(PolyPilotBaseDir, "organization.json"); } } /// /// Override base directory for tests to prevent writing to real ~/.polypilot/. @@ -120,18 +121,21 @@ private static string GetPolyPilotBaseDir() /// internal static void SetBaseDirForTesting(string path) { - Volatile.Write(ref _polyPilotBaseDir, path); - _activeSessionsFile = null; - _sessionAliasesFile = null; - _uiStateFile = null; - _organizationFile = null; - Volatile.Write(ref _copilotBaseDir, null); - _sessionStatePath = null; - _pendingOrchestrationFile = null; + lock (_pathLock) + { + _polyPilotBaseDir = path; + _activeSessionsFile = null; + _sessionAliasesFile = null; + _uiStateFile = null; + _organizationFile = null; + _copilotBaseDir = null; + _sessionStatePath = null; + _pendingOrchestrationFile = null; + } } private static string? _projectDir; - private static string ProjectDir => _projectDir ??= FindProjectDir(); + private static string ProjectDir { get { lock (_pathLock) return _projectDir ??= FindProjectDir(); } } private static string FindProjectDir() { diff --git a/PolyPilot/Services/RepoManager.cs b/PolyPilot/Services/RepoManager.cs index 4c519f6a9a..51b3193987 100644 --- a/PolyPilot/Services/RepoManager.cs +++ b/PolyPilot/Services/RepoManager.cs @@ -50,7 +50,8 @@ private void EnsureLoaded() private static string GetBaseDir() { - var over = Volatile.Read(ref _baseDirOverride); + // Called from within _pathLock — no Volatile.Read needed + var over = _baseDirOverride; if (over != null) return over; try { @@ -75,9 +76,10 @@ public void Load() _loadedSuccessfully = false; try { - if (File.Exists(StateFile)) + var stateFile = StateFile; // resolve once + if (File.Exists(stateFile)) { - var json = File.ReadAllText(StateFile); + var json = File.ReadAllText(stateFile); _state = JsonSerializer.Deserialize(json) ?? new RepositoryState(); } _loadedSuccessfully = true; @@ -102,9 +104,10 @@ private void Save() _loadedSuccessfully = true; try { - Directory.CreateDirectory(Path.GetDirectoryName(StateFile)!); + var stateFile = StateFile; // resolve once + Directory.CreateDirectory(Path.GetDirectoryName(stateFile)!); var json = JsonSerializer.Serialize(_state, new JsonSerializerOptions { WriteIndented = true }); - File.WriteAllText(StateFile, json); + File.WriteAllText(stateFile, json); } catch (Exception ex) {