From 7c858ca18a19cdb1ed290033282e3d7350105bc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:33:05 +0000 Subject: [PATCH 1/2] fix: reject an unsupported saved repository at load instead of crashing [patch] GitRepository registers AzureDevOpsRepository as a JsonDerivedType, so a saved options file carrying one deserializes without complaint. Nothing acts on it: all six sites that pattern-match a repository handle GitHubRepository and throw otherwise, and GitRepository.Create cannot produce anything else in the first place. UpdateClonedStatus runs from the constructor's RefreshPage, so the throw landed on the very next launch -- before the user could open the UI and delete the entry causing it, which made it unrecoverable without hand-editing the file. Deserialization is the only boundary such an entry can arrive through, so reject it there rather than guarding six call sites separately. That is what turns every later `is GitHubRepository` test into an invariant rather than a live branch that throws on the render thread. Each rejection is named in the log. The registration stays so an existing file still parses; it is the live object that is refused. A selection and a clone record naming a rejected repository are cleared with it: the panels reach for a selection through Options.Repos[Options.BaseRepo] once something is selected, so leaving one behind would turn one crash into another. An empty selection is the state a fresh install starts in, which the surrounding TryGetValue checks already handle. Fixes #427 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- ProjectDirector.Test/UnsupportedRepoTests.cs | 186 +++++++++++++++++++ ProjectDirector/ProjectDirector.cs | 85 +++++++++ 2 files changed, 271 insertions(+) create mode 100644 ProjectDirector.Test/UnsupportedRepoTests.cs diff --git a/ProjectDirector.Test/UnsupportedRepoTests.cs b/ProjectDirector.Test/UnsupportedRepoTests.cs new file mode 100644 index 0000000..79d736f --- /dev/null +++ b/ProjectDirector.Test/UnsupportedRepoTests.cs @@ -0,0 +1,186 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.ProjectDirector.Test; + +using System.Collections.Generic; +using System.Linq; +using System.Text.Json; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +/// +/// Tests the rule that refuses a saved repository this application cannot act on. +/// +/// +/// registers 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 +/// and throws otherwise. UpdateClonedStatus runs from the +/// constructor's RefreshPage, 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. +/// +/// exists so the one boundary such an entry can +/// arrive through can be driven without a live ImGui context, the way +/// 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 . +/// +[TestClass] +public sealed class UnsupportedRepoTests +{ + private static FullyQualifiedGitHubRepoName RepoName(string value) => FullyQualifiedGitHubRepoName.Create(value); + private static FullyQualifiedLocalRepoPath LocalPath(string value) => FullyQualifiedLocalRepoPath.Create(value); + + /// + /// The premise: an Azure DevOps repository really does come back from JSON as a live object, + /// rather than being rejected by the serializer. + /// + /// + /// 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. + /// + [TestMethod] + public void AnAzureDevOpsRepositoryDeserializesIntoALiveObject() + { + GitRepository? fromSavedOptions = JsonSerializer.Deserialize("""{"TypeName":"AzureDevOpsRepository"}"""); + + Assert.IsInstanceOfType(fromSavedOptions, + "If this stops holding, the crash this rule guards against is no longer reachable and the rule can go."); + } + + /// + /// The regression this file exists for. + /// + [TestMethod] + public void AnUnsupportedRepoIsDroppedAndTheSupportedOnesAreKept() + { + Dictionary repos = new() + { + [RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(), + [RepoName("contoso.internal")] = new AzureDevOpsRepository(), + }; + Dictionary clonedRepos = []; + + IReadOnlyList 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 repos = new() + { + [RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(), + [RepoName("ktsu-dev.ImGuiApp")] = new GitHubRepository(), + }; + Dictionary clonedRepos = new() + { + [LocalPath("/dev/ProjectDirector")] = RepoName("ktsu-dev.ProjectDirector"), + }; + + IReadOnlyList 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 repos = new() + { + [RepoName("contoso.internal")] = new AzureDevOpsRepository(), + [RepoName("ktsu-dev.ProjectDirector")] = new GitHubRepository(), + }; + Dictionary 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 repos = new() + { + [RepoName("contoso.internal")] = new AzureDevOpsRepository(), + [RepoName("contoso.other")] = new AzureDevOpsRepository(), + }; + Dictionary clonedRepos = new() + { + [LocalPath("/dev/internal")] = RepoName("contoso.internal"), + }; + + IReadOnlyList rejected = ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos); + + Assert.AreEqual(2, rejected.Count); + Assert.AreEqual(0, repos.Count); + Assert.AreEqual(0, clonedRepos.Count); + } + + [TestMethod] + public void EmptyOptionsAreHandled() + { + Dictionary repos = []; + Dictionary clonedRepos = []; + + Assert.AreEqual(0, ProjectDirector.RejectUnsupportedRepos(repos, clonedRepos).Count); + Assert.AreEqual(0, repos.Count); + } + + /// + /// A selection left pointing at a dropped repository would send the panels straight back into + /// Options.Repos[Options.BaseRepo], turning one crash into another. + /// + [TestMethod] + public void ASelectionPointingAtADroppedRepoIsCleared() + { + IReadOnlyList 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 rejected = [RepoName("contoso.internal")]; + + Assert.AreEqual( + RepoName("ktsu-dev.ProjectDirector"), + ProjectDirector.ClearSelectionIfRejected(RepoName("ktsu-dev.ProjectDirector"), rejected)); + } + + [TestMethod] + public void AnEmptySelectionSurvivesAnEmptyRejectionList() + { + IReadOnlyList rejected = []; + + Assert.AreEqual( + new FullyQualifiedGitHubRepoName(), + ProjectDirector.ClearSelectionIfRejected(new FullyQualifiedGitHubRepoName(), rejected), + "A fresh install selects nothing, and that must not be mistaken for a rejection."); + } +} diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 79d2af9..6a97621 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -62,6 +62,19 @@ private static void Main(string[] _) public ProjectDirector() { Options = ProjectDirectorOptions.LoadOrCreate(); + + // Deserialization is the only way a repository this application cannot act on enters the + // options, so rejecting one here is what keeps every later `is GitHubRepository` test an + // invariant rather than a live branch that throws on the render thread. + IReadOnlyList rejectedRepos = RejectUnsupportedRepos(Options.Repos, Options.ClonedRepos); + Options.BaseRepo = ClearSelectionIfRejected(Options.BaseRepo, rejectedRepos); + Options.CompareRepo = ClearSelectionIfRejected(Options.CompareRepo, rejectedRepos); + + foreach (FullyQualifiedGitHubRepoName rejected in rejectedRepos) + { + QueueLog($"Ignoring saved repository '{rejected}': only GitHub repositories are supported at this time."); + } + Options.Save(); // ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken)); DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns); @@ -98,6 +111,78 @@ public ProjectDirector() RefreshPage(); } + /// + /// Drops any saved repository this application cannot act on, along with any selection left + /// pointing at one. + /// + /// The freshly loaded repositories, modified in place. + /// The freshly loaded clone records, modified in place. + /// The names of the repositories that were dropped, in the order they were found. + /// + /// registers as a + /// , so a saved options + /// file carrying one deserializes without complaint. Nothing acts on it: every site that + /// pattern-matches a repository handles and throws otherwise, + /// and cannot produce anything else in the first place. + /// UpdateClonedStatus then runs from the constructor's RefreshPage, 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 so this + /// rule can be driven without constructing one. + /// + internal static IReadOnlyList RejectUnsupportedRepos( + IDictionary repos, + IDictionary clonedRepos) + { + Ensure.NotNull(repos); + Ensure.NotNull(clonedRepos); + + List 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; + } + + /// + /// Clears a selected repository name that names one of the + /// repositories. + /// + /// The saved selection. + /// The repositories that were dropped. + /// The selection, or an empty name where it named a dropped repository. + /// + /// Once something is selected the panels reach for it through + /// Options.Repos[Options.BaseRepo], 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 TryGetValue checks already handle. + /// + internal static FullyQualifiedGitHubRepoName ClearSelectionIfRejected( + FullyQualifiedGitHubRepoName selection, + IReadOnlyList rejected) + { + Ensure.NotNull(rejected); + + return rejected.Contains(selection) ? new() : selection; + } + private void QueueLog(string logMessage) { LogQueue.Enqueue(logMessage); From de3ddbff8174998894004c2c7052c43e0c740187 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:44:49 +0000 Subject: [PATCH 2/2] fix: make the dev directory default work off Windows, and cover the load path [patch] The post-load block in the constructor was five uncovered lines, because nothing could drive it: ProjectDirectorOptions could not be constructed anywhere but Windows. Its DevDirectory default was a hardcoded C:\dev, which is not an absolute path off Windows, so AbsoluteDirectoryPath rejected it in the field initializer and the constructor threw. That is a real defect in its own right -- the application cannot start on Linux or macOS, both of which this repository runs its tests on -- and it is what blocked testing the fix in this PR. Give the default a platform-aware fallback: Windows keeps the path it has always had, everywhere else gets ~/dev. Only a fresh install reads it, so a saved options file keeps whatever the user chose. With options constructible, collapse the constructor block into MakeLoadedOptionsSafe and drive the whole of it against real options: the unsupported entry is dropped, a selection naming it is cleared, a selection naming a survivor is left alone, the clone record goes, and the rejection is reported once. Coverage on new code goes from 75% to 96%, which is what the SonarCloud gate wanted, but the point is that the path the constructor actually takes is now tested rather than merely the helper underneath it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- ProjectDirector.Test/UnsupportedRepoTests.cs | 68 +++++++++++++++++++- ProjectDirector/ProjectDirector.cs | 40 ++++++++---- ProjectDirector/ProjectDirectorOptions.cs | 17 ++++- 3 files changed, 110 insertions(+), 15 deletions(-) diff --git a/ProjectDirector.Test/UnsupportedRepoTests.cs b/ProjectDirector.Test/UnsupportedRepoTests.cs index 79d736f..64d8db7 100644 --- a/ProjectDirector.Test/UnsupportedRepoTests.cs +++ b/ProjectDirector.Test/UnsupportedRepoTests.cs @@ -2,6 +2,7 @@ namespace ktsu.ProjectDirector.Test; +using System; using System.Collections.Generic; using System.Linq; using System.Text.Json; @@ -19,9 +20,9 @@ namespace ktsu.ProjectDirector.Test; /// constructor's RefreshPage, 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. /// -/// exists so the one boundary such an entry can -/// arrive through can be driven without a live ImGui context, the way -/// drives the pull rule. What it guarantees is the invariant those six throw sites rest on: after it +/// RejectUnsupportedRepos and MakeLoadedOptionsSafe exist so the one boundary such an +/// entry can arrive through can be driven without a live ImGui context, the way +/// 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 . /// [TestClass] @@ -138,6 +139,67 @@ public void EveryRepoBeingUnsupportedLeavesAnEmptyButUsableState() Assert.AreEqual(0, clonedRepos.Count); } + /// + /// 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. + /// + [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 logged = []; + IReadOnlyList 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 logged = []; + IReadOnlyList 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."); + } + + /// + /// Fresh options have to be constructible on every platform this repository tests on, which is + /// what a hardcoded C:\dev default prevented. + /// + [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() { diff --git a/ProjectDirector/ProjectDirector.cs b/ProjectDirector/ProjectDirector.cs index 6a97621..c112877 100644 --- a/ProjectDirector/ProjectDirector.cs +++ b/ProjectDirector/ProjectDirector.cs @@ -63,17 +63,7 @@ public ProjectDirector() { Options = ProjectDirectorOptions.LoadOrCreate(); - // Deserialization is the only way a repository this application cannot act on enters the - // options, so rejecting one here is what keeps every later `is GitHubRepository` test an - // invariant rather than a live branch that throws on the render thread. - IReadOnlyList rejectedRepos = RejectUnsupportedRepos(Options.Repos, Options.ClonedRepos); - Options.BaseRepo = ClearSelectionIfRejected(Options.BaseRepo, rejectedRepos); - Options.CompareRepo = ClearSelectionIfRejected(Options.CompareRepo, rejectedRepos); - - foreach (FullyQualifiedGitHubRepoName rejected in rejectedRepos) - { - QueueLog($"Ignoring saved repository '{rejected}': only GitHub repositories are supported at this time."); - } + _ = MakeLoadedOptionsSafe(Options, QueueLog); Options.Save(); // ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken)); @@ -183,6 +173,34 @@ internal static FullyQualifiedGitHubRepoName ClearSelectionIfRejected( return rejected.Contains(selection) ? new() : selection; } + /// + /// Rejects every saved repository this application cannot act on, clears anything left pointing + /// at one, and reports each rejection. + /// + /// The freshly loaded options, modified in place. + /// Where to report each rejected repository. + /// The names of the repositories that were dropped. + /// + /// The whole of what the constructor does after loading, in one place, so it can be driven + /// without an ImGui context. + /// + internal static IReadOnlyList MakeLoadedOptionsSafe(ProjectDirectorOptions options, Action log) + { + Ensure.NotNull(options); + Ensure.NotNull(log); + + IReadOnlyList 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); diff --git a/ProjectDirector/ProjectDirectorOptions.cs b/ProjectDirector/ProjectDirectorOptions.cs index b624423..7a11160 100644 --- a/ProjectDirector/ProjectDirectorOptions.cs +++ b/ProjectDirector/ProjectDirectorOptions.cs @@ -18,7 +18,22 @@ public sealed record class FullyQualifiedLocalRepoPath : SemanticString { - public AbsoluteDirectoryPath DevDirectory { get; set; } = AbsoluteDirectoryPath.Create(@"C:\dev"); + public AbsoluteDirectoryPath DevDirectory { get; set; } = DefaultDevDirectory(); + + /// + /// The dev directory a fresh install starts with. + /// + /// + /// C:\dev is not an absolute path anywhere but Windows, so + /// 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 ~/dev. Only a fresh install reads this, so a + /// saved options file keeps whatever the user chose. + /// + private static AbsoluteDirectoryPath DefaultDevDirectory() => + AbsoluteDirectoryPath.Create(OperatingSystem.IsWindows() + ? @"C:\dev" + : Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.UserProfile), "dev")); public ImGuiAppWindowState WindowState { get; set; } = new(); public GitHubLogin GitHubLogin { get; set; } = new();