Eliminate all build warnings and unblock the test suite on Windows 11 23H2 - #154
Open
jafin wants to merge 4 commits into
Open
Eliminate all build warnings and unblock the test suite on Windows 11 23H2#154jafin wants to merge 4 commits into
jafin wants to merge 4 commits into
Conversation
Replace == string comparisons with string.Equals(..., StringComparison.Ordinal) to satisfy MA0006, and drop a duplicate 'using System;' (CS0105).
Nullability: align IRegistryRead.GetValue with its nullable implementation (CS8766); use [AllowNull] on the Control.Text overrides in MrngIpTextBox and CommandButton (CS8765/CS8764); match the base TypeConverter.CanConvertFrom signature (CS8604); return string.Empty rather than default from the RDM CSV tuple, which callers already test with IsNullOrEmpty (CS8619); add null guards in DataTableSerializer, PortableSettingsInitializer, RdpProtocol8, PuttyBase and XmlConnectionsDeserializer; drop a [NotNull] on an unassigned parameter (CS8777). Analyzers: Environment.CurrentManagedThreadId (CA1840); System.Threading.Lock (MA0158); CultureInfo-aware StringBuilder.Append (CA1305); explicit discards on BCrypt cleanup results (CA1806); log instead of swallowing in a best-effort hit-test (RCS1075). Dead code: remove the unreferenced FrmMain.UserInterfaceResize event and the BackupPage._frmMain field, whose only uses sit in a commented-out block; the never-raised ICredentialRecord.PropertyChanged on PlaceholderCredentialRecord keeps its normal semantics behind a scoped CS0067 suppression. Pkcs5S2KeyGenerator moves to the static Rfc2898DeriveBytes.Pbkdf2 overload, clearing SYSLIB0060 and CA5379. SHA1 is retained deliberately - it is required to decrypt existing connection files - and the derived bytes are unchanged.
The package is supplied by the targeted framework, so the explicit PackageReference only produced NU1510; no source in the repo uses the System.DirectoryServices namespace. Removing it forced a real recompile of ExternalConnectors, which surfaced an unused designer field (CS0169).
SupportedOSPlatformVersion was pinned at 10.0.26100.0 - stricter than mRemoteNG.csproj's 10.0.17763.0 - so the test host refused to run on anything older than Windows 11 24H2, reporting 'Only supported on Windows10.0.26100.0' and zero executed tests. Aligning it with the app lets the suite run on 23H2 (10.0.22631) and older; no CA1416 warnings result. Also set Nullable=annotations so tests may use 'T?' without enabling flow analysis warnings across the project (CS8632).
PR Summary by QodoEliminate build warnings and unblock test runs on Windows 11 23H2
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
There was a problem hiding this comment.
Pull request overview
This PR targets build/test hygiene across mRemoteNG by removing compiler/analyzer warnings, aligning test project platform settings so tests can execute on older Windows 11 builds, and applying small correctness/robustness tweaks in a few runtime components (notably logging, XML decryption, and key derivation).
Changes:
- Aligns
mRemoteNGTestsplatform/nullable settings to allow the test host to run on Windows 10.0.17763+ while enabling annotations-only nullability. - Eliminates warning sources via small API- and nullability-focused edits (e.g., string comparisons, nullability annotations, safer DataTable primary key setup).
- Updates crypto and threading-related code paths to use newer framework APIs (
Rfc2898DeriveBytes.Pbkdf2,System.Threading.Lock) while preserving existing behavior.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| mRemoteNGTests/mRemoteNGTests.csproj | Lowers SupportedOSPlatformVersion and enables Nullable=annotations to allow tests to run on older Windows builds. |
| mRemoteNGTests/Config/Serializers/MiscSerializers/MobaXTermSessionDeserializerTests.cs | Replaces string equality checks with string.Equals(..., Ordinal) to satisfy analyzers. |
| mRemoteNGTests/Config/Serializers/MiscSerializers/MicrosoftRdClientBackupDeserializerTests.cs | Same analyzer-driven ordinal string comparisons. |
| mRemoteNGTests/Config/Serializers/ConnectionSerializers/Json/JsonConnectionsSerializerTests.cs | Removes duplicate using System; (CS0105). |
| mRemoteNG/UI/TaskDialog/CommandButton.cs | Adjusts Text override nullability to match base while still allowing null assignment. |
| mRemoteNG/UI/Forms/OptionsPages/BackupPage.cs | Removes an unused field. |
| mRemoteNG/UI/Forms/frmMain.cs | Adds guarded dev logging for a hit-test exception path; adjusts formatting call and removes an unused event. |
| mRemoteNG/UI/Controls/mrngIpTextBox.cs | Allows null assignment to Text override without making the property nullable. |
| mRemoteNG/UI/Controls/Adapters/CredentialRecordListAdaptor.cs | Removes [NotNull] from event sender parameter to match event handler conventions. |
| mRemoteNG/Tools/WindowsRegistry/IRegistryRead.cs | Updates registry read API nullability for name/return value. |
| mRemoteNG/Tools/MiscTools.cs | Fixes override nullability signature for TypeConverter.CanConvertFrom. |
| mRemoteNG/Security/KeyDerivation/Pkcs5S2KeyGenerator.cs | Uses the static PBKDF2 API to eliminate obsoletions/analyzer warnings while keeping SHA1 for compatibility. |
| mRemoteNG/Credential/PlaceholderCredentialRecord.cs | Uses a scoped pragma to suppress an unused event warning without changing runtime event semantics. |
| mRemoteNG/Connection/Protocol/VNC/VncDesHelper.cs | Explicitly ignores cleanup return codes in finally to satisfy analyzers. |
| mRemoteNG/Connection/Protocol/RDP/RdpProtocol8.cs | Avoids potential nullability warnings in logging output. |
| mRemoteNG/Connection/Protocol/PuttyBase.cs | Applies null-forgiveness to satisfy nullability analysis for VaultOpenbao secret engine access. |
| mRemoteNG/Config/Settings/Providers/PortableSettingsInitializer.cs | Uses null! to satisfy nullability where an API requires a non-null argument but null is used by design. |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Xml/XmlConnectionsDeserializer.cs | Makes full-file decrypt assignment paths null-tolerant and preserves protective “empty tree” safety behavior. |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Sql/DataTableSerializer.cs | Throws a clear exception if the expected primary key column is missing. |
| mRemoteNG/Config/Serializers/ConnectionSerializers/Csv/RemoteDesktopManager/CsvConnectionsDeserializerRdmFormat.cs | Normalizes tuple return value to string.Empty instead of a null default. |
| mRemoteNG/Config/Connections/Multiuser/RemoteConnectionsSyncronizer.cs | Switches timer synchronization to System.Threading.Lock and keeps locking semantics localized. |
| mRemoteNG/App/DevLog.cs | Switches to System.Threading.Lock and uses Environment.CurrentManagedThreadId for logging. |
| ExternalConnectors/ExternalConnectors.csproj | Removes the redundant System.DirectoryServices package reference. |
| ExternalConnectors/CPS/CPSConnectionForm.Designer.cs | Removes an unused designer field. |
Files not reviewed (1)
- ExternalConnectors/CPS/CPSConnectionForm.Designer.cs: Generated file
Code Review by Qodo
1. GetValue missing XML param docs
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| private readonly System.Timers.Timer _updateTimer; | ||
| private readonly IConnectionsUpdateChecker _updateChecker; | ||
| private readonly object _timerLock = new(); | ||
| private readonly System.Threading.Lock _timerLock = new(); |
There was a problem hiding this comment.
1. _timerlock violates field naming 📘 Rule violation ⚙ Maintainability
Modified non-public field names use leading underscores, violating the camelCase naming rule for non-public fields and leaving touched code non-compliant with required naming conventions. This change introduces/retains additional non-compliant naming in the updated declarations.
Agent Prompt
## Issue description
The modified non-public fields `_timerLock` and `_lock` use leading underscores, which violates the camelCase naming requirement for non-public fields.
## Issue Context
These fields were modified in this PR (type changed), so they should be brought into compliance while touched to avoid introducing or retaining naming-rule violations.
## Fix Focus Areas
- mRemoteNG/Config/Connections/Multiuser/RemoteConnectionsSyncronizer.cs[18-18]
- mRemoteNG/App/DevLog.cs[14-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Gets the value of a registry entry specified by its name. | ||
| /// </summary> | ||
| string GetValue(RegistryHive hive, string path, string name); | ||
| string? GetValue(RegistryHive hive, string path, string? name); |
There was a problem hiding this comment.
2. getvalue missing xml param docs 📘 Rule violation ⚙ Maintainability
The modified public interface method GetValue lacks required XML documentation elements for its parameters and return value. This makes the public API documentation incomplete and non-compliant.
Agent Prompt
## Issue description
The public API member `IRegistryRead.GetValue(...)` is missing required XML documentation (`<param>` for each parameter and `<returns>` for the return value).
## Issue Context
The signature was modified in this PR (return type and `name` nullability), so its XML docs must meet the rule requirements.
## Fix Focus Areas
- mRemoteNG/Tools/WindowsRegistry/IRegistryRead.cs[13-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+70
to
+72
| // Cleanup status is intentionally ignored — nothing actionable can be done in a finally block. | ||
| if (hKey != 0) _ = BCryptDestroyKey(hKey); | ||
| if (hAlg != 0) _ = BCryptCloseAlgorithmProvider(hAlg, 0); |
There was a problem hiding this comment.
3. Bcrypt calls outside security/ 📘 Rule violation ⛨ Security
The modified cryptographic cleanup code in VncDesHelper calls BCrypt APIs directly from outside the Security/ namespace, rather than delegating to an approved security provider. This violates the project’s required crypto centralization pattern.
Agent Prompt
## Issue description
Crypto-related operations are being performed directly in `mRemoteNG/Connection/Protocol/VNC/VncDesHelper.cs` (outside the `Security/` namespace). The compliance rule requires crypto/credential handling to be invoked via approved providers under `Security/`.
## Issue Context
This PR modified the BCrypt cleanup calls (discarding return values) and added commentary, so the touched crypto code path must comply with the required provider pattern.
## Fix Focus Areas
- mRemoteNG/Connection/Protocol/VNC/VncDesHelper.cs[47-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
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.
Clears every compiler and analyzer warning in the solution (the build now reports 0 warnings) and fixes a test-project setting that made the suite unrunnable on any OS older than Windows 11 24H2.
Commits
chore(tests)—string.Equals(..., StringComparison.Ordinal)for MA0006; duplicateusing System;removed (CS0105).fix(build)— nullability, analyzer and dead-code fixes across mRemoteNG. See the commit body for the per-warning rationale.chore(deps)— drops theSystem.DirectoryServicesPackageReference (NU1510); the framework supplies it and no source uses the namespace. The forced recompile surfaced an unused designer field (CS0169).chore(tests)— lowersSupportedOSPlatformVersionfrom 10.0.26100.0 to 10.0.17763.0, and setsNullable=annotations.Notable decisions
Pkcs5S2KeyGeneratornow calls the staticRfc2898DeriveBytes.Pbkdf2overload, clearing SYSLIB0060 and CA5379. SHA1 is retained deliberately — it is required to decrypt existing connection files. Derived bytes are unchanged. This is the change most worth a careful review, since it sits on the key-derivation path for every stored password.SupportedOSPlatformVersion: the test project was pinned stricter than the app it tests (26100 vs the app's 17763). The test host therefore reportedOnly supported on Windows10.0.26100.0and executed zero tests on Windows 11 23H2. Aligning it with mRemoteNG.csproj produces no CA1416 warnings. CI onwindows-2025-vs2026is unaffected.Nullable=annotationsrather than per-file#nullable enablepragmas: it enables the annotation context without turning on flow-analysis warnings across a ~6.3k-test project.PlaceholderCredentialRecord.PropertyChangeduses a scoped#pragma warning disable CS0067instead of no-opadd/removeaccessors, so the event still behaves normally if the type ever starts raising it.Verification
build.ps1: exit 0, 0 warnings, 0 errors.Known failure, pre-existing
ConnectionsServiceStartupPathTests.StartupConnectionPathReturnsSavedPathWhenItIsTheSoleCandidatefails on my dev box. It callsGetStartupConnectionFileNamevia reflection, which performs live filesystem discovery; discovery finds a candidate beside the test binaries and prefers it over the injected temp path. The test's own comment anticipates this scenario, but the assertion is stricter than the comment.None of the files touched here are on that code path —
ConnectionsServiceandConnectionsFileResolverare untouched, and the one settings-adjacent edit (null→null!) is annotation-only with identical IL. Caveat: I have no local baseline, because the suite could not run on this machine before commit 4.Relatedly, the parallel group runner reports 2 failures where a single-process run reports 1 — the extra one is order-dependent, as this test mutates the static
OptionsConnectionsPage.Defaultsingleton.