dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

fix(analyzers): resolve Medium code-review findings (Analyzers-001, Analyzers-006)

Browse Source 
Analyzers-001: IsInsideWrapperLambda now matches the wrapper method name
(ExecuteAsync/ExecuteWriteAsync) in addition to the containing type, so a
future non-callSite lambda overload cannot suppress the diagnostic.
Analyzers-006: extended StubSources and added coverage for the remaining
guarded interfaces, synchronous members, concrete-driver receivers,
ExecuteWriteAsync wrapping, and nested lambdas.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-22 08:08:09 -04:00
parent a0aa4a4819
commit 9f5a5c9997
3 changed files with 460 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

10
code-reviews/Analyzers/findings.md
View File

@@ -7,7 +7,7 @@
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 7 |
| Open findings | 5 |
## Checklist coverage
@@ -33,13 +33,13 @@
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:135-139` |
| Status | Open |
| Status | Resolved |
**Description:** `IsInsideWrapperLambda` treats a guarded call as "wrapped" if it is textually inside ANY lambda that is an argument to ANY invocation whose containing type is `CapabilityInvoker` or `AlarmSurfaceInvoker`. It matches the containing type only, never the parameter the lambda is bound to. The real wrapping contract is specifically the `callSite` (`Func<CancellationToken, ValueTask>` / `Func<CancellationToken, ValueTask<T>>`) parameter of `CapabilityInvoker.ExecuteAsync` / `ExecuteWriteAsync`. Any other lambda argument to a method on those types — a future overload that takes a predicate/selector lambda, or a lambda passed in a non-`callSite` position — would suppress the diagnostic even though the guarded call is not actually executed inside the resilience pipeline. The analyzer's own XML doc (lines 21-23) describes exactly this looser-than-intended behaviour. It is a latent false-negative gap rather than an active bug because the current `CapabilityInvoker` surface has no non-`callSite` lambda parameter.
**Recommendation:** Resolve the symbol of the lambda argument's parameter (`IMethodSymbol.Parameters[i]`) and require its type to be the `Func<CancellationToken, ValueTask>` / `Func<CancellationToken, ValueTask<T>>` callsite shape, or at minimum match the wrapper method name (`ExecuteAsync` / `ExecuteWriteAsync`) rather than only the containing type. This closes the gap before a new overload silently widens the escape hatch.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — Replaced `WrapperTypes` string array with `WrapperMethods` (type FQN + method name) tuples so `IsInsideWrapperLambda` matches both containing type and method name, preventing future non-`callSite` overloads from silently suppressing the diagnostic.
### Analyzers-002
@@ -108,7 +108,7 @@
| Severity | Medium |
| Category | Testing coverage |
| Location | `tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs` |
| Status | Open |
| Status | Resolved |
**Description:** The test suite exercises only 3 of the 7 guarded interfaces (`IReadable`, `IWritable`, `ITagDiscovery`) and one positive / one negative lambda case. Significant untested behaviour for an analyzer that gates a repo-wide resilience invariant:
@@ -121,7 +121,7 @@
**Recommendation:** Extend `StubSources` with the remaining guarded interfaces and `AlarmSurfaceInvoker`, then add tests for: each remaining guarded interface (positive plus wrapped), a synchronous member not being flagged, a concrete driver-class receiver with a renamed implementing method, `ExecuteWriteAsync` wrapping, and a nested-lambda case.
**Resolution:** _(open)_
**Resolution:** Resolved 2026-05-22 — Extended `StubSources` with `ISubscribable`, `IAlarmSource`, `IHistoryProvider`, `IHostConnectivityProbe`, and `AlarmSurfaceInvoker` stubs; added 14 new tests covering each missing guarded interface (positive + wrapped), synchronous member not flagged, concrete driver receiver, `ExecuteWriteAsync` wrapping, and nested-lambda cases (19 tests total, all passing).
### Analyzers-007