From 5243e164dc15d584cbdb4f5186b7c7a29c2b8210 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 27 Aug 2026 16:34:47 +0100 Subject: [PATCH 1/6] diagnose: allow creation of empty diagnose command Allow the creation of the diagnose command without the standard diagnostics. This will enable us to add tests in the future that exercise the diagnostic infrastructure without always adding the standard diagnostics. The main application adds them when it creates the command. Signed-off-by: Matthew John Cheetham --- src/Core/Application.cs | 1 + src/Core/Commands/DiagnoseCommand.cs | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/Core/Application.cs b/src/Core/Application.cs index 7099cf4b0c..5a185ccddd 100644 --- a/src/Core/Application.cs +++ b/src/Core/Application.cs @@ -70,6 +70,7 @@ protected override async Task RunInternalAsync(string[] args) { var rootCommand = new RootCommand(); var diagnoseCommand = new DiagnoseCommand(Context); + diagnoseCommand.AddStandardDiagnostics(); // Add common options var noGuiOption = new Option("--no-ui", "Do not use graphical user interface prompts"); diff --git a/src/Core/Commands/DiagnoseCommand.cs b/src/Core/Commands/DiagnoseCommand.cs index 3603d073b1..550eb28234 100644 --- a/src/Core/Commands/DiagnoseCommand.cs +++ b/src/Core/Commands/DiagnoseCommand.cs @@ -13,7 +13,7 @@ public class DiagnoseCommand : Command private const string TestOutputIndent = " "; private readonly ICommandContext _context; - private readonly ICollection _diagnostics; + private readonly List _diagnostics = new(); public DiagnoseCommand(ICommandContext context) : base("diagnose", "Run diagnostics and gather logs to diagnose problems with Git Credential Manager") @@ -21,16 +21,6 @@ public DiagnoseCommand(ICommandContext context) EnsureArgument.NotNull(context, nameof(context)); _context = context; - _diagnostics = new List - { - // Add standard diagnostics - new EnvironmentDiagnostic(context), - new FileSystemDiagnostic(context), - new NetworkingDiagnostic(context), - new GitDiagnostic(context), - new CredentialStoreDiagnostic(context), - new EntraAuthenticationDiagnostic(context) - }; var output = new Option(new[] { "--output", "-o" }, "Output directory for diagnostic logs."); AddOption(output); @@ -38,11 +28,26 @@ public DiagnoseCommand(ICommandContext context) this.SetHandler(ExecuteAsync, output); } + public void AddStandardDiagnostics() + { + _diagnostics.Add(new EnvironmentDiagnostic(_context)); + _diagnostics.Add(new FileSystemDiagnostic(_context)); + _diagnostics.Add(new NetworkingDiagnostic(_context)); + _diagnostics.Add(new GitDiagnostic(_context)); + _diagnostics.Add(new CredentialStoreDiagnostic(_context)); + _diagnostics.Add(new EntraAuthenticationDiagnostic(_context)); + } + public void AddDiagnostic(IDiagnostic diagnostic) { _diagnostics.Add(diagnostic); } + public void AddDiagnostics(IEnumerable diagnostics) + { + _diagnostics.AddRange(diagnostics); + } + private async Task ExecuteAsync(string output) { // Don't use IStandardStreams for writing output in this command as we From 8a184e42a1ddbaeb9469ad35dd26ffe6a8ed11b7 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 27 Aug 2026 17:13:16 +0100 Subject: [PATCH 2/6] diagnose: move to file-scoped namespace Signed-off-by: Matthew John Cheetham --- src/Core/Diagnostics/Diagnostic.cs | 93 +++++++++++++++--------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/src/Core/Diagnostics/Diagnostic.cs b/src/Core/Diagnostics/Diagnostic.cs index 45813bd1ec..6a4ec3d97e 100644 --- a/src/Core/Diagnostics/Diagnostic.cs +++ b/src/Core/Diagnostics/Diagnostic.cs @@ -3,67 +3,66 @@ using System.Text; using System.Threading.Tasks; -namespace GitCredentialManager.Diagnostics +namespace GitCredentialManager.Diagnostics; + +public interface IDiagnostic +{ + string Name { get; } + + bool CanRun(); + + Task RunAsync(); +} + +public abstract class Diagnostic : IDiagnostic { - public interface IDiagnostic + protected ICommandContext CommandContext; + + protected Diagnostic(string name, ICommandContext commandContext) { - string Name { get; } + Name = name; + CommandContext = commandContext; + } - bool CanRun(); + public string Name { get; } - Task RunAsync(); + public virtual bool CanRun() + { + return true; } - public abstract class Diagnostic : IDiagnostic + public async Task RunAsync() { - protected ICommandContext CommandContext; + var log = new StringBuilder(); - protected Diagnostic(string name, ICommandContext commandContext) + bool success = false; + Exception exception = null; + var additionalFiles = new List(); + try { - Name = name; - CommandContext = commandContext; + success = await RunInternalAsync(log, additionalFiles); } - - public string Name { get; } - - public virtual bool CanRun() + catch (Exception ex) { - return true; + exception = ex; } - public async Task RunAsync() + return new DiagnosticResult { - var log = new StringBuilder(); - - bool success = false; - Exception exception = null; - var additionalFiles = new List(); - try - { - success = await RunInternalAsync(log, additionalFiles); - } - catch (Exception ex) - { - exception = ex; - } - - return new DiagnosticResult - { - IsSuccess = success, - DiagnosticLog = log.ToString(), - Exception = exception, - AdditionalFiles = additionalFiles - }; - } - - protected abstract Task RunInternalAsync(StringBuilder log, IList additionalFiles); + IsSuccess = success, + DiagnosticLog = log.ToString(), + Exception = exception, + AdditionalFiles = additionalFiles + }; } - public class DiagnosticResult - { - public bool IsSuccess { get; set; } - public Exception Exception { get; set; } - public string DiagnosticLog { get; set; } - public ICollection AdditionalFiles { get; set; } - } + protected abstract Task RunInternalAsync(StringBuilder log, IList additionalFiles); +} + +public class DiagnosticResult +{ + public bool IsSuccess { get; set; } + public Exception Exception { get; set; } + public string DiagnosticLog { get; set; } + public ICollection AdditionalFiles { get; set; } } From 88cf26474bd21693a4621c09abd2b0fc5c4133bf Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 27 Aug 2026 17:59:40 +0100 Subject: [PATCH 3/6] diagnose: report structured outcomes Diagnostics currently collapse each run into a Boolean and an unstructured string log. That makes warnings indistinguishable from useful progress and forces tests to assert against presentation text. Introduce typed reports and derive success, warning, error, and skipped outcomes from them. Migrate the built-in diagnostics and teach the command to present warnings without turning them into failures. The reporter also retains associated exceptions and additional files for the consolidated log. Cover command exit behavior and networking reports at the typed reporter boundary. Assisted-by: GPT-5.6 Sol Fast (Internal only) Signed-off-by: Matthew John Cheetham --- .../Commands/DiagnoseCommandTests.cs | 108 +++++++++++-- src/Core/Commands/DiagnoseCommand.cs | 71 ++++++--- .../Diagnostics/CredentialStoreDiagnostic.cs | 45 +++--- src/Core/Diagnostics/Diagnostic.cs | 150 ++++++++++++++---- .../EntraAuthenticationDiagnostic.cs | 42 ++--- src/Core/Diagnostics/EnvironmentDiagnostic.cs | 19 +-- src/Core/Diagnostics/FileSystemDiagnostic.cs | 35 ++-- src/Core/Diagnostics/GitDiagnostic.cs | 35 ++-- src/Core/Diagnostics/NetworkingDiagnostic.cs | 58 +++---- src/GitHub/Diagnostics/GitHubApiDiagnostic.cs | 12 +- .../Objects/TestDiagnosticReporter.cs | 39 +++++ 11 files changed, 402 insertions(+), 212 deletions(-) create mode 100644 src/TestInfrastructure/Objects/TestDiagnosticReporter.cs diff --git a/src/Core.Tests/Commands/DiagnoseCommandTests.cs b/src/Core.Tests/Commands/DiagnoseCommandTests.cs index 42f5cedc7b..b3a078ba80 100644 --- a/src/Core.Tests/Commands/DiagnoseCommandTests.cs +++ b/src/Core.Tests/Commands/DiagnoseCommandTests.cs @@ -1,34 +1,109 @@ using System; +using System.CommandLine; using System.Net.Http; using System.Security.AccessControl; using System.Text; using System.Threading.Tasks; +using GitCredentialManager.Commands; using GitCredentialManager.Diagnostics; using GitCredentialManager.Tests.Objects; +using Moq; using Xunit; namespace Core.Tests.Commands; public class DiagnoseCommandTests { + [Fact] + public async Task DiagnoseCommand_AllSuccessful_ReturnsZero() + { + string skip = It.IsAny(); + var diagnosticMock = new Mock(MockBehavior.Strict); + diagnosticMock.SetupGet(x => x.Name).Returns("TestDiagnostic"); + diagnosticMock.Setup(x => x.CanRun(out skip)).Returns(true); + diagnosticMock.Setup(x => x.RunAsync(It.IsAny>())) + .ReturnsAsync(new DiagnosticResult([], [])); + + var context = new TestCommandContext(); + var command = new DiagnoseCommand(context); + command.AddDiagnostic(diagnosticMock.Object); + + int result = await command.InvokeAsync([]); + + Assert.Equal(0, result); + diagnosticMock.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); + } + + [Fact] + public async Task DiagnoseCommand_AtLeastOneFailure_ReturnsNonZero() + { + string skip = It.IsAny(); + var diagnosticMock1 = new Mock(MockBehavior.Strict); + diagnosticMock1.SetupGet(x => x.Name).Returns("TestDiagnostic1"); + diagnosticMock1.Setup(x => x.CanRun(out skip)).Returns(true); + diagnosticMock1.Setup(x => x.RunAsync(It.IsAny>())) + .ReturnsAsync(new DiagnosticResult([ + new DiagnosticReport(DiagnosticReportKind.Error, "Failure") + ], [])); + + var diagnosticMock2 = new Mock(MockBehavior.Strict); + diagnosticMock2.SetupGet(x => x.Name).Returns("TestDiagnostic2"); + diagnosticMock2.Setup(x => x.CanRun(out skip)).Returns(true); + diagnosticMock2.Setup(x => x.RunAsync(It.IsAny>())) + .ReturnsAsync(new DiagnosticResult([], [])); + + var context = new TestCommandContext(); + var command = new DiagnoseCommand(context); + command.AddDiagnostic(diagnosticMock1.Object); + command.AddDiagnostic(diagnosticMock2.Object); + + int result = await command.InvokeAsync([]); + + Assert.NotEqual(0, result); + diagnosticMock1.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); + diagnosticMock2.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); + } + + [Fact] + public async Task DiagnoseCommand_Warnings_ReturnsZero() + { + string skip = It.IsAny(); + var diagnosticMock = new Mock(MockBehavior.Strict); + diagnosticMock.SetupGet(x => x.Name).Returns("TestDiagnostic"); + diagnosticMock.Setup(x => x.CanRun(out skip)).Returns(true); + diagnosticMock.Setup(x => x.RunAsync(It.IsAny>())) + .ReturnsAsync(new DiagnosticResult([ + new DiagnosticReport(DiagnosticReportKind.Warning, "Caution") + ], [])); + + var context = new TestCommandContext(); + var command = new DiagnoseCommand(context); + command.AddDiagnostic(diagnosticMock.Object); + + int result = await command.InvokeAsync([]); + + Assert.Equal(0, result); + diagnosticMock.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); + } + [Fact] public async Task NetworkingDiagnostic_SendHttpRequest_Primary_OK() { var primaryUriString = "http://example.com"; - var sb = new StringBuilder(); + var reporter = new TestDiagnosticReporter(); var context = new TestCommandContext(); var networkingDiagnostic = new NetworkingDiagnostic(context); var primaryUri = new Uri(primaryUriString); var httpHandler = new TestHttpMessageHandler(); var httpResponse = new HttpResponseMessage(); - var expected = $"Sending HEAD request to {primaryUriString}... OK{Environment.NewLine}"; httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); - await networkingDiagnostic.SendHttpRequestAsync(sb, new HttpClient(httpHandler)); + await networkingDiagnostic.SendHttpRequestAsync(reporter, new HttpClient(httpHandler)); httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); - Assert.Contains(expected, sb.ToString()); + Assert.Single(reporter.Progress); + Assert.Equal($"Sending HEAD request to {primaryUriString}", reporter.Progress[0]); } [Fact] @@ -36,24 +111,26 @@ public async Task NetworkingDiagnostic_SendHttpRequest_Backup_OK() { var primaryUriString = "http://example.com"; var backupUriString = "http://httpforever.com"; - var sb = new StringBuilder(); + var reporter = new TestDiagnosticReporter(); var context = new TestCommandContext(); var networkingDiagnostic = new NetworkingDiagnostic(context); var primaryUri = new Uri(primaryUriString); var backupUri = new Uri(backupUriString); var httpHandler = new TestHttpMessageHandler { SimulatePrimaryUriFailure = true }; var httpResponse = new HttpResponseMessage(); - var expected = $"Sending HEAD request to {primaryUriString}... warning: HEAD request failed{Environment.NewLine}" + - $"Sending HEAD request to {backupUriString}... OK{Environment.NewLine}"; httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); httpHandler.Setup(HttpMethod.Head, backupUri, httpResponse); - await networkingDiagnostic.SendHttpRequestAsync(sb, new HttpClient(httpHandler)); + await networkingDiagnostic.SendHttpRequestAsync(reporter, new HttpClient(httpHandler)); httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); httpHandler.AssertRequest(HttpMethod.Head, backupUri, expectedNumberOfCalls: 1); - Assert.Contains(expected, sb.ToString()); + Assert.Equal(2, reporter.Progress.Count); + Assert.Single(reporter.Warnings); + Assert.Equal($"Sending HEAD request to {primaryUriString}", reporter.Progress[0]); + Assert.Equal("HEAD request failed", reporter.Warnings[0]); + Assert.Equal($"Sending HEAD request to {backupUriString}", reporter.Progress[1]); } [Fact] @@ -61,23 +138,26 @@ public async Task NetworkingDiagnostic_SendHttpRequest_No_Network() { var primaryUriString = "http://example.com"; var backupUriString = "http://httpforever.com"; - var sb = new StringBuilder(); + var reporter = new TestDiagnosticReporter(); var context = new TestCommandContext(); var networkingDiagnostic = new NetworkingDiagnostic(context); var primaryUri = new Uri(primaryUriString); var backupUri = new Uri(backupUriString); var httpHandler = new TestHttpMessageHandler { SimulateNoNetwork = true }; var httpResponse = new HttpResponseMessage(); - var expected = $"Sending HEAD request to {primaryUriString}... warning: HEAD request failed{Environment.NewLine}" + - $"Sending HEAD request to {backupUriString}... warning: HEAD request failed{Environment.NewLine}"; httpHandler.Setup(HttpMethod.Head, primaryUri, httpResponse); httpHandler.Setup(HttpMethod.Head, backupUri, httpResponse); - await networkingDiagnostic.SendHttpRequestAsync(sb, new HttpClient(httpHandler)); + await networkingDiagnostic.SendHttpRequestAsync(reporter, new HttpClient(httpHandler)); httpHandler.AssertRequest(HttpMethod.Head, primaryUri, expectedNumberOfCalls: 1); httpHandler.AssertRequest(HttpMethod.Head, backupUri, expectedNumberOfCalls: 1); - Assert.Contains(expected, sb.ToString()); + Assert.Equal(2, reporter.Progress.Count); + Assert.Equal(2, reporter.Warnings.Count); + Assert.Equal($"Sending HEAD request to {primaryUriString}", reporter.Progress[0]); + Assert.Equal("HEAD request failed", reporter.Warnings[0]); + Assert.Equal($"Sending HEAD request to {backupUriString}", reporter.Progress[1]); + Assert.Equal("HEAD request failed", reporter.Warnings[1]); } } diff --git a/src/Core/Commands/DiagnoseCommand.cs b/src/Core/Commands/DiagnoseCommand.cs index 550eb28234..31b5c449e3 100644 --- a/src/Core/Commands/DiagnoseCommand.cs +++ b/src/Core/Commands/DiagnoseCommand.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.CommandLine; using System.IO; +using System.Linq; using System.Text; using System.Threading.Tasks; using GitCredentialManager.Diagnostics; @@ -62,6 +63,7 @@ private async Task ExecuteAsync(string output) int numFailed = 0; int numSkipped = 0; + int numWarned = 0; string currentDir = Directory.GetCurrentDirectory(); string outputDir; @@ -99,13 +101,17 @@ private async Task ExecuteAsync(string output) fullLog.WriteLine("------------"); fullLog.WriteLine($"Diagnostic: {diagnostic.Name}"); - if (!diagnostic.CanRun()) + if (!diagnostic.CanRun(out string skipReason)) { - fullLog.WriteLine("Skipped: True"); + fullLog.Write("Outcome: Skipped"); + if (!string.IsNullOrWhiteSpace(skipReason)) + { + fullLog.Write($" ({skipReason})"); + } fullLog.WriteLine(); Console.Write(" "); - ConsoleEx.WriteColor("[SKIP]", ConsoleColor.Gray); + ConsoleEx.WriteColor("[SKIP]", ConsoleColor.DarkGray); Console.WriteLine(" {0}", diagnostic.Name); numSkipped++; @@ -115,34 +121,50 @@ private async Task ExecuteAsync(string output) string inProgressMsg = $" >>>> {diagnostic.Name}"; Console.Write(inProgressMsg); - fullLog.WriteLine("Skipped: False"); DiagnosticResult result = await diagnostic.RunAsync(); - fullLog.WriteLine("Success: {0}", result.IsSuccess); + fullLog.WriteLine("Outcome: {0}", result.Outcome); - if (result.Exception is null) + if (result.Exception is AggregateException aex) { - fullLog.WriteLine("Exception: None"); + fullLog.WriteLine("Exception: AggregateException"); + fullLog.WriteLine("InnerExceptions (flattened):"); + foreach (var inner in aex.Flatten().InnerExceptions) + { + fullLog.WriteLine(inner.ToString()); + } } - else + else if (result.Exception is not null) { - fullLog.WriteLine("Exception:"); - fullLog.WriteLine(result.Exception.ToString()); + fullLog.WriteLine("Exception: {0}", result.Exception); } fullLog.WriteLine("Log:"); - fullLog.WriteLine(result.DiagnosticLog); + foreach (var report in result.Reports) + { + fullLog.WriteLine(report.Message); + } Console.Write(new string('\b', inProgressMsg.Length - 1)); - ConsoleEx.WriteColor( - result.IsSuccess ? "[ OK ]" : "[FAIL]", - result.IsSuccess ? ConsoleColor.DarkGreen : ConsoleColor.Red - ); + switch (result.Outcome) + { + case DiagnosticOutcome.Success: + ConsoleEx.WriteColor("[ OK ]", ConsoleColor.DarkGreen); + break; + case DiagnosticOutcome.Warning: + ConsoleEx.WriteColor("[WARN]", ConsoleColor.DarkYellow); + numWarned++; + break; + case DiagnosticOutcome.Error: + ConsoleEx.WriteColor("[FAIL]", ConsoleColor.Red); + numFailed++; + break; + default: + throw new ArgumentOutOfRangeException(); + } Console.WriteLine(" {0}", diagnostic.Name); - if (!result.IsSuccess) + if (result.Outcome != DiagnosticOutcome.Success) { - numFailed++; - if (result.Exception is not null) { Console.WriteLine(); @@ -152,7 +174,7 @@ private async Task ExecuteAsync(string output) Console.WriteLine(); ConsoleEx.WriteLineIndent("[*] Diagnostic test log [*]"); - ConsoleEx.WriteLineIndent(result.DiagnosticLog); + ConsoleEx.WriteLineIndent(result.Reports.Select(x => x.Message)); Console.WriteLine(); } @@ -177,7 +199,8 @@ private async Task ExecuteAsync(string output) } Console.WriteLine(); - string summary = $"Diagnostic summary: {_diagnostics.Count - numFailed} passed, {numSkipped} skipped, {numFailed} failed."; + int numPassed = _diagnostics.Count - numFailed - numSkipped - numWarned; + string summary = $"Diagnostic summary: {numPassed} passed, {numSkipped} skipped, {numWarned} warned, {numFailed} failed."; Console.WriteLine(summary); Console.WriteLine("Log files:"); Console.WriteLine($" {logFilePath}"); @@ -189,7 +212,7 @@ private async Task ExecuteAsync(string output) Console.WriteLine("Caution: Log files may include sensitive information - redact before sharing."); Console.WriteLine(); - if (numFailed > 0) + if (numFailed + numWarned > 0) { Console.WriteLine("Diagnostics indicate a possible problem with your installation."); Console.WriteLine($"Please open an issue at {Constants.HelpUrls.GcmNewIssue} and include log files."); @@ -197,7 +220,7 @@ private async Task ExecuteAsync(string output) } fullLog.Close(); - return numFailed; + return numFailed > 0 ? 1 : 0; } private static class ConsoleEx @@ -205,7 +228,11 @@ private static class ConsoleEx public static void WriteLineIndent(string str) { string[] lines = str?.Split('\n', '\r'); + WriteLineIndent(lines); + } + public static void WriteLineIndent(IEnumerable lines) + { if (lines is null) return; foreach (string line in lines) diff --git a/src/Core/Diagnostics/CredentialStoreDiagnostic.cs b/src/Core/Diagnostics/CredentialStoreDiagnostic.cs index c5c8fac701..7d4a188f54 100644 --- a/src/Core/Diagnostics/CredentialStoreDiagnostic.cs +++ b/src/Core/Diagnostics/CredentialStoreDiagnostic.cs @@ -1,6 +1,4 @@ using System; -using System.Collections.Generic; -using System.Text; using System.Threading.Tasks; namespace GitCredentialManager.Diagnostics @@ -11,9 +9,9 @@ public CredentialStoreDiagnostic(ICommandContext commandContext) : base("Credential storage", commandContext) { } - protected override Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override Task RunInternalAsync(IDiagnosticReporter reporter) { - log.AppendLine($"Credential store is: {CommandContext.CredentialStore.Name}"); + reporter.ReportInfo($"Credential store is: {Context.CredentialStore.Name}"); // Create a service that is guaranteed to be unique string service = $"https://example.com/{Guid.NewGuid():N}"; @@ -22,45 +20,40 @@ protected override Task RunInternalAsync(StringBuilder log, IList try { - log.Append("Writing test credential..."); - CommandContext.CredentialStore.AddOrUpdate(service, account, password); - log.AppendLine(" OK"); + reporter.ReportProgress("Writing test credential..."); + Context.CredentialStore.AddOrUpdate(service, account, password); - log.Append("Reading test credential..."); - ICredential outCredential = CommandContext.CredentialStore.Get(service, account); + reporter.ReportProgress("Reading test credential..."); + ICredential outCredential = Context.CredentialStore.Get(service, account); if (outCredential is null) { - log.AppendLine(" Failed"); - log.AppendLine("Test credential object is null!"); - return Task.FromResult(false); + reporter.ReportError("Test credential object is null!"); + return Task.CompletedTask; } - log.AppendLine(" OK"); - if (!StringComparer.Ordinal.Equals(account, outCredential.Account)) { - log.Append("Test credential account did not match!"); - log.AppendLine($"Expected: {account}"); - log.AppendLine($"Actual: {outCredential.Account}"); - return Task.FromResult(false); + reporter.ReportError("Test credential account did not match!"); + reporter.ReportError($"Expected: {account}"); + reporter.ReportError($"Actual: {outCredential.Account}"); + return Task.CompletedTask; } if (!StringComparer.Ordinal.Equals(password, outCredential.Password)) { - log.Append("Test credential password did not match!"); - log.AppendLine($"Expected: {password}"); - log.AppendLine($"Actual: {outCredential.Password}"); - return Task.FromResult(false); + reporter.ReportError("Test credential password did not match!"); + reporter.ReportError($"Expected: {password}"); + reporter.ReportError($"Actual: {outCredential.Password}"); + return Task.CompletedTask; } } finally { - log.Append("Deleting test credential..."); - CommandContext.CredentialStore.Remove(service, account); - log.AppendLine(" OK"); + reporter.ReportProgress("Deleting test credential"); + Context.CredentialStore.Remove(service, account); } - return Task.FromResult(true); + return Task.CompletedTask; } } } diff --git a/src/Core/Diagnostics/Diagnostic.cs b/src/Core/Diagnostics/Diagnostic.cs index 6a4ec3d97e..bb864218a6 100644 --- a/src/Core/Diagnostics/Diagnostic.cs +++ b/src/Core/Diagnostics/Diagnostic.cs @@ -1,6 +1,6 @@ using System; using System.Collections.Generic; -using System.Text; +using System.Linq; using System.Threading.Tasks; namespace GitCredentialManager.Diagnostics; @@ -9,60 +9,148 @@ public interface IDiagnostic { string Name { get; } - bool CanRun(); + bool CanRun(out string reason); - Task RunAsync(); + Task RunAsync(Action progress = null); } -public abstract class Diagnostic : IDiagnostic +public enum DiagnosticOutcome { - protected ICommandContext CommandContext; + Success, + Warning, + Error, + Skipped +} - protected Diagnostic(string name, ICommandContext commandContext) - { - Name = name; - CommandContext = commandContext; - } +public enum DiagnosticReportKind +{ + Progress, + Information, + Warning, + Error +} + +public sealed class DiagnosticReport(DiagnosticReportKind kind, string message, Exception exception = null) +{ + public DiagnosticReportKind Kind { get; } = kind; + public string Message { get; } = message; + public Exception Exception { get; } = exception; + + public static DiagnosticReport Info(string message) => new(DiagnosticReportKind.Information, message); + public static DiagnosticReport Warning(string message) => new(DiagnosticReportKind.Warning, message); + public static DiagnosticReport Error(string message, Exception exception = null) => new(DiagnosticReportKind.Error, message, exception); + public static DiagnosticReport Progress(string message) => new(DiagnosticReportKind.Progress, message); +} - public string Name { get; } +public interface IDiagnosticReporter +{ + void ReportProgress(string message); + void ReportInfo(string message); + void ReportWarning(string message); + void ReportError(string message, Exception exception = null); + void AddFile(string path); +} + +public abstract class Diagnostic(string name, ICommandContext context) : IDiagnostic +{ + protected readonly ICommandContext Context = context; + + public string Name { get; } = name; - public virtual bool CanRun() + public virtual bool CanRun(out string reason) { + reason = null; return true; } - public async Task RunAsync() + public async Task RunAsync(Action progress) { - var log = new StringBuilder(); + var reporter = new DiagnosticReporter(progress); - bool success = false; - Exception exception = null; - var additionalFiles = new List(); try { - success = await RunInternalAsync(log, additionalFiles); + if (CanRun(out string skipReason)) + { + await RunInternalAsync(reporter); + } + else + { + return DiagnosticResult.Skipped(skipReason); + } } catch (Exception ex) { - exception = ex; + reporter.ReportError($"Unhandled exception: {ex.Message}", ex); } - return new DiagnosticResult - { - IsSuccess = success, - DiagnosticLog = log.ToString(), - Exception = exception, - AdditionalFiles = additionalFiles - }; + return reporter.CreateResult(); } - protected abstract Task RunInternalAsync(StringBuilder log, IList additionalFiles); + protected abstract Task RunInternalAsync(IDiagnosticReporter reporter); + + private class DiagnosticReporter(Action progress) : IDiagnosticReporter + { + private readonly Action _progress = progress; + private readonly List _reports = new(); + private readonly List _files = new(); + + public void AddFile(string path) => _files.Add(path); + + public void ReportProgress(string message) + { + _reports.Add(DiagnosticReport.Progress(message)); + _progress?.Invoke(message); + } + + public void ReportInfo(string message) => _reports.Add(DiagnosticReport.Info(message)); + public void ReportWarning(string message) => _reports.Add(DiagnosticReport.Warning(message)); + public void ReportError(string message, Exception exception = null) => + _reports.Add(DiagnosticReport.Error(message, exception)); + + public DiagnosticResult CreateResult() => new(_reports, _files); + } } public class DiagnosticResult { - public bool IsSuccess { get; set; } - public Exception Exception { get; set; } - public string DiagnosticLog { get; set; } - public ICollection AdditionalFiles { get; set; } + public static DiagnosticResult Skipped(string reason) => new([], [], reason); + + public DiagnosticResult(IEnumerable reports, IEnumerable additionalFiles) + : this(reports, additionalFiles, null) { } + + private DiagnosticResult(IEnumerable reports, IEnumerable additionalFiles, string skipReason) + { + Reports = reports.ToArray(); + AdditionalFiles = additionalFiles.ToArray(); + SkipReason = skipReason; + + ErrorCount = Reports.Count(x => x.Kind == DiagnosticReportKind.Error); + WarningCount = Reports.Count(x => x.Kind == DiagnosticReportKind.Warning); + Outcome = !string.IsNullOrWhiteSpace(skipReason) + ? DiagnosticOutcome.Skipped + : ErrorCount > 0 + ? DiagnosticOutcome.Error + : WarningCount > 0 + ? DiagnosticOutcome.Warning + : DiagnosticOutcome.Success; + + Exception[] exceptions = Reports.Where(x => x.Exception is not null) + .Select(x => x.Exception) + .ToArray(); + + Exception = exceptions.Length switch + { + 0 => null, + 1 => exceptions[0], + _ => new AggregateException(exceptions) + }; + } + + public DiagnosticOutcome Outcome { get; } + public string SkipReason { get; } + public int ErrorCount { get; } + public int WarningCount { get; } + public IReadOnlyList Reports { get; } + public IReadOnlyList AdditionalFiles { get; } + public Exception Exception { get; } } diff --git a/src/Core/Diagnostics/EntraAuthenticationDiagnostic.cs b/src/Core/Diagnostics/EntraAuthenticationDiagnostic.cs index aea77c4b5e..ef0e69948e 100644 --- a/src/Core/Diagnostics/EntraAuthenticationDiagnostic.cs +++ b/src/Core/Diagnostics/EntraAuthenticationDiagnostic.cs @@ -1,6 +1,4 @@ using System; -using System.Collections.Generic; -using System.Text; using System.Threading.Tasks; using GitCredentialManager.Authentication.Entra; using Microsoft.Identity.Client.Extensions.Msal; @@ -13,50 +11,44 @@ public EntraAuthenticationDiagnostic(ICommandContext context) : base("Microsoft Entra authentication", context) { } - protected override async Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override async Task RunInternalAsync(IDiagnosticReporter reporter) { - var entraAuth = new EntraAuthentication(CommandContext, new PublicClientConfig + var entraAuth = new EntraAuthentication(Context, new PublicClientConfig { UseSharedCache = true, }); - log.Append("Gathering MSAL token cache data..."); + reporter.ReportProgress("Gathering MSAL token cache data"); StorageCreationProperties cacheProps = entraAuth.CreateUserTokenCacheProps(true); - log.AppendLine(" OK"); - log.AppendLine($"CacheDirectory: {cacheProps.CacheDirectory}"); - log.AppendLine($"CacheFileName: {cacheProps.CacheFileName}"); - log.AppendLine($"CacheFilePath: {cacheProps.CacheFilePath}"); + reporter.ReportInfo($"CacheDirectory: {cacheProps.CacheDirectory}"); + reporter.ReportInfo($"CacheFileName: {cacheProps.CacheFileName}"); + reporter.ReportInfo($"CacheFilePath: {cacheProps.CacheFilePath}"); if (PlatformUtils.IsMacOS()) { - log.AppendLine($"MacKeyChainAccountName: {cacheProps.MacKeyChainAccountName}"); - log.AppendLine($"MacKeyChainServiceName: {cacheProps.MacKeyChainServiceName}"); + reporter.ReportInfo($"MacKeyChainAccountName: {cacheProps.MacKeyChainAccountName}"); + reporter.ReportInfo($"MacKeyChainServiceName: {cacheProps.MacKeyChainServiceName}"); } else if (PlatformUtils.IsLinux()) { - log.AppendLine($"KeyringCollection: {cacheProps.KeyringCollection}"); - log.AppendLine($"KeyringSchemaName: {cacheProps.KeyringSchemaName}"); - log.AppendLine($"KeyringSecretLabel: {cacheProps.KeyringSecretLabel}"); - log.AppendLine($"KeyringAttribute1: ({cacheProps.KeyringAttribute1.Key},{cacheProps.KeyringAttribute1.Value})"); - log.AppendLine($"KeyringAttribute2: ({cacheProps.KeyringAttribute2.Key},{cacheProps.KeyringAttribute2.Value})"); + reporter.ReportInfo($"KeyringCollection: {cacheProps.KeyringCollection}"); + reporter.ReportInfo($"KeyringSchemaName: {cacheProps.KeyringSchemaName}"); + reporter.ReportInfo($"KeyringSecretLabel: {cacheProps.KeyringSecretLabel}"); + reporter.ReportInfo($"KeyringAttribute1: ({cacheProps.KeyringAttribute1.Key},{cacheProps.KeyringAttribute1.Value})"); + reporter.ReportInfo($"KeyringAttribute2: ({cacheProps.KeyringAttribute2.Key},{cacheProps.KeyringAttribute2.Value})"); } - log.Append("Creating cache helper..."); + reporter.ReportProgress("Creating cache helper"); var cacheHelper = await MsalCacheHelper.CreateAsync(cacheProps); - log.AppendLine(" OK"); try { - log.Append("Verifying MSAL token cache persistence..."); + reporter.ReportProgress("Verifying MSAL token cache persistence"); cacheHelper.VerifyPersistence(); - log.AppendLine(" OK"); } - catch (Exception) + catch (Exception ex) { - log.AppendLine(" Failed"); - throw; + reporter.ReportError("Failed cache persistence test", ex); } - - return true; } } } diff --git a/src/Core/Diagnostics/EnvironmentDiagnostic.cs b/src/Core/Diagnostics/EnvironmentDiagnostic.cs index dbec71f02b..b3165d2b25 100644 --- a/src/Core/Diagnostics/EnvironmentDiagnostic.cs +++ b/src/Core/Diagnostics/EnvironmentDiagnostic.cs @@ -13,25 +13,22 @@ public EnvironmentDiagnostic(ICommandContext commandContext) : base("Environment", commandContext) { } - protected override Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override Task RunInternalAsync(IDiagnosticReporter reporter) { - PlatformInformation platformInfo = PlatformUtils.GetPlatformInformation(CommandContext.Trace2); - log.AppendLine($"OSType: {platformInfo.OperatingSystemType}"); - log.AppendLine($"OSVersion: {platformInfo.OperatingSystemVersion}"); + PlatformInformation platformInfo = PlatformUtils.GetPlatformInformation(Context.Trace2); + reporter.ReportInfo($"OSType: {platformInfo.OperatingSystemType}"); + reporter.ReportInfo($"OSVersion: {platformInfo.OperatingSystemVersion}"); - log.Append("Reading environment variables..."); + reporter.ReportProgress("Reading environment variables"); IDictionary envars = Environment.GetEnvironmentVariables(); - log.AppendLine(" OK"); - log.AppendLine(" Variables:"); + reporter.ReportInfo("Variables:"); foreach (DictionaryEntry envar in envars) { - log.AppendFormat("{0}={1}", envar.Key, envar.Value); - log.AppendLine(); + reporter.ReportInfo($"{envar.Key}={envar.Value}"); } - log.AppendLine(); - return Task.FromResult(true); + return Task.CompletedTask; } } } diff --git a/src/Core/Diagnostics/FileSystemDiagnostic.cs b/src/Core/Diagnostics/FileSystemDiagnostic.cs index a6711036f6..b26e276c57 100644 --- a/src/Core/Diagnostics/FileSystemDiagnostic.cs +++ b/src/Core/Diagnostics/FileSystemDiagnostic.cs @@ -1,7 +1,5 @@ using System; -using System.Collections.Generic; using System.IO; -using System.Text; using System.Threading.Tasks; namespace GitCredentialManager.Diagnostics @@ -12,42 +10,39 @@ public FileSystemDiagnostic(ICommandContext commandContext) : base("File system", commandContext) { } - protected override Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override Task RunInternalAsync(IDiagnosticReporter reporter) { string tempDir = Path.GetTempPath(); - log.AppendLine($"Temporary directory is '{tempDir}'..."); + reporter.ReportInfo($"Temporary directory is '{tempDir}'"); - log.AppendLine("Checking basic file I/O..."); + reporter.ReportProgress("Checking basic file I/O"); const string testContent = "Hello, GCM!"; string fileName = Guid.NewGuid().ToString("N").Substring(8); string path = Path.Combine(tempDir, fileName); - log.Append($"Writing to temporary file '{path}'..."); + reporter.ReportProgress($"Writing to temporary file '{path}'"); File.WriteAllText(path, testContent); - log.AppendLine(" OK"); - log.Append($"Reading from temporary file '{path}'..."); + reporter.ReportProgress($"Reading from temporary file '{path}'"); string actualContent = File.ReadAllText(path); - log.AppendLine(" OK"); if (!StringComparer.Ordinal.Equals(testContent, actualContent)) { - log.AppendLine("File data did not match!"); - log.AppendLine($"Expected: {testContent}"); - log.AppendLine($"Actual: {actualContent}"); - return Task.FromResult(false); + reporter.ReportError("File data did not match!"); + reporter.ReportError($"Expected: {testContent}"); + reporter.ReportError($"Actual: {actualContent}"); + return Task.CompletedTask; } - log.Append($"Deleting temporary file '{path}'..."); + reporter.ReportProgress($"Deleting temporary file '{path}'"); File.Delete(path); - log.AppendLine(" OK"); - log.AppendLine("Testing IFileSystem instance..."); - log.AppendLine($"UserHomePath: {CommandContext.FileSystem.UserHomePath}"); - log.AppendLine($"UserDataDirectoryPath: {CommandContext.FileSystem.UserDataDirectoryPath}"); - log.AppendLine($"GetCurrentDirectory(): {CommandContext.FileSystem.GetCurrentDirectory()}"); + reporter.ReportProgress("Testing IFileSystem instance"); + reporter.ReportInfo($"UserHomePath: {Context.FileSystem.UserHomePath}"); + reporter.ReportInfo($"UserDataDirectoryPath: {Context.FileSystem.UserDataDirectoryPath}"); + reporter.ReportInfo($"GetCurrentDirectory(): {Context.FileSystem.GetCurrentDirectory()}"); - return Task.FromResult(true); + return Task.CompletedTask; } } } diff --git a/src/Core/Diagnostics/GitDiagnostic.cs b/src/Core/Diagnostics/GitDiagnostic.cs index 74c4c76b34..6220249c66 100644 --- a/src/Core/Diagnostics/GitDiagnostic.cs +++ b/src/Core/Diagnostics/GitDiagnostic.cs @@ -1,6 +1,3 @@ -using System.Collections.Generic; -using System.Diagnostics; -using System.Text; using System.Threading.Tasks; namespace GitCredentialManager.Diagnostics @@ -11,38 +8,34 @@ public GitDiagnostic(ICommandContext commandContext) : base("Git", commandContext) { } - protected override Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override Task RunInternalAsync(IDiagnosticReporter reporter) { - log.Append("Getting Git version..."); - GitVersion gitVersion = CommandContext.Git.Version; - log.AppendLine(" OK"); - log.AppendLine($"Git version is '{gitVersion.OriginalString}'"); + reporter.ReportProgress("Getting Git version"); + GitVersion gitVersion = Context.Git.Version; + reporter.ReportInfo($"Git version is '{gitVersion.OriginalString}'"); - log.Append("Locating current repository..."); - if (!CommandContext.Git.IsInsideRepository()) + reporter.ReportProgress("Locating current repository"); + if (!Context.Git.IsInsideRepository()) { - log.AppendLine("Not inside a Git repository."); + reporter.ReportInfo("Not inside a Git repository."); } else { - string thisRepo = CommandContext.Git.GetCurrentRepository(); - log.AppendLine($"Git repository at '{thisRepo}'"); + string thisRepo = Context.Git.GetCurrentRepository(); + reporter.ReportInfo($"Git repository at '{thisRepo}'"); } - log.AppendLine(" OK"); - log.Append("Listing all Git configuration..."); - ChildProcess configProc = CommandContext.Git.CreateProcess("config --list --show-origin"); + reporter.ReportProgress("Listing all Git configuration"); + ChildProcess configProc = Context.Git.CreateProcess("config --list --show-origin"); configProc.Start(Trace2ProcessClass.Git); // To avoid deadlocks, always read the output stream first and then wait // TODO: don't read in all the data at once; stream it string gitConfig = configProc.StandardOutput.ReadToEnd().TrimEnd(); configProc.WaitForExit(); - log.AppendLine(" OK"); - log.AppendLine("Git configuration:"); - log.AppendLine(gitConfig); - log.AppendLine(); + reporter.ReportInfo("Git configuration:"); + reporter.ReportInfo(gitConfig); - return Task.FromResult(true); + return Task.CompletedTask; } } } diff --git a/src/Core/Diagnostics/NetworkingDiagnostic.cs b/src/Core/Diagnostics/NetworkingDiagnostic.cs index c49104ea81..45c5bef87d 100644 --- a/src/Core/Diagnostics/NetworkingDiagnostic.cs +++ b/src/Core/Diagnostics/NetworkingDiagnostic.cs @@ -19,30 +19,27 @@ public NetworkingDiagnostic(ICommandContext commandContext) : base("Networking", commandContext) { } - protected override async Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override async Task RunInternalAsync(IDiagnosticReporter reporter) { - log.AppendLine("Checking networking and HTTP stack..."); - log.Append("Creating HTTP client..."); - using var httpClient = CommandContext.HttpClientFactory.CreateClient(); - log.AppendLine(" OK"); + reporter.ReportProgress("Checking networking and HTTP stack"); + reporter.ReportProgress("Creating HTTP client"); + using var httpClient = Context.HttpClientFactory.CreateClient(); bool hasNetwork = NetworkInterface.GetIsNetworkAvailable(); - log.AppendLine($"IsNetworkAvailable: {hasNetwork}"); + reporter.ReportInfo($"IsNetworkAvailable: {hasNetwork}"); - await SendHttpRequestAsync(log, httpClient); + await SendHttpRequestAsync(reporter, httpClient); - log.Append($"Sending HEAD request to {TestHttpsUri}..."); + reporter.ReportProgress($"Sending HEAD request to {TestHttpsUri}"); using var httpsResponse = await httpClient.HeadAsync(TestHttpsUri); - log.AppendLine(" OK"); - log.Append("Acquiring free TCP port..."); + reporter.ReportProgress("Acquiring free TCP port"); var tcpListener = new TcpListener(IPAddress.Loopback, 0); int tcpPort; try { tcpListener.Start(); tcpPort = ((IPEndPoint) tcpListener.LocalEndpoint).Port; - log.AppendLine(" OK"); } finally { @@ -51,67 +48,62 @@ protected override async Task RunInternalAsync(StringBuilder log, IList listenContextTask = httpListener.GetContextAsync(); Task localResponseTask = httpClient.GetAsync(localAddress); - log.Append("Waiting for loopback connection..."); + reporter.ReportProgress("Waiting for loopback connection"); HttpListenerContext listenContext = await listenContextTask; - log.AppendLine(" OK"); - log.Append("Writing response..."); + reporter.ReportProgress("Writing response"); listenContext.Response.ContentLength64 = responseData.Length; listenContext.Response.OutputStream.Write(responseData, 0, responseData.Length); listenContext.Response.Close(); - log.AppendLine(" OK"); - log.Append("Waiting for response data..."); + reporter.ReportProgress("Waiting for response data"); using HttpResponseMessage localResponse = await localResponseTask; byte[] actualResponseData = await localResponse.Content.ReadAsByteArrayAsync(); string actualResponseContent = Encoding.UTF8.GetString(actualResponseData); - log.AppendLine(" OK"); if (!StringComparer.Ordinal.Equals(responseContent, actualResponseContent)) { - log.AppendLine("Loopback connection data did not match!"); - log.AppendLine($"Expected: {responseContent}"); - log.AppendLine($"Actual: {actualResponseContent}"); - return false; + reporter.ReportError("Loopback connection data did not match!"); + reporter.ReportError($"Expected: {responseContent}"); + reporter.ReportError($"Actual: {actualResponseContent}"); + return; } - log.AppendLine("Loopback connection data OK"); - - return true; + reporter.ReportInfo("Loopback connection data OK"); } - internal /* For testing purposes */ async Task SendHttpRequestAsync(StringBuilder log, HttpClient httpClient) + internal /* For testing purposes */ async Task SendHttpRequestAsync( + IDiagnosticReporter reporter, HttpClient httpClient) { foreach (var uri in new List { TestHttpUri, TestHttpUriFallback }) { try { - log.Append($"Sending HEAD request to {uri}..."); + reporter.ReportProgress($"Sending HEAD request to {uri}"); using var httpResponse = await httpClient.HeadAsync(uri); - log.AppendLine(" OK"); break; } catch (HttpRequestException) { - log.AppendLine(" warning: HEAD request failed"); + reporter.ReportWarning("HEAD request failed"); } } } diff --git a/src/GitHub/Diagnostics/GitHubApiDiagnostic.cs b/src/GitHub/Diagnostics/GitHubApiDiagnostic.cs index 8482d001f7..c7ef671dab 100644 --- a/src/GitHub/Diagnostics/GitHubApiDiagnostic.cs +++ b/src/GitHub/Diagnostics/GitHubApiDiagnostic.cs @@ -1,7 +1,4 @@ using System; -using System.Collections.Generic; -using System.CommandLine; -using System.Text; using System.Threading.Tasks; using GitCredentialManager; using GitCredentialManager.Diagnostics; @@ -18,16 +15,13 @@ public GitHubApiDiagnostic(IGitHubRestApi api, ICommandContext commandContext) _api = api; } - protected override async Task RunInternalAsync(StringBuilder log, IList additionalFiles) + protected override async Task RunInternalAsync(IDiagnosticReporter reporter) { var targetUri = new Uri("https://github.com"); - log.AppendLine($"Using '{targetUri}' as API target."); + reporter.ReportInfo($"Using '{targetUri}' as API target."); - log.Append("Querying '/meta' endpoint..."); + reporter.ReportProgress("Querying '/meta' endpoint"); GitHubMetaInfo metaInfo = await _api.GetMetaInfoAsync(targetUri); - log.AppendLine(" OK"); - - return true; } } } diff --git a/src/TestInfrastructure/Objects/TestDiagnosticReporter.cs b/src/TestInfrastructure/Objects/TestDiagnosticReporter.cs new file mode 100644 index 0000000000..7f08d8b447 --- /dev/null +++ b/src/TestInfrastructure/Objects/TestDiagnosticReporter.cs @@ -0,0 +1,39 @@ +using System; +using System.Collections.Generic; +using GitCredentialManager.Diagnostics; + +namespace GitCredentialManager.Tests.Objects; + +public class TestDiagnosticReporter : IDiagnosticReporter +{ + public IList Progress { get; } = new List(); + public IList Info { get; } = new List(); + public IList Warnings { get; } = new List(); + public IList<(string Message, Exception Exception)> Errors { get; } = new List<(string, Exception)>(); + public IList Files { get; } = new List(); + + public void ReportProgress(string message) + { + Progress.Add(message); + } + + public void ReportInfo(string message) + { + Info.Add(message); + } + + public void ReportWarning(string message) + { + Warnings.Add(message); + } + + public void ReportError(string message, Exception exception = null) + { + Errors.Add((message, exception)); + } + + public void AddFile(string path) + { + Files.Add(path); + } +} From ce2da0546cfb44ff6ccee24e78652483d4421fa9 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Thu, 27 Aug 2026 20:39:16 +0100 Subject: [PATCH 4/6] diagnose: centralize result handling The command now retains outcome, report, exception, and file data for each diagnostic. Keeping presentation and file copying inside the execution loop would make the summary depend on mutable counters and scatter the logging rules. Return a result from each run and aggregate those results before copying files or printing the summary. Isolating header, exception, and presentation logic keeps the command flow linear and gives later options one place to decide how outcomes affect the exit status. Assisted-by: GPT-5.6 Sol Fast (Internal only) Signed-off-by: Matthew John Cheetham --- src/Core/Commands/DiagnoseCommand.cs | 244 +++++++++++++++------------ 1 file changed, 138 insertions(+), 106 deletions(-) diff --git a/src/Core/Commands/DiagnoseCommand.cs b/src/Core/Commands/DiagnoseCommand.cs index 31b5c449e3..03a5b51641 100644 --- a/src/Core/Commands/DiagnoseCommand.cs +++ b/src/Core/Commands/DiagnoseCommand.cs @@ -61,10 +61,6 @@ private async Task ExecuteAsync(string output) return 0; } - int numFailed = 0; - int numSkipped = 0; - int numWarned = 0; - string currentDir = Directory.GetCurrentDirectory(); string outputDir; if (string.IsNullOrWhiteSpace(output)) @@ -82,9 +78,55 @@ private async Task ExecuteAsync(string output) } string logFilePath = Path.Combine(outputDir, "gcm-diagnose.log"); - var extraLogs = new List(); + var results = new List(); using var fullLog = new StreamWriter(logFilePath, append: false, Encoding.UTF8); + WriteLogHeader(fullLog); + + foreach (IDiagnostic diagnostic in _diagnostics) + { + DiagnosticResult result = await RunDiagnosticAsync(diagnostic, fullLog); + results.Add(result); + } + + IReadOnlyList additionalFiles = CopyFiles(outputDir, results.SelectMany(x => x.AdditionalFiles)); + + Console.WriteLine(); + PrintSummary(logFilePath, results, additionalFiles); + + fullLog.Close(); + return results.Any(x => x.Outcome == DiagnosticOutcome.Error) ? 1 : 0; + } + + private void PrintSummary(string logFilePath, IReadOnlyList results, IReadOnlyList additionalFiles) + { + int numPassed = results.Count(x => x.Outcome == DiagnosticOutcome.Success); + int numFailed = results.Count(x => x.Outcome == DiagnosticOutcome.Error); + int numWarned = results.Count(x => x.Outcome == DiagnosticOutcome.Warning); + int numSkipped = results.Count(x => x.Outcome == DiagnosticOutcome.Skipped); + + string summary = $"Diagnostic summary: {numPassed} passed, {numSkipped} skipped, {numWarned} warned, {numFailed} failed."; + Console.WriteLine(summary); + Console.WriteLine("Log files:"); + Console.WriteLine($" {logFilePath}"); + foreach (string filePath in additionalFiles) + { + Console.WriteLine($" {filePath}"); + } + Console.WriteLine(); + Console.WriteLine("Caution: Log files may include sensitive information - redact before sharing."); + Console.WriteLine(); + + if (numFailed + numWarned > 0) + { + Console.WriteLine("Diagnostics indicate a possible problem with your installation."); + Console.WriteLine($"Please open an issue at {Constants.HelpUrls.GcmNewIssue} and include log files."); + Console.WriteLine(); + } + } + + private void WriteLogHeader(StreamWriter fullLog) + { fullLog.WriteLine("Diagnose log at {0:s}Z", DateTime.UtcNow); fullLog.WriteLine(); fullLog.WriteLine($"AppPath: {_context.ApplicationPath}"); @@ -95,132 +137,122 @@ private async Task ExecuteAsync(string output) : "Version: [!] Failed to get version information [!]" ); fullLog.WriteLine(); + } - foreach (IDiagnostic diagnostic in _diagnostics) - { - fullLog.WriteLine("------------"); - fullLog.WriteLine($"Diagnostic: {diagnostic.Name}"); + private async Task RunDiagnosticAsync(IDiagnostic diagnostic, StreamWriter fullLog) + { + fullLog.WriteLine("------------"); + fullLog.WriteLine($"Diagnostic: {diagnostic.Name}"); - if (!diagnostic.CanRun(out string skipReason)) + if (!diagnostic.CanRun(out string skipReason)) + { + fullLog.Write("Outcome: Skipped"); + if (!string.IsNullOrWhiteSpace(skipReason)) { - fullLog.Write("Outcome: Skipped"); - if (!string.IsNullOrWhiteSpace(skipReason)) - { - fullLog.Write($" ({skipReason})"); - } - fullLog.WriteLine(); - - Console.Write(" "); - ConsoleEx.WriteColor("[SKIP]", ConsoleColor.DarkGray); - Console.WriteLine(" {0}", diagnostic.Name); - - numSkipped++; - continue; + fullLog.Write($" ({skipReason})"); } + fullLog.WriteLine(); - string inProgressMsg = $" >>>> {diagnostic.Name}"; - Console.Write(inProgressMsg); + Console.Write(" "); + ConsoleEx.WriteColor("[SKIP]", ConsoleColor.DarkGray); + Console.WriteLine(" {0}", diagnostic.Name); - DiagnosticResult result = await diagnostic.RunAsync(); - fullLog.WriteLine("Outcome: {0}", result.Outcome); + return DiagnosticResult.Skipped(skipReason); + } - if (result.Exception is AggregateException aex) - { - fullLog.WriteLine("Exception: AggregateException"); - fullLog.WriteLine("InnerExceptions (flattened):"); - foreach (var inner in aex.Flatten().InnerExceptions) - { - fullLog.WriteLine(inner.ToString()); - } - } - else if (result.Exception is not null) - { - fullLog.WriteLine("Exception: {0}", result.Exception); - } + string inProgressMsg = $" >>>> {diagnostic.Name}"; + Console.Write(inProgressMsg); - fullLog.WriteLine("Log:"); - foreach (var report in result.Reports) - { - fullLog.WriteLine(report.Message); - } + DiagnosticResult result = await diagnostic.RunAsync(); + fullLog.WriteLine("Outcome: {0}", result.Outcome); - Console.Write(new string('\b', inProgressMsg.Length - 1)); - switch (result.Outcome) - { - case DiagnosticOutcome.Success: - ConsoleEx.WriteColor("[ OK ]", ConsoleColor.DarkGreen); - break; - case DiagnosticOutcome.Warning: - ConsoleEx.WriteColor("[WARN]", ConsoleColor.DarkYellow); - numWarned++; - break; - case DiagnosticOutcome.Error: - ConsoleEx.WriteColor("[FAIL]", ConsoleColor.Red); - numFailed++; - break; - default: - throw new ArgumentOutOfRangeException(); - } - Console.WriteLine(" {0}", diagnostic.Name); + WriteException(fullLog, result.Exception); - if (result.Outcome != DiagnosticOutcome.Success) - { - if (result.Exception is not null) - { - Console.WriteLine(); - ConsoleEx.WriteLineIndent("[!] Encountered an exception [!]"); - ConsoleEx.WriteLineIndent(result.Exception.ToString()); - } + fullLog.WriteLine("Log:"); + foreach (var report in result.Reports) + { + fullLog.WriteLine(report.Message); + } - Console.WriteLine(); - ConsoleEx.WriteLineIndent("[*] Diagnostic test log [*]"); - ConsoleEx.WriteLineIndent(result.Reports.Select(x => x.Message)); + Console.Write(new string('\b', inProgressMsg.Length - 1)); + switch (result.Outcome) + { + case DiagnosticOutcome.Success: + ConsoleEx.WriteColor("[ OK ]", ConsoleColor.DarkGreen); + break; + case DiagnosticOutcome.Warning: + ConsoleEx.WriteColor("[WARN]", ConsoleColor.DarkYellow); + break; + case DiagnosticOutcome.Error: + ConsoleEx.WriteColor("[FAIL]", ConsoleColor.Red ); + break; + default: + throw new ArgumentOutOfRangeException(); + } + Console.WriteLine(" {0}", diagnostic.Name); + if (result.Outcome != DiagnosticOutcome.Success) + { + if (result.Exception is not null) + { Console.WriteLine(); + ConsoleEx.WriteLineIndent("[!] Encountered an exception [!]"); + ConsoleEx.WriteLineIndent(result.Exception.ToString()); } - foreach (string filePath in result.AdditionalFiles) + Console.WriteLine(); + ConsoleEx.WriteLineIndent("[*] Diagnostic test log [*]"); + ConsoleEx.WriteLineIndent(result.Reports.Select(x => x.ToLogString())); + + Console.WriteLine(); + } + + fullLog.Flush(); + return result; + } + + private static IReadOnlyList CopyFiles(string outputDir, IEnumerable additionalFiles) + { + var extraLogs = new List(); + foreach (string filePath in additionalFiles) + { + string fileName = Path.GetFileName(filePath); + string destPath = Path.Combine(outputDir, fileName); + try + { + File.Copy(filePath, destPath, overwrite: true); + } + catch { - string fileName = Path.GetFileName(filePath); - string destPath = Path.Combine(outputDir, fileName); - try - { - File.Copy(filePath, destPath, overwrite: true); - } - catch - { - ConsoleEx.WriteLineIndent($"Failed to copy additional file '{filePath}'"); - } - - extraLogs.Add(destPath); + ConsoleEx.WriteLineIndent($"Failed to copy additional file '{filePath}'"); } - fullLog.Flush(); + extraLogs.Add(destPath); } - Console.WriteLine(); - int numPassed = _diagnostics.Count - numFailed - numSkipped - numWarned; - string summary = $"Diagnostic summary: {numPassed} passed, {numSkipped} skipped, {numWarned} warned, {numFailed} failed."; - Console.WriteLine(summary); - Console.WriteLine("Log files:"); - Console.WriteLine($" {logFilePath}"); - foreach (string log in extraLogs) + return extraLogs; + } + + private void WriteException(StreamWriter log, Exception exception) + { + if (exception is null) { - Console.WriteLine($" {log}"); + return; } - Console.WriteLine(); - Console.WriteLine("Caution: Log files may include sensitive information - redact before sharing."); - Console.WriteLine(); - if (numFailed + numWarned > 0) + if (exception is AggregateException aex) { - Console.WriteLine("Diagnostics indicate a possible problem with your installation."); - Console.WriteLine($"Please open an issue at {Constants.HelpUrls.GcmNewIssue} and include log files."); - Console.WriteLine(); + log.WriteLine("Exception: AggregateException"); + log.WriteLine("InnerExceptions (flattened):"); + foreach (var inner in aex.Flatten().InnerExceptions) + { + log.WriteLine(inner.ToString()); + } + } + else + { + log.WriteLine("Exception: {0}", exception); } - - fullLog.Close(); - return numFailed > 0 ? 1 : 0; } private static class ConsoleEx From ed0298eb927e161466c5bdaf8761df8a853afdec Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Fri, 28 Aug 2026 10:17:30 +0100 Subject: [PATCH 5/6] diagnose: add strict mode (warning => non-zero exit) Add a new --strict flag to the diagnose command to return a non-zero result when warnings are emitted. Signed-off-by: Matthew John Cheetham --- .../Commands/DiagnoseCommandTests.cs | 24 ++++++++++++++++++- src/Core/Commands/DiagnoseCommand.cs | 16 +++++++++---- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/Core.Tests/Commands/DiagnoseCommandTests.cs b/src/Core.Tests/Commands/DiagnoseCommandTests.cs index b3a078ba80..c131714e23 100644 --- a/src/Core.Tests/Commands/DiagnoseCommandTests.cs +++ b/src/Core.Tests/Commands/DiagnoseCommandTests.cs @@ -65,7 +65,7 @@ public async Task DiagnoseCommand_AtLeastOneFailure_ReturnsNonZero() } [Fact] - public async Task DiagnoseCommand_Warnings_ReturnsZero() + public async Task DiagnoseCommand_Warnings_NonStrict_ReturnsZero() { string skip = It.IsAny(); var diagnosticMock = new Mock(MockBehavior.Strict); @@ -86,6 +86,28 @@ public async Task DiagnoseCommand_Warnings_ReturnsZero() diagnosticMock.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); } + [Fact] + public async Task DiagnoseCommand_Warnings_Strict_ReturnsNonZero() + { + string skip = It.IsAny(); + var diagnosticMock = new Mock(MockBehavior.Strict); + diagnosticMock.SetupGet(x => x.Name).Returns("TestDiagnostic"); + diagnosticMock.Setup(x => x.CanRun(out skip)).Returns(true); + diagnosticMock.Setup(x => x.RunAsync(It.IsAny>())) + .ReturnsAsync(new DiagnosticResult([ + new DiagnosticReport(DiagnosticReportKind.Warning, "Caution") + ], [])); + + var context = new TestCommandContext(); + var command = new DiagnoseCommand(context); + command.AddDiagnostic(diagnosticMock.Object); + + int result = await command.InvokeAsync(["--strict"]); + + Assert.NotEqual(0, result); + diagnosticMock.Verify(x => x.RunAsync(It.IsAny>()), Times.Once); + } + [Fact] public async Task NetworkingDiagnostic_SendHttpRequest_Primary_OK() { diff --git a/src/Core/Commands/DiagnoseCommand.cs b/src/Core/Commands/DiagnoseCommand.cs index 03a5b51641..156ad5d4d1 100644 --- a/src/Core/Commands/DiagnoseCommand.cs +++ b/src/Core/Commands/DiagnoseCommand.cs @@ -23,10 +23,13 @@ public DiagnoseCommand(ICommandContext context) _context = context; - var output = new Option(new[] { "--output", "-o" }, "Output directory for diagnostic logs."); + var output = new Option(["--output", "-o"], "Output directory for diagnostic logs."); AddOption(output); - this.SetHandler(ExecuteAsync, output); + var strict = new Option(["--strict"], "Exit with a non-zero code if diagnostic warnings are present."); + AddOption(strict); + + this.SetHandler(ExecuteAsync, output, strict); } public void AddStandardDiagnostics() @@ -49,7 +52,7 @@ public void AddDiagnostics(IEnumerable diagnostics) _diagnostics.AddRange(diagnostics); } - private async Task ExecuteAsync(string output) + private async Task ExecuteAsync(string output, bool strict) { // Don't use IStandardStreams for writing output in this command as we // cannot trust any component on the ICommandContext is working correctly. @@ -95,7 +98,12 @@ private async Task ExecuteAsync(string output) PrintSummary(logFilePath, results, additionalFiles); fullLog.Close(); - return results.Any(x => x.Outcome == DiagnosticOutcome.Error) ? 1 : 0; + + // In strict mode we treat warnings as errors, otherwise we only treat errors as failures. + DiagnosticOutcome[] failureOutcomes = strict + ? [DiagnosticOutcome.Error, DiagnosticOutcome.Warning] + : [DiagnosticOutcome.Error]; + return results.Any(x => failureOutcomes.Contains(x.Outcome)) ? 1 : 0; } private void PrintSummary(string logFilePath, IReadOnlyList results, IReadOnlyList additionalFiles) From 63406bfcb58fc9396c60d36c332d88974aa9a250 Mon Sep 17 00:00:00 2001 From: Matthew John Cheetham Date: Fri, 28 Aug 2026 15:49:09 +0100 Subject: [PATCH 6/6] diagnose: use Spectre.Console to spruce up output Use the default AnsiConsole from Spectre.Console to spruce up our diagnostic terminal output. Signed-off-by: Matthew John Cheetham --- src/Core/Commands/DiagnoseCommand.cs | 140 +++++++++++++-------------- 1 file changed, 70 insertions(+), 70 deletions(-) diff --git a/src/Core/Commands/DiagnoseCommand.cs b/src/Core/Commands/DiagnoseCommand.cs index 156ad5d4d1..e01aad4376 100644 --- a/src/Core/Commands/DiagnoseCommand.cs +++ b/src/Core/Commands/DiagnoseCommand.cs @@ -6,13 +6,12 @@ using System.Text; using System.Threading.Tasks; using GitCredentialManager.Diagnostics; +using Spectre.Console; namespace GitCredentialManager.Commands { public class DiagnoseCommand : Command { - private const string TestOutputIndent = " "; - private readonly ICommandContext _context; private readonly List _diagnostics = new(); @@ -54,13 +53,15 @@ public void AddDiagnostics(IEnumerable diagnostics) private async Task ExecuteAsync(string output, bool strict) { - // Don't use IStandardStreams for writing output in this command as we - // cannot trust any component on the ICommandContext is working correctly. - Console.WriteLine($"Running diagnostics...{Environment.NewLine}"); + // Don't use IStandardStreams or IConsoleService for writing output in this command + // as we cannot trust any component on the ICommandContext is working correctly. + // Using the default AnsiConsole directly should be safe. + AnsiConsole.MarkupLine("[b]Running diagnostics...[/]"); + AnsiConsole.WriteLine(); if (_diagnostics.Count == 0) { - Console.WriteLine("No diagnostics to run."); + AnsiConsole.WriteLine("No diagnostics to run."); return 0; } @@ -86,15 +87,20 @@ private async Task ExecuteAsync(string output, bool strict) using var fullLog = new StreamWriter(logFilePath, append: false, Encoding.UTF8); WriteLogHeader(fullLog); - foreach (IDiagnostic diagnostic in _diagnostics) + await AnsiConsole.Status() + .Spinner(Spinner.Known.BouncingBar) + .StartAsync("Running", async ctx => { - DiagnosticResult result = await RunDiagnosticAsync(diagnostic, fullLog); - results.Add(result); - } + foreach (IDiagnostic diagnostic in _diagnostics) + { + DiagnosticResult result = await RunDiagnosticAsync(diagnostic, fullLog, ctx); + results.Add(result); + } + }); IReadOnlyList additionalFiles = CopyFiles(outputDir, results.SelectMany(x => x.AdditionalFiles)); - Console.WriteLine(); + AnsiConsole.WriteLine(); PrintSummary(logFilePath, results, additionalFiles); fullLog.Close(); @@ -113,23 +119,46 @@ private void PrintSummary(string logFilePath, IReadOnlyList re int numWarned = results.Count(x => x.Outcome == DiagnosticOutcome.Warning); int numSkipped = results.Count(x => x.Outcome == DiagnosticOutcome.Skipped); - string summary = $"Diagnostic summary: {numPassed} passed, {numSkipped} skipped, {numWarned} warned, {numFailed} failed."; - Console.WriteLine(summary); - Console.WriteLine("Log files:"); - Console.WriteLine($" {logFilePath}"); + AnsiConsole.MarkupLine("[b u]Summary[/]"); + + void WriteCount(int count, string label, string color = null) + { + if (color is not null && count > 0) + { + AnsiConsole.Markup($"[{color}][b]{Markup.Escape(count.ToString())}[/] {Markup.Escape(label)}[/]"); + } + else + { + AnsiConsole.MarkupInterpolated($"[b]{count}[/] {Markup.Escape(label)}"); + } + } + + const string sep = " "; + WriteCount(numPassed, "passed", "green"); + AnsiConsole.Write(sep); + WriteCount(numSkipped, "skipped"); + AnsiConsole.Write(sep); + WriteCount(numWarned, "warned", "yellow"); + AnsiConsole.Write(sep); + WriteCount(numFailed, "failed", "red"); + AnsiConsole.WriteLine(); + + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine("[b u]Log files[/]"); + AnsiConsole.WriteLine(logFilePath); foreach (string filePath in additionalFiles) { - Console.WriteLine($" {filePath}"); + AnsiConsole.WriteLine(filePath); } - Console.WriteLine(); - Console.WriteLine("Caution: Log files may include sensitive information - redact before sharing."); - Console.WriteLine(); + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine("[yellow]Caution: Log files may include [b]sensitive information[/] - redact before sharing![/]"); + AnsiConsole.WriteLine(); if (numFailed + numWarned > 0) { - Console.WriteLine("Diagnostics indicate a possible problem with your installation."); - Console.WriteLine($"Please open an issue at {Constants.HelpUrls.GcmNewIssue} and include log files."); - Console.WriteLine(); + AnsiConsole.MarkupLine("[yellow]Diagnostics indicate a possible problem with your installation.[/]"); + AnsiConsole.MarkupLine($"[yellow]Please open an issue at [link]{Constants.HelpUrls.GcmNewIssue}[/] and include log files.[/]"); + AnsiConsole.WriteLine(); } } @@ -147,7 +176,8 @@ private void WriteLogHeader(StreamWriter fullLog) fullLog.WriteLine(); } - private async Task RunDiagnosticAsync(IDiagnostic diagnostic, StreamWriter fullLog) + private async Task RunDiagnosticAsync( + IDiagnostic diagnostic, StreamWriter fullLog, StatusContext statusContext) { fullLog.WriteLine("------------"); fullLog.WriteLine($"Diagnostic: {diagnostic.Name}"); @@ -161,17 +191,14 @@ private async Task RunDiagnosticAsync(IDiagnostic diagnostic, } fullLog.WriteLine(); - Console.Write(" "); - ConsoleEx.WriteColor("[SKIP]", ConsoleColor.DarkGray); - Console.WriteLine(" {0}", diagnostic.Name); + AnsiConsole.MarkupLineInterpolated($"[grey b][[SKIP]][/] {diagnostic.Name} [grey i]({skipReason})[/]"); return DiagnosticResult.Skipped(skipReason); } - string inProgressMsg = $" >>>> {diagnostic.Name}"; - Console.Write(inProgressMsg); - + statusContext.Status(diagnostic.Name); DiagnosticResult result = await diagnostic.RunAsync(); + fullLog.WriteLine("Outcome: {0}", result.Outcome); WriteException(fullLog, result.Exception); @@ -182,37 +209,38 @@ private async Task RunDiagnosticAsync(IDiagnostic diagnostic, fullLog.WriteLine(report.Message); } - Console.Write(new string('\b', inProgressMsg.Length - 1)); switch (result.Outcome) { case DiagnosticOutcome.Success: - ConsoleEx.WriteColor("[ OK ]", ConsoleColor.DarkGreen); + AnsiConsole.MarkupLineInterpolated($"[green b][[ OK ]][/] {diagnostic.Name}"); break; case DiagnosticOutcome.Warning: - ConsoleEx.WriteColor("[WARN]", ConsoleColor.DarkYellow); + AnsiConsole.MarkupLineInterpolated($"[yellow b][[WARN]][/] {diagnostic.Name}"); break; case DiagnosticOutcome.Error: - ConsoleEx.WriteColor("[FAIL]", ConsoleColor.Red ); + AnsiConsole.MarkupLineInterpolated($"[red b][[FAIL]][/] {diagnostic.Name}"); break; default: throw new ArgumentOutOfRangeException(); } - Console.WriteLine(" {0}", diagnostic.Name); if (result.Outcome != DiagnosticOutcome.Success) { if (result.Exception is not null) { - Console.WriteLine(); - ConsoleEx.WriteLineIndent("[!] Encountered an exception [!]"); - ConsoleEx.WriteLineIndent(result.Exception.ToString()); + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine("[red u]Exception Details[/]"); + AnsiConsole.WriteLine(result.Exception.ToString()); } - Console.WriteLine(); - ConsoleEx.WriteLineIndent("[*] Diagnostic test log [*]"); - ConsoleEx.WriteLineIndent(result.Reports.Select(x => x.ToLogString())); + AnsiConsole.WriteLine(); + AnsiConsole.MarkupLine("[u]Diagnostic Log[/]"); + foreach (var report in result.Reports) + { + AnsiConsole.WriteLine(report.Message); + } - Console.WriteLine(); + AnsiConsole.WriteLine(); } fullLog.Flush(); @@ -232,7 +260,7 @@ private static IReadOnlyList CopyFiles(string outputDir, IEnumerable lines) - { - if (lines is null) return; - - foreach (string line in lines) - { - Console.Write(TestOutputIndent); - Console.WriteLine(line); - } - } - - public static void WriteColor(string str, ConsoleColor fgColor) - { - var initFgColor = Console.ForegroundColor; - Console.ForegroundColor = fgColor; - Console.Write(str); - Console.ForegroundColor = initFgColor; - } - } } }