fix: resolve failing "👌 Verify build" CI job by providing Refitter.Core as a local NuGet package#1152
Closed
christianhelle wants to merge 5 commits into
Closed
fix: resolve failing "👌 Verify build" CI job by providing Refitter.Core as a local NuGet package#1152christianhelle wants to merge 5 commits into
christianhelle wants to merge 5 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces an ChangesIGeneratorRunner abstraction and MSBuild task refactor
Sequence Diagram(s)sequenceDiagram
participant MSBuild
participant RefitterGenerateTask
participant CreateDefaultRunner
participant CliGeneratorRunner
participant CoreGeneratorRunner
participant dotnetProcess as dotnet Process
participant RefitGenerator
MSBuild->>RefitterGenerateTask: Execute()
RefitterGenerateTask->>CreateDefaultRunner: GeneratorRunner ?? CreateDefaultRunner()
CreateDefaultRunner->>CreateDefaultRunner: ResolveRefitterDllForTask()
alt refitter.dll found
CreateDefaultRunner-->>RefitterGenerateTask: CliGeneratorRunner
RefitterGenerateTask->>CliGeneratorRunner: RunAsync(settings, flags, ct)
CliGeneratorRunner->>dotnetProcess: Start(dotnet refitter.dll args)
dotnetProcess-->>CliGeneratorRunner: stdout/stderr lines
CliGeneratorRunner-->>RefitterGenerateTask: generated file paths
else refitter.dll not found
CreateDefaultRunner-->>RefitterGenerateTask: CoreGeneratorRunner
RefitterGenerateTask->>CoreGeneratorRunner: RunAsync(settings, flags, ct)
CoreGeneratorRunner->>RefitGenerator: Generate(settings)
RefitGenerator-->>CoreGeneratorRunner: generated code
CoreGeneratorRunner-->>RefitterGenerateTask: written file paths
end
RefitterGenerateTask-->>MSBuild: GeneratedFiles / success or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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 |
christianhelle
changed the title
christianhelle
added
enhancement
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs (1)
291-375: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftUpdate this new OpenAPI scenario test to follow required test helpers and matrix coverage.
This test currently uses hand-written JSON and only validates one OpenAPI 3.0 case. Please align it with project test conventions by using
SwaggerFileHelper.CreateSwaggerJsonFile(), adding an OpenAPI 2.0 counterpart, and asserting generated code compilation viaBuildHelper.BuildCSharp(..., targetFramework: "net8.0"|"net9.0"|"net10.0").As per coding guidelines, "Use
SwaggerFileHelper.CreateSwaggerFile()for YAML temp files andCreateSwaggerJsonFile()for JSON in tests", "UseBuildHelper.BuildCSharp()in tests to verify generated code compiles, supportingnet8.0,net9.0, andnet10.0via the targetFramework parameter", and "Test with both OpenAPI 2.0 and 3.0 specifications when working with OpenAPI specifications".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs` around lines 291 - 375, The test method Execute_Should_Use_CoreGeneratorRunner_When_No_Cli_Available() needs to be refactored to follow project conventions. Replace the hand-written JSON schema creation with SwaggerFileHelper.CreateSwaggerJsonFile() for consistency. Create a second test or parameterize the existing test to cover OpenAPI 2.0 specification in addition to OpenAPI 3.0. Add verification that the generated code compiles by calling BuildHelper.BuildCSharp() after the code generation with targetFramework parameter supporting "net8.0", "net9.0", and "net10.0". This ensures the test validates both proper code generation and successful compilation across multiple target frameworks.Source: Coding guidelines
🧹 Nitpick comments (1)
src/Refitter.Core/TestGeneratorRunner.cs (1)
9-9: ⚡ Quick winRename
_handlerto follow repository private-field naming.Use
handler(camelCase, no underscore prefix) for the private field to match project conventions.♻️ Proposed fix
- private readonly Func<RefitGeneratorSettings, bool, bool, CancellationToken, Task<IReadOnlyList<string>>>? _handler; + private readonly Func<RefitGeneratorSettings, bool, bool, CancellationToken, Task<IReadOnlyList<string>>>? handler; @@ - _handler = handler; + this.handler = handler; @@ - if (_handler is not null) + if (handler is not null) { - return await _handler(settings, skipValidation, noLogging, cancellationToken).ConfigureAwait(false); + return await handler(settings, skipValidation, noLogging, cancellationToken).ConfigureAwait(false); }As per coding guidelines, "Private fields must use camelCase without underscore prefix (e.g., openApiPath not _openApiPath)".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Refitter.Core/TestGeneratorRunner.cs` at line 9, The private field `_handler` uses an underscore prefix, which violates the repository's naming convention for private fields. Rename the field `_handler` to `handler` (camelCase without underscore prefix) throughout the TestGeneratorRunner class, including all references to this field such as constructor assignments and any usages within methods.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Refitter.Core/CliGeneratorRunner.cs`:
- Around line 30-38: The CLI arguments are being built as a single interpolated
string by concatenating values like refitterDll and settings.OpenApiPath
directly into the args variable, which fails when these paths contain spaces or
quotes since the subprocess cannot parse them correctly. Instead of building
args as a string, construct the arguments as a list or array of tokens
(individual argument strings), then pass that collection to the process
execution method. This ensures proper handling of special characters and spaces
in file paths. Apply this change at the main location where args is initially
set and also at the second location mentioned in the review (lines 45-48) where
additional arguments are conditionally appended.
In `@src/Refitter.Core/CoreGeneratorRunner.cs`:
- Around line 63-66: The GetContractsOutputPath method resolves
ContractsOutputFolder relative to the process working directory instead of
anchoring to the settings file root, causing misplaced file generation. Fix the
GetContractsOutputPath method to resolve relative paths from the directory
containing the settings file, ensuring ContractsOutputFolder is always
interpreted relative to the settings file's location regardless of the current
working directory. Verify this same issue and correction applies at the other
affected location around lines 104-107 where similar path resolution occurs.
In `@src/Refitter.MSBuild/Refitter.MSBuild.csproj`:
- Around line 22-24: The Refitter.MSBuild.csproj project is a production project
that currently lacks warning-as-error enforcement. Add a PropertyGroup element
to the project file that sets TreatWarningsAsErrors to true, ensuring that all
compiler warnings are treated as errors during the build process. This will
prevent silent regressions and maintain code quality standards across the
production codebase.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs`:
- Around line 95-107: The CreateDefaultRunner() method selects a bundled
refitter DLL without verifying that the host machine has the required runtime
installed, causing failures when the first available binary by TFM order
(net10.0, net9.0, net8.0) is not compatible with the host. Modify the
ResolveRefitterDllForTask method or the logic in CreateDefaultRunner to check
host runtime compatibility before returning a refitter DLL path. Ensure that
only a binary whose target runtime is available on the host is selected, and if
no compatible binary exists, fall back to the CoreGeneratorRunner as a safe
default rather than failing with an unusable CLI binary.
---
Outside diff comments:
In `@src/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs`:
- Around line 291-375: The test method
Execute_Should_Use_CoreGeneratorRunner_When_No_Cli_Available() needs to be
refactored to follow project conventions. Replace the hand-written JSON schema
creation with SwaggerFileHelper.CreateSwaggerJsonFile() for consistency. Create
a second test or parameterize the existing test to cover OpenAPI 2.0
specification in addition to OpenAPI 3.0. Add verification that the generated
code compiles by calling BuildHelper.BuildCSharp() after the code generation
with targetFramework parameter supporting "net8.0", "net9.0", and "net10.0".
This ensures the test validates both proper code generation and successful
compilation across multiple target frameworks.
---
Nitpick comments:
In `@src/Refitter.Core/TestGeneratorRunner.cs`:
- Line 9: The private field `_handler` uses an underscore prefix, which violates
the repository's naming convention for private fields. Rename the field
`_handler` to `handler` (camelCase without underscore prefix) throughout the
TestGeneratorRunner class, including all references to this field such as
constructor assignments and any usages within methods.
🪄 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: 8ccb288f-cb83-401b-9187-7cbbdcfec7de
📒 Files selected for processing (8)
src/Refitter.Core/CliGeneratorRunner.cssrc/Refitter.Core/CoreGeneratorRunner.cssrc/Refitter.Core/IGeneratorRunner.cssrc/Refitter.Core/OutputPlanner.cssrc/Refitter.Core/TestGeneratorRunner.cssrc/Refitter.MSBuild/Refitter.MSBuild.csprojsrc/Refitter.MSBuild/RefitterGenerateTask.cssrc/Refitter.Tests/MSBuild/RefitterGenerateTaskTests.cs
Comment on lines
+30
to
+38
| var args = $"{refitterDll} --settings-file \"{settings.OpenApiPath}\" --simple-output"; | ||
| if (noLogging) | ||
| { | ||
| args += " --no-logging"; | ||
| } | ||
| if (skipValidation) | ||
| { | ||
| args += " --skip-validation"; | ||
| } |
Contributor
There was a problem hiding this comment.
Build CLI arguments as tokens, not a single interpolated string.
The current argument composition can break when refitterDll or settings paths include spaces/quotes, causing the subprocess to parse arguments incorrectly and fail generation.
🐛 Proposed fix
- var args = $"{refitterDll} --settings-file \"{settings.OpenApiPath}\" --simple-output";
- if (noLogging)
- {
- args += " --no-logging";
- }
- if (skipValidation)
- {
- args += " --skip-validation";
- }
+ ProcessStartInfo startInfo = new()
+ {
+ FileName = "dotnet",
+ WorkingDirectory = processDirectory,
+ RedirectStandardOutput = true,
+ RedirectStandardError = true,
+ RedirectStandardInput = true,
+ UseShellExecute = false,
+ CreateNoWindow = true,
+ };
+ startInfo.ArgumentList.Add(refitterDll);
+ startInfo.ArgumentList.Add("--settings-file");
+ startInfo.ArgumentList.Add(settings.OpenApiPath);
+ startInfo.ArgumentList.Add("--simple-output");
+ if (noLogging) startInfo.ArgumentList.Add("--no-logging");
+ if (skipValidation) startInfo.ArgumentList.Add("--skip-validation");
@@
- StartInfo = new ProcessStartInfo
- {
- FileName = "dotnet",
- Arguments = args,
- WorkingDirectory = processDirectory,
- RedirectStandardOutput = true,
- RedirectStandardError = true,
- RedirectStandardInput = true,
- UseShellExecute = false,
- CreateNoWindow = true,
- }
+ StartInfo = startInfoAlso applies to: 45-48
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Refitter.Core/CliGeneratorRunner.cs` around lines 30 - 38, The CLI
arguments are being built as a single interpolated string by concatenating
values like refitterDll and settings.OpenApiPath directly into the args
variable, which fails when these paths contain spaces or quotes since the
subprocess cannot parse them correctly. Instead of building args as a string,
construct the arguments as a list or array of tokens (individual argument
strings), then pass that collection to the process execution method. This
ensures proper handling of special characters and spaces in file paths. Apply
this change at the main location where args is initially set and also at the
second location mentioned in the review (lines 45-48) where additional arguments
are conditionally appended.
Comment on lines
+63
to
+66
| if (OutputPlanner.ShouldRerouteToContractsFolder(settings, outputFile)) | ||
| { | ||
| outputPath = GetContractsOutputPath(settings, outputFile); | ||
| } |
Contributor
There was a problem hiding this comment.
Contracts output path resolution can write files to the wrong directory.
GetContractsOutputPath resolves ContractsOutputFolder without anchoring to the settings file root, so relative contracts folders can be interpreted from the process working directory. This diverges from the existing contracts-path contract and can misplace generated files.
Also applies to: 104-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Refitter.Core/CoreGeneratorRunner.cs` around lines 63 - 66, The
GetContractsOutputPath method resolves ContractsOutputFolder relative to the
process working directory instead of anchoring to the settings file root,
causing misplaced file generation. Fix the GetContractsOutputPath method to
resolve relative paths from the directory containing the settings file, ensuring
ContractsOutputFolder is always interpreted relative to the settings file's
location regardless of the current working directory. Verify this same issue and
correction applies at the other affected location around lines 104-107 where
similar path resolution occurs.
Comment on lines
+22
to
+24
| <ItemGroup> | ||
| <ProjectReference Include="..\Refitter.Core\Refitter.Core.csproj" /> | ||
| </ItemGroup> |
Contributor
There was a problem hiding this comment.
Enable warning-as-error enforcement in this production project.
Since this project is production-facing and gained a new dependency edge, warnings should be promoted to errors to prevent silent regressions.
🛡️ Proposed fix
<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
+ <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
<Product>Refitter MSBuild Tasks</Product>As per coding guidelines, "Enable TreatWarningsAsErrors on production projects in C#".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Refitter.MSBuild/Refitter.MSBuild.csproj` around lines 22 - 24, The
Refitter.MSBuild.csproj project is a production project that currently lacks
warning-as-error enforcement. Add a PropertyGroup element to the project file
that sets TreatWarningsAsErrors to true, ensuring that all compiler warnings are
treated as errors during the build process. This will prevent silent regressions
and maintain code quality standards across the production codebase.
Source: Coding guidelines
Comment on lines
+95
to
+107
| private IGeneratorRunner CreateDefaultRunner() | ||
| { | ||
| var packageFolder = GetPackageFolder(); | ||
| var refitterDll = ResolveRefitterDllForTask(packageFolder); | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(refitterDll) && System.IO.File.Exists(refitterDll)) | ||
| { | ||
| TryLogCommandLine($"Using CLI adapter with {refitterDll}"); | ||
| return new Core.CliGeneratorRunner(refitterDll, ProcessTimeoutMilliseconds); | ||
| } | ||
|
|
||
| TryLogCommandLine("Using Core generator runner (in-process)"); | ||
| return new Core.CoreGeneratorRunner(); |
Contributor
There was a problem hiding this comment.
Default runner selection is not runtime-compatible and can pick an unusable CLI binary.
The new resolver picks the first existing bundled binary by TFM order (net10.0, then net9.0, then net8.0) without checking host runtime compatibility. On machines lacking that runtime, generation fails even when another bundled binary would work.
Also applies to: 135-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Refitter.MSBuild/RefitterGenerateTask.cs` around lines 95 - 107, The
CreateDefaultRunner() method selects a bundled refitter DLL without verifying
that the host machine has the required runtime installed, causing failures when
the first available binary by TFM order (net10.0, net9.0, net8.0) is not
compatible with the host. Modify the ResolveRefitterDllForTask method or the
logic in CreateDefaultRunner to check host runtime compatibility before
returning a refitter DLL path. Ensure that only a binary whose target runtime is
available on the host is selected, and if no compatible binary exists, fall back
to the CoreGeneratorRunner as a safe default rather than failing with an
unusable CLI binary.
… in msbuild.yml workflow
Copilot
AI
changed the title
Decouple MSBuild task from CLI binary dependency
fix: resolve failing "👌 Verify build" CI job by providing Refitter.Core as a local NuGet package
Jun 15, 2026
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/msbuild.yml (1)
37-38: 💤 Low valueRefitter.Core packaging steps are correctly placed.
The order ensures Refitter.Core is available in the local NuGet source before Refitter.MSBuild package is consumed by the test project (line 41), which is essential since Refitter.MSBuild now depends on Refitter.Core.
For consistency with lines 35-36, consider adding an explicit build step for Refitter.Core before the pack step:
📝 Optional consistency improvement
dotnet build -c release ../../src/Refitter/Refitter.csproj dotnet build -c release ../../src/Refitter.MSBuild/Refitter.MSBuild.csproj +dotnet build -c release ../../src/Refitter.Core/Refitter.Core.csproj dotnet pack -c release ../../src/Refitter.Core/Refitter.Core.csproj -o .While not strictly necessary (line 36 transitively builds Refitter.Core via ProjectReference, and
dotnet packwill build if needed), an explicit build step would make the workflow more uniform and self-documenting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/msbuild.yml around lines 37 - 38, For consistency with the explicit build steps at lines 35-36, add an explicit build step for Refitter.Core before the pack step (line 37). Add a dotnet build command targeting the Refitter.Core.csproj file with release configuration before the dotnet pack command. This makes the workflow more uniform and self-documenting, even though the pack step would transitively build the project if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/msbuild.yml:
- Around line 37-38: For consistency with the explicit build steps at lines
35-36, add an explicit build step for Refitter.Core before the pack step (line
37). Add a dotnet build command targeting the Refitter.Core.csproj file with
release configuration before the dotnet pack command. This makes the workflow
more uniform and self-documenting, even though the pack step would transitively
build the project if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14f98a1c-bc4f-42aa-ab4f-282e2c35002c
📒 Files selected for processing (1)
.github/workflows/msbuild.yml
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.
Description:
The
👌 Verify buildGitHub Actions job inmsbuild.ymlwas failing with:Root cause:
Refitter.MSBuild.csprojreferencesRefitter.Corevia a<ProjectReference>. Whendotnet packgenerates the NuGet package, this project reference is converted into a NuGet package dependency onRefitter.Corein the.nuspecmanifest. The CI workflow's Prepare step useddotnet add package Refitter.MSBuild -s ., which restricts NuGet to only the localtest/MSBuilddirectory as a source. BecauseRefitter.Corewas never packed and placed into that local feed, NuGet could not resolve the dependency and the restore step failed.Fix: Updated
.github/workflows/msbuild.ymlto packRefitter.Core.csprojand register the resulting.nupkgin the local NuGet feed before packing and installingRefitter.MSBuild:This ensures NuGet can resolve the
Refitter.Coredependency when installingRefitter.MSBuildfrom the local feed. All ofRefitter.Core's transitive dependencies (NSwag, Microsoft.CodeAnalysis, etc.) are already present in the NuGet cache from the earlierdotnet restore ../../src/Refitter.slnxstep and require no additional changes.Summary by CodeRabbit
New Features
Chores