Resolve the open SonarCloud issues across the codebase#99
Open
Reefact wants to merge 18 commits into
Open
Conversation
The single job already overrides permissions with the two write scopes it needs, so the workflow-level default only needs a minimal explicit read. Enumerating `contents: read` clears Sonar's S8234 (avoid `read-all`) without changing the analysis job's effective token. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
S1125 fires on the codebase-wide `x is false` style. It is ignored through sonar.issue.ignore.multicriteria rather than in .editorconfig (which by policy carries no rule severities) or at 72 identical call sites. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Every `case` gains a no-op `*)` branch (S131) and `err()` ends with an explicit `return 0` (S7682); behaviour is unchanged, verified against the conforming, non-conforming, merge, and autosquash paths. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
- ErrorDescription: drop the redundant null-conditional in a branch that already guarantees a value (S2589); the '!' states it for the netstandard2.0 compiler, which lacks the [NotNullWhen(false)] flow annotation. - ErrorDocumentationBuilder: rename the parameter to match the interface (S927) and delegate WithDiagnostic to AndDiagnostic rather than duplicating it (S4144). - ErrorContextKey (S4035, abstract so unsealable), AssemblyErrorDocumentationReader (S3011, intentional non-public reflection) and the Outcome assertion/extension overloads (S4136, grouped by receiver type): justified [SuppressMessage] attributes. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Pass the single diagnostic tag through the params argument instead of an explicit array (S3878); move the filtering into Where/Any (S3267); and split DuplicateDocumentedCodeAnalyzer.Collect so the nested factory scan lives in its own method, bringing cognitive complexity back under the limit (S3776). Behaviour is unchanged — the 51 analyzer unit tests pass. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Renderers and the solution generator: ArgumentNullException.ThrowIfNull for the guard clauses (CA1510); concrete List<T> return/local types where the interface added nothing (CA1859); filtering moved into Where/AddRange (S3267); a 'dotnet' constant (S1192); string.Contains and StartsWith(char) (CA2249, CA1865); and a merged guard (S1066). Worker: arguments are parsed in a local function (S3776; a while loop also drops the for-body index mutation, S127), the console/file writes are awaited (S6966), and the target is loaded via AssemblyLoadContext.Default.LoadFromAssemblyPath (S3885), resolved to an absolute path to preserve LoadFrom's relative-path behaviour. Verified end to end: the worker still extracts the 10 Usage errors, --culture included. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
The inline theme bootstrap and the page script now use the DOM dataset API instead of get/set/removeAttribute (S7761), for-of over the NodeLists instead of index loops (S4138), String.includes instead of indexOf (S7765), and carry an explanatory comment in the two intentionally empty catch blocks (S2486). Behaviour is unchanged: the emitted script was syntax-checked and the search filter re-exercised; the 16 HTML snapshots are regenerated to match. This clears the 176 JavaScript findings Sonar raised across the snapshot fixtures. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Program awaits app.RunAsync instead of the blocking Run (S6966). RendererCatalog caches the built-in format list so the property no longer rebuilds a collection on each read (S2365) and selects the custom renderer with FirstOrDefault (S3267). RendererLoader/Add/List keep Assembly.LoadFrom behind a justified [SuppressMessage] (S3885): Assembly.Load cannot load an assembly from a file path, and LoadFrom deliberately probes the plugin's own directory for its dependencies — behaviour AssemblyLoadContext.LoadFromAssemblyPath would drop. Verified: fce --help and 'config renderer list' run and list the built-in formats. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Delete the unused DeltaTemperature marker (S2094); give Amount, which implements IComparable, its comparison operators (S1210); and use string.Contains(char) in DocumentationFormatter (CA1847). The intentional minimal marker types (S2094) and the nested error-code holders that deliberately mirror their factory-method names (S3218) carry justified [SuppressMessage] attributes — the latter matching the // ReSharper disable MemberHidesStaticFromOuterClass already present on two of them. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Hoist the constant assembly-path arrays into static readonly fields so they are allocated once (CA1861), drop a redundant OrderBy over an already-sorted literal, and discard the results of the constructor calls that exist only to assert they throw (CA1806). Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
The 'x is false' idiom that S1125 flagged was machine-generated across the codebase, not a deliberate hand-written style, so it is converted to '!x' (71 call sites) rather than exempted. This supersedes the earlier sonar.yml ignore, which is removed. Conversion verified: full build clean and all 598 tests pass, so no condition was inverted. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Revert the comparison operators added for S1210: they were untested new surface on an intentionally minimal illustrative type that the suite only exercises indirectly. Instead, S1210 is silenced with a justified [SuppressMessage] explaining the example stays minimal. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
The 'foreach (x in seq.Where(pred)) { return x; }' shape introduced for S3267 always returns on the first match, so the loop ran at most once (S1751). Replace it with FirstOrDefault(pred), clearing both S3267 and S1751. Behaviour unchanged; full build and 598 tests stay green.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
The *.verified.* files are Verify snapshot oracles — generated test fixtures, not production source. Excluding them stops Sonar from analysing the emitted HTML/JS/Markdown as hand-written code (which raised S2486 on the renderer's intentionally-ignored localStorage catch) and from counting the near-identical snapshots as duplication. Standard treatment for generated fixtures, distinct from exempting a rule on real source. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
The theme-toggle script swallowed localStorage failures in an empty (comment-only) catch, which SonarJS still reports as an ignored exception (S2486) — a comment is not handling. Extract the write into a store() helper whose catch returns a success flag, so the exception is actually handled; the caller ignores the result because persistence is best effort. 16 HTML snapshots regenerated; behaviour verified (theme still applies when storage is unavailable). Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Converting the 'is false' idiom touched previously-untested lines (ConfigurationStore.Load's missing-file return and ConsoleAndFileOutputSink.WriteFile's directory creation), which dropped new-code coverage below the Quality Gate's 80%. Add focused tests that execute both branches, restoring coverage. ConfigShowCommand's guard stays uncovered: its Execute is protected and no test in the suite constructs a Spectre CommandContext. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe76eafb68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Load into the default context (not Assembly.LoadFrom): the worker already runs under the target's | ||
| // deps.json, so its dependency closure — the target's own FirstClassErrors included — resolves there. | ||
| // LoadFromAssemblyPath requires an absolute path (LoadFrom resolved relative paths against the cwd itself). | ||
| Assembly assembly = AssemblyLoadContext.Default.LoadFromAssemblyPath(Path.GetFullPath(assemblyPath)); |
There was a problem hiding this comment.
When users document a prebuilt assembly that has no sibling .deps.json (a supported path because RunWorker only adds --depsfile when the file exists), this switch drops the Assembly.LoadFrom behavior that registers resolution for dependencies next to the target DLL. A target whose documentation factories reference a co-located dependency can now fail during type loading/execution with missing dependency errors even though the same folder worked before; keep the LoadFrom path or add an explicit resolver for the target directory in the no-depsfile case.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in 204ce73. The worker now keeps Assembly.LoadFrom (restored via a LoadTarget helper) and documents the intent with a justified [SuppressMessage] for S3885, rather than changing the load semantics — the same treatment already used for the CLI renderer loaders.
Verified end-to-end against exactly this case: a prebuilt target with no .deps.json and a co-located dependency the worker does not itself carry. LoadFromAssemblyPath fails there with ReflectionTypeLoadException (the sibling never resolves); Assembly.LoadFrom resolves it via target-directory probing and the documented error extracts cleanly. Full build clean, 601 tests pass.
Generated by Claude Code
The S3885 fix swapped the worker's target load from Assembly.LoadFrom to AssemblyLoadContext.Default.LoadFromAssemblyPath. That drops LoadFrom's target-directory probing: when the generator runs the worker WITHOUT a --depsfile (the target ships no deps.json), a co-located dependency the worker does not itself carry no longer resolves, so GetTypes() throws ReflectionTypeLoadException and the reader drops those types. Restore Assembly.LoadFrom via a LoadTarget helper and document the intent with a justified [SuppressMessage] instead of changing the load semantics, matching the Cli renderer loaders. Assembly.Load is not an option: it resolves by name, not path, and cannot load a target given only its path. Verified end-to-end against a prebuilt target with no deps.json and a worker-unknown co-located sibling: LoadFromAssemblyPath fails to resolve the sibling (ReflectionTypeLoadException), LoadFrom resolves it and the documented error extracts cleanly. Full build clean, 601 tests pass. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Owner
Author
|
@codex any thoughts about the fix ? |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The snapshot tests lock the generated HTML byte-for-byte against an approved oracle, so a regression that makes the generator emit invalid markup — an unescaped '<', a broken attribute, a misnested or unclosed tag — would just be baked into a new snapshot and approved. Nothing actually asserts the output is well-formed. Add a gate that parses every generated HTML document (single, split pages, and the four localized renderings) with a strict HTML5 parser: any parse error fails the build. This also guards the inlined <style>/<script> islands, which SonarJS cannot see from the C# string constants that hold them (only from the rendered page, which the Sonar analysis excludes as a generated fixture). AngleSharp is added as a test-only dependency because the BCL has no HTML5 parser (a strict XmlReader chokes on HTML5 void elements and on the '<'/'&' inside inline CSS/JS). The gate is verified to have teeth: injecting an unescaped '<' into the renderer's Text() makes all six tests fail with HtmlParseException; reverting restores green. Strict mode is not over-strict either — every document currently emitted parses cleanly. 607 tests pass. Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
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
Resolves the SonarCloud findings on the project: every unresolved issue reported by the analysis is fixed, either in code or behind a justified in-source suppression. One Quality Gate condition is still open — new-code coverage — which is why this PR is raised, so the coverage is measured on the real diff rather than against the (temporarily polluted)
mainbaseline. See the note under Testing.Type of change
Changes
x is falseidiom with!xacross the codebase (S1125) and drop the temporary analysis-side ignore that had been added for it.dataset,for-of,String.includes, and a realcatchhandler instead of an empty one (S7761 / S4138 / S7765 / S2486); the 16 Verify HTML snapshots are regenerated to match.ArgumentNullException.ThrowIfNull, concreteListreturn/local types,FirstOrDefault/Where/AddRangeover hand-written loops,string.Contains/StartsWith(char), adotnetconstant, merged guards, and reduced cognitive complexity.await …Async, a cached property, sealed / parameter / dedup fixes, a discard in a property test, …) plus justified[SuppressMessage]for the genuinely intentional patterns (non-public reflection in the doc reader, minimal marker types, receiver-grouped overloads, path-basedAssembly.LoadFromfor plugins and the doc worker, the minimalAmountexample).scorecard.ymlleast-privilege permissions (S8234, the only reported vulnerability), default cases + explicit return in the commit-lint script (S131 / S7682), and exclusion of the Verify snapshot oracles from analysis.Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.sln(601 tests pass)FirstClassErrors.Analyzers.UnitTests)On-demand branch scans confirm 0 unresolved SonarCloud issues, all ratings A, and the duplication and security-hotspot-review conditions met. The open Quality Gate item is new-code coverage: converting the
is falseidiom touched ~36 lines in pre-existing untested code paths (CLI command glue whoseExecuteisprotected, plugin loading, and the generator's external-process orchestration). This PR exists so that coverage is computed on the actual diff; how far to take the added tests is left to review.Documentation
🤖 Generated with Claude Code
https://claude.ai/code/session_01McivG9boVrcWekrYwLBZUZ
Generated by Claude Code