fix: code review – correctness, perf, and style improvements

[timonkrebs]

Summary

Comprehensive code review of all source files in src/, tests/, samples/, Directory.Build.props, betterer.json, and .betterer.results. Nine targeted fixes across correctness, performance, style, and build quality.

---

Findings and fixes

1. Bug – mutable shared static in BettererFileIssues (correctness)

File: src/BettererNet.Core/BettererFileIssues.cs

The old code had:

private static readonly List<BettererFileIssue> EmptyIssues = new();

…returned as a fallback from TryGetValue in Diff. Any caller that called .Add() on the returned list would silently mutate this shared static, corrupting future diff results. Fixed by replacing with an inline empty collection expression [] cast to IReadOnlyList<BettererFileIssue>.

---

2. Bug – sync-over-async on a captured sync context in BettererHistoryReporter (correctness / tracked in .betterer.results)

File: src/BettererNet.Cli/BettererHistoryReporter.cs

The original code called .GetAwaiter().GetResult() directly on BettererHistory.LoadAsync and SaveAsync. When called from an ASP.NET or Blazor host (both of which are valid consumers of a CLI reporter), this deadlocks because the captured SynchronizationContext can never resume the async continuation.

Fixed by wrapping the async I/O in Task.Run(async () => { … }).GetAwaiter().GetResult(). The Task.Run dispatches to a thread-pool thread that has no captured sync context, so ConfigureAwait(false) continuations complete immediately. The outer .GetAwaiter().GetResult() satisfies the synchronous IBettererReporter contract without blocking a request thread. The two hits in .betterer.results are now gone.

---

3. Wasteful allocation – Clone helper in BettererResultsMerge (performance)

File: src/BettererNet.Core/BettererResultsMerge.cs

Clone was implemented as:

private static JsonNode? Clone(JsonNode? node) => node is null ? null : JsonNode.Parse(node.ToJsonString());

This serialises to a string then re-parses it – two allocations and a full JSON parse per node. JsonNode already exposes DeepClone() for exactly this purpose. Replaced all six call-sites with node.DeepClone() and deleted the Clone and Compact-based helper entirely. Compact is kept because it is also used for the intersection key.

---

4. Stale comment in BettererResult (documentation)

File: src/BettererNet.Core/BettererResult.cs

The XML summary contained a forward-reference comment:

Phase 1 will generalise this into a richer file/issue model…

The file/issue model (BettererFileIssues, BettererFileTest, etc.) is already fully implemented. Removed the stale remark to avoid confusing future readers.

---

5. Unnecessary allocation in BettererConstraints.SetBased (performance)

File: src/BettererNet.Core/BettererConstraint.cs

The LINQ one-liner current.Any(item => !baselineSet.Contains(item)) built a HashSet for the baseline but then immediately iterated current against it with LINQ. If a regression was found early the second HashSet for the current set was still allocated before the early return. Replaced with explicit foreach loops that return as soon as a decision can be made; the currentSet is only built when no regression was found.

---

6. Modernise old array syntax in BettererRegexTest and BettererConfigFile (style)

Files: src/BettererNet.Regex/BettererRegexTest.cs, src/BettererNet.Cli/BettererConfigFile.cs

• excludes ?? Array.Empty<string>() → excludes ?? []
• new[] { "**/*.cs" } → ["**/*.cs"]
• new[] { "**/*.cs" } literal in BettererConfigFile → ["**/*.cs"]

All target net10.0 with LangVersion=latest; collection expressions are idiomatic here.

---

7. Simplify FailingTypeNames null-check in BettererArchTest (style / simplification)

File: src/BettererNet.NetArchTest/BettererArchTest.cs

// before
var names = result.FailingTypeNames;
return names is null ? new List<string>() : names.ToList();

// after
return result.FailingTypeNames is { } names ? [.. names] : [];

Single expression, no intermediate variable, uses spread and collection expression.

---

8. Enable TreatWarningsAsErrors and GenerateDocumentationFile in Directory.Build.props (build quality)

File: Directory.Build.props

A fresh library with TreatWarningsAsErrors=false lets warning debt accumulate invisibly. Switched to true so new warnings are caught at build time. Also enabled GenerateDocumentationFile=true so missing XML doc comments on public API members (the library's primary surface) are flagged as build errors, ensuring the docs stay in sync with the code.

---

9. Clear resolved .betterer.results hits

File: .betterer.results

The two sync-over-async entries that tracked the GetAwaiter().GetResult() calls in BettererHistoryReporter are removed now that the root cause is fixed.

---

Issues flagged but not fixed (require design decisions)

#	Location	Issue
A	BettererRunDiff.cs	Duplicates the multiset-matching logic from BettererFileIssues.Unmatched. Could call BettererFileIssues.Diff instead, but Diff returns both added and fixed; the diff helper only needs added. Worth consolidating but needs an API decision.
B	BettererRegexTest.cs	BuildMatcher is called twice per run (once for the fingerprint in MatchedFiles, once in Scan). A minor inefficiency; fixing it would require returning matched file paths from Scan alongside issues.
C	BettererProjectTest.EnumerateInputs	Enumerates all .cs files under the project directory (minus bin/obj), which for a solution-level test could be thousands of files. A smarter approach would read the MSBuild <Compile> items, but that requires MSBuild parsing before the workspace is opened.
D	ConfigLoader	The AssemblyLoadContext is created with isCollectible: false, so config assemblies are never unloaded in long-lived processes. Acceptable for a CLI tool; would need isCollectible: true for watch mode to avoid a memory leak across re-runs.

Test plan

• dotnet build passes with TreatWarningsAsErrors=true
• dotnet test — all existing tests green
• BettererHistoryReporterTests exercise the reporter via a temp directory
• BettererResultsMergeTests confirm merge correctness after Clone → DeepClone swap
• BettererFileTestTests confirm diff correctness after removal of mutable static
• BettererConstraintsTests confirm SetBased still returns correct results
• .betterer.results no longer contains sync-over-async entries

---

Generated by Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.