fix: reject an unsupported saved repository at load instead of crashing [patch]

ProjectDirector.Test/UnsupportedRepoTests.cs

@@ -0,0 +1,248 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector.Test;
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text.Json;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Tests the rule that refuses a saved repository this application cannot act on.
+/// </summary>
+/// <remarks>
+/// <see cref="GitRepository"/> registers <see cref="AzureDevOpsRepository"/> as a derived type for
+/// polymorphic JSON, so a saved options file carrying one deserializes without complaint. Nothing
+/// then acts on it: every site that pattern-matches a repository handles
+/// <see cref="GitHubRepository"/> and throws otherwise. <c>UpdateClonedStatus</c> runs from the
+/// constructor's <c>RefreshPage</c>, so that throw landed on the next launch, before the user could
+/// open the UI to delete the entry causing it -- unrecoverable without hand-editing the file.
+///
+/// <c>RejectUnsupportedRepos</c> and <c>MakeLoadedOptionsSafe</c> exist so the one boundary such an
+/// entry can arrive through can be driven without a live ImGui context, the way
+/// <see cref="PullDecisionTests"/> drives the pull rule. What it guarantees is the invariant those six throw sites rest on: after it
+/// runs, every repository left in the options is a <see cref="GitHubRepository"/>.
+/// </remarks>
+[TestClass]
+public sealed class UnsupportedRepoTests
+{
+	private static FullyQualifiedGitHubRepoName RepoName(string value) => FullyQualifiedGitHubRepoName.Create<FullyQualifiedGitHubRepoName>(value);
+	private static FullyQualifiedLocalRepoPath LocalPath(string value) => FullyQualifiedLocalRepoPath.Create<FullyQualifiedLocalRepoPath>(value);
+
+	/// <summary>
+	/// The premise: an Azure DevOps repository really does come back from JSON as a live object,
+	/// rather than being rejected by the serializer.
+	/// </summary>
+	/// <remarks>
+	/// The discriminator alone is enough, which is the point -- this is what a hand-edited options
+	/// file, or one left over from a partially built feature, looks like.
+	/// </remarks>
+	[TestMethod]
+	public void AnAzureDevOpsRepositoryDeserializesIntoALiveObject()
+	{
+		GitRepository? fromSavedOptions = JsonSerializer.Deserialize<GitRepository>("""{"TypeName":"AzureDevOpsRepository"}""");
+
+		Assert.IsInstanceOfType<AzureDevOpsRepository>(fromSavedOptions,
+			"If this stops holding, the crash this rule guards against is no longer reachable and the rule can go.");
+	}
+
+	/// <summary>
+	/// The regression this file exists for.
+	/// </summary>
+	[TestMethod]
+	public void AnUnsupportedRepoIsDroppedAndTheSupportedOnesAreKept()
+	{
+		Dictionary<FullyQualifiedGitHubRepoName, GitRepository> repos = new()
+		{
+			[RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(),
+			[RepoName("contoso.internal")] = new AzureDevOpsRepository(),
+		};
+		Dictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos = [];
+
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos);
+
+		CollectionAssert.AreEqual(new[] { RepoName("contoso.internal") }, rejected.ToArray());
+		CollectionAssert.AreEqual(
+			new[] { RepoName("ktsu-dev.ProjectDirector") },
+			repos.Keys.ToArray(),
+			"The supported repositories must survive.");
+
+		Assert.IsTrue(repos.Values.All(repo => repo is GitHubRepository),
+			"Every site that pattern-matches a repository rests on this invariant.");
+	}
+
+	[TestMethod]
+	public void NothingUnsupportedMeansNothingIsTouched()
+	{
+		Dictionary<FullyQualifiedGitHubRepoName, GitRepository> repos = new()
+		{
+			[RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(),
+			[RepoName("ktsu-dev.ImGuiApp")] = new GitHubRepository(),
+		};
+		Dictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos = new()
+		{
+			[LocalPath("/dev/ProjectDirector")] = RepoName("ktsu-dev.ProjectDirector"),
+		};
+
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos);
+
+		Assert.AreEqual(0, rejected.Count);
+		Assert.AreEqual(2, repos.Count);
+		Assert.AreEqual(1, clonedRepos.Count);
+		Assert.AreEqual(
+			RepoName("ktsu-dev.ProjectDirector"),
+			ProjectDirector.ClearSelectionIfRejected(RepoName("ktsu-dev.ProjectDirector"), rejected),
+			"An ordinary selection must not be disturbed.");
+	}
+
+	[TestMethod]
+	public void ACloneRecordedAgainstADroppedRepoIsCleared()
+	{
+		Dictionary<FullyQualifiedGitHubRepoName, GitRepository> repos = new()
+		{
+			[RepoName("contoso.internal")] = new AzureDevOpsRepository(),
+			[RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(),
+		};
+		Dictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos = new()
+		{
+			[LocalPath("/dev/internal")] = RepoName("contoso.internal"),
+			[LocalPath("/dev/ProjectDirector")] = RepoName("ktsu-dev.ProjectDirector"),
+		};
+
+		_ = ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos);
+
+		CollectionAssert.AreEqual(
+			new[] { LocalPath("/dev/ProjectDirector") },
+			clonedRepos.Keys.ToArray(),
+			"A clone must not keep naming a repository that is no longer in the options.");
+	}
+
+	[TestMethod]
+	public void EveryRepoBeingUnsupportedLeavesAnEmptyButUsableState()
+	{
+		Dictionary<FullyQualifiedGitHubRepoName, GitRepository> repos = new()
+		{
+			[RepoName("contoso.internal")] = new AzureDevOpsRepository(),
+			[RepoName("contoso.other")] = new AzureDevOpsRepository(),
+		};
+		Dictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos = new()
+		{
+			[LocalPath("/dev/internal")] = RepoName("contoso.internal"),
+		};
+
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos);
+
+		Assert.AreEqual(2, rejected.Count);
+		Assert.AreEqual(0, repos.Count);
+		Assert.AreEqual(0, clonedRepos.Count);
+	}
+
+	/// <summary>
+	/// The whole of what the constructor does after loading, against real options: the crashing
+	/// entry goes, the selection pointing at it goes with it, the supported repository stays, and
+	/// the user is told why.
+	/// </summary>
+	[TestMethod]
+	public void LoadedOptionsCarryingAnUnsupportedRepoAreMadeSafe()
+	{
+		using ProjectDirectorOptions options = new()
+		{
+			BaseRepo = RepoName("contoso.internal"),
+			CompareRepo = RepoName("ktsu-dev.ProjectDirector"),
+		};
+		options.Repos[RepoName("contoso.internal")] = new AzureDevOpsRepository();
+		options.Repos[RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository();
+		options.ClonedRepos[LocalPath("/dev/internal")] = RepoName("contoso.internal");
+
+		List<string> logged = [];
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = ProjectDirector.MakeLoadedOptionsSafe(options, logged.Add);
+
+		CollectionAssert.AreEqual(new[] { RepoName("contoso.internal") }, rejected.ToArray());
+		CollectionAssert.AreEqual(new[] { RepoName("ktsu-dev.ProjectDirector") }, options.Repos.Keys.ToArray());
+		Assert.AreEqual(0, options.ClonedRepos.Count);
+
+		Assert.AreEqual(new FullyQualifiedGitHubRepoName(), options.BaseRepo, "The base selection named the rejected repository.");
+		Assert.AreEqual(RepoName("ktsu-dev.ProjectDirector"), options.CompareRepo, "The compare selection named a surviving one and must be left alone.");
+
+		Assert.AreEqual(1, logged.Count, "Each rejection should be reported once.");
+		StringAssert.Contains(logged[0], "contoso.internal", StringComparison.Ordinal);
+	}
+
+	[TestMethod]
+	public void LoadedOptionsWithNothingUnsupportedAreLeftAlone()
+	{
+		using ProjectDirectorOptions options = new()
+		{
+			BaseRepo = RepoName("ktsu-dev.ProjectDirector"),
+		};
+		options.Repos[RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository();
+
+		List<string> logged = [];
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = ProjectDirector.MakeLoadedOptionsSafe(options, logged.Add);
+
+		Assert.AreEqual(0, rejected.Count);
+		Assert.AreEqual(1, options.Repos.Count);
+		Assert.AreEqual(RepoName("ktsu-dev.ProjectDirector"), options.BaseRepo);
+		Assert.AreEqual(0, logged.Count, "Nothing to reject means nothing to report.");
+	}
+
+	/// <summary>
+	/// Fresh options have to be constructible on every platform this repository tests on, which is
+	/// what a hardcoded <c>C:\dev</c> default prevented.
+	/// </summary>
+	[TestMethod]
+	public void FreshOptionsCanBeConstructed()
+	{
+		using ProjectDirectorOptions options = new();
+
+		Assert.IsFalse(string.IsNullOrEmpty(options.DevDirectory), "A fresh install needs a usable dev directory default.");
+	}
+
+	[TestMethod]
+	public void EmptyOptionsAreHandled()
+	{
+		Dictionary<FullyQualifiedGitHubRepoName, GitRepository> repos = [];
+		Dictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos = [];
+
+		Assert.AreEqual(0, ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos).Count);
+		Assert.AreEqual(0, repos.Count);
+	}
+
+	/// <summary>
+	/// A selection left pointing at a dropped repository would send the panels straight back into
+	/// <c>Options.Repos[Options.BaseRepo]</c>, turning one crash into another.
+	/// </summary>
+	[TestMethod]
+	public void ASelectionPointingAtADroppedRepoIsCleared()
+	{
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = [RepoName("contoso.internal")];
+
+		Assert.AreEqual(
+			new FullyQualifiedGitHubRepoName(),
+			ProjectDirector.ClearSelectionIfRejected(RepoName("contoso.internal"), rejected),
+			"The selection must not outlive the repository it names.");
+	}
+
+	[TestMethod]
+	public void ASelectionNamingASurvivingRepoIsKept()
+	{
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = [RepoName("contoso.internal")];
+
+		Assert.AreEqual(
+			RepoName("ktsu-dev.ProjectDirector"),
+			ProjectDirector.ClearSelectionIfRejected(RepoName("ktsu-dev.ProjectDirector"), rejected));
+	}
+
+	[TestMethod]
+	public void AnEmptySelectionSurvivesAnEmptyRejectionList()
+	{
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = [];
+
+		Assert.AreEqual(
+			new FullyQualifiedGitHubRepoName(),
+			ProjectDirector.ClearSelectionIfRejected(new FullyQualifiedGitHubRepoName(), rejected),
+			"A fresh install selects nothing, and that must not be mistaken for a rejection.");
+	}
+}

ProjectDirector/ProjectDirector.cs

@@ -18,7 +18,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -43,7 +43,7 @@
 	private Collection<RelativePath> BrowserContentsCompare { get; set; } = [];
 	private PopupPropagateFile PopupPropagateFile { get; } = new();
 
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _)
 	{
@@ -62,8 +62,11 @@
 	public ProjectDirector()
 	{
 		Options = ProjectDirectorOptions.LoadOrCreate();
+
+		_ = MakeLoadedOptionsSafe(Options, QueueLog);
+
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -98,6 +101,106 @@
 		RefreshPage();
 	}
 
+	/// <summary>
+	/// Drops any saved repository this application cannot act on, along with any selection left
+	/// pointing at one.
+	/// </summary>
+	/// <param name="repos">The freshly loaded repositories, modified in place.</param>
+	/// <param name="clonedRepos">The freshly loaded clone records, modified in place.</param>
+	/// <returns>The names of the repositories that were dropped, in the order they were found.</returns>
+	/// <remarks>
+	/// <see cref="GitRepository"/> registers <see cref="AzureDevOpsRepository"/> as a
+	/// <see cref="System.Text.Json.Serialization.JsonDerivedTypeAttribute"/>, so a saved options
+	/// file carrying one deserializes without complaint. Nothing acts on it: every site that
+	/// pattern-matches a repository handles <see cref="GitHubRepository"/> and throws otherwise,
+	/// and <see cref="GitRepository.Create"/> cannot produce anything else in the first place.
+	/// <c>UpdateClonedStatus</c> then runs from the constructor's <c>RefreshPage</c>, so the throw
+	/// landed on the very next launch -- before the user could open the UI and delete the entry
+	/// that was causing it, which made it unrecoverable without hand-editing the file.
+	/// Rejecting the entry at the one boundary it can arrive through is what makes that
+	/// unreachable, rather than guarding six call sites separately. The registration is left in
+	/// place so an existing file still parses; it is the live object that is refused.
+	/// The collections are taken rather than the whole <see cref="ProjectDirectorOptions"/> so this
+	/// rule can be driven without constructing one.
+	/// </remarks>
+	internal static IReadOnlyList<FullyQualifiedGitHubRepoName> RejectUnsupportedRepos(
+		IDictionary<FullyQualifiedGitHubRepoName, GitRepository> repos,
+		IDictionary<FullyQualifiedLocalRepoPath, FullyQualifiedGitHubRepoName> clonedRepos)
+	{
+		Ensure.NotNull(repos);
+		Ensure.NotNull(clonedRepos);
+
+		List<FullyQualifiedGitHubRepoName> rejected = [.. repos
+			.Where(kvp => kvp.Value is not GitHubRepository)
+			.Select(kvp => kvp.Key)];
+
+		foreach (FullyQualifiedGitHubRepoName name in rejected)
+		{
+			_ = repos.Remove(name);
+		}
+
+		// A clone recorded against a rejected repository would otherwise keep naming it.
+		foreach (FullyQualifiedLocalRepoPath path in clonedRepos
+			.Where(kvp => rejected.Contains(kvp.Value))
+			.Select(kvp => kvp.Key)
+			.ToList())
+		{
+			_ = clonedRepos.Remove(path);
+		}
+
+		return rejected;
+	}
+
+	/// <summary>
+	/// Clears a selected repository name that names one of the <paramref name="rejected"/>
+	/// repositories.
+	/// </summary>
+	/// <param name="selection">The saved selection.</param>
+	/// <param name="rejected">The repositories that were dropped.</param>
+	/// <returns>The selection, or an empty name where it named a dropped repository.</returns>
+	/// <remarks>
+	/// Once something is selected the panels reach for it through
+	/// <c>Options.Repos[Options.BaseRepo]</c>, so a selection outliving its repository turns one
+	/// crash into another. An empty name is the state a fresh install starts in, which the
+	/// surrounding <c>TryGetValue</c> checks already handle.
+	/// </remarks>
+	internal static FullyQualifiedGitHubRepoName ClearSelectionIfRejected(
+		FullyQualifiedGitHubRepoName selection,
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected)
+	{
+		Ensure.NotNull(rejected);
+
+		return rejected.Contains(selection) ? new() : selection;
+	}
+
+	/// <summary>
+	/// Rejects every saved repository this application cannot act on, clears anything left pointing
+	/// at one, and reports each rejection.
+	/// </summary>
+	/// <param name="options">The freshly loaded options, modified in place.</param>
+	/// <param name="log">Where to report each rejected repository.</param>
+	/// <returns>The names of the repositories that were dropped.</returns>
+	/// <remarks>
+	/// The whole of what the constructor does after loading, in one place, so it can be driven
+	/// without an ImGui context.
+	/// </remarks>
+	internal static IReadOnlyList<FullyQualifiedGitHubRepoName> MakeLoadedOptionsSafe(ProjectDirectorOptions options, Action<string> log)
+	{
+		Ensure.NotNull(options);
+		Ensure.NotNull(log);
+
+		IReadOnlyList<FullyQualifiedGitHubRepoName> rejected = RejectUnsupportedRepos(options.Repos, options.ClonedRepos);
+		options.BaseRepo = ClearSelectionIfRejected(options.BaseRepo, rejected);
+		options.CompareRepo = ClearSelectionIfRejected(options.CompareRepo, rejected);
+
+		foreach (FullyQualifiedGitHubRepoName name in rejected)
+		{
+			log($"Ignoring saved repository '{name}': only GitHub repositories are supported at this time.");
+		}
+
+		return rejected;
+	}
+
 	private void QueueLog(string logMessage)
 	{
 		LogQueue.Enqueue(logMessage);
@@ -501,7 +604,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -1614,7 +1717,7 @@
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;
 					//}

ProjectDirector/ProjectDirectorOptions.cs

@@ -18,7 +18,22 @@ public sealed record class FullyQualifiedLocalRepoPath : SemanticString<FullyQua
 
 public sealed class ProjectDirectorOptions : AppData<ProjectDirectorOptions>
 {
-	public AbsoluteDirectoryPath DevDirectory { get; set; } = AbsoluteDirectoryPath.Create<AbsoluteDirectoryPath>(@"C:\dev");
+	public AbsoluteDirectoryPath DevDirectory { get; set; } = DefaultDevDirectory();
+
+	/// <summary>
+	/// The dev directory a fresh install starts with.
+	/// </summary>
+	/// <remarks>
+	/// <c>C:\dev</c> is not an absolute path anywhere but Windows, so
+	/// <see cref="AbsoluteDirectoryPath"/> rejected it and this type could not be constructed at all
+	/// off Windows -- not by the application, and not by a test. Windows keeps the path it has
+	/// always had; everywhere else falls back to <c>~/dev</c>. Only a fresh install reads this, so a
+	/// saved options file keeps whatever the user chose.
+	/// </remarks>
+	private static AbsoluteDirectoryPath DefaultDevDirectory() =>
+		AbsoluteDirectoryPath.Create<AbsoluteDirectoryPath>(OperatingSystem.IsWindows()
+			? @"C:\dev"
+			: Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "dev"));
 	public ImGuiAppWindowState WindowState { get; set; } = new();
 
 	public GitHubLogin GitHubLogin { get; set; } = new();