Skip to content

Fix Sonar analyzer findings - #55

Merged
AriusII merged 1 commit into
mainfrom
fix/client-sonar-analyzer-findings
Sep 21, 2026
Merged

AriusII merged 1 commit into
mainfrom
fix/client-sonar-analyzer-findings

Conversation

@AriusII

@AriusII AriusII commented Sep 21, 2026 •

Copy link
Copy Markdown
Collaborator

Cause

The post-merge Sonar run 35645780134 reported one S8949 finding in LuaModuleLifecycle because module registration omitted its available cancellation token, and four S3903 findings because the benchmark helper types were declared in the global namespace.

Fix

  • Forward ICheatEngineClient.Stopping into ILuaClient.RegisterModule, so activation-time module registration observes shutdown cancellation before it reserves or dispatches work.
  • Keep the top-level benchmark entrypoint intact while moving BenchmarkEntryPoint, BenchmarkArtifactsDirectory, BenchmarkInvocation, and BenchmarkSummaryExtensions into CheatEngine.Client.Benchmarks; discovery still targets the same executing assembly.
  • Add a focused regression test that captures and asserts the exact stopping token passed to registration.

Validation

  • dotnet restore CheatEngine.Client.slnx --locked-mode
  • dotnet build CheatEngine.Client.slnx --configuration Release --no-restore --no-incremental --disable-build-servers --warnaserror (0 warnings, 0 errors)
  • MTP test suite: 554 passed, 0 failed, 0 skipped; includes LuaModuleLifecycleForwardsTheClientStoppingTokenToRegistration
  • dotnet pack CheatEngine.Client.slnx --configuration Release --no-build --no-restore
  • ./eng/Invoke-PackageSmoke.ps1 -PackageSource ./artifacts/packages
  • ./eng/Invoke-TemplateSmoke.ps1 -PackageSource ./artifacts/packages
  • Native AOT win-x64 publish and probe execution

Local Sonar submission was not possible because neither SONAR_TOKEN nor dotnet-sonarscanner is available in the workstation environment; the PR Sonar workflow remains the authoritative analysis.

Summary by CodeRabbit

  • Bug Fixes

    • Lua modules now correctly follow the client’s stopping lifecycle, ensuring their registrations are released when the client shuts down.
    • Improved shutdown handling helps prevent module resources from remaining active after client termination.
  • Tests

    • Added coverage to verify that the client’s stopping signal is forwarded during Lua module registration.

@coderabbitai

coderabbitai Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 905afbc1-babf-425c-809b-a87e563ae76d

📥 Commits

Reviewing files that changed from the base of the PR and between bd1d105 and b945eaa.

📒 Files selected for processing (3)
  • libs/CheatEngine.Client.Extensions.DependencyInjection/LuaModuleLifecycle.cs
  • tests/CheatEngine.Client.Benchmarks/Program.cs
  • tests/CheatEngine.Client.Extensions.DependencyInjection.Tests/CheatEngineClientServiceCollectionExtensionsTests.cs

Limit details: You’ve used all 10 included reviews currently available.


📝 Walkthrough

Walkthrough

The change forwards the client stopping token to Lua module registration, adds test coverage for that behavior, and updates benchmark discovery to use a namespaced entry point.

Changes

Lua module lifecycle

Layer / File(s) Summary
Stopping token registration
libs/CheatEngine.Client.Extensions.DependencyInjection/LuaModuleLifecycle.cs, tests/CheatEngine.Client.Extensions.DependencyInjection.Tests/CheatEngineClientServiceCollectionExtensionsTests.cs
LuaModuleLifecycle.OnEnabled passes client.Stopping to module registration. Tests record and verify the token. TestClient.Stopping is now configurable.

Benchmark entry-point discovery

Layer / File(s) Summary
Namespaced benchmark entry point
tests/CheatEngine.Client.Benchmarks/Program.cs
The benchmark entry point is placed in the CheatEngine.Client.Benchmarks namespace. Discovery uses BenchmarkEntryPoint to load the assembly.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the pull request, which addresses Sonar analyzer findings through code changes and a regression test.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@AriusII
AriusII merged commit afc0978 into main Sep 21, 2026
6 checks passed
@AriusII
AriusII deleted the fix/client-sonar-analyzer-findings branch September 21, 2026 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant