fix: enforce generated analyzer config ordering#130
fix: enforce generated analyzer config ordering#130ancplua-automerge[bot] merged 29 commits intomainfrom
Conversation
|
@coderabbitai autofix |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🧰 Additional context used📓 Path-based instructions (2)tools/**⚙️ CodeRabbit configuration file
Files:
**/*.props⚙️ CodeRabbit configuration file
Files:
🪛 GitHub Actions: Claude Code Review / 0_claude-review.txttools/ConfigFilesGenerator/Program.cs[error] 62-62: lint_config failure: Cannot use local variable 'CompilerAnalyzerConfigGlobalLevel' before it is declared (tools/ConfigFilesGenerator/Program.cs, line 62). [warning] 377-377: lint_config warning: Unreachable code detected at Program.cs:377. 🪛 GitHub Actions: Claude Code Review / claude-reviewtools/ConfigFilesGenerator/Program.cs[error] 62-62: lint_config failure: Cannot use local variable 'CompilerAnalyzerConfigGlobalLevel' before it is declared. [warning] 377-377: lint_config warning: Unreachable code detected. 🔇 Additional comments (12)
📝 WalkthroughWalkthroughDirectory.Build.props now applies .NET SDK defaults only when unset (ImplicitUsings=enable, Deterministic=true, EnableNETAnalyzers=true, AnalysisLevel=latest-all), sets ContinuousIntegrationBuild=true only when empty and CI==true, and enables MSBuildTreatWarningsAsErrors/TreatWarningsAsErrors/EnforceCodeStyleInBuild only for CI or Release when unset. It adds EnableEditorConfigDogfooding (default true) to conditionally include src\Config*.editorconfig (excluding src\Config\ANcpLua.NET.Sdk*.editorconfig). Multiple src/Config/*.editorconfig files had global_level adjustments. tools/ConfigFilesGenerator was made deterministic and enforces pinned analyzer versions from src/Build/Common/Version.props (which adds NetAnalyzersVersion). Tests and helpers use StringComparison.Ordinal; Directory.Build.props injection now parses XML. Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (6 passed)
Comment |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Pull request overview
This PR makes analyzer configuration generation and consumption more deterministic by introducing stable global_level precedence across SDK-shipped global AnalyzerConfig files, and by dogfooding those configs in the repo’s own projects. It also tightens repo build analysis settings and updates tests to be explicit about string-comparison semantics.
Changes:
- Assign deterministic, distinct
global_levelvalues across generated and SDK-shipped global.editorconfiganalyzer config files. - Update config-generation tooling to compute stable global levels and attempt to pin
Microsoft.CodeAnalysis.NetAnalyzersgeneration to the repo’s declared .NET SDK version. - Enable/strengthen analyzers + warnings-as-errors for CI/Release and adjust tests to be explicit about
StringComparisonand disposal warnings.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/ConfigFilesGenerator/Program.cs | Adds deterministic global_level assignment, reads DotNetSdkVersion from repo, and pins NetAnalyzers generation when available. |
| tests/ANcpLua.Sdk.Tests/TemplatesTests.cs | Makes string assertions explicit with StringComparison.Ordinal. |
| tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cs | Suppresses CA2000 for a builder factory method disposed by tests. |
| tests/ANcpLua.Sdk.Tests/SdkTests.cs | Suppresses CA2000 for builder creation, makes string comparisons explicit, and ensures PEReader disposal. |
| tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cs | Suppresses CA2000 for builder creation and makes CLI arg assertions ordinal. |
| tests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cs | Uses ordinal Replace, improves retry constant naming, and makes SDK resolution checks ordinal. |
| src/Config/NamingConvention.editorconfig | Sets a non-zero global_level for deterministic precedence. |
| src/Config/Global.editorconfig | Raises global_level to define highest precedence among SDK globals. |
| src/Config/Compiler.editorconfig | Updates header comment and assigns a deterministic global_level. |
| src/Config/CodingStyle.editorconfig | Assigns global_level, removes editor-only formatting settings, and updates/normalizes style options. |
| src/Config/ANcpLua.NET.Sdk.Web.editorconfig | Raises global_level to ensure it overrides base SDK-shipped globals. |
| src/Config/ANcpLua.NET.Sdk.SingleFileApp.editorconfig | Raises global_level to ensure it overrides base SDK-shipped globals. |
| src/Config/Analyzer.Microsoft.CodeAnalysis.NetAnalyzers.editorconfig | Sets deterministic global_level and updates generated content ordering/contents. |
| src/Config/Analyzer.Microsoft.CodeAnalysis.BannedApiAnalyzers.editorconfig | Sets deterministic global_level and updates header/trailing formatting. |
| src/Config/Analyzer.Microsoft.CodeAnalysis.Analyzers.editorconfig | Sets deterministic global_level and updates header/trailing formatting. |
| src/Config/Analyzer.ANcpLua.Analyzers.editorconfig | Sets deterministic global_level and updates header/trailing formatting. |
| Directory.Build.props | Enables implicit usings + analyzers, enforces warnings-as-errors in CI/Release, and includes SDK config files for dogfooding. |
| .editorconfig | Changes C# file charset from UTF-8 BOM to UTF-8. |
Comments suppressed due to low confidence (1)
tools/ConfigFilesGenerator/Program.cs:240
GetAllReferencedNuGetPackages()can return multiple entries for the same package ID at different versions (the set is keyed by ID+version, and dependency traversal usesdependency.VersionRange.MinVersion). Since the generator writes to a path derived only frompackageId(Analyzer.{packageId}.editorconfig) while iterating inParallel.ForEachAsync, two versions of the same ID could race and produce nondeterministic output. Consider deduplicating to a single version per package ID (e.g., pick the highest resolved version) before running the parallel loop, or otherwise ensure only one writer per output file.
async Task GenerateEditorConfigForAnalyzers()
{
var packages = await GetAllReferencedNuGetPackages().ConfigureAwait(false);
var globalLevelsByPackageId = GetAnalyzerConfigGlobalLevels(packages.Select(static p => p.Id));
await Parallel.ForEachAsync(packages, async (item, cancellationToken) =>
{
var (packageId, packageVersion) = item;
Console.WriteLine(packageId + "@" + packageVersion);
| .Distinct(StringComparer.Ordinal) | ||
| .OrderBy(static id => id, StringComparer.Ordinal) | ||
| .ToArray(); | ||
|
|
||
| // Stable, deterministic, collision-free within the referenced package set. | ||
| // Low range is reserved for known "core" analyzer packs. | ||
| var result = new Dictionary<string, int>(StringComparer.Ordinal); |
|
|
||
| if (item.Type is DependencyType.NuGet && item.Name is not null && | ||
| (item.Version is null || !item.Version.Contains("$("))) | ||
| (item.Version is null || !item.Version.Contains("$(", StringComparison.Ordinal))) | ||
| yield return (item.Name, item.Version); | ||
|
|
||
| foreach (var package in new[] | ||
| { | ||
| "Microsoft.CodeAnalysis.NetAnalyzers" | ||
| }) | ||
| yield return (package, null); | ||
| // Pin analyzer package versions to the repo's declared .NET SDK version to keep |
There was a problem hiding this comment.
Implemented in 5b85e59: the generator now resolves $(NetAnalyzersVersion) from src/Build/Common/Version.props (including property indirection like $(DotNetSdkVersion)) and fails fast if unresolved, so NetAnalyzers pinning is explicit/reproducible instead of drifting to latest. No UI changes in this PR (screenshot: N/A).
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cs (1)
335-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace string-based XML mutation with proper XDocument parsing—silent injection failure when
<Project>has attributes.The
content.Replace("<Project>", ...)pattern fails when the root element is<Project Sdk="...">or any variant with attributes. Evidence in codebase shows<Project Sdk="{rootSdkName}">is used, meaning DisableVersionAnalyzer injection silently skips for such content, breaking analyzer suppression guarantees in test projects.Suggested refactor
public new SdkProjectBuilder WithDirectoryBuildProps(string content) { - // Inject DisableVersionAnalyzer to prevent AL0017-AL0019 errors in test projects - var modifiedContent = content.Replace( - "<Project>", - """ - <Project> - <PropertyGroup> - <DisableVersionAnalyzer>true</DisableVersionAnalyzer> - </PropertyGroup> - """, - StringComparison.Ordinal); - base.WithDirectoryBuildProps(modifiedContent); + var doc = XDocument.Parse(content, LoadOptions.PreserveWhitespace); + if (doc.Root is null || doc.Root.Name.LocalName != "Project") + throw new InvalidOperationException("Directory.Build.props must have a <Project> root element."); + + doc.Root.AddFirst( + new XElement("PropertyGroup", + new XElement("DisableVersionAnalyzer", "true"))); + + base.WithDirectoryBuildProps(doc.ToString(SaveOptions.DisableFormatting)); return this; }🤖 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 `@tests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cs` around lines 335 - 344, The code currently does a string Replace on "<Project>" which fails when the root <Project> has attributes (e.g., Sdk="..."); change the mutation in SdkProjectBuilder.cs to parse the XML with XDocument.Parse(content), locate the root XElement (doc.Root, typically named "Project"), create a new XElement("PropertyGroup") containing XElement("DisableVersionAnalyzer", "true"), insert it (use root.AddFirst(propertyGroup) to ensure it precedes other content), then serialize the XDocument back to a string and pass that to base.WithDirectoryBuildProps(modifiedContent); this guarantees injection even when the <Project> element has attributes.
🤖 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 `@Directory.Build.props`:
- Around line 6-13: The repo-level property overrides (ImplicitUsings,
Deterministic, EnableNETAnalyzers, AnalysisLevel) and the policy flags
(MSBuildTreatWarningsAsErrors, TreatWarningsAsErrors, EnforceCodeStyleInBuild)
must be made opt-out by adding Condition guards so they only set a value when
the project hasn’t already provided one; update each property to include a
Condition testing for an empty/undefined value (e.g.,
Condition="'$(ImplicitUsings)' == ''") and update each policy flag to combine
its existing release/CI condition with a check that the project hasn’t set it
(e.g., Condition="('$(MSBuildTreatWarningsAsErrors)' == '') AND
('$(ContinuousIntegrationBuild)' == 'true' OR '$(Configuration)' ==
'Release')"), referencing the exact property names ImplicitUsings,
Deterministic, EnableNETAnalyzers, AnalysisLevel, MSBuildTreatWarningsAsErrors,
TreatWarningsAsErrors, and EnforceCodeStyleInBuild.
- Around line 37-38: Add a condition guard and opt-out property so editorconfig
injection can be disabled by consumers: introduce a PropertyGroup with a
property like EnableEditorConfigDogfooding that defaults to true when unset
(Condition="'$(EnableEditorConfigDogfooding)'==''" set to "true"), then wrap the
existing ItemGroup/EditorConfigFiles entry with a Condition that checks
EnableEditorConfigDogfooding == 'true' so downstream projects can set that
property to false to opt out.
In `@tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cs`:
- Line 130: The test currently checks for the forbidden flag with a trailing
space which can miss occurrences at end-of-string; update the assertion in
MtpDetectionTests (the Assert.DoesNotContain call that checks cliArgs) to look
for the token "--report-trx" (no trailing space) so the forbidden flag is
detected whether it appears mid-string or at the end; leave the
StringComparison.Ordinal argument unchanged.
In `@tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cs`:
- Around line 33-36: Remove the CA2000 SuppressMessage attribute applied to the
helper (the attribute shown above) and instead make disposal ownership explicit
where SdkProjectBuilder.Create(...) is called: update each caller (e.g., tests
MtpDetectionTests, SdkTests, and SourceGeneratorDefaultsTests) to use "await
using var project = SdkProjectBuilder.Create(...);" so the returned builder is
disposed by the test; ensure no helper-level suppressions remain and adjust call
sites to own the async disposal.
In `@tools/ConfigFilesGenerator/Program.cs`:
- Around line 59-62: Move the const CompilerAnalyzerConfigGlobalLevel so it is
declared before any use: place the const declaration above the first method that
references it (i.e., before GenerateEditorConfigForCompilerAnalyzers) and remove
the later duplicate declaration at line 377; ensure callers that reference
CompilerAnalyzerConfigGlobalLevel (e.g., in
GenerateEditorConfigForCompilerAnalyzers where it's used to append
"global_level") still compile against the single, earlier declaration.
- Around line 420-423: The bare catch that returns null in Program.cs is
swallowing XML parsing exceptions (the try/catch that returns null when reading
Version.props); change it to catch (Exception ex) instead and log a warning with
the exception details before returning null (use the existing logger if
available or Console.Error/Console.WriteLine) so malformed Version.props parsing
failures are visible to build authors.
---
Outside diff comments:
In `@tests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cs`:
- Around line 335-344: The code currently does a string Replace on "<Project>"
which fails when the root <Project> has attributes (e.g., Sdk="..."); change the
mutation in SdkProjectBuilder.cs to parse the XML with XDocument.Parse(content),
locate the root XElement (doc.Root, typically named "Project"), create a new
XElement("PropertyGroup") containing XElement("DisableVersionAnalyzer", "true"),
insert it (use root.AddFirst(propertyGroup) to ensure it precedes other
content), then serialize the XDocument back to a string and pass that to
base.WithDirectoryBuildProps(modifiedContent); this guarantees injection even
when the <Project> element has attributes.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e7a7085-b132-4f46-82f2-94c51151258f
⛔ Files ignored due to path filters (1)
.editorconfigis excluded by none and included by none
📒 Files selected for processing (17)
Directory.Build.propssrc/Config/ANcpLua.NET.Sdk.SingleFileApp.editorconfigsrc/Config/ANcpLua.NET.Sdk.Web.editorconfigsrc/Config/Analyzer.ANcpLua.Analyzers.editorconfigsrc/Config/Analyzer.Microsoft.CodeAnalysis.Analyzers.editorconfigsrc/Config/Analyzer.Microsoft.CodeAnalysis.BannedApiAnalyzers.editorconfigsrc/Config/Analyzer.Microsoft.CodeAnalysis.NetAnalyzers.editorconfigsrc/Config/CodingStyle.editorconfigsrc/Config/Compiler.editorconfigsrc/Config/Global.editorconfigsrc/Config/NamingConvention.editorconfigtests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cstests/ANcpLua.Sdk.Tests/MtpDetectionTests.cstests/ANcpLua.Sdk.Tests/SdkTests.cstests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cstests/ANcpLua.Sdk.Tests/TemplatesTests.cstools/ConfigFilesGenerator/Program.cs
📜 Review details
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (windows-latest)
- GitHub Check: test (macos-latest)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (3)
**/*.props
⚙️ CodeRabbit configuration file
**/*.props: MSBuild property files. Review for: condition guards on every override
(consumer must be able to opt out), no<RemoveAttributes>on items
that consumers may have set, and that defaults are sensible without
forcing a downstream cascade.
Files:
Directory.Build.props
tests/**/*.cs
⚙️ CodeRabbit configuration file
tests/**/*.cs: xUnit + FluentAssertions. AAA pattern. NSubstitute for mocks.
Flag: blocking-async patterns (Task.Run(() => x.Result)), missing
async Task(sync-void),Thread.Sleepinstead ofawait Task.Delay,
tests asserting on private state via reflection.
Files:
tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cstests/ANcpLua.Sdk.Tests/TemplatesTests.cstests/ANcpLua.Sdk.Tests/MtpDetectionTests.cstests/ANcpLua.Sdk.Tests/Helpers/SdkProjectBuilder.cstests/ANcpLua.Sdk.Tests/SdkTests.cs
tools/**
⚙️ CodeRabbit configuration file
tools/**: Internal tooling — config file generators, build helpers. Lower bar than
production src/, but still flag obvious bugs (missing null check that
crashes the build, hardcoded paths, swallowed exceptions).
Files:
tools/ConfigFilesGenerator/Program.cs
🪛 GitHub Check: lint_config
tools/ConfigFilesGenerator/Program.cs
[failure] 62-62:
Cannot use local variable 'CompilerAnalyzerConfigGlobalLevel' before it is declared
[warning] 377-377:
Unreachable code detected
🔇 Additional comments (16)
src/Config/NamingConvention.editorconfig (1)
2-2: LGTM.Precedence value
20slots correctly between generated analyzer configs (10–13) and CodingStyle (30).src/Config/Global.editorconfig (1)
2-2: LGTM.
global_level = 50correctly establishes this as the highest-precedence SDK-shipped global config.src/Config/Analyzer.ANcpLua.Analyzers.editorconfig (1)
1-4: LGTM.
global_level = 13aligns with the generator's stable mapping. Comment clarifies the conflict-avoidance rationale.src/Config/Analyzer.Microsoft.CodeAnalysis.NetAnalyzers.editorconfig (1)
1-4: LGTM.
global_level = 11is deterministic per the generator. Rule removals reflect the current analyzer assembly state.src/Config/Analyzer.Microsoft.CodeAnalysis.BannedApiAnalyzers.editorconfig (1)
1-4: LGTM.
global_level = 12matches the generator's stable assignment.src/Config/Analyzer.Microsoft.CodeAnalysis.Analyzers.editorconfig (1)
1-4: LGTM.
global_level = 10is the lowest in the analyzer-config tier, correct per generator mapping.src/Config/CodingStyle.editorconfig (2)
2-6: LGTM.
global_level = 30correctly slots between NamingConvention and Compiler configs. Comment clarifies the file's role as a global AnalyzerConfig.
67-67: LGTM.Correct option name:
csharp_style_prefer_local_over_anonymous_functionis the canonical Roslyn property.tools/ConfigFilesGenerator/Program.cs (3)
379-403: LGTM.
GetAnalyzerConfigGlobalLevelsprovides stable, deterministic precedence mapping. The fallback1000 + iensures unknown packages get unique levels without colliding with the reserved range.
405-424: SDK version extraction logic is sound.Correctly reads
<DotNetSdkVersion>fromVersion.propsand gracefully degrades when unavailable.
498-508: LGTM.Pinning
Microsoft.CodeAnalysis.NetAnalyzersto the repo's SDK version eliminates "latest stable" drift and keeps generated configs reproducible.src/Config/Compiler.editorconfig (1)
1-4: Deterministic global precedence looks correct.The explicit precedence comment +
global_level = 40is clear and aligns with deterministic analyzer-config ordering.src/Config/ANcpLua.NET.Sdk.Web.editorconfig (1)
1-3: Precedence bump is coherent and deterministic.
global_level = 60with the explicit intent note is consistent with the repo-wide ordering strategy.src/Config/ANcpLua.NET.Sdk.SingleFileApp.editorconfig (1)
1-3: Ordering update is good.
global_level = 61cleanly establishes intended precedence for this config.tests/ANcpLua.Sdk.Tests/TemplatesTests.cs (1)
81-82: String-comparison hardening is solid.Using explicit
StringComparison.Ordinalon these content assertions improves determinism and avoids locale-sensitive drift in template validation tests.Also applies to: 141-142, 153-155, 185-188, 211-211, 244-247
tests/ANcpLua.Sdk.Tests/SdkTests.cs (1)
79-79: These correctness tweaks look good.Ordinal comparison usage and the scoped
PEReaderdisposal improve determinism/resource hygiene without changing test intent.Also applies to: 1048-1048, 1340-1340
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
ANcpLua
force-pushed
the
fix/refix-codestyle-policy-pass
branch
from
d0411ea to
b6e83a8
Compare
|
@coderabbitai autofix |
| // Skip duplicate top-level package IDs we've already yielded; the same | ||
| // package showing up in multiple projects in src/ shouldn't fan out | ||
| // into repeated NuGet work. Don't mark the ID as seen yet — only after | ||
| // we've successfully resolved its version, so that a first sighting | ||
| // with an unresolved $(...) doesn't lock out the pinned fallback. | ||
| if (emittedIds.Contains(item.Name)) | ||
| continue; |
| var result = new Dictionary<string, int>(StringComparer.OrdinalIgnoreCase); | ||
| for (var i = 0; i < orderedIds.Length; i++) | ||
| { | ||
| var id = orderedIds[i]; | ||
| result[id] = id switch | ||
| { | ||
| _ when string.Equals(id, "Microsoft.CodeAnalysis.Analyzers", StringComparison.OrdinalIgnoreCase) => 10, | ||
| _ when string.Equals(id, "Microsoft.CodeAnalysis.NetAnalyzers", StringComparison.OrdinalIgnoreCase) => 11, | ||
| _ when string.Equals(id, "Microsoft.CodeAnalysis.BannedApiAnalyzers", StringComparison.OrdinalIgnoreCase) => 12, | ||
| _ when string.Equals(id, "ANcpLua.Analyzers", StringComparison.OrdinalIgnoreCase) => 13, | ||
| _ when string.Equals(id, "AwesomeAssertions.Analyzers", StringComparison.OrdinalIgnoreCase) => 14, | ||
| // xunit.analyzers ships as a transitive of xunit.v3 (resolved via Version.props | ||
| // property expansion). Reserved so its config doesn't churn when the package set shifts. | ||
| _ when string.Equals(id, "xunit.analyzers", StringComparison.OrdinalIgnoreCase) => 15, | ||
| _ => 1000 + i | ||
| }; | ||
| } | ||
|
|
||
| return result; |
GetReferencedNuGetPackages was deduping by package id alone, so when the same id appeared with different versions (e.g. Microsoft.CodeAnalysis.CSharp without a version in src/*.csproj plus a VersionOverride in SourceGenerators.targets) the first sighting won and later occurrences got dropped before the downstream highest-version dedup could see them. Switch the early-skip to an (id, resolved-version) tuple via a small file-scoped comparer so identical (id, version) pairs are still pruned but distinct versions of the same id flow through. GetAnalyzerConfigGlobalLevels was assigning the non-reserved fallback as 1000+i, where i indexed into the full alphabetically-sorted package list. Adding or removing any unrelated package shifted i and re-numbered every later entry's level, churning the regenerated configs. Derive the fallback from a deterministic FNV-1a hash of the package id (uppercased UTF-8) reduced into [1000,9999], with linear-probe collision resolution that wraps inside the same range. Stable IDs keep their level when unrelated packages join or leave; collisions only renumber the colliding neighbor. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
@coderabbitai autofix |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cs (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: this helper suppression still violates the no-suppressions path for CA2000.
CreateProject()is still hiding the ownership transfer instead of making it explicit. InlineSdkProjectBuilder.Create(...)at theawait usingsites in this file and remove theSuppressMessage/import, otherwise the CA2000 warning remains suppressed rather than fixed.Also applies to: 33-36
🤖 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 `@tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cs` at line 1, CreateProject() is hiding ownership transfer and keeps CA2000 suppressed; replace calls to SdkProjectBuilder.Create(...) by inlining its object construction directly at each await using site in SourceGeneratorDefaultsTests (remove the helper CreateProject method and its using/import of SuppressMessage), ensure each disposable resource is created in the await using statement so ownership is explicit (e.g., await using var builder = new SdkProjectBuilder(...)), and delete the SuppressMessage/using System.Diagnostics.CodeAnalysis import so CA2000 is no longer suppressed.tests/ANcpLua.Sdk.Tests/SdkTests.cs (1)
1-1:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBlocking: the shared test-builder helper is still suppressing CA2000.
This keeps the analyzer exception alive across the biggest test surface in the PR. To actually satisfy the objective, replace
await using var project = CreateProject(...);with directSdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...)ownership at the test call sites in this file, then remove the helper suppression andSystem.Diagnostics.CodeAnalysisimport.Also applies to: 41-46
🤖 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 `@tests/ANcpLua.Sdk.Tests/SdkTests.cs` at line 1, Tests still rely on the shared helper that suppresses CA2000; replace each usage like "await using var project = CreateProject(...);" in SdkTests.cs with explicit ownership by calling SdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...) directly at the test call sites so the test owns disposal, then remove the helper suppression and the System.Diagnostics.CodeAnalysis using. Update all occurrences (including the ones around lines 41–46) to use the SdkProjectBuilder fluent chain and delete the unnecessary suppression/import.tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cs (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlocking: remove the helper-level CA2000 suppression.
This still suppresses analyzer dogfooding in a test file the PR is supposed to harden. The fix needs to be at the ownership boundary: stop routing disposal through
CreateProject(...)here, inlineSdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...)at theawait using var project = ...call sites in this file, then drop the attribute andSystem.Diagnostics.CodeAnalysisimport. The current suppression is also one of the pipeline warnings.Also applies to: 46-49
🤖 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 `@tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cs` at line 1, Remove the helper-level CA2000 suppression and the System.Diagnostics.CodeAnalysis using; stop routing disposal through CreateProject(...) by inlining the builder chain at each await using var project = ... call site in this file: replace calls that use CreateProject(...) with SdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...) directly so the project is created and disposed at the ownership boundary, then delete the suppression attribute and unused import; ensure every location that previously called CreateProject(...) now constructs the project inline so analyzers see correct disposal.tools/ConfigFilesGenerator/Program.cs (1)
489-498:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t let
Version.propsparse failures abort the generator.
XDocument.Load(versionPropsPath)is unguarded now. Any malformed XML or transient conflict insrc/Build/Common/Version.propswill throw and stop config generation, but the requested behavior here was “warn and fall back.” Wrap the load incatch (Exception ex), log to stderr, and return an empty property map instead of crashing the tool.Proposed fix
static IReadOnlyDictionary<string, string> LoadMsBuildProperties(FullPath rootFolder) { var versionPropsPath = rootFolder / "src" / "Build" / "Common" / "Version.props"; if (!File.Exists(versionPropsPath)) return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); - var doc = XDocument.Load(versionPropsPath); + XDocument doc; + try + { + doc = XDocument.Load(versionPropsPath); + } + catch (Exception ex) + { + Console.Error.WriteLine($"Warning: failed to load '{versionPropsPath}': {ex}"); + return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); + } + var root = doc.Root; if (root is null) return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);As per coding guidelines,
tools/**: "Internal tooling — config file generators, build helpers. Lower bar than production src/, but still flag obvious bugs (missing null check that crashes the build, hardcoded paths, swallowed exceptions)."🤖 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 `@tools/ConfigFilesGenerator/Program.cs` around lines 489 - 498, In LoadMsBuildProperties, guard the XDocument.Load(versionPropsPath) call so a malformed or missing Version.props does not crash the generator: wrap the load in a try/catch(Exception ex), on exception write a warning including ex.Message to stderr (e.g., Console.Error) and return an empty Dictionary<string,string>(StringComparer.OrdinalIgnoreCase); keep existing null-root handling intact; this ensures failures in XDocument.Load/versionPropsPath are logged and the method falls back safely.
🤖 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 `@Directory.Build.props`:
- Around line 41-47: The ItemGroup is guarded by the wrong MSBuild property;
change the property that defines the opt-out/default from
EnableRepoDogfoodEditorConfigs to the public contract
EnableEditorConfigDogfooding so downstream consumers can opt out as intended:
update the <PropertyGroup> default definition and the ItemGroup Condition that
currently reference EnableRepoDogfoodEditorConfigs to use
EnableEditorConfigDogfooding, keeping the same default value/logic and the
EditorConfigFiles Include/Exclude entries unchanged.
---
Duplicate comments:
In `@tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cs`:
- Line 1: Remove the helper-level CA2000 suppression and the
System.Diagnostics.CodeAnalysis using; stop routing disposal through
CreateProject(...) by inlining the builder chain at each await using var project
= ... call site in this file: replace calls that use CreateProject(...) with
SdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...)
directly so the project is created and disposed at the ownership boundary, then
delete the suppression attribute and unused import; ensure every location that
previously called CreateProject(...) now constructs the project inline so
analyzers see correct disposal.
In `@tests/ANcpLua.Sdk.Tests/SdkTests.cs`:
- Line 1: Tests still rely on the shared helper that suppresses CA2000; replace
each usage like "await using var project = CreateProject(...);" in SdkTests.cs
with explicit ownership by calling
SdkProjectBuilder.Create(...).WithDotnetSdkVersion(...).RecordProperties(...)
directly at the test call sites so the test owns disposal, then remove the
helper suppression and the System.Diagnostics.CodeAnalysis using. Update all
occurrences (including the ones around lines 41–46) to use the SdkProjectBuilder
fluent chain and delete the unnecessary suppression/import.
In `@tests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cs`:
- Line 1: CreateProject() is hiding ownership transfer and keeps CA2000
suppressed; replace calls to SdkProjectBuilder.Create(...) by inlining its
object construction directly at each await using site in
SourceGeneratorDefaultsTests (remove the helper CreateProject method and its
using/import of SuppressMessage), ensure each disposable resource is created in
the await using statement so ownership is explicit (e.g., await using var
builder = new SdkProjectBuilder(...)), and delete the SuppressMessage/using
System.Diagnostics.CodeAnalysis import so CA2000 is no longer suppressed.
In `@tools/ConfigFilesGenerator/Program.cs`:
- Around line 489-498: In LoadMsBuildProperties, guard the
XDocument.Load(versionPropsPath) call so a malformed or missing Version.props
does not crash the generator: wrap the load in a try/catch(Exception ex), on
exception write a warning including ex.Message to stderr (e.g., Console.Error)
and return an empty Dictionary<string,string>(StringComparer.OrdinalIgnoreCase);
keep existing null-root handling intact; this ensures failures in
XDocument.Load/versionPropsPath are logged and the method falls back safely.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e74a574-0768-4d6d-b689-375af37d4aaf
📒 Files selected for processing (5)
Directory.Build.propstests/ANcpLua.Sdk.Tests/MtpDetectionTests.cstests/ANcpLua.Sdk.Tests/SdkTests.cstests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cstools/ConfigFilesGenerator/Program.cs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
tests/**/*.cs
⚙️ CodeRabbit configuration file
tests/**/*.cs: xUnit + FluentAssertions. AAA pattern. NSubstitute for mocks.
Flag: blocking-async patterns (Task.Run(() => x.Result)), missing
async Task(sync-void),Thread.Sleepinstead ofawait Task.Delay,
tests asserting on private state via reflection.
Files:
tests/ANcpLua.Sdk.Tests/MtpDetectionTests.cstests/ANcpLua.Sdk.Tests/SourceGeneratorDefaultsTests.cstests/ANcpLua.Sdk.Tests/SdkTests.cs
**/*.props
⚙️ CodeRabbit configuration file
**/*.props: MSBuild property files. Review for: condition guards on every override
(consumer must be able to opt out), no<RemoveAttributes>on items
that consumers may have set, and that defaults are sensible without
forcing a downstream cascade.
Files:
Directory.Build.props
tools/**
⚙️ CodeRabbit configuration file
tools/**: Internal tooling — config file generators, build helpers. Lower bar than
production src/, but still flag obvious bugs (missing null check that
crashes the build, hardcoded paths, swallowed exceptions).
Files:
tools/ConfigFilesGenerator/Program.cs
🪛 GitHub Actions: Claude Code Review / 0_claude-review.txt
tests/ANcpLua.Sdk.Tests/SdkTests.cs
[error] 1-1: No Datetime.Now/Utcnow check failed: three DateTime.Now instances lack required inline comments (policy RS0030).
tools/ConfigFilesGenerator/Program.cs
[error] 62-62: lint_config failed: Cannot use local variable 'CompilerAnalyzerConfigGlobalLevel' before it is declared.
[warning] 377-377: lint_config warning: Unreachable code detected.
🪛 GitHub Actions: Claude Code Review / claude-review
tests/ANcpLua.Sdk.Tests/SdkTests.cs
[error] 1-1: No Datetime.Now/Utcnow check failed: three DateTime.Now instances lack required inline comments (policy RS0030).
[warning] 1-1: No Suppression Of Fixable Diagnostics: 3 CA2000 suppressions added in test files not matching documented allowlist from .github/copilot-instructions.md.
tools/ConfigFilesGenerator/Program.cs
[error] 62-62: lint_config failed: Cannot use local variable 'CompilerAnalyzerConfigGlobalLevel' before it is declared.
[warning] 377-377: lint_config warning: Unreachable code detected.
The earlier reviewer guidance asked for the public opt-out switch to be EnableEditorConfigDogfooding, but the PropertyGroup and ItemGroup shipped with EnableRepoDogfoodEditorConfigs, so a consumer setting the documented name to false still got the repo editorconfigs injected. Rename both the default-definition and the ItemGroup Condition to the contract name. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The agent ran but didn't make any changes. The issues may already be fixed or require manual intervention. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
| static async Task ListAllPackageDependencies( | ||
| PackageIdentity package, |
ListAllPackageDependencies's early-exit was 'dependencies.Contains(package)' where dependencies is HashSet<SourcePackageDependencyInfo> and package is PackageIdentity. The call did compile (IEnumerable<T> covariance lets the LINQ Contains extension bind with TSource=PackageIdentity), but it silently degraded to an O(n) linear scan because the set's hash bucket lookup is keyed on SourcePackageDependencyInfo rather than PackageIdentity, and intent was muddled. Add a separate HashSet<PackageIdentity> with PackageIdentityComparer.Default; the early-exit is now an explicit O(1) hash lookup with the right comparer, and the existing HashSet<SourcePackageDependencyInfo> stays focused on accumulating the resolved dependency infos for downstream use. Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
@coderabbitai autofix |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
ANcpLua
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
Dismissing stale bot CHANGES_REQUESTED review: subsequent commits addressed the actionable items and the publish workflow is green (lint_config + tests).
Summary\n- dogfood generated analyzer configs through this repo's own projects\n- assign deterministic global_level values for generated analyzer config files\n- pin NetAnalyzers config generation to the repo's DotNetSdkVersion when available\n\n## Verification\n- dotnet test --project tests/ANcpLua.Sdk.Tests/ANcpLua.Sdk.Tests.csproj --filter-class 'TemplatesTests' --filter-class 'MtpDetectionTests' --filter-class 'SdkTests' --filter-class 'SourceGeneratorDefaultsTests'
Need help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit
Chores
Tests