From 1c2c9404115f375b0e8f7cc1ffc5c60bb5f70831 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 15:53:41 +0200 Subject: [PATCH 1/9] Fix remaining #1057 audit regressions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 9 +- src/Refitter.Core/OpenApiDocumentFactory.cs | 55 ++- src/Refitter.Core/ParameterExtractor.cs | 15 +- src/Refitter.Core/RefitGenerator.cs | 9 +- src/Refitter.MSBuild/RefitterGenerateTask.cs | 126 +++++- .../Refitter.SourceGenerator.Tests.csproj | 4 +- .../SourceGeneratorDiagnosticsTests.cs | 115 +++++ .../RefitterSourceGenerator.cs | 193 +++++---- .../Examples/InlineJsonConvertersTests.cs | 24 ++ src/Refitter.Tests/GenerateCommandTests.cs | 125 ++++++ .../MultipleOpenApiPathsTests.cs | 9 +- .../OpenApiDocumentFactoryMergeTests.cs | 404 ++++++++++++++++++ .../ParameterExtractorPrivateCoverageTests.cs | 40 ++ .../RefitterGenerateTaskTests.cs | 78 +++- ...sue1039_DynamicQuerystringMutationTests.cs | 83 ++++ src/Refitter.Tests/SettingsTests.cs | 50 +++ src/Refitter/GenerateCommand.cs | 4 +- src/Refitter/Settings.cs | 42 +- src/Refitter/SettingsValidator.cs | 5 + 19 files changed, 1249 insertions(+), 141 deletions(-) create mode 100644 src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs create mode 100644 src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs diff --git a/README.md b/README.md index cd16d19d6..5490f3f29 100644 --- a/README.md +++ b/README.md @@ -158,10 +158,11 @@ OPTIONS: --no-inline-json-converters Don't inline JsonConverter attributes for enum types. When disabled, no [JsonConverter(typeof(JsonStringEnumConverter))] attributes are emitted. By default (enabled), the attribute is placed on the enum type declaration (not on properties), allowing custom converters to be registered via JsonSerializerOptions.Converters --integer-type int The .NET type to use for OpenAPI integer types without a format specifier. Common values: 'int' (default), 'long' --custom-template-directory Custom directory with NSwag fluid templates for code generation. Default is null which uses the default NSwag templates. See - --generate-authentication-header None Controls generation of Authorization header support. - Options: None (no authentication code is generated), - Parameter (adds method parameters for authentication), - Method (generates a Refit [Headers] attribute for bearer token authentication) + --generate-authentication-header [STYLE] Controls generation of Authorization header support. + Options: None (no authentication code is generated), + Parameter (adds method parameters for authentication), + Method (generates a Refit [Headers] attribute for bearer token authentication). + Legacy boolean forms (true/false) and omitting the value are also accepted for compatibility. --security-scheme Generate Authorization header for a specific security scheme. When omitted, authentication headers will be generated for all security schemes ``` diff --git a/src/Refitter.Core/OpenApiDocumentFactory.cs b/src/Refitter.Core/OpenApiDocumentFactory.cs index 0278a65ac..86c6ab8a2 100644 --- a/src/Refitter.Core/OpenApiDocumentFactory.cs +++ b/src/Refitter.Core/OpenApiDocumentFactory.cs @@ -54,7 +54,7 @@ public static async Task CreateAsync(IEnumerable openAp private static OpenApiDocument Merge(OpenApiDocument[] documents) { - var baseDocument = documents[0]; + var baseDocument = OpenApiDocument.FromJsonAsync(documents[0].ToJson(documents[0].SchemaType)).GetAwaiter().GetResult(); var tags = baseDocument.Tags; HashSet? tagNames = null; @@ -68,18 +68,14 @@ private static OpenApiDocument Merge(OpenApiDocument[] documents) var document = documents[i]; foreach (var path in document.Paths) { - if (!baseDocument.Paths.ContainsKey(path.Key)) - baseDocument.Paths[path.Key] = path.Value; + MergeIfMissingOrThrowOnConflict(baseDocument.Paths, path.Key, path.Value, "path"); } if (document.Components?.Schemas != null) { - // Ensure base document has schemas dictionary initialized (#1016) - // Components property is read-only but auto-initialized by NSwag foreach (var schema in document.Components.Schemas) { - if (!baseDocument.Components.Schemas.ContainsKey(schema.Key)) - baseDocument.Components.Schemas[schema.Key] = schema.Value; + MergeIfMissingOrThrowOnConflict(baseDocument.Components.Schemas, schema.Key, schema.Value, "schema"); } } @@ -87,8 +83,15 @@ private static OpenApiDocument Merge(OpenApiDocument[] documents) { foreach (var definition in document.Definitions) { - if (!baseDocument.Definitions.ContainsKey(definition.Key)) - baseDocument.Definitions[definition.Key] = definition.Value; + MergeIfMissingOrThrowOnConflict(baseDocument.Definitions, definition.Key, definition.Value, "definition"); + } + } + + if (document.SecurityDefinitions != null) + { + foreach (var securityDefinition in document.SecurityDefinitions) + { + MergeIfMissingOrThrowOnConflict(baseDocument.SecurityDefinitions, securityDefinition.Key, securityDefinition.Value, "security scheme"); } } @@ -107,6 +110,40 @@ private static OpenApiDocument Merge(OpenApiDocument[] documents) return baseDocument; } + private static void MergeIfMissingOrThrowOnConflict( + IDictionary target, + string key, + TValue value, + string itemType) + { + if (!target.TryGetValue(key, out var existingValue)) + { + target[key] = value; + return; + } + + if (!AreEquivalent(existingValue, value)) + throw CreateMergeConflictException(itemType, key); + } + + private static bool AreEquivalent(TValue existingValue, TValue incomingValue) + { + if (ReferenceEquals(existingValue, incomingValue) || EqualityComparer.Default.Equals(existingValue, incomingValue)) + return true; + + try + { + return Serializer.Serialize(existingValue!) == Serializer.Serialize(incomingValue!); + } + catch + { + return false; + } + } + + private static InvalidOperationException CreateMergeConflictException(string itemType, string key) => + new($"Cannot merge OpenAPI documents because a duplicate {itemType} '{key}' was found. Refitter fails fast on merge collisions to avoid silent data loss."); + /// /// Creates a new instance of the class asynchronously. /// diff --git a/src/Refitter.Core/ParameterExtractor.cs b/src/Refitter.Core/ParameterExtractor.cs index 4f1d16dd7..aad8c95f7 100644 --- a/src/Refitter.Core/ParameterExtractor.cs +++ b/src/Refitter.Core/ParameterExtractor.cs @@ -421,14 +421,13 @@ private static List GetQueryParameters(CSharpOperationModel operationMod { List? parameters = null; var dynamicQuerystringParametersCodeBuilder = new StringBuilder(); + var queryParameters = operationModel.Parameters + .Where(p => p.Kind == OpenApiParameterKind.Query) + .ToList(); if (settings.UseDynamicQuerystringParameters) { - var operationParameters = operationModel.Parameters - .Where(p => p.Kind == OpenApiParameterKind.Query) - .ToList(); - - if (operationParameters.Count >= 2) + if (queryParameters.Count >= 2) { var modifier = settings.TypeAccessibility.ToString().ToLowerInvariant(); var isRecord = settings.ImmutableRecords || @@ -444,7 +443,7 @@ private static List GetQueryParameters(CSharpOperationModel operationMod var initializedParametersCodeBuilder = new StringBuilder(); var propertiesCodeBuilder = new StringBuilder(); var allNullable = true; - foreach (var operationParameter in operationParameters) + foreach (var operationParameter in queryParameters) { var propertyType = GetQueryParameterType(operationParameter, settings); allNullable = allNullable && propertyType.EndsWith("?"); @@ -483,7 +482,6 @@ private static List GetQueryParameters(CSharpOperationModel operationMod propertiesCodeBuilder.Append($" = {formattedDefaultValue};"); } propertiesCodeBuilder.AppendLine(); - operationModel.Parameters.Remove(operationParameter); } dynamicQuerystringParametersCodeBuilder.AppendLine( @@ -519,8 +517,7 @@ private static List GetQueryParameters(CSharpOperationModel operationMod dynamicQuerystringParameters = dynamicQuerystringParametersCodeBuilder.ToString(); - parameters ??= operationModel.Parameters - .Where(p => p.Kind == OpenApiParameterKind.Query) + parameters ??= queryParameters .Select(p => { var variableName = GetVariableName(p); diff --git a/src/Refitter.Core/RefitGenerator.cs b/src/Refitter.Core/RefitGenerator.cs index 8de05642e..d1b627253 100644 --- a/src/Refitter.Core/RefitGenerator.cs +++ b/src/Refitter.Core/RefitGenerator.cs @@ -289,10 +289,12 @@ private string SanitizeGeneratedContracts(string contracts) // This allows users to override the converter via JsonSerializerOptions.Converters (e.g. to use // JsonStringEnumMemberConverter for enums with [EnumMember] values containing special characters). contracts = JsonStringEnumConverterAttributeRegex.Replace(contracts, string.Empty); + var newLine = GetPreferredNewLine(contracts); return EnumDeclarationRegex .Replace( contracts, - "$1[System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))]\n$1$2") + match => + $"{match.Groups[1].Value}[System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))]{newLine}{match.Groups[1].Value}{match.Groups[2].Value}") .TrimEnd(); } @@ -302,6 +304,11 @@ private string SanitizeGeneratedContracts(string contracts) .TrimEnd(); } + private static string GetPreferredNewLine(string content) => + content.Contains("\r\n", StringComparison.Ordinal) + ? "\r\n" + : "\n"; + private string NormalizeSwagger2OptionalReferencePropertyNullability(string contracts) { if (document.SchemaType != NJsonSchema.SchemaType.Swagger2 || diff --git a/src/Refitter.MSBuild/RefitterGenerateTask.cs b/src/Refitter.MSBuild/RefitterGenerateTask.cs index 32489fd83..ef8afa808 100644 --- a/src/Refitter.MSBuild/RefitterGenerateTask.cs +++ b/src/Refitter.MSBuild/RefitterGenerateTask.cs @@ -12,6 +12,21 @@ public class RefitterGenerateTask : MSBuildTask private static readonly System.Threading.AsyncLocal, Action, ProcessExecutionResult>?> ProcessRunnerOverride = new(); private static readonly System.Threading.AsyncLocal ProcessTimeoutMillisecondsOverride = new(); private static readonly System.Threading.AsyncLocal?> ProcessTerminatorOverride = new(); + private static readonly System.Threading.AsyncLocal?> FileExistsOverride = new(); + + private static readonly (string TargetFramework, string RuntimePrefix)[] PreferredRuntimeOrder = + [ + ("net10.0", "Microsoft.NETCore.App 10."), + ("net9.0", "Microsoft.NETCore.App 9."), + ("net8.0", "Microsoft.NETCore.App 8.") + ]; + + private static readonly string[] CompatibilityFallbackOrder = + [ + "net8.0", + "net9.0", + "net10.0" + ]; internal sealed class ProcessExecutionResult { @@ -53,6 +68,12 @@ internal static Action ProcessTerminator set => ProcessTerminatorOverride.Value = value; } + internal static Func FileExists + { + get => FileExistsOverride.Value ?? File.Exists; + set => FileExistsOverride.Value = value; + } + public string ProjectFileDirectory { get; set; } public bool DisableLogging { get; set; } @@ -70,6 +91,7 @@ internal static void ResetTestHooks() ProcessRunnerOverride.Value = null; ProcessTimeoutMillisecondsOverride.Value = null; ProcessTerminatorOverride.Value = null; + FileExistsOverride.Value = null; } public override bool Execute() @@ -131,26 +153,24 @@ private List StartProcess(string file, out bool failed) failed = false; var assembly = Assembly.GetExecutingAssembly(); var packageFolder = Path.GetDirectoryName(assembly.Location); - var separator = Path.DirectorySeparatorChar; - var refitterDll = $"{packageFolder}{separator}..{separator}net8.0{separator}refitter.dll"; var outputLines = new List(); - List installedRuntimes = InstalledDotnetRuntimesProvider(); - if (installedRuntimes.Any(r => r.StartsWith("Microsoft.NETCore.App 10."))) + List? installedRuntimes = null; + try { - // Use .NET 10 version if available - refitterDll = $"{packageFolder}{separator}..{separator}net10.0{separator}refitter.dll"; - TryLogCommandLine("Detected .NET 10 runtime. Using .NET 10 version of Refitter."); + installedRuntimes = InstalledDotnetRuntimesProvider(); } - else if (installedRuntimes.Any(r => r.StartsWith("Microsoft.NETCore.App 9."))) + catch (Exception exception) { - // Use .NET 9 version if available - refitterDll = $"{packageFolder}{separator}..{separator}net9.0{separator}refitter.dll"; - TryLogCommandLine("Detected .NET 9 runtime. Using .NET 9 version of Refitter."); + TryLogCommandLine($"Failed to inspect installed .NET runtimes: {exception.Message}. Falling back to bundled Refitter runtime selection."); } - else + + var refitterDll = ResolveRefitterDll(packageFolder, installedRuntimes, TryLogCommandLine); + if (string.IsNullOrWhiteSpace(refitterDll) || !FileExists(refitterDll)) { - TryLogCommandLine("Using .NET 8 version of Refitter."); + failed = true; + TryLogError("Unable to locate a bundled Refitter CLI runtime for the MSBuild task."); + return new List(); } var args = $"\"{refitterDll}\" --settings-file \"{file}\" --simple-output"; @@ -185,13 +205,14 @@ private List StartProcess(string file, out bool failed) if (processResult.TimedOut) { failed = true; + var timeoutDescription = FormatTimeout(ProcessTimeoutMilliseconds); if (processResult.TerminationException is null) { - TryLogError("Refitter process timed out after 300 seconds and was terminated"); + TryLogError($"Refitter process timed out after {timeoutDescription} and was terminated"); } else { - TryLogError($"Failed to terminate timed-out process: {processResult.TerminationException.Message}"); + TryLogError($"Refitter process timed out after {timeoutDescription}. Failed to terminate timed-out process: {processResult.TerminationException.Message}"); } return new List(); @@ -261,8 +282,14 @@ private static List GetInstalledDotnetRuntimes() process.Start(); using (var reader = process.StandardOutput) { - var output = reader.ReadToEnd(); - installedRuntimes.AddRange(output.Split([Environment.NewLine], StringSplitOptions.RemoveEmptyEntries)); + while (!reader.EndOfStream) + { + var line = reader.ReadLine(); + if (!string.IsNullOrWhiteSpace(line)) + { + installedRuntimes.Add(line); + } + } } process.WaitForExit(); } @@ -270,6 +297,71 @@ private static List GetInstalledDotnetRuntimes() return installedRuntimes; } + internal static string? ResolveRefitterDll(string? packageFolder, IReadOnlyList? installedRuntimes, Action logCommandLine) + { + if (string.IsNullOrWhiteSpace(packageFolder)) + { + return null; + } + + var bundledRuntimes = PreferredRuntimeOrder + .Select(candidate => new + { + candidate.TargetFramework, + candidate.RuntimePrefix, + Path = Path.GetFullPath(Path.Combine(packageFolder, "..", candidate.TargetFramework, "refitter.dll")), + }) + .ToArray(); + + if (installedRuntimes is not null) + { + foreach (var runtime in bundledRuntimes) + { + if (FileExists(runtime.Path) && + installedRuntimes.Any(installed => + !string.IsNullOrWhiteSpace(installed) && + installed.StartsWith(runtime.RuntimePrefix, StringComparison.Ordinal))) + { + logCommandLine($"Detected {GetDisplayFramework(runtime.TargetFramework)} runtime. Using {GetDisplayFramework(runtime.TargetFramework)} version of Refitter."); + return runtime.Path; + } + } + } + + foreach (var targetFramework in CompatibilityFallbackOrder) + { + var fallbackPath = bundledRuntimes + .First(runtime => runtime.TargetFramework == targetFramework) + .Path; + + if (FileExists(fallbackPath)) + { + logCommandLine($"Falling back to bundled {GetDisplayFramework(targetFramework)} version of Refitter."); + return fallbackPath; + } + } + + var coLocatedCli = Path.GetFullPath(Path.Combine(packageFolder, "refitter.dll")); + if (FileExists(coLocatedCli)) + { + logCommandLine("Falling back to co-located Refitter CLI."); + return coLocatedCli; + } + + return bundledRuntimes + .Select(runtime => runtime.Path) + .FirstOrDefault(); + } + + private static string FormatTimeout(int timeoutMilliseconds) => + timeoutMilliseconds >= 1000 && timeoutMilliseconds % 1000 == 0 + ? $"{timeoutMilliseconds / 1000} seconds" + : timeoutMilliseconds >= 1000 + ? $"{timeoutMilliseconds / 1000d:0.###} seconds" + : $"{timeoutMilliseconds} ms"; + + private static string GetDisplayFramework(string targetFramework) => targetFramework.Replace("net", ".NET "); + private void TryLogErrorFromException(Exception e) { try diff --git a/src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj b/src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj index cf2ba6f13..1cac2f39e 100644 --- a/src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj +++ b/src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj @@ -13,7 +13,9 @@ - + + + runtime; build; native; contentfiles; analyzers; buildtransitive diff --git a/src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs b/src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs new file mode 100644 index 000000000..47a83c1ba --- /dev/null +++ b/src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs @@ -0,0 +1,115 @@ +using System.Collections.Immutable; +using System.Reflection; +using FluentAssertions; +using H.Generators.Extensions; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using TUnit.Core; + +namespace Refitter.SourceGenerators.Tests; + +public class SourceGeneratorDiagnosticsTests +{ + [Test] + public void GeneratedCode_Equality_Should_Be_Structural_For_Diagnostics() + { + var sourceGeneratorAssembly = LoadSourceGeneratorAssembly(); + var diagnosticType = sourceGeneratorAssembly.GetType("Refitter.SourceGenerator.RefitterSourceGenerator+GeneratedDiagnostic", throwOnError: true)!; + var generatedCodeType = sourceGeneratorAssembly.GetType("Refitter.SourceGenerator.RefitterSourceGenerator+GeneratedCode", throwOnError: true)!; + var equatableArray = CreateEquatableArray(sourceGeneratorAssembly, diagnosticType); + + var left = Activator.CreateInstance(generatedCodeType, equatableArray, "code", "Output.g.cs"); + var right = Activator.CreateInstance(generatedCodeType, equatableArray, "code", "Output.g.cs"); + + left.Should().NotBeNull(); + right.Should().NotBeNull(); + left!.Equals(right).Should().BeTrue(); + left.GetHashCode().Should().Be(right!.GetHashCode()); + } + + [Test] + public void Generator_Should_Report_Warning_When_No_Refitter_Files_Are_Present() + { + var compilation = CSharpCompilation.Create( + "SourceGeneratorDiagnosticsTests", + [CSharpSyntaxTree.ParseText("namespace Refitter.SourceGenerator.Tests; public sealed class Stub { }")], + GetMetadataReferences(), + new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary)); + + var generatorAssembly = LoadSourceGeneratorAssembly(); + var generator = Activator.CreateInstance(generatorAssembly.GetType("Refitter.SourceGenerator.RefitterSourceGenerator", throwOnError: true)!)!; + + GeneratorDriver driver = CSharpGeneratorDriver.Create( + [((IIncrementalGenerator)generator).AsSourceGenerator()], + parseOptions: CSharpParseOptions.Default); + + driver = driver.RunGenerators(compilation); + var result = driver.GetRunResult(); + var diagnostics = result.Diagnostics.Concat(result.Results.SelectMany(generatorResult => generatorResult.Diagnostics)).ToArray(); + + diagnostics.Should().Contain(diagnostic => + diagnostic.Id == "REFITTER003" && + diagnostic.Severity == DiagnosticSeverity.Warning && + diagnostic.GetMessage().Contains("No .refitter files found", StringComparison.Ordinal)); + } + + private static IEnumerable GetMetadataReferences() => + [ + MetadataReference.CreateFromFile(typeof(object).Assembly.Location), + MetadataReference.CreateFromFile(typeof(Enumerable).Assembly.Location), + MetadataReference.CreateFromFile(typeof(System.Runtime.GCSettings).Assembly.Location) + ]; + + private static System.Reflection.Assembly LoadSourceGeneratorAssembly() + { + var assemblyPath = Path.GetFullPath( + Path.Combine( + AppContext.BaseDirectory, + "..", + "..", + "..", + "..", + "Refitter.SourceGenerator", + "bin", + "Release", + "netstandard2.0", + "Refitter.SourceGenerator.dll")); + + return System.Reflection.Assembly.LoadFrom(assemblyPath); + } + + private static object CreateEquatableArray(System.Reflection.Assembly sourceGeneratorAssembly, Type diagnosticType) + { + var diagnostic = Activator.CreateInstance( + diagnosticType, + "REFITTER002", + "Warning", + "message", + DiagnosticSeverity.Warning, + true)!; + + var immutableArrayCreate = typeof(ImmutableArray) + .GetMethods(BindingFlags.Public | BindingFlags.Static) + .Single(method => + method.Name == nameof(ImmutableArray.Create) && + method.IsGenericMethodDefinition && + method.GetParameters().Length == 1 && + method.GetParameters()[0].ParameterType.IsArray); + + var items = Array.CreateInstance(diagnosticType, 1); + items.SetValue(diagnostic, 0); + + var immutableArray = immutableArrayCreate + .MakeGenericMethod(diagnosticType) + .Invoke(null, [items])!; + + var equatableArrayType = typeof(EquatableArray) + .Assembly + .GetType("H.Generators.Extensions.EquatableArray`1", throwOnError: true)! + .MakeGenericType(diagnosticType); + + return equatableArrayType + .GetMethod("FromImmutableArray", BindingFlags.Public | BindingFlags.Static)! + .Invoke(null, [immutableArray])!; + } +} diff --git a/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs b/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs index d71b5064d..44ede5c6e 100644 --- a/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs +++ b/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs @@ -1,5 +1,7 @@ -using System.Diagnostics; +using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; +using H.Generators.Extensions; using Microsoft.CodeAnalysis; using Refitter.Core; @@ -12,6 +14,8 @@ namespace Refitter.SourceGenerator; [Generator(LanguageNames.CSharp)] public class RefitterSourceGenerator : IIncrementalGenerator { + internal const string Category = "Refitter"; + /// /// Initializes the incremental generator with the necessary configurations. /// @@ -25,23 +29,15 @@ public void Initialize(IncrementalGeneratorInitializationContext context) // collect and sort the paths of the .refitter files for logging var refitterPathList = refitterFiles .Select((t, _) => t.Path) - .Collect() - .Select((arr, _) => arr.Sort(StringComparer.InvariantCultureIgnoreCase)); + .CollectAsEquatableArray() + .Select((arr, _) => arr.AsImmutableArray().Sort(StringComparer.InvariantCultureIgnoreCase).AsEquatableArray()); - // add a source output that logs what we found for easier troubleshooting and setup + // add a source output that warns when no .refitter files were found context.RegisterSourceOutput(refitterPathList, static (spc, paths) => { - if (paths.Length == 0) + if (paths.IsEmpty) { - // log a warning if no .refitter files were found, instructing the user how to add them - Debug.WriteLine("[Refitter] No .refitter files found. Ensure they are added to your project as ``"); - return; - } - - // log each found .refitter file path - foreach (var path in paths) - { - Debug.WriteLine($"[Refitter] Found .refitter file: {path}"); + spc.ReportDiagnostic(CreateDiagnostic(CreateNoRefitterFilesFoundDiagnostic())); } }); @@ -53,22 +49,13 @@ private static void ProcessResults(SourceProductionContext context, GeneratedCod { foreach (var diagnostic in result.Diagnostics) { - context.ReportDiagnostic(diagnostic); + context.ReportDiagnostic(CreateDiagnostic(diagnostic)); } if (result.Code is not null && result.HintName is not null) { context.AddSource(result.HintName, result.Code); - context.ReportDiagnostic( - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER001", - "Refitter", - $"Refitter generated {result.HintName} successfully", - "Refitter", - DiagnosticSeverity.Info, - true), - Location.None)); + context.ReportDiagnostic(CreateDiagnostic(CreateGeneratedSuccessfullyDiagnostic(result.HintName))); } } @@ -76,22 +63,14 @@ private static void ProcessResults(SourceProductionContext context, GeneratedCod "MicrosoftCodeAnalysisCorrectness", "RS1035:Do not use APIs banned for analyzers", Justification = "By design")] - private static GeneratedCode GenerateCode( + internal static GeneratedCode GenerateCode( AdditionalText file, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); - var diagnostics = new List + var diagnostics = new List { - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER001", - "Refitter", - $"Found .refitter File: {file.Path}", - "Refitter", - DiagnosticSeverity.Info, - true), - Location.None) + CreateFoundFileDiagnostic(file.Path) }; try @@ -99,21 +78,12 @@ private static GeneratedCode GenerateCode( var content = file.GetText(cancellationToken)!; var json = content.ToString(); - diagnostics.Add( - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER001", - "Refitter File Contents", - json, - "Refitter", - DiagnosticSeverity.Info, - true), - Location.None)); + diagnostics.Add(CreateFileContentsDiagnostic(json)); var settings = TryDeserialize(json, diagnostics); if (settings is null) { - return new GeneratedCode(diagnostics); + return new GeneratedCode(diagnostics.ToImmutableArray().AsEquatableArray()); } cancellationToken.ThrowIfCancellationRequested(); @@ -129,16 +99,7 @@ private static GeneratedCode GenerateCode( if (settings.UseIsoDateFormat && settings.CodeGeneratorSettings?.DateFormat is not null) { - diagnostics.Add( - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER002", - "Warning", - "'codeGeneratorSettings.dateFormat' will be ignored due to 'useIsoDateFormat' set to true", - "Refitter", - DiagnosticSeverity.Warning, - true), - Location.None)); + diagnostics.Add(CreateIsoDateFormatOverrideDiagnostic()); } cancellationToken.ThrowIfCancellationRequested(); @@ -151,26 +112,17 @@ private static GeneratedCode GenerateCode( // when multiple .refitter files with the same name exist in different directories var hintName = CreateUniqueHintName(file.Path, settings.OutputFilename); - return new GeneratedCode(diagnostics, refit, hintName); + return new GeneratedCode(diagnostics.ToImmutableArray().AsEquatableArray(), refit, hintName); } catch (Exception e) { - diagnostics.Add( - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER000", - "Error", - $"Refitter failed to generate code: {e}", - "Refitter", - DiagnosticSeverity.Error, - true), - Location.None)); - - return new GeneratedCode(diagnostics); + diagnostics.Add(CreateErrorDiagnostic($"Refitter failed to generate code: {e}")); + + return new GeneratedCode(diagnostics.ToImmutableArray().AsEquatableArray()); } } - private static RefitGeneratorSettings? TryDeserialize(string json, List diagnostics) + private static RefitGeneratorSettings? TryDeserialize(string json, List diagnostics) { try { @@ -178,24 +130,65 @@ private static GeneratedCode GenerateCode( } catch (Exception e) { - diagnostics.Add( - Diagnostic.Create( - new DiagnosticDescriptor( - "REFITTER000", - "Error", - $"Unable to deserialize .refitter file: {e}", - "Refitter", - DiagnosticSeverity.Info, - true - ), - Location.None - ) - ); + diagnostics.Add(CreateErrorDiagnostic($"Unable to deserialize .refitter file: {e}")); return null; } } + internal static GeneratedDiagnostic CreateNoRefitterFilesFoundDiagnostic() => + new( + "REFITTER003", + "No .refitter files found", + "No .refitter files found. Ensure they are added to your project as ``.", + DiagnosticSeverity.Warning); + + internal static GeneratedDiagnostic CreateGeneratedSuccessfullyDiagnostic(string hintName) => + new( + "REFITTER001", + "Refitter", + $"Refitter generated {hintName} successfully", + DiagnosticSeverity.Info); + + private static GeneratedDiagnostic CreateFoundFileDiagnostic(string path) => + new( + "REFITTER001", + "Refitter", + $"Found .refitter File: {path}", + DiagnosticSeverity.Info); + + private static GeneratedDiagnostic CreateFileContentsDiagnostic(string json) => + new( + "REFITTER001", + "Refitter File Contents", + json, + DiagnosticSeverity.Info); + + private static GeneratedDiagnostic CreateIsoDateFormatOverrideDiagnostic() => + new( + "REFITTER002", + "Warning", + "'codeGeneratorSettings.dateFormat' will be ignored due to 'useIsoDateFormat' set to true", + DiagnosticSeverity.Warning); + + private static GeneratedDiagnostic CreateErrorDiagnostic(string message) => + new( + "REFITTER000", + "Error", + message, + DiagnosticSeverity.Error); + + private static Diagnostic CreateDiagnostic(GeneratedDiagnostic diagnostic) => + Diagnostic.Create( + new DiagnosticDescriptor( + diagnostic.Id, + diagnostic.Title, + diagnostic.Message, + Category, + diagnostic.Severity, + diagnostic.EnabledByDefault), + Location.None); + /// /// Creates a unique hint name for AddSource that prevents collisions when multiple /// .refitter files with the same name exist in different directories. @@ -248,5 +241,35 @@ private static string GetStableHash(string input) } } - private record GeneratedCode(List Diagnostics, string? Code = null, string? HintName = null); + internal readonly record struct GeneratedCode( + EquatableArray Diagnostics, + string? Code = null, + string? HintName = null); + + internal readonly record struct GeneratedDiagnostic( + string Id, + string Title, + string Message, + DiagnosticSeverity Severity, + bool EnabledByDefault = true) + : IEquatable + { + public bool Equals(GeneratedDiagnostic other) => + string.Equals(Id, other.Id, StringComparison.Ordinal) && + string.Equals(Title, other.Title, StringComparison.Ordinal) && + string.Equals(Message, other.Message, StringComparison.Ordinal) && + Severity == other.Severity && + EnabledByDefault == other.EnabledByDefault; + + public override int GetHashCode() + { + HashCode hashCode = default; + hashCode.Add(Id, StringComparer.Ordinal); + hashCode.Add(Title, StringComparer.Ordinal); + hashCode.Add(Message, StringComparer.Ordinal); + hashCode.Add((int)Severity); + hashCode.Add(EnabledByDefault); + return hashCode.ToHashCode(); + } + } } diff --git a/src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs b/src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs index d06ca7c51..b5c870d73 100644 --- a/src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs +++ b/src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs @@ -1,5 +1,8 @@ +using System.Reflection; +using System.Text.RegularExpressions; using FluentAssertions; using FluentAssertions.Execution; +using NSwag; using Refitter.Core; using Refitter.Tests.Build; using Refitter.Tests.TestUtilities; @@ -116,6 +119,27 @@ public async Task Generated_Code_Places_JsonConverter_On_Enum_Type_Not_Property( } } + [Test] + public void Generated_Code_Preserves_CRLF_When_Moving_JsonConverter_Attributes() + { + const string contracts = "[System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))]\r\npublic enum PetStatus\r\n{\r\n}\r\n"; + var settings = new RefitGeneratorSettings + { + CodeGeneratorSettings = new CodeGeneratorSettings + { + InlineJsonConverters = true + } + }; + + var method = typeof(RefitGenerator).GetMethod("SanitizeGeneratedContracts", BindingFlags.Instance | BindingFlags.NonPublic); + method.Should().NotBeNull(); + + var result = (string)method!.Invoke(new RefitGenerator(settings, new OpenApiDocument()), new object[] { contracts })!; + + result.Should().Contain("[System.Text.Json.Serialization.JsonConverter(typeof(System.Text.Json.Serialization.JsonStringEnumConverter))]\r\npublic enum PetStatus"); + Regex.Matches(result, "(? + argument.Contains(' ', StringComparison.Ordinal) + ? $"\"{argument}\"" + : argument; } diff --git a/src/Refitter.Tests/MultipleOpenApiPathsTests.cs b/src/Refitter.Tests/MultipleOpenApiPathsTests.cs index 4eba5043b..bad7fc0bf 100644 --- a/src/Refitter.Tests/MultipleOpenApiPathsTests.cs +++ b/src/Refitter.Tests/MultipleOpenApiPathsTests.cs @@ -177,15 +177,14 @@ public async Task OpenApiDocumentFactory_Merge_Keeps_Base_Document_Info() } [Test] - public async Task OpenApiDocumentFactory_Merge_Does_Not_Overwrite_Existing_Paths() + public async Task OpenApiDocumentFactory_Merge_Throws_For_Duplicate_Paths() { var (file1, _) = await CreateTestSpecFiles(); - var merged = await OpenApiDocumentFactory.CreateAsync(new[] { file1, file1 }); + var act = async () => await OpenApiDocumentFactory.CreateAsync(new[] { file1, file1 }); - // /pets appears in both but should only be present once - merged.Paths.Should().ContainKey("/pets"); - merged.Paths.Count.Should().Be(1); + await act.Should().ThrowAsync() + .WithMessage("*duplicate path '/pets'*"); } [Test] diff --git a/src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs b/src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs index b575976b1..d30832e88 100644 --- a/src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs +++ b/src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs @@ -1,4 +1,7 @@ +using System.Reflection; using FluentAssertions; +using NJsonSchema; +using NSwag; using Refitter.Core; using TUnit.Core; @@ -450,4 +453,405 @@ public async Task Merge_Handles_Three_Documents_With_Tags() merged.Tags.Should().Contain(t => t.Name == "products"); merged.Tags.Should().Contain(t => t.Name == "orders"); } + + [Test] + public async Task Merge_For_NonConflicting_Documents_Returns_A_New_Document_Without_Mutating_Inputs() + { + const string baseSpec = """ + openapi: '3.0.0' + info: + title: Base API + version: '1.0' + tags: + - name: users + description: User operations + paths: + /users: + get: + operationId: listUsersBase + tags: + - users + responses: + '200': + description: Base users + components: + schemas: + User: + type: object + properties: + id: + type: string + """; + + const string secondSpec = """ + openapi: '3.0.0' + info: + title: Second API + version: '1.0' + tags: + - name: orders + description: Order operations + paths: + /orders: + get: + operationId: listOrders + tags: + - orders + responses: + '200': + description: Orders + components: + schemas: + Order: + type: object + properties: + orderId: + type: string + """; + + var baseDocument = await OpenApiYamlDocument.FromYamlAsync(baseSpec); + var secondDocument = await OpenApiYamlDocument.FromYamlAsync(secondSpec); + + var merged = InvokeMerge(baseDocument, secondDocument); + + merged.Should().NotBeSameAs(baseDocument); + merged.Paths.Should().ContainKeys("/users", "/orders"); + merged.Components.Schemas.Should().ContainKeys("User", "Order"); + merged.Tags.Should().Contain(t => t.Name == "users"); + merged.Tags.Should().Contain(t => t.Name == "orders"); + + baseDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/users"); + baseDocument.Components.Schemas.Should().ContainSingle().Which.Key.Should().Be("User"); + baseDocument.Tags.Should().ContainSingle().Which.Name.Should().Be("users"); + + secondDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/orders"); + secondDocument.Components.Schemas.Should().ContainSingle().Which.Key.Should().Be("Order"); + secondDocument.Tags.Should().ContainSingle().Which.Name.Should().Be("orders"); + } + + [Test] + public async Task Merge_With_Collisions_Throws_And_Does_Not_Mutate_Inputs() + { + const string baseSpec = """ + openapi: '3.0.0' + info: + title: Base API + version: '1.0' + tags: + - name: users + description: User operations + paths: + /users: + get: + operationId: listUsersBase + tags: + - users + responses: + '200': + description: Base users + components: + schemas: + User: + type: object + properties: + id: + type: string + """; + + const string secondSpec = """ + openapi: '3.0.0' + info: + title: Second API + version: '1.0' + tags: + - name: orders + description: Order operations + paths: + /users: + get: + operationId: listUsersSecond + tags: + - users + responses: + '200': + description: Second users + /orders: + get: + operationId: listOrders + tags: + - orders + responses: + '200': + description: Orders + components: + schemas: + User: + type: object + properties: + email: + type: string + Order: + type: object + properties: + orderId: + type: string + """; + + var baseDocument = await OpenApiYamlDocument.FromYamlAsync(baseSpec); + var secondDocument = await OpenApiYamlDocument.FromYamlAsync(secondSpec); + + var act = () => InvokeMerge(baseDocument, secondDocument); + + act.Should().Throw() + .WithMessage("*duplicate path '/users'*"); + + baseDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/users"); + baseDocument.Components.Schemas.Should().ContainSingle().Which.Key.Should().Be("User"); + baseDocument.Tags.Should().ContainSingle().Which.Name.Should().Be("users"); + + secondDocument.Paths.Should().ContainKeys("/users", "/orders"); + secondDocument.Components.Schemas.Should().ContainKeys("User", "Order"); + secondDocument.Tags.Should().Contain(t => t.Name == "orders"); + } + + [Test] + public async Task Merge_With_Schema_Collision_Throws_And_Does_Not_Mutate_Inputs() + { + const string baseSpec = """ + { + "openapi": "3.0.0", + "info": { + "title": "Base API", + "version": "1.0" + }, + "paths": { + "/users": { + "get": { + "operationId": "ListUsers", + "responses": { + "200": { + "description": "Success" + } + } + } + } + }, + "components": { + "schemas": { + "Shared": { + "type": "object", + "properties": { + "id": { + "type": "string" + } + } + } + } + } + } + """; + + const string secondSpec = """ + { + "openapi": "3.0.0", + "info": { + "title": "Second API", + "version": "1.0" + }, + "paths": { + "/orders": { + "get": { + "operationId": "ListOrders", + "responses": { + "200": { + "description": "Success" + } + } + } + } + }, + "components": { + "schemas": { + "Shared": { + "type": "object", + "properties": { + "email": { + "type": "string" + } + } + } + } + } + } + """; + + var baseDocument = await ParseJsonDocument(baseSpec); + var secondDocument = await ParseJsonDocument(secondSpec); + + var act = () => InvokeMerge(baseDocument, secondDocument); + + act.Should().Throw() + .WithMessage("*duplicate schema 'Shared'*"); + + baseDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/users"); + baseDocument.Components.Schemas.Should().ContainSingle().Which.Key.Should().Be("Shared"); + secondDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/orders"); + secondDocument.Components.Schemas.Should().ContainSingle().Which.Key.Should().Be("Shared"); + secondDocument.Components.Schemas["Shared"].Properties.Should().ContainKey("email"); + } + + [Test] + public void Merge_With_Swagger2_Definition_Collision_Throws_And_Does_Not_Mutate_Dictionaries() + { + var target = new Dictionary + { + ["Shared"] = new() + { + Type = JsonObjectType.Object, + Properties = + { + ["id"] = new JsonSchemaProperty + { + Type = JsonObjectType.String + } + } + } + }; + var incoming = new JsonSchema + { + Type = JsonObjectType.Object, + Properties = + { + ["total"] = new JsonSchemaProperty + { + Type = JsonObjectType.Integer + } + } + }; + + var act = () => InvokeMergeIfMissingOrThrowOnConflict(target, "Shared", incoming, "definition"); + + act.Should().Throw() + .WithMessage("*duplicate definition 'Shared'*"); + + target.Should().ContainSingle().Which.Key.Should().Be("Shared"); + target["Shared"].Properties.Should().ContainKey("id"); + incoming.Properties.Should().ContainKey("total"); + } + + [Test] + public async Task Merge_With_Security_Scheme_Collision_Throws_And_Does_Not_Mutate_Inputs() + { + const string baseSpec = """ + { + "swagger": "2.0", + "info": { + "title": "Base API", + "version": "1.0" + }, + "paths": { + "/users": { + "get": { + "operationId": "ListUsers", + "responses": { + "200": { + "description": "Success" + } + } + } + } + }, + "securityDefinitions": { + "ApiKey": { + "type": "apiKey", + "name": "X-Base-Key", + "in": "header" + } + } + } + """; + + const string secondSpec = """ + { + "swagger": "2.0", + "info": { + "title": "Second API", + "version": "1.0" + }, + "paths": { + "/orders": { + "get": { + "operationId": "ListOrders", + "responses": { + "200": { + "description": "Success" + } + } + } + } + }, + "securityDefinitions": { + "ApiKey": { + "type": "apiKey", + "name": "X-Second-Key", + "in": "header" + } + } + } + """; + + var baseDocument = await ParseJsonDocument(baseSpec); + var secondDocument = await ParseJsonDocument(secondSpec); + + var act = () => InvokeMerge(baseDocument, secondDocument); + + act.Should().Throw() + .WithMessage("*duplicate security scheme 'ApiKey'*"); + + baseDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/users"); + baseDocument.SecurityDefinitions.Should().ContainSingle().Which.Key.Should().Be("ApiKey"); + baseDocument.SecurityDefinitions["ApiKey"].Name.Should().Be("X-Base-Key"); + secondDocument.Paths.Should().ContainSingle().Which.Key.Should().Be("/orders"); + secondDocument.SecurityDefinitions.Should().ContainSingle().Which.Key.Should().Be("ApiKey"); + secondDocument.SecurityDefinitions["ApiKey"].Name.Should().Be("X-Second-Key"); + } + + private static Task ParseJsonDocument(string json) + => OpenApiDocument.FromJsonAsync(json); + + private static OpenApiDocument InvokeMerge(params OpenApiDocument[] documents) + { + var mergeMethod = typeof(OpenApiDocumentFactory).GetMethod("Merge", BindingFlags.NonPublic | BindingFlags.Static); + + mergeMethod.Should().NotBeNull(); + + try + { + return (OpenApiDocument)mergeMethod!.Invoke(null, [documents])!; + } + catch (TargetInvocationException exception) when (exception.InnerException != null) + { + throw exception.InnerException; + } + } + + private static void InvokeMergeIfMissingOrThrowOnConflict( + IDictionary target, + string key, + TValue value, + string itemType) + { + var mergeMethod = typeof(OpenApiDocumentFactory) + .GetMethod("MergeIfMissingOrThrowOnConflict", BindingFlags.NonPublic | BindingFlags.Static)! + .MakeGenericMethod(typeof(TValue)); + + try + { + mergeMethod.Invoke(null, [target, key, value!, itemType]); + } + catch (TargetInvocationException exception) when (exception.InnerException != null) + { + throw exception.InnerException; + } + } } diff --git a/src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs b/src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs index f801cbe04..3d34bcafb 100644 --- a/src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs +++ b/src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs @@ -281,6 +281,46 @@ public void GetParameters_Adds_Multipart_Text_Fields_When_NSwag_Parameters_Are_E arguments[4].Should().Be(string.Empty); } + [Test] + public void GetParameters_Does_Not_Mutate_Query_Parameter_Collection_When_Generating_Dynamic_Querystring_Wrapper() + { + var firstParameter = CreateParameterModel( + "query", + "query", + parameter: new OpenApiParameter + { + Name = "query", + Kind = OpenApiParameterKind.Query, + IsRequired = true, + Schema = new JsonSchema { Type = JsonObjectType.String } + }); + var secondParameter = CreateParameterModel( + "page", + "page", + type: "int?", + parameter: new OpenApiParameter + { + Name = "page", + Kind = OpenApiParameterKind.Query, + Schema = new JsonSchema { Type = JsonObjectType.Integer } + }); + var operationModel = CreateOperationModel(firstParameter, secondParameter); + var operation = new OpenApiOperation(); + + var parameters = ParameterExtractor.GetParameters( + operationModel, + operation, + new RefitGeneratorSettings { UseDynamicQuerystringParameters = true }, + "SearchQueryParams", + out var dynamicQuerystringParameters) + .ToList(); + + parameters.Should().ContainSingle().Which.Should().Be("[Query] SearchQueryParams queryParams"); + dynamicQuerystringParameters.Should().Contain("class SearchQueryParams"); + operationModel.Parameters.Should().HaveCount(2); + operationModel.Parameters.Should().ContainInOrder(firstParameter, secondParameter); + } + private static T InvokePrivate(string methodName, Type[] parameterTypes, params object?[] arguments) { var method = typeof(ParameterExtractor).GetMethod( diff --git a/src/Refitter.Tests/RefitterGenerateTaskTests.cs b/src/Refitter.Tests/RefitterGenerateTaskTests.cs index b27b064e0..0f4e42b94 100644 --- a/src/Refitter.Tests/RefitterGenerateTaskTests.cs +++ b/src/Refitter.Tests/RefitterGenerateTaskTests.cs @@ -384,16 +384,26 @@ public void Execute_Should_Return_False_When_Runtime_Discovery_Throws() try { CreateRefitterSettingsFile(workspace); + var generatedFile = CreateGeneratedFile(workspace); RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => throw new InvalidOperationException("boom"); + RefitterGenerateTask.FileExists = path => + path.EndsWith("refitter.dll", StringComparison.OrdinalIgnoreCase) || File.Exists(path); + RefitterGenerateTask.ProcessRunner = (startInfo, logOutput, _) => + { + startInfo.Arguments.Should().Contain("net8.0"); + logOutput($"{RefitterGenerateTask.GeneratedFileMarker}{generatedFile}"); + return new RefitterGenerateTask.ProcessExecutionResult(false, 0); + }; var buildEngine = new RecordingBuildEngine(); var task = CreateTask(workspace, buildEngine); var result = task.Execute(); - result.Should().BeFalse(); - task.GeneratedFiles.Should().BeEmpty(); - buildEngine.Errors.Should().Contain(message => message.Contains("Failed to generate code from", StringComparison.Ordinal)); + result.Should().BeTrue(); + task.GeneratedFiles.Should().ContainSingle().Which.ItemSpec.Should().Be(generatedFile); + buildEngine.Messages.Should().Contain(message => message.Contains("Failed to inspect installed .NET runtimes", StringComparison.Ordinal)); + buildEngine.Messages.Should().Contain(message => message.Contains("Falling back to bundled .NET 8.0 version of Refitter.", StringComparison.Ordinal)); } finally { @@ -402,6 +412,31 @@ public void Execute_Should_Return_False_When_Runtime_Discovery_Throws() } } + [Test] + public void ResolveRefitterDll_Should_Fall_Back_When_Preferred_Runtime_Binary_Is_Missing() + { + try + { + var packageFolder = Path.Combine("C:", "repo", "tasks"); + var messages = new List(); + RefitterGenerateTask.FileExists = path => + path.Contains("net9.0", StringComparison.OrdinalIgnoreCase) || + path.Contains("net8.0", StringComparison.OrdinalIgnoreCase); + + var result = RefitterGenerateTask.ResolveRefitterDll( + packageFolder, + ["Microsoft.NETCore.App 10.0.1", "Microsoft.NETCore.App 9.0.5"], + messages.Add); + + result.Should().Be(Path.GetFullPath(Path.Combine(packageFolder, "..", "net9.0", "refitter.dll"))); + messages.Should().Contain(message => message.Contains("Detected .NET 9.0 runtime", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + } + } + [Test] public void Execute_Should_Use_DotNet9_Runtime_When_Available() { @@ -413,6 +448,9 @@ public void Execute_Should_Use_DotNet9_Runtime_When_Available() var generatedFile = CreateGeneratedFile(workspace); RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => ["Microsoft.NETCore.App 9.0.0"]; + RefitterGenerateTask.FileExists = path => + path.Contains("net9.0", StringComparison.OrdinalIgnoreCase) || + File.Exists(path); RefitterGenerateTask.ProcessRunner = (startInfo, logOutput, _) => { startInfo.Arguments.Should().Contain("net9.0"); @@ -428,7 +466,7 @@ public void Execute_Should_Use_DotNet9_Runtime_When_Available() result.Should().BeTrue(); task.GeneratedFiles.Should().ContainSingle(); task.GeneratedFiles.Single().ItemSpec.Should().Be(generatedFile); - buildEngine.Messages.Should().Contain(message => message.Contains(".NET 9 runtime", StringComparison.Ordinal)); + buildEngine.Messages.Should().Contain(message => message.Contains("Detected .NET 9.0 runtime", StringComparison.Ordinal)); } finally { @@ -448,6 +486,9 @@ public void Execute_Should_Fall_Back_To_DotNet8_Runtime_When_Newer_Runtimes_Are_ var generatedFile = CreateGeneratedFile(workspace); RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => ["Microsoft.NETCore.App 8.0.0"]; + RefitterGenerateTask.FileExists = path => + path.Contains("net8.0", StringComparison.OrdinalIgnoreCase) || + File.Exists(path); RefitterGenerateTask.ProcessRunner = (startInfo, logOutput, _) => { startInfo.Arguments.Should().Contain("net8.0"); @@ -462,7 +503,7 @@ public void Execute_Should_Fall_Back_To_DotNet8_Runtime_When_Newer_Runtimes_Are_ result.Should().BeTrue(); task.GeneratedFiles.Should().ContainSingle(); - buildEngine.Messages.Should().Contain(message => message.Contains("Using .NET 8 version of Refitter.", StringComparison.Ordinal)); + buildEngine.Messages.Should().Contain(message => message.Contains("Detected .NET 8.0 runtime", StringComparison.Ordinal)); } finally { @@ -527,6 +568,33 @@ public void Execute_Should_Log_When_Timed_Out_Process_Cannot_Be_Terminated() } } + [Test] + public void Execute_Should_Log_Configured_Timeout_Value() + { + var workspace = CreateWorkspace(); + + try + { + CreateRefitterSettingsFile(workspace); + RefitterGenerateTask.ProcessTimeoutMilliseconds = 1500; + RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => ["Microsoft.NETCore.App 10.0.0"]; + RefitterGenerateTask.ProcessRunner = (_, _, _) => new RefitterGenerateTask.ProcessExecutionResult(true, -1); + + var buildEngine = new RecordingBuildEngine(); + var task = CreateTask(workspace, buildEngine); + + var result = task.Execute(); + + result.Should().BeFalse(); + buildEngine.Errors.Should().Contain(message => message.Contains($"timed out after {1500 / 1000d:0.###} seconds", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + DeleteWorkspace(workspace); + } + } + [Test] public void Execute_Should_Log_When_Process_Exits_With_Non_Zero_Code() { diff --git a/src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs b/src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs new file mode 100644 index 000000000..fa27305fb --- /dev/null +++ b/src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs @@ -0,0 +1,83 @@ +using FluentAssertions; +using Refitter.Core; +using TUnit.Core; + +namespace Refitter.Tests.RegressionTests; + +/// +/// Regression tests for Issue #1039: dynamic querystring extraction mutates the shared NSwag model. +/// Validates that XML documentation still sees the original query parameters after wrapper generation. +/// +public class Issue1039_DynamicQuerystringMutationTests +{ + private const string OpenApiSpec = """ + { + "openapi": "3.0.1", + "info": { + "title": "Search API", + "version": "v1" + }, + "paths": { + "/search": { + "get": { + "operationId": "SearchItems", + "summary": "Search items", + "tags": [ + "search" + ], + "parameters": [ + { + "name": "query", + "in": "query", + "description": "Search text", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "page", + "in": "query", + "description": "Page number", + "schema": { + "type": "integer" + } + } + ], + "responses": { + "200": { + "description": "Success" + } + } + } + } + } + } + """; + + [Test] + [Arguments(MultipleInterfaces.Unset)] + [Arguments(MultipleInterfaces.ByTag)] + [Arguments(MultipleInterfaces.ByEndpoint)] + public async Task Dynamic_Querystring_Generation_Preserves_Original_Query_Param_Documentation( + MultipleInterfaces multipleInterfaces) + { + var swaggerFile = await TestFile.CreateSwaggerFile(OpenApiSpec, "issue-1039.json"); + var settings = new RefitGeneratorSettings + { + OpenApiPath = swaggerFile, + UseDynamicQuerystringParameters = true, + GenerateXmlDocCodeComments = true, + MultipleInterfaces = multipleInterfaces + }; + + var sut = await RefitGenerator.CreateAsync(settings); + + var generatedCode = sut.Generate(); + + generatedCode.Should().Contain("/// Search text"); + generatedCode.Should().Contain("/// Page number"); + generatedCode.Should().Contain("/// The dynamic querystring parameter wrapping all others."); + generatedCode.Should().Contain("QueryParams queryParams"); + } +} diff --git a/src/Refitter.Tests/SettingsTests.cs b/src/Refitter.Tests/SettingsTests.cs index 851cd1d05..58ca38965 100644 --- a/src/Refitter.Tests/SettingsTests.cs +++ b/src/Refitter.Tests/SettingsTests.cs @@ -1,6 +1,7 @@ using FluentAssertions; using FluentAssertions.Execution; using Refitter.Core; +using Spectre.Console.Cli; namespace Refitter.Tests; @@ -58,6 +59,7 @@ public void Default_Values_Should_Be_Set_Correctly() settings.IntegerType.Should().Be(IntegerType.Int32); settings.CustomTemplateDirectory.Should().BeNull(); settings.PropertyNamingPolicy.Should().Be(PropertyNamingPolicy.PascalCase); + settings.GenerateAuthenticationHeader.Should().BeNull(); } [Test] @@ -204,6 +206,54 @@ public void Should_Allow_Setting_PropertyNamingPolicy() settings.PropertyNamingPolicy.Should().Be(PropertyNamingPolicy.PascalCase); } + [Test] + public void TryGetAuthenticationHeaderStyle_Should_Map_Legacy_Boolean_True_To_Method() + { + var settings = new Settings { GenerateAuthenticationHeader = new FlagValue { IsSet = true, Value = "true" } }; + + var parsed = settings.TryGetAuthenticationHeaderStyle(out var style, out var errorMessage); + + parsed.Should().BeTrue(); + style.Should().Be(AuthenticationHeaderStyle.Method); + errorMessage.Should().BeNull(); + } + + [Test] + public void TryGetAuthenticationHeaderStyle_Should_Map_Legacy_Boolean_False_To_None() + { + var settings = new Settings { GenerateAuthenticationHeader = new FlagValue { IsSet = true, Value = "false" } }; + + var parsed = settings.TryGetAuthenticationHeaderStyle(out var style, out var errorMessage); + + parsed.Should().BeTrue(); + style.Should().Be(AuthenticationHeaderStyle.None); + errorMessage.Should().BeNull(); + } + + [Test] + public void TryGetAuthenticationHeaderStyle_Should_Parse_Enum_Values_Case_Insensitively() + { + var settings = new Settings { GenerateAuthenticationHeader = new FlagValue { IsSet = true, Value = "parameter" } }; + + var parsed = settings.TryGetAuthenticationHeaderStyle(out var style, out var errorMessage); + + parsed.Should().BeTrue(); + style.Should().Be(AuthenticationHeaderStyle.Parameter); + errorMessage.Should().BeNull(); + } + + [Test] + public void TryGetAuthenticationHeaderStyle_Should_Reject_Invalid_Values() + { + var settings = new Settings { GenerateAuthenticationHeader = new FlagValue { IsSet = true, Value = "headers" } }; + + var parsed = settings.TryGetAuthenticationHeaderStyle(out var style, out var errorMessage); + + parsed.Should().BeFalse(); + style.Should().Be(AuthenticationHeaderStyle.None); + errorMessage.Should().Contain("Valid values are None, Method, Parameter, true, or false"); + } + [Test] public void Should_Allow_Setting_String_Arrays() { diff --git a/src/Refitter/GenerateCommand.cs b/src/Refitter/GenerateCommand.cs index 29e891092..7211c4e39 100644 --- a/src/Refitter/GenerateCommand.cs +++ b/src/Refitter/GenerateCommand.cs @@ -320,6 +320,8 @@ protected override async Task ExecuteAsync(CommandContext context, Settings private static RefitGeneratorSettings CreateRefitGeneratorSettings(Settings settings) { + settings.TryGetAuthenticationHeaderStyle(out var authenticationHeaderStyle, out _); + return new RefitGeneratorSettings { OpenApiPath = settings.OpenApiPath!, @@ -367,7 +369,7 @@ private static RefitGeneratorSettings CreateRefitGeneratorSettings(Settings sett IntegerType = settings.IntegerType }, CustomTemplateDirectory = settings.CustomTemplateDirectory, - AuthenticationHeaderStyle = settings.GenerateAuthenticationHeader, + AuthenticationHeaderStyle = authenticationHeaderStyle, SecurityScheme = settings.SecurityScheme, GenerateJsonSerializerContext = settings.GenerateJsonSerializerContext, }; diff --git a/src/Refitter/Settings.cs b/src/Refitter/Settings.cs index 8d142e7a3..a5938c1ba 100644 --- a/src/Refitter/Settings.cs +++ b/src/Refitter/Settings.cs @@ -285,10 +285,10 @@ payloads with (yet) unknown types are offered by newer versions of an API /// /// Controls how authorization headers are generated for authenticated operations. /// - [Description("Controls generation of Authorization header support. Options: None (no authentication code is generated), Parameter (adds method parameters for authentication), Method (generates a Refit [[Headers]] attribute for bearer token authentication). Also see 'security-scheme' option")] - [CommandOption("--generate-authentication-header")] - [DefaultValue(AuthenticationHeaderStyle.None)] - public AuthenticationHeaderStyle GenerateAuthenticationHeader { get; set; } = AuthenticationHeaderStyle.None; + [Description("Controls generation of Authorization header support. Options: None (no authentication code is generated), Parameter (adds method parameters for authentication), Method (generates a Refit [[Headers]] attribute for bearer token authentication). For backward compatibility, the legacy boolean forms 'true', 'false', or omitting the value are also accepted. Also see 'security-scheme' option")] + [CommandOption("--generate-authentication-header [STYLE]")] + [DefaultValue("None")] + public FlagValue? GenerateAuthenticationHeader { get; set; } /// /// Restricts authorization header generation to a specific OpenAPI security scheme. @@ -302,4 +302,38 @@ payloads with (yet) unknown types are offered by newer versions of an API [CommandOption("--json-serializer-context")] [DefaultValue(false)] public bool GenerateJsonSerializerContext { get; set; } + + internal bool TryGetAuthenticationHeaderStyle(out AuthenticationHeaderStyle style, out string? errorMessage) + { + style = AuthenticationHeaderStyle.None; + errorMessage = null; + + if (GenerateAuthenticationHeader is null || !GenerateAuthenticationHeader.IsSet) + { + return true; + } + + var rawValue = GenerateAuthenticationHeader.Value; + if (string.IsNullOrWhiteSpace(rawValue)) + { + style = AuthenticationHeaderStyle.Method; + return true; + } + + if (bool.TryParse(rawValue, out var enabled)) + { + style = enabled ? AuthenticationHeaderStyle.Method : AuthenticationHeaderStyle.None; + return true; + } + + if (Enum.TryParse(rawValue, ignoreCase: true, out style) && + Enum.IsDefined(style)) + { + return true; + } + + errorMessage = "Invalid value for --generate-authentication-header. Valid values are None, Method, Parameter, true, or false."; + style = AuthenticationHeaderStyle.None; + return false; + } } diff --git a/src/Refitter/SettingsValidator.cs b/src/Refitter/SettingsValidator.cs index 28b673d48..f5122b615 100644 --- a/src/Refitter/SettingsValidator.cs +++ b/src/Refitter/SettingsValidator.cs @@ -17,6 +17,11 @@ public static ValidationResult Validate(Settings settings, out RefitGeneratorSet { refitSettings = null; + if (!settings.TryGetAuthenticationHeaderStyle(out _, out var authHeaderError)) + { + return ValidationResult.Error(authHeaderError!); + } + if (BothSettingsFilesAreEmpty(settings) || BothSettingsFilesArePresent(settings)) { return GetValidationErrorForSettingsFiles(); From 03bab3834cc18fe4efd19134d41f961db618d64e Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 17:29:45 +0200 Subject: [PATCH 2/9] chore(squad): log Lambert help decision Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/ash/history.md | 231 +--------- .squad/agents/dallas/history.md | 66 +++ .squad/agents/lambert/history.md | 205 +-------- .squad/agents/parker/history.md | 56 ++- .squad/agents/ripley/history.md | 31 ++ .squad/agents/scribe/history.md | 1 + .squad/decisions-archive.md | 358 ++++++++++++++++ .squad/decisions.md | 396 +++++++----------- .../spectre-cli-help-assertions/SKILL.md | 28 ++ 9 files changed, 719 insertions(+), 653 deletions(-) create mode 100644 .squad/skills/spectre-cli-help-assertions/SKILL.md diff --git a/.squad/agents/ash/history.md b/.squad/agents/ash/history.md index 19a568d33..5c49428c8 100644 --- a/.squad/agents/ash/history.md +++ b/.squad/agents/ash/history.md @@ -8,220 +8,29 @@ ## Learnings -- Added to the squad on 2026-04-20 as a specialist reviewer for PR #1064 review work. -- PR #1064's Roslyn rewrite fixes raw regex corruption for ContractTypeSuffix, but issue #1013 is not closed unless it also blocks suffix-target collisions like `Pet` + `PetDto`. -- For ParameterExtractor multipart fields, deduplication must use the emitted C# identifier, not the original OpenAPI property key, or issue #1018 still reproduces through sanitization collisions. -- For source-generator dependency reviews, inspect the packed `.nuspec` and `analyzers/dotnet/cs/` payload, not just the `.csproj`; this repo's package bundles `OasReader.dll` as an analyzer asset but still declares `Refit` as a transitive NuGet dependency. -- `RuntimeCompatibilityTests.Does_Not_Auto_Enable_Optional_Properties_As_Nullable_When_NRT_Enabled_Swagger2` currently fails because Swagger 2 generation still emits `string?` for optional properties when only `GenerateNullableReferenceTypes=true`. -- `JsonSerializerContextGenerator` now statically covers nested types, closed generic usages, cross-namespace qualification, and `I`-prefix stripping; polymorphic specs also register all derived DTOs in the generated serializer context, but there is still no runtime AOT serialization regression test for that path. -- The minimal safe #1026 fix lives in `src/Refitter.Core/RefitGenerator.cs`: for Swagger 2 only, when NRT is enabled but `GenerateOptionalPropertiesAsNullable` is false, a Roslyn rewrite strips `?` from generated nullable reference-type property declarations after NSwag generation. -- Focused #1026 regression coverage now includes Swagger 2 explicit opt-in in `src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs`, and the four targeted NRT/nullability tests pass against the updated core/test binaries. +- Added to the squad on 2026-04-20 as a specialist reviewer for PR #1064 / #1057 safety gates. +- For suffix rewrites, block both source-name corruption and suffix-target collisions (Pet + PetDto cannot both land on PetDto). +- For multipart/query parameter extraction, deduplicate by the emitted C# identifier, not the original OpenAPI key. +- For source-generator packaging reviews, inspect the packed .nuspec and analyzer payload, not just the .csproj. +- The minimal safe Swagger 2 nullable-shape fix for #1026 lives in src/Refitter.Core/RefitGenerator.cs as targeted post-processing when NRT is enabled but optional-nullable generation stays opt-in. -## 2026-04-20 Update: PR #1064 Review Complete +## Core Context -**Role Assignment:** Formal appointment as Safety Reviewer -**Scope:** Code correctness, collision detection, identifier validation -**Status:** Completed PR #1064 static analysis; identified 2 confirmed blockers (#1013, #1018) and multiple non-blocking edge cases +- **PR #1064 safety review:** Rejected the initial blocker set until #1013 collision detection and #1018 sanitized-identifier deduplication were fully closed; treated #1053 as safe once keyword escaping routed through EscapeReservedKeyword(). +- **Blocker-fix verification:** Confirmed the eventual #1013, #1018, and #1053 fixes once collision handling, naming consistency, and test expectations aligned. +- **#1024 packaging closure:** Verified that PrivateAssets="all" is required to keep Refit out of the packed source-generator nuspec, then aligned README/docs guidance around explicit consumer references. -**Key Contributions:** -- Analyzed ContractTypeSuffixApplier collision risk; confirmed no duplicate-target detection -- Analyzed ParameterExtractor multipart field deduplication; confirmed uses wrong key (original vs. sanitized) -- Reviewed automated comment surface; validated blocking vs. non-blocking classifications +## 2026-04-25: Core Review Chain -**Team Verdict:** NO MERGE YET — 5 blockers from 4 lanes (Bishop docs-ready only) -**Next Steps:** Await blocker fixes + final Parker/Lambert confirmations +- Rejected Parker's closure set and kept **#1034** and **#1039** open as real blockers. +- Cleared **#1039** once grouped query-parameter extraction stopped mutating operationModel.Parameters and the private coverage locked that invariant. +- Rejected Dallas's first merge revision because OpenApiDocumentFactory.Merge() still warned and kept the first conflicting entry instead of failing fast. +- Rejected Lambert's later proof pass because the Swagger 2 definition-collision evidence still tripped the mirrored schema conflict before isolating the intended definition lane. +- Queued Ripley for the final narrow proof-gap revision once Parker, Dallas, and Lambert were all locked out. -## 2026-04-20 Update: Blocker Gate Safety Review Complete +## 2026-04-25: Final Signoff on Ripley Follow-up -**Task:** Preflight safety review for PR #1064 merge blockers (#1013, #1018, #1053) -**Status:** COMPLETED — 3 blockers identified, acceptance checklist created -**Deliverable:** `.squad/decisions/inbox/ash-pr1064-blocker-gate.md` - -**Key Findings:** -1. **Issue #1013 (ContractTypeSuffixApplier):** Roslyn rewrite implemented ✅, but collision detection MISSING ❌ - - Current code does not verify that `typeName + suffix` doesn't collide with existing types. - - Edge case: OpenAPI with both `Pet` and `PetDto` → generates duplicate `PetDto` declarations → CS0101. - - Fix: Pre-flight collision check before building `typeRenameMap`. - -2. **Issue #1018 (ParameterExtractor Multipart):** Identifier sanitization implemented ✅, but deduplication uses WRONG KEY ❌ - - `ConvertToVariableName` now routes through `IdentifierUtils.ToCompilableIdentifier` (line 611). - - BUT: `seenFormParameterNames` tracks original OpenAPI key, not sanitized identifier. - - Edge case: `"a-b"` + `"a b"` both sanitize to `a_b` → duplicate parameter names → CS0100. - - Fix: Track `variableName` instead of `property.Key`, use `IdentifierUtils.Counted()` for collisions. - -3. **Issue #1053 (IdentifierUtils):** PARTIAL FIX — keyword escaping complete ✅, second-order risk minimal - - `__arglist`, `__makeref`, `__reftype`, `__refvalue` added to `ReservedKeywords`. - - `Sanitize()` now routes through `EscapeReservedKeyword` (line 146). - - Call-site audit confirms no invalid method/interface names (capitalization prevents keyword matches). - - Verdict: Safe (no blocking issues, audit-only re-review). - -**Acceptance Checklist Created:** -- Collision detection for #1013 -- Dedup-by-sanitized-identifier for #1018 -- Test coverage for both edge cases -- Post-fix re-review sites identified - -**Estimated Fix Time:** ~35 minutes (10min + 15min + 10min) - -**Safety-Critical Call Sites Identified:** -- `ContractTypeSuffixApplier.cs:19-46` (collision check needed) -- `ParameterExtractor.cs:97-136` (dedup key change needed) -- `ApizrRegistrationGenerator.cs:33`, `OperationNameGenerator.cs:73`, `RefitInterfaceGenerator.cs:366` (audit-only for #1053) - -**Patterns Learned:** -- For syntax-tree rewriting, always check both source names AND target names for collisions. -- For identifier sanitization, always deduplicate by the EMITTED identifier, not the input key. -- For keyword escaping changes, audit all call sites for second-order capitalization/concatenation interactions. - -## 2026-04-20 Update: Blocker Fixes Verified - -**Task:** Review working tree changes for blocker fix implementations -**Status:** COMPLETED — all 3 blockers FULLY FIXED ✅ -**Verdict:** READY FOR FINAL RE-REVIEW (cleanup pending) - -**Fix Verification:** - -1. **Issue #1013 (ContractTypeSuffixApplier) — ✅ FULLY FIXED** - - Lines 20, 29-33: `existingTypeNames` set now tracks ALL type declarations. - - Lines 50-62: Pre-flight collision check implemented — skips rename if `name + suffix` already exists. - - No exception thrown; uses skip strategy (collision types retain original name). - - Test coverage: `PR1064BlockerRegressions.cs:81-112` covers collision scenario with `Pet` + `PetDto`. - - **Verdict:** Acceptance criteria met. Implementation is safe and well-tested. - -2. **Issue #1018 (ParameterExtractor Multipart) — ✅ FULLY FIXED** - - Line 100: `seenFormParameterNames` now initialized with `GetVariableName(p)` (sanitized identifiers). - - Line 129: Deduplication check uses `variableName` (sanitized), not `property.Key` (original). - - Uses "first wins" strategy for collisions (consistent with HashSet.Add). - - Test coverage: `PR1064BlockerRegressions.cs:164-193` verifies `"a-b"`, `"a b"`, `"a.b"` dedupe to single parameter. - - **Verdict:** Acceptance criteria met. Deduplication by sanitized identifier is correct. - -3. **Issue #1053 (IdentifierUtils Keyword Escaping) — ✅ FULLY FIXED** - - RefitInterfaceGenerator.cs:370: Interface name now uses `$"I{title.CapitalizeFirstCharacter()}".Sanitize()` (sanitize AFTER prefixing). - - Prevents `I@class` pattern (now generates `I@Class` which is invalid, but capitalizes before sanitize → `IClass` → safe). - - Actually: wait, let me re-check this logic... - - Line 370: `$"I{title.CapitalizeFirstCharacter()}".Sanitize()` means if title = "class": - - `title.CapitalizeFirstCharacter()` → `"Class"` - - `$"I{Class}"` → `"IClass"` - - `"IClass".Sanitize()` → `"IClass"` (not a keyword, no escaping needed) - - **Verdict:** Implementation is correct. Comment says "prevent I@keyword" which is achieved by capitalizing before concatenation. - -**Test File Review:** -- `PR1064BlockerRegressions.cs` has comprehensive coverage for all 3 blockers (13 tests total). -- Tests verify both correctness AND compilation success (BuildHelper.BuildCSharp). -- All edge cases from blocker gate review are covered. - -**Temporary Files to Clean:** -- ⚠️ `src/test-multipart.json` — repro file for #1018 (should be deleted before merge) -- ⚠️ `src/test-keywords.json` — repro file for #1053 (should be deleted before merge) - -**Final Recommendation:** -- ✅ All 3 blockers are FULLY RESOLVED. -- ✅ Test coverage is comprehensive. -- ⚠️ DELETE temporary repro JSON files before merge. -- ✅ Code is ready for final CI/CD validation. - -## 2026-04-20 Update: PR #1064 Blocker Revision Complete - -**Task:** Fix remaining merge blockers after Parker's patch was rejected by Dallas validation. -**Status:** COMPLETED — All blockers fixed and tests passing ✅ -**Parker Lockout:** Parker's revision failed validation; Ash owned the final fix. -**Ripley Triage:** Correctly diagnosed naming method mismatch (#1018) and NSwag contract bypass (#1053). - -**Issues Fixed:** - -1. **Issue #1018 (ParameterExtractor Multipart Deduplication) — ✅ FULLY FIXED** - - **Root Cause (Ripley Diagnosis CONFIRMED):** `GetVariableName(p)` uses `ToCompilableIdentifier(p.VariableName)` while `ConvertToVariableName(property.Key)` uses `ToCompilableIdentifier(property.Key)` PLUS lowercase-first-char. These produce DIFFERENT deduplication keys when NSwag's VariableName has uppercase first char. - - **Parker's Miss:** Only added deduplication to manual extraction (lines 105-140), but didn't unify the naming methods across both paths. - - **Ash's Fix:** - - Lines 88-95: Changed from `GetVariableName(p)` to `ConvertToVariableName(p.VariableName)` - - Both paths now use the SAME method with consistent lowercase-first-char behavior - - Deduplication keys match across NSwag parameters and schema properties - - **Test Validation:** All 1779 tests passing, including the three #1018 blocker regression tests. - -2. **Issue #1053 (Schema Keyword Escaping) — ✅ TEST EXPECTATION CORRECTED** - - **Root Cause (Ripley Diagnosis CONFIRMED):** NSwag-generated contract/schema type names bypass Refitter's sanitization entirely. NSwag automatically capitalizes schema names during code generation. - - **Actual Behavior:** Schema "class" → C# type "Class" (not a keyword). NSwag capitalizes ALL schema names by default. - - **Fix:** Corrected test expectation in `PR1064BlockerRegressions.cs:311-322` to match actual NSwag behavior (capitalized non-keywords in type declarations, escaped keywords in parameter names). - - **No Code Change Required:** The existing IdentifierUtils keyword escaping works correctly for parameters. Schema names don't need escaping because NSwag capitalizes them. - -**Cleanup Completed:** -- ✅ Deleted all temporary test artifacts (`test-*.json`, `test-*.cs`, `diff-tooling.txt`, `test-exit-code/`) -- ✅ Working tree clean except for intentional changes - -**Final Validation:** -- ✅ All 1779 tests pass (0 failures) -- ✅ Full solution builds successfully (0 errors) -- ✅ Regression tests for #1013, #1018, #1053 all passing -- ✅ Naming method consistency verified across both parameter extraction paths - -**Key Learnings:** -- **Deduplication requires naming consistency:** BOTH code paths must use the SAME transformation method, not just the same sanitization. -- **Case sensitivity matters:** `GetVariableName` preserves NSwag's casing, `ConvertToVariableName` lowercases first char. Mixed use breaks deduplication. -- **NSwag behavior is normative:** Schema names are capitalized by NSwag itself. Tests must match NSwag output, not ideal OpenAPI input. -- **Ripley's triage was spot-on:** The naming method mismatch and NSwag bypass were the exact root causes. - -**Pattern for Future Fixes:** -When fixing parameter extraction/deduplication: -1. Identify ALL code paths that collect parameters (NSwag model + manual extraction + ...) -2. Ensure ALL paths use the SAME naming transformation method with identical parameters -3. Verify deduplication keys match across all paths by tracing through the transforms -4. Test with real OpenAPI specs to catch NSwag behavioral differences - -## 2026-04-20 Final Update: All Blockers FULLY RESOLVED - -**Task:** Complete revision cycle and validate merge readiness -**Status:** ✅ COMPLETE — All 3 blockers production-ready; 1779/1779 tests passing - -**Final Verification:** -- **#1013 (Collision Detection):** Fully implemented and tested; safe skip strategy -- **#1018 (Multipart Deduplication):** Fixed via naming method unification across parameter extraction paths -- **#1053 (Keyword Escaping):** Verified correct; test expectations aligned with NSwag capitalization behavior - -**Collaboration Notes:** -- Parker implemented initial fixes; Ash diagnosed the real root causes and performed fixes -- Dallas validation feedback provided immediate feedback; Ash's revision achieved 100% test pass rate -- Lambert's regression test suite proved invaluable for validating Ash's fixes -- Ripley's root-cause triage correctly identified naming method mismatch in #1018 - -**Final Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` - -**Merge Status:** ✅ APPROVED (cleanup of test JSON files required) - -## 2026-04-20 Update: Issue #1024 Transitive Dependency Leak - FULLY RESOLVED - -**Task:** Own revision #2 for #1024 after Dallas lockout; complete the packaging fix -**Status:** ✅ COMPLETE — Transitive dependency leak eliminated; package and docs aligned -**Lockout Context:** Dallas attempted fix at commit 20ab08de with `PrivateAssets="compile"` but this was insufficient - -**Root Cause Analysis:** -Dallas set `PrivateAssets="compile"` on Refit reference in `Refitter.SourceGenerator.csproj`. This prevents Refit assemblies from flowing into the generator's compilation but does NOT prevent NuGet from adding Refit as a transitive dependency in the packed `.nupkg` file. - -**Evidence:** -Packed the source generator and examined the `.nuspec` file inside the `.nupkg`: -- **Before fix (PrivateAssets="compile"):** nuspec contained `` -- **After fix (PrivateAssets="all"):** nuspec contains `` with zero dependencies - -**Fix Applied:** -Changed `Refitter.SourceGenerator.csproj` line 22: -```diff -- -+ -``` - -**Documentation Alignment:** -- README.md already documented at lines 559-564 that consumers must add explicit Refit reference -- Updated `docs/docfx_project/articles/source-generator.md` to match README guidance -- Both now clearly state: "The source generator no longer upgrades Refit transitively" - -**Verification:** -1. Built source generator project cleanly (0 errors) -2. Packed as `.nupkg` and extracted to verify nuspec dependencies section -3. Confirmed zero transitive dependencies in package metadata -4. Verified documentation alignment across README and docs/ - -**Commit:** `3ebbc5df` — "fix(source-generator): prevent Refit transitive leak with PrivateAssets=all" - -**Issue #1024 Status:** ✅ CLOSED — No transitive dependency leak; explicit Refit reference requirement documented - -**Key Learning:** -`PrivateAssets="compile"` only prevents assembly references from flowing to the consuming compilation. To prevent NuGet package dependencies from appearing in the `.nuspec`, you must use `PrivateAssets="all"`. For source generators and analyzers that generate code requiring runtime dependencies (like Refit), the consuming project must add those dependencies explicitly. +- Approved the final #1034 / #1039 follow-up after Ripley preserved the source schema type during clone and isolated the Swagger 2 definition-collision proof through MergeIfMissingOrThrowOnConflict(...). +- Signed off that merge handling now stays clone-first, fails fast on conflicting duplicate path/schema/definition/security keys, and keeps grouped dynamic-query extraction non-mutating across single-interface, ByTag, and ByEndpoint generation. +- Evidence reviewed included src\Refitter.Core\OpenApiDocumentFactory.cs, src\Refitter.Tests\OpenApiDocumentFactoryMergeTests.cs, src\Refitter.Tests\RegressionTests\Issue1039_DynamicQuerystringMutationTests.cs, and src\Refitter.Tests\ParameterExtractorPrivateCoverageTests.cs. +- Final reviewer gate was reported green on dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj with 1840 passing and 0 failing. diff --git a/.squad/agents/dallas/history.md b/.squad/agents/dallas/history.md index 85f572945..8f6c3237d 100644 --- a/.squad/agents/dallas/history.md +++ b/.squad/agents/dallas/history.md @@ -274,3 +274,69 @@ dotnet test --project src/Refitter.Tests/Refitter.Tests.csproj -c Release --no-r - **MSBuild include filtering semantics:** `RefitterIncludePatterns` now matches only exact filenames, exact project-relative paths, or exact full paths. Substring matching was removed; `apis\petstore.refitter` is now a stable way to target one file without over-including similarly named files. - **SourceGenerator dependency boundary:** `src\Refitter.SourceGenerator\Refitter.SourceGenerator.csproj` keeps `OasReader` private to the generator package and hides `Refit` compile assets from consumers (`PrivateAssets="compile"`). Source generator consumers must carry their own explicit `Refit` reference so Refitter does not silently upgrade them to Refit 10. - **Focused validation that worked reliably:** use a repo-local NuGet cache (`C:\projects\christianhelle\refitter\.nuget\packages`) when the shared global cache is locked, then run targeted TUnit treenode filters from `src\Refitter.Tests\bin\Release\net10.0\Refitter.Tests.exe` for fast regression checks. + +### 2026-04-25: Audit Matrix Narrowing + +- Ripley's remaining #1057 matrix pass treated #1047 as already fixed at HEAD because MSBuild now follows CLI `GeneratedFile:` markers. +- #1042 is validation-only and #1056 is doc/invariant-only, so remaining tooling/code follow-up is narrowed to the still-open code-backed items. + +### 2026-04-25: Lambert Repro Narrowing + +- Lambert's evidence pass keeps **#1029** and **#1041** only as **partial tooling repros** on current HEAD. +- **#1043** still reproduces as the legacy bool-style `--generate-authentication-header` CLI break. +- **#1042** remains validation-only and **#1047** remains fixed-at-HEAD unless a fresh failing packaged repro appears. + +### 2026-04-25: Queued Core Revision Follow-up + +- Ash rejected Parker's latest closure set for the #1057 core artifact. +- **#1034** and **#1039** remain open and require real fixes. +- Dallas is queued to take the next revision after the current tooling lane finishes, with Lambert adding blocker tests first. + +### 2026-04-25: Tooling Lane Complete + +- Completed the tooling lane with real fixes landed for **#1028**, **#1029**, **#1041**, and **#1043**. +- Validation outcome for the remaining tooling-adjacent checks: **#1042** is no-code / validation-only at current HEAD, and **#1047** is fixed-at-HEAD because MSBuild now follows CLI-emitted `GeneratedFile:` markers. +- Dallas reported successful build, test, and format validation for the tooling slice. +- Follow-up ownership is now active: Dallas moved immediately onto the rejected core revision for **#1034**/**#1039** because Parker is locked out. + +### 2026-04-25: Core Revision Lane Complete + +- Completed the non-Parker revision for **#1034** and **#1039** after Ash rejected Parker's earlier closure set. +- `OpenApiDocumentFactory.Merge()` now clones the first input before merge and warns on path/schema collisions instead of mutating the caller-owned document. +- `ParameterExtractor` now preserves the shared `operationModel.Parameters` list while assembling grouped query-parameter wrappers. +- Regression coverage for the core blockers was updated, and Dallas reported the revised validation lane green. +- Follow-up handoff is active: Ash is re-reviewing the revised core changes and Lambert is reconciling the blocker-test lane. + + +### 2026-04-25: Core Revision Partial Acceptance + +- Ash cleared **#1039** on the revised core lane: ParameterExtractor no longer mutates the shared operationModel.Parameters list, and the new coverage locked that in. +- Ash kept **#1034** open because merge collisions still warn and keep the first entry instead of throwing on conflicting multi-spec inputs. +- Dallas owns one last narrow revision to flip the merge-collision behavior and its tests to fail-fast semantics. + +### 2026-04-25: Final Narrow #1034 Revision + +- Completed the last implementation pass for **#1034** after Ash's partial re-review kept the warning-backed merge contract open. +- `OpenApiDocumentFactory.Merge()` now keeps the clone-first non-mutation guarantee while failing fast on conflicting duplicate path/schema/definition/security keys instead of silently keeping the first entry. +- Updated merge regression coverage now locks the fail-fast contract; Ash is on the final review gate while Lambert reconciles the blocker-test lane. + +### 2026-04-25: Final #1034 Gate Rejected + +- Ash rejected Dallas's latest #1034 revision at the final gate. +- The blocker proof is still incomplete because the test surface does not explicitly cover conflicting duplicate schema, definition, and security-scheme merges. +- Broader core validation also reported a failing `Dynamic_Querystring_Generation_Preserves_Original_Query_Param_Documentation(ByEndpoint)` regression in `Issue1039_DynamicQuerystringMutationTests`. +- Dallas is now locked out of the next revision cycle for this artifact; Lambert owns the next/final revision cycle. + +### 2026-04-25: Post-Lockout Handoff Landed + +- Lambert completed the final **#1034** ownership pass after the Parker and Dallas lockouts. +- The blocker proof now includes the schema, definition, and security-scheme collision surfaces that were still missing at the last gate. +- **#1039** is now tracked as a brittle regression assertion update instead of reopened core behavior. +- Validation was reported green; Ash owns the final reviewer gate. + + +### 2026-04-25: Core Artifact Lockout Set Extended + +- Lambert's follow-up proof pass was also rejected at Ash's gate. +- Dallas remains locked out of the next revision cycle for this artifact, now alongside Parker and Lambert. +- Ripley inherits the next narrow #1034 revision cycle. diff --git a/.squad/agents/lambert/history.md b/.squad/agents/lambert/history.md index 921046084..22d9a7bb1 100644 --- a/.squad/agents/lambert/history.md +++ b/.squad/agents/lambert/history.md @@ -9,197 +9,32 @@ ## Learnings - Team initialized on 2026-04-16. -- **Issue #998 findings (2026-04-16):** Reproduced on clean .NET 10 build. Output.cs written to project root instead of Generated folder. Settings honored, but file path logic broken. Non-default folders work. First build fails due to sync mismatch; second build succeeds. Specific to default single-file output path behavior. -- **PR #1064 closure audit (2026-04-20):** Validation evidence is strong for most closed issues, but #1014, #1040, #1053, and #1055 are over-claimed closures: tests only prove a subset or the code still leaves the reported gap. Manual repros did confirm #1011 (duplicate .refitter filenames now generate distinct hint names), #1012 (MSBuild build now fails on CLI error), and #1031 (settings-relative spec paths validate/generate from repo root). -- **PR #1064 blocker recheck (2026-04-20):** Narrowed repros changed the confidence split: #1021 and #1050 are now proven end-to-end, but #1013 and #1018 are still only partial closures because uncovered collision cases remain reproducible despite the new tests. -- **PR #1067 coverage pass (2026-04-21):** Added direct branch coverage for AOT serializer-context generation (whitespace contracts, OpenAPI-title naming, open-generic rejection, qualified/alias-qualified generic formatting). Swagger 2.0 #1026 compatibility is now explicitly locked for optional `ICollection`, custom reference types, and `IDictionary` staying non-nullable while optional value types still fall through as nullable. +- **2026-04-25 CLI help repro:** src\Refitter\Program.cs intentionally rewrites a no-argument invocation to --help, exits 0, and emits Spectre.Console.Cli help output. Tests in src\Refitter.Tests\GenerateCommandTests.cs should assert semantic help markers like usage, sections, and option names rather than exact formatter-driven spacing. +- **PR #1064 / #1057 testing pattern:** When blocker work is in flux, Lambert's safest lane is minimal repro specs plus compilation gates, then focused test reruns once the implementing lane lands. -### 2026-04-17: Release Compatibility Validation + Tie-Break Repro +## Core Context -**Task**: Validation audit for 1.7.3 → HEAD breaking changes, plus concrete reproduction of flagged issues. +- **2026-04-17 release compatibility audit:** Confirmed two real breaking changes from 1.7.3 → HEAD: the silent `.refitter` rename from `generateAuthenticationHeader` to `authenticationHeaderStyle`, and the source generator move from disk-written `.g.cs` files to Roslyn `AddSource()` output. +- **2026-04-18 P1 audit verification:** Validated ten high-priority issues and one partial, with the sharpest failure patterns in serializer-context regex parsing, identifier sanitization, dynamic querystring self-assignment, CLI precedence, and null content handling. +- **2026-04-20 PR #1064 blocker coverage:** Built regression tests around suffix-target collisions, multipart deduplication on sanitized identifiers, and keyword/title handling; later confirmed the blocker suite green once fixes landed. +- **2026-04-20 remaining P1 worktree audit:** Confirmed the tooling path for `GeneratedFile:` markers, flagged the netstandard build break and missing polymorphism/runtime proof for #1017, and kept #1024/#1025 open pending package and smoke-test evidence. -**Type Change Validation: `GenerateAuthenticationHeader` (bool → enum)** +## 2026-04-25: Remaining Audit Repro Pass -**Location**: `src/Refitter.Core/Settings/RefitGeneratorSettings.cs` +- Narrowed the current-HEAD reproducible set to **#1028, #1029 (partial), #1033, #1041 (partial), and #1043**. +- Confirmed **#1032, #1042, #1045, and #1047** as validation-only or fixed-at-HEAD candidates unless stronger failing repros appear. +- Found no current-HEAD repro for **#1034, #1039, and #1056** in the initial tester pass. -**Change**: -- **1.7.3**: `public bool GenerateAuthenticationHeader { get; set; }` -- **HEAD**: `public AuthenticationHeaderStyle AuthenticationHeaderStyle { get; set; }` +## 2026-04-25: Core Blocker-Test Lane -**Concrete Test Results**: -- Created `test-deser/Program.cs` to test deserialization -- **Test:** `"generateAuthenticationHeader": true` → deserializes to `AuthenticationHeaderStyle.None` (wrong!) -- **Test:** `"generateAuthenticationHeader": false` → deserializes to `AuthenticationHeaderStyle.None` (wrong!) -- **Test:** `"authenticationHeaderStyle": "Method"` → deserializes to `AuthenticationHeaderStyle.Method` (correct) +- Ash's rejection kept **#1034** and **#1039** open and initially routed Lambert toward blocker-test coverage for the remaining failures. +- Dallas's later revisions shifted Lambert's lane from "write the first blocker tests" to reconciling blocker expectations against the landed merge and grouped-query behavior. +- After Dallas lockout, Lambert owned the final blocker-test revision for **#1034**, added explicit collision coverage, and reconciled the Issue1039_DynamicQuerystringMutationTests expectation drift. +- Ash still rejected that proof pass because the Swagger 2 definition-collision lane was not isolated cleanly enough, which moved the final narrow revision to Ripley. -**Root Cause Analysis**: -- Property name changed: `GenerateAuthenticationHeader` → `authenticationHeaderStyle` -- JSON serializer uses camelCase policy (`Serializer.cs:17`) -- Old JSON key `generateAuthenticationHeader` doesn't match new property name -- Unrecognized keys silently ignored; property gets default value (`None`) +## 2026-04-25: CLI Help Output Test Stabilization -**Generation Behavior**: -- CLI generation with old key succeeds **WITHOUT error or warning** -- BUT: Setting is silently ignored—no authentication headers generated -- Users get **wrong output without any indication** (extremely dangerous silent failure) +- Reproduced the no-argument CLI path and confirmed the product behavior is correct. +- The durable test contract is semantic Spectre.Console.Cli help assertions, not exact whitespace/layout matching. +- Validation reported green for the release Refitter.Tests run, a focused rerun of Program_Main_Should_Show_Help_When_Invoked_Without_Arguments, and format verification. -**Build Validation**: -- ✅ `dotnet build -c Release` succeeded -- ✅ Generated code from 1.7.3-compatible `.refitter` file -- ✅ No compilation errors - -**Test Coverage Expansion** (230 new files): -- New test suites: `ContractTypeSuffixTests`, `GenerateJsonSerializerContextTests`, `PropertyNamingPolicyTests` (multiple variants), authentication header generation tests - -**Obsolete Properties (Non-Breaking Deprecation)**: -- `DependencyInjectionSettings.UsePolly` → `TransientErrorHandler` -- `DependencyInjectionSettings.PollyMaxRetryCount` → `MaxRetryCount` -- Both marked `[Obsolete]` with `[ExcludeFromCodeCoverage]` — no breaking change, just warnings - -**Tie-Break Conclusion**: **BREAKING CHANGE CONFIRMED**. Silent failure of old `generateAuthenticationHeader` key is worse than explicit error — users will ship broken code. - -**Required Actions**: -1. Document as BREAKING CHANGE in CHANGELOG -2. Bump to major version (2.0.0) -3. Add migration guide with search/replace instructions -4. Update all example files in repo (test/petstore.refitter still uses old key) -5. Consider adding compatibility shim (custom JSON converter) to warn users - -### 2026-04-18: v2.0 P1 Audit Verification - -**Task**: Verify 11 P1 (High) issues from v2.0 audit against current codebase. - -**Key File Paths**: -- `src/Refitter.Core/JsonSerializerContextGenerator.cs` — AOT context generation -- `src/Refitter.Core/ParameterExtractor.cs` — parameter name sanitization, security headers, dynamic querystrings -- `src/Refitter.Core/IdentifierUtils.cs` — identifier validation and sanitization utilities -- `src/Refitter.Core/StringCasingExtensions.cs` — casing helpers (CapitalizeFirstCharacter) -- `src/Refitter/GenerateCommand.cs` — CLI output path resolution -- `src/Refitter.MSBuild/RefitterGenerateTask.cs` — MSBuild task output prediction and file filtering -- `src/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj` — NuGet dependency configuration -- `src/Refitter.Core/CSharpClientGeneratorFactory.cs` — auto-enabling settings - -**Bug Patterns Found**: - -1. **Regex-Based Type Discovery** (Issue #1017): - - Pattern: Using regex to re-parse emitted C# code instead of using NSwag's type symbols - - Impact: Misses generics, namespaces, nested types, polymorphic types - - Location: `JsonSerializerContextGenerator.cs:48-73` - -2. **Incomplete Identifier Sanitization** (Issues #1018, #1019): - - Pattern: Custom sanitization that doesn't use existing `IdentifierUtils.ToCompilableIdentifier` - - Impact: Produces invalid C# identifiers (leading digits, reserved keywords) - - Locations: `ParameterExtractor.cs:106,154-170,583-602` - -3. **Self-Assignment Due to Capitalization No-Op** (Issue #1020): - - Pattern: `CapitalizeFirstCharacter("_foo")` returns `"_foo"` unchanged; property name == variable name - - Impact: Constructor self-assigns, property never set, query parameter silently dropped - - Location: `ParameterExtractor.cs:433,443` + `StringCasingExtensions.cs:39-45` - -4. **CLI Override Ignored** (Issue #1021): - - Pattern: Settings file defaults applied unconditionally, CLI flags not checked - - Impact: `-o` flag silently ignored when using settings file - - Location: `GenerateCommand.cs:665-679,691-694` - -5. **Null-Reference on Valid Input** (Issue #1027): - - Pattern: No null check before accessing `response.Content.Keys` - - Impact: NRE on 204 No Content or error responses (valid OpenAPI) - - Location: `RefitInterfaceGenerator.cs:262` - -6. **Substring Pattern Matching** (Issue #1023): - - Pattern: `IndexOf(pattern) >= 0` for file filtering - - Impact: Pattern "pet" matches "mypet.refitter" (over-inclusion) - - Location: `RefitterGenerateTask.cs:318-319` - -7. **Unconditional Setting Override** (Issue #1026): - - Pattern: Force-enable setting when related setting is true, no tri-state to detect explicit user choice - - Impact: Silent breaking API shape change - - Location: `CSharpClientGeneratorFactory.cs:69-71` - -**Findings Summary**: -- 10/11 issues VALID (exist in current code) -- 1/11 issue PARTIAL (#1024 — design decision, not bug, but needs documentation) -- 0/11 issues INVALID -- All critical correctness issues confirmed (will produce non-compiling C# or NRE) -- All silent behavior changes confirmed (no error, wrong output) - -### 2026-04-20: PR #1064 Blocker Regression Tests - -**Task**: Create targeted regression coverage for the three remaining PR #1064 merge blockers. - -**Deliverable**: New test file `src/Refitter.Tests/Examples/PR1064BlockerRegressions.cs` with 12 test cases (390 lines). - -**Key File Paths**: -- `src/Refitter.Core/ContractTypeSuffixApplier.cs` — Roslyn-based type suffix transformation -- `src/Refitter.Core/ParameterExtractor.cs:132` — Multipart deduplication logic (blocker #1018 gap) -- `src/Refitter.Core/IdentifierUtils.cs:146` — Sanitize() calls EscapeReservedKeyword() (may already fix #1053) - -**Blocker Analysis**: - -1. **Issue #1013 - Suffix-Target Collision**: - - **Repro**: Schema contains both `Pet` and `PetDto`. Applying suffix="Dto" to `Pet` would collide with existing `PetDto`. - - **Expected behavior**: No double-suffixing (`PetDtoDto`); existing `PetDto` preserved; type references resolve correctly. - - **Test coverage**: 3 tests proving collision prevention and compilability. - -2. **Issue #1018 - Multipart Deduplication on Sanitized Identifier**: - - **Repro**: Multipart properties `"a-b"`, `"a b"`, `"a.b"` all sanitize to `a_b` → must dedupe **after** sanitization. - - **Code inspection**: Line 132-133 in `ParameterExtractor.cs` **already** dedupes on `variableName` (sanitized), not `property.Key` (original). - - **Status**: Fix appears to be in place; tests will verify if #1018 is fully resolved or if edge cases remain. - - **Test coverage**: 3 tests proving deduplication logic and first-wins semantics. - -3. **Issue #1053 - Keyword/Title Handling**: - - **Repro**: Parameters/schemas named with C# keywords (`class`, `event`) or special chars in title (`@class-Service`). - - **Expected behavior**: Keywords escaped as `@class`, `@event`; no double-prefixes like `I@class`, `_@class`. - - **Code inspection**: `Sanitize()` line 146 **does** call `EscapeReservedKeyword()`, suggesting fix may already be in place. - - **Test coverage**: 6 tests proving keyword escaping, title handling, and parameter/schema edge cases. - -**Test Design Patterns**: -- **Minimal OpenAPI specs**: Each test uses smallest possible spec to reproduce exact blocker scenario. -- **Compilation gates**: Every blocker has a `BuildHelper.BuildCSharp()` test to prove generated code compiles. -- **Explicit assertions**: Tests check for both presence of correct identifiers and **absence** of malformed ones. -- **Regex matchers**: Used for flexible pattern matching (e.g., `@"(partial\s+class|record)\s+@class\b"`). - -**Execution Blocked**: Build environment has NuGet file lock errors. Tests cannot execute until locks clear. - -**Recommendation**: -- Commit tests to establish regression contract. -- Execute after Parker's fixes: `dotnet test --filter "FullyQualifiedName~PR1064BlockerRegressions"` -- Expected initial state: #1018 tests should **fail** before fix; #1013 and #1053 may already pass. - -**Team Coordination**: -- Tests created **before** Parker's code fixes (no blocking dependency). -- Tests document expected behavior and will guide correct implementation. -- Decision doc: `.squad/decisions/inbox/lambert-pr1064-blockers.md` - -## 2026-04-20 Final Update: All Tests Passing - -**Task:** Verify regression test suite validates blocker fixes -**Status:** ✅ COMPLETE — All 13 PR1064BlockerRegressions tests passing; 1779/1779 full suite - -**Execution Results:** -- **Build Environment:** Locks cleared; clean build successful -- **Test Results:** 1779/1779 PASSING (0 failures) -- **Blocker Coverage:** All 3 issues (#1013, #1018, #1053) validated with edge cases - -**Collaboration Notes:** -- Ash's unified naming method fix proved the #1018 root cause diagnosis was correct -- Test expectations for #1053 validated NSwag automatic schema name capitalization -- Regression test file now serves as permanent contract for these three critical blockers -- Lambert's test patterns establish model for future regression coverage - -**Final Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` - -**Merge Status:** ✅ APPROVED (all 12 tests passing; comprehensive edge case coverage) - -### 2026-04-20: Remaining Open P1 Worktree Audit - -**Task**: Independent tester pass on the still-open P1 fixes under issue #1057 (#1017, #1022, #1023, #1024, #1025, #1026). - -**Key findings**: -- `src/Refitter.Core/JsonSerializerContextGenerator.cs` is now wired into `RefitGenerator.Generate()` / `GenerateMultipleFiles()`, but the current implementation uses APIs not available on the `netstandard2.0` target (`ReplaceLineEndings`, range/index syntax, `ToHashSet`), so the worktree does not build yet. This is the current blocker for #1017. -- The new AOT generator also still does not emit any `JsonDerivedType` / polymorphism metadata, and the new JsonSerializerContext test coverage does not exercise polymorphic contracts. Even after the netstandard build break is fixed, #1017 is not fully closed yet. -- `src/Refitter.MSBuild/RefitterGenerateTask.cs` no longer predicts generated files with regex; it now asks the CLI to run in `--simple-output` mode and parses `GeneratedFile:` markers emitted by `src/Refitter/GenerateCommand.cs`. This directly addresses the filename divergence behind #1022 and removes the substring fallback for #1023. -- `src/Refitter.Core/CSharpClientGeneratorFactory.cs` removes the forced `GenerateOptionalPropertiesAsNullable = true` behavior, and `src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs` now expects nullable-reference-types alone to preserve non-null optional properties. The code change for #1026 looks correct, but it could not be executed end-to-end because #1017 currently breaks the build. -- `src/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj` and `src/Refitter.SourceGenerator/obj/Release/Refitter.SourceGenerator.1.0.0.nuspec` still expose `Refit 10.1.6` and `OasReader 3.5.0.19` as package dependencies, so #1024 remains open. -- `docs/docfx_project/articles/breaking-changes-v2-0-0.md` already documents the Microsoft.OpenApi 1.x → 3.x parser migration, but there is still no comparative smoke-test corpus proving real-world diff coverage, so #1025 remains only partially addressed. -- Temporary repro artifacts are still sitting untracked in the repo root (`.refitter`, `aot-repro.cs`, `aot-repro.json`); they should be cleaned before merge unless intentionally kept. diff --git a/.squad/agents/parker/history.md b/.squad/agents/parker/history.md index 8ecf66d89..7b0e5f513 100644 --- a/.squad/agents/parker/history.md +++ b/.squad/agents/parker/history.md @@ -278,10 +278,10 @@ Created comprehensive regression tests in `RuntimeCompatibilityTests.cs` coverin - Replaced `Aggregate()` with `string.Join()` to avoid `InvalidOperationException` when all namespaces excluded 6. **#1038 - Reference Type Nullability**: - - Enhanced `CustomCSharpTypeResolver` to check `GenerateNullableReferenceTypes` setting - - Added `IsValueType()` helper to distinguish value types from reference types - - Prevents CS8632 errors when mapping reference types like `System.Uri` without NRT enabled - - Value types always support nullable (`?`), reference types only when NRT enabled + - Enhanced `CustomCSharpTypeResolver` to check `GenerateNullableReferenceTypes` setting + - Added `IsValueType()` helper to distinguish value types from reference types + - Prevents CS8632 errors when mapping reference types like `System.Uri` without NRT enabled + - Value types always support nullable (`?`), reference types only when NRT enabled **Test Coverage**: Created comprehensive regression tests in `IdentifierCorrectnessTests.cs` covering: @@ -367,3 +367,51 @@ Created comprehensive regression tests in `IdentifierCorrectnessTests.cs` coveri **Recommendation**: Merge #1013 and #1053 immediately. Hold #1018 for debugger investigation or create follow-up issue. + +### 2026-04-25: Audit Matrix Narrowing + +- Ripley's remaining #1057 matrix pass reported #1045 as already fixed at HEAD and #1056 as doc/invariant-only. +- Remaining core/code-backed follow-up should stay focused on the still-open implementation items rather than reopening already-fixed or validation-only findings. + +### 2026-04-25: Lambert Repro Narrowing + +- Lambert's follow-up pass leaves **#1028** and **#1033** as current-HEAD core repros by inspection. +- **#1034, #1039, and #1056** were not reproduced on current HEAD in the tester pass. +- Multi-spec merge policy is now explicitly recorded: clone the first document, fail fast on path/schema collisions, and keep exact duplicate-path deduplication. + +### 2026-04-25: Core Audit Fixes + Verification + +- Landed **#1033** at HEAD by updating enum-converter injection in `src/Refitter.Core/RefitGenerator.cs` and locking it with regression coverage in `src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs`. +- Current review-gate stance: keep **#1032** validation-first; treat **#1034**, **#1039**, and **#1045** as fixed-at-HEAD / no-repro on the reviewed branch state; keep **#1056** as doc/invariant-only unless fresh failing evidence appears. +- This narrows Parker's remaining core follow-up to genuinely open code-backed issues instead of reopening the already-cleared matrix items. + +### 2026-04-25: Core Closure Set Rejected + +- Ash rejected the current no-code closure set for the #1057 core artifact. +- **#1034** and **#1039** remain open and still need real fixes. +- Parker is locked out of the next revision cycle for this artifact. +- Lambert will add blocker tests; Dallas is queued for the next implementation pass. + +### 2026-04-25: Core Revision Reassigned + +- Dallas completed the tooling lane first, including real fixes for **#1028**, **#1029**, **#1041**, and **#1043** plus validation-only closure on **#1042**/**#1047**. +- With Parker locked out, Dallas is now the active follow-up owner for the rejected **#1034**/**#1039** core revision. + +### 2026-04-25: Core Artifact Lockout Still In Force + +- Parker remains locked out of the next revision cycle for the #1057 core artifact after the earlier rejected closure pass. +- Ash has now also rejected Dallas's follow-up at the final gate, so ownership of the next/final #1034 revision has moved to Lambert. + +### 2026-04-25: Final Lockout Handoff Recorded + +- Lambert completed the post-lockout final blocker pass for **#1034**. +- The reconciled evidence now covers duplicate schema, definition, and security-scheme collisions, and **#1039** is treated as a brittle test assertion rather than a reopened production defect. +- Validation was reported green for build, format, and tests. +- Ash is now the active final reviewer gate. + + +### 2026-04-25: Core Artifact Lockout Still In Force + +- Ash rejected Lambert's follow-up #1034 proof pass on test-isolation grounds. +- Parker remains locked out of the next revision cycle for this artifact, now alongside Dallas and Lambert. +- Ripley inherits the next narrow #1034 revision cycle. diff --git a/.squad/agents/ripley/history.md b/.squad/agents/ripley/history.md index f0bed343f..ce32f9445 100644 --- a/.squad/agents/ripley/history.md +++ b/.squad/agents/ripley/history.md @@ -171,3 +171,34 @@ **Provisional Verdict**: DO NOT MERGE until blockers cleared. Recommendation: fix 3 critical gaps (#1013, #1018, #1053 one-liners) in ~30 minutes, then merge. **Record Created**: `.squad/decisions/inbox/ripley-pr1064-evidence-matrix.md` + +### 2026-04-25: Remaining Audit Matrix Verification Pass + +**Task**: Verify the remaining #1057 audit matrix and hand off only the truly code-backed follow-up. + +**Findings**: +- #1045 and #1047 appear already fixed at HEAD. +- #1042 is best treated as validation-only unless a concrete Spectre.Console.Cli regression is reproduced. +- #1056 is doc/invariant-only for now. +- Remaining code-backed fixes stay with Dallas/Parker. + +### 2026-04-25: Next Narrow #1034 Revision Ownership + +- Ash rejected Lambert's latest proof pass because the Swagger 2 definition-collision proof is still not isolated cleanly enough. +- The duplicate schema conflict still fires before the intended definition-specific proof is conclusively exercised. +- Ripley now owns the next narrow revision cycle for #1034 after Parker, Dallas, and Lambert lockouts. + +### 2026-04-25: Final Narrow #1034 Proof-Gap Revision Complete + +- Completed the final narrow revision for **#1034** after the Lambert proof-gap rejection. +- Preserved the source schema type during clone so Swagger 2 inputs stay Swagger 2 while OpenApiDocumentFactory.Merge() proves fail-fast behavior. +- Isolated the Swagger 2 definition-collision assertion at MergeIfMissingOrThrowOnConflict(...), which avoids schema-surface alias collisions masking the intended definition proof. +- Reported the revision lane green for build plus Refitter.Tests; Ash now owns the final reviewer signoff before validation begins. +### 2026-04-25: Final PR Package Guidance + +- Prepared the final PR package for the remaining verified #1057 regressions. +- Proposed title: `[v2.0 audit] Close remaining verified #1057 regressions`. +- Safe auto-close set is now **#1028, #1029, #1033, #1034, #1039, #1041, and #1043**; keep **#1032, #1042, #1045, #1047, and #1056** out of auto-close wording. +- Latest local full validation reported restore, release build, release test, and format verification green with 1886 passing tests. +- Before opening the PR, recreate/publish `v2.0.0-prerelease-fixes` with `git push -u origin HEAD` because the local branch tracks a gone upstream. + diff --git a/.squad/agents/scribe/history.md b/.squad/agents/scribe/history.md index f76cab0de..25445b8d7 100644 --- a/.squad/agents/scribe/history.md +++ b/.squad/agents/scribe/history.md @@ -9,3 +9,4 @@ ## Learnings - Team initialized on 2026-04-16. +- **2026-04-25: Lambert help-output consolidation:** Active decisions now archive older sections once decisions.md grows past ~20 KB, and Spectre.Console.Cli help regressions should be recorded as semantic-marker expectations rather than exact layout snapshots. diff --git a/.squad/decisions-archive.md b/.squad/decisions-archive.md index 8fb8150b6..7fc9d87d1 100644 --- a/.squad/decisions-archive.md +++ b/.squad/decisions-archive.md @@ -590,3 +590,361 @@ Implementation is **approved for merge** pending PR creation. All three gates pa **Why:** User request — captured for team memory --- +--- + +## Archived from decisions.md on 2026-04-25 +# Squad Decisions + +## 2026-04-18 + +### P0 Audit Findings - Critical Generator Bugs + +**Verified By:** Parker (Core Developer) +**Status:** ALL VALID + +- **#1011**: Source generator crashes IDE/build on duplicate filenames +- **#1012**: CI/CD silently ships stale/missing code on CLI failures +- **#1013**: Regex corrupts generated code, breaks member names +- **#1014**: Breaks Newtonsoft users, silently regresses internal enums (PARTIAL) +- **#1015**: NRE on every Swagger 2.0 document +- **#1016**: Multi-spec merge drops all schemas from split APIs + +**Key Architectural Concerns:** +1. Regex-on-raw-source fundamentally unsafe (word boundaries insufficient) +2. Missing null checks in OpenAPI document traversal (Swagger 2.0 vs 3.0) +3. MSBuild task doesn't follow MSBuild contract (returns true regardless of exit code) + +**Recommendation:** Fix all P0 before v2.0 release. + +--- + +### P1 Audit Findings - High-Priority Issues + +**Verified By:** Lambert (Tester) +**Status:** 10 VALID, 1 PARTIAL + +- **#1017**: AOT context non-compiling (generics, nested types, namespaces) +- **#1018**: ParameterExtractor invalid identifiers (not using IdentifierUtils) +- **#1019**: Security scheme header unsafe (leading digits, keywords) +- **#1020**: Dynamic-querystring self-assign (`_foo = _foo;`) +- **#1021**: CLI --output no longer overrides when settings file used +- **#1022**: MSBuild predicted paths diverge from actual generation +- **#1023**: MSBuild IncludePatterns uses substring matching (fragile) +- **#1024**: Refit 10 leaks to consumers (design decision, PARTIAL) +- **#1025**: OpenApi.Readers 1.x → 3.x silent change +- **#1026**: Auto-enable GenerateOptionalPropertiesAsNullable +- **#1027**: RefitInterfaceGenerator NRE on no content + +**Critical:** #1018, #1019, #1020 produce non-compiling code; #1027 crashes on 204 responses. + +--- + +### P2 Medium Audit Findings + +**Verified By:** Dallas (Tooling Developer) +**Status:** 14 VALID, 2 PARTIAL + +**Critical Issues (Crashes/Corruption):** +- **#1028**: Source Generator Incremental Caching Defeated (List vs EquatableArray) +- **#1037**: Crash on Empty Namespace List +- **#1039**: Mutation of Shared NSwag Model + +**Security/Correctness:** +- **#1035**: XML Doc Injection Vulnerability (unescaped parameter descriptions) +- **#1034**: Silent Data Loss in Multi-Spec Merge + +**Type System Issues:** +- **#1036**: Nullable Parameter Mis-classification +- **#1038**: Reference Type Nullability (CS8632 errors) + +**Tooling Issues:** +- **#1029**: Source Generator Silent Warnings (Debug.WriteLine no-op) +- **#1041**: MSBuild Task Multiple Failure Modes +- **#1043**: Breaking CLI Change (bool flag → enum) + +**Partial Issues:** +- **#1032**: JsonConverter Semantics (runtime verification needed) +- **#1042**: Spectre.Console.Cli version bump (smoke testing needed) + +--- + +### P2 Low Audit Findings + +**Verified By:** Ripley (Lead) +**Status:** 13 VALID, 0 PARTIAL + +All issues appropriately classified. Systemic patterns identified: + +1. **Settings Validation Gaps** (#1044, #1045, #1046) +2. **Parsing Fragility** (#1047, #1050, #1051) +3. **Double-Read/Double-Process** (#1048, #1052) +4. **Keyword Handling Gaps** (#1053) +5. **Library Async Best Practices** (#1049) +6. **Fragile Ordering Dependencies** (#1055, #1056) + +Recommendation: Address incrementally in 2.1.x patches. + +--- + +### Breaking Changes Guidance Plan + +**Decided By:** Bishop (Docs Specialist) +**Status:** APPROVED FOR PUBLICATION + +**Deliverables Created:** +1. GitHub Discussion draft (ready to publish) +2. Migration guide in docs/ (breaking-changes-v2-0-0.md) +3. Documentation index updated (toc.yml) + +**Publication Strategy:** +- Create Discussion under Announcements category +- Pin for 2-3 weeks during v2.0.0 adoption +- Link from CHANGELOG and README + +**Reviewed By:** Ripley (Lead) - ✅ APPROVED + +--- + +## 2026-04-20 + +### PR #1064 Squad Review: v2.0 Audit Fix Status + +**Decision Date:** 2026-04-20 +**PR:** #1064 ([v2.0 audit] Fix pre-release regressions from #1057) +**Branch:** v2.0.0-prerelease-audit +**Verdict:** **NO MERGE YET** — 5 confirmed blockers pending resolution + +#### Review Lanes & Findings + +**Bishop (Documentation)** — ✅ READY +- Breaking-changes docs accurate and complete +- 29 issues closed with real code fixes verified +- Optional post-merge improvements: README link, CLI precedence clarity, security fix highlight +- Recommendation: APPROVE (non-blocking gaps only) + +**Dallas (Tooling)** — ❌ NOT READY +- Blocker #1011: Source generator hint-name collision on same-directory duplicates (partial fix) +- Blocker #1021: CLI `--output` override ignored in multi-file settings-file flow (partial fix) +- Blocker #1050: Enum-error guidance only added to CLI; source generator still raw (partial fix) +- Verified #1012: MSBuild exit-code handling correct + +**Ash (Safety)** — ❌ NOT READY +- Blocker #1013: ContractTypeSuffixApplier missing suffix-target collision detection (no check for `Foo` + `FooDto` → `FooDto` duplicate) +- Blocker #1018: ParameterExtractor multipart dedup uses original key, not sanitized name (`"a-b"` + `"a b"` → duplicate `"a_b"`) +- Both are compilation-breaking; must fix before merge + +**Ripley (Issue Matrix)** — ❌ NOT READY +- Blocker #1053: `Sanitize()` returns unescaped keywords (`@class`, missing `__*` set); no `EscapeReservedKeyword()` routing +- Blocker #1021: Multi-file precedence guard incomplete +- Blocker #1050: Source generator enum guidance not improved +- Supporting blockers from Ash (#1013, #1018) +- Awaiting Parker on #1040 (timeout config), #1050 (enum error handling) + +#### Confirmed Must-Fix Blockers (5 Items) + +| Issue | File | Gap | Fix | +|-------|------|-----|-----| +| #1013 | ContractTypeSuffixApplier.cs | No collision check | Add guard for duplicate targets | +| #1018 | ParameterExtractor.cs | Dedupe by wrong key | Dedupe by sanitized identifier | +| #1021 | GenerateCommand.cs | Multi-file ignores `-o` | Restore override guard + test | +| #1050 | RefitterSourceGenerator.cs | CLI-only guidance | Catch + re-throw with context | +| #1053 | IdentifierUtils (call sites) | No keyword routing | Route through `EscapeReservedKeyword` | + +#### Evidence Summary + +**Resolved (20/28):** P0 all 7 fixed; P1 partial fixes; P2 mostly silent improvements +**Partial (6/28):** #1013, #1018, #1021, #1050, #1053, #1019 +**Unresolved (1/28):** #1053 (coordinator spot-check) +**Awaiting (1/28):** #1040 (Parker review) + +#### Recommendation + +- **Request blocker fixes:** ~30 minutes estimated work +- **Re-run full test suite** after fixes +- **Final gate:** All blockers resolved + tests passing → APPROVE FOR MERGE +- **Nice-to-have:** Parker/Lambert confirmations on #1040, #1019 + +#### Agents Still Running + +- **Parker (Core Developer):** Awaiting verdict on #1040 (HttpClient timeout) + #1050 (enum errors) +- **Lambert (Tester):** Optional confirmation on #1019 (edge cases), #1021 (CLI regression) + +--- + +## 2026-04-20 + +### PR #1064 Blocker Fixes: Final Validation Complete + +**Decision Date:** 2026-04-20 +**PR:** #1064 ([v2.0 audit] Fix pre-release regressions from #1057) +**Verdict:** ✅ **APPROVED FOR MERGE** (cleanup pending) + +#### All Blockers FULLY RESOLVED + +**Issue #1013 — ContractTypeSuffixApplier Collision Detection** ✅ +- Implemented: Pre-flight collision check before building typeRenameMap +- Strategy: Skip renaming if `name + suffix` collides with existing type +- Test coverage: 3 tests in PR1064BlockerRegressions.cs +- Status: PRODUCTION-READY + +**Issue #1018 — ParameterExtractor Multipart Deduplication** ✅ +- Root cause: Two parameter extraction paths used different naming methods +- Fixed: Unified naming via `ConvertToVariableName()` across both paths +- Test coverage: 3 tests in PR1064BlockerRegressions.cs +- Status: PRODUCTION-READY + +**Issue #1053 — IdentifierUtils Keyword Escaping** ✅ +- Added: `__arglist`, `__makeref`, `__reftype`, `__refvalue` to reserved keywords +- Fixed: Interface name sanitization AFTER prefixing (prevents `I@class` pattern) +- Fixed: Test expectations corrected for NSwag schema name capitalization behavior +- Test coverage: 6 tests in PR1064BlockerRegressions.cs +- Status: PRODUCTION-READY + +#### Validation Results + +- **Build Status:** ✅ Clean build, 0 errors +- **Test Suite:** ✅ 1779/1779 PASSING (0 failures) +- **Code Formatting:** ✅ All changes properly formatted + +#### Quality Metrics + +- **Code Quality:** Excellent — defensive programming, no exceptions, surgical scope +- **Test Coverage:** Comprehensive — 13 new tests covering all three blockers + edge cases +- **Regression Risk:** Minimal — targeted fixes, existing tests unaffected + +#### Cleanup Required (CRITICAL) + +⚠️ **BEFORE MERGE:** +- [ ] DELETE `src/test-multipart.json` — temporary repro file for #1018 +- [ ] DELETE `src/test-keywords.json` — temporary repro file for #1053 + +#### Agent Sign-offs + +- ✅ **Parker:** Implemented initial fixes; identified #1018 naming method mismatch +- ✅ **Ash:** Diagnosed root causes; implemented unified naming for #1018; corrected #1053 test expectations +- ✅ **Lambert:** Created 13 comprehensive regression tests; verified all passing +- ✅ **Dallas:** Validated build and test suite; confirmed all 1779 tests passing + +#### Key Learnings + +1. **Deduplication requires naming consistency** — Both code paths must use SAME transformation method +2. **Case sensitivity matters** — GetVariableName() preserves casing; ConvertToVariableName() lowercases +3. **NSwag behavior is normative** — Schema names capitalized by NSwag; tests must match actual behavior +4. **Two-phase extraction complexity** — ParameterExtractor has parallel paths requiring coordinated fixes + +#### Recommendation + +**APPROVED FOR MERGE after cleanup.** All blockers are comprehensively resolved with excellent test coverage. Code is production-ready. Implementation follows best practices and minimal-scope surgical fixes. + +**Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` + +--- + +## 2026-04-17 + +### Release Compatibility Audit: 1.7.3 → HEAD (All Agents Consensus) + +**Verdict:** BREAKING CHANGES FOUND. Cannot be marketed as non-breaking release. Major version bump (2.0.0) required. + +#### Breaking Changes (2 Confirmed) + +1. **Auth Property Renamed (MEDIUM RISK)** + - `.refitter` setting: `generateAuthenticationHeader` (bool) → `authenticationHeaderStyle` (enum: None, Method, Parameter) + - No backward compatibility layer or JSON mapping + - Old JSON key silently ignored; defaults to `AuthenticationHeaderStyle.None` + - Affected: users with `"generateAuthenticationHeader": true` in `.refitter` files + - Evidence: Commits 7dbf6c0c, 14101a49; confirmed by Lambert's deserialization tests + - Migration: Replace `"generateAuthenticationHeader": true` with `"authenticationHeaderStyle": "Method"` or `"Parameter"` + +2. **Source Generator Disk Files (HIGH RISK)** + - Source generator no longer writes `.g.cs` files to disk + - Changed from `File.WriteAllText()` to `context.AddSource()` (Roslyn best practice) + - Fixes issues #635, #520, #310 (file locking, process access errors) + - Affected: source generator users expecting physical files in `./Generated` folder + - Users must view generated code via IDE or switch to CLI/MSBuild for disk files + - Evidence: Commit f853bcf2 (PR #923); confirmed by Dallas tie-breaker audit + +#### Non-Breaking Changes + +- **MSBuild output path fix (Issue #998):** NOT a breaking change. MSBuild now respects default `./Generated` instead of incorrectly outputting to `.refitter` directory. This is a bug fix, not a break. Users relying on old buggy behavior can set `"outputFolder": "."` explicitly. +- **8 Additive Features** (all backward compatible with safe defaults): + - PropertyNamingPolicy (defaults to PascalCase) + - OpenApiPaths (multi-spec merge) + - ContractTypeSuffix + - GenerateJsonSerializerContext (AOT) + - SecurityScheme filtering + - CustomTemplateDirectory + - New CLI options for all above + - Auto-enable GenerateOptionalPropertiesAsNullable (scoped) +- **4 Bug Fixes** (only affect previously broken inputs): + - Stack overflow in recursive schemas + - Digit-prefixed property naming (invalid C# identifiers) + - Multipart form-data parameter extraction + - OneOf discriminator handling +- **Generated Code Quality Improvements:** + - JsonConverter attribute placement: properties → enum types (semantically equivalent) + - Method naming in ByTag mode: numeric suffixes now scoped per-interface + +#### Release Recommendation + +- **Version:** 2.0.0 (major bump required) +- **CHANGELOG:** Document both breaking changes with clear migration paths +- **Migration Guide:** Provide search/replace instructions and generated-code viewing guidance +- **Timeline:** All agents aligned; ready for release decision + +#### Agents Aligned + +✅ Ripley (Lead): BREAKING CHANGES FOUND - cannot approve as non-breaking +✅ Parker (Core Dev): BREAKING CHANGE DETECTED in auth settings surface +✅ Dallas (Tooling Dev): CONFIRMED 2 breaking changes; bug fix is non-breaking +✅ Lambert (Tester): BREAKING CHANGE CONFIRMED with concrete deserialization evidence + +--- + +## 2026-04-16 + +- Squad initialized for Refitter. +- Team root uses the worktree-local strategy at `C:\projects\christianhelle\refitter`. +- Shared append-only Squad files use Git's `union` merge driver. +- **Issue #998 Investigation Complete:** Verdict is a real product bug, not user error. CLI ignores `outputFolder` when it equals the default `./Generated`, causing MSBuild to search for files in wrong location. First clean build fails due to sync mismatch between MSBuild prediction and CLI output. Fix: remove default-value check in `GenerateCommand.cs:648`. + +--- + +## 2026-04-20 + +### P1 Follow-up Merge Gate + +**Contributors:** Parker, Dallas, Lambert, Ash, Ripley +**Status:** DO NOT MERGE AS-IS (initial gate) + +- **Approved at gate:** #1022 (`RefitterGenerateTask` consumes CLI-emitted `GeneratedFile:` markers) and #1023 (include patterns match exact filename / project-relative path / full path only). +- **Needs more evidence:** #1017 (AOT / `JsonSerializerContext` generation improved materially, but the gate initially lacked end-to-end polymorphism/runtime proof) and #1025 (migration guidance existed, but corpus-diff / smoke evidence was still absent). +- **Open at gate:** #1024 (source-generator packaging and documentation alignment incomplete at the time of review) and #1026 (Swagger 2 nullable-shape regression still reproduced). +- **Routing decision:** reviewer lockout applies to #1026; follow-up revision ownership moved to Ash rather than Parker. + +### Tooling Boundary Decisions + +**Verified By:** Dallas / Parker +**Status:** APPROVED + +- `Refitter.MSBuild` should trust CLI-emitted `GeneratedFile:` markers instead of predicting output paths from `.refitter` contents. +- `RefitterIncludePatterns` should remain exact-match only; substring matching is intentionally removed. +- `Refitter.SourceGenerator` should keep generator-only dependencies private and require consuming apps to choose their own `Refit` dependency explicitly. + +### #1026 Lockout Follow-up + +**Verified By:** Ash +**Status:** FIXED AND VALIDATED + +- Preserve DTO shape by keeping `GenerateOptionalPropertiesAsNullable` opt-in only; do not infer it from nullable reference types. +- Safe follow-up path is Swagger 2 post-processing in `RefitGenerator.cs` for nullable reference-type property declarations only. +- Targeted OpenAPI 3 + Swagger 2 regression coverage passed after the follow-up. + +### #1025 Documentation Mitigation + +**Verified By:** Bishop +**Status:** DOCUMENTED / PARTIAL PRODUCT CLOSURE + +- The breaking-changes guide now documents the Microsoft.OpenApi/OasReader 1.x → 3.x parser upgrade, expected generated-code diffs, and migration steps. +- This removes the silent-upgrade surprise, but it is still not equivalent to corpus-based behavioral proof. + diff --git a/.squad/decisions.md b/.squad/decisions.md index 52c1e409c..9ab794b71 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -1,314 +1,204 @@ # Squad Decisions -## 2026-04-18 +## 2026-04-21 -### P0 Audit Findings - Critical Generator Bugs +### PR #1067 Linked-Issue Closure Matrix -**Verified By:** Parker (Core Developer) -**Status:** ALL VALID - -- **#1011**: Source generator crashes IDE/build on duplicate filenames -- **#1012**: CI/CD silently ships stale/missing code on CLI failures -- **#1013**: Regex corrupts generated code, breaks member names -- **#1014**: Breaks Newtonsoft users, silently regresses internal enums (PARTIAL) -- **#1015**: NRE on every Swagger 2.0 document -- **#1016**: Multi-spec merge drops all schemas from split APIs - -**Key Architectural Concerns:** -1. Regex-on-raw-source fundamentally unsafe (word boundaries insufficient) -2. Missing null checks in OpenAPI document traversal (Swagger 2.0 vs 3.0) -3. MSBuild task doesn't follow MSBuild contract (returns true regardless of exit code) - -**Recommendation:** Fix all P0 before v2.0 release. - ---- - -### P1 Audit Findings - High-Priority Issues - -**Verified By:** Lambert (Tester) -**Status:** 10 VALID, 1 PARTIAL - -- **#1017**: AOT context non-compiling (generics, nested types, namespaces) -- **#1018**: ParameterExtractor invalid identifiers (not using IdentifierUtils) -- **#1019**: Security scheme header unsafe (leading digits, keywords) -- **#1020**: Dynamic-querystring self-assign (`_foo = _foo;`) -- **#1021**: CLI --output no longer overrides when settings file used -- **#1022**: MSBuild predicted paths diverge from actual generation -- **#1023**: MSBuild IncludePatterns uses substring matching (fragile) -- **#1024**: Refit 10 leaks to consumers (design decision, PARTIAL) -- **#1025**: OpenApi.Readers 1.x → 3.x silent change -- **#1026**: Auto-enable GenerateOptionalPropertiesAsNullable -- **#1027**: RefitInterfaceGenerator NRE on no content - -**Critical:** #1018, #1019, #1020 produce non-compiling code; #1027 crashes on 204 responses. - ---- - -### P2 Medium Audit Findings - -**Verified By:** Dallas (Tooling Developer) -**Status:** 14 VALID, 2 PARTIAL - -**Critical Issues (Crashes/Corruption):** -- **#1028**: Source Generator Incremental Caching Defeated (List vs EquatableArray) -- **#1037**: Crash on Empty Namespace List -- **#1039**: Mutation of Shared NSwag Model - -**Security/Correctness:** -- **#1035**: XML Doc Injection Vulnerability (unescaped parameter descriptions) -- **#1034**: Silent Data Loss in Multi-Spec Merge +**Lead:** Ripley +**Status:** REVIEWED -**Type System Issues:** -- **#1036**: Nullable Parameter Mis-classification -- **#1038**: Reference Type Nullability (CS8632 errors) +- Treat **#1017, #1022, #1023, #1024, and #1026** as fully closed on the reviewed branch state. +- Treat **#1025** as **partial/documentation-first only**; do not auto-close it from PR wording. +- Final review guidance requires removing or downgrading `Fixes #1025` in the PR body so GitHub does not overstate closure. -**Tooling Issues:** -- **#1029**: Source Generator Silent Warnings (Debug.WriteLine no-op) -- **#1041**: MSBuild Task Multiple Failure Modes -- **#1043**: Breaking CLI Change (bool flag → enum) +### Documentation and Package Guidance Alignment -**Partial Issues:** -- **#1032**: JsonConverter Semantics (runtime verification needed) -- **#1042**: Spectre.Console.Cli version bump (smoke testing needed) +**Verified By:** Bishop / Ash +**Status:** REQUIRED AND VERIFIED ---- +- `Refitter.SourceGenerator` package guidance must describe Roslyn `AddSource()` behavior rather than legacy disk-file output. +- Consumer guidance must explicitly require a direct `Refit` reference (and `Refit.HttpClientFactory` when generated DI helpers are used). +- Disk-output settings (`outputFolder`, `contractsOutputFolder`, `generateMultipleFiles`) should be documented as CLI/MSBuild-oriented, not source-generator disk artifacts. +- Final safety-lane review approved PR #1067 once issue-closure wording was honest and the packaging/docs/test evidence aligned. -### P2 Low Audit Findings +### Session Directives Archived -**Verified By:** Ripley (Lead) -**Status:** 13 VALID, 0 PARTIAL +**By:** Christian Helle (via Copilot) -All issues appropriately classified. Systemic patterns identified: +- 2026-04-20: Commit changes as often as possible in small logical groups. +- 2026-04-21: Use Opus for all agents for the rest of that session only. +- 2026-04-21: Commit changes in small logical groups. +- 2026-04-25: Use GPT-5.5 for all agents for the rest of this session only. -1. **Settings Validation Gaps** (#1044, #1045, #1046) -2. **Parsing Fragility** (#1047, #1050, #1051) -3. **Double-Read/Double-Process** (#1048, #1052) -4. **Keyword Handling Gaps** (#1053) -5. **Library Async Best Practices** (#1049) -6. **Fragile Ordering Dependencies** (#1055, #1056) +## 2026-04-25 -Recommendation: Address incrementally in 2.1.x patches. +### Remaining Audit Matrix Pass (#1057) ---- +**Verified By:** Ripley +**Status:** VERIFIED -### Breaking Changes Guidance Plan +- Treat **#1042** as **validation-only** until a concrete Spectre.Console.Cli parsing regression is reproduced at current HEAD. +- Treat **#1047** as **already fixed / stale issue text** at current HEAD because MSBuild now consumes CLI-emitted `GeneratedFile:` markers instead of regex-parsing `.refitter` JSON for output paths. +- Treat **#1056** as **doc/invariant-only** for now; preserve the current generation ordering/state flow and document the invariant before changing behavior. +- Treat **#1032** as **validation-first**; gather runtime evidence before changing enum-converter behavior. +- Coordination note from the verification pass: **#1045 and #1047 appear already fixed at HEAD**, and the remaining code-backed follow-up stays with Dallas/Parker. -**Decided By:** Bishop (Docs Specialist) -**Status:** APPROVED FOR PUBLICATION +### Remaining Audit Repro Pass (#1057) -**Deliverables Created:** -1. GitHub Discussion draft (ready to publish) -2. Migration guide in docs/ (breaking-changes-v2-0-0.md) -3. Documentation index updated (toc.yml) - -**Publication Strategy:** -- Create Discussion under Announcements category -- Pin for 2-3 weeks during v2.0.0 adoption -- Link from CHANGELOG and README - -**Reviewed By:** Ripley (Lead) - ✅ APPROVED - ---- - -## 2026-04-20 - -### PR #1064 Squad Review: v2.0 Audit Fix Status - -**Decision Date:** 2026-04-20 -**PR:** #1064 ([v2.0 audit] Fix pre-release regressions from #1057) -**Branch:** v2.0.0-prerelease-audit -**Verdict:** **NO MERGE YET** — 5 confirmed blockers pending resolution - -#### Review Lanes & Findings - -**Bishop (Documentation)** — ✅ READY -- Breaking-changes docs accurate and complete -- 29 issues closed with real code fixes verified -- Optional post-merge improvements: README link, CLI precedence clarity, security fix highlight -- Recommendation: APPROVE (non-blocking gaps only) - -**Dallas (Tooling)** — ❌ NOT READY -- Blocker #1011: Source generator hint-name collision on same-directory duplicates (partial fix) -- Blocker #1021: CLI `--output` override ignored in multi-file settings-file flow (partial fix) -- Blocker #1050: Enum-error guidance only added to CLI; source generator still raw (partial fix) -- Verified #1012: MSBuild exit-code handling correct - -**Ash (Safety)** — ❌ NOT READY -- Blocker #1013: ContractTypeSuffixApplier missing suffix-target collision detection (no check for `Foo` + `FooDto` → `FooDto` duplicate) -- Blocker #1018: ParameterExtractor multipart dedup uses original key, not sanitized name (`"a-b"` + `"a b"` → duplicate `"a_b"`) -- Both are compilation-breaking; must fix before merge - -**Ripley (Issue Matrix)** — ❌ NOT READY -- Blocker #1053: `Sanitize()` returns unescaped keywords (`@class`, missing `__*` set); no `EscapeReservedKeyword()` routing -- Blocker #1021: Multi-file precedence guard incomplete -- Blocker #1050: Source generator enum guidance not improved -- Supporting blockers from Ash (#1013, #1018) -- Awaiting Parker on #1040 (timeout config), #1050 (enum error handling) - -#### Confirmed Must-Fix Blockers (5 Items) +**Verified By:** Lambert (Tester) +**Status:** EVIDENCE NARROWED -| Issue | File | Gap | Fix | -|-------|------|-----|-----| -| #1013 | ContractTypeSuffixApplier.cs | No collision check | Add guard for duplicate targets | -| #1018 | ParameterExtractor.cs | Dedupe by wrong key | Dedupe by sanitized identifier | -| #1021 | GenerateCommand.cs | Multi-file ignores `-o` | Restore override guard + test | -| #1050 | RefitterSourceGenerator.cs | CLI-only guidance | Catch + re-throw with context | -| #1053 | IdentifierUtils (call sites) | No keyword routing | Route through `EscapeReservedKeyword` | +- Treat **#1028** as **still reproducible by inspection** on current HEAD; the source-generator incremental pipeline still carries a `List` equality hazard. +- Treat **#1029** as **partial** on current HEAD; visible diagnostics improved, but the "no .refitter files found" path is still only `Debug.WriteLine`. +- Treat **#1033** as **still reproducible by inspection**; enum-converter injection still uses a hard-coded LF and needs newline normalization coverage. +- Treat **#1041** as **partial**; runtime resolution improved, but argument escaping and timeout kill semantics still leave repro surface. +- Treat **#1043** as **still reproducible**; legacy `--generate-authentication-header` bool-style CLI usage still fails at current HEAD. +- Treat **#1032, #1042, #1045, and #1047** as **validation-only / fixed-at-HEAD evidence** unless stronger failing repros appear. +- Treat **#1034, #1039, and #1056** as **not reproduced on current HEAD** in Lambert's pass. -#### Evidence Summary +### Multi-spec Merge Collision Policy -**Resolved (20/28):** P0 all 7 fixed; P1 partial fixes; P2 mostly silent improvements -**Partial (6/28):** #1013, #1018, #1021, #1050, #1053, #1019 -**Unresolved (1/28):** #1053 (coordinator spot-check) -**Awaiting (1/28):** #1040 (Parker review) +**Decided By:** Parker (Core Developer) +**Status:** APPROVED -#### Recommendation +- `OpenApiDocumentFactory` should clone the first loaded document before merging additional specs so callers do not observe mutation of a previously loaded `OpenApiDocument`. +- Path and schema-key collisions across distinct OpenAPI inputs should fail fast with `InvalidOperationException` instead of silently keeping the first definition. +- Exact duplicate input paths should continue to deduplicate up front so feeding the same spec twice stays harmless. -- **Request blocker fixes:** ~30 minutes estimated work -- **Re-run full test suite** after fixes -- **Final gate:** All blockers resolved + tests passing → APPROVE FOR MERGE -- **Nice-to-have:** Parker/Lambert confirmations on #1040, #1019 +**Rationale:** Silent first-one-wins merge behavior hides real API-shape conflicts and only surfaces later during generation or runtime use. Failing fast is the safer core-library policy. -#### Agents Still Running +### Core Lane Follow-up Gate (#1057) -- **Parker (Core Developer):** Awaiting verdict on #1040 (HttpClient timeout) + #1050 (enum errors) -- **Lambert (Tester):** Optional confirmation on #1019 (edge cases), #1021 (CLI regression) +**Verified By:** Parker (Core Developer) +**Status:** FIXED / NARROWED ---- +- **#1033**: landed at HEAD with a code change in `src/Refitter.Core/RefitGenerator.cs` plus regression coverage in `src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs`. +- **#1032**: treat as validation-first pending review gate; no current core-lane code change required. +- **#1034** and **#1039**: treat as fixed-at-HEAD / no-repro on the reviewed branch state pending final gate review. +- **#1045**: treat as fixed-at-HEAD on the reviewed branch state pending final gate review. +- **#1056**: treat as doc/invariant-only for now; preserve current ordering behavior unless new failing evidence appears. +### Tooling Compatibility Follow-up -## 2026-04-20 +**Verified By:** Dallas (Tooling Developer) +**Status:** APPROVED -### PR #1064 Blocker Fixes: Final Validation Complete +- Preserve CLI compatibility for `--generate-authentication-header` by treating the legacy boolean forms (`true`, `false`) and the bare flag as valid inputs. The bare flag and `true` now map to `AuthenticationHeaderStyle.Method`; `false` maps to `None`, while `Parameter` still requires the explicit enum value. +- Keep MSBuild runtime resolution resilient across both packed and test-project layouts. The task now prefers bundled framework-specific Refitter binaries, falls back to lower compatible TFMs when probing fails, and finally uses a co-located `refitter.dll` when the packaged layout is unavailable. -**Decision Date:** 2026-04-20 -**PR:** #1064 ([v2.0 audit] Fix pre-release regressions from #1057) -**Verdict:** ✅ **APPROVED FOR MERGE** (cleanup pending) +### PR Prep Closure Guidance -#### All Blockers FULLY RESOLVED +**Verified By:** Ripley +**Status:** DRAFTED FOR PR ASSEMBLY -**Issue #1013 — ContractTypeSuffixApplier Collision Detection** ✅ -- Implemented: Pre-flight collision check before building typeRenameMap -- Strategy: Skip renaming if `name + suffix` collides with existing type -- Test coverage: 3 tests in PR1064BlockerRegressions.cs -- Status: PRODUCTION-READY +- Safe auto-close candidates on the reviewed branch state: **#1028**, **#1029**, **#1033**, and **#1043**. +- Keep **#1032**, **#1034**, **#1039**, **#1041**, **#1042**, **#1045**, **#1047**, and **#1056** out of PR auto-close wording until stronger evidence or final lane approval exists. +- **#1041** specifically remains a Dallas-owned tooling verdict before any PR body claims closure. -**Issue #1018 — ParameterExtractor Multipart Deduplication** ✅ -- Root cause: Two parameter extraction paths used different naming methods -- Fixed: Unified naming via `ConvertToVariableName()` across both paths -- Test coverage: 3 tests in PR1064BlockerRegressions.cs -- Status: PRODUCTION-READY +### Ash core review of remaining #1057 closures -**Issue #1053 — IdentifierUtils Keyword Escaping** ✅ -- Added: `__arglist`, `__makeref`, `__reftype`, `__refvalue` to reserved keywords -- Fixed: Interface name sanitization AFTER prefixing (prevents `I@class` pattern) -- Fixed: Test expectations corrected for NSwag schema name capitalization behavior -- Test coverage: 6 tests in PR1064BlockerRegressions.cs -- Status: PRODUCTION-READY +**Verified By:** Ash +**Status:** REJECT -#### Validation Results +- Verified acceptable: + - **#1032** does not reproduce the claimed custom `JsonNamingPolicy` override regression at current HEAD; runtime repro with a type-level `JsonStringEnumConverter` still serialized via `JsonSerializerOptions.Converters` (`"my_value"`). + - **#1045** is effectively fixed at current HEAD because `RefitGenerator.GetOpenApiDocument()` uses `OpenApiPaths` directly when populated instead of dereferencing `OpenApiPath`. + - **#1033** is the only intentional core code change in the working tree (`src/Refitter.Core/RefitGenerator.cs`) and it has matching regression coverage in `src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs`. +- Still open / false closure: + - **#1034** remains open in `src/Refitter.Core/OpenApiDocumentFactory.cs:55-107`; `Merge()` still mutates `documents[0]` and still silently keeps the first path/schema on key collisions. + - **#1039** remains open in `src/Refitter.Core/ParameterExtractor.cs:447-487` plus `src/Refitter.Core/RefitInterfaceGenerator.cs:69-82`; `GetParameters()` removes query parameters from `operationModel.Parameters` before XML-doc generation reads the shared model. +- Follow-up requirement: reassign the remaining core revisions to Parker (or another core implementer) for real fixes before closing **#1034**/**#1039** from the `#1057` matrix. -- **Build Status:** ✅ Clean build, 0 errors -- **Test Suite:** ✅ 1779/1779 PASSING (0 failures) -- **Code Formatting:** ✅ All changes properly formatted +### Dallas core revision on rejected blockers -#### Quality Metrics +**Verified By:** Dallas +**Status:** IMPLEMENTED / PENDING ASH RE-REVIEW -- **Code Quality:** Excellent — defensive programming, no exceptions, surgical scope -- **Test Coverage:** Comprehensive — 13 new tests covering all three blockers + edge cases -- **Regression Risk:** Minimal — targeted fixes, existing tests unaffected +- **#1034:** `OpenApiDocumentFactory.Merge()` now clones the first input before merge so callers no longer observe mutation of a previously loaded `OpenApiDocument`. +- **#1034:** duplicate path/schema collisions now emit warnings while preserving the existing merged entry; this revised pass does **not** follow the earlier fail-fast proposal. +- **#1039:** `ParameterExtractor` no longer mutates the shared `operationModel.Parameters` collection when building grouped query-parameter wrappers, so downstream consumers keep the original operation model intact. +- Regression coverage was refreshed for the revised core pass in `src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs`, `src/Refitter.Tests/ParameterExtractorEdgeCaseTests.cs`, `src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs`, and `src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs`. +- Dallas reported the revised core validation lane green; Ash is performing re-review and Lambert is reconciling the blocker-test lane against the landed behavior. -#### Cleanup Required (CRITICAL) +### Ash core re-review of Dallas revision -⚠️ **BEFORE MERGE:** -- [ ] DELETE `src/test-multipart.json` — temporary repro file for #1018 -- [ ] DELETE `src/test-keywords.json` — temporary repro file for #1053 +**Verified By:** Ash +**Status:** PARTIAL / BLOCKED -#### Agent Sign-offs +- **#1039 resolved:** ParameterExtractor.GetParameters() no longer mutates operationModel.Parameters, and ParameterExtractorPrivateCoverageTests now lock that invariant for XML-doc generation and shared-model reuse. +- **#1034 still open:** OpenApiDocumentFactory.Merge() now clones the first input, but it still keeps the first conflicting path/schema/definition/security entry via Trace.TraceWarning(...) instead of failing fast. +- src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs still codifies warning-backed first-wins collision handling; the next narrow revision must flip that coverage to an InvalidOperationException contract for conflicting inputs. +- Do **not** close **#1034** from the #1057 matrix yet. Dallas owns one last narrow revision, and Lambert remains on the blocker-test lane. -- ✅ **Parker:** Implemented initial fixes; identified #1018 naming method mismatch -- ✅ **Ash:** Diagnosed root causes; implemented unified naming for #1018; corrected #1053 test expectations -- ✅ **Lambert:** Created 13 comprehensive regression tests; verified all passing -- ✅ **Dallas:** Validated build and test suite; confirmed all 1779 tests passing +### Dallas final #1034 revision / Lambert blocker-test reconciliation -#### Key Learnings +**Verified By:** Dallas / Lambert +**Status:** IMPLEMENTED / READY FOR ASH FINAL GATE -1. **Deduplication requires naming consistency** — Both code paths must use SAME transformation method -2. **Case sensitivity matters** — GetVariableName() preserves casing; ConvertToVariableName() lowercases -3. **NSwag behavior is normative** — Schema names capitalized by NSwag; tests must match actual behavior -4. **Two-phase extraction complexity** — ParameterExtractor has parallel paths requiring coordinated fixes +- `OpenApiDocumentFactory.Merge()` now preserves the clone-first non-mutation guarantee **and** fails fast with `InvalidOperationException` when distinct inputs introduce conflicting duplicate path, schema, definition, or security keys. +- Non-conflicting merges still return a new document without mutating either input document, and exact duplicate input paths remain harmless because they are deduplicated before merge. +- Blocker coverage is now aligned to the fail-fast contract in `src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs`; `Issue1039_DynamicQuerystringMutationTests.cs` still preserves grouped-query XML-doc assertions across single-interface, `MultipleInterfaces.ByTag`, and `MultipleInterfaces.ByEndpoint` generation. +- Dallas's final narrow implementation pass is complete. Ash is performing the final review gate, and Lambert is reconciling the blocker-test lane against the landed fail-fast behavior. -#### Recommendation +### Ash final core gate rejection -**APPROVED FOR MERGE after cleanup.** All blockers are comprehensively resolved with excellent test coverage. Code is production-ready. Implementation follows best practices and minimal-scope surgical fixes. +**Verified By:** Ash +**Status:** REJECTED -**Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` +- **#1034** is still not proven closed: `src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs` only demonstrates fail-fast behavior for duplicate **paths**, not explicit conflicting **schemas**, **definitions**, and **security schemes**. +- The broader core validation lane is not green because `dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj` still fails `Dynamic_Querystring_Generation_Preserves_Original_Query_Param_Documentation(ByEndpoint)` in `Issue1039_DynamicQuerystringMutationTests`. +- Dallas is now locked out of the next revision cycle for this artifact; Parker remains locked out from the prior rejected cycle. +- Lambert now owns the next/final revision cycle for **#1034** while staying in the blocker-test lane. ---- +### Ash final review of Lambert revision -## 2026-04-17 +**Verified By:** Ash +**Status:** REJECTED -### Release Compatibility Audit: 1.7.3 → HEAD (All Agents Consensus) +- **#1039 acceptable:** `ParameterExtractor.GetQueryParameters()` still snapshots query parameters locally and preserves the shared `operationModel.Parameters` list; the regression coverage remains aligned with the intended non-mutating behavior. +- **#1034 still not proven closed:** the Swagger 2 definition-collision proof is still not isolated cleanly enough. `OpenApiDocumentFactoryMergeTests.Merge_With_Definition_Collision_Throws_And_Does_Not_Mutate_Inputs` still trips the duplicate **schema** conflict before it conclusively proves the duplicate **definition** lane. +- Remaining blocker for the next cycle: isolate the definition-specific fail-fast proof so the test fails for the intended definition-collision reason instead of the mirrored schema path. +- Lambert now joins Parker and Dallas in lockout for the next revision cycle on this artifact. +- Ripley now owns the next narrow revision cycle for **#1034**. -**Verdict:** BREAKING CHANGES FOUND. Cannot be marketed as non-breaking release. Major version bump (2.0.0) required. +### Ripley final #1034 proof-gap revision -#### Breaking Changes (2 Confirmed) +**Verified By:** Ripley +**Status:** IMPLEMENTED / PENDING ASH FINAL SIGNOFF -1. **Auth Property Renamed (MEDIUM RISK)** - - `.refitter` setting: `generateAuthenticationHeader` (bool) → `authenticationHeaderStyle` (enum: None, Method, Parameter) - - No backward compatibility layer or JSON mapping - - Old JSON key silently ignored; defaults to `AuthenticationHeaderStyle.None` - - Affected: users with `"generateAuthenticationHeader": true` in `.refitter` files - - Evidence: Commits 7dbf6c0c, 14101a49; confirmed by Lambert's deserialization tests - - Migration: Replace `"generateAuthenticationHeader": true` with `"authenticationHeaderStyle": "Method"` or `"Parameter"` +- Preserve the source document schema type during clone/copy so Swagger 2 inputs stay on the intended definitions surface throughout merge handling. +- Isolate the Swagger 2 definition-collision proof at MergeIfMissingOrThrowOnConflict(...) so the definition-specific fail-fast contract is asserted directly instead of being masked by the mirrored schema collision first. +- Reported validation from the revision lane is green for dotnet build -c Release src\Refitter.slnx and dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj. +- Ash now owns the final reviewer signoff before broader validation resumes. +### Ash final signoff on Ripley #1034/#1039 follow-up -2. **Source Generator Disk Files (HIGH RISK)** - - Source generator no longer writes `.g.cs` files to disk - - Changed from `File.WriteAllText()` to `context.AddSource()` (Roslyn best practice) - - Fixes issues #635, #520, #310 (file locking, process access errors) - - Affected: source generator users expecting physical files in `./Generated` folder - - Users must view generated code via IDE or switch to CLI/MSBuild for disk files - - Evidence: Commit f853bcf2 (PR #923); confirmed by Dallas tie-breaker audit +**Verified By:** Ash +**Status:** APPROVED -#### Non-Breaking Changes +- **#1034 approved:** OpenApiDocumentFactory.Merge() now clones the first document before merge, fails fast on conflicting duplicate path/schema/definition/security keys, and isolates the remaining Swagger 2 definition proof through the shared MergeIfMissingOrThrowOnConflict(...) path. +- **#1039 approved:** grouped dynamic-query extraction still snapshots query parameters instead of mutating operationModel.Parameters, and XML-doc regression coverage remains locked for single-interface, ByTag, and ByEndpoint generation. +- Evidence reviewed: src/Refitter.Core/OpenApiDocumentFactory.cs, src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs, src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs, and src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs. +- Reviewer signoff was reported against dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj with 1840 passing and 0 failing. -- **MSBuild output path fix (Issue #998):** NOT a breaking change. MSBuild now respects default `./Generated` instead of incorrectly outputting to `.refitter` directory. This is a bug fix, not a break. Users relying on old buggy behavior can set `"outputFolder": "."` explicitly. -- **8 Additive Features** (all backward compatible with safe defaults): - - PropertyNamingPolicy (defaults to PascalCase) - - OpenApiPaths (multi-spec merge) - - ContractTypeSuffix - - GenerateJsonSerializerContext (AOT) - - SecurityScheme filtering - - CustomTemplateDirectory - - New CLI options for all above - - Auto-enable GenerateOptionalPropertiesAsNullable (scoped) -- **4 Bug Fixes** (only affect previously broken inputs): - - Stack overflow in recursive schemas - - Digit-prefixed property naming (invalid C# identifiers) - - Multipart form-data parameter extraction - - OneOf discriminator handling -- **Generated Code Quality Improvements:** - - JsonConverter attribute placement: properties → enum types (semantically equivalent) - - Method naming in ByTag mode: numeric suffixes now scoped per-interface +### Final PR package guidance for #1057 -#### Release Recommendation +**Prepared By:** Ripley +**Status:** READY FOR PR ASSEMBLY -- **Version:** 2.0.0 (major bump required) -- **CHANGELOG:** Document both breaking changes with clear migration paths -- **Migration Guide:** Provide search/replace instructions and generated-code viewing guidance -- **Timeline:** All agents aligned; ready for release decision +- Proposed PR title: `[v2.0 audit] Close remaining verified #1057 regressions`. +- Keep PR summary focused on five landed lanes: source-generator diagnostics, newline-safe enum-converter rewriting, non-mutating dynamic querystring generation, fail-fast multi-spec merge handling, and tooling/runtime compatibility hardening. +- Safe auto-close set for the final PR body: **#1028, #1029, #1033, #1034, #1039, #1041, #1043**. +- Keep **#1032, #1042, #1045, #1047, and #1056** out of auto-close wording because they are validation-only, fixed-at-HEAD/stale, or doc/invariant-only. +- Before opening the PR, recreate/publish v2.0.0-prerelease-fixes with `git push -u origin HEAD` because the local branch tracks a gone upstream. +- Latest local full validation reported: dotnet restore src\Refitter.slnx, dotnet build -c Release src\Refitter.slnx --no-restore, dotnet test -c Release src\Refitter.slnx --no-build, and dotnet format --verify-no-changes src\Refitter.slnx --no-restore with 1886 tests passing. -#### Agents Aligned +### CLI help output assertions should be semantic -✅ Ripley (Lead): BREAKING CHANGES FOUND - cannot approve as non-breaking -✅ Parker (Core Dev): BREAKING CHANGE DETECTED in auth settings surface -✅ Dallas (Tooling Dev): CONFIRMED 2 breaking changes; bug fix is non-breaking -✅ Lambert (Tester): BREAKING CHANGE CONFIRMED with concrete deserialization evidence +**Verified By:** Lambert (Tester) +**Status:** APPROVED ---- +- src\Refitter\Program.cs intentionally rewrites a no-argument invocation to --help, exits 0, and emits Spectre.Console.Cli help output. +- The current product behavior is correct; the instability sits in whitespace-sensitive test expectations, not in production code. +- src\Refitter.Tests\GenerateCommandTests.cs should assert semantic help markers (usage pattern, sections, and known option names) instead of exact formatter-driven spacing/default-value layout. +- Validation reported: release run of src\Refitter.Tests\Refitter.Tests.csproj, focused rerun of Program_Main_Should_Show_Help_When_Invoked_Without_Arguments, and dotnet format --verify-no-changes src\Refitter.slnx. -## 2026-04-16 -- Squad initialized for Refitter. -- Team root uses the worktree-local strategy at `C:\projects\christianhelle\refitter`. -- Shared append-only Squad files use Git's `union` merge driver. -- **Issue #998 Investigation Complete:** Verdict is a real product bug, not user error. CLI ignores `outputFolder` when it equals the default `./Generated`, causing MSBuild to search for files in wrong location. First clean build fails due to sync mismatch between MSBuild prediction and CLI output. Fix: remove default-value check in `GenerateCommand.cs:648`. diff --git a/.squad/skills/spectre-cli-help-assertions/SKILL.md b/.squad/skills/spectre-cli-help-assertions/SKILL.md new file mode 100644 index 000000000..016ad61ea --- /dev/null +++ b/.squad/skills/spectre-cli-help-assertions/SKILL.md @@ -0,0 +1,28 @@ +--- +name: "spectre-cli-help-assertions" +description: "Write resilient assertions for Spectre.Console.Cli help output" +domain: "testing" +confidence: "high" +source: "observed" +--- + +## Context +Use this when testing CLI help output generated by Spectre.Console.Cli. The framework controls wrapping, spacing, and inline default-value formatting, so exact string matches are brittle across versions and environments. + +## Patterns +- Reproduce the real help output first by invoking the compiled CLI with no arguments or `--help`. +- Assert the behavior contract, not the formatter details: + - exit code is `0` + - usage matches with whitespace-tolerant regex + - expected sections such as `ARGUMENTS:` and `OPTIONS:` are present + - one or more product-specific option names are present +- Avoid asserting whole rendered lines when the same information can be checked semantically. + +## Examples +- `result.Output.Should().MatchRegex(@"USAGE:\s+refitter\s+\[URL or input file\]\s+\[OPTIONS\]");` +- `result.Output.Should().Contain("ARGUMENTS:");` +- `result.Output.Should().Contain("--generate-authentication-header");` + +## Anti-Patterns +- Matching the full usage/help block with exact spaces or line breaks. +- Treating formatter-driven whitespace differences as product regressions. From 5d2dcd3fca0da9fb2836184b9515bc66f287b025 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 17:33:26 +0200 Subject: [PATCH 3/9] Stabilize CLI help output test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Refitter.Tests/GenerateCommandTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Refitter.Tests/GenerateCommandTests.cs b/src/Refitter.Tests/GenerateCommandTests.cs index 67d083b1e..8e7ae23fb 100644 --- a/src/Refitter.Tests/GenerateCommandTests.cs +++ b/src/Refitter.Tests/GenerateCommandTests.cs @@ -419,8 +419,9 @@ public void Program_Main_Should_Show_Help_When_Invoked_Without_Arguments() var result = InvokeProgram([]); result.ExitCode.Should().Be(0); - result.Output.Should().Contain("USAGE:"); - result.Output.Should().Contain("refitter [URL or input file] [OPTIONS]"); + result.Output.Should().MatchRegex(@"USAGE:\s+refitter\s+\[URL or input file\]\s+\[OPTIONS\]"); + result.Output.Should().Contain("ARGUMENTS:"); + result.Output.Should().Contain("OPTIONS:"); result.Output.Should().Contain("--generate-authentication-header"); } From 48efd0ccad063b03ccc298f0ec2e2fd2d7e35f4d Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 17:58:41 +0200 Subject: [PATCH 4/9] Normalize help output test across platforms Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Refitter.Tests/GenerateCommandTests.cs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/Refitter.Tests/GenerateCommandTests.cs b/src/Refitter.Tests/GenerateCommandTests.cs index 8e7ae23fb..e0c10f7ba 100644 --- a/src/Refitter.Tests/GenerateCommandTests.cs +++ b/src/Refitter.Tests/GenerateCommandTests.cs @@ -1,5 +1,6 @@ using System.Diagnostics; using System.Reflection; +using System.Text.RegularExpressions; using FluentAssertions; using Refitter.Core; using TUnit.Core; @@ -417,12 +418,14 @@ public void ApplySettingsFileDefaults_Should_Set_Default_When_Empty_OutputFolder public void Program_Main_Should_Show_Help_When_Invoked_Without_Arguments() { var result = InvokeProgram([]); + var normalizedOutput = NormalizeConsoleOutput(result.Output); result.ExitCode.Should().Be(0); - result.Output.Should().MatchRegex(@"USAGE:\s+refitter\s+\[URL or input file\]\s+\[OPTIONS\]"); - result.Output.Should().Contain("ARGUMENTS:"); - result.Output.Should().Contain("OPTIONS:"); - result.Output.Should().Contain("--generate-authentication-header"); + normalizedOutput.Should().Contain("USAGE:"); + normalizedOutput.Should().Contain("refitter [URL or input file] [OPTIONS]"); + normalizedOutput.Should().Contain("ARGUMENTS:"); + normalizedOutput.Should().Contain("OPTIONS:"); + normalizedOutput.Should().Contain("--generate-authentication-header"); } [Test] @@ -533,6 +536,12 @@ private static void DeleteWorkspace(string workspace) } } + private static string NormalizeConsoleOutput(string output) + { + var withoutAnsi = Regex.Replace(output, @"\x1B\[[0-9;?]*[ -/]*[@-~]", string.Empty); + return withoutAnsi.ReplaceLineEndings("\n"); + } + private static string QuoteArgument(string argument) => argument.Contains(' ', StringComparison.Ordinal) ? $"\"{argument}\"" From 78e2b398693912f334da8356c878011f2e1cee47 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 18:01:48 +0200 Subject: [PATCH 5/9] chore(squad): log linux help stabilization Record the Dallas/Lambert Linux help-output handoff, merge the semantic-help decision details, and summarize Dallas history. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/dallas/history.md | 343 +----------------- .squad/agents/lambert/history.md | 7 + .squad/agents/scribe/history.md | 1 + .squad/decisions.md | 2 + ...-59-12Z-linux-help-output-consolidation.md | 23 ++ ...25T15-59-12Z-dallas-linux-help-analysis.md | 26 ++ ...04-25T15-59-12Z-lambert-linux-help-test.md | 26 ++ 7 files changed, 103 insertions(+), 325 deletions(-) create mode 100644 .squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md create mode 100644 .squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md create mode 100644 .squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md diff --git a/.squad/agents/dallas/history.md b/.squad/agents/dallas/history.md index 8f6c3237d..890c4a271 100644 --- a/.squad/agents/dallas/history.md +++ b/.squad/agents/dallas/history.md @@ -9,334 +9,27 @@ ## Learnings - Team initialized on 2026-04-16. -- **Issue #998 findings (2026-04-16):** Validated MSBuild tooling path. CLI loads settings correctly (naming honored). Single-file output without explicit `outputFilename` falls back to `Output.cs` and skips `./Generated` folder when it matches default. Real product bug: MSBuild expects wrong file locations on first clean build. -- **PR #1067 tooling proof (2026-04-21):** The strongest regression signal came from testing the actual stdout marker contract and the packed `Refitter.SourceGenerator` artifact. `RefitterGenerateTask` now has unit coverage for exact include matching plus duplicate/zero-marker handling, and package validation inspects the produced `.nupkg`/`.nuspec` instead of trusting project metadata. +- **Issue #998 findings (2026-04-16):** MSBuild's first-clean-build path was the real tooling bug; CLI settings loading and the default single-file `Output.cs` behavior were otherwise correct. +- **PR #1067 tooling proof (2026-04-21):** The most reliable tooling evidence comes from validating real stdout marker contracts and packed artifacts (`.nupkg`/`.nuspec`), not just project metadata or happy-path check runs. +- **2026-04-25 Linux help-output analysis:** The Ubuntu help failure was a test portability problem caused by raw Spectre.Console ANSI/wrapping noise, not a product regression in the CLI help path. -### 2026-04-20: PR #1064 Tooling Review +## Core Context -- **#1011 is only partially fixed:** `RefitterSourceGenerator.CreateUniqueHintName()` hashes only the directory path, so two `.refitter` files in the same folder that share the same `outputFilename` still collide on `context.AddSource()`. The attempted fix moved the failure instead of making hint names unique per `.refitter` input. -- **#1021 is only partially fixed:** single-file `--output` override was repaired, but multi-file generation still ignores `settings.OutputPath` whenever `OutputFolder` is populated/defaulted. Settings-file + `--multiple-files`/contracts-output flows still write under the settings output folder. -- **#1050 is only fixed for CLI/MSBuild:** helpful enum diagnostics were added to `SettingsValidator`, but `RefitterSourceGenerator.TryDeserialize()` still emits the raw exception text without property/value guidance. `.refitter` enum mistakes remain hard to diagnose in source-generator hosts. -- **Validation evidence gap:** PR #1064 check runs were green, but the `test` GitHub check is a no-op placeholder (`No build commands configured — update squad-ci.yml`). The local `test\MSBuild\test-exit-code.ps1` script passes when launched from `test\MSBuild`, but fails from repo root because its relative paths assume that working directory. +- **Breaking-change audit:** The two durable v2 break signals were the source generator's move from disk-written `.g.cs` files to `AddSource()` output and the settings rename from `generateAuthenticationHeader` to `authenticationHeaderStyle` without a compatibility alias. +- **Tooling architecture:** `src\Refitter.MSBuild\RefitterGenerateTask.cs` should trust CLI-emitted `GeneratedFile:` markers, exact include-pattern matches, and real process exit codes; `src\Refitter.SourceGenerator\RefitterSourceGenerator.cs` should keep hint names unique per input and surface user-visible diagnostics instead of relying on `Debug.WriteLine`. +- **Common audit hazards:** shared NSwag model mutation, hard-coded newlines, path resolution tied to CWD, and silent/weak conflict handling were recurring root causes across the tooling/core lanes. +- **Reliable validation patterns:** when the shared NuGet cache is locked, prefer the repo-local cache at `C:\projects\christianhelle\refitter\.nuget\packages`, then validate with targeted tests, packed artifacts, and formatter/build gates. -### 2026-04-17: Breaking Changes Audit + Tie-Break Decision +## 2026-04-25: Remaining Audit Lanes -**Primary Audit Task**: Reviewed 370 files (36,658 insertions, 6,808 deletions) between 1.7.3 and HEAD for breaking changes. **INITIAL ASSESSMENT:** No breaking changes found. All changes appeared backward-compatible or additive. +- Lambert's repro pass narrowed the tooling concerns to **#1029** and **#1041** as partial repros plus the legacy bool-form **#1043** CLI break; **#1042** stayed validation-only and **#1047** stayed fixed-at-HEAD. +- Dallas completed the tooling lane for **#1028**, **#1029**, **#1041**, and **#1043**, then moved onto the rejected core revision for **#1034**/**#1039** after Parker lockout. +- Dallas's core pass fixed the shared-parameter mutation for **#1039** and iterated the multi-spec merge contract for **#1034** from warning-backed cloning to fail-fast collision handling before final proof ownership moved to Lambert and then Ripley. -**Tie-Breaker Re-Check**: Parker and Ripley flagged 3 candidates for re-verification. +## 2026-04-25: Linux Help Output Analysis -**Final Verdict**: **2 CONFIRMED BREAKING CHANGES** - -1. **Source Generator Disk Files** (HIGH RISK) - - Commit: f853bcf2 (PR #923) - "Fix #635: Use context.AddSource() instead of File.WriteAllText()" - - Fixes issues #635, #520, #310 (file locking, process access errors) — legitimate bug fix - - BUT: Behavioral break for users expecting physical `.g.cs` files in `./Generated` folder - - Users who committed generated files or relied on disk inspection are affected - - Migration: Use IDE "View Generated Files" or switch to CLI/MSBuild for disk files - -2. **Auth Property Renamed** (MEDIUM RISK) - - Commit: 7dbf6c0c (PR #897) + 14101a49 (PR #936) - - Old: `"generateAuthenticationHeader": true` (boolean) - - New: `"authenticationHeaderStyle": "Method"|"Parameter"|"None"` (enum) - - No backward compatibility layer or JSON property alias - - Old JSON key **silently ignored** — deserializes to default (`None`) - - Users get wrong behavior without error (dangerous failure mode) - - Migration: Replace old key with new enum value - -**NOT BREAKING (Bug Fix)**: -- Default output folder unchanged (`./Generated`) -- MSBuild bug fix makes it **consistent** with CLI (Issue #998) -- Users relying on old buggy behavior: set `"outputFolder": "."` explicitly - -**Release Recommendation**: **v2.0.0** (major bump required). Both breaking changes require migration guide. - -**CLI/Settings/Options**: -- All new options since 1.7.3 have safe defaults and are optional -- No removed or renamed options -- No changed default values -- Settings file processing order improved (`.refitter` first, then CLI override) - -**Dependency Updates**: -- Spectre.Console.Cli v0.55.0: Methods changed from `public override` to `protected override` (non-breaking for CLI users) -- Microsoft.OpenApi v3.x (internal only) -- Refit v10 (users should upgrade) - -**Key File Paths for Tooling**: -- CLI options: `src/Refitter/Settings.cs` -- CLI logic: `src/Refitter/GenerateCommand.cs` -- Settings model: `src/Refitter.Core/Settings/RefitGeneratorSettings.cs` -- Source generator: `src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` -- MSBuild task: `src/Refitter.MSBuild/RefitterGenerateTask.cs` -- Settings docs: `docs/docfx_project/articles/refitter-file-format.md` - -### 2026-04-18: Critical Tooling Fixes (Issues #1011, #1012) - -**Fixed #1011 — Source Generator Hint-Name Collisions** - -Problem: When multiple `.refitter` files with the same filename existed in different directories (e.g., `src/ApiA/petstore.refitter` and `src/ApiB/petstore.refitter`), the source generator crashed with `ArgumentException: hintName was already added` because hint names were computed only from the filename. - -Solution implemented: -- Created `CreateUniqueHintName()` method that generates stable, unique hint names by combining the base filename with a hash of the directory path -- Honors explicit `OutputFilename` when set while still preventing collisions -- Uses simple deterministic hash (31-bit polynomial) formatted as 8-char hex suffix -- Hint name format: `{baseName}_{pathHash}.g.cs` (e.g., `petstore_A1B2C3D4.g.cs`) - -Files changed: -- `src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`: Added `CreateUniqueHintName()` and `GetStableHash()` helper methods - -Regression coverage: -- Created `src/Refitter.SourceGenerator.Tests/HintNameCollisionTests.cs` with two test cases: - 1. Verifies generator doesn't crash with duplicate filenames in different directories - 2. Verifies explicit `outputFilename` intent is preserved in hint name base - -**Fixed #1012 — MSBuild Task Swallows CLI Failures** - -Problem: `RefitterGenerateTask.Execute()` ignored the `dotnet refitter.dll` process exit code and unconditionally returned `true`, causing CI/CD pipelines to silently ship stale/missing generated code when the CLI failed. - -Solution implemented: -- Added exit code inspection: `process.ExitCode != 0` → log error and mark as failed -- Added process timeout (5 minutes) to prevent build hangs -- Modified `TryExecuteRefitter()` to return `out bool failed` parameter -- Modified `Execute()` to track `hasErrors` and return `false` if any .refitter file failed -- Exception paths also set `failed = true` for comprehensive error handling - -Files changed: -- `src/Refitter.MSBuild/RefitterGenerateTask.cs`: Modified `Execute()`, `TryExecuteRefitter()`, and `StartProcess()` methods - -Regression coverage: -- Created `test/MSBuild/test-exit-code.ps1` script that: - 1. Creates test project with invalid .refitter (unreachable URL) - 2. Runs `dotnet build` - 3. Asserts build fails with non-zero exit code (would pass before fix) - -**Build Status**: -- MSBuild project compiled successfully: ✅ `src\Refitter.MSBuild\bin\Release\netstandard2.0\Refitter.MSBuild.dll` -- Source generator changes are syntactically correct but compilation blocked by pre-existing Refitter.Core issues (unrelated to this work): - - `OpenApiDocumentFactory.cs(68,21)`: Property assignment errors - - `RefitGenerator.cs(275,48)`: Missing `JsonLibrary` definition - - These are outside the scope of the critical tooling slice -- All modified code formatted with `dotnet format` - -**Validation**: -- MSBuild task changes compile cleanly -- Code formatted according to project standards -- Regression test infrastructure in place (execution depends on Core build fix) - -**Scope Notes**: -- Tightly scoped to P0 tooling issues #1011 and #1012 -- No coupling with other audit findings -- Both fixes are self-contained and can ship independently once Core compilation is restored - -### 2026-04-17: GitHub Discussions Setup + Distribution Audit - -**Discussion Creation Capability**: Confirmed repo has discussions enabled. GitHub CLI (v2.73.0) authenticated as `christianhelle`. Available categories: Announcements (recommended), General, Ideas, Polls, Q&A, Show and tell. Can create Discussion directly via `gh api graphql` with `createDiscussion` mutation. - -**Evidence Artifacts for Discussion Post**: -- Auth property change: Commits 7dbf6c0c (PR #897), 14101a49 (PR #936) -- Source generator change: Commit f853bcf2 (PR #923) - fixes issues #635, #520, #310 -- 359 total commits, 370 files changed (+36,658/-6,808) -- All breaking changes have unit test coverage -- Team consensus: v2.0.0 major bump required - -**Detailed Audit Report Written**: `.squad/decisions/inbox/dallas-breaking-audit.md` with full evidence chain, migration paths, and non-breaking change summary for reference during release planning. - -### P2 Issue Verification (2026-04-18) - -**Verified 16 P2 issues from v2.0 audit** - -Key patterns and file paths discovered: - -**Source Generator Architecture:** -- src\Refitter.SourceGenerator\RefitterSourceGenerator.cs - Main generator using incremental compilation -- Pipeline outputs defined as private record GeneratedCode(List, string?, string?) -- Uses Debug.WriteLine for logging (no-op in Release) -- Should use context.ReportDiagnostic for user-visible warnings -- Should use EquatableArray or implement IEquatable for incremental caching - -**CLI Validation Flow:** -- src\Refitter\SettingsValidator.cs - Validates settings before generation -- src\Refitter\GenerateCommand.cs - Orchestrates validation and generation -- Path resolution inconsistency: CLI uses CWD, generator uses .refitter directory -- Multi-spec validation only checks first entry - -**Core Generation Critical Files:** -- src\Refitter.Core\RefitGenerator.cs - Main generation orchestrator - - Line 275: Hard-coded \n in regex replacement (should use Environment.NewLine) - - Handles JsonConverter attribute placement -- src\Refitter.Core\OpenApiDocumentFactory.cs - Spec loading and merging - - Lines 15-19: Static HttpClient without timeout/User-Agent configuration - - Line 46: Merge mutates documents[0] directly - - Lines 60-70: Silent conflict resolution (first wins) -- src\Refitter.Core\XmlDocumentationGenerator.cs - XML comment generation - - Line 133: Parameter descriptions not escaped before XML emission - - Has EscapeSymbols method but not always used - - AppendXmlCommentBlock doesn't escape attribute values -- src\Refitter.Core\ParameterExtractor.cs - Parameter processing - - Line 180: Uses Contains("?") to detect nullability (matches generics) - - Line 465: Mutates shared operationModel.Parameters collection -- src\Refitter.Core\RefitInterfaceImports.cs - Namespace generation - - Line 62: Uses Aggregate which throws on empty sequence -- src\Refitter.Core\CustomCSharpTypeResolver.cs - Type mapping - - Lines 32-33: Appends ? without checking NRT setting - - Fragile Contains("Nullable<") check - -**MSBuild Integration:** -- src\Refitter.MSBuild\RefitterGenerateTask.cs - Build-time generation - - Line 93: Unescaped arguments (path injection risk) - - Line 150: No null check before Split - - Lines 76-91: TFM selection without File.Exists check - -**Common Anti-patterns Found:** -1. Mutation of shared NSwag models (breaks subsequent generators) -2. String contains checks instead of proper parsing (nullable detection, type checking) -3. Missing XML escaping for user-supplied content -4. Hard-coded line endings instead of Environment.NewLine -5. Aggregate on potentially empty sequences -6. Debug.WriteLine in libraries (invisible in Release) -7. Path resolution relative to CWD instead of file location - -**Settings Architecture:** -- src\Refitter\Settings.cs - CLI settings with Spectre.Console attributes -- src\Refitter.Core\Settings\RefitGeneratorSettings.cs - Core settings -- src\Refitter.Core\Settings\CodeGeneratorSettings.cs - Code generation settings -- Breaking change: --generate-authentication-header changed from bool to AuthenticationHeaderStyle enum - -**Dependencies:** -- Spectre.Console.Cli 0.55.0 (potential parsing changes from 0.53) -- NSwag for OpenAPI document model and code generation -- H.Generators.Extensions (provides EquatableArray for source generators) - -### 2025-01-09: PR #1064 Blocker Validation - -**Status**: ❌ **NOT MERGE-READY** — 3 regression test failures detected - -**Validation Scope**: Focused validation of merge blocker fixes for issues #1013, #1018, #1053 - -**Build Results**: -- ✅ Clean build: `dotnet build -c Release src/Refitter.slnx --no-restore` → 0 errors -- ⚠️ Test suite: 1776 passed, **3 failed**, 0 skipped (1779 total) -- Test run time: ~52 seconds - -**Failed Regression Tests**: -1. `Issue1053_Schema_Names_As_Keywords_Are_Properly_Escaped` — Keywords not being escaped in type declarations (@class, @event missing) -2. `Issue1018_Deduplicates_Multipart_Parameters_By_Sanitized_Identifier` — Finding 3 "a_b" params instead of deduplicating to 1 -3. `Issue1018_Generated_Code_With_Duplicate_Sanitized_Names_Compiles` — Build fails with "parameter a_b is a duplicate" - -**Code Analysis**: -- Blocker fix code IS present in codebase (lines 97-140 in ParameterExtractor.cs verified) -- ContractTypeSuffixApplier.cs has collision detection correctly implemented -- BUT: Fixes are incomplete or have logic bugs that tests expose - -**Key Findings**: -- Deduplication HashSet logic looks correct but isn't preventing duplicates -- Possible issue: Variable name generation in one code path differs from HashSet check path -- Keywords in schema type names aren't being escaped; EscapeReservedKeyword() may not be in type generation chain - -**Recommendation**: -- Do NOT merge until regression tests pass -- Need debug trace to see what variable names are actually being generated vs checked -- Check if `ConvertToVariableName()` correctly reduces "a-b", "a b", "a.b" to identical "a_b" -- Verify keyword escaping is called during schema type name generation, not just parameters - -**Validation Command**: -```bash -dotnet test --project src/Refitter.Tests/Refitter.Tests.csproj -c Release --no-restore --no-build --output Detailed -``` - -**Report Location**: `.squad/decisions/inbox/dallas-pr1064-validation.md` - -## 2026-04-20 Final Update: Blocker Validation Successful - -**Task:** Re-validate blocker fixes after Ash's revision -**Status:** ✅ COMPLETE — All blockers resolved; 1779/1779 tests passing - -**Final Validation Results:** -- **Build Status:** ✅ Clean build, 0 errors -- **Test Suite:** ✅ 1779/1779 PASSING (0 failures, up from 1776/1779) -- **Code Formatting:** ✅ All changes properly formatted - -**Root Cause Feedback Used by Ash:** -- **#1018:** Dallas's observation about variable name mismatch guided Ash to unified naming method -- **#1053:** Dallas's identification of keyword escaping path gap led to test expectation correction - -**Collaboration Notes:** -- Dallas provided concrete test failure data that enabled rapid root-cause diagnosis -- Validation report became feedback loop for Ash's revision cycle -- Final validation confirmed all three blockers comprehensively resolved - -**Final Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` - -**Merge Status:** ✅ APPROVED (temporary test JSON files marked for deletion) - -### 2026-04-20: P1 Tooling Fixes (#1022, #1023, #1024) - -- **MSBuild generated-file discovery:** `src\Refitter.MSBuild\RefitterGenerateTask.cs` now trusts `GeneratedFile:` markers emitted by `src\Refitter\GenerateCommand.cs --simple-output` instead of re-parsing `.refitter` contents. This removes duplicated output-path prediction logic and keeps MSBuild compile items aligned with the CLI's actual writes. -- **MSBuild include filtering semantics:** `RefitterIncludePatterns` now matches only exact filenames, exact project-relative paths, or exact full paths. Substring matching was removed; `apis\petstore.refitter` is now a stable way to target one file without over-including similarly named files. -- **SourceGenerator dependency boundary:** `src\Refitter.SourceGenerator\Refitter.SourceGenerator.csproj` keeps `OasReader` private to the generator package and hides `Refit` compile assets from consumers (`PrivateAssets="compile"`). Source generator consumers must carry their own explicit `Refit` reference so Refitter does not silently upgrade them to Refit 10. -- **Focused validation that worked reliably:** use a repo-local NuGet cache (`C:\projects\christianhelle\refitter\.nuget\packages`) when the shared global cache is locked, then run targeted TUnit treenode filters from `src\Refitter.Tests\bin\Release\net10.0\Refitter.Tests.exe` for fast regression checks. - -### 2026-04-25: Audit Matrix Narrowing - -- Ripley's remaining #1057 matrix pass treated #1047 as already fixed at HEAD because MSBuild now follows CLI `GeneratedFile:` markers. -- #1042 is validation-only and #1056 is doc/invariant-only, so remaining tooling/code follow-up is narrowed to the still-open code-backed items. - -### 2026-04-25: Lambert Repro Narrowing - -- Lambert's evidence pass keeps **#1029** and **#1041** only as **partial tooling repros** on current HEAD. -- **#1043** still reproduces as the legacy bool-style `--generate-authentication-header` CLI break. -- **#1042** remains validation-only and **#1047** remains fixed-at-HEAD unless a fresh failing packaged repro appears. - -### 2026-04-25: Queued Core Revision Follow-up - -- Ash rejected Parker's latest closure set for the #1057 core artifact. -- **#1034** and **#1039** remain open and require real fixes. -- Dallas is queued to take the next revision after the current tooling lane finishes, with Lambert adding blocker tests first. - -### 2026-04-25: Tooling Lane Complete - -- Completed the tooling lane with real fixes landed for **#1028**, **#1029**, **#1041**, and **#1043**. -- Validation outcome for the remaining tooling-adjacent checks: **#1042** is no-code / validation-only at current HEAD, and **#1047** is fixed-at-HEAD because MSBuild now follows CLI-emitted `GeneratedFile:` markers. -- Dallas reported successful build, test, and format validation for the tooling slice. -- Follow-up ownership is now active: Dallas moved immediately onto the rejected core revision for **#1034**/**#1039** because Parker is locked out. - -### 2026-04-25: Core Revision Lane Complete - -- Completed the non-Parker revision for **#1034** and **#1039** after Ash rejected Parker's earlier closure set. -- `OpenApiDocumentFactory.Merge()` now clones the first input before merge and warns on path/schema collisions instead of mutating the caller-owned document. -- `ParameterExtractor` now preserves the shared `operationModel.Parameters` list while assembling grouped query-parameter wrappers. -- Regression coverage for the core blockers was updated, and Dallas reported the revised validation lane green. -- Follow-up handoff is active: Ash is re-reviewing the revised core changes and Lambert is reconciling the blocker-test lane. - - -### 2026-04-25: Core Revision Partial Acceptance - -- Ash cleared **#1039** on the revised core lane: ParameterExtractor no longer mutates the shared operationModel.Parameters list, and the new coverage locked that in. -- Ash kept **#1034** open because merge collisions still warn and keep the first entry instead of throwing on conflicting multi-spec inputs. -- Dallas owns one last narrow revision to flip the merge-collision behavior and its tests to fail-fast semantics. - -### 2026-04-25: Final Narrow #1034 Revision - -- Completed the last implementation pass for **#1034** after Ash's partial re-review kept the warning-backed merge contract open. -- `OpenApiDocumentFactory.Merge()` now keeps the clone-first non-mutation guarantee while failing fast on conflicting duplicate path/schema/definition/security keys instead of silently keeping the first entry. -- Updated merge regression coverage now locks the fail-fast contract; Ash is on the final review gate while Lambert reconciles the blocker-test lane. - -### 2026-04-25: Final #1034 Gate Rejected - -- Ash rejected Dallas's latest #1034 revision at the final gate. -- The blocker proof is still incomplete because the test surface does not explicitly cover conflicting duplicate schema, definition, and security-scheme merges. -- Broader core validation also reported a failing `Dynamic_Querystring_Generation_Preserves_Original_Query_Param_Documentation(ByEndpoint)` regression in `Issue1039_DynamicQuerystringMutationTests`. -- Dallas is now locked out of the next revision cycle for this artifact; Lambert owns the next/final revision cycle. - -### 2026-04-25: Post-Lockout Handoff Landed - -- Lambert completed the final **#1034** ownership pass after the Parker and Dallas lockouts. -- The blocker proof now includes the schema, definition, and security-scheme collision surfaces that were still missing at the last gate. -- **#1039** is now tracked as a brittle regression assertion update instead of reopened core behavior. -- Validation was reported green; Ash owns the final reviewer gate. - - -### 2026-04-25: Core Artifact Lockout Set Extended - -- Lambert's follow-up proof pass was also rejected at Ash's gate. -- Dallas remains locked out of the next revision cycle for this artifact, now alongside Parker and Lambert. -- Ripley inherits the next narrow #1034 revision cycle. +- Investigated the persistent Ubuntu Actions failure for `Program_Main_Should_Show_Help_When_Invoked_Without_Arguments`. +- Confirmed the CLI/help path is correct: `Program.cs` rewrites no-args to `--help`, and `Settings.cs` metadata is rendered by `Spectre.Console.Cli` without platform-specific product logic. +- Proved the failure was formatter noise in the raw redirected help payload: ANSI sequences and host-dependent wrapping differed across Linux Actions and local Windows captures while preserving the same semantic help content. +- Handed off the recommended fix to Lambert: normalize redirected console output first, then assert semantic help markers in `src\Refitter.Tests\GenerateCommandTests.cs`. +- Lambert's final landed change stayed test-only and validated green with `dotnet build -c Release src\Refitter.slnx`, `dotnet test -c Release src\Refitter.slnx`, and `dotnet format --verify-no-changes src\Refitter.slnx`. diff --git a/.squad/agents/lambert/history.md b/.squad/agents/lambert/history.md index 22d9a7bb1..e01c3f356 100644 --- a/.squad/agents/lambert/history.md +++ b/.squad/agents/lambert/history.md @@ -10,6 +10,7 @@ - Team initialized on 2026-04-16. - **2026-04-25 CLI help repro:** src\Refitter\Program.cs intentionally rewrites a no-argument invocation to --help, exits 0, and emits Spectre.Console.Cli help output. Tests in src\Refitter.Tests\GenerateCommandTests.cs should assert semantic help markers like usage, sections, and option names rather than exact formatter-driven spacing. +- **2026-04-25 Linux help-test follow-up:** GitHub Actions on Ubuntu still showed the semantic help text, but the raw redirected Spectre output did not satisfy the single regex assertion. The safe regression contract is to normalize console control sequences/line endings first and then assert semantic help markers (`USAGE`, usage text, sections, known option names). - **PR #1064 / #1057 testing pattern:** When blocker work is in flux, Lambert's safest lane is minimal repro specs plus compilation gates, then focused test reruns once the implementing lane lands. ## Core Context @@ -38,3 +39,9 @@ - The durable test contract is semantic Spectre.Console.Cli help assertions, not exact whitespace/layout matching. - Validation reported green for the release Refitter.Tests run, a focused rerun of Program_Main_Should_Show_Help_When_Invoked_Without_Arguments, and format verification. +## 2026-04-25: Linux Help Output Fix Landed + +- Dallas's Ubuntu log analysis proved the failure was raw ANSI/wrapping noise from Spectre.Console help output rather than a CLI product bug. +- Lambert changed only src\Refitter.Tests\GenerateCommandTests.cs, normalizing redirected console output and asserting semantic help markers instead of formatter-specific layout. +- Reported final validation: dotnet build -c Release src\Refitter.slnx, dotnet test -c Release src\Refitter.slnx, and dotnet format --verify-no-changes src\Refitter.slnx. + diff --git a/.squad/agents/scribe/history.md b/.squad/agents/scribe/history.md index 25445b8d7..f7991a475 100644 --- a/.squad/agents/scribe/history.md +++ b/.squad/agents/scribe/history.md @@ -10,3 +10,4 @@ - Team initialized on 2026-04-16. - **2026-04-25: Lambert help-output consolidation:** Active decisions now archive older sections once decisions.md grows past ~20 KB, and Spectre.Console.Cli help regressions should be recorded as semantic-marker expectations rather than exact layout snapshots. +- **2026-04-25: Linux help-output merge:** When Dallas proves a formatter/noise failure and Lambert lands the test-only stabilization, record the root cause as ANSI/wrapping noise in raw Spectre output and keep the decision phrased as a semantic-assertion contract. diff --git a/.squad/decisions.md b/.squad/decisions.md index 9ab794b71..bb78a7ca1 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -200,5 +200,7 @@ - The current product behavior is correct; the instability sits in whitespace-sensitive test expectations, not in production code. - src\Refitter.Tests\GenerateCommandTests.cs should assert semantic help markers (usage pattern, sections, and known option names) instead of exact formatter-driven spacing/default-value layout. - Validation reported: release run of src\Refitter.Tests\Refitter.Tests.csproj, focused rerun of Program_Main_Should_Show_Help_When_Invoked_Without_Arguments, and dotnet format --verify-no-changes src\Refitter.slnx. +- Cross-agent merge outcome: Dallas proved the Ubuntu failure was ANSI/wrapping noise in raw Spectre.Console help output, and Lambert landed the test-only fix in src\Refitter.Tests\GenerateCommandTests.cs by normalizing redirected console output before asserting semantic help markers. +- Final landed validation for the product commit `normalize help output test across platforms`: dotnet build -c Release src\Refitter.slnx, dotnet test -c Release src\Refitter.slnx, and dotnet format --verify-no-changes src\Refitter.slnx. diff --git a/.squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md b/.squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md new file mode 100644 index 000000000..8b420eae0 --- /dev/null +++ b/.squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md @@ -0,0 +1,23 @@ +# Session Log: Linux Help Output Consolidation + +**Timestamp:** 2026-04-25T15:59:12Z +**Session Type:** Squad Scribe Consolidation +**Reference:** Linux help-output test failure on GitHub Actions +**Requested By:** Christian Helle + +## Spawn Manifest + +- **Agents:** Dallas, Lambert +- **Topic:** Linux help-output test failure on GitHub Actions +- **Outcome:** Dallas proved the failure was ANSI/wrapping noise from Spectre help output rather than a product bug. Lambert changed only `src\Refitter.Tests\GenerateCommandTests.cs` to normalize console output and assert semantic help markers. +- **Product commit:** `normalize help output test across platforms` +- **Validation reported:** `dotnet build -c Release src\Refitter.slnx`, `dotnet test -c Release src\Refitter.slnx`, and `dotnet format --verify-no-changes src\Refitter.slnx`. + +## Actions Completed + +1. Wrote orchestration logs for Dallas and Lambert. +2. Wrote this session log. +3. Merged the Linux help-output inbox context into `.squad/decisions.md` without duplicating the existing semantic-help decision. +4. Updated Dallas, Lambert, and Scribe histories with the cross-agent handoff and landed validation. +5. Summarized older Dallas history into `## Core Context` because the file exceeded the size threshold. +6. Deleted the merged inbox files from `.squad/decisions/inbox/`. diff --git a/.squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md b/.squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md new file mode 100644 index 000000000..96bf1e41e --- /dev/null +++ b/.squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md @@ -0,0 +1,26 @@ +# Orchestration Log: dallas-linux-help-analysis + +**Timestamp:** 2026-04-25T15:59:12Z +**Agent:** Dallas +**Requested By:** Christian Helle +**Task:** Diagnose the Linux GitHub Actions help-output failure +**Status:** Completed + +## Outcome + +- Dallas proved the failing Ubuntu check was not a CLI product regression. +- The raw redirected Spectre.Console help payload differed across hosts because of ANSI styling and formatter-driven wrapping. +- The actionable fix belonged in the test contract: normalize output, then assert semantic help markers. + +## Key Results + +- `src\Refitter\Program.cs` remains correct: no-arg invocation rewrites to `--help` and exits successfully. +- The Linux failure was caused by raw-output portability noise, not missing help content. +- Dallas handed Lambert a test-only stabilization path that preserved product behavior. + +## Files Touched + +- .squad\decisions.md +- .squad\log\2026-04-25T15-59-12Z-linux-help-output-consolidation.md +- .squad\orchestration-log\2026-04-25T15-59-12Z-dallas-linux-help-analysis.md +- .squad\agents\dallas\history.md diff --git a/.squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md b/.squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md new file mode 100644 index 000000000..33a6097d7 --- /dev/null +++ b/.squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md @@ -0,0 +1,26 @@ +# Orchestration Log: lambert-linux-help-test + +**Timestamp:** 2026-04-25T15:59:12Z +**Agent:** Lambert +**Requested By:** Christian Helle +**Task:** Land the test-only fix for the Linux help-output failure +**Status:** Completed + +## Outcome + +- Lambert changed only `src\Refitter.Tests\GenerateCommandTests.cs`. +- The test now normalizes redirected console output and checks semantic help markers instead of formatter-sensitive raw layout. +- The product commit stayed scoped to cross-platform help-test stabilization. + +## Key Results + +- The CLI help path remains unchanged. +- The regression contract now tolerates ANSI/wrapping differences across hosts. +- Reported validation was green for build, full test, and format verification. + +## Files Touched + +- .squad\decisions.md +- .squad\log\2026-04-25T15-59-12Z-linux-help-output-consolidation.md +- .squad\orchestration-log\2026-04-25T15-59-12Z-lambert-linux-help-test.md +- .squad\agents\lambert\history.md From ef11697fb75814360beb02b3dd054c3de46bc3e7 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 22:16:41 +0200 Subject: [PATCH 6/9] Cover RefitterGenerateTask edge cases Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../RefitterGenerateTaskTests.cs | 174 ++++++++++++++++++ 1 file changed, 174 insertions(+) diff --git a/src/Refitter.Tests/RefitterGenerateTaskTests.cs b/src/Refitter.Tests/RefitterGenerateTaskTests.cs index 0f4e42b94..54be397e3 100644 --- a/src/Refitter.Tests/RefitterGenerateTaskTests.cs +++ b/src/Refitter.Tests/RefitterGenerateTaskTests.cs @@ -437,6 +437,85 @@ public void ResolveRefitterDll_Should_Fall_Back_When_Preferred_Runtime_Binary_Is } } + [Test] + public void ResolveRefitterDll_Should_Return_Null_When_PackageFolder_Is_Blank() + { + var messages = new List(); + + var result = RefitterGenerateTask.ResolveRefitterDll(" ", ["Microsoft.NETCore.App 10.0.0"], messages.Add); + + result.Should().BeNull(); + messages.Should().BeEmpty(); + } + + [Test] + public void ResolveRefitterDll_Should_Ignore_Whitespace_Runtime_Entries_When_Falling_Back() + { + try + { + var packageFolder = Path.Combine("C:", "repo", "tasks"); + var messages = new List(); + var net8Path = Path.GetFullPath(Path.Combine(packageFolder, "..", "net8.0", "refitter.dll")); + RefitterGenerateTask.FileExists = path => string.Equals(path, net8Path, StringComparison.OrdinalIgnoreCase); + + var result = RefitterGenerateTask.ResolveRefitterDll( + packageFolder, + [" ", "Microsoft.NETCore.App 7.0.0"], + messages.Add); + + result.Should().Be(net8Path); + messages.Should().Contain(message => message.Contains("Falling back to bundled .NET 8.0 version of Refitter.", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + } + } + + [Test] + public void ResolveRefitterDll_Should_Fall_Back_To_CoLocated_Cli() + { + try + { + var packageFolder = Path.Combine("C:", "repo", "tasks"); + var messages = new List(); + var coLocatedCli = Path.GetFullPath(Path.Combine(packageFolder, "refitter.dll")); + RefitterGenerateTask.FileExists = path => string.Equals(path, coLocatedCli, StringComparison.OrdinalIgnoreCase); + + var result = RefitterGenerateTask.ResolveRefitterDll( + packageFolder, + ["Microsoft.NETCore.App 7.0.0"], + messages.Add); + + result.Should().Be(coLocatedCli); + messages.Should().ContainSingle(message => message.Contains("Falling back to co-located Refitter CLI.", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + } + } + + [Test] + public void ResolveRefitterDll_Should_Return_First_Bundled_Path_When_No_Binaries_Exist() + { + try + { + var packageFolder = Path.Combine("C:", "repo", "tasks"); + var messages = new List(); + RefitterGenerateTask.FileExists = _ => false; + + var result = RefitterGenerateTask.ResolveRefitterDll(packageFolder, null, messages.Add); + + result.Should().Be(Path.GetFullPath(Path.Combine(packageFolder, "..", "net10.0", "refitter.dll"))); + messages.Should().BeEmpty(); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + } + } + [Test] public void Execute_Should_Use_DotNet9_Runtime_When_Available() { @@ -512,6 +591,33 @@ public void Execute_Should_Fall_Back_To_DotNet8_Runtime_When_Newer_Runtimes_Are_ } } + [Test] + public void Execute_Should_Return_False_When_Refitter_Cli_Cannot_Be_Located() + { + var workspace = CreateWorkspace(); + + try + { + CreateRefitterSettingsFile(workspace); + RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => null!; + RefitterGenerateTask.FileExists = _ => false; + + var buildEngine = new RecordingBuildEngine(); + var task = CreateTask(workspace, buildEngine); + + var result = task.Execute(); + + result.Should().BeFalse(); + task.GeneratedFiles.Should().BeEmpty(); + buildEngine.Errors.Should().Contain(message => message.Contains("Unable to locate a bundled Refitter CLI runtime for the MSBuild task.", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + DeleteWorkspace(workspace); + } + } + [Test] public void Execute_Should_Log_Timeout_When_Process_Does_Not_Exit() { @@ -539,6 +645,33 @@ public void Execute_Should_Log_Timeout_When_Process_Does_Not_Exit() } } + [Test] + public void Execute_Should_Log_Millisecond_Timeout_Value() + { + var workspace = CreateWorkspace(); + + try + { + CreateRefitterSettingsFile(workspace); + RefitterGenerateTask.ProcessTimeoutMilliseconds = 500; + RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => ["Microsoft.NETCore.App 10.0.0"]; + RefitterGenerateTask.ProcessRunner = (_, _, _) => new RefitterGenerateTask.ProcessExecutionResult(true, -1); + + var buildEngine = new RecordingBuildEngine(); + var task = CreateTask(workspace, buildEngine); + + var result = task.Execute(); + + result.Should().BeFalse(); + buildEngine.Errors.Should().Contain(message => message.Contains("timed out after 500 ms", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + DeleteWorkspace(workspace); + } + } + [Test] public void Execute_Should_Log_When_Timed_Out_Process_Cannot_Be_Terminated() { @@ -595,6 +728,36 @@ public void Execute_Should_Log_Configured_Timeout_Value() } } + [Test] + public void Execute_Should_Log_ProcessRunner_Exception_And_Return_False() + { + var workspace = CreateWorkspace(); + + try + { + CreateRefitterSettingsFile(workspace); + RefitterGenerateTask.InstalledDotnetRuntimesProvider = () => ["Microsoft.NETCore.App 10.0.0"]; + RefitterGenerateTask.FileExists = path => + path.Contains("net10.0", StringComparison.OrdinalIgnoreCase) || + File.Exists(path); + RefitterGenerateTask.ProcessRunner = (_, _, _) => throw new InvalidOperationException("boom"); + + var buildEngine = new RecordingBuildEngine(); + var task = CreateTask(workspace, buildEngine); + + var result = task.Execute(); + + result.Should().BeFalse(); + task.GeneratedFiles.Should().BeEmpty(); + buildEngine.Errors.Should().Contain(message => message.Contains("boom", StringComparison.Ordinal)); + } + finally + { + RefitterGenerateTask.ResetTestHooks(); + DeleteWorkspace(workspace); + } + } + [Test] public void Execute_Should_Log_When_Process_Exits_With_Non_Zero_Code() { @@ -686,6 +849,17 @@ public void TryLogErrorFromException_Should_Swallow_BuildEngine_Exceptions() action.Should().NotThrow(); } + [Test] + public void TryLogErrorFromException_Should_Log_When_BuildEngine_Allows_It() + { + var buildEngine = new RecordingBuildEngine(); + var task = new RefitterGenerateTask { BuildEngine = buildEngine }; + + InvokePrivateMethod(task, "TryLogErrorFromException", new InvalidOperationException("boom")); + + buildEngine.Errors.Should().Contain(message => message.Contains("boom", StringComparison.Ordinal)); + } + private static string CreateWorkspace() { var workspace = Path.Combine(AppContext.BaseDirectory, "RefitterGenerateTaskTests", Guid.NewGuid().ToString("N")); From 718f592a4216b5ae853fc59afd315ac365ae1a8b Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 22:18:32 +0200 Subject: [PATCH 7/9] chore: record coverage squad log Record Dallas and Lambert coverage closure state in squad logs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/dallas/history.md | 8 ++++++++ .squad/agents/lambert/history.md | 13 +++++++++++++ .squad/agents/scribe/history.md | 1 + .squad/decisions.md | 11 +++++++++++ 4 files changed, 33 insertions(+) diff --git a/.squad/agents/dallas/history.md b/.squad/agents/dallas/history.md index 890c4a271..a3933f1c5 100644 --- a/.squad/agents/dallas/history.md +++ b/.squad/agents/dallas/history.md @@ -33,3 +33,11 @@ - Proved the failure was formatter noise in the raw redirected help payload: ANSI sequences and host-dependent wrapping differed across Linux Actions and local Windows captures while preserving the same semantic help content. - Handed off the recommended fix to Lambert: normalize redirected console output first, then assert semantic help markers in `src\Refitter.Tests\GenerateCommandTests.cs`. - Lambert's final landed change stayed test-only and validated green with `dotnet build -c Release src\Refitter.slnx`, `dotnet test -c Release src\Refitter.slnx`, and `dotnet format --verify-no-changes src\Refitter.slnx`. + +## 2026-04-25: RefitterGenerateTask Coverage Closure + +- Closed the remaining `src\Refitter.MSBuild\RefitterGenerateTask.cs` coverage gaps with test-only changes in `src\Refitter.Tests\RefitterGenerateTaskTests.cs`. +- Lambert's gap analysis isolated the exact missing defensive branches first, which let Dallas keep the implementation pass scoped to regression coverage instead of production changes. +- Added direct coverage for the defensive tooling paths: blank package folder, whitespace runtime entries, co-located CLI fallback, first-bundled-path fallback, missing bundled CLI failure, process-runner exception handling, millisecond timeout formatting, and successful `LogErrorFromException` forwarding. +- Validation reported green with `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --coverage --coverage-output coverage.cobertura.xml --coverage-output-format xml`, `dotnet build -c Release src\Refitter.slnx --no-restore`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore`. +- The reported end state for `src\Refitter.MSBuild\RefitterGenerateTask.cs` was 100% line coverage, 100% block coverage, and 0 partial functions. diff --git a/.squad/agents/lambert/history.md b/.squad/agents/lambert/history.md index e01c3f356..d2c8c99fc 100644 --- a/.squad/agents/lambert/history.md +++ b/.squad/agents/lambert/history.md @@ -45,3 +45,16 @@ - Lambert changed only src\Refitter.Tests\GenerateCommandTests.cs, normalizing redirected console output and asserting semantic help markers instead of formatter-specific layout. - Reported final validation: dotnet build -c Release src\Refitter.slnx, dotnet test -c Release src\Refitter.slnx, and dotnet format --verify-no-changes src\Refitter.slnx. +## 2026-04-25: RefitterGenerateTask coverage gap analysis + +- Coverage evidence from `src\Refitter.Tests\bin\Release\net10.0\TestResults\coverage-all.xml` leaves `src\Refitter.MSBuild\RefitterGenerateTask.cs` at **91.86% line / 94.29% block** coverage. +- Exact uncovered lanes are: `TryExecuteRefitter()` exception handling (lines 143-147), missing bundled CLI handling in `StartProcess()` (171-173), unresolved package-folder / co-located CLI / final-first-bundle fallbacks in `ResolveRefitterDll()` (304, 344-348, 351-353), the `<1000 ms` arm of `FormatTimeout()` (357-361 partial), and the non-throwing `Log.LogErrorFromException(e)` path (370). +- Full `Refitter.Tests` coverage run still hit the known network-dependent failures (`IsHttp_Detects_Https_Protocol` and `Can_Build_Generated_Code_From_Url(...)`), but it produced the decisive per-function coverage report needed for Dallas. + +## 2026-04-25: RefitterGenerateTask coverage closure landed + +- Dallas used the isolated branch list to add only regression coverage in `src\Refitter.Tests\RefitterGenerateTaskTests.cs`; no product behavior changes were needed in `src\Refitter.MSBuild\RefitterGenerateTask.cs`. +- The landed test pass covered exception handling, missing bundled CLI failure, blank package folder handling, whitespace runtime probing, co-located and first-bundled fallback resolution, millisecond timeout formatting, and successful `LogErrorFromException` forwarding. +- Reported validation: `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --coverage --coverage-output coverage.cobertura.xml --coverage-output-format xml`, `dotnet build -c Release src\Refitter.slnx --no-restore`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore`. +- Reported end state: `src\Refitter.MSBuild\RefitterGenerateTask.cs` at 100% line coverage, 100% block coverage, and 0 partial functions. + diff --git a/.squad/agents/scribe/history.md b/.squad/agents/scribe/history.md index f7991a475..af99f48b2 100644 --- a/.squad/agents/scribe/history.md +++ b/.squad/agents/scribe/history.md @@ -11,3 +11,4 @@ - Team initialized on 2026-04-16. - **2026-04-25: Lambert help-output consolidation:** Active decisions now archive older sections once decisions.md grows past ~20 KB, and Spectre.Console.Cli help regressions should be recorded as semantic-marker expectations rather than exact layout snapshots. - **2026-04-25: Linux help-output merge:** When Dallas proves a formatter/noise failure and Lambert lands the test-only stabilization, record the root cause as ANSI/wrapping noise in raw Spectre output and keep the decision phrased as a semantic-assertion contract. +- **2026-04-25: Coverage-closure merge:** When Lambert isolates residual coverage gaps and Dallas closes them with test-only coverage, merge the inboxes into a single approved decision that preserves the no-production-change rationale and the final validation evidence. diff --git a/.squad/decisions.md b/.squad/decisions.md index bb78a7ca1..6cd4bb2de 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -203,4 +203,15 @@ - Cross-agent merge outcome: Dallas proved the Ubuntu failure was ANSI/wrapping noise in raw Spectre.Console help output, and Lambert landed the test-only fix in src\Refitter.Tests\GenerateCommandTests.cs by normalizing redirected console output before asserting semantic help markers. - Final landed validation for the product commit `normalize help output test across platforms`: dotnet build -c Release src\Refitter.slnx, dotnet test -c Release src\Refitter.slnx, and dotnet format --verify-no-changes src\Refitter.slnx. +### RefitterGenerateTask edge-case coverage stays test-only + +**Verified By:** Dallas / Lambert +**Status:** APPROVED + +- Preserve the current `src\Refitter.MSBuild\RefitterGenerateTask.cs` behavior and close the remaining coverage gap with regression tests instead of production changes. +- Lambert isolated the last uncovered branches to `TryExecuteRefitter()` exception handling, missing bundled CLI handling in `StartProcess()`, `ResolveRefitterDll()` fallback edges, sub-second timeout formatting, and the non-throwing `TryLogErrorFromException()` path. +- Dallas landed the test-only closure in `src\Refitter.Tests\RefitterGenerateTaskTests.cs`, covering blank package folders, whitespace runtime entries, co-located and first-bundled fallback resolution, missing bundled CLI failure, process-runner exception handling, millisecond timeout formatting, and successful `LogErrorFromException` forwarding. +- Reported validation: `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --coverage --coverage-output coverage.cobertura.xml --coverage-output-format xml`, `dotnet build -c Release src\Refitter.slnx --no-restore`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore`. +- Result: `RefitterGenerateTask.cs` reached 100% line coverage, 100% block coverage, and 0 partial functions in the reported coverage output. + From 62122ac53b0e29ff8ca325d7f079887e99794802 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 23:14:15 +0200 Subject: [PATCH 8/9] Address SonarCloud quality gate issues Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/Refitter.Core/ParameterExtractor.cs | 119 +++++++++--------- src/Refitter.MSBuild/RefitterGenerateTask.cs | 30 +++-- .../RefitterSourceGenerator.cs | 54 +++++--- 3 files changed, 113 insertions(+), 90 deletions(-) diff --git a/src/Refitter.Core/ParameterExtractor.cs b/src/Refitter.Core/ParameterExtractor.cs index aad8c95f7..ee40b7945 100644 --- a/src/Refitter.Core/ParameterExtractor.cs +++ b/src/Refitter.Core/ParameterExtractor.cs @@ -425,94 +425,91 @@ private static List GetQueryParameters(CSharpOperationModel operationMod .Where(p => p.Kind == OpenApiParameterKind.Query) .ToList(); - if (settings.UseDynamicQuerystringParameters) + if (settings.UseDynamicQuerystringParameters && queryParameters.Count >= 2) { - if (queryParameters.Count >= 2) + var modifier = settings.TypeAccessibility.ToString().ToLowerInvariant(); + var isRecord = settings.ImmutableRecords || + settings.CodeGeneratorSettings?.GenerateNativeRecords is true; + var classStyle = isRecord + ? "record" + : "class"; + var setterStyle = isRecord + ? "init" + : "set"; + + var injectedParametersCodeBuilder = new StringBuilder(); + var initializedParametersCodeBuilder = new StringBuilder(); + var propertiesCodeBuilder = new StringBuilder(); + var allNullable = true; + foreach (var operationParameter in queryParameters) { - var modifier = settings.TypeAccessibility.ToString().ToLowerInvariant(); - var isRecord = settings.ImmutableRecords || - settings.CodeGeneratorSettings?.GenerateNativeRecords is true; - var classStyle = isRecord - ? "record" - : "class"; - var setterStyle = isRecord - ? "init" - : "set"; - - var injectedParametersCodeBuilder = new StringBuilder(); - var initializedParametersCodeBuilder = new StringBuilder(); - var propertiesCodeBuilder = new StringBuilder(); - var allNullable = true; - foreach (var operationParameter in queryParameters) + var propertyType = GetQueryParameterType(operationParameter, settings); + allNullable = allNullable && propertyType.EndsWith("?"); + var variableName = GetVariableName(operationParameter); + var attributes = $"{JoinAttributes(GetQueryAttribute(operationParameter, settings), GetAliasAsAttribute(operationParameter.Name, variableName))}"; + var propertyName = variableName.CapitalizeFirstCharacter(); + if (operationParameter.IsRequired) { - var propertyType = GetQueryParameterType(operationParameter, settings); - allNullable = allNullable && propertyType.EndsWith("?"); - var variableName = GetVariableName(operationParameter); - var attributes = $"{JoinAttributes(GetQueryAttribute(operationParameter, settings), GetAliasAsAttribute(operationParameter.Name, variableName))}"; - var propertyName = variableName.CapitalizeFirstCharacter(); - if (operationParameter.IsRequired) - { - injectedParametersCodeBuilder.Append(injectedParametersCodeBuilder.Length == 0 - ? $$"""{{propertyType}} {{variableName}}""" - : $$""", {{propertyType}} {{variableName}}"""); + injectedParametersCodeBuilder.Append(injectedParametersCodeBuilder.Length == 0 + ? $$"""{{propertyType}} {{variableName}}""" + : $$""", {{propertyType}} {{variableName}}"""); - initializedParametersCodeBuilder.AppendLine(); - initializedParametersCodeBuilder.Append( - $$""" + initializedParametersCodeBuilder.AppendLine(); + initializedParametersCodeBuilder.Append( + $$""" this.{{propertyName}} = {{variableName}}; """); - } + } - propertiesCodeBuilder.AppendLine(); - if (settings.GenerateXmlDocCodeComments && !string.IsNullOrWhiteSpace(operationParameter.Description)) - { - var escapedDescription = XmlDocumentationGenerator.SanitizeResponseDescription(operationParameter.Description); - AppendXmlDocComment(escapedDescription, propertiesCodeBuilder); - } + propertiesCodeBuilder.AppendLine(); + if (settings.GenerateXmlDocCodeComments && !string.IsNullOrWhiteSpace(operationParameter.Description)) + { + var escapedDescription = XmlDocumentationGenerator.SanitizeResponseDescription(operationParameter.Description); + AppendXmlDocComment(escapedDescription, propertiesCodeBuilder); + } - propertiesCodeBuilder.Append( - $$""" + propertiesCodeBuilder.Append( + $$""" {{attributes}} {{modifier}} {{propertyType}} {{propertyName}} { get; {{setterStyle}}; } """); - var defaultValue = operationParameter.Schema.Default; - if (defaultValue != null) - { - var formattedDefaultValue = FormatDefaultValue(defaultValue, propertyType); - propertiesCodeBuilder.Append($" = {formattedDefaultValue};"); - } - propertiesCodeBuilder.AppendLine(); + var defaultValue = operationParameter.Schema.Default; + if (defaultValue != null) + { + var formattedDefaultValue = FormatDefaultValue(defaultValue, propertyType); + propertiesCodeBuilder.Append($" = {formattedDefaultValue};"); } + propertiesCodeBuilder.AppendLine(); + } - dynamicQuerystringParametersCodeBuilder.AppendLine( - $$""" + dynamicQuerystringParametersCodeBuilder.AppendLine( + $$""" {{modifier}} {{classStyle}} {{dynamicQuerystringParameterType}} { """); - if (injectedParametersCodeBuilder.Length > 0) - { - dynamicQuerystringParametersCodeBuilder.AppendLine( - $$""" + if (injectedParametersCodeBuilder.Length > 0) + { + dynamicQuerystringParametersCodeBuilder.AppendLine( + $$""" {{modifier}} {{dynamicQuerystringParameterType}}({{injectedParametersCodeBuilder}}) { {{initializedParametersCodeBuilder}} } """); - } + } - dynamicQuerystringParametersCodeBuilder.AppendLine( - $$""" + dynamicQuerystringParametersCodeBuilder.AppendLine( + $$""" {{propertiesCodeBuilder}} } """); - var dynamicQuerystringParameter = $"[Query] {dynamicQuerystringParameterType}"; - if (allNullable) - dynamicQuerystringParameter += "?"; - dynamicQuerystringParameter += " queryParams"; - parameters = [dynamicQuerystringParameter]; - } + var dynamicQuerystringParameter = $"[Query] {dynamicQuerystringParameterType}"; + if (allNullable) + dynamicQuerystringParameter += "?"; + dynamicQuerystringParameter += " queryParams"; + parameters = [dynamicQuerystringParameter]; } dynamicQuerystringParameters = dynamicQuerystringParametersCodeBuilder.ToString(); diff --git a/src/Refitter.MSBuild/RefitterGenerateTask.cs b/src/Refitter.MSBuild/RefitterGenerateTask.cs index ef8afa808..6f73dbaec 100644 --- a/src/Refitter.MSBuild/RefitterGenerateTask.cs +++ b/src/Refitter.MSBuild/RefitterGenerateTask.cs @@ -315,11 +315,13 @@ private static List GetInstalledDotnetRuntimes() if (installedRuntimes is not null) { - foreach (var runtime in bundledRuntimes) + var detectedRuntimes = installedRuntimes + .Where(installed => !string.IsNullOrWhiteSpace(installed)) + .ToArray(); + + foreach (var runtime in bundledRuntimes.Where(runtime => FileExists(runtime.Path))) { - if (FileExists(runtime.Path) && - installedRuntimes.Any(installed => - !string.IsNullOrWhiteSpace(installed) && + if (detectedRuntimes.Any(installed => installed.StartsWith(runtime.RuntimePrefix, StringComparison.Ordinal))) { logCommandLine($"Detected {GetDisplayFramework(runtime.TargetFramework)} runtime. Using {GetDisplayFramework(runtime.TargetFramework)} version of Refitter."); @@ -353,12 +355,20 @@ private static List GetInstalledDotnetRuntimes() .FirstOrDefault(); } - private static string FormatTimeout(int timeoutMilliseconds) => - timeoutMilliseconds >= 1000 && timeoutMilliseconds % 1000 == 0 - ? $"{timeoutMilliseconds / 1000} seconds" - : timeoutMilliseconds >= 1000 - ? $"{timeoutMilliseconds / 1000d:0.###} seconds" - : $"{timeoutMilliseconds} ms"; + private static string FormatTimeout(int timeoutMilliseconds) + { + if (timeoutMilliseconds < 1000) + { + return $"{timeoutMilliseconds} ms"; + } + + if (timeoutMilliseconds % 1000 == 0) + { + return $"{timeoutMilliseconds / 1000} seconds"; + } + + return $"{timeoutMilliseconds / 1000d:0.###} seconds"; + } private static string GetDisplayFramework(string targetFramework) => targetFramework.Replace("net", ".NET "); diff --git a/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs b/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs index 44ede5c6e..364c595f9 100644 --- a/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs +++ b/src/Refitter.SourceGenerator/RefitterSourceGenerator.cs @@ -15,6 +15,7 @@ namespace Refitter.SourceGenerator; public class RefitterSourceGenerator : IIncrementalGenerator { internal const string Category = "Refitter"; + private const string RefitterDiagnosticTitle = Category; /// /// Initializes the incremental generator with the necessary configurations. @@ -146,20 +147,20 @@ internal static GeneratedDiagnostic CreateNoRefitterFilesFoundDiagnostic() => internal static GeneratedDiagnostic CreateGeneratedSuccessfullyDiagnostic(string hintName) => new( "REFITTER001", - "Refitter", - $"Refitter generated {hintName} successfully", + RefitterDiagnosticTitle, + $"{RefitterDiagnosticTitle} generated {hintName} successfully", DiagnosticSeverity.Info); private static GeneratedDiagnostic CreateFoundFileDiagnostic(string path) => new( - "REFITTER001", - "Refitter", + "REFITTER004", + RefitterDiagnosticTitle, $"Found .refitter File: {path}", DiagnosticSeverity.Info); private static GeneratedDiagnostic CreateFileContentsDiagnostic(string json) => new( - "REFITTER001", + "REFITTER005", "Refitter File Contents", json, DiagnosticSeverity.Info); @@ -206,7 +207,7 @@ private static string CreateUniqueHintName(string refitterFilePath, string? outp if (string.IsNullOrEmpty(baseName) || baseName == ".") { - baseName = "Refitter"; + baseName = RefitterDiagnosticTitle; } // Create a stable unique suffix from the directory path to prevent collisions @@ -246,20 +247,35 @@ internal readonly record struct GeneratedCode( string? Code = null, string? HintName = null); - internal readonly record struct GeneratedDiagnostic( - string Id, - string Title, - string Message, - DiagnosticSeverity Severity, - bool EnabledByDefault = true) - : IEquatable + [SuppressMessage( + "Major Code Smell", + "S1206:Equals(object) and GetHashCode() should be overridden in pairs", + Justification = "readonly record struct synthesizes the paired Equals overloads; only the hash code is customized to keep ordinal semantics explicit.")] + internal readonly record struct GeneratedDiagnostic : IEquatable { - public bool Equals(GeneratedDiagnostic other) => - string.Equals(Id, other.Id, StringComparison.Ordinal) && - string.Equals(Title, other.Title, StringComparison.Ordinal) && - string.Equals(Message, other.Message, StringComparison.Ordinal) && - Severity == other.Severity && - EnabledByDefault == other.EnabledByDefault; + public GeneratedDiagnostic( + string id, + string title, + string message, + DiagnosticSeverity severity, + bool enabledByDefault = true) + { + Id = id; + Title = title; + Message = message; + Severity = severity; + EnabledByDefault = enabledByDefault; + } + + public string Id { get; } + + public string Title { get; } + + public string Message { get; } + + public DiagnosticSeverity Severity { get; } + + public bool EnabledByDefault { get; } public override int GetHashCode() { From b5d309e4079c99142d202d3f95c4adaa4e45bd10 Mon Sep 17 00:00:00 2001 From: Christian Helle Date: Sat, 25 Apr 2026 23:20:10 +0200 Subject: [PATCH 9/9] docs(squad): log PR 1070 sonar gate Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .squad/agents/ash/history.md | 25 ++ .squad/agents/dallas/history.md | 14 + .squad/agents/parker/history.md | 456 +++----------------------------- .squad/decisions.md | 8 + 4 files changed, 89 insertions(+), 414 deletions(-) diff --git a/.squad/agents/ash/history.md b/.squad/agents/ash/history.md index 5c49428c8..2630dcf89 100644 --- a/.squad/agents/ash/history.md +++ b/.squad/agents/ash/history.md @@ -34,3 +34,28 @@ - Signed off that merge handling now stays clone-first, fails fast on conflicting duplicate path/schema/definition/security keys, and keeps grouped dynamic-query extraction non-mutating across single-interface, ByTag, and ByEndpoint generation. - Evidence reviewed included src\Refitter.Core\OpenApiDocumentFactory.cs, src\Refitter.Tests\OpenApiDocumentFactoryMergeTests.cs, src\Refitter.Tests\RegressionTests\Issue1039_DynamicQuerystringMutationTests.cs, and src\Refitter.Tests\ParameterExtractorPrivateCoverageTests.cs. - Final reviewer gate was reported green on dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj with 1840 passing and 0 failing. + +## 2026-04-25: SonarCloud PR #1070 gate review + +- Reviewed SonarCloud PR #1070 quality-gate output: 5 findings total, with the gate failing only because Sonar classified `src\Refitter.SourceGenerator\RefitterSourceGenerator.cs` `GeneratedDiagnostic` as a BUG (`S1206`). +- Treat the `S1206` finding as analyzer noise, not a real product bug: `GeneratedDiagnostic` is a `record struct`, so changing equality members just to satisfy Sonar would risk destabilizing incremental-generator equality semantics without fixing an observed defect. +- Treat the remaining findings in `src\Refitter.SourceGenerator\RefitterSourceGenerator.cs`, `src\Refitter.Core\ParameterExtractor.cs`, and `src\Refitter.MSBuild\RefitterGenerateTask.cs` as maintainability-only/style noise rather than product or test failures. +- Risk guidance: avoid style-only rewrites in the dynamic querystring extraction and MSBuild runtime-resolution/timeout-formatting paths, because those areas were recently hardened and a cleanup-only patch could regress real behavior while merely silencing Sonar. + +## 2026-04-25: Review of landed Sonar cleanup + +- Re-reviewed the landed cleanup on the three implicated files. The `S1066`, `S3267`, `S3358`, and `S1192` changes are acceptable as behavior-preserving cleanup only. +- Rejected the `S1206` fix direction: converting `GeneratedDiagnostic` from `readonly record struct` to a manual `readonly struct` is unnecessary churn for a likely Sonar false positive and weakens the safety story by making equality/hash maintenance manual. +- Gate guidance for Dallas: request changes only on the source-generator `GeneratedDiagnostic` rewrite; the other four Sonar items do not justify blocking once they stay narrow and behavior-preserving. + +## 2026-04-25: Parker follow-up on source-generator Sonar fix + +- Approved Parker's revised `src\Refitter.SourceGenerator\RefitterSourceGenerator.cs` artifact. +- The safe correction was restoring `GeneratedDiagnostic` to `readonly record struct` and suppressing `S1206` with an explicit justification, which preserves synthesized equality while keeping the custom ordinal `GetHashCode()` implementation. +- The `S1192` cleanup remains cosmetic and safe, and no new generator-safety regression is evident in the revised source-generator artifact. +## 2026-04-25: Scribe consolidation of PR #1070 + +- Final squad memory keeps Dallas's ParameterExtractor / RefitterGenerateTask cleanups and the source-generator diagnostic-ID cleanup as the approved behavior-preserving Sonar response. +- The only rejected direction was the first manual-struct S1206 rewrite; Parker's follow-up restored the readonly record struct shape and was the approved final source-generator artifact. +- Shared validation to cite for the consolidated PR #1070 outcome: dotnet build -c Release src\Refitter.slnx --no-restore, dotnet test -c Release --solution src\Refitter.slnx --no-build, and dotnet format --verify-no-changes src\Refitter.slnx --no-restore. + diff --git a/.squad/agents/dallas/history.md b/.squad/agents/dallas/history.md index a3933f1c5..b4fece1be 100644 --- a/.squad/agents/dallas/history.md +++ b/.squad/agents/dallas/history.md @@ -41,3 +41,17 @@ - Added direct coverage for the defensive tooling paths: blank package folder, whitespace runtime entries, co-located CLI fallback, first-bundled-path fallback, missing bundled CLI failure, process-runner exception handling, millisecond timeout formatting, and successful `LogErrorFromException` forwarding. - Validation reported green with `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --coverage --coverage-output coverage.cobertura.xml --coverage-output-format xml`, `dotnet build -c Release src\Refitter.slnx --no-restore`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore`. - The reported end state for `src\Refitter.MSBuild\RefitterGenerateTask.cs` was 100% line coverage, 100% block coverage, and 0 partial functions. + +## 2026-04-25: PR #1070 SonarCloud Quality Gate Repair + +- Pulled the live SonarCloud PR issue feed for `christianhelle_refitter` / PR `1070` and confirmed five leak-period findings: one reliability bug plus one maintainability finding in `src\Refitter.SourceGenerator\RefitterSourceGenerator.cs`, one maintainability finding in `src\Refitter.Core\ParameterExtractor.cs`, and two maintainability findings in `src\Refitter.MSBuild\RefitterGenerateTask.cs`. +- Collapsed the nested dynamic-query guard in `ParameterExtractor.GetQueryParameters()` to satisfy the clumsy-condition finding without changing the non-mutating grouped-query behavior. +- Simplified `ResolveRefitterDll()` to pre-filter whitespace runtime rows / missing bundled binaries and rewrote `FormatTimeout()` as straight-line conditionals so the MSBuild task no longer trips the Sonar loop and nested-ternary smells. +- Hardened source-generator diagnostics by reusing the shared `Refitter` title constant, assigning distinct stable IDs to the found-file and file-contents diagnostics, and replacing the `GeneratedDiagnostic` record struct with an explicit readonly struct that pairs `Equals(...)` with the custom hash code. +- Validation reported green with `dotnet build -c Release src\Refitter.slnx --no-restore`, `dotnet test -c Release src\Refitter.slnx --no-build`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore` (1894 tests passed). +## 2026-04-25: Scribe consolidation of PR #1070 + +- Ash approved Dallas's S1066, S3267, and S3358 cleanups plus the distinct diagnostic-ID/source-title cleanup as the safe parts of the Sonar response. +- Ash rejected Dallas's first S1206 direction because converting GeneratedDiagnostic away from a readonly record struct was risky semantic churn for an analyzer-only complaint. +- Parker superseded only the source-generator S1206 artifact; the final merged state keeps Dallas's ParameterExtractor and RefitterGenerateTask changes alongside Parker's approved record-struct suppression revision. + diff --git a/.squad/agents/parker/history.md b/.squad/agents/parker/history.md index 7b0e5f513..74efb224a 100644 --- a/.squad/agents/parker/history.md +++ b/.squad/agents/parker/history.md @@ -1,417 +1,45 @@ -# Parker History - -## Context - -- User: Christian Helle -- Product: Refitter generates C# REST API clients from OpenAPI specifications using Refit. -- Stack: .NET, Refit, NSwag, Source Generator, MSBuild, Microsoft OpenAPI.NET - -## Learnings - -- Team initialized on 2026-04-16. -- 2026-04-20: `JsonSerializerContextGenerator` must use Roslyn syntax analysis instead of regex, emit attributes inside the contracts namespace, register nested types plus closed generic usages, and strip a conventional leading `I` from serializer-context names. -- 2026-04-20: `GenerateJsonSerializerContext` is wired through both `RefitGenerator.Generate()` and `GenerateMultipleFiles()`; multi-file generation emits a dedicated `{ContextName}.cs` file alongside contracts. -- 2026-04-20: `GenerateNullableReferenceTypes` must not silently flip `GenerateOptionalPropertiesAsNullable`; optional-property nullability stays an explicit user choice in `CodeGeneratorSettings`. - -## 2026-04-20: P1 Audit Closures (#1017, #1026) - -**Task:** Close the remaining core P1 audit findings for AOT serializer-context generation and silent nullable-shape mutation. -**Status:** ✅ COMPLETE — 3 commits created, manual generator/CLI validation passed, focused regression tests added. - -**Implementation Summary:** -- **#1017:** Replaced regex-based serializer-context discovery with Roslyn syntax traversal in `src/Refitter.Core/JsonSerializerContextGenerator.cs`, added namespace-safe emission in `RefitGenerator`, and covered nested types, closed generics, multi-file output, and contracts-namespace separation in tests. -- **#1026:** Removed the automatic `GenerateOptionalPropertiesAsNullable` mutation from `src/Refitter.Core/CSharpClientGeneratorFactory.cs`; runtime regressions now assert that NRT alone preserves the old DTO shape while explicit opt-in still yields nullable optional properties. -- **Follow-up hardening:** Added fallback handling for null/blank `Naming.InterfaceName` when deriving serializer-context names and restored a missing Roslyn import in `RefitGenerator.cs` after review. - -**Validation Notes:** -- Built `src\Refitter.Core\Refitter.Core.csproj` and `src\Refitter\Refitter.csproj` successfully with a fresh local NuGet cache after shared-cache file locks blocked the default/global cache. -- Manual validation harness confirmed: - - direct serializer-context generation handles nested types and closed generics, - - single-file and multi-file AOT output compile in standalone projects, - - NRT-only generation keeps optional properties non-nullable unless `GenerateOptionalPropertiesAsNullable` is explicitly set. - -**Key File Paths:** -- `src/Refitter.Core/JsonSerializerContextGenerator.cs` -- `src/Refitter.Core/CSharpClientGeneratorFactory.cs` -- `src/Refitter.Core/RefitGenerator.cs` -- `src/Refitter.Tests/JsonSerializerContextGeneratorTests.cs` -- `src/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cs` -- `src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs` - -## 2026-04-20 Final Update: Blockers Implemented and Validated - -**Task:** Implement fixes for PR #1064 merge blockers -**Status:** ✅ COMPLETE — Parker's initial fixes + Ash's revision; all 1779 tests passing - -**Implementation Summary:** -- **#1013 (Collision Detection):** Parker implemented pre-flight collision check; Ash verified production-ready -- **#1018 (Multipart Dedup):** Parker's fix was incomplete; Ash diagnosed naming method mismatch and unified both code paths -- **#1053 (Keyword Escaping):** Parker's code fix was correct; Ash corrected test expectations for NSwag capitalization - -**Key Collaboration Points:** -- Parker's blocker gate review correctly identified gaps in initial implementation -- Dallas's validation report provided concrete test failures for Ash to root-cause -- Ripley's triage correctly predicted naming method mismatch in #1018 -- Ash's revision achieved comprehensive fix for all three blockers - -**Lessons for Future Work:** -1. When fixing multipart/parameter extraction, verify ALL code paths use same naming method -2. Case sensitivity in identifier deduplication is critical -3. NSwag's automatic schema capitalization bypasses post-generation sanitization -4. Test expectations must match actual framework behavior, not ideal behavior - -**Final Session Log:** `.squad/log/2026-04-20T16-00-14Z-pr1064-blocker-fixes.md` - -**Merge Status:** ✅ APPROVED (cleanup of temporary test JSON files required) - -### 2026-04-20: PR #1064 Core Closure Review - -**Task**: Re-review PR #1064 against the exact closed-issue list and verify that the current branch actually resolves the core-generator issues it claims to close. - -**Key Findings**: -- **#1013 remains PARTIAL**: moving from regex to Roslyn avoids comment/string corruption, but `ContractTypeSuffixApplier` still blindly maps `name -> name + suffix` with no collision detection. A spec that already contains `PetDto` still collides when `Pet` is suffixed to `PetDto`, and `VisitIdentifierName()` rewrites every matching identifier token rather than only proven type contexts. -- **#1018 remains PARTIAL**: multipart manual extraction still deduplicates by the original OpenAPI key (`property.Key`) instead of the sanitized identifier. Reproducing with `Class/class` and `user-name/user name` still emits duplicate `@class` and `user_name` parameters. -- **#1014 remains PARTIAL**: internal enum handling is fixed, but `RefitGenerator.SanitizeGeneratedContracts()` still injects `System.Text.Json.Serialization.JsonStringEnumConverter` unconditionally and `CSharpClientGeneratorFactory` still hard-codes `JsonLibrary = SystemTextJson`. -- **#1053 remains PARTIAL and introduces a new regression**: `IdentifierUtils.Sanitize()` now escapes reserved keywords too early. Prefixed emit sites still compose invalid identifiers like `I@class` when `UseOpenApiTitle=true` and the title is `class`. -- **#1044 / #1050 are only CLI-side fixes**: `SettingsValidator` now reports them better, but the core library still silently prefers `OpenApiPaths` over `OpenApiPath` and `Serializer.Deserialize()` still throws the raw `JsonException` for non-CLI callers/source generator flows. -- **#1040 is only PARTIAL**: timeout and `User-Agent` were added, but `OpenApiDocumentFactory` still has no cancellation-token plumbing. - -**Verified Full Closures In Core Lane**: -- #1015, #1016, #1019, #1020, #1027, #1035, #1036, #1037, #1038, #1046, #1049, #1051, #1052, #1054, #1055. - -**Validation Run**: -- `dotnet build src\Refitter.Tests\Refitter.Tests.csproj -c Release --no-restore` -- `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --framework net10.0 --no-build --no-restore` -- `dotnet build src\Refitter.slnx -c Release --no-restore` -- `dotnet format --verify-no-changes src\Refitter.slnx --no-restore` - -**Useful Repros**: -- Multipart collision repro still generates: `Task UploadFile([AliasAs("Class")] string @class, [AliasAs("class")] string @class, [AliasAs("user-name")] string user_name, [AliasAs("user name")] string user_name);` -- OpenAPI title keyword repro still generates: `public partial interface I@class` - -### 2026-04-17: Release Compatibility Audit (1.7.3 to HEAD) - -**Task**: Audit generator-core changes between 1.7.3 and HEAD for breaking behavior ahead of major release. - -**Key Finding - CONFIRMED BREAKING CHANGE**: -- `GenerateAuthenticationHeader` property changed from `bool` to `AuthenticationHeaderStyle` enum (commit 7dbf6c0c, 14101a49). -- **Impact**: Source compatibility break. Existing .refitter files using `"generateAuthenticationHeader": true` will fail JSON deserialization. -- **Runtime impact**: `AuthenticationHeaderStyle` defaults to `None` (0), whereas old `bool` defaulted to `false`. Users who explicitly set `true` will see authentication headers **disappear** after upgrade unless they migrate to `"authenticationHeaderStyle": "Parameter"`. -- **Confidence**: 100% - This is a hard breaking change affecting settings deserialization. - -**Non-Breaking Changes (Bug Fixes, Safe Additions)**: -1. **JsonConverter attribute placement** (1b9a76c8): Moved from enum properties to enum types. Fixes user ability to override converters. Not breaking - only affects previously broken scenarios (hyphened enum values). -2. **Stack overflow fix** (3d9cdb6c): Schema traversal cycle detection. Only affects previously crashing inputs (recursive schemas). -3. **PropertyNamingPolicy** (76230c9e): New optional setting, defaults to `PascalCase` (existing behavior). No break. -4. **Auto-enable GenerateOptionalPropertiesAsNullable** (29de01ed): Only activates when `GenerateNullableReferenceTypes` is already true. Behavioral change but makes nullable reference types actually work correctly - likely expected behavior. -5. **Digit-prefixed property names** (fdcf675b): Now prefixed with underscore for C# compilation. Fixes previously invalid generated code. -6. **Multipart form-data extraction** (57498ea5): Fixes missing parameters. Only affects previously incomplete code generation. -7. **Method naming in ByTag mode** (79939948): Scopes per-interface instead of global. Prevents unexpected numeric suffixes - improves output quality. -8. **OneOf with discriminator transformation** (CSharpClientGeneratorFactory): Converts to allOf pattern for NSwag. Fixes undefined anonymous types. -9. **ContractTypeSuffix, SecurityScheme, OpenApiPaths, GenerateJsonSerializerContext**: New optional features with safe defaults. - -**Audit Result**: BREAKING CHANGE confirmed. Team consensus: major version 2.0.0 required. - -**Code Patterns Learned**: -- Schema traversal must use instance-based visited tracking with `HashSet` and `ActualSchema` resolution to prevent stack overflow. -- Settings format changes (type changes, not just new properties) constitute hard breaks for .refitter file users. -- NSwag's oneOf/anyOf handling requires preprocessing transformation to allOf for proper inheritance generation. - -**File Locations**: -- Core settings: `src/Refitter.Core/Settings/RefitGeneratorSettings.cs` -- Schema preprocessing: `src/Refitter.Core/CSharpClientGeneratorFactory.cs` -- Generator logic: `src/Refitter.Core/RefitGenerator.cs` -- Parameter extraction: `src/Refitter.Core/ParameterExtractor.cs` - -### 2026-04-18: P0 Audit Issue Verification - -**Task**: Verify 6 critical (P0) issues from v2.0 audit by examining current codebase. - -**Verified Issues**: - -1. **#1011 - SourceGenerator hint-name collisions** (VALID) - - Location: `src/Refitter.SourceGenerator/RefitterSourceGenerator.cs:155-160` - - Bug: Uses `Path.GetFileNameWithoutExtension(file.Path)` only for hint name, causing collisions when multiple .refitter files share filename in different directories - - Impact: `ArgumentException: hintName 'X.g.cs' was already added` crashes analyzer - - Notes: `filename` variable from `OutputFilename` (line 148) is computed but never used in hint name - -2. **#1012 - MSBuild task swallows CLI failures** (VALID) - - Location: `src/Refitter.MSBuild/RefitterGenerateTask.cs:22-52, 105-129` - - Bug: `Execute()` unconditionally returns `true` (line 51), never checks `process.ExitCode` (line 125) - - Impact: Build succeeds with stale/missing output when CLI fails; no errors logged to MSBuild - - Notes: `TryExecuteRefitter` returns null on exception (line 63) but caller ignores it - -3. **#1013 - ContractTypeSuffixApplier regex corruption** (VALID) - - Location: `src/Refitter.Core/ContractTypeSuffixApplier.cs:36-56` - - Bug: Line 50-55 uses raw `\b{typeName}\b` word-boundary regex on entire generated source - - Impact: Renames type references in comments, strings, member names; no collision detection; no double-suffix protection - - Notes: Does use `OrderByDescending(t => t.Length)` to reduce partial match risk, but still unsafe - -4. **#1014 - Forced JsonStringEnumConverter injection** (PARTIAL) - - Location: `src/Refitter.Core/RefitGenerator.cs:12-20, 264-283` - - Bug CONFIRMED: Enum regex `^(\s*)(public\s+(?:partial\s+)?enum\s+\w+\b)` (line 18) only matches `public` enums, not `internal` → internal enums silently lose converter attributes - - Bug CONFIRMED: Hard-coded STJ converter injection (line 275) ignores `JsonLibrary` setting - - Bug NOT FOUND: Generic form stripping claim appears outdated - regex includes `(?:<[\w.]+>)?` (line 13) - - Impact: Newtonsoft users get STJ references; internal enums serialize as integers - - Note: `CSharpClientGeneratorFactory.cs:38` forces `JsonLibrary = CSharpJsonLibrary.SystemTextJson` (hard-coded, user setting ignored) - -5. **#1015 - ConvertOneOfWithDiscriminatorToAllOf NRE** (VALID) - - Location: `src/Refitter.Core/CSharpClientGeneratorFactory.cs:97-131` - - Bug: Line 99 `foreach (var kvp in document.Components.Schemas)` has no null check - - Impact: NRE on every Swagger 2.0 doc (uses `definitions`, not `components`) and OpenAPI 3.0 docs without components - - Notes: Sibling methods like `EnumerateDocumentSchemaRoots()` (line 200) properly check `document.Components?.Schemas != null` - -6. **#1016 - Multi-spec merge drops schemas** (VALID) - - Location: `src/Refitter.Core/OpenApiDocumentFactory.cs:64-72` - - Bug: Line 68 checks `baseDocument.Components?.Schemas != null` before adding schemas from later docs - - Impact: If first spec lacks components/schemas, all schemas from subsequent specs are silently dropped - - Notes: No lazy initialization of `baseDocument.Components` or `.Schemas` dictionary - -**Code Patterns Learned**: -- Source generator hint names must disambiguate by path (not just filename) to avoid collisions in multi-project solutions -- MSBuild tasks MUST check `process.ExitCode` and return `false` on failure, otherwise CI/CD silently ships broken builds -- Regex replacement on raw source text is extremely fragile - should operate on NSwag model or Roslyn syntax tree instead -- Enum accessibility patterns must include `(public|internal)` to match NSwag's actual output -- OpenAPI document traversal must always null-check `document.Components` and `document.Components.Schemas` - Swagger 2 uses `definitions` instead -- Multi-document merge must lazily initialize collections on base document to handle "split" API definitions (paths-only + schemas-only) - -**File Locations**: -- Source generator: `src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` -- MSBuild task: `src/Refitter.MSBuild/RefitterGenerateTask.cs` -- Type suffix applier: `src/Refitter.Core/ContractTypeSuffixApplier.cs` -- Contract sanitization: `src/Refitter.Core/RefitGenerator.cs` -- Schema preprocessing: `src/Refitter.Core/CSharpClientGeneratorFactory.cs` -- Document factory: `src/Refitter.Core/OpenApiDocumentFactory.cs` - -## 2026-04-19: Runtime and Compatibility Workstream - -**Task**: Implement end-to-end fixes for runtime compatibility issues (#1025, #1026, #1027, #1040, #1042, #1049, #1052, #1055). - -**Completed**: - -1. **#1027 - Null Response Content Handling**: - - Added null check for `response.Content` in `RefitInterfaceGenerator.cs` (line 262-263) - - Prevents NRE when processing OpenAPI responses without content (e.g., 204 No Content, default responses) - - Impact: Fixes crashes on specs with content-less responses - -2. **#1040 - Static HttpClient Configuration**: - - Added explicit 30-second timeout to static `HttpClient` in `OpenApiDocumentFactory.cs` - - Added `User-Agent` header with assembly version via static constructor - - Impact: Better timeout control and server-side logging for OpenAPI document downloads - -3. **#1049 - ConfigureAwait(false) in Library Code**: - - Added `.ConfigureAwait(false)` to all await calls in `OpenApiDocumentFactory.cs` (9 locations) - - Added `.ConfigureAwait(false)` to all await calls in `RefitGenerator.cs` (3 locations) - - Impact: Prevents sync-over-async deadlocks in WPF/WinForms hosts calling library code - -4. **#1052 - Duplicate Operation ID Detection Efficiency**: - - Replaced `List` + `Distinct()` + `Count()` with `HashSet` in `OperationNameGenerator.cs` - - Short-circuits on first duplicate found instead of processing all operations twice - - Impact: Halves cost of duplicate detection on large specs; eliminates double allocation - -5. **#1055 - Interface Generator Construction Ordering**: - - Extracted `CreateInterfaceGenerator()` helper method in `RefitGenerator.cs` - - Encapsulates "create generator before GenerateFile()" pattern in single location - - Impact: Prevents future bugs from reintroducing incorrect ordering in new entry points - -6. **#1026 - Auto-Enable GenerateOptionalPropertiesAsNullable Documentation**: - - Added comprehensive comment in `CSharpClientGeneratorFactory.cs` explaining behavioral change - - Documents v1.x -> v2.0 migration path for users who need old behavior - - Impact: Clarifies intent and provides explicit override path - -**Test Coverage**: -Created comprehensive regression tests in `RuntimeCompatibilityTests.cs` covering: -- Response with no content (null Content handling) -- Accept header generation for valid responses -- Auto-enabling of optional properties as nullable when NRT enabled -- Duplicate operation ID detection (efficiency verification) -- Interface generator creation ordering (no numeric suffixes) -- Async operations with ConfigureAwait in library code - -**Code Patterns Learned**: -- Always use `ConfigureAwait(false)` in library code (netstandard2.0 targets) to prevent deadlocks -- Static HttpClient should have explicit timeout and User-Agent for observability -- Short-circuit on first match when detecting duplicates to minimize cost -- Encapsulate fragile ordering dependencies in dedicated helper methods -- Use HashSet membership tests (`Add()` returns false on duplicate) for efficient duplicate detection -- Document behavioral changes that affect v1.x users with migration guidance - -**File Locations**: -- `src/Refitter.Core/RefitInterfaceGenerator.cs` (null content handling) -- `src/Refitter.Core/OpenApiDocumentFactory.cs` (HttpClient config, ConfigureAwait) -- `src/Refitter.Core/RefitGenerator.cs` (ConfigureAwait, CreateInterfaceGenerator helper) -- `src/Refitter.Core/OperationNameGenerator.cs` (efficient duplicate detection) -- `src/Refitter.Core/CSharpClientGeneratorFactory.cs` (optional properties documentation) -- `src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs` (regression tests) - -**Build Status**: ✅ Solution builds successfully in Release mode with no errors (only pre-existing NuGet packaging warnings). - -**Known Issues Not Addressed**: -- **#1025** (Microsoft.OpenApi.Readers 1.x → 3.x): Requires smoke-test suite over corpus of real-world specs; marked for documentation/release notes -- **#1042** (Spectre.Console.Cli version bump): Requires manual CLI smoke testing; marked for validation before release - -## 2026-04-19: Identifier and Signature Correctness Workstream - -**Task**: Implement end-to-end fixes for identifier and signature correctness issues (#1018, #1019, #1020, #1036, #1037, #1038, #1053, #1056). - -**Completed**: - -1. **#1053 - Reserved Keyword Escaping**: - - Added missing reserved keywords (__arglist, __makeref, __reftype, __refvalue) - - Modified ` Sanitize()` to call `EscapeReservedKeyword()` for comprehensive keyword protection - -2. **#1018 & #1019 - Invalid Identifiers in ParameterExtractor**: - - Replaced `ReplaceUnsafeCharacters()` with `IdentifierUtils.ToCompilableIdentifier()` (line 154-169 → 154-157) - - Replaced `ConvertToVariableName()` to use `ToCompilableIdentifier()` (line 583-602) - - Now properly handles leading digits, reserved keywords, and special characters in both security headers and multipart form data - -3. **#1020 - Dynamic Querystring Self-Assignment**: - - Modified dynamic querystring constructor generation to use `this.` prefix (line 442-444) - - Prevents self-assignment when parameter name equals property name (e.g., `_foo = _foo`) - -4. **#1036 - Nullable Parameter Reordering**: - - Fixed `ReOrderNullableParameters()` to use regex pattern matching `\?\s+\w+(\s*=\s*[^,]+)?$` (line 172-192) - - Prevents mis-classification of generic parameters containing `?` as nullable (e.g., `IDictionary`) - -5. **#1037 - Empty Namespace List Crash**: - - Fixed `RefitInterfaceImports.GenerateNamespaceImports()` to handle empty namespace arrays (line 59-62) - - Replaced `Aggregate()` with `string.Join()` to avoid `InvalidOperationException` when all namespaces excluded - -6. **#1038 - Reference Type Nullability**: - - Enhanced `CustomCSharpTypeResolver` to check `GenerateNullableReferenceTypes` setting - - Added `IsValueType()` helper to distinguish value types from reference types - - Prevents CS8632 errors when mapping reference types like `System.Uri` without NRT enabled - - Value types always support nullable (`?`), reference types only when NRT enabled - -**Test Coverage**: -Created comprehensive regression tests in `IdentifierCorrectnessTests.cs` covering: -- Multipart form data with invalid identifiers (leading digits, keywords, special chars) -- Security scheme headers with invalid identifiers -- Dynamic querystring self-assignment scenarios -- Generic parameter reordering -- Empty namespace list handling -- Reserved keyword escaping - -**Code Patterns Learned**: -- Always route identifier generation through `IdentifierUtils.ToCompilableIdentifier()` for consistency -- Use `this.` prefix in constructor assignments when property and parameter names can match -- Use regex patterns to match nullable markers at end of type declarations, not substring matching -- Use `string.Join()` instead of `Aggregate()` for operations that may have empty collections -- Consult NRT settings before appending `?` to custom type mappings -- Distinguish value types from reference types when applying nullability - -**File Locations**: -- `src/Refitter.Core/IdentifierUtils.cs` (keyword list + Sanitize method) -- `src/Refitter.Core/ParameterExtractor.cs` (parameter extraction + sanitization + reordering) -- `src/Refitter.Core/RefitInterfaceImports.cs` (namespace import generation) -- `src/Refitter.Core/CustomCSharpTypeResolver.cs` (custom format mapping nullability) -- `src/Refitter.Tests/Examples/IdentifierCorrectnessTests.cs` (regression tests) - -**Build Status**: ✅ Core and CLI projects build successfully with only pre-existing warnings. - -## 2026-04-20: PR #1064 Remaining Merge Blockers - -**Task**: Fix the final three merge blockers from PR #1064: #1013 (suffix collision), #1018 (multipart dedup), #1053 (keyword escaping). - -**Completed Fixes**: - -1. **#1013 - ContractTypeSuffixApplier Collision Detection**: - - Added pre-flight collision check before building type rename map - - Collects all existing type names into a HashSet - - Skips renaming if `typeName + suffix` already exists in the codebase - - Prevents `Pet` → `PetDto` when `PetDto` already exists (would cause duplicate type names) - - Location: `src/Refitter.Core/ContractTypeSuffixApplier.cs` lines 18-47 - -2. **#1053 - Reserved Keyword Escaping Order**: - - Fixed interface name generation to sanitize AFTER prefixing "I" - - OLD: `title.Sanitize()` → `"@class"` → prepend "I" → `"I@class"` (invalid!) - - NEW: prepend "I" to title → `"I@class-Service"` → `Sanitize()` → `"IClassService"` (valid!) - - Sanitize strips illegal chars (`@`, `-`, etc.) and escapes keywords in final step - - Location: `src/Refitter.Core/RefitInterfaceGenerator.cs` line 370 - -**Partial Fix**: - -3. **#1018 - Multipart Parameter Deduplication**: - - Changed deduplication to use sanitized C# identifier instead of original OpenAPI key - - Initialize `seenFormParameterNames` with `GetVariableName(p)` instead of `p.Name` - - Check `seenFormParameterNames.Add(variableName)` instead of `property.Key` - - Logic verified correct: HashSet.Add returns true on first occurrence, false on duplicates - - **Issue**: Generated output still shows 3 duplicate parameters despite correct logic - - **Status**: Requires debugger-attached investigation to trace runtime behavior - - Location: `src/Refitter.Core/ParameterExtractor.cs` lines 97-100, 129-135 - -**Investigation Notes - #1018**: -- Manual trace-through confirms logic is sound: "a-b", "a b", "a.b" all produce "a_b" -- Clean rebuild verified - DLL hash matches between source and output directories -- Code path confirmed - only one location generates multipart parameters -- Test spec manually verified - produces 3 duplicate parameters with current code -- Possible causes: NSwag model quirk, multiple generation passes, or unknown secondary code path - -**Test File Fix**: -- Fixed `PR1064BlockerRegressions.cs` line 174: `BeLessOrEqualTo` → `BeLessThanOrEqualTo` (FluentAssertions API correction) - -**Code Patterns Learned**: -- ContractTypeSuffix must check for collision with target names, not just double-suffixing -- Identifier escaping must happen as the FINAL step after all string composition -- HashSet.Add returns true when item added (first occurrence), false when already present -- Multipart/form-data parameter extraction is manual because NSwag doesn't populate operationModel.Parameters for OpenAPI 3.x requestBody schemas - -**File Locations**: -- `src/Refitter.Core/ContractTypeSuffixApplier.cs` (collision detection) -- `src/Refitter.Core/ParameterExtractor.cs` (multipart deduplication - partial) -- `src/Refitter.Core/RefitInterfaceGenerator.cs` (keyword escaping order) -- `src/Refitter.Tests/Examples/PR1064BlockerRegressions.cs` (test fix) - -**Build Status**: ✅ Solution builds successfully with no errors (only pre-existing warnings) -**Test Status**: ⚠️ #1018 tests still failing - requires debugging investigation - -**Recommendation**: Merge #1013 and #1053 immediately. Hold #1018 for debugger investigation or create follow-up issue. - - -### 2026-04-25: Audit Matrix Narrowing - -- Ripley's remaining #1057 matrix pass reported #1045 as already fixed at HEAD and #1056 as doc/invariant-only. -- Remaining core/code-backed follow-up should stay focused on the still-open implementation items rather than reopening already-fixed or validation-only findings. - -### 2026-04-25: Lambert Repro Narrowing - -- Lambert's follow-up pass leaves **#1028** and **#1033** as current-HEAD core repros by inspection. -- **#1034, #1039, and #1056** were not reproduced on current HEAD in the tester pass. -- Multi-spec merge policy is now explicitly recorded: clone the first document, fail fast on path/schema collisions, and keep exact duplicate-path deduplication. - -### 2026-04-25: Core Audit Fixes + Verification - -- Landed **#1033** at HEAD by updating enum-converter injection in `src/Refitter.Core/RefitGenerator.cs` and locking it with regression coverage in `src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs`. -- Current review-gate stance: keep **#1032** validation-first; treat **#1034**, **#1039**, and **#1045** as fixed-at-HEAD / no-repro on the reviewed branch state; keep **#1056** as doc/invariant-only unless fresh failing evidence appears. -- This narrows Parker's remaining core follow-up to genuinely open code-backed issues instead of reopening the already-cleared matrix items. - -### 2026-04-25: Core Closure Set Rejected - -- Ash rejected the current no-code closure set for the #1057 core artifact. -- **#1034** and **#1039** remain open and still need real fixes. -- Parker is locked out of the next revision cycle for this artifact. -- Lambert will add blocker tests; Dallas is queued for the next implementation pass. - -### 2026-04-25: Core Revision Reassigned - -- Dallas completed the tooling lane first, including real fixes for **#1028**, **#1029**, **#1041**, and **#1043** plus validation-only closure on **#1042**/**#1047**. -- With Parker locked out, Dallas is now the active follow-up owner for the rejected **#1034**/**#1039** core revision. - -### 2026-04-25: Core Artifact Lockout Still In Force - -- Parker remains locked out of the next revision cycle for the #1057 core artifact after the earlier rejected closure pass. -- Ash has now also rejected Dallas's follow-up at the final gate, so ownership of the next/final #1034 revision has moved to Lambert. - -### 2026-04-25: Final Lockout Handoff Recorded +# Parker History -- Lambert completed the post-lockout final blocker pass for **#1034**. -- The reconciled evidence now covers duplicate schema, definition, and security-scheme collisions, and **#1039** is treated as a brittle test assertion rather than a reopened production defect. -- Validation was reported green for build, format, and tests. -- Ash is now the active final reviewer gate. - +## Context -### 2026-04-25: Core Artifact Lockout Still In Force +- User: Christian Helle +- Product: Refitter generates C# REST API clients from OpenAPI specifications using Refit. +- Stack: .NET, Refit, NSwag, Source Generator, MSBuild, Microsoft OpenAPI.NET -- Ash rejected Lambert's follow-up #1034 proof pass on test-isolation grounds. -- Parker remains locked out of the next revision cycle for this artifact, now alongside Dallas and Lambert. -- Ripley inherits the next narrow #1034 revision cycle. +## Learnings + +- Team initialized on 2026-04-16. +- 2026-04-20: JsonSerializerContextGenerator must use Roslyn syntax analysis instead of regex, emit attributes inside the contracts namespace, register nested types plus closed generic usages, and strip a conventional leading I from serializer-context names. +- 2026-04-20: GenerateJsonSerializerContext is wired through both RefitGenerator.Generate() and GenerateMultipleFiles(); multi-file generation emits a dedicated {ContextName}.cs file alongside contracts. +- 2026-04-20: GenerateNullableReferenceTypes must not silently flip GenerateOptionalPropertiesAsNullable; optional-property nullability stays an explicit user choice in CodeGeneratorSettings. + +## Core Context + +- **Breaking-change audit:** the durable v2 break was the settings-model change from generateAuthenticationHeader: bool to authenticationHeaderStyle: AuthenticationHeaderStyle; treat that as a hard compatibility break that justified the 2.0 line. +- **P0/P1 audit patterns:** source-generator hint names must disambiguate by path, MSBuild tasks must fail the build on CLI/process errors, regex replacement on raw generated source is fragile, Swagger/OpenAPI traversals must null-check Components/Schemas, and multi-spec merge logic must not silently drop later document state. +- **Runtime / compatibility patterns:** library awaits should use ConfigureAwait(false), static HttpClient setup needs explicit timeout plus User-Agent, null response content must be handled, duplicate operation IDs should short-circuit through HashSet.Add, and fragile generator ordering belongs in a helper rather than being duplicated across entry points. +- **Identifier / signature patterns:** route emitted identifiers through IdentifierUtils.ToCompilableIdentifier(), sanitize after final string composition, use this. for dynamic-query constructor assignments, detect nullable parameters at the tail of the type declaration, prefer string.Join() for potentially empty namespace lists, and only append ? to custom reference types when NRT is enabled. +- **PR #1064 lessons:** suffix collision checks must consider the final target name, multipart/query dedup should happen on the emitted C# identifier, reserved-keyword escaping is a final-step concern, and source-generator/package review needs the packed .nuspec plus analyzer payload instead of only the project file. +- **#1057 core-lane state:** keep #1032 validation-first, #1056 doc/invariant-only, #1045 fixed at HEAD, and #1033 closed via the newline-safe enum-converter hardening. Only treat #1034 / #1039 as closed when merge handling is clone-first and fail-fast on conflicting duplicate path/schema/definition/security keys while dynamic-query extraction stays non-mutating for downstream XML-doc generation. +- **Lockout chain:** Parker's initial no-code closure set for the late #1057 core artifact was rejected, which moved the follow-up through Dallas, Lambert, and Ripley before Ash gave final approval on the isolated #1034 / #1039 proof. + +## 2026-04-25: PR #1070 Sonar source-generator revision + +**Task:** Rework the RefitterSourceGenerator.cs Sonar fix so the source-generator lane keeps the GeneratedDiagnostic record-struct shape while satisfying the quality gate and Ash's review constraint. +**Status:** COMPLETE — source-generator-only revision landed and reviewer-approved. + +**Implementation Summary:** +- Kept the S1192 cleanup in src\Refitter.SourceGenerator\RefitterSourceGenerator.cs by reusing the shared Refitter diagnostic-title constant and assigning distinct diagnostic IDs for the found-file and file-contents info diagnostics. +- Restored GeneratedDiagnostic to a readonly record struct, preserved the explicit ordinal GetHashCode() implementation, and suppressed Sonar S1206 on the type because record structs already synthesize the paired Equals overloads. +- Left Dallas's src\Refitter.Core\ParameterExtractor.cs and src\Refitter.MSBuild\RefitterGenerateTask.cs changes untouched. + +**Validation Notes:** +- dotnet build -c Release src\Refitter.slnx --no-restore +- dotnet test -c Release --solution src\Refitter.slnx --no-build +- dotnet format --verify-no-changes src\Refitter.slnx --no-restore + +## 2026-04-25: Scribe consolidation of PR #1070 + +- Dallas's ParameterExtractor / RefitterGenerateTask cleanups remain the approved behavior-preserving response for S1066, S3267, and S3358. +- Ash explicitly rejected only the first manual-struct S1206 direction; Parker's revision replaced that one artifact and became the final approved source-generator state. +- The merged squad decision now records the stable diagnostic-ID contract, the preserved readonly record struct shape, and the shared build/test/format validation for PR #1070. diff --git a/.squad/decisions.md b/.squad/decisions.md index 6cd4bb2de..7e96eae3d 100644 --- a/.squad/decisions.md +++ b/.squad/decisions.md @@ -213,5 +213,13 @@ - Dallas landed the test-only closure in `src\Refitter.Tests\RefitterGenerateTaskTests.cs`, covering blank package folders, whitespace runtime entries, co-located and first-bundled fallback resolution, missing bundled CLI failure, process-runner exception handling, millisecond timeout formatting, and successful `LogErrorFromException` forwarding. - Reported validation: `dotnet test --project src\Refitter.Tests\Refitter.Tests.csproj -c Release --coverage --coverage-output coverage.cobertura.xml --coverage-output-format xml`, `dotnet build -c Release src\Refitter.slnx --no-restore`, and `dotnet format --verify-no-changes src\Refitter.slnx --no-restore`. - Result: `RefitterGenerateTask.cs` reached 100% line coverage, 100% block coverage, and 0 partial functions in the reported coverage output. +### PR #1070 SonarCloud quality-gate repair +**Verified By:** Dallas / Ash / Parker +**Status:** APPROVED + +- Keep the ParameterExtractor (S1066) and RefitterGenerateTask (S3267, S3358) changes narrow and behavior-preserving; Ash approved those cleanups as safe. +- Keep the source-generator diagnostic cleanup on the stable one-descriptor-per-ID contract: reuse the shared Refitter title/category constant, but assign distinct IDs when the title/message semantics differ. +- Preserve GeneratedDiagnostic as a readonly record struct; retain the explicit ordinal GetHashCode() behavior and handle Sonar S1206 with a targeted suppression plus justification instead of rewriting the type into a manual struct. +- Final validation recorded for the landed artifact: dotnet build -c Release src\Refitter.slnx --no-restore, dotnet test -c Release --solution src\Refitter.slnx --no-build, and dotnet format --verify-no-changes src\Refitter.slnx --no-restore.