Answer the remaining SonarQube findings, rule by rule - #374
Merged
Conversation
Three rules account for 191 of the 255 findings left in the Sonar report, and all three arrive under external_roslyn: they are compiler and BCL analyzer diagnostics that the scanner republishes, so a severity of none removes them at the source rather than dismissing them in the server's UI. Each is declined for a reason the analyzer cannot see. CA1859 reads IReadOnlyList<string> as an oversight where it is the contract, and honouring it would hand .Add() to every caller to buy nanoseconds on error-message paths. CA1861 would move the expected values of an assertion away from the check that reads them, to save an allocation occurring a few hundred times in a suite. IDE0028 asks for 147 edits across 13 projects that change no behaviour, on a codebase already split 85 to 147 between the two spellings — applying it would be adopting a convention, not fixing a defect. Scope follows the reason. CA1861's argument is about tests and all 22 of its findings sit in test projects, so it is declined under *Tests/ only and stays live for shipping code, where a hot path can genuinely want it. The other two are declined repository-wide, their justifications holding everywhere they fire. Verified on JustDummies.UnitTests through the SARIF log the scanner reads: 78 diagnostics before (64 IDE0028, 9 CA1859, 5 CA1861), 0 after, and the other 12 findings in that project untouched. The full solution still builds at zero warnings. ADR-0060 records the policy the three share: stated intent outranks generic analyzer advice, and readability outranks micro-performance unless a measured need says otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
Sonar reports the workflow-level `permissions: contents: read` in both mutation
workflows as githubactions:S8264, typed VULNERABILITY. They are the only two
vulnerabilities in the report and the only findings standing between the project
and a green Quality Gate.
The rule is right here rather than merely loud. A workflow-level grant reaches
every job, including any added later that does not need it, and both workflows
already contradicted their own floor: the advisory `gate` job declares
`permissions: {}` precisely because it checks nothing out. The floor was
therefore granting read access to one job that had gone to the trouble of
refusing it.
Each job now states the narrowest scope it can — `contents: read` for `changed`
and `full`, which check the repository out, and the unchanged `{}` for `gate` —
and the header comment records that a job added later must carry its own block,
since without one it inherits the repository default rather than a floor set in
the workflow.
The sixteen other workflows keep their workflow-level declaration: Sonar does not
flag them, their jobs all needing the scope they are granted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
`skip` already encapsulated the `applied=false` side of the workflow's output contract, but its counterpart was written out at each of the four branches that succeed — the literal `applied=true` four times, and the `impact=`/`summary=` keys with it (Sonar shelldre:S1192). The asymmetry was the defect: one side of the same contract had a name, the other did not. `applied <impact> <summary>` gives it one. Both keys the workflow reads are now written in exactly two places, and each case branch ends by saying what it did rather than by restating the output format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
S7682 and S7679 account for 21 of the remaining findings, all of them in the repository's shell tooling and Claude hooks. Unlike the Roslyn rules declined in .editorconfig, these are SonarQube's own analysis rather than build diagnostics the scanner republishes, so nothing the compiler does can reach them; they are ignored in the scanner invocation instead, which is the only place that can carry the refusal. S7682 asks for an explicit return at the end of a shell function. Every function it flags ends with the command whose exit status IS the function's result — a cat heredoc, an awk invocation, a printf — so `return 0` would mask those failures and a bare `return` restates the default. One of them ends with `exit`, after which a return is unreachable at all. S7679 asks that a positional parameter be assigned to a local variable. Every script here declares #!/bin/sh and `local` is not POSIX. tools/trains.sh already shows the price of obeying without it: the one helper needing named parameters carries _tf_-prefixed globals. Paying that in every two-line helper, to name a $1 sitting one line below the function's own name, buys nothing. ADR-0060 covers both under the policy it already stated, and now records where each family of refusal is written and why the two cannot share a home. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
The report's largest group, 147 IDE0028 findings across 13 projects, marked a drift rather than a preference: the codebase spelled collection initializers both ways, 85 sites in brackets against 147 in `new()`, with both forms inside the same file — JustDummies.UnitTests alone held 32 of the first and 64 of the second. Declining the rule would not have stopped that; it would have removed the only thing able to stop it, which is exactly how the explicit-type rule drifted to 203 violations while it lived only in the DotSettings (ADR-0055). So the rule is honoured instead, and .editorconfig gains nothing: at its default severity IDE0028 already fires on every regression, which is what produced these 147 in the first place. Drift is caught by the report rather than blocked at the build, as it is for every rule but the explicit-type one. Applied with `dotnet format style --diagnostics IDE0028`, iterated to a fixed point — the Roslyn fixer, not a text substitution, so target-typed and spread forms are rewritten by the compiler's own rules. Two statements collapse into one where the fixer could see a `new()` followed by `AddRange`. Verified: IDE0028 down to zero in the three projects that carried 107 of the 147, through the SARIF log the scanner reads; the solution builds at zero warnings and the full suite passes, 1960 tests. The net472 floor leg builds too — collection expressions are a C# 12 compiler feature, not a BCL one, so the netstandard2.0 and .NET Framework 4.7.2 targets are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
IDE0028 is applied rather than declined, so it leaves the ADR: what remains is CA1859, CA1861, S7682 and S7679 — 65 findings whose common trait is that the code they flag is deliberate, not defective. Dropping it costs the ADR its largest number and improves it. The rationale now says outright that volume is not a reason to decline a rule, and cites IDE0028 as the case that proves it: the biggest group in the report and the cheapest to silence, applied because its findings marked a real drift. An ADR that had declined 147 findings on cost would have been quotable as licence to decline the next inconvenient rule on the same ground. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
SYSLIB1045 asks that every `new Regex("...")` become a partial method carrying
[GeneratedRegex], so a source generator emits the automaton at compile time. The
attribute arrived in .NET 7. All 7 findings are in JustDummies.UnitTests, whose
floor leg is net472 (ADR-0022), and the three suites they sit in — AnyPatternTests,
XmlDocCrefConventionTests, ConcurrentDrawTests — are all compiled on that leg:
none of them appears in the project's net472 <Compile Remove> list.
So this is CA1510's wall again, and it takes CA1510's remedy: a NoWarn in the
one project that owns the constraint, stated next to a comment naming the floor,
rather than in Directory.Build.props or .editorconfig where it would also switch
the rule off for projects that can honour it. It expires the same way, the day
this project stops building against net472.
This is an application of ADR-0058, not a new decision, and not a case of
ADR-0060 either: the rule is not contradicted by deliberate code, it is
unsatisfiable on a target framework the project must still support.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
Six S1192 findings, four literals. Each was repeated four times in a place where the copies have to agree and nothing made them: the `breaking` fail-on policy, read once to default, once to validate, once to decide the exit code and once to word the error; the example assembly path the four worker-failure errors illustrate themselves with, which the generated documentation shows side by side; and the two benchmark fixture values every payload is built from, where a drifting literal would silently make two "comparable" benchmarks measure different work. Naming them is the point, not the count: a constant is the only thing that makes "the same value" a fact rather than a coincidence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
Fourteen findings asking for a newer BCL spelling. Each was checked against the support floor (ADR-0022) rather than taken or refused wholesale, because the floor decides them one at a time. Taken, ten of them. CA1864's two ContainsKey-then-Add pairs become TryAdd, in a net8.0-only project and with the "first entry wins" behaviour unchanged. CA2249 and CA1847 become Contains(char) at seven sites: measured, that binds to String.Contains(char) on the modern leg and to Enumerable.Contains<char> on net472, same ordinal answer, and both legs compile. Refused, four, each with the reason at the site. CA2249's remaining two want Contains(string, StringComparison), absent from netstandard2.0 — one of them in the shipped library, where IndexOf with the same StringComparison.Ordinal is the only spelling that exists. CA1865 wants StartsWith(char): measured, the net472 leg rejects it with CS1503, cannot convert from 'char' to 'string'. CA1870 wants SearchValues<T>, a .NET 8 type. CA2263's suppression only writes down what the call site already said about Enum.IsDefined<TEnum>. Verified on both legs: netstandard2.0 and net472 build, 1960 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
CA2231 and S1210 fire on three model types in the binder's Usage samples — code written to be read and copied. The gap they mark is real, and worse in a sample than it would be in a library. NightCount and RoomNumber are readonly structs implementing IEquatable. A struct gets no `==` from the language, so a reader who copies the pattern and writes `roomA == roomB` meets a compile error. BookingDate is worse: a sealed class implementing IEquatable and IComparable, where `==` DID compile and compared references while Equals compared values. Two instances of the same date answered differently depending on which the reader reached for. The operators are now declared on all three, delegating to Equals and CompareTo so the answers cannot diverge. BookingDate's four ordering operators share one private Compare, which is also the single place its null verdict — a null sorts before any date, as CompareTo already said — is written down. The project carries no public-API baseline, so nothing needs regenerating. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
xUnit1042 reports the Builders member as returning untyped rows: an IEnumerable<object[]> whose shape only has to match the theory's parameters at run time, so a row with the wrong arity or a swapped pair fails as a test error rather than a compile error. TheoryData<Type, string[]> states the shape once and the compiler checks every row against it. The twenty-four rows and their comments are unchanged, the net8-only block still sits behind its #if, and the suite still reports 632 tests — the same count as before, which is what says no row was lost in the conversion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
S3267 asks that the exclusion loop in AnyPattern.Excluding become a Where clause. It cannot: the loop body appends to the very list its condition reads, so a duplicate later in `values` is rejected against the values already taken. A Where clause reads as a filter over a fixed collection, which is exactly what this is not — and the two would not even agree on the result. S125 reports commented-out code in AnyString at a line that is an English sentence. The rule reads the trailing "(ADR-0045);" of the prose explaining the member's null handling as a statement. Nothing there is code. Both carry their reason in a SuppressMessage rather than being quietly deleted from the report, per ADR-0060: a refusal that is not written down is indistinguishable from an oversight the next time someone reads it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
Merging origin/main brought in JD007-JD022, written on the branch point where IDE0028 was still unanswered, so four of the new analyzer files reintroduced the `new()` spelling this branch had just finished removing. Same treatment as the rest — `dotnet format style --diagnostics IDE0028`, run to a fixed point — for four single-line initializers. Nothing else in the merged work needed touching, and the alignment of the surrounding declarations is left exactly as main wrote it. This is the drift the branch's own commit predicted: the rule fires at its default severity, so a regression is visible in the report rather than blocked at the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bxVrnCNsXW3RvwmLc9Kvy
Reefact
pushed a commit
that referenced
this pull request
Jul 29, 2026
The two rules share one walk over a ScalarConstraintState because they read the same state from opposite sides: JD023 asks whether anything remains, JD024 whether anything changed. JD023 reports an integer chain narrowed to nothing — an empty interval, an empty lattice, an emptied allow-list. The library computes this with one emptiness test; the rule runs the same test over the constants written at the call site and stays silent for every argument it cannot fold. JD024 is the only member of the constraint family the run time NEVER reports. Every other contradiction throws eventually and loudly, while an inert constraint leaves the test green exercising a domain the author did not write. The dangerous case is an exclusion of a sentinel the generator could never draw: it misses silently, and starts mattering the day someone widens the range. Info rather than Warning, because a defensive exclusion kept so the intent survives a future widening is a real and reasonable style. Dogfooding caught a genuine bug in the model, not a misread of the library. Any.Int64().LessThanOrEqualTo(long.MinValue) is legal and yields exactly that value, yet the first version declared it empty: the state used -long.MaxValue as its "unbounded" sentinel, which made long.MinValue unrepresentable. The bounds now run to the true extremes, and a bound genuinely beyond the range — GreaterThan(long.MaxValue) — is carried by an explicit saturation flag rather than by arithmetic that would overflow. Four tests pin the extremes in both directions. Scope is deliberately narrow: integer generators only, one expression, every argument folded. A constraint the model has never seen ends the walk rather than being guessed at — a rule claiming a chain is unsatisfiable has to be certain. Rebased onto main after #374, which carried the analyzer library to collection expressions. The two new files were written before that convention landed, so they are brought to it here the same way: `dotnet format style --diagnostics IDE0028`, run to a fixed point, five single initializers, nothing else touched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GKqyhG9Y4AfdxEYekP2Evq
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
2 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
Works through the 255 open SonarQube Cloud findings and answers each one rather than clearing the count: 249 are resolved, by complying where the advice is right and by declining — in writing, next to the reason — where the code it flags is deliberate or the support floor makes it impossible. The two findings blocking the Quality Gate are fixed.
Type of change
Changes
Quality Gate (2)
githubactions:S8264, the report's only two findings typed VULNERABILITY, in both mutation workflows. Permissions move from workflow level to per job. Both workflows already contradicted their own floor: the advisorygatejob declarespermissions: {}because it checks nothing out, while the workflow-level grant handed it read access anyway.Applied (164)
IDE0028(147, 13 projects) — collection initializers become collection expressions. These marked a genuine drift rather than a preference: the codebase spelled them both ways, 85 sites in brackets against 147 innew(), both forms inside the same file. Applied withdotnet format style --diagnostics IDE0028iterated to a fixed point — the Roslyn fixer, not a text substitution. Nothing is added to.editorconfig: the rule fires at its default severity, which is what produced these 147.CA2249/CA1847(7) —IndexOf(char) >= 0becomesContains(char).csharpsquid:S1192(7) — four literals repeated four times each become named constants, plus one inapply-fix.shwhere a helper now mirrors the existingskip.CA1864(2) —ContainsKey-then-AddbecomesTryAdd, "first entry wins" unchanged.CA2231/csharpsquid:S1210(3) — equality and ordering operators on the binder's sample value objects.BookingDatewas the reason: a class implementingIEquatableandIComparablewhere==compiled and compared references whileEqualscompared values.xUnit1042(1) — the surface-parityMemberDatasource becomesTheoryData<Type, string[]>.Declined, each with its reason at the point of effect (83)
CA1859(22) andCA1861(22) in.editorconfig,shelldre:S7682(12) andshelldre:S7679(9) in the scanner invocation — the four rules ADR-0060 records.CA1861is scoped to test projects only, so shipping code keeps the rule.SYSLIB1045(7) via a projectNoWarn, and 10 findings via[SuppressMessage]—CA2249×2,CA1865,CA1870×2,CA2263for the support floor;csharpsquid:S125andcsharpsquid:S3267because the rule is wrong at those two sites.Not in this PR
csharpsquid:S3776(6) — cognitive complexity 16 to 28, includingRegexParser.csat 28. Shipping code with real regression risk; it deserves its own PR and its own mutation attention.Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.slnFirstClassErrors.Analyzers.UnitTests)Zero warnings, 2092 tests passing across 13 test projects after merging
origin/main.Beyond the checklist, because most of this change turns on which BCL surface the support floor carries:
netstandard2.0andnet472legs were built explicitly (-p:EnableNet472Floor=true -f net472). Collection expressions are a C# 12 compiler feature, not a BCL one, so the floor is unaffected by the 147 rewrites.Contains(char)compiles onnet472— it binds toEnumerable.Contains<char>, same ordinal answer — which is why 7 sites were fixed rather than declined.StartsWith(char)was applied, observed to fail withCS1503: cannot convert from 'char' to 'string', and reverted.IDE0028result was verified through the SARIF log the scanner reads, filtering onsuppressionStates: 0 live diagnostics in the sampled projects.JustDummies.UnitTestsalone went from 90 findings to 0.Documentation
doc/updateddoc/handwritten/for-users/README.fr.md) updated if user-facing behavior changedADR-0060 is added in English and French, and indexed. No user-facing behaviour changes, so
README.fr.mdneeds no update; the shipped public API is untouched, so no baseline regeneration. The operators added toFirstClassErrors.RequestBinder.Usageare in a sample project, which carries no public-API baseline.Architecture decisions
Proposed: ADR-0060ADR-0060 — Let stated intent outrank generic analyzer advice. Two limbs: a generic rule yields where the code it flags is a deliberate expression of intent, and readability outranks micro-performance unless a measured need is recorded. Its rationale states explicitly that volume is not a reason to decline a rule, citing
IDE0028as the case that proves it — the largest group in the report and the cheapest to silence, applied instead.SYSLIB1045is an application of ADR-0058, not a new decision: the rule is not contradicted by deliberate code, it is unsatisfiable on a target framework the project must still support.Related issues
None.
Generated by Claude Code