Resolve high-severity audit findings from #1057#1067
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces regex-based serializer-context extraction with Roslyn parsing; emits JSON serializer contexts (single/multi-file) and preserves namespace/closed-generic/nested-type correctness; switches MSBuild integration to parse CLI-emitted Changes
Sequence Diagram(s)sequenceDiagram
actor Dev
participant CLI as GenerateCommand (CLI)
participant Proc as Refitter Process
participant MSBuild as RefitterGenerateTask (MSBuild)
participant FS as File System
Dev->>CLI: run refitter (--simple-output)
CLI->>Proc: spawn generator
Proc->>FS: write generated file(s) to disk (when CLI run)
Proc->>CLI: stdout "GeneratedFile: /abs/path/RefitInterfaces.cs"
Proc->>CLI: stdout "GeneratedFile: /abs/path/Contracts.cs"
Proc->>CLI: stdout "GeneratedFile: /abs/path/JsonSerializerContext.cs"
Proc->>CLI: exit 0
MSBuild->>CLI: capture process stdout
MSBuild->>MSBuild: parse lines for GeneratedFile marker
MSBuild->>FS: verify reported file paths exist
MSBuild->>MSBuild: deduplicate (case-insensitive)
alt markers found
MSBuild->>MSBuild: add files to <Compile> items
MSBuild->>Dev: task succeeds
else no markers
MSBuild->>Dev: task fails (error: no generated files reported)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/docfx_project/articles/source-generator.md (1)
5-18:⚠️ Potential issue | 🟡 MinorUpdate the stale disk-output description on this page.
The new installation note is for the updated source-generator packaging, but Line 5 still says the source generator writes a
Generatedfolder and that users should commit generated files. That conflicts with the v2.0 behavior documented inbreaking-changes-v2-0-0.mdwhere source generation uses Roslyn in-memory output.📝 Proposed documentation update
-The Refitter source generator is a bit untraditional in a sense that it creates a folder called `Generated` in the same location as the `.refitter` file and generates files to disk under the `Generated` folder (can be changed with `--outputFolder`). The source generator output should be included in the project and committed to source control. This is done because there is no other way to trigger the Refit source generator to pickup the Refitter generated code +The Refitter source generator emits generated code into the compilation through Roslyn's source-generator APIs. Generated code is compiled into your project but is not written to disk. If you need persistent generated files for review or source control, use the Refitter CLI or Refitter MSBuild package instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docfx_project/articles/source-generator.md` around lines 5 - 18, Update the stale documentation paragraph that claims the source generator writes a Generated folder next to a .refitter file and that generated files must be committed; instead remove or rewrite references to "Generated", ".refitter" and "--outputFolder" to reflect v2.0 behavior (in-memory Roslyn output) and mention that generated artifacts are produced in-memory and do not need to be committed, aligning the text with breaking-changes-v2-0-0.md and the new installation note about Refitter.SourceGenerator packaging and explicit Refit package references.src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs (1)
240-247:⚠️ Potential issue | 🟡 MinorCompile the generated nullability variants too.
These tests validate the string shape, but the changed Roslyn nullability rewrite can also break generated syntax. Add
BuildHelper.BuildCSharp(code).Should().BeTrue();to the no-auto-enable and explicit opt-in OpenAPI 3.0/Swagger 2.0 cases.As per coding guidelines,
src/Refitter.Tests/**/*.cs: “All new code must include unit tests following the pattern inRefitter.Tests.Examplesnamespace with test methods for code generation, pattern validation, and build compilation”.Also applies to: 313-320, 386-390, 458-462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs` around lines 240 - 247, The test currently generates code via RefitGenerator.CreateAsync(...) and asserts string shapes using sut.Generate(); add a compilation assertion to also ensure generated code compiles by calling BuildHelper.BuildCSharp(code).Should().BeTrue() immediately after the existing code assertions in the no-auto-enable and explicit opt-in OpenAPI 3.0/Swagger 2.0 test methods (the methods that call RefitGenerator.CreateAsync and then sut.Generate()); repeat the same addition for the other affected test methods referenced in the review (the similar blocks at the other two locations) so each generation test performs pattern validation and BuildHelper.BuildCSharp(code).Should().BeTrue().
🧹 Nitpick comments (2)
src/Refitter/GenerateCommand.cs (1)
817-818: Marker emission looks good; consider also emitting in non-SimpleOutput mode.
WriteGeneratedFileMarkeris only invoked whensettings.SimpleOutputis true (lines 435-438, 543-546, 578-581). This is fine for the current MSBuild integration (which always passes--simple-output), but note that any future consumer that invokes the CLI without--simple-outputwill silently get no manifest markers. TheGenerateFile:prefix is distinctive enough that emitting it unconditionally would be safe and would avoid that trap — the AnsiConsole panels/tables never start with that literal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter/GenerateCommand.cs` around lines 817 - 818, WriteGeneratedFileMarker currently only emits when settings.SimpleOutput is true, which silently omits manifest markers for non-simple runs; change the code so the marker is emitted unconditionally by moving or adding the call to WriteGeneratedFileMarker outside the settings.SimpleOutput guards (i.e., replace the guarded calls that call WriteGeneratedFileMarker in GenerateCommand with an unconditional invocation after file generation or remove the SimpleOutput check around those calls), keeping the existing Console.WriteLine behavior in WriteGeneratedFileMarker to ensure the distinct "GenerateFile:" prefix is always printed.src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs (1)
26-36: Minor:GetRepositoryRootcan NRE ifdirectorybecomes null before the assertion.If
AppContext.BaseDirectoryever runs outside the repo,directory.Parentwalks tonulland the loop exits;directory.Should().NotBeNull(...)would then fail cleanly, but consider whether this helper is duplicated elsewhere in the test project — if so, extract it into a shared test utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs` around lines 26 - 36, GetRepositoryRoot can end up with directory == null after walking up from AppContext.BaseDirectory which risks a null dereference; change the method to explicitly handle the null case instead of relying on the FluentAssertions call to prevent an NRE: after the loop check if directory is null and throw a clear InvalidOperationException (or use Assert/FluentAssertions with a descriptive message) before accessing directory.FullName, and if this helper exists elsewhere extract it into a shared test utility; reference: GetRepositoryRoot, DirectoryInfo, AppContext.BaseDirectory, README.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.squad/agents/dallas/history.md:
- Around line 270-275: Update the stale history entry: change the description
that says Refifit was set with PrivateAssets="compile" to reflect the final
shipped state PrivateAssets="all" for both OasReader and Refit; locate the
mention in the Dallas history entry and align its text with the actual project
change in Refitter.SourceGenerator.csproj where OasReader and Refit were made
private (PrivateAssets="all"), and mirror the wording used in the Ash history
note so the logs remain consistent.
In `@src/Refitter.Core/JsonSerializerContextGenerator.cs`:
- Around line 24-33: The code uses a null-coalescing fallback for
contextNamespace which doesn't handle empty or whitespace strings and can emit
an invalid "namespace " line; change the assignment of contextNamespace to treat
whitespace as unset (use string.IsNullOrWhiteSpace on
settings.ContractsNamespace and settings.Namespace) and then only emit the
namespace wrapper (the sb.AppendLine($"namespace {contextNamespace}") and the
opening/closing braces) when contextNamespace is non-empty/non-whitespace; keep
calls to ExtractTypeNames(contracts, contextNamespace) and
GetContextTypeName(settings) unchanged but guard the namespace emission around
contextNamespace so no empty namespace is emitted.
- Around line 82-84: The code registers non-generic declared types by checking
Arity == 0 (in the declaredTypes LINQ leading to GetDisplayName), but nested
types inside generic containers must be excluded because their total arity
includes ancestor type parameters; update DeclaredTypeInfo.Create() to record
each ancestor container's arity in a ContainerArities collection, then change
the filter used where declaredTypes are selected to compute totalArity =
DeclaredTypeInfo.Arity + DeclaredTypeInfo.ContainerArities.Sum() (or ensure any
container arity > 0 causes exclusion) and only select types with totalArity == 0
before calling GetDisplayName; also add a unit test for a nested type inside a
generic outer (e.g., class Outer<T>{ class Inner {} }) to assert it is not
registered.
In `@src/Refitter.Core/RefitGenerator.cs`:
- Around line 305-344: The rewriter may strip nullability from value types
because IsReferenceType currently uses only syntax kinds; update
Swagger2OptionalReferencePropertyNullabilityRewriter to use semantic analysis
rather than syntactic heuristics: modify
NormalizeSwagger2OptionalReferencePropertyNullability to obtain a SemanticModel
for the parsed tree and pass it into the rewriter, then change IsReferenceType
to inspect the symbol for the TypeSyntax (via
SemanticModel.GetTypeInfo/GetSymbolInfo) and return true only if the resolved
ITypeSymbol.IsReferenceType is true (and explicitly exclude
TypeKind.Struct/Enum), falling back to conservative false when the symbol cannot
be resolved; alternatively, add regression tests exercising
NormalizeSwagger2OptionalReferencePropertyNullability (Swagger2 +
GenerateNullableReferenceTypes=true +
GenerateOptionalPropertiesAsNullable=false) with optional enum/struct properties
to lock behavior.
In
`@src/Refitter.Tests/Examples/GenerateJsonSerializerContextPolymorphismTests.cs`:
- Around line 124-195: The RunGeneratedCode method currently reads
process.StandardOutput.ReadToEnd() then process.StandardError.ReadToEnd(), which
can deadlock; change RunGeneratedCode so it begins asynchronous reads of both
StandardOutput and StandardError (e.g., ReadToEndAsync on process.StandardOutput
and process.StandardError or using events) before calling process.WaitForExit,
and enforce a timeout (e.g., Task.WhenAny against Task.Delay) to kill/cleanup
the Process if the run exceeds the bound; ensure you still collect both outputs
and return the process.ExitCode (or a non-zero code on timeout) and combined
output.
In `@src/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cs`:
- Around line 10-56: The current tests only define OpenApiSpec (OpenAPI 3.0) in
GenerateJsonSerializerContextTests.cs and lack a Swagger 2.0 fixture; add a new
constant (e.g., Swagger2Spec or OpenApi2Spec) containing the equivalent Swagger
2.0 JSON (use "swagger": "2.0", move "components/schemas" to "definitions", and
adjust response/content to Swagger 2.0 shape), then update the test(s) that
reference OpenApiSpec to also exercise this new constant—either by
paramaterizing the test or duplicating the test logic to run once with
OpenApiSpec and once with Swagger2Spec—so AOT serializer-context generation is
validated for both OpenAPI 2.0 and 3.0. Ensure you reference the existing symbol
OpenApiSpec and the test class GenerateJsonSerializerContextTests when making
changes.
In `@src/Refitter.Tests/RefitGeneratorAdvancedTests.cs`:
- Around line 672-701: Add a companion unit test that mirrors
GenerateMultipleFiles_With_JsonSerializerContext_Adds_Context_File but uses a
Swagger 2.0 spec (definitions) instead of OpenAPI 3.0: create a new test method
(e.g.,
GenerateMultipleFiles_With_JsonSerializerContext_Adds_Context_File_Swagger2)
that uses SwaggerFileHelper.CreateSwaggerFile with a Swagger 2.0 example, set
RefitGeneratorSettings.GenerateMultipleFiles = true and
GenerateJsonSerializerContext = true, call
RefitGenerator.CreateAsync(...).GenerateMultipleFiles(), assert result.Files
contains the serializer context file (TypeName == "TestApiSerializerContext")
and additionally assert the context file Content includes the expected DTO type
names from the Swagger 2.0 definitions (not just build success), then verify
compilation via BuildHelper.BuildCSharp(combinedCode).Should().BeTrue(), and
remember to CleanupSwaggerFile(swaggerFile); follow the Refitter.Tests.Examples
test pattern for naming and assertions.
In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs`:
- Around line 22-23: The test asserts the wrong PrivateAssets for the "Refit"
package; update the assertion in SourceGeneratorPackageReferenceTests (where
packageReferences is used) to expect "all" instead of "compile", and add
explicit ContainsKey checks (e.g., packageReferences.ContainsKey("Refit") and
packageReferences.ContainsKey("OasReader")) before indexing to produce clearer
failures than KeyNotFoundException when either key is missing.
---
Outside diff comments:
In `@docs/docfx_project/articles/source-generator.md`:
- Around line 5-18: Update the stale documentation paragraph that claims the
source generator writes a Generated folder next to a .refitter file and that
generated files must be committed; instead remove or rewrite references to
"Generated", ".refitter" and "--outputFolder" to reflect v2.0 behavior
(in-memory Roslyn output) and mention that generated artifacts are produced
in-memory and do not need to be committed, aligning the text with
breaking-changes-v2-0-0.md and the new installation note about
Refitter.SourceGenerator packaging and explicit Refit package references.
In `@src/Refitter.Tests/Examples/RuntimeCompatibilityTests.cs`:
- Around line 240-247: The test currently generates code via
RefitGenerator.CreateAsync(...) and asserts string shapes using sut.Generate();
add a compilation assertion to also ensure generated code compiles by calling
BuildHelper.BuildCSharp(code).Should().BeTrue() immediately after the existing
code assertions in the no-auto-enable and explicit opt-in OpenAPI 3.0/Swagger
2.0 test methods (the methods that call RefitGenerator.CreateAsync and then
sut.Generate()); repeat the same addition for the other affected test methods
referenced in the review (the similar blocks at the other two locations) so each
generation test performs pattern validation and
BuildHelper.BuildCSharp(code).Should().BeTrue().
---
Nitpick comments:
In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs`:
- Around line 26-36: GetRepositoryRoot can end up with directory == null after
walking up from AppContext.BaseDirectory which risks a null dereference; change
the method to explicitly handle the null case instead of relying on the
FluentAssertions call to prevent an NRE: after the loop check if directory is
null and throw a clear InvalidOperationException (or use Assert/FluentAssertions
with a descriptive message) before accessing directory.FullName, and if this
helper exists elsewhere extract it into a shared test utility; reference:
GetRepositoryRoot, DirectoryInfo, AppContext.BaseDirectory, README.md.
In `@src/Refitter/GenerateCommand.cs`:
- Around line 817-818: WriteGeneratedFileMarker currently only emits when
settings.SimpleOutput is true, which silently omits manifest markers for
non-simple runs; change the code so the marker is emitted unconditionally by
moving or adding the call to WriteGeneratedFileMarker outside the
settings.SimpleOutput guards (i.e., replace the guarded calls that call
WriteGeneratedFileMarker in GenerateCommand with an unconditional invocation
after file generation or remove the SimpleOutput check around those calls),
keeping the existing Console.WriteLine behavior in WriteGeneratedFileMarker to
ensure the distinct "GenerateFile:" prefix is always printed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e947415c-9e78-4381-939e-b23758c8613a
📒 Files selected for processing (26)
.squad/agents/ash/history.md.squad/agents/dallas/history.md.squad/agents/parker/history.md.squad/skills/aot-json-serializer-context/SKILL.md.squad/skills/generated-file-manifest/SKILL.mdCHANGELOG.mdREADME.mddocs/docfx_project/articles/breaking-changes-v2-0-0.mddocs/docfx_project/articles/source-generator.mdsrc/Refitter.Core/CSharpClientGeneratorFactory.cssrc/Refitter.Core/JsonSerializerContextGenerator.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.Core/Settings/RefitGeneratorSettings.cssrc/Refitter.MSBuild/AssemblyInfo.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator/Refitter.SourceGenerator.csprojsrc/Refitter.Tests/Examples/GenerateJsonSerializerContextPolymorphismTests.cssrc/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cssrc/Refitter.Tests/Examples/RuntimeCompatibilityTests.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/JsonSerializerContextGeneratorTests.cssrc/Refitter.Tests/RefitGeneratorAdvancedTests.cssrc/Refitter.Tests/Refitter.Tests.csprojsrc/Refitter.Tests/RefitterGenerateTaskTests.cssrc/Refitter.Tests/SourceGeneratorPackageReferenceTests.cssrc/Refitter/GenerateCommand.cs
💤 Files with no reviewable changes (1)
- src/Refitter.Core/CSharpClientGeneratorFactory.cs
christianhelle
changed the title
Owner
Author
|
@copilot fix the build errors |
Contributor
Fixed in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/ConsoleApp/Directory.Build.props (1)
14-27:⚠️ Potential issue | 🔴 Critical
Compile Removeruns before theCompile Include, so it removes nothing.MSBuild evaluates
ItemGroups top-to-bottom, andRemoveonly affects items already in the item list. The Remove on line 20 fires before the Include on line 26 adds../../GeneratedCode/**/*.cs. SDK defaults for .NET projects only include.csfiles in the standard project directory—not external paths like../../GeneratedCode/. So when Remove executes duringSmokeTest=true, the Compile list contains only SDK defaults, the Remove pattern matches nothing, and the subsequent Include adds all GeneratedCode files anyway (includingJsonSerializerCtx*.cs), re-introducing the STJ hint-name collision this code is supposed to prevent.Either move the
Removebelow the broadInclude, or useExcludeon the sameInclude.🛠️ Proposed fix (Exclude on the Include)
- <!-- JsonSerializerContext files cannot be compiled together in batch because the STJ source - generator uses the simple class name (not fully-qualified) as the hint file prefix. - Two files with the same context class simple name but different namespaces cause a - duplicate-hint-name collision that breaks the STJ source generator. - These files ARE validated individually by unit tests (BuildHelper.BuildCSharp). --> - <ItemGroup Condition="'$(SmokeTest)' == 'true'"> - <Compile Remove="../../GeneratedCode/JsonSerializerCtx*.cs" /> - </ItemGroup> <ItemGroup> <Compile Include="..\Program.cs"> <Link>Program.cs</Link> </Compile> - <Compile Include="../../GeneratedCode/**/*.cs" /> + <Compile Include="../../GeneratedCode/**/*.cs" + Exclude="$(SmokeTest) == 'true' ? '../../GeneratedCode/JsonSerializerCtx*.cs' : ''" /> </ItemGroup> + <!-- JsonSerializerContext files cannot be compiled together in batch because the STJ source + generator uses the simple class name (not fully-qualified) as the hint file prefix. + Two files with the same context class simple name but different namespaces cause a + duplicate-hint-name collision that breaks the STJ source generator. + These files ARE validated individually by unit tests (BuildHelper.BuildCSharp). --> + <ItemGroup Condition="'$(SmokeTest)' == 'true'"> + <Compile Remove="../../GeneratedCode/JsonSerializerCtx*.cs" /> + </ItemGroup>(The simpler, safest variant is to keep the existing Remove block and relocate it after the Include.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/ConsoleApp/Directory.Build.props` around lines 14 - 27, The Remove for Compile (pattern JsonSerializerCtx*.cs) runs before the Include that brings in ../../GeneratedCode/**/*.cs so it has no effect; fix by either moving the <Compile Remove="../../GeneratedCode/JsonSerializerCtx*.cs" /> into a later ItemGroup after the <Compile Include="../../GeneratedCode/**/*.cs" /> or, more simply, change the <Compile Include="../../GeneratedCode/**/*.cs" /> to exclude the STJ contexts (e.g. add Exclude="../../GeneratedCode/JsonSerializerCtx*.cs") so the JsonSerializerCtx*.cs files are never added; keep the existing SmokeTest Condition on the Remove if you move it, or apply the Exclude on the same Include to ensure the STJ hint-name collision is prevented.
♻️ Duplicate comments (3)
src/Refitter.Core/JsonSerializerContextGenerator.cs (2)
25-47:⚠️ Potential issue | 🟠 MajorEmpty
ContractsNamespacestill emitsnamespacewrapper.
settings.ContractsNamespace ?? settings.Namespaceonly falls back onnull, soContractsNamespace = ""(or whitespace) producesnamespaceon line 33 and uncompilable output. UseIsNullOrWhiteSpaceand skip the namespace wrapper entirely when both are empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 25 - 47, The code builds a namespace wrapper using settings.ContractsNamespace ?? settings.Namespace which still emits "namespace " when ContractsNamespace is an empty string; change the namespace selection to use string.IsNullOrWhiteSpace (e.g. var contextNamespace = !string.IsNullOrWhiteSpace(settings.ContractsNamespace) ? settings.ContractsNamespace : settings.Namespace) and then, when string.IsNullOrWhiteSpace(contextNamespace) is true, do not emit the namespace wrapper or its braces and do not indent the JsonSerializable attributes and the internal partial class (references: settings.ContractsNamespace, contextNamespace, ExtractTypeNames, GetContextTypeName, contextName); otherwise emit the namespace and keep the existing indented lines.
95-97:⚠️ Potential issue | 🟠 MajorNested types inside generic containers are still registered as invalid
typeof(Outer.Inner).
Where(t => t.Arity == 0)filters only the leaf's own type-parameter count. Forclass Outer<T> { class Inner {} },Inner.Arity == 0so it gets registered asOuter.Inner, which is not a validtypeof(...)expression in C#.DeclaredTypeInfoneeds to carry container arities and the filter must check total arity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 95 - 97, The current filter uses DeclaredTypeInfo.Arity which only reflects the type's own generic parameter count and thus still includes nested types inside generic containers (e.g., Outer<T>.Inner) which cannot be emitted as typeof(Outer.Inner). Add a TotalArity (or ContainerArity + OwnArity) property to DeclaredTypeInfo that sums the arities of the type and all its containing types, then change the filter in JsonSerializerContextGenerator (the declaredTypes -> typeNames projection) to use that total (e.g., .Where(t => t.TotalArity == 0)) so only types with zero total arity are registered; update any places that construct DeclaredTypeInfo to populate the new field or compute it via a helper method that walks containing types.src/Refitter.Core/RefitGenerator.cs (1)
305-344:⚠️ Potential issue | 🟠 Major
IsReferenceTypestill uses syntactic heuristics and will strip nullability from value types.
IdentifierNameSyntax,GenericNameSyntax,QualifiedNameSyntaxandAliasQualifiedNameSyntaxare all returned as reference types, but they can equally resolve to enums (Status?) or structs (Money?,DateTime?under an alias). When this rewriter runs (Swagger2 + NRT=true +GenerateOptionalPropertiesAsNullable=false), it will silently changeStatus? Status { get; set; }toStatus Status { get; set; }, altering DTO shapes. Prefer semantic resolution viaSemanticModel.GetTypeInfo().Type.IsReferenceType, or at minimum add regression tests covering optional enum/struct properties in that exact configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/RefitGenerator.cs` around lines 305 - 344, The current rewriter in NormalizeSwagger2OptionalReferencePropertyNullability (class Swagger2OptionalReferencePropertyNullabilityRewriter) uses syntactic heuristics in IsReferenceType and can strip nullability from value types; change VisitPropertyDeclaration to use a SemanticModel to call GetTypeInfo on the property type (or the nullable element type) and check Type.IsReferenceType before removing NullableTypeSyntax, so enums/structs remain nullable; if obtaining a SemanticModel requires creating a Compilation, pass it into the rewriter (e.g., via constructor) and use it for the resolution, and add regression tests that run the Swagger2 + GenerateNullableReferenceTypes=true + GenerateOptionalPropertiesAsNullable=false scenario with optional enum/struct properties to ensure Status? and struct? are preserved.
🧹 Nitpick comments (2)
src/Refitter.MSBuild/RefitterGenerateTask.cs (1)
331-342:GetRelativePathassumes absolute inputs — consider hardening.
new Uri(path)throwsUriFormatExceptionon relative paths. In practiceProjectFileDirectoryis set by MSBuild to an absolute path andDirectory.GetFilesinherits that, so this is unlikely to bite today. But a defensivePath.GetFullPathon both inputs costs nothing and prevents a surprising failure mode if a consumer ever passes a relativeProjectFileDirectory.♻️ Proposed defensive change
private static string GetRelativePath(string relativeTo, string path) { - var relativeToUri = new Uri(AppendDirectorySeparator(relativeTo)); - var pathUri = new Uri(path); + var relativeToUri = new Uri(AppendDirectorySeparator(Path.GetFullPath(relativeTo))); + var pathUri = new Uri(Path.GetFullPath(path)); return Uri.UnescapeDataString(relativeToUri.MakeRelativeUri(pathUri).ToString()) .Replace('/', Path.DirectorySeparatorChar); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.MSBuild/RefitterGenerateTask.cs` around lines 331 - 342, GetRelativePath currently constructs Uris from the inputs and will throw UriFormatException for relative paths; make it defensive by calling Path.GetFullPath on both parameters before using AppendDirectorySeparator/creating new Uri instances (i.e., normalize both relativeTo and path with Path.GetFullPath at the start of GetRelativePath), ensuring AppendDirectorySeparator still works as expected and preserving the existing Uri.UnescapeDataString/Replace logic.src/Refitter.Core/JsonSerializerContextGenerator.cs (1)
120-137: DefensiveContains('<') && Contains('>')check silently drops registrations on mismatch.By this point
declaredType.Arity > 0and!ContainsOpenTypeParameter(...), soFormatTypeSyntaxshould always produce a<...>form. If it doesn't, something upstream is wrong and we'd benefit from failing loudly (or registering the result anyway) rather than dropping it. Consider removing the textual check or replacing with aDebug.Assert.♻️ Suggested simplification
- typeName = FormatTypeSyntax(typeSyntax, declaredTypes, contextNamespace); - return typeName.Contains('<') && typeName.Contains('>'); + typeName = FormatTypeSyntax(typeSyntax, declaredTypes, contextNamespace); + return !string.IsNullOrEmpty(typeName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 120 - 137, The defensive runtime check in TryFormatClosedGenericType that returns false unless typeName.Contains('<') && typeName.Contains('>') can silently drop valid registrations; after verifying TryResolveDeclaredType, declaredType.Arity>0 and !ContainsOpenTypeParameter, FormatTypeSyntax should produce a closed-generic form so remove the textual Contains('<')/Contains('>') gating (or replace it with a Debug.Assert for diagnostics) and always assign typeName = FormatTypeSyntax(...) and return true; reference TryFormatClosedGenericType, TryResolveDeclaredType, declaredType.Arity, ContainsOpenTypeParameter and FormatTypeSyntax to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/ConsoleApp/Directory.Build.props`:
- Around line 14-27: The Remove for Compile (pattern JsonSerializerCtx*.cs) runs
before the Include that brings in ../../GeneratedCode/**/*.cs so it has no
effect; fix by either moving the <Compile
Remove="../../GeneratedCode/JsonSerializerCtx*.cs" /> into a later ItemGroup
after the <Compile Include="../../GeneratedCode/**/*.cs" /> or, more simply,
change the <Compile Include="../../GeneratedCode/**/*.cs" /> to exclude the STJ
contexts (e.g. add Exclude="../../GeneratedCode/JsonSerializerCtx*.cs") so the
JsonSerializerCtx*.cs files are never added; keep the existing SmokeTest
Condition on the Remove if you move it, or apply the Exclude on the same Include
to ensure the STJ hint-name collision is prevented.
---
Duplicate comments:
In `@src/Refitter.Core/JsonSerializerContextGenerator.cs`:
- Around line 25-47: The code builds a namespace wrapper using
settings.ContractsNamespace ?? settings.Namespace which still emits "namespace "
when ContractsNamespace is an empty string; change the namespace selection to
use string.IsNullOrWhiteSpace (e.g. var contextNamespace =
!string.IsNullOrWhiteSpace(settings.ContractsNamespace) ?
settings.ContractsNamespace : settings.Namespace) and then, when
string.IsNullOrWhiteSpace(contextNamespace) is true, do not emit the namespace
wrapper or its braces and do not indent the JsonSerializable attributes and the
internal partial class (references: settings.ContractsNamespace,
contextNamespace, ExtractTypeNames, GetContextTypeName, contextName); otherwise
emit the namespace and keep the existing indented lines.
- Around line 95-97: The current filter uses DeclaredTypeInfo.Arity which only
reflects the type's own generic parameter count and thus still includes nested
types inside generic containers (e.g., Outer<T>.Inner) which cannot be emitted
as typeof(Outer.Inner). Add a TotalArity (or ContainerArity + OwnArity) property
to DeclaredTypeInfo that sums the arities of the type and all its containing
types, then change the filter in JsonSerializerContextGenerator (the
declaredTypes -> typeNames projection) to use that total (e.g., .Where(t =>
t.TotalArity == 0)) so only types with zero total arity are registered; update
any places that construct DeclaredTypeInfo to populate the new field or compute
it via a helper method that walks containing types.
In `@src/Refitter.Core/RefitGenerator.cs`:
- Around line 305-344: The current rewriter in
NormalizeSwagger2OptionalReferencePropertyNullability (class
Swagger2OptionalReferencePropertyNullabilityRewriter) uses syntactic heuristics
in IsReferenceType and can strip nullability from value types; change
VisitPropertyDeclaration to use a SemanticModel to call GetTypeInfo on the
property type (or the nullable element type) and check Type.IsReferenceType
before removing NullableTypeSyntax, so enums/structs remain nullable; if
obtaining a SemanticModel requires creating a Compilation, pass it into the
rewriter (e.g., via constructor) and use it for the resolution, and add
regression tests that run the Swagger2 + GenerateNullableReferenceTypes=true +
GenerateOptionalPropertiesAsNullable=false scenario with optional enum/struct
properties to ensure Status? and struct? are preserved.
---
Nitpick comments:
In `@src/Refitter.Core/JsonSerializerContextGenerator.cs`:
- Around line 120-137: The defensive runtime check in TryFormatClosedGenericType
that returns false unless typeName.Contains('<') && typeName.Contains('>') can
silently drop valid registrations; after verifying TryResolveDeclaredType,
declaredType.Arity>0 and !ContainsOpenTypeParameter, FormatTypeSyntax should
produce a closed-generic form so remove the textual Contains('<')/Contains('>')
gating (or replace it with a Debug.Assert for diagnostics) and always assign
typeName = FormatTypeSyntax(...) and return true; reference
TryFormatClosedGenericType, TryResolveDeclaredType, declaredType.Arity,
ContainsOpenTypeParameter and FormatTypeSyntax to locate the change.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 331-342: GetRelativePath currently constructs Uris from the inputs
and will throw UriFormatException for relative paths; make it defensive by
calling Path.GetFullPath on both parameters before using
AppendDirectorySeparator/creating new Uri instances (i.e., normalize both
relativeTo and path with Path.GetFullPath at the start of GetRelativePath),
ensuring AppendDirectorySeparator still works as expected and preserving the
existing Uri.UnescapeDataString/Replace logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3868a4f7-dcef-4e28-bacf-a2d3b8e32e3b
📒 Files selected for processing (10)
.squad/agents/bishop/history.md.squad/agents/lambert/history.md.squad/agents/ripley/history.md.squad/identity/now.md.squad/skills/analyzer-package-audit/SKILL.mdsrc/Refitter.Core/JsonSerializerContextGenerator.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.Tests/SourceGeneratorPackageReferenceTests.cstest/ConsoleApp/Directory.Build.props
✅ Files skipped from review due to trivial changes (5)
- .squad/identity/now.md
- .squad/agents/bishop/history.md
- .squad/skills/analyzer-package-audit/SKILL.md
- .squad/agents/ripley/history.md
- src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1067 +/- ##
==========================================
- Coverage 95.26% 95.20% -0.06%
==========================================
Files 27 27
Lines 1858 2359 +501
==========================================
+ Hits 1770 2246 +476
- Misses 15 40 +25
Partials 73 73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/Refitter.Core/JsonSerializerContextGenerator.cs (3)
25-44:⚠️ Potential issue | 🟠 MajorGuard empty namespaces before emitting the wrapper.
ContractsNamespace = ""still emitsnamespace, producing uncompilable generated code. Treat whitespace as unset and only write the namespace wrapper when a non-empty namespace exists.Proposed fix
- var contextNamespace = settings.ContractsNamespace ?? settings.Namespace; + var contextNamespace = !string.IsNullOrWhiteSpace(settings.ContractsNamespace) + ? settings.ContractsNamespace + : settings.Namespace; var typeNames = ExtractTypeNames(contracts, contextNamespace); @@ - sb.AppendLine($"namespace {contextNamespace}"); - sb.AppendLine("{"); + var hasNamespace = !string.IsNullOrWhiteSpace(contextNamespace); + var indent = hasNamespace ? " " : string.Empty; + + if (hasNamespace) + { + sb.AppendLine($"namespace {contextNamespace}"); + sb.AppendLine("{"); + } foreach (var typeName in typeNames.OrderBy(t => t)) { - sb.AppendLine($" [global::System.Text.Json.Serialization.JsonSerializable(typeof({typeName}))]"); + sb.AppendLine($"{indent}[global::System.Text.Json.Serialization.JsonSerializable(typeof({typeName}))]"); } - sb.AppendLine($" internal partial class {contextName} : global::System.Text.Json.Serialization.JsonSerializerContext"); - sb.AppendLine(" {"); - sb.AppendLine(" }"); - sb.AppendLine("}"); + sb.AppendLine($"{indent}internal partial class {contextName} : global::System.Text.Json.Serialization.JsonSerializerContext"); + sb.AppendLine($"{indent}{{"); + sb.AppendLine($"{indent}}}"); + + if (hasNamespace) + sb.AppendLine("}");#!/bin/bash # Verify whether the current generator still always emits a namespace wrapper. rg -n -C3 'contextNamespace|AppendLine\(\$"namespace' src/Refitter.Core/JsonSerializerContextGenerator.cs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 25 - 44, The generator currently uses settings.ContractsNamespace ?? settings.Namespace to build contextNamespace and emits a namespace wrapper unconditionally, which produces "namespace " when ContractsNamespace is empty/whitespace; update the generation in JsonSerializerContextGenerator.cs so contextNamespace = (settings.ContractsNamespace ?? settings.Namespace)?.Trim() and only emit the namespace wrapper (the sb.AppendLine($"namespace {contextNamespace}") / matching braces) when contextNamespace is not null or empty; ensure ExtractTypeNames and GetContextTypeName are still used but the namespace block is skipped entirely for empty/whitespace contextNamespace so generated code remains compilable.
177-180:⚠️ Potential issue | 🟠 MajorPreserve qualification for external generic container types.
When a declared generic DTO is used as
PagedResult<ImmutableList<Pet>>,ImmutableListis emitted raw because the context file does not copy the contracts file’susing System.Collections.Immutable;. That can still generate uncompilable context code for valid contracts.Add coverage for a closed generic DTO whose type argument is an imported external generic container, then either preserve/import required namespaces or emit fully qualified names for unresolved generic containers.
#!/bin/bash # Inspect unresolved generic formatting and existing tests for external generic containers. sed -n '159,205p' src/Refitter.Core/JsonSerializerContextGenerator.cs rg -n -C2 'ImmutableList|ObservableCollection|JsonSerializerContextGenerator' src/Refitter.Tests --type csAlso applies to: 190-204
95-97:⚠️ Potential issue | 🟠 MajorAccount for generic container arity before registering nested types.
A nested type inside
Outer<T>hasArity == 0, buttypeof(Outer.Inner)is invalid because the containing type is generic. Track container arities and filter on total arity before emitting[JsonSerializable].Proposed direction
- .Where(t => t.Arity == 0) + .Where(t => t.TotalArity == 0)- private sealed record DeclaredTypeInfo(string Namespace, IReadOnlyList<string> Containers, string Name, int Arity) + private sealed record DeclaredTypeInfo( + string Namespace, + IReadOnlyList<string> Containers, + IReadOnlyList<int> ContainerArities, + string Name, + int Arity) { + public int TotalArity => Arity + ContainerArities.Sum();Populate
ContainerAritiesfrom ancestorTypeDeclarationSyntax.TypeParameterList.#!/bin/bash # Verify the current model tracks container names but not container generic arity. sed -n '90,105p;255,285p' src/Refitter.Core/JsonSerializerContextGenerator.csAlso applies to: 255-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 95 - 97, The current logic in JsonSerializerContextGenerator that builds declaredTypes/typeNames does not account for generic container arity, causing nested types inside generic containers (e.g., Outer<T>.Inner) to be treated as non-generic (Arity==0) and emitted as JsonSerializable incorrectly; update the generator to compute and track ContainerArities from ancestor TypeDeclarationSyntax.TypeParameterList for each declared type (populate the ContainerArities map) and then filter declaredTypes by the total arity (type.Arity + containerArity == 0) before selecting GetDisplayName and emitting [JsonSerializable]; ensure the new check is applied in both places referenced (around declaredTypes selection and the similar logic at lines ~255-284) so nested types in generic containers are excluded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.squad/agents/lambert/history.md:
- Around line 198-205: The history entry is stale: update or remove the items
that no longer apply (particularly the bullet claiming
JsonSerializerContextGenerator.cs uses netstandard2.0-only-missing APIs and
blocks `#1017`) so the changelog matches the final PR state; edit the "Key
findings" to either mark `#1017` as resolved or remove that failing-build note and
adjust related items referencing
RefitGenerator.Generate()/GenerateMultipleFiles(),
JsonSerializerContextGenerator.cs,
CSharpClientGeneratorFactory.GenerateOptionalPropertiesAsNullable,
RuntimeCompatibilityTests.cs, RefitterGenerateTask.cs/GenerateCommand.cs, and
Refitter.SourceGenerator.csproj to reflect their final statuses, and delete or
note the cleanup of temporary repro artifacts (.refitter, aot-repro.cs,
aot-repro.json) before committing.
In `@docs/docfx_project/articles/source-generator.md`:
- Around line 7-18: The document jumps from "## Source Generator" to "####
Installation", causing a heading-level skip; change the "Installation" heading
(text: "Installation") from level 4 to level 3 (use "### Installation") and do
the same for the following "Usage" heading (text: "Usage") so both are immediate
children of "## Source Generator" to satisfy markdownlint and maintain proper
heading hierarchy.
In `@src/Refitter.Tests/RefitterGenerateTaskTests.cs`:
- Around line 12-22: The tests use a Windows-centric fake root
(Path.Combine("C:", "repo")) which breaks on non-Windows platforms; replace that
hardcoded root with the cross-platform workspace helper used elsewhere
(CreateWorkspace) or another real temp directory and update the files array
accordingly so FilterFiles is exercised with full platform-agnostic paths;
specifically change the setup in the RefitterGenerateTask tests that call
RefitterGenerateTask.FilterFiles to use CreateWorkspace (or
Path.GetTempPath/CreateTempDirectory pattern) instead of the "C:" path so
RefitterGenerateTask.FilterFiles and its Path.GetFullPath normalization behave
consistently across OSes.
---
Duplicate comments:
In `@src/Refitter.Core/JsonSerializerContextGenerator.cs`:
- Around line 25-44: The generator currently uses settings.ContractsNamespace ??
settings.Namespace to build contextNamespace and emits a namespace wrapper
unconditionally, which produces "namespace " when ContractsNamespace is
empty/whitespace; update the generation in JsonSerializerContextGenerator.cs so
contextNamespace = (settings.ContractsNamespace ?? settings.Namespace)?.Trim()
and only emit the namespace wrapper (the sb.AppendLine($"namespace
{contextNamespace}") / matching braces) when contextNamespace is not null or
empty; ensure ExtractTypeNames and GetContextTypeName are still used but the
namespace block is skipped entirely for empty/whitespace contextNamespace so
generated code remains compilable.
- Around line 95-97: The current logic in JsonSerializerContextGenerator that
builds declaredTypes/typeNames does not account for generic container arity,
causing nested types inside generic containers (e.g., Outer<T>.Inner) to be
treated as non-generic (Arity==0) and emitted as JsonSerializable incorrectly;
update the generator to compute and track ContainerArities from ancestor
TypeDeclarationSyntax.TypeParameterList for each declared type (populate the
ContainerArities map) and then filter declaredTypes by the total arity
(type.Arity + containerArity == 0) before selecting GetDisplayName and emitting
[JsonSerializable]; ensure the new check is applied in both places referenced
(around declaredTypes selection and the similar logic at lines ~255-284) so
nested types in generic containers are excluded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28ceb77a-5981-4cde-8cf7-348ab82c27af
📒 Files selected for processing (16)
.squad/agents/bishop/history.md.squad/agents/dallas/history.md.squad/agents/lambert/history.mdCHANGELOG.mdREADME.mddocs/docfx_project/articles/breaking-changes-v2-0-0.mddocs/docfx_project/articles/source-generator.mdsrc/Refitter.Core/JsonSerializerContextGenerator.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator/README.mdsrc/Refitter.Tests/Examples/RuntimeCompatibilityTests.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/JsonSerializerContextGeneratorTests.cssrc/Refitter.Tests/RefitterGenerateTaskTests.cssrc/Refitter.Tests/SourceGeneratorPackageReferenceTests.cssrc/Refitter/GenerateCommand.cs
✅ Files skipped from review due to trivial changes (3)
- src/Refitter.Tests/GenerateCommandTests.cs
- .squad/agents/dallas/history.md
- docs/docfx_project/articles/breaking-changes-v2-0-0.md
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGELOG.md
- src/Refitter.Tests/JsonSerializerContextGeneratorTests.cs
- .squad/agents/bishop/history.md
- src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs
- src/Refitter.MSBuild/RefitterGenerateTask.cs
Owner
Author
|
@copilot resolve the merge conflicts in this pull request |
Closes #1025. Documents silent behavioral change from Microsoft.OpenApi.Readers upgrade. Users upgrading to v2.0.0 should regenerate and review diffs as the new parser has improved schema interpretation (nullability, discriminators, refs, Swagger 2.0 edge cases). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use CLI-reported generated paths for MSBuild compile items and make RefitterIncludePatterns exact-match filenames or project-relative paths. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep generator-only packages private, require an explicit Refit reference for consumers, and document the exact MSBuild include-pattern behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Record the P1 tooling learnings and document the generated-file marker pattern for future agents. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Direct users to breaking changes documentation when upgrading, with emphasis on the OpenAPI parser migration (#1025). Uses blockquote to stand out in rendered docs and repository view. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ts=all Complete #1024 fix: change Refit from PrivateAssets=compile to PrivateAssets=all to prevent it from leaking as a transitive dependency in the packed nuspec. Consumers must now explicitly add Refit (and Refit.HttpClientFactory if needed) as documented in README.md and source-generator.md. Verified: packed nuspec now shows zero dependencies for netstandard2.0 group. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Ash owned revision #2 for issue #1024 after Dallas lockout. Root cause: PrivateAssets=compile insufficient; required PrivateAssets=all. Verified with pack/extract nuspec inspection: zero dependencies in package. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refs #1017 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nd STJ hint collision Agent-Logs-Url: https://github.com/christianhelle/refitter/sessions/1d220ac8-0337-4eca-a04f-39bef4e81db1 Co-authored-by: christianhelle <710400+christianhelle@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
christianhelle
force-pushed
the
v2.0.0-prerelease-fixes
branch
from
April 23, 2026 12:14
72f28d7 to
b4e32d7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cs (1)
10-56:⚠️ Potential issue | 🟡 MinorStill missing a Swagger 2.0 fixture for serializer-context generation.
The assertions now focus on the exact emitted shape (
[global::System.Text.Json.Serialization.JsonSerializable(typeof(User))],UsersApiSerializerContext : JsonSerializerContext), but the spec fixture is OpenAPI 3.0 only. Please add an equivalent Swagger 2.0 variant (sameUserschema underdefinitions, response schema under Swagger 2.0 shape) so the AOT context generation path is validated against both OpenAPI corpora.As per coding guidelines
src/Refitter.Tests/**/*.cs: "Test new code generation features with both OpenAPI 2.0 and 3.0 specifications".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cs` around lines 10 - 56, Add a Swagger 2.0 fixture alongside the existing OpenApiSpec by introducing a new constant (e.g., Swagger20Spec) in GenerateJsonSerializerContextTests that mirrors the User schema under "definitions" and places the response schema in the Swagger 2.0 shape (swagger: "2.0", paths response using "schema": { "type":"array", "items": { "$ref":"#/definitions/User" } }). Ensure the test(s) that assert the emitted attributes and generated context (references to JsonSerializable(typeof(User)) and UsersApiSerializerContext : JsonSerializerContext) run against both OpenApiSpec and the new Swagger20Spec so the AOT context generation path is validated for OpenAPI 3.0 and Swagger 2.0.docs/docfx_project/articles/source-generator.md (1)
7-18:⚠️ Potential issue | 🟡 MinorHeading levels still jump
##→####(markdownlint MD001).
## Source Generator→#### Installation/#### Usageskips###. Static analysis (markdownlint-cli2MD001) still flags this.Proposed fix
-#### Installation +### Installation @@ -#### Usage +### Usage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docfx_project/articles/source-generator.md` around lines 7 - 18, The documentation jumps from "## Source Generator" to "#### Installation" and "#### Usage", triggering markdownlint MD001; update the heading hierarchy so it flows correctly (either change "#### Installation" and "#### Usage" to "### Installation" and "### Usage" or introduce an intermediate "###" section under the "## Source Generator" heading) by editing the headings in docs/docfx_project/articles/source-generator.md—look for the "Source Generator", "Installation", and "Usage" headings and adjust their levels to remove the jump.src/Refitter.Core/JsonSerializerContextGenerator.cs (2)
95-118:⚠️ Potential issue | 🟠 MajorNested types inside generic containers still produce invalid
typeof(Outer.Inner)registrations.The
Where(t => t.Arity == 0)filter only checks the declared type's own type-parameter count. A non-generic type declared inside a generic container (e.g.,class Outer<T> { class Inner {} }) hasArity == 0but its containerOuteris still generic, soGetDisplayNameyieldsOuter.Inner, which is not a legaltypeof(...)argument in C# (requiresOuter<>.InnerorOuter<SomeArg>.Inner).
DeclaredTypeInfoneeds to track container arities, and both the filter here and the matching inTryResolveDeclaredType(lines 222-226) should reason over the total arity.Sketch of the fix
- private sealed record DeclaredTypeInfo(string Namespace, IReadOnlyList<string> Containers, string Name, int Arity) + private sealed record DeclaredTypeInfo( + string Namespace, + IReadOnlyList<string> Containers, + IReadOnlyList<int> ContainerArities, + string Name, + int Arity) { + public int TotalArity => Arity + ContainerArities.Sum();Then change the filter to
Where(t => t.TotalArity == 0)and populateContainerAritiesinDeclaredTypeInfo.Createfrom the ancestorTypeDeclarationSyntax.TypeParameterListcounts. Add a regression test covering a nested non-generic inside a generic container.Also applies to: 255-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 95 - 118, The filter that builds typeNames must exclude types whose containing types are generic: update DeclaredTypeInfo to track ContainerArities (populate it in DeclaredTypeInfo.Create by walking ancestor TypeDeclarationSyntax nodes and recording each TypeParameterList count) and add a TotalArity property that sums the declared Arity plus container arities; then change the selection here (and the similar spot in the block around TryResolveDeclaredType) from Where(t => t.Arity == 0) to Where(t => t.TotalArity == 0) and adjust TryResolveDeclaredType/GetDisplayName usage to use TotalArity-aware logic so nested non-generic types inside generic containers produce valid typeof(...) names; add a regression test for a non-generic Inner inside a generic Outer<T>.
25-44:⚠️ Potential issue | 🟠 MajorEmpty
ContractsNamespacestill emits an invalidnamespacewrapper.Line 25 only falls back when
ContractsNamespaceisnull;ContractsNamespace = ""will propagate through and Line 33 will emit a barenamespaceline, producing uncompilable output. Treat whitespace as unset and skip the wrapper entirely when no namespace is configured.🐛 Proposed fix
- var contextNamespace = settings.ContractsNamespace ?? settings.Namespace; + var contextNamespace = !string.IsNullOrWhiteSpace(settings.ContractsNamespace) + ? settings.ContractsNamespace! + : settings.Namespace ?? string.Empty; var typeNames = ExtractTypeNames(contracts, contextNamespace); if (typeNames.Count == 0) return string.Empty; var contextName = GetContextTypeName(settings, documentTitle); var sb = new StringBuilder(); + var hasNamespace = !string.IsNullOrWhiteSpace(contextNamespace); + var indent = hasNamespace ? " " : string.Empty; - sb.AppendLine($"namespace {contextNamespace}"); - sb.AppendLine("{"); + if (hasNamespace) + { + sb.AppendLine($"namespace {contextNamespace}"); + sb.AppendLine("{"); + } foreach (var typeName in typeNames.OrderBy(t => t)) { - sb.AppendLine($" [global::System.Text.Json.Serialization.JsonSerializable(typeof({typeName}))]"); + sb.AppendLine($"{indent}[global::System.Text.Json.Serialization.JsonSerializable(typeof({typeName}))]"); } - sb.AppendLine($" internal partial class {contextName} : global::System.Text.Json.Serialization.JsonSerializerContext"); - sb.AppendLine(" {"); - sb.AppendLine(" }"); - sb.AppendLine("}"); + sb.AppendLine($"{indent}internal partial class {contextName} : global::System.Text.Json.Serialization.JsonSerializerContext"); + sb.AppendLine($"{indent}{{"); + sb.AppendLine($"{indent}}}"); + if (hasNamespace) sb.AppendLine("}");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/JsonSerializerContextGenerator.cs` around lines 25 - 44, The code emits an invalid empty "namespace " when settings.ContractsNamespace is an empty or whitespace string; update the logic around contextNamespace in JsonSerializerContextGenerator (where you call ExtractTypeNames and GetContextTypeName) to treat ContractsNamespace as unset when string.IsNullOrWhiteSpace(settings.ContractsNamespace) and fall back to settings.Namespace; if the resulting contextNamespace is null/empty/whitespace then do not emit the namespace wrapper lines at all (emit the JsonSerializable attributes and the internal partial class {contextName} at top level) so the generated output remains compilable..squad/agents/lambert/history.md (1)
198-205:⚠️ Potential issue | 🟡 MinorAudit entries still contradict the final PR state.
Line 199 still claims
JsonSerializerContextGenerator.csrelies on APIs unavailable onnetstandard2.0(ReplaceLineEndings, range/index,ToHashSet) and that the worktree does not build, while the PR summary reports a successful build and 1779/1779 tests passing. Line 205 still lists the temporary repro artifacts (.refitter,aot-repro.cs,aot-repro.json) as untracked in the repo root — verify and update or remove these bullets so the log matches the final state before merge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.squad/agents/lambert/history.md around lines 198 - 205, Update the audit bullets in .squad/agents/lambert/history.md so they reflect the PR final state: remove or correct the claim that JsonSerializerContextGenerator (src/Refitter.Core/JsonSerializerContextGenerator.cs) still uses netstandard2.0-only-missing APIs (ReplaceLineEndings, range/index, ToHashSet) if the build and tests actually succeeded, and remove or mark as intentionally retained the temporary repro artifacts (.refitter, aot-repro.cs, aot-repro.json) instead of listing them as untracked; confirm against the actual build/test output reported by RefitGenerator.Generate()/GenerateMultipleFiles() and the repo working tree, then edit the specific bullets to match the verified outcome so the audit matches the PR summary before merge.src/Refitter.Tests/RefitterGenerateTaskTests.cs (1)
12-74:⚠️ Potential issue | 🟠 MajorCross-platform path issue still present in five tests.
Path.Combine("C:", "repo")does not produce a rooted path on any platform: on Linux/macOS it becomes"C:/repo"(a relative path whose first segment literally contains a colon), and even on WindowsPath.Combine("C:", "repo")yields drive-relative"C:repo"rather than"C:\repo". Downstream inFilterFiles,Path.GetFullPath(file)will resolve these against the current working directory andnew Uri(AppendDirectorySeparator(relativeTo))insideGetRelativePathwill throwUriFormatExceptionon non-Windows.Use
CreateWorkspace()(already defined at line 300 and used at lines 152 and 213) so filtering exercises real absolute paths on all platforms. Applies to lines 12, 28, 43, 59, and 74.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Tests/RefitterGenerateTaskTests.cs` around lines 12 - 74, The tests use Path.Combine("C:", "repo") which produces non-absolute or drive-relative paths cross-platform and causes UriFormatException in RefitterGenerateTask.FilterFiles; replace those hard-coded projectRoot values with the existing CreateWorkspace() helper used elsewhere so each test (the methods FilterFiles_Should_Match_Filename, FilterFiles_Should_Match_Project_Relative_Path, FilterFiles_Should_Match_Exact_Full_Path, FilterFiles_Should_Not_Match_Substring_Project_Relative_Path, and the ParseGeneratedFilePath test) constructs files using the CreateWorkspace() result as projectRoot (and then Path.Combine(projectRoot, ...)) to ensure real absolute paths on all platforms before calling RefitterGenerateTask.FilterFiles.
🧹 Nitpick comments (4)
src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs (1)
9-64: Consider sharing thedotnet packinvocation between the two tests.Each test independently spins up a
dotnet pack+ creates/deletes its own workspace. Packing Refitter.SourceGenerator is not cheap (multi-second, plus I/O), and running it twice per test run (and potentially in parallel) is wasteful. Consider hoisting the pack into a one-time class-level fixture (TUnit[Before(Class)]/[ClassDataSource]) so both assertions operate on the same produced.nupkg.This would also remove the parallel-test race where both tests set
WorkingDirectory = repoRootand run MSBuild concurrently against the same project.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs` around lines 9 - 64, Both tests repeatedly call PackSourceGeneratorPackage/CreateWorkspace/DeleteWorkspace which runs dotnet pack twice and can race; move packing into a one-time class fixture (e.g., add a OneTimeSetUp/OneTimeTearDown in the test class) that calls CreateWorkspace and PackSourceGeneratorPackage once and stores the resulting workspace and packagePath in class-level fields, then update the tests (Packed_SourceGenerator_Package_Should_Not_Expose_Generator_Implementation_Dependencies and Packed_SourceGenerator_Package_Should_Only_Ship_Its_Own_Analyzer_Assets) to use those fields and remove their local CreateWorkspace/Pack/try/finally/DeleteWorkspace logic so the heavy dotnet pack is executed once and cleaned up in OneTimeTearDown using DeleteWorkspace.src/Refitter/GenerateCommand.cs (2)
24-24: Marker contract is duplicated across CLI and MSBuild task; factor it into one shared source.
GeneratedFileMarker = "GeneratedFile: "is defined here and again atsrc/Refitter.MSBuild/RefitterGenerateTask.cs:10as two independentinternal conststrings. Since the two projects form a strict producer/consumer contract, any future edit on one side (e.g. renaming the prefix, changing the trailing space) will silently break MSBuild output discovery — the task will just report "Refitter did not report any generated files" with no other clue. Consider exposing it from a single shared location (e.g. aRefitter.Corepublic const, orInternalsVisibleTointo MSBuild) so both sides drift together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter/GenerateCommand.cs` at line 24, The GeneratedFileMarker string is duplicated between GenerateCommand.cs (GeneratedFileMarker) and RefitterGenerateTask.cs, risking drift; move this constant into a single shared location (e.g., expose a public const in a common assembly like Refitter.Core or make it accessible via InternalsVisibleTo) and update both GenerateCommand and RefitterGenerateTask to reference that single symbol instead of their own internal copies so the producer/consumer contract is maintained.
781-791: Align the static single-fileGetOutputPathwith the same helpers.The instance overload here is nicely consolidated around
HasExplicitCliOutputOverride+CombineWithSettingsRoot, but the static single-fileGetOutputPathat lines 695-739 still inlines the equivalent check (cliOverridesOutput = !IsNullOrWhiteSpace && != DefaultOutputPath) and hand-rolls the rooting logic. Reusing the new helpers there would eliminate the subtle duplication and make the override semantics identical across single- and multi-file paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter/GenerateCommand.cs` around lines 781 - 791, Update the static single-file GetOutputPath implementation to reuse the existing helpers instead of duplicating logic: replace the inline check of cliOverridesOutput (!IsNullOrWhiteSpace && != DefaultOutputPath) and the manual rooting/code path with a call to HasExplicitCliOutputOverride(settings) and then use CombineWithSettingsRoot(root, outputFolder, fileName) (or CombineWithSettingsRoot(root, fileName) when no explicit output) so the static GetOutputPath uses the same semantics as the instance overload and the refitGeneratorSettings.OutputFolder/DefaultOutputPath behavior is preserved.src/Refitter.MSBuild/RefitterGenerateTask.cs (1)
291-304:HandleProcessStandardOutputaccepts the collection and its lock as separate parameters.The call site at line 132 passes
outputLinestwice (once as the collection, once as the lock), and the tests passnew object()as the lock. The double-parameter shape is easy to misuse (e.g. two different callers could pass two unrelated lock objects while sharing the same collection, defeating synchronization). Consider either wrapping the collection in a small thread-safe helper or locking on the collection reference internally so callers can't accidentally desync lock and list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.MSBuild/RefitterGenerateTask.cs` around lines 291 - 304, HandleProcessStandardOutput currently takes the collection and its lock as separate parameters which is error-prone; change it so callers cannot desynchronize the lock and list by either (a) removing the outputLinesLock parameter and locking on the collection itself (lock(outputLines) after null-check), or (b) introduce a small thread-safe wrapper type (e.g., OutputLinesBuffer with an Add(string) method) and accept that instead; update all callers that invoke HandleProcessStandardOutput (and tests) to pass the single safe object (either the collection to be locked on or the new wrapper) and ensure the method uses that single object for synchronization when adding to outputLines and logging via logCommandLine.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 318-332: ResolveGeneratedFiles currently drops
reported-but-missing paths silently; modify it to iterate the parsed paths (from
ParseGeneratedFilePath) and for each path that is null/whitespace or where
File.Exists(path) is false call TryLogWarning (or TryLogError per caller style)
with a clear message that includes the settingsFilePath and the reported path so
the mismatch is visible, while still returning the existingGeneratedFiles list
and preserving the existing errorMessage behavior when the final list is empty;
reference ResolveGeneratedFiles and ParseGeneratedFilePath and add calls to
TryLogWarning/TryLogError for each missing file.
In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs`:
- Around line 66-95: Replace the blocking process.WaitForExit() call in
PackSourceGeneratorPackage with a bounded wait: call process.WaitForExit with a
reasonable timeout (e.g., 30s), and if it returns false, kill the process
(process.Kill/Close) and fail the test including the captured stdout/stderr;
otherwise continue to read StandardOutput/StandardError and assert
process.ExitCode as before. Ensure you still capture output and error variables
from process.StandardOutput/StandardError and include them in the failure
message when the timeout elapses so debugging info is preserved.
---
Duplicate comments:
In @.squad/agents/lambert/history.md:
- Around line 198-205: Update the audit bullets in
.squad/agents/lambert/history.md so they reflect the PR final state: remove or
correct the claim that JsonSerializerContextGenerator
(src/Refitter.Core/JsonSerializerContextGenerator.cs) still uses
netstandard2.0-only-missing APIs (ReplaceLineEndings, range/index, ToHashSet) if
the build and tests actually succeeded, and remove or mark as intentionally
retained the temporary repro artifacts (.refitter, aot-repro.cs, aot-repro.json)
instead of listing them as untracked; confirm against the actual build/test
output reported by RefitGenerator.Generate()/GenerateMultipleFiles() and the
repo working tree, then edit the specific bullets to match the verified outcome
so the audit matches the PR summary before merge.
In `@docs/docfx_project/articles/source-generator.md`:
- Around line 7-18: The documentation jumps from "## Source Generator" to "####
Installation" and "#### Usage", triggering markdownlint MD001; update the
heading hierarchy so it flows correctly (either change "#### Installation" and
"#### Usage" to "### Installation" and "### Usage" or introduce an intermediate
"###" section under the "## Source Generator" heading) by editing the headings
in docs/docfx_project/articles/source-generator.md—look for the "Source
Generator", "Installation", and "Usage" headings and adjust their levels to
remove the jump.
In `@src/Refitter.Core/JsonSerializerContextGenerator.cs`:
- Around line 95-118: The filter that builds typeNames must exclude types whose
containing types are generic: update DeclaredTypeInfo to track ContainerArities
(populate it in DeclaredTypeInfo.Create by walking ancestor
TypeDeclarationSyntax nodes and recording each TypeParameterList count) and add
a TotalArity property that sums the declared Arity plus container arities; then
change the selection here (and the similar spot in the block around
TryResolveDeclaredType) from Where(t => t.Arity == 0) to Where(t => t.TotalArity
== 0) and adjust TryResolveDeclaredType/GetDisplayName usage to use
TotalArity-aware logic so nested non-generic types inside generic containers
produce valid typeof(...) names; add a regression test for a non-generic Inner
inside a generic Outer<T>.
- Around line 25-44: The code emits an invalid empty "namespace " when
settings.ContractsNamespace is an empty or whitespace string; update the logic
around contextNamespace in JsonSerializerContextGenerator (where you call
ExtractTypeNames and GetContextTypeName) to treat ContractsNamespace as unset
when string.IsNullOrWhiteSpace(settings.ContractsNamespace) and fall back to
settings.Namespace; if the resulting contextNamespace is null/empty/whitespace
then do not emit the namespace wrapper lines at all (emit the JsonSerializable
attributes and the internal partial class {contextName} at top level) so the
generated output remains compilable.
In `@src/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cs`:
- Around line 10-56: Add a Swagger 2.0 fixture alongside the existing
OpenApiSpec by introducing a new constant (e.g., Swagger20Spec) in
GenerateJsonSerializerContextTests that mirrors the User schema under
"definitions" and places the response schema in the Swagger 2.0 shape (swagger:
"2.0", paths response using "schema": { "type":"array", "items": {
"$ref":"#/definitions/User" } }). Ensure the test(s) that assert the emitted
attributes and generated context (references to JsonSerializable(typeof(User))
and UsersApiSerializerContext : JsonSerializerContext) run against both
OpenApiSpec and the new Swagger20Spec so the AOT context generation path is
validated for OpenAPI 3.0 and Swagger 2.0.
In `@src/Refitter.Tests/RefitterGenerateTaskTests.cs`:
- Around line 12-74: The tests use Path.Combine("C:", "repo") which produces
non-absolute or drive-relative paths cross-platform and causes
UriFormatException in RefitterGenerateTask.FilterFiles; replace those hard-coded
projectRoot values with the existing CreateWorkspace() helper used elsewhere so
each test (the methods FilterFiles_Should_Match_Filename,
FilterFiles_Should_Match_Project_Relative_Path,
FilterFiles_Should_Match_Exact_Full_Path,
FilterFiles_Should_Not_Match_Substring_Project_Relative_Path, and the
ParseGeneratedFilePath test) constructs files using the CreateWorkspace() result
as projectRoot (and then Path.Combine(projectRoot, ...)) to ensure real absolute
paths on all platforms before calling RefitterGenerateTask.FilterFiles.
---
Nitpick comments:
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 291-304: HandleProcessStandardOutput currently takes the
collection and its lock as separate parameters which is error-prone; change it
so callers cannot desynchronize the lock and list by either (a) removing the
outputLinesLock parameter and locking on the collection itself
(lock(outputLines) after null-check), or (b) introduce a small thread-safe
wrapper type (e.g., OutputLinesBuffer with an Add(string) method) and accept
that instead; update all callers that invoke HandleProcessStandardOutput (and
tests) to pass the single safe object (either the collection to be locked on or
the new wrapper) and ensure the method uses that single object for
synchronization when adding to outputLines and logging via logCommandLine.
In `@src/Refitter.Tests/SourceGeneratorPackageReferenceTests.cs`:
- Around line 9-64: Both tests repeatedly call
PackSourceGeneratorPackage/CreateWorkspace/DeleteWorkspace which runs dotnet
pack twice and can race; move packing into a one-time class fixture (e.g., add a
OneTimeSetUp/OneTimeTearDown in the test class) that calls CreateWorkspace and
PackSourceGeneratorPackage once and stores the resulting workspace and
packagePath in class-level fields, then update the tests
(Packed_SourceGenerator_Package_Should_Not_Expose_Generator_Implementation_Dependencies
and Packed_SourceGenerator_Package_Should_Only_Ship_Its_Own_Analyzer_Assets) to
use those fields and remove their local
CreateWorkspace/Pack/try/finally/DeleteWorkspace logic so the heavy dotnet pack
is executed once and cleaned up in OneTimeTearDown using DeleteWorkspace.
In `@src/Refitter/GenerateCommand.cs`:
- Line 24: The GeneratedFileMarker string is duplicated between
GenerateCommand.cs (GeneratedFileMarker) and RefitterGenerateTask.cs, risking
drift; move this constant into a single shared location (e.g., expose a public
const in a common assembly like Refitter.Core or make it accessible via
InternalsVisibleTo) and update both GenerateCommand and RefitterGenerateTask to
reference that single symbol instead of their own internal copies so the
producer/consumer contract is maintained.
- Around line 781-791: Update the static single-file GetOutputPath
implementation to reuse the existing helpers instead of duplicating logic:
replace the inline check of cliOverridesOutput (!IsNullOrWhiteSpace && !=
DefaultOutputPath) and the manual rooting/code path with a call to
HasExplicitCliOutputOverride(settings) and then use
CombineWithSettingsRoot(root, outputFolder, fileName) (or
CombineWithSettingsRoot(root, fileName) when no explicit output) so the static
GetOutputPath uses the same semantics as the instance overload and the
refitGeneratorSettings.OutputFolder/DefaultOutputPath behavior is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 240c275f-86ce-4b50-aa6d-41512247b1d9
📒 Files selected for processing (33)
.squad/agents/ash/history.md.squad/agents/bishop/history.md.squad/agents/dallas/history.md.squad/agents/lambert/history.md.squad/agents/parker/history.md.squad/agents/ripley/history.md.squad/identity/now.md.squad/skills/analyzer-package-audit/SKILL.md.squad/skills/aot-json-serializer-context/SKILL.md.squad/skills/generated-file-manifest/SKILL.mdCHANGELOG.mdREADME.mddocs/docfx_project/articles/breaking-changes-v2-0-0.mddocs/docfx_project/articles/source-generator.mdsrc/Refitter.Core/CSharpClientGeneratorFactory.cssrc/Refitter.Core/JsonSerializerContextGenerator.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.Core/Settings/RefitGeneratorSettings.cssrc/Refitter.MSBuild/AssemblyInfo.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator/README.mdsrc/Refitter.SourceGenerator/Refitter.SourceGenerator.csprojsrc/Refitter.Tests/Examples/GenerateJsonSerializerContextPolymorphismTests.cssrc/Refitter.Tests/Examples/GenerateJsonSerializerContextTests.cssrc/Refitter.Tests/Examples/RuntimeCompatibilityTests.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/JsonSerializerContextGeneratorTests.cssrc/Refitter.Tests/RefitGeneratorAdvancedTests.cssrc/Refitter.Tests/Refitter.Tests.csprojsrc/Refitter.Tests/RefitterGenerateTaskTests.cssrc/Refitter.Tests/SourceGeneratorPackageReferenceTests.cssrc/Refitter/GenerateCommand.cstest/ConsoleApp/Directory.Build.props
💤 Files with no reviewable changes (1)
- src/Refitter.Core/CSharpClientGeneratorFactory.cs
✅ Files skipped from review due to trivial changes (10)
- src/Refitter.MSBuild/AssemblyInfo.cs
- src/Refitter.SourceGenerator/Refitter.SourceGenerator.csproj
- src/Refitter.Tests/Refitter.Tests.csproj
- .squad/skills/generated-file-manifest/SKILL.md
- .squad/identity/now.md
- .squad/skills/analyzer-package-audit/SKILL.md
- src/Refitter.Core/Settings/RefitGeneratorSettings.cs
- .squad/agents/ash/history.md
- CHANGELOG.md
- .squad/agents/parker/history.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Refitter.Tests/RefitGeneratorAdvancedTests.cs
- test/ConsoleApp/Directory.Build.props
- .squad/agents/dallas/history.md
- .squad/agents/bishop/history.md
- src/Refitter.Tests/Examples/GenerateJsonSerializerContextPolymorphismTests.cs
- .squad/agents/ripley/history.md
- src/Refitter.Core/RefitGenerator.cs
- README.md
christianhelle
force-pushed
the
v2.0.0-prerelease-fixes
branch
from
April 23, 2026 22:30
0cae1aa to
8fe4b50
Compare
Prevent the preparatory Release build from creating a shared nupkg in repo outputs during coverage runs. Also switch nested dotnet invocations to ArgumentList and async stream reads to avoid quoting bugs and pipe deadlocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add deterministic task-process hooks so runtime, timeout, and termination branches can be covered without changing production behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
christianhelle
force-pushed
the
v2.0.0-prerelease-fixes
branch
from
April 25, 2026 08:46
791b2fd to
29d9f0b
Compare
|
christianhelle
enabled auto-merge
April 25, 2026 08:55
This was referenced May 3, 2026
hwinther
pushed a commit
to hwinther/test
that referenced
this pull request
May 6, 2026
Updated [refitter](https://github.com/christianhelle/refitter) from 1.7.1 to 2.0.0. <details> <summary>Release notes</summary> _Sourced from [refitter's releases](https://github.com/christianhelle/refitter/releases)._ ## 2.0.0 ## What's Changed * Fix numeric format with pattern quirk - infer type from format for all numeric types by @Copilot in christianhelle/refitter#869 * Read group documentation from document tags. by @DJ4ddi in christianhelle/refitter#887 * Fix null reference and XML escaping in XmlDocumentationGenerator by @Copilot in christianhelle/refitter#890 * Fix build workflow: add dotnet restore before dotnet msbuild in Prepare step by @Copilot in christianhelle/refitter#892 christianhelle/refitter#895 * Fix MSBuild workflow by @christianhelle in christianhelle/refitter#898 * Add support for generating a single client from multiple OpenAPI specifications by @Copilot in christianhelle/refitter#904 * Migrate from Microsoft.OpenApi.Readers 1.x to Microsoft.OpenApi 3.x by @vgmello in christianhelle/refitter#907 * Improve Smoke Tests execution time by @christianhelle in christianhelle/refitter#915 * Fix #580: Nullable strings marked correctly by @christianhelle in christianhelle/refitter#921 * Fix #672: MultipleInterfaces ByTag method naming scoped per-interface by @christianhelle in christianhelle/refitter#922 * Fix #635: Refactor source generator to use context.AddSource() by @christianhelle in christianhelle/refitter#923 * Add custom format mappings configuration by @christianhelle in christianhelle/refitter#927 * Fix multipart form-data parameter extraction by @christianhelle in christianhelle/refitter#928 christianhelle/refitter#911 christianhelle/refitter#902 * Fix smoke tests: --interface-only variant missing using directive for contract types by @Copilot in christianhelle/refitter#933 * Fix SonarCloud Code Quality Issues by @Copilot in christianhelle/refitter#932 * Add debug logging for source generator when searching for .refitter files by @codymullins in christianhelle/refitter#743 * Fix: Base type not generated for types using oneOf with discriminator by @Copilot in christianhelle/refitter#906 * Add option for Method Level Authorization header attribute by @Roflincopter in christianhelle/refitter#897 * Fix PR #897 review feedback and add comprehensive bearer auth tests by @christianhelle in christianhelle/refitter#936 * Fix numeric suffix added to interface method names in ByTag mode by @Copilot in christianhelle/refitter#914 * Improve OpenAPI parse + codegen throughput by removing avoidable allocations and repeated regex work by @Copilot in christianhelle/refitter#937 * Move [JsonConverter] from enum properties to enum types by @christianhelle in christianhelle/refitter#938 * Fix broken CLI tool help text by @christianhelle in christianhelle/refitter#940 * Improve code coverage to >90% by @christianhelle in christianhelle/refitter#941 * Microsoft.OpenApi v3.4 by @christianhelle in christianhelle/refitter#945 * Add Unicode support for XML doc comment generation by @christianhelle in christianhelle/refitter#948 * Add PropertyNamingPolicy support for JSON property naming by @christianhelle in christianhelle/refitter#969 christianhelle/refitter#966 * Fix recursive schema stack overflows by @christianhelle in christianhelle/refitter#971 * Made Header Parameters for Security Schemes safe to use as C# variable name by @smoerijf in christianhelle/refitter#977 * Enhance schema alias handling and property name generation by @christianhelle in christianhelle/refitter#996 * Verify alias handling and sanitize PascalCase properties by @christianhelle in christianhelle/refitter#997 * Harden .refitter settings deserialization and output path handling by @christianhelle in christianhelle/refitter#1000 * Special-case the default .refitter filename before deriving OutputFilename by @coderabbitai[bot] in christianhelle/refitter#1002 * [v2.0 audit] Fix pre-release regressions from #1057 by @christianhelle in christianhelle/refitter#1064 * Resolve high-severity audit findings from #1057 by @christianhelle in christianhelle/refitter#1067 * [v2.0 audit] Close remaining verified #1057 regressions by @christianhelle in christianhelle/refitter#1070 * Harden generation flows by @christianhelle in christianhelle/refitter#1071 * Breaking changes and Migration Guide for v2.0.0 by @christianhelle in christianhelle/refitter#1009 * Handle equivalent duplicate schemas in multi-spec merge by @christianhelle in christianhelle/refitter#1076 * Tighten warning handling across Refitter builds by @christianhelle in christianhelle/refitter#1079 * Fix default solution path and update .NET version in VS Code config by @christianhelle in christianhelle/refitter#1082 * Fix docker smoke tests by @christianhelle in christianhelle/refitter#1081 * Sanitize malformed schema type names without renaming clean schemas by @christianhelle in christianhelle/refitter#1085 ... (truncated) ## 1.7.3 ## What's Changed * Add support for systems running only .NET 10.0 (without .NET 8.0 or 9.0) in Refitter.MSBuild by @christianhelle in christianhelle/refitter#882 * Update to return HttpResponseMessage for file downloads by @frogcrush in christianhelle/refitter#877 ## New Contributors * @frogcrush made their first contribution in christianhelle/refitter#877 **Full Changelog**: christianhelle/refitter@1.7.2...1.7.3 ## 1.7.2 **Implemented enhancements:** - Improve Immutable Records ergonomics [\#844](christianhelle/refitter#844) - Omit certain operation headers and include all others [\#840](christianhelle/refitter#840) - Create .refitter settings file as part of output [\#859](christianhelle/refitter#859) by @[christianhelle - Fix integerType enum deserialization issue [\#855](christianhelle/refitter#855) ([christianhelle](https://github.com/christianhelle)) - support custom nswag template directory \#844 [\#854](christianhelle/refitter#854) by @kmc059000 - Fix missing method parameter XML code-documentation [\#850](christianhelle/refitter#850) ([christianhelle](https://github.com/christianhelle)) **Fixed bugs:** - integerType parsing in settings file fails [\#851](christianhelle/refitter#851) - CS1573 : Method parameter has no matching XML comment [\#846](christianhelle/refitter#846) **Merged pull requests:** - docs: add frogcrush as a contributor for code [\#878](christianhelle/refitter#878) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - Migrate solution files from .sln to .slnx format [\#876](christianhelle/refitter#876) ([Copilot](https://github.com/apps/copilot-swe-agent)) - Fix issue with randomly failing tests to due parallel execution [\#872](christianhelle/refitter#872) ([christianhelle](https://github.com/christianhelle)) - chore\(deps\): update dependency tunit to 1.9.2 [\#863](christianhelle/refitter#863) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.7.7 [\#862](christianhelle/refitter#862) ([renovate[bot]](https://github.com/apps/renovate)) - Add unit tests for WriteRefitterSettingsFile functionality [\#860](christianhelle/refitter#860) ([Copilot](https://github.com/apps/copilot-swe-agent)) - chore\(deps\): update dependency ruby to v4 [\#858](christianhelle/refitter#858) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.28 [\#857](christianhelle/refitter#857) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add 0x2badc0de as a contributor for bug [\#856](christianhelle/refitter#856) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency swashbuckle.aspnetcore to 10.1.0 [\#853](christianhelle/refitter#853) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.6.0 [\#852](christianhelle/refitter#852) ([renovate[bot]](https://github.com/apps/renovate)) - docs: add lilinus as a contributor for code [\#849](christianhelle/refitter#849) ([allcontributors[bot]](https://github.com/apps/allcontributors)) - chore\(deps\): update dependency ruby to v3.4.8 [\#845](christianhelle/refitter#845) ([renovate[bot]](https://github.com/apps/renovate)) - chore\(deps\): update dependency tunit to 1.5.70 [\#837](christianhelle/refitter#837) ([renovate[bot]](https://github.com/apps/renovate)) **Full Changelog**: christianhelle/refitter@1.7.1...1.7.2 Commits viewable in [compare view](christianhelle/refitter@1.7.1...2.0.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
vgmello
pushed a commit
to vgmello/momentum
that referenced
this pull request
May 10, 2026
Updated [Refitter.MSBuild](https://github.com/christianhelle/refitter) from 1.7.3 to 2.0.0. <details> <summary>Release notes</summary> _Sourced from [Refitter.MSBuild's releases](https://github.com/christianhelle/refitter/releases)._ ## 2.0.0 ## What's Changed * Fix numeric format with pattern quirk - infer type from format for all numeric types by @Copilot in christianhelle/refitter#869 * Read group documentation from document tags. by @DJ4ddi in christianhelle/refitter#887 * Fix null reference and XML escaping in XmlDocumentationGenerator by @Copilot in christianhelle/refitter#890 * Fix build workflow: add dotnet restore before dotnet msbuild in Prepare step by @Copilot in christianhelle/refitter#892 christianhelle/refitter#895 * Fix MSBuild workflow by @christianhelle in christianhelle/refitter#898 * Add support for generating a single client from multiple OpenAPI specifications by @Copilot in christianhelle/refitter#904 * Migrate from Microsoft.OpenApi.Readers 1.x to Microsoft.OpenApi 3.x by @vgmello in christianhelle/refitter#907 * Improve Smoke Tests execution time by @christianhelle in christianhelle/refitter#915 * Fix #580: Nullable strings marked correctly by @christianhelle in christianhelle/refitter#921 * Fix #672: MultipleInterfaces ByTag method naming scoped per-interface by @christianhelle in christianhelle/refitter#922 * Fix #635: Refactor source generator to use context.AddSource() by @christianhelle in christianhelle/refitter#923 * Add custom format mappings configuration by @christianhelle in christianhelle/refitter#927 * Fix multipart form-data parameter extraction by @christianhelle in christianhelle/refitter#928 christianhelle/refitter#911 christianhelle/refitter#902 * Fix smoke tests: --interface-only variant missing using directive for contract types by @Copilot in christianhelle/refitter#933 * Fix SonarCloud Code Quality Issues by @Copilot in christianhelle/refitter#932 * Add debug logging for source generator when searching for .refitter files by @codymullins in christianhelle/refitter#743 * Fix: Base type not generated for types using oneOf with discriminator by @Copilot in christianhelle/refitter#906 * Add option for Method Level Authorization header attribute by @Roflincopter in christianhelle/refitter#897 * Fix PR #897 review feedback and add comprehensive bearer auth tests by @christianhelle in christianhelle/refitter#936 * Fix numeric suffix added to interface method names in ByTag mode by @Copilot in christianhelle/refitter#914 * Improve OpenAPI parse + codegen throughput by removing avoidable allocations and repeated regex work by @Copilot in christianhelle/refitter#937 * Move [JsonConverter] from enum properties to enum types by @christianhelle in christianhelle/refitter#938 * Fix broken CLI tool help text by @christianhelle in christianhelle/refitter#940 * Improve code coverage to >90% by @christianhelle in christianhelle/refitter#941 * Microsoft.OpenApi v3.4 by @christianhelle in christianhelle/refitter#945 * Add Unicode support for XML doc comment generation by @christianhelle in christianhelle/refitter#948 * Add PropertyNamingPolicy support for JSON property naming by @christianhelle in christianhelle/refitter#969 christianhelle/refitter#966 * Fix recursive schema stack overflows by @christianhelle in christianhelle/refitter#971 * Made Header Parameters for Security Schemes safe to use as C# variable name by @smoerijf in christianhelle/refitter#977 * Enhance schema alias handling and property name generation by @christianhelle in christianhelle/refitter#996 * Verify alias handling and sanitize PascalCase properties by @christianhelle in christianhelle/refitter#997 * Harden .refitter settings deserialization and output path handling by @christianhelle in christianhelle/refitter#1000 * Special-case the default .refitter filename before deriving OutputFilename by @coderabbitai[bot] in christianhelle/refitter#1002 * [v2.0 audit] Fix pre-release regressions from #1057 by @christianhelle in christianhelle/refitter#1064 * Resolve high-severity audit findings from #1057 by @christianhelle in christianhelle/refitter#1067 * [v2.0 audit] Close remaining verified #1057 regressions by @christianhelle in christianhelle/refitter#1070 * Harden generation flows by @christianhelle in christianhelle/refitter#1071 * Breaking changes and Migration Guide for v2.0.0 by @christianhelle in christianhelle/refitter#1009 * Handle equivalent duplicate schemas in multi-spec merge by @christianhelle in christianhelle/refitter#1076 * Tighten warning handling across Refitter builds by @christianhelle in christianhelle/refitter#1079 * Fix default solution path and update .NET version in VS Code config by @christianhelle in christianhelle/refitter#1082 * Fix docker smoke tests by @christianhelle in christianhelle/refitter#1081 * Sanitize malformed schema type names without renaming clean schemas by @christianhelle in christianhelle/refitter#1085 ... (truncated) Commits viewable in [compare view](christianhelle/refitter@1.7.3...2.0.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore <dependency name> major version` will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself) - `@dependabot ignore <dependency name> minor version` will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself) - `@dependabot ignore <dependency name>` will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself) - `@dependabot unignore <dependency name>` will remove all of the ignore conditions of the specified dependency - `@dependabot unignore <dependency name> <ignore condition>` will remove the ignore condition of the specified dependency and ignore conditions </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This was referenced May 30, 2026
This was referenced Jun 12, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR resolves the high-severity audit findings from #1057 that are fully fixed in code, and adds migration guidance for the remaining documented OpenAPI parser upgrade behavior.
Fixed Issues
Changes
Validation
Breaking Changes
See docs/docfx_project/articles/breaking-changes-v2-0-0.md for migration guidance.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Documentation