fix: migrate PerfDiff to System.CommandLine 2.0.3#1043
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates PerfDiff tool from System.CommandLine beta 2.0.0 to stable 2.0.3 by removing IConsole dependencies, eliminating System.CommandLine.Rendering package, refactoring command-line option handling into static Option properties, replacing console abstraction with direct Console usage, and restructuring logging initialization to use dependency injection instead of console-based setup. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as System.CommandLine
participant SetAction as SetAction Handler
participant Program as Program.RunAsync
participant DI as ServiceProvider
participant Logger as Logger/LoggerFactory
participant PerfDiff as PerfDiff.CompareAsync
participant Console as System.Console
User->>CLI: Execute with args
CLI->>CLI: Parse args (DiffCommand options)
CLI->>SetAction: Invoke with parsed options
SetAction->>SetAction: Extract baseline, results,<br/>verbosity, failOnRegression
SetAction->>Program: RunAsync(params, CancellationToken)
Program->>Program: SetupLogging(verbosity)
Program->>DI: Configure ServiceProvider<br/>(AddLogging)
DI->>Logger: Create LoggerFactory
Logger->>Logger: AddSimpleConsole<br/>(minimalLogLevel)
DI-->>Program: Return ServiceProvider
Program->>DI: GetService<ILogger>
DI-->>Program: Return Logger instance
Program->>PerfDiff: CompareAsync(baseline,<br/>results, failOnRegression,<br/>logger, token)
PerfDiff->>Logger: Log warnings/traces
Logger->>Console: Write with formatting
Console-->>Logger:
PerfDiff-->>Program: Return exit code
Program-->>CLI: Return int
CLI-->>User: Exit with code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
📝 Coding Plan
Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Mar 14, 2026 7:28p.m. | Review ↗ |
rjmurillo
added
dependencies
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesFootnotes
|
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Directory.Packages.props`:
- Around line 33-38: The Directory.Packages.props entry for Perfolizer is still
pinned to 0.6.1 while the PR migrates PerfDiff to 0.6.7; update the
PackageVersion element for Perfolizer to Version="0.6.7" so the central package
table matches the code changes (look for the PackageVersion Include="Perfolizer"
element and change its Version value), and run a restore/build to verify no
dependency mismatches.
In `@src/tools/PerfDiff/DiffCommand.cs`:
- Around line 20-27: The BaselineOption and ResultsOption in DiffCommand.cs are
currently optional and default to null, allowing empty strings to reach
RunAsync; make both options required so the parser rejects missing values early:
update the Option<string?> definitions for BaselineOption and ResultsOption (the
Option instances declared) to mark them as required (e.g., set IsRequired = true
or construct them with required behavior) and remove/avoid a default null
fallback so the CLI parser enforces presence before invoking the action.
In `@src/tools/PerfDiff/Program.cs`:
- Around line 27-30: The CheckEltTraces tuple conflates "comparison succeeded"
with "should fail the command", so change CheckEltTraces (and any overloads) to
return two distinct booleans (e.g., comparisonSucceeded / etlSucceeded and
shouldFailOrHasRegression) and update CompareAsync and PerfDiff.CompareAsync
callers to accept both values; wire the parsed failOnRegression flag from
RunAsync/Program.cs into CompareAsync so that CompareAsync only returns a
failing exit code when either comparisonSucceeded is false OR the new shouldFail
value is true (honoring failOnRegression), and ensure places that previously
treated the single boolean as both are updated to use the two separate booleans
(functions to edit: CheckEltTraces, CompareAsync, PerfDiff.CompareAsync and
RunAsync wiring of failOnRegression).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2f1cc5d2-747e-4c7a-90df-7040c6813430
📒 Files selected for processing (7)
Directory.Packages.propssrc/tools/PerfDiff/DiffCommand.cssrc/tools/PerfDiff/Logging/SimpleConsoleLogger.cssrc/tools/PerfDiff/Logging/SimpleConsoleLoggerFactoryExtensions.cssrc/tools/PerfDiff/Logging/SimpleConsoleLoggerProvider.cssrc/tools/PerfDiff/PerfDiff.csprojsrc/tools/PerfDiff/Program.cs
💤 Files with no reviewable changes (1)
- src/tools/PerfDiff/PerfDiff.csproj
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tools/PerfDiff/Program.cs`:
- Around line 67-76: The SetupLogging method currently instantiates
LoggerFactory and registers it as a singleton; make it more idiomatic by
removing the explicit LoggerFactory creation and instead configure logging via
ServiceCollection.AddLogging(...) — call serviceCollection.AddLogging(builder =>
builder.SetMinimumLevel(minimalLogLevel).AddSimpleConsole(minimalLogLevel,
minimalErrorLevel)) (or the equivalent AddConsole provider your project uses) so
logging is configured through the DI builder and the ServiceProvider manages
disposal; update references to LoggerFactory only if other code depends on the
concrete instance.
- Around line 22-33: The SetAction delegate currently uses an async lambda that
assigns parseResult.Action!.ExitCode but must instead return the exit code
because rootCommand.SetAction expects Func<ParseResult, CancellationToken,
Task<int>>; update the lambda used in rootCommand.SetAction to return the int
from RunAsync(baseline, results, verbosity, failOnRegression, cancellationToken)
(i.e., replace the assignment to parseResult.Action!.ExitCode with "return
exitCode;") so the exit code propagates correctly from the lambda, keeping the
RunAsync call and variables (baseline, results, verbosity, failOnRegression)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c50c625e-fe39-45e0-97b5-a229a50eb783
📒 Files selected for processing (4)
Directory.Packages.propssrc/tools/PerfDiff/DiffCommand.cssrc/tools/PerfDiff/PerfDiff.cssrc/tools/PerfDiff/Program.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools/PerfDiff/Program.cs (1)
66-74: 🧹 Nitpick | 🔵 TrivialConsider using the DI builder pattern for cleaner logging setup.
The custom
SimpleConsoleLoggerProvideralready implements filtering viaIsEnabled()inSimpleConsoleLogger, soDebug/Tracemessages do reach the provider. However, the current pattern—manually creating aLoggerFactory, adding the provider, registering it in DI, and then callingAddLogging()—mixes manual factory management with DI conventions, which is unconventional and harder to reason about.♻️ Suggested refactor
static ServiceProvider SetupLogging(LogLevel minimalLogLevel, LogLevel minimalErrorLevel) { ServiceCollection serviceCollection = new ServiceCollection(); - LoggerFactory loggerFactory = new LoggerFactory(); - loggerFactory.AddSimpleConsole(minimalLogLevel, minimalErrorLevel); - serviceCollection.AddSingleton<ILoggerFactory>(loggerFactory); - serviceCollection.AddLogging(); + serviceCollection.AddLogging(builder => + { + builder.AddProvider(new SimpleConsoleLoggerProvider(minimalLogLevel, minimalErrorLevel)); + }); return serviceCollection.BuildServiceProvider(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/PerfDiff/Program.cs` around lines 66 - 74, Replace the manual LoggerFactory creation with the DI logging builder pattern inside SetupLogging: stop instantiating LoggerFactory and registering it, and instead call ServiceCollection.AddLogging(Action<ILoggingBuilder>) to configure minimum level and add the SimpleConsoleLoggerProvider (or call your existing AddSimpleConsole extension) so the provider is registered through the ILoggingBuilder; reference SetupLogging, ServiceCollection, AddLogging, SimpleConsoleLoggerProvider and SimpleConsoleLogger.IsEnabled when making the change and keep returning serviceCollection.BuildServiceProvider() after configuring logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools/PerfDiff/Program.cs`:
- Around line 66-74: Replace the manual LoggerFactory creation with the DI
logging builder pattern inside SetupLogging: stop instantiating LoggerFactory
and registering it, and instead call
ServiceCollection.AddLogging(Action<ILoggingBuilder>) to configure minimum level
and add the SimpleConsoleLoggerProvider (or call your existing AddSimpleConsole
extension) so the provider is registered through the ILoggingBuilder; reference
SetupLogging, ServiceCollection, AddLogging, SimpleConsoleLoggerProvider and
SimpleConsoleLogger.IsEnabled when making the change and keep returning
serviceCollection.BuildServiceProvider() after configuring logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 520617cd-129e-4b78-b972-57865eae80b7
📒 Files selected for processing (1)
src/tools/PerfDiff/Program.cs
|
|
Replace removed IConsole, ITerminal, CommandHandler.Create, and System.CommandLine.Rendering APIs with System.Console and SetAction. The CancellationToken from SetAction replaces the manual CancellationTokenSource + Console.CancelKeyPress hook. Closes #914 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…allocation Wrap LogToConsole call in try/finally to ensure Console.ResetColor() runs even if an exception occurs. Change LogLevelColorMap from expression-bodied property (=>) to static readonly field (=) so the dictionary is allocated once, not on every access. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Return ServiceProvider from SetupLogging so the caller can dispose it in the existing finally block. Prevents resource leak. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… code Use await using with ConfigureAwait(false) for async disposal instead of manual Dispose() in a finally block. Remove dead currentDirectory code that was never assigned. Register LoggerFactory as ILoggerFactory singleton so the container manages its lifetime. Use GetRequiredService for fail-fast behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The PackageVersion was pinned to 0.6.1 but the PR migrates to 0.6.7. Aligns the central package version with the actual dependency used. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set IsRequired = true on both options so the CLI parser rejects missing values. Change types from string? to string and remove the null-to-empty fallback in Program.cs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CheckEltTraces previously conflated "comparison succeeded" with "should fail based on regression". Separated concerns: CheckEltTraces now returns pure comparison results without policy logic. CompareAsync uses failOnRegression to decide the exit code. Also fixed typo "is was" to "it was" in log message. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The System.CommandLine 2.0.3 SetAction async overload expects Func<ParseResult, CancellationToken, Task<int>>. Return the exit code directly instead of assigning to parseResult.Action.ExitCode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rjmurillo-bot
force-pushed
the
fix/914-perfdiff-system-commandline
branch
from
c2b9e70 to
4893082
Compare
- Pass description to RootCommand constructor instead of setting it after - Remove redundant null assignment in TryGetETLPaths - Remove unused `using System` directive from Program.cs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix build errors from breaking API changes in System.CommandLine 2.0.3: IsRequired -> Required, LegalFilePathsOnly -> AcceptLegalFilePathsOnly, FromAmong -> AcceptOnlyFromAmong, constructor signature changes. Consolidate repeated CA1848/CA2254 pragma suppressions to method scope. Remove redundant null assignment in TryGetETLPaths. Remove unused using. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The DI container now owns the LoggerFactory lifetime. Previously, the manually created instance was registered as a singleton object, which meant the container would not dispose it on teardown. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…um level Promote LogTrace to LogWarning on exit paths that return non-zero so callers see why the tool failed without requiring diagnostic verbosity. Add SetMinimumLevel to the logging factory so Debug/Trace messages reach the provider when requested via --verbosity. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodAnalyzerTests.cs (1)
220-261:⚠️ Potential issue | 🟡 MinorXML documentation contradicts the updated test behavior.
The XML doc comments (lines 221-223, 225-233) still describe this as a "known limitation" where generic
.Callback<T>()type mismatches are "NOT currently validated." However, the test now expects diagnosticMoq1100to be raised, indicating the analyzer does detect this mismatch.The documentation should be updated to reflect that this validation is now implemented.
📝 Suggested documentation update
- /// <summary> - /// Test to document the known limitation with generic callback validation. - /// This test documents that .Callback<T>() with wrong type parameters is NOT currently validated. - /// This is an accepted limitation as the explicit generic syntax is rarely used in practice. - /// </summary> - /// <remarks> - /// <para> - /// Analysis shows zero real-world usage of the explicit generic .Callback<T>() syntax in open-source projects. - /// The recommended approach is to use lambda parameter inference which provides full type validation: - /// <c>.Callback(param => { })</c> or <c>.Callback((string param) => { })</c>. - /// </para> - /// <para> - /// See docs/rules/Moq1100.md "Known Limitations" section for best practices. - /// </para> - /// </remarks> + /// <summary> + /// Verifies that generic <c>.Callback<T>()</c> with mismatched type parameters + /// triggers the Moq1100 diagnostic. + /// </summary> + /// <remarks> + /// When the generic type argument does not match the mocked method's parameter type + /// (e.g., <c>.Callback<int></c> on a method expecting <see langword="string"/>), + /// the analyzer correctly reports a type mismatch. + /// </remarks> /// <param name="referenceAssemblyGroup">The Moq version reference assembly group.</param> /// <returns>A task representing the asynchronous unit test.</returns>Based on learnings: The retrieved learning stating "Moq1100 currently has a gap where it does not validate generic .Callback<T>() type parameters" is now outdated—the analyzer has been fixed to detect this mismatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodAnalyzerTests.cs` around lines 220 - 261, The XML documentation for the test GenericCallbackValidation_DetectsTypeMismatch is outdated: it states generic .Callback<T>() type mismatches are "NOT currently validated" but the test expects diagnostic Moq1100; update the summary/remarks to reflect that generic .Callback<T>() mismatches are now validated by the analyzer and remove wording about the known limitation, and adjust any guidance recommending lambda inference (you can keep it as an alternative) so the doc comments around the GenericCallbackValidation_DetectsTypeMismatch test and the remarks referencing docs/rules/Moq1100.md accurately state the current behavior (Moq1100 is raised when .Callback<T> uses the wrong type parameter).src/tools/PerfDiff/PerfDiff.cs (2)
17-19:⚠️ Potential issue | 🟠 MajorRe-check cancellation after the BenchmarkDotNet compare completes.
tokenis only observed before the first await. If Ctrl+C arrives whileTryCompareBenchmarkDotNetResultsAsyncis running, this method can still continue into the ETL path and even return a normal success/failure code instead of honoring cancellation. Add anotherThrowIfCancellationRequested()after the await and before ETL processing, and pass the token into downstream compares if they can do long-running I/O.🛑 Minimal fix
(bool compareSucceeded, bool regressionDetected) = await BenchmarkDotNetDiffer.TryCompareBenchmarkDotNetResultsAsync(baselineFolder, resultsFolder, logger).ConfigureAwait(false); + token.ThrowIfCancellationRequested(); ... + token.ThrowIfCancellationRequested(); (bool etlCompareSucceeded, bool etlRegressionDetected) = CheckEltTraces(baselineFolder, resultsFolder);Also applies to: 33-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/PerfDiff/PerfDiff.cs` around lines 17 - 19, After awaiting BenchmarkDotNetDiffer.TryCompareBenchmarkDotNetResultsAsync, re-check cancellation by calling token.ThrowIfCancellationRequested() immediately before entering the ETL/processing path; also propagate the CancellationToken into any downstream long-running compare/IO methods (e.g., pass token into BenchmarkDotNetDiffer.TryCompareBenchmarkDotNetResultsAsync and any subsequent compare/ETL helpers) so those operations can observe cancellation too. Ensure you add the same post-await ThrowIfCancellationRequested() in the other affected block (lines ~33-47) where a long-running await precedes ETL work.
74-87:⚠️ Potential issue | 🟠 MajorHarden ETL discovery against false matches and duplicate traces.
ETLFileExtension = "etl.zip"also matches names likehotel.zip, andSingleOrDefault()will throw if a directory contains more than one matching trace. That turns a recoverable "ETL unavailable" case into an unhandled exception. Use.etl.zipas the suffix and returnfalsewhen you see 0 or >1 matches instead of throwing.🧭 Proposed fix
- private static readonly string ETLFileExtension = "etl.zip"; + private const string ETLFileExtension = ".etl.zip"; ... - string[] files = Directory.GetFiles(path, $"*{ETLFileExtension}", SearchOption.AllDirectories); - etlPath = files.SingleOrDefault(); - return etlPath is not null; + string? singleMatch = null; + foreach (string candidate in Directory.EnumerateFiles(path, $"*{ETLFileExtension}", SearchOption.AllDirectories)) + { + if (singleMatch is not null) + { + etlPath = null; + return false; + } + + singleMatch = candidate; + } + + etlPath = singleMatch; + return etlPath is not null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/PerfDiff/PerfDiff.cs` around lines 74 - 87, Change ETLFileExtension from "etl.zip" to ".etl.zip" to avoid partial matches like "hotel.zip", and in TryGetETLPaths replace the SingleOrDefault usage with a safe length check: when Directory.Exists(path) call Directory.GetFiles(path, $"*{ETLFileExtension}", SearchOption.AllDirectories) and if files.Length == 1 set etlPath = files[0] and return true; otherwise set etlPath = null and return false (so 0 or >1 matches both return false). Also ensure the file-case branch still uses path.EndsWith(ETLFileExtension, StringComparison.OrdinalIgnoreCase).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Directory.Packages.props`:
- Line 36: The PR updated the PackageVersion for System.CommandLine to 2.0.3 but
lacks machine-readable validation evidence; run dotnet build and dotnet test for
all affected projects after this .props change and capture/attach the full
machine-readable logs (e.g., use dotnet test --logger
"trx;LogFileName=tests.trx" and dotnet build /v:minimal or msbuild /nologo
/clp:PerformanceSummary) that show successful compilation and test results, then
add those artifacts to the PR to validate the System.CommandLine Version="2.0.3"
change.
In `@src/tools/PerfDiff/Program.cs`:
- Around line 21-49: Add integration tests that exercise the new CLI contract by
invoking the command path that uses rootCommand.SetAction and by calling
Program.RunAsync directly: cover parser errors (missing required options from
DiffCommand), cancellation via a CancellationToken, and all exit-code branches
including success, failOnRegression=true/false, and ETL/PerfDiff.CompareAsync
failure paths; ensure tests assert the returned int exit codes and that
logging/exception propagation behavior mirrors the SetAction/RunAsync changes so
parser, cancellation, and CompareAsync outcomes are pinned down.
---
Outside diff comments:
In `@src/tools/PerfDiff/PerfDiff.cs`:
- Around line 17-19: After awaiting
BenchmarkDotNetDiffer.TryCompareBenchmarkDotNetResultsAsync, re-check
cancellation by calling token.ThrowIfCancellationRequested() immediately before
entering the ETL/processing path; also propagate the CancellationToken into any
downstream long-running compare/IO methods (e.g., pass token into
BenchmarkDotNetDiffer.TryCompareBenchmarkDotNetResultsAsync and any subsequent
compare/ETL helpers) so those operations can observe cancellation too. Ensure
you add the same post-await ThrowIfCancellationRequested() in the other affected
block (lines ~33-47) where a long-running await precedes ETL work.
- Around line 74-87: Change ETLFileExtension from "etl.zip" to ".etl.zip" to
avoid partial matches like "hotel.zip", and in TryGetETLPaths replace the
SingleOrDefault usage with a safe length check: when Directory.Exists(path) call
Directory.GetFiles(path, $"*{ETLFileExtension}", SearchOption.AllDirectories)
and if files.Length == 1 set etlPath = files[0] and return true; otherwise set
etlPath = null and return false (so 0 or >1 matches both return false). Also
ensure the file-case branch still uses path.EndsWith(ETLFileExtension,
StringComparison.OrdinalIgnoreCase).
In
`@tests/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodAnalyzerTests.cs`:
- Around line 220-261: The XML documentation for the test
GenericCallbackValidation_DetectsTypeMismatch is outdated: it states generic
.Callback<T>() type mismatches are "NOT currently validated" but the test
expects diagnostic Moq1100; update the summary/remarks to reflect that generic
.Callback<T>() mismatches are now validated by the analyzer and remove wording
about the known limitation, and adjust any guidance recommending lambda
inference (you can keep it as an alternative) so the doc comments around the
GenericCallbackValidation_DetectsTypeMismatch test and the remarks referencing
docs/rules/Moq1100.md accurately state the current behavior (Moq1100 is raised
when .Callback<T> uses the wrong type parameter).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea84db64-3942-4cb0-b892-a19433821934
📒 Files selected for processing (9)
Directory.Packages.propssrc/tools/PerfDiff/DiffCommand.cssrc/tools/PerfDiff/Logging/SimpleConsoleLogger.cssrc/tools/PerfDiff/Logging/SimpleConsoleLoggerFactoryExtensions.cssrc/tools/PerfDiff/Logging/SimpleConsoleLoggerProvider.cssrc/tools/PerfDiff/PerfDiff.cssrc/tools/PerfDiff/PerfDiff.csprojsrc/tools/PerfDiff/Program.cstests/Moq.Analyzers.Test/CallbackSignatureShouldMatchMockedMethodAnalyzerTests.cs
💤 Files with no reviewable changes (2)
- src/tools/PerfDiff/PerfDiff.csproj
- src/tools/PerfDiff/Logging/SimpleConsoleLoggerFactoryExtensions.cs
Resolved conflict in CallbackSignatureShouldMatchMockedMethodAnalyzerTests.cs by taking main's version (GenericCallbackWithWrongType_ProducesDiagnostic). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…com/rjmurillo/moq.analyzers into fix/914-perfdiff-system-commandline
ECS0200 is disabled in PerfDiff's .editorconfig, so const is the correct choice for compile-time literals. Reverts the unnecessary static readonly conversions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tools/PerfDiff/PerfDiff.cs (1)
76-82:⚠️ Potential issue | 🟡 Minor
SingleOrDefaultthrows when multiple ETL files exist.If a folder contains more than one
*.etl.zipfile,SingleOrDefault()throwsInvalidOperationExceptioninstead of returningnull. This causes an unhandled crash rather than the expected(false, false)fallback.Consider using
FirstOrDefault()if any match suffices, or catch the exception / checkfiles.Lengthto handle the ambiguous case gracefully.🛠️ Proposed fix using length check
if (Directory.Exists(path)) { string[] files = Directory.GetFiles(path, $"*{ETLFileExtension}", SearchOption.AllDirectories); - etlPath = files.SingleOrDefault(); - return etlPath is not null; + if (files.Length == 1) + { + etlPath = files[0]; + return true; + } + + etlPath = null; + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/PerfDiff/PerfDiff.cs` around lines 76 - 82, The TryGetETLPaths method uses files.SingleOrDefault() which will throw if more than one ETL file exists; change the logic to handle multiple matches gracefully by either using files.FirstOrDefault() (if any match is acceptable) or explicitly check files.Length and treat multiple results as an ambiguous case (set etlPath = null and return false or log/handle as appropriate) so the method never throws; update the code in TryGetETLPaths to perform that safe selection/length check and return false when ambiguous.
🤖 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/tools/PerfDiff/Program.cs`:
- Around line 51-61: In the FileNotFoundException catch block replace the
current call that only logs fex.Message with a call that passes the exception
object to the logger so the stack trace and FileName are preserved (i.e., change
the use of logger.LogError(fex.Message) in the catch (FileNotFoundException fex)
block to an overload that accepts the exception like logger.LogError(fex, "...")
and keep the existing pragma disables around the statement if needed); this
ensures FileNotFoundException fex, its stacktrace and FileName are recorded for
debugging.
---
Outside diff comments:
In `@src/tools/PerfDiff/PerfDiff.cs`:
- Around line 76-82: The TryGetETLPaths method uses files.SingleOrDefault()
which will throw if more than one ETL file exists; change the logic to handle
multiple matches gracefully by either using files.FirstOrDefault() (if any match
is acceptable) or explicitly check files.Length and treat multiple results as an
ambiguous case (set etlPath = null and return false or log/handle as
appropriate) so the method never throws; update the code in TryGetETLPaths to
perform that safe selection/length check and return false when ambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ccabf817-eaaf-4583-8398-33631ddfa5ae
📒 Files selected for processing (2)
src/tools/PerfDiff/PerfDiff.cssrc/tools/PerfDiff/Program.cs
Pass the exception object to LogError to preserve stack trace and FileName property for debugging file-not-found issues. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [Meziantou.Analyzer](https://github.com/meziantou/Meziantou.Analyzer) | `3.0.21` → `3.0.22` |  |  | --- ### Release Notes <details> <summary>meziantou/Meziantou.Analyzer (Meziantou.Analyzer)</summary> ### [`v3.0.22`](https://github.com/meziantou/Meziantou.Analyzer/releases/tag/3.0.22) [Compare Source](https://github.com/meziantou/Meziantou.Analyzer/compare/3.0.21...3.0.22) NuGet package: <https://www.nuget.org/packages/Meziantou.Analyzer/3.0.22> ##### What's Changed - Add MA0188: Use System.TimeProvider instead of a custom time abstraction by [@​Copilot](https://github.com/Copilot) in [#​1055](https://github.com/meziantou/Meziantou.Analyzer/pull/1055) **Full Changelog**: <meziantou/Meziantou.Analyzer@3.0.21...3.0.22> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/rjmurillo/moq.analyzers). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41OS4wIiwidXBkYXRlZEluVmVyIjoiNDMuNTkuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOltdfQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
rjmurillo
requested changes
Mar 14, 2026
Owner
rjmurillo
left a comment
rjmurillo
left a comment
There was a problem hiding this comment.
Review: Request Changes - Migration is solid, DeepSource failure + CancellationToken gap
What Works Well
- Correct migration from
System.CommandLine 2.0.0-beta1to stable2.0.3.SetActionwithFunc<ParseResult, CancellationToken, Task<int>>is the right API. FrozenDictionarywithStringComparer.OrdinalIgnoreCasefor verbosity mapping is appropriate.IsRequired = trueon--baselineand--resultsoptions (fixed in follow-up commit).- Perfolizer correctly pinned to
0.6.1to match BenchmarkDotNet0.15.8exact dependency[0.6.1]. - Separation of
CheckEltTracesfromfailOnRegressionpolicy improves cohesion. - Visibility narrowing to
internalis correct for a non-shipped tool. - All CodeRabbit review issues were addressed in follow-up commits.
Required Fixes
1. DeepSource CI failure (Blocking)
DeepSource C# analysis reports blocking issues. These must be investigated and resolved before merge. Check the run at the DeepSource dashboard for specific findings.
2. CancellationToken not threaded through operations (Medium)
CancellationToken is only checked once at entry (ThrowIfCancellationRequested). It is never passed to TryCompareBenchmarkDotNetResultsAsync or CheckEltTraces. Long-running operations remain non-cancellable. This is a pre-existing gap but worth addressing in this migration since the framework now provides the token natively.
Suggested Follow-ups
- File an issue for CLI contract test coverage (parser errors, exit code branches).
- The
SetupLoggingmethod with manualLoggerFactorycreation is functional but unconventional. Not blocking.
Pass CancellationToken from CompareAsync through BenchmarkDotNetDiffer, BenchmarkComparisonService, and BenchmarkFileReader down to File.ReadAllTextAsync. Previously the token was only checked once at entry via ThrowIfCancellationRequested. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SingleOrDefault throws InvalidOperationException when multiple ETL files exist in a directory. Use FirstOrDefault instead and log a warning when multiple files are found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Fix CS1573 and SA1611 warnings from missing XML documentation for the cancellationToken parameters added in the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace .Any() with .Length for array emptiness checks. Use Array.TrueForAll instead of negated LINQ .Any(). Replace double dictionary lookup (ContainsKey + indexer) with single TryGetValue loop. Fix string interpolation in log template to use structured logging parameter. Remove stray blank line in csproj. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
…ontract Add PerfDiff.Tests project with 20 tests covering: - CancellationToken propagation through async call chain - FirstOrDefault behavior with multiple ETL files - DiffCommand CLI option parsing and validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace `await using ConfiguredAsyncDisposable asyncDisposal` with explicit try/finally and DisposeAsync().ConfigureAwait(false) to eliminate DeepSource CS-W1100 (unused variable) while preserving async disposal with ConfigureAwait(false) for MA0004 compliance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| } | ||
| } | ||
|
|
||
| return matched.ToArray(); |
Resolve conflicts in BenchmarkFileReader.cs and Program.cs: - Keep CancellationToken threading and Array.TrueForAll from #1043 - Add CancelledExitCode from main for distinct cancellation exit code - Keep AddLogging builder pattern from #1043 (replaces IConsole) - Keep try/finally serviceProvider disposal from static analysis fix Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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
3 participants
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
IConsole,ITerminal,CommandHandler.CreateAPIs withSystem.ConsoleandSetActionCancellationTokenfromSetActionreplaces manualCancellationTokenSource+Console.CancelKeyPresshookChanges
Directory.Packages.propsPerfDiff.csprojDiffCommand.csHandlerdelegate, expose options as static propertiesProgram.csSetAction+ParseResult.GetValue, removeIConsoleparameterSimpleConsoleLogger.csIConsole/ITerminalwithSystem.ConsoleSimpleConsoleLoggerProvider.csIConsoledependencySimpleConsoleLoggerFactoryExtensions.csIConsoleparameterTest plan
dotnet build Moq.Analyzers.slnpasses with 0 errors, 0 warningsCloses #914
🤖 Generated with Claude Code
Summary by CodeRabbit