[v2.0 audit] Close remaining verified #1057 regressions#1070
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 13 minutes and 25 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 Walkthrough📝 Walkthrough🚥 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)
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses remaining verified regressions from the v2.0 audit (#1057) across the CLI, core generator behavior, source generator diagnostics/caching, OpenAPI multi-spec merge behavior, and MSBuild task runtime resolution—aiming to make generation deterministic, non-mutating, and backward compatible.
Changes:
- Restore/extend CLI backward compatibility for
--generate-authentication-headerwhile validating/normalizing the value intoAuthenticationHeaderStyle. - Prevent shared NSwag model mutation during dynamic querystring wrapper generation; add regression coverage.
- Make multi-spec merge fail fast on conflicts and avoid mutating caller-owned documents; improve MSBuild runtime selection robustness; improve source-generator diagnostics & incremental caching behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Refitter/SettingsValidator.cs | Validates --generate-authentication-header early and returns a CLI-friendly error message on invalid values. |
| src/Refitter/Settings.cs | Changes auth-header option to accept legacy boolean forms and adds parsing/normalization helper. |
| src/Refitter/GenerateCommand.cs | Uses normalized AuthenticationHeaderStyle rather than binding directly to the old settings property shape. |
| src/Refitter.Tests/SettingsTests.cs | Adds unit tests covering legacy boolean mapping + enum parsing for auth header style. |
| src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs | New regression test ensuring dynamic querystring generation doesn’t break later XML doc generation. |
| src/Refitter.Tests/RefitterGenerateTaskTests.cs | Expands MSBuild task coverage for runtime fallback + timeout logging. |
| src/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cs | Adds direct assertion that query parameter collections are not mutated during wrapper generation. |
| src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs | Adds tests asserting merge returns a new document and throws on collisions without mutating inputs. |
| src/Refitter.Tests/MultipleOpenApiPathsTests.cs | Updates expectations to throw on duplicate paths rather than silently accepting “first wins”. |
| src/Refitter.Tests/GenerateCommandTests.cs | Adds CLI-level tests for help output and legacy boolean auth-header syntax. |
| src/Refitter.Tests/Examples/InlineJsonConvertersTests.cs | Adds coverage that inline enum-converter rewriting preserves CRLF newline style. |
| src/Refitter.SourceGenerator/RefitterSourceGenerator.cs | Switches diagnostics output to structural equality (EquatableArray) and emits user-visible diagnostics (incl. “no .refitter files found”). |
| src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs | New tests for diagnostic emission and structural equality of generator outputs. |
| src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj | Updates test dependencies to support Roslyn + EquatableArray-based assertions. |
| src/Refitter.MSBuild/RefitterGenerateTask.cs | Improves runtime discovery resiliency, adds fallback resolution, and improves timeout error messaging. |
| src/Refitter.Core/RefitGenerator.cs | Preserves original newline style when rewriting enum converter attributes. |
| src/Refitter.Core/ParameterExtractor.cs | Stops mutating operationModel.Parameters during dynamic querystring wrapper generation. |
| src/Refitter.Core/OpenApiDocumentFactory.cs | Clones base document for merge and throws on conflicting collisions instead of silently dropping/mutating. |
| README.md | Updates CLI option help text for --generate-authentication-header [STYLE] and documents legacy boolean compatibility. |
![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: 6
🧹 Nitpick comments (9)
src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs (1)
58-82: LGTM — solid end-to-end regression coverage acrossMultipleInterfacesmodes.Parameterizing across
Unset,ByTag, andByEndpointis the right call here because the dynamic-querystring code path is invoked from bothRefitInterfaceGeneratorandRefitMultipleInterfaceGenerator, and the prior mutation bug would have affected XML doc generation in all three modes. The fourShould().Contain(...)assertions cleanly fingerprint both per-parameter docs being preserved and the wrapper doc being added.Optional: per the repo guideline that new generation behavior be exercised against both OpenAPI 2.0 and 3.0, you might consider mirroring this with a Swagger 2.0 spec variant — but since this is a regression for a Core-level extraction issue rather than a new generation feature, I wouldn't block on it.
As per coding guidelines: "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/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs` around lines 58 - 82, Add a companion test exercising the same assertions against an OpenAPI 2.0 (Swagger 2.0) spec: duplicate the existing Issue1039_DynamicQuerystringGenerationPreserves_Original_Query_Param_Documentation test (or add a new test method) that uses a Swagger 2.0 spec file (e.g., create TestFile.CreateSwaggerFile(swaggerV2Spec, "issue-1039-swagger2.json")) instead of OpenApiSpec, keep UseDynamicQuerystringParameters = true and the same MultipleInterfaces paramization, and assert the identical XML doc and signature expectations (the four Should().Contain(...) checks) so generation behavior is validated for both OpenAPI 3.0 and 2.0 paths.src/Refitter.Core/OpenApiDocumentFactory.cs (1)
55-57: Sync-over-async inMergeblocks the calling task.
Mergeis invoked from the asyncCreateAsyncchain but usesGetAwaiter().GetResult()to round-trip the base document. This blocks a thread-pool thread for an otherwise async operation and trips analyzer rules in some setups. Consider makingMergeasync and awaitingOpenApiDocument.FromJsonAsyncdirectly.♻️ Proposed refactor
- return Merge(documents); + return await Merge(documents).ConfigureAwait(false); } - private static OpenApiDocument Merge(OpenApiDocument[] documents) + private static async Task<OpenApiDocument> Merge(OpenApiDocument[] documents) { - var baseDocument = OpenApiDocument.FromJsonAsync(documents[0].ToJson(documents[0].SchemaType)).GetAwaiter().GetResult(); + var baseDocument = await OpenApiDocument + .FromJsonAsync(documents[0].ToJson(documents[0].SchemaType)) + .ConfigureAwait(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.Core/OpenApiDocumentFactory.cs` around lines 55 - 57, The Merge method currently blocks on an async call by using GetAwaiter().GetResult(); change Merge to be async Task<OpenApiDocument> Merge(OpenApiDocument[] documents) and await OpenApiDocument.FromJsonAsync(...) instead of calling GetAwaiter().GetResult(), then update callers (e.g., the async CreateAsync chain that invokes Merge) to await the new Merge(...) async method so the operation remains fully asynchronous and avoids thread-pool blocking.src/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cs (1)
532-615: Coverage gap: no positive test for theAreEquivalent"equivalent value" path.The new tests cover collision-throws and non-mutation thoroughly, but there's no test asserting that adding a key whose value is equivalent (per
AreEquivalent) succeeds silently. Given thatAreEquivalenthas a JSON-string fallback that's specifically designed to allow structurally-identical merges, a positive test would lock in the intended semantics — and if the related concern aboutSerializer.Serializeis correct, it would currently fail and surface the issue.Suggestion: add a
Merge_With_Equivalent_Schema_Does_Not_Throwtest usingInvokeMergeIfMissingOrThrowOnConflictwith two distinct-but-structurally-identicalJsonSchemainstances, and aMerge_With_Equivalent_Path_Does_Not_Throwusing two documents that share the same path body. The expected behavior should match whatever the PR decides is the canonical fail-fast policy (see the related comment onMultipleOpenApiPathsTests.cs:180-188).src/Refitter.SourceGenerator/RefitterSourceGenerator.cs (2)
249-274: ManualIEquatable<T>/Equals/GetHashCodeon thisrecord structis redundant.
record structalready auto-generates structuralEquals/GetHashCodeviaEqualityComparer<T>.Defaultfor each component, andEqualityComparer<string>.Defaultis alreadyStringComparer.Ordinal, so the manual implementations don't change behavior. Dropping them removes a maintenance hazard (a future field added to the primary constructor would silently be excluded from the hand-written comparison/hash). The: IEquatable<GeneratedDiagnostic>interface declaration is also already implied for record structs.♻️ Suggested simplification
- internal readonly record struct GeneratedDiagnostic( - string Id, - string Title, - string Message, - DiagnosticSeverity Severity, - bool EnabledByDefault = true) - : IEquatable<GeneratedDiagnostic> - { - public bool Equals(GeneratedDiagnostic other) => - string.Equals(Id, other.Id, StringComparison.Ordinal) && - string.Equals(Title, other.Title, StringComparison.Ordinal) && - string.Equals(Message, other.Message, StringComparison.Ordinal) && - Severity == other.Severity && - EnabledByDefault == other.EnabledByDefault; - - public override int GetHashCode() - { - HashCode hashCode = default; - hashCode.Add(Id, StringComparer.Ordinal); - hashCode.Add(Title, StringComparer.Ordinal); - hashCode.Add(Message, StringComparer.Ordinal); - hashCode.Add((int)Severity); - hashCode.Add(EnabledByDefault); - return hashCode.ToHashCode(); - } - } + internal readonly record struct GeneratedDiagnostic( + string Id, + string Title, + string Message, + DiagnosticSeverity Severity, + bool EnabledByDefault = true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` around lines 249 - 274, The GeneratedDiagnostic record struct contains manual implementations of Equals, GetHashCode and an explicit IEquatable<GeneratedDiagnostic> declaration which are redundant; remove the custom Equals method, the GetHashCode override, and the explicit ": IEquatable<GeneratedDiagnostic>" from the GeneratedDiagnostic declaration so the compiler-generated structural equality and hashing for the record struct handle comparisons and hashing automatically (this prevents future constructor fields from being accidentally omitted).
30-33: ConsiderStringComparer.Ordinalinstead ofInvariantCultureIgnoreCasefor incremental cache stability.For
.refitterpaths used purely as a stable ordering key for incremental caching,Ordinalis slightly faster and avoids any locale-dependent edge cases. Functionally not blocking, butOrdinalis the more common choice for "I just want a deterministic, comparison-only sort" of file paths in incremental generators.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` around lines 30 - 33, The Sort call used to produce refitterPathList is using StringComparer.InvariantCultureIgnoreCase causing locale-dependent ordering; change the comparer to StringComparer.Ordinal to ensure deterministic, faster, culture-agnostic ordering for the refitterFiles -> refitterPathList pipeline (look for the refitterPathList variable and the chain using CollectAsEquatableArray / AsImmutableArray().Sort(StringComparer.InvariantCultureIgnoreCase) and replace the comparer there).src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs (3)
14-28: Test name and assertions don't fully cover the stated intent.
GeneratedCode_Equality_Should_Be_Structural_For_Diagnosticsconstructs bothleftandrightfrom the sameequatableArrayreference, so even ifEquatableArray<T>had reference-equality semantics on itsDiagnosticsfield, the assertion would still pass. To genuinely prove structural equality of the diagnostics array, build two independent equatable arrays (each with separately-allocated diagnostic instances having equal field values) and pass each into one side. As written, the test reduces to "doesrecord structsynthesizeEqualsfor non-collection fields", which is a language guarantee.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs` around lines 14 - 28, The test GeneratedCode_Equality_Should_Be_Structural_For_Diagnostics currently uses the same equatableArray reference for both instances, which doesn't verify structural equality of diagnostics; change the setup to create two independent equatable arrays (call CreateEquatableArray twice or add a CreateEquatableArrayClone helper) that allocate separate GeneratedDiagnostic instances with identical field values, then pass one into the left Activator.CreateInstance(generatedCodeType, ...) and the other into the right one so left.Equals(right) and matching GetHashCode() truly validate structural equality of the diagnostics field.
56-61: Metadata reference set is too narrow if the stub source ever grows.The compilation only references
System.Private.CoreLib(typeof object),System.Linq, andSystem.Runtime. That is sufficient for the currentStubclass, but very small additions to the parsed source (e.g., using astring,Console.WriteLine, attributes fromSystem.Runtime.InteropServices) will produce missing-reference compilation diagnostics that polluteresult.Diagnosticsand could mask theREFITTER003assertion. A common idiom is to load allBasic.Reference.Assembliesor every assembly fromAppDomain.CurrentDomain.GetAssemblies(); either is more future-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs` around lines 56 - 61, The current GetMetadataReferences returns a minimal set (using MetadataReference.CreateFromFile with typeof(object)/Enumerable/System.Runtime.GCSettings) which is too narrow and causes spurious missing-reference diagnostics if the Stub source grows; update GetMetadataReferences to return a broader, future-proof set of references—for example, load Basic.Reference.Assemblies (e.g., ReferenceAssemblies.Net60 or appropriate net version) or enumerate AppDomain.CurrentDomain.GetAssemblies() and call MetadataReference.CreateFromFile for each non-dynamic assembly—so the test compilation includes the common framework assemblies and avoids unrelated missing-reference diagnostics that could mask the REFITTER003 assertion.
11-28: Reflection-based access of internal types is brittle — preferInternalsVisibleTo.
GeneratedCodeandGeneratedDiagnosticareinternalnested types in the source generator. Accessing them viaActivator.CreateInstance+Type.GetType("…+GeneratedDiagnostic")means:
- Renames inside the generator silently break tests at runtime instead of compile time.
- The structural-equality assertion is moot for a
record structwhoseEquals/GetHashCodeare auto-generated by the compiler — you're testing the C# language, not your code.- The two
Activator.CreateInstancecalls share the sameequatableArrayinstance, so the test would also pass with reference equality on that field; consider constructing two independent equatable arrays with equal contents to actually exercise structural comparison.A simpler design: add
[InternalsVisibleTo("Refitter.SourceGenerator.Tests")]to the generator project, reference the generator project normally for tests, and write the assertions against the strongly-typedGeneratedCode/GeneratedDiagnostic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs` around lines 11 - 28, The test uses brittle reflection to construct internal nested types (Refitter.SourceGenerator.RefitterSourceGenerator+GeneratedCode and +GeneratedDiagnostic) via LoadSourceGeneratorAssembly, Type.GetType and Activator.CreateInstance and also reuses the same equatable array instance from CreateEquatableArray; change the approach to expose internals to the test assembly by adding InternalsVisibleTo("Refitter.SourceGenerator.Tests") in the source generator project, remove the reflection/Activator.CreateInstance usage in GeneratedCode_Equality_Should_Be_Structural_For_Diagnostics, reference the generator project directly from the test project, construct two independent equatable arrays with equal contents (instead of sharing the same instance) and create strongly-typed GeneratedCode/GeneratedDiagnostic instances to assert structural equality and matching hash codes.src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj (1)
17-17:Microsoft.Bcl.HashCodeis unnecessary onnet8.0;net10.0.
Microsoft.Bcl.HashCodeexists to back-portSystem.HashCodetonetstandard2.0/ .NET Framework. This test project targetsnet8.0;net10.0whereHashCodeis part of the BCL, and the new test code itself doesn't useHashCodedirectly (only the source generator does, but it lives in a separatenetstandard2.0project with its own references). This package adds nothing here and just inflates the dependency graph. Safe to drop.♻️ Proposed change
<PackageReference Include="H.Generators.Extensions" Version="1.24.2" /> - <PackageReference Include="Microsoft.Bcl.HashCode" Version="6.0.0" /> <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.12.0" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj` at line 17, Remove the unnecessary PackageReference to Microsoft.Bcl.HashCode from the test project: delete the <PackageReference Include="Microsoft.Bcl.HashCode" Version="6.0.0" /> entry in the Refitter.SourceGenerator.Tests project file because the project targets net8.0;net10.0 where System.HashCode is built into the BCL and the test code does not directly use it.
🤖 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.Core/OpenApiDocumentFactory.cs`:
- Around line 119-127: The merge currently assigns incoming OpenAPI node objects
by reference (target[key] = value in OpenApiDocumentFactory), which causes
post-merge mutations to leak into caller documents; instead deep-copy each
incoming Value before inserting (use the same JSON round-trip cloning used for
the base doc at L57 or a dedicated Clone/DeepClone helper) for the entries
handled in OpenApiDocumentFactory (paths, schemas/definitions, security schemes
and the code path that does target[key] = value) so that documents[1..] are not
aliased into the merged document.
In `@src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs`:
- Around line 63-79: The LoadSourceGeneratorAssembly method currently hard-codes
"Release" and "netstandard2.0" and uses fragile path traversal which breaks
Debug runs and different TFMs; change it to obtain the generator DLL path from a
build-injected constant or a safe ProjectReference instead: update
LoadSourceGeneratorAssembly to read a path constant (e.g., injected via an
MSBuild <ItemGroup> or <PropertyGroup> into the test assembly) that carries the
built analyzer/generator output, or remove the reflection approach and add a
ProjectReference with ReferenceOutputAssembly="true"/PrivateAssets="all" so
tests can load generator types directly; ensure the code uses that injected
value (or the referenced assembly types) instead of the hard-coded assemblyPath
string.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 160-165: CreateFileContentsDiagnostic currently emits the entire
.refitter JSON (via CreateFileContentsDiagnostic) which floods diagnostics and
leaks config; change its behavior to avoid outputting the full payload by either
gating emission behind a verbosity option, replacing the diagnostic message with
a short summary (e.g. file length and a hash or first N chars + "…"), or
downgrade the severity to Hidden/Info and remove sensitive content; update any
code that calls CreateFileContentsDiagnostic to use CreateFoundFileDiagnostic or
the new summary factory so diagnostics no longer contain the full JSON.
- Around line 146-165: The three diagnostic factory methods
CreateGeneratedSuccessfullyDiagnostic, CreateFoundFileDiagnostic, and
CreateFileContentsDiagnostic reuse the same Id ("REFITTER001"); change
CreateFoundFileDiagnostic to use a new stable id (e.g., "REFITTER100") and
CreateFileContentsDiagnostic to use another distinct stable id (e.g.,
"REFITTER101") while leaving CreateGeneratedSuccessfullyDiagnostic as
"REFITTER001", and update any related references or tests and EditorConfig
entries that expect the old ids so each diagnostic descriptor has a unique,
stable ID.
In `@src/Refitter.Tests/GenerateCommandTests.cs`:
- Around line 427-493: Extend the end-to-end test coverage by adding cases to
Program_Main_Should_Accept_Legacy_GenerateAuthenticationHeader_Boolean_Syntax
(or new tests) that (1) invoke InvokeProgram with the bare flag form
"--generate-authentication-header" (no "true" argument) and assert the same
success conditions and presence of the Authorization header in the generated
output, and (2) use a Swagger 2.0 (OpenAPI 2.0) input JSON that defines the auth
under "securityDefinitions" (e.g., a "bearerAuth" http scheme or apiKey as
appropriate) and run InvokeProgram (both with bare flag and explicit "true") and
assert ExitCode == 0, output file exists, and
File.ReadAllText(outputPath).Should().Contain("[Headers(\"Authorization:
Bearer\")]"); locate and modify the existing test method and/or add two new
tests that mirror the current OpenAPI 3.0 flow but swap the input JSON and CLI
flag forms so both compat paths are verified.
---
Nitpick comments:
In `@src/Refitter.Core/OpenApiDocumentFactory.cs`:
- Around line 55-57: The Merge method currently blocks on an async call by using
GetAwaiter().GetResult(); change Merge to be async Task<OpenApiDocument>
Merge(OpenApiDocument[] documents) and await OpenApiDocument.FromJsonAsync(...)
instead of calling GetAwaiter().GetResult(), then update callers (e.g., the
async CreateAsync chain that invokes Merge) to await the new Merge(...) async
method so the operation remains fully asynchronous and avoids thread-pool
blocking.
In `@src/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csproj`:
- Line 17: Remove the unnecessary PackageReference to Microsoft.Bcl.HashCode
from the test project: delete the <PackageReference
Include="Microsoft.Bcl.HashCode" Version="6.0.0" /> entry in the
Refitter.SourceGenerator.Tests project file because the project targets
net8.0;net10.0 where System.HashCode is built into the BCL and the test code
does not directly use it.
In `@src/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cs`:
- Around line 14-28: The test
GeneratedCode_Equality_Should_Be_Structural_For_Diagnostics currently uses the
same equatableArray reference for both instances, which doesn't verify
structural equality of diagnostics; change the setup to create two independent
equatable arrays (call CreateEquatableArray twice or add a
CreateEquatableArrayClone helper) that allocate separate GeneratedDiagnostic
instances with identical field values, then pass one into the left
Activator.CreateInstance(generatedCodeType, ...) and the other into the right
one so left.Equals(right) and matching GetHashCode() truly validate structural
equality of the diagnostics field.
- Around line 56-61: The current GetMetadataReferences returns a minimal set
(using MetadataReference.CreateFromFile with
typeof(object)/Enumerable/System.Runtime.GCSettings) which is too narrow and
causes spurious missing-reference diagnostics if the Stub source grows; update
GetMetadataReferences to return a broader, future-proof set of references—for
example, load Basic.Reference.Assemblies (e.g., ReferenceAssemblies.Net60 or
appropriate net version) or enumerate AppDomain.CurrentDomain.GetAssemblies()
and call MetadataReference.CreateFromFile for each non-dynamic assembly—so the
test compilation includes the common framework assemblies and avoids unrelated
missing-reference diagnostics that could mask the REFITTER003 assertion.
- Around line 11-28: The test uses brittle reflection to construct internal
nested types (Refitter.SourceGenerator.RefitterSourceGenerator+GeneratedCode and
+GeneratedDiagnostic) via LoadSourceGeneratorAssembly, Type.GetType and
Activator.CreateInstance and also reuses the same equatable array instance from
CreateEquatableArray; change the approach to expose internals to the test
assembly by adding InternalsVisibleTo("Refitter.SourceGenerator.Tests") in the
source generator project, remove the reflection/Activator.CreateInstance usage
in GeneratedCode_Equality_Should_Be_Structural_For_Diagnostics, reference the
generator project directly from the test project, construct two independent
equatable arrays with equal contents (instead of sharing the same instance) and
create strongly-typed GeneratedCode/GeneratedDiagnostic instances to assert
structural equality and matching hash codes.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 249-274: The GeneratedDiagnostic record struct contains manual
implementations of Equals, GetHashCode and an explicit
IEquatable<GeneratedDiagnostic> declaration which are redundant; remove the
custom Equals method, the GetHashCode override, and the explicit ":
IEquatable<GeneratedDiagnostic>" from the GeneratedDiagnostic declaration so the
compiler-generated structural equality and hashing for the record struct handle
comparisons and hashing automatically (this prevents future constructor fields
from being accidentally omitted).
- Around line 30-33: The Sort call used to produce refitterPathList is using
StringComparer.InvariantCultureIgnoreCase causing locale-dependent ordering;
change the comparer to StringComparer.Ordinal to ensure deterministic, faster,
culture-agnostic ordering for the refitterFiles -> refitterPathList pipeline
(look for the refitterPathList variable and the chain using
CollectAsEquatableArray /
AsImmutableArray().Sort(StringComparer.InvariantCultureIgnoreCase) and replace
the comparer there).
In
`@src/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cs`:
- Around line 58-82: Add a companion test exercising the same assertions against
an OpenAPI 2.0 (Swagger 2.0) spec: duplicate the existing
Issue1039_DynamicQuerystringGenerationPreserves_Original_Query_Param_Documentation
test (or add a new test method) that uses a Swagger 2.0 spec file (e.g., create
TestFile.CreateSwaggerFile(swaggerV2Spec, "issue-1039-swagger2.json")) instead
of OpenApiSpec, keep UseDynamicQuerystringParameters = true and the same
MultipleInterfaces paramization, and assert the identical XML doc and signature
expectations (the four Should().Contain(...) checks) so generation behavior is
validated for both OpenAPI 3.0 and 2.0 paths.
🪄 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: 457358bf-6a29-432e-a2aa-c1488255046d
📒 Files selected for processing (19)
README.mdsrc/Refitter.Core/OpenApiDocumentFactory.cssrc/Refitter.Core/ParameterExtractor.cssrc/Refitter.Core/RefitGenerator.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator.Tests/Refitter.SourceGenerator.Tests.csprojsrc/Refitter.SourceGenerator.Tests/SourceGeneratorDiagnosticsTests.cssrc/Refitter.SourceGenerator/RefitterSourceGenerator.cssrc/Refitter.Tests/Examples/InlineJsonConvertersTests.cssrc/Refitter.Tests/GenerateCommandTests.cssrc/Refitter.Tests/MultipleOpenApiPathsTests.cssrc/Refitter.Tests/OpenApiDocumentFactoryMergeTests.cssrc/Refitter.Tests/ParameterExtractorPrivateCoverageTests.cssrc/Refitter.Tests/RefitterGenerateTaskTests.cssrc/Refitter.Tests/RegressionTests/Issue1039_DynamicQuerystringMutationTests.cssrc/Refitter.Tests/SettingsTests.cssrc/Refitter/GenerateCommand.cssrc/Refitter/Settings.cssrc/Refitter/SettingsValidator.cs
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 Dallas/Lambert Linux help-output handoff, merge the semantic-help decision details, and summarize Dallas history. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1070 +/- ##
==========================================
+ Coverage 95.20% 95.38% +0.17%
==========================================
Files 27 27
Lines 2359 2428 +69
==========================================
+ Hits 2246 2316 +70
+ Misses 40 39 -1
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
🧹 Nitpick comments (5)
.squad/decisions.md (1)
40-57: Consider reducing repetitive “Treat …” bullet openings for readability.This section is clear functionally, but Line 40–57 repeats the same sentence start across many bullets, which reduces scanability in long audit logs. Grouping by status label (
still reproducible,partial,validation-only, etc.) would read cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.squad/decisions.md around lines 40 - 57, The repeated "Treat **#XXXX** as ..." bullet openings in the "Remaining Audit Repro Pass (`#1057`)" section make the audit hard to scan; refactor lines 40–57 by grouping issues under status headings (e.g., "Still reproducible", "Partial", "Validation-only / Fixed-at-HEAD", "Not reproduced") and list the issue numbers and short notes after each heading (preserve the original notes for `#1028`, `#1029`, `#1033`, `#1041`, `#1043`, `#1032/`#1042/#1045/#1047, and `#1034/`#1039/#1056) so the content is unchanged but the repetitive lead phrase is removed for better readability..squad/agents/ash/history.md (2)
11-15: Documentation accurately captures technical learnings.The updated learnings section clearly documents key design decisions around collision detection, deduplication, packaging, and nullable handling. The content aligns well with the PR objectives and referenced code changes.
Optional: Consider minor style polish
If you'd like to reduce repetition, you could vary the sentence structure slightly:
- Added to the squad on 2026-04-20 as a specialist reviewer for PR `#1064` / `#1057` safety gates. -- For suffix rewrites, block both source-name corruption and suffix-target collisions (Pet + PetDto cannot both land on PetDto). -- For multipart/query parameter extraction, deduplicate by the emitted C# identifier, not the original OpenAPI key. -- For source-generator packaging reviews, inspect the packed .nuspec and analyzer payload, not just the .csproj. +- Suffix rewrites must block both source-name corruption and suffix-target collisions (Pet + PetDto cannot both land on PetDto). +- Multipart/query parameter extraction should deduplicate by the emitted C# identifier, not the original OpenAPI key. +- Source-generator packaging reviews should inspect the packed .nuspec and analyzer payload, not just the .csproj. - The minimal safe Swagger 2 nullable-shape fix for `#1026` lives in src/Refitter.Core/RefitGenerator.cs as targeted post-processing when NRT is enabled but optional-nullable generation stays opt-in.This addresses the stylistic repetition flagged by static analysis, but it's entirely optional for documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.squad/agents/ash/history.md around lines 11 - 15, The history entry shows repetitive sentence starts and can be polished for style; edit .squad/agents/ash/history.md to vary sentence structure and reduce repetition across the bullet points (e.g., rephrase the three "For ..." lines into a mix of noun phrases and clauses, or combine related items like suffix rewrites and multipart/query deduplication), keep the same technical content and references (PR `#1064/`#1057, `#1026`, and src/Refitter.Core/RefitGenerator.cs) but improve flow and brevity.
31-36: Final signoff comprehensively documents approval.The final signoff section provides excellent evidence-based closure, listing specific files reviewed and concrete test results. The technical summary accurately captures the key invariants (clone-first, fail-fast on collisions, non-mutating extraction).
Optional: Consider varying sentence structure
If you'd like to address the stylistic repetition flagged by static analysis:
- Approved the final `#1034` / `#1039` follow-up after Ripley preserved the source schema type during clone and isolated the Swagger 2 definition-collision proof through MergeIfMissingOrThrowOnConflict(...). - Signed off that merge handling now stays clone-first, fails fast on conflicting duplicate path/schema/definition/security keys, and keeps grouped dynamic-query extraction non-mutating across single-interface, ByTag, and ByEndpoint generation. -- Evidence reviewed included src\Refitter.Core\OpenApiDocumentFactory.cs, src\Refitter.Tests\OpenApiDocumentFactoryMergeTests.cs, src\Refitter.Tests\RegressionTests\Issue1039_DynamicQuerystringMutationTests.cs, and src\Refitter.Tests\ParameterExtractorPrivateCoverageTests.cs. +- The evidence reviewed spanned src\Refitter.Core\OpenApiDocumentFactory.cs, src\Refitter.Tests\OpenApiDocumentFactoryMergeTests.cs, src\Refitter.Tests\RegressionTests\Issue1039_DynamicQuerystringMutationTests.cs, and src\Refitter.Tests\ParameterExtractorPrivateCoverageTests.cs. - Final reviewer gate was reported green on dotnet test -c Release src\Refitter.Tests\Refitter.Tests.csproj with 1840 passing and 0 failing.This is entirely optional for documentation purposes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.squad/agents/ash/history.md around lines 31 - 36, The "Final signoff" paragraph under the header "## 2026-04-25: Final Signoff on Ripley Follow-up" reads well but repeats similar sentence structures; revise that section and the optional details block to vary sentence starts and cadence (shorter/longer sentences, combine or split clauses) while preserving the same facts (files reviewed, tests passing, invariants) and keeping the [approve_code_changes] token intact; no logic changes required—just edit wording for stylistic variety in the final signoff and the <details> note..squad/agents/lambert/history.md (2)
31-34: Consider adding context for agent references.The documentation references multiple agents (Ash, Dallas, Ripley) without providing context about their roles or responsibilities. For future maintainability, consider adding a brief explanation of the squad structure or linking to a team roster.
Example addition to the "Context" section:
## Context - User: Christian Helle - Product: Refitter generates C# REST API clients from OpenAPI specifications using Refit. - Stack: .NET, Refit, NSwag, Source Generator, MSBuild, Microsoft OpenAPI.NET + - Squad: Lambert (audit/testing), Ash (review/rejection), Dallas (analysis), Ripley (fixes)🤖 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 31 - 34, The history references agents (Ash, Dallas, Ripley, Lambert) without explaining roles; update .squad/agents/lambert/history.md by adding a short "Context" or "Squad" subsection that lists each agent with a one-line role (e.g., "Lambert: audit/testing", "Ash: review/rejection", "Dallas: analysis", "Ripley: fixes") or link to the team roster, and then keep the existing narrative unchanged so readers understand who performed each action; ensure the new section appears near the top of the file (e.g., next to existing Context) for discoverability.
12-14: Consider using forward slashes for paths in documentation.The file paths use Windows-style backslashes (e.g.,
src\Refitter\Program.cs). For better cross-platform readability in markdown documentation, consider using forward slashes (src/Refitter/Program.cs) or wrapping paths in backticks.📝 Example with forward slashes
-- **2026-04-25 CLI help repro:** src\Refitter\Program.cs intentionally rewrites a no-argument invocation to --help, exits 0, and emits Spectre.Console.Cli help output. Tests in src\Refitter.Tests\GenerateCommandTests.cs should assert semantic help markers like usage, sections, and option names rather than exact formatter-driven spacing. -+ **2026-04-25 CLI help repro:** src/Refitter/Program.cs intentionally rewrites a no-argument invocation to --help, exits 0, and emits Spectre.Console.Cli help output. Tests in src/Refitter.Tests/GenerateCommandTests.cs should assert semantic help markers like usage, sections, and option names rather than exact formatter-driven spacing.🤖 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 12 - 14, Summary: Documentation uses Windows-style backslashes in paths; convert them to forward slashes and/or wrap paths in code ticks for cross-platform readability. Fix: update occurrences like "src\Refitter\Program.cs", "src\Refitter.Tests\GenerateCommandTests.cs", and any other backslash paths in .squad/agents/lambert/history.md to use forward slashes (e.g., src/Refitter/Program.cs) or wrap them in backticks (`src/Refitter/Program.cs`) so the markdown renders consistently across platforms and preserves literal paths; ensure examples and inline mentions (e.g., USAGE, Spectre.Console.Cli, GenerateCommandTests) remain unchanged except for path formatting.
🤖 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/parker/history.md:
- Line 413: The duplicate markdown heading "### 2026-04-25: Core Artifact
Lockout Still In Force" appears twice (the second at the shown diff);
disambiguate the second occurrence by editing that heading text (for example
append " (Lambert proof rejection)" or another brief qualifier) so it is unique
and resolves MD024 while preserving the audit log semantics; update the second
heading string exactly where "### 2026-04-25: Core Artifact Lockout Still In
Force" appears.
In @.squad/decisions-archive.md:
- Line 774: There are duplicate top-level headings "## 2026-04-20" in this
document which will trip markdownlint and create ambiguous anchors; update one
of the duplicate headings (the second occurrence of "## 2026-04-20") to be
unique—either change its text to a more specific date/descriptor or demote it to
a subheading (e.g., "### 2026-04-20" or "## 2026-04-20 — followup") so anchors
and TOC entries are distinct.
In `@src/Refitter.Tests/GenerateCommandTests.cs`:
- Around line 514-521: The current test reads process.StandardOutput.ReadToEnd()
then process.StandardError.ReadToEnd() and calls process.WaitForExit() with no
timeout, which can deadlock; update the helper that starts the process (the code
using Process.Start, StandardOutput.ReadToEnd, StandardError.ReadToEnd, and
WaitForExit) to read both streams concurrently (e.g., via async ReadToEndAsync
or BeginOutputReadLine/BeginErrorReadLine with events) and use a bounded timeout
for WaitForExit (or Task.WhenAny with a timeout) so the test fails
deterministically instead of hanging; ensure you still capture and return the
combined stdout+stderr and the ExitCode when the process completes or when the
timeout triggers.
---
Nitpick comments:
In @.squad/agents/ash/history.md:
- Around line 11-15: The history entry shows repetitive sentence starts and can
be polished for style; edit .squad/agents/ash/history.md to vary sentence
structure and reduce repetition across the bullet points (e.g., rephrase the
three "For ..." lines into a mix of noun phrases and clauses, or combine related
items like suffix rewrites and multipart/query deduplication), keep the same
technical content and references (PR `#1064/`#1057, `#1026`, and
src/Refitter.Core/RefitGenerator.cs) but improve flow and brevity.
- Around line 31-36: The "Final signoff" paragraph under the header "##
2026-04-25: Final Signoff on Ripley Follow-up" reads well but repeats similar
sentence structures; revise that section and the optional details block to vary
sentence starts and cadence (shorter/longer sentences, combine or split clauses)
while preserving the same facts (files reviewed, tests passing, invariants) and
keeping the [approve_code_changes] token intact; no logic changes required—just
edit wording for stylistic variety in the final signoff and the <details> note.
In @.squad/agents/lambert/history.md:
- Around line 31-34: The history references agents (Ash, Dallas, Ripley,
Lambert) without explaining roles; update .squad/agents/lambert/history.md by
adding a short "Context" or "Squad" subsection that lists each agent with a
one-line role (e.g., "Lambert: audit/testing", "Ash: review/rejection", "Dallas:
analysis", "Ripley: fixes") or link to the team roster, and then keep the
existing narrative unchanged so readers understand who performed each action;
ensure the new section appears near the top of the file (e.g., next to existing
Context) for discoverability.
- Around line 12-14: Summary: Documentation uses Windows-style backslashes in
paths; convert them to forward slashes and/or wrap paths in code ticks for
cross-platform readability. Fix: update occurrences like
"src\Refitter\Program.cs", "src\Refitter.Tests\GenerateCommandTests.cs", and any
other backslash paths in .squad/agents/lambert/history.md to use forward slashes
(e.g., src/Refitter/Program.cs) or wrap them in backticks
(`src/Refitter/Program.cs`) so the markdown renders consistently across
platforms and preserves literal paths; ensure examples and inline mentions
(e.g., USAGE, Spectre.Console.Cli, GenerateCommandTests) remain unchanged except
for path formatting.
In @.squad/decisions.md:
- Around line 40-57: The repeated "Treat **#XXXX** as ..." bullet openings in
the "Remaining Audit Repro Pass (`#1057`)" section make the audit hard to scan;
refactor lines 40–57 by grouping issues under status headings (e.g., "Still
reproducible", "Partial", "Validation-only / Fixed-at-HEAD", "Not reproduced")
and list the issue numbers and short notes after each heading (preserve the
original notes for `#1028`, `#1029`, `#1033`, `#1041`, `#1043`, `#1032/`#1042/#1045/#1047,
and `#1034/`#1039/#1056) so the content is unchanged but the repetitive lead
phrase is removed for better readability.
🪄 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: 0eea6ffd-f21b-4301-b959-d7816bd181e1
📒 Files selected for processing (13)
.squad/agents/ash/history.md.squad/agents/dallas/history.md.squad/agents/lambert/history.md.squad/agents/parker/history.md.squad/agents/ripley/history.md.squad/agents/scribe/history.md.squad/decisions-archive.md.squad/decisions.md.squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md.squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md.squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md.squad/skills/spectre-cli-help-assertions/SKILL.mdsrc/Refitter.Tests/GenerateCommandTests.cs
✅ Files skipped from review due to trivial changes (7)
- .squad/log/2026-04-25T15-59-12Z-linux-help-output-consolidation.md
- .squad/agents/scribe/history.md
- .squad/skills/spectre-cli-help-assertions/SKILL.md
- .squad/orchestration-log/2026-04-25T15-59-12Z-lambert-linux-help-test.md
- .squad/orchestration-log/2026-04-25T15-59-12Z-dallas-linux-help-analysis.md
- .squad/agents/ripley/history.md
- .squad/agents/dallas/history.md
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Record Dallas and Lambert coverage closure state in squad logs. 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>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.squad/agents/dallas/history.md (1)
21-21: Avoid machine-specific cache paths in shared guidance.Line 21 hard-codes a local Windows path. Prefer a repo-relative or env-var based wording so contributors on different hosts can follow the same instruction reliably.
Suggested wording update
-- **Reliable validation patterns:** when the shared NuGet cache is locked, prefer the repo-local cache at `C:\projects\christianhelle\refitter\.nuget\packages`, then validate with targeted tests, packed artifacts, and formatter/build gates. +- **Reliable validation patterns:** when the shared NuGet cache is locked, prefer a repo-local cache (for example, `<repo>/.nuget/packages`), then validate with targeted tests, packed artifacts, and formatter/build gates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.squad/agents/dallas/history.md at line 21, The guidance line that hard-codes the Windows path "C:\projects\christianhelle\refitter\.nuget\packages" should be replaced with a repo-relative or environment-variable based instruction; update the "Reliable validation patterns" sentence to suggest using a repo-local cache like "./.nuget/packages" or respect the NUGET_PACKAGES env var (or instruct contributors to set a project-local NUGET_PACKAGES) and then validate with targeted tests, packed artifacts, and formatter/build gates so the guidance is host-agnostic.src/Refitter.SourceGenerator/RefitterSourceGenerator.cs (2)
31-43: Sort + collect chain is overkill for an emptiness check; considerOrdinalfor path comparisons.
refitterPathListis consumed solely bypaths.IsEmpty, so theAsImmutableArray().Sort(...).AsEquatableArray()step is wasted work on every change to the additional-files set (and grows linearly with the number of.refitterfiles). Additionally,StringComparer.InvariantCultureIgnoreCaseis the wrong tool for filesystem paths —OrdinalIgnoreCaseis the conventional choice and avoids culture/Turkic-I edge cases. Either way, sorting isn't needed if the only signal you derive is "any vs none".♻️ Simpler equivalent that only carries the boolean signal
- // collect and sort the paths of the .refitter files for logging - var refitterPathList = refitterFiles - .Select((t, _) => t.Path) - .CollectAsEquatableArray() - .Select((arr, _) => arr.AsImmutableArray().Sort(StringComparer.InvariantCultureIgnoreCase).AsEquatableArray()); - - // add a source output that warns when no .refitter files were found - context.RegisterSourceOutput(refitterPathList, static (spc, paths) => - { - if (paths.IsEmpty) - { - spc.ReportDiagnostic(CreateDiagnostic(CreateNoRefitterFilesFoundDiagnostic())); - } - }); + // warn when no .refitter files were found + var hasRefitterFiles = refitterFiles + .Collect() + .Select(static (arr, _) => !arr.IsDefaultOrEmpty); + + context.RegisterSourceOutput(hasRefitterFiles, static (spc, hasFiles) => + { + if (!hasFiles) + { + spc.ReportDiagnostic(CreateDiagnostic(CreateNoRefitterFilesFoundDiagnostic())); + } + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` around lines 31 - 43, refitterPathList is doing expensive collect+sort work only to check emptiness; replace that chain by computing a simple boolean (e.g., hasRefitterFiles = refitterFiles.Any() or !refitterFiles.IsEmpty) and pass that into context.RegisterSourceOutput instead of the heavy EquatableArray, then inside the registered callback check the boolean to call spc.ReportDiagnostic(CreateDiagnostic(CreateNoRefitterFilesFoundDiagnostic())). If you later need to compare paths, prefer StringComparer.OrdinalIgnoreCase over InvariantCultureIgnoreCase; update references to refitterPathList, refitterFiles, context.RegisterSourceOutput, CreateNoRefitterFilesFoundDiagnostic and spc.ReportDiagnostic accordingly.
182-191: RefactorCreateDiagnosticto reuse staticDiagnosticDescriptorinstances instead of allocating fresh ones per call.Roslyn's convention is one static
DiagnosticDescriptorper diagnostic ID, withMessageFormatas a stable template and dynamic content passed asmessageArgstoDiagnostic.Create. The current approach of building the descriptor inline on every call:
- allocates a new descriptor and
LocalizableStringwrappers for every diagnostic,- sets
MessageFormatto the already-substituted message, which prevents analyzer release-tracking tooling from recognizing a stable format string per ID,- creates a latent bug: REFITTER005 passes JSON containing
{and}characters as the message. If any caller adds format args, callingDiagnostic.GetMessage()would throwFormatException.Define one static readonly
DiagnosticDescriptorper ID and pass the dynamic content throughmessageArgs. This follows official Roslyn guidance (Microsoft Learn, Roslyn wiki, roslyn-analyzers) and improves correctness, performance, and IDE compatibility with EditorConfig severity overrides.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs` around lines 182 - 191, The CreateDiagnostic method currently constructs a new DiagnosticDescriptor each call which allocates and embeds the fully-formatted message; change this to use one static readonly DiagnosticDescriptor per diagnostic Id (create descriptors keyed by GeneratedDiagnostic.Id as static fields or a static dictionary) and call Diagnostic.Create(descriptor, Location.None, messageArgs: new object[] { /* dynamic content from GeneratedDiagnostic e.g. diagnostic.MessageArguments or diagnostic.Message */ }) so that MessageFormat stays a stable template and dynamic content is passed via messageArgs; update any callers or the GeneratedDiagnostic shape to expose template vs arguments (or split Message into format+args) and ensure CreateDiagnostic uses the prebuilt descriptor for descriptor.Title, descriptor.MessageFormat and Descriptor.Severity mapping from the existing GeneratedDiagnostic values.
🤖 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/decisions.md:
- Around line 40-57: The repeated "Treat …" sentence openings in the bulleted
block for issue IDs (`#1042`, `#1047`, `#1056`, `#1032`, `#1045`, `#1047`, `#1028`, `#1029`,
`#1033`, `#1041`, `#1043`, `#1034`, `#1039`, `#1056`) make the matrix hard to scan; rewrite
the bullets to vary sentence starts and condense semantics by leading with the
status then the issue (e.g., "Validation-only: `#1042`; Fixed/stale: `#1047`;
Doc/invariant-only: `#1056`; Validation-first: `#1032`"), preserve exact status
semantics and existing phrasing for any coordination notes (e.g., "Coordination
note… Dallas/Parker") and keep the "Remaining Audit Repro Pass (`#1057`)" section
header and verifier metadata unchanged.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 140-145: The diagnostic created by
CreateNoRefitterFilesFoundDiagnostic currently uses DiagnosticSeverity.Warning
for "REFITTER003", which can break builds under TreatWarningsAsErrors; change
the severity to DiagnosticSeverity.Info (or make it configurable via an analyzer
config option/property) and/or guard emission so it only runs for projects that
explicitly opt into Refitter (e.g., check for a project-level opt-in flag in
additional files or AnalyzerConfigOptions before producing the REFITTER003
diagnostic); update CreateNoRefitterFilesFoundDiagnostic and the location where
it's invoked to respect the new severity/config option and only emit when
opted-in.
---
Nitpick comments:
In @.squad/agents/dallas/history.md:
- Line 21: The guidance line that hard-codes the Windows path
"C:\projects\christianhelle\refitter\.nuget\packages" should be replaced with a
repo-relative or environment-variable based instruction; update the "Reliable
validation patterns" sentence to suggest using a repo-local cache like
"./.nuget/packages" or respect the NUGET_PACKAGES env var (or instruct
contributors to set a project-local NUGET_PACKAGES) and then validate with
targeted tests, packed artifacts, and formatter/build gates so the guidance is
host-agnostic.
In `@src/Refitter.SourceGenerator/RefitterSourceGenerator.cs`:
- Around line 31-43: refitterPathList is doing expensive collect+sort work only
to check emptiness; replace that chain by computing a simple boolean (e.g.,
hasRefitterFiles = refitterFiles.Any() or !refitterFiles.IsEmpty) and pass that
into context.RegisterSourceOutput instead of the heavy EquatableArray, then
inside the registered callback check the boolean to call
spc.ReportDiagnostic(CreateDiagnostic(CreateNoRefitterFilesFoundDiagnostic())).
If you later need to compare paths, prefer StringComparer.OrdinalIgnoreCase over
InvariantCultureIgnoreCase; update references to refitterPathList,
refitterFiles, context.RegisterSourceOutput,
CreateNoRefitterFilesFoundDiagnostic and spc.ReportDiagnostic accordingly.
- Around line 182-191: The CreateDiagnostic method currently constructs a new
DiagnosticDescriptor each call which allocates and embeds the fully-formatted
message; change this to use one static readonly DiagnosticDescriptor per
diagnostic Id (create descriptors keyed by GeneratedDiagnostic.Id as static
fields or a static dictionary) and call Diagnostic.Create(descriptor,
Location.None, messageArgs: new object[] { /* dynamic content from
GeneratedDiagnostic e.g. diagnostic.MessageArguments or diagnostic.Message */ })
so that MessageFormat stays a stable template and dynamic content is passed via
messageArgs; update any callers or the GeneratedDiagnostic shape to expose
template vs arguments (or split Message into format+args) and ensure
CreateDiagnostic uses the prebuilt descriptor for descriptor.Title,
descriptor.MessageFormat and Descriptor.Severity mapping from the existing
GeneratedDiagnostic values.
🪄 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: 6c453322-6016-4e60-9531-f08937cb024a
📒 Files selected for processing (8)
.squad/agents/dallas/history.md.squad/agents/lambert/history.md.squad/agents/scribe/history.md.squad/decisions.mdsrc/Refitter.Core/ParameterExtractor.cssrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.SourceGenerator/RefitterSourceGenerator.cssrc/Refitter.Tests/RefitterGenerateTaskTests.cs
✅ Files skipped from review due to trivial changes (3)
- .squad/agents/lambert/history.md
- .squad/agents/scribe/history.md
- src/Refitter.MSBuild/RefitterGenerateTask.cs
|
This was referenced Apr 28, 2026
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 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
.refitterinputs clearly--generate-authentication-headerLinked issues
Related to #1057
Closes #1028
Closes #1029
Closes #1033
Closes #1034
Closes #1039
Closes #1041
Closes #1043
Validation
dotnet build -c Release src\Refitter.slnxdotnet test -c Release --solution src\Refitter.slnxdotnet format --verify-no-changes src\Refitter.slnxNot auto-closed from this branch
Summary by CodeRabbit
New Features
--generate-authentication-headeraccepts an optional style (Method, Parameter, None) while still honoring legacy boolean forms.Bug Fixes
Documentation
Tests