8568f5cd85
Reviewed all 31 src/ production projects against the 10-category checklist in REVIEW-PROCESS.md. Each module gets its own findings.md; code-reviews/README.md is regenerated from them. 334 findings: 6 Critical, 46 High, 126 Medium, 156 Low. Critical findings: - Server-001: WriteNodeIdUnknown recurses unconditionally — a HistoryRead on an unresolvable node crashes the process (remote DoS). - Admin-001/002: app-wide auth bypass (RouteView not AuthorizeRouteView) plus unauthenticated mutating routes. - Core.Scripting-001: System.Environment reachable from operator scripts; Environment.Exit() terminates the server. - Core.AlarmHistorian-001: rowIds/events parallel-list desync on a corrupt payload misapplies outcomes — silent alarm-event data loss. - Driver.Galaxy-001: ReconnectSupervisor is built but never triggered, so a transient gateway drop permanently kills the event stream. All findings are Status=Open; resolution is tracked per REVIEW-PROCESS.md section 4. Review only — no source code changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
140 lines
12 KiB
Markdown
140 lines
12 KiB
Markdown
# Code Review — Analyzers
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Module | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers` |
|
|
| Reviewer | Claude Code |
|
|
| Review date | 2026-05-22 |
|
|
| Commit reviewed | `76d35d1` |
|
|
| Status | Reviewed |
|
|
| Open findings | 7 |
|
|
|
|
## Checklist coverage
|
|
|
|
| # | Category | Result |
|
|
|---|---|---|
|
|
| 1 | Correctness & logic bugs | Analyzers-001, Analyzers-002 |
|
|
| 2 | OtOpcUa conventions | No issues found |
|
|
| 3 | Concurrency & thread safety | No issues found |
|
|
| 4 | Error handling & resilience | Analyzers-003 |
|
|
| 5 | Security | No issues found |
|
|
| 6 | Performance & resource management | Analyzers-004 |
|
|
| 7 | Design-document adherence | Analyzers-005 |
|
|
| 8 | Code organization & conventions | No issues found |
|
|
| 9 | Testing coverage | Analyzers-006 |
|
|
| 10 | Documentation & comments | Analyzers-007 |
|
|
|
|
## Findings
|
|
|
|
### Analyzers-001
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Medium |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:135-139` |
|
|
| Status | Open |
|
|
|
|
**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)_
|
|
|
|
### Analyzers-002
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Correctness & logic bugs |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:46-50,130` |
|
|
| Status | Open |
|
|
|
|
**Description:** `AlarmSurfaceInvoker` is listed in `WrapperTypes`, but `AlarmSurfaceInvoker`'s public methods (`SubscribeAsync`, `UnsubscribeAsync`, `AcknowledgeAsync`) take no lambda arguments at all — callers pass `IReadOnlyList<...>` / `IAlarmSubscriptionHandle`, and the invoker builds the resilience lambdas internally. `IsInsideWrapperLambda` only ever returns `true` when it finds an `AnonymousFunctionExpressionSyntax` argument in the outer call's argument list. Because no `AlarmSurfaceInvoker` call site can have a lambda argument, the `AlarmSurfaceInvoker` entry in `WrapperTypes` is effectively dead — it can never satisfy the suppression condition. Guarded `IAlarmSource` calls written inside `AlarmSurfaceInvoker.cs` are in fact suppressed correctly, but only because they sit inside `CapabilityInvoker.ExecuteAsync` lambdas (the `CapabilityInvoker` entry does the work). The dead entry is misleading and suggests the analyzer recognises an `AlarmSurfaceInvoker` "lambda home" that does not exist.
|
|
|
|
**Recommendation:** Either remove `AlarmSurfaceInvoker` from `WrapperTypes` (its calls are already covered transitively by the `CapabilityInvoker` match) and update the XML doc, or — if the intent is to allow `IAlarmSource` calls anywhere inside `AlarmSurfaceInvoker` regardless of lambda nesting — add an explicit "call site is lexically within the `AlarmSurfaceInvoker` type declaration" check rather than relying on a lambda-argument scan that never fires.
|
|
|
|
**Resolution:** _(open)_
|
|
|
|
### Analyzers-003
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Error handling & resilience |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:80,114-116` |
|
|
| Status | Open |
|
|
|
|
**Description:** `IsInsideWrapperLambda` is passed `context.Operation.SemanticModel` and returns `false` when that model is `null`. A `false` return means "not wrapped", so a null semantic model produces a false-positive diagnostic rather than silently skipping the call. For `RegisterOperationAction` the `SemanticModel` is non-null in normal compilation, so this is low-risk in practice, but the failure mode is the wrong direction — a tooling/IDE edge case where the model is unavailable would flag correct code. Separately, the analyzer has no defensive guard against partially-bound / malformed call sites: `method.ContainingType`, `method.ReturnType`, and `iface.GetMembers()` are dereferenced without null checks. `IInvocationOperation.TargetMethod` is non-null by contract and `ContainingType` is non-null for an ordinary method, so a hard crash is unlikely, but an analyzer that throws on malformed in-progress syntax degrades the IDE experience for the whole solution.
|
|
|
|
**Recommendation:** When `semanticModel is null` in `AnalyzeInvocation`, return early (skip the call) instead of letting `IsInsideWrapperLambda` report it as unwrapped, so unavailable semantics never produce a false positive.
|
|
|
|
**Resolution:** _(open)_
|
|
|
|
### Analyzers-004
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Performance & resource management |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:95-112` |
|
|
| Status | Open |
|
|
|
|
**Description:** `ImplementsGuardedInterface` runs on every invocation operation in the compilation (every keystroke in the IDE). For each candidate it allocates via `AllInterfaces.Concat(new[] { method.ContainingType })`, builds a fully-qualified display string per interface and calls `string.Replace("global::", ...)`, then for matching interfaces iterates `iface.GetMembers().OfType<IMethodSymbol>()` calling `FindImplementationForInterfaceMember` per member. The `GuardedInterfaces` / `WrapperTypes` lookups are `string[].Contains` (linear scan) rather than a hash set. None of this is catastrophic — the interface sets are tiny — but the work is repeated for every invocation including the overwhelming majority that target non-guarded methods, and the FQN string formatting plus `Replace` allocation on the hot path is avoidable.
|
|
|
|
**Recommendation:** Move to `RegisterCompilationStartAction`: resolve the guarded interface and wrapper-type symbols once via `Compilation.GetTypeByMetadataName`, capture them, and compare invocation symbols by `SymbolEqualityComparer` identity. Replace the `string[]` membership checks with a `HashSet`. This also makes the analyzer correctly no-op in compilations that do not reference `Core.Abstractions`.
|
|
|
|
**Resolution:** _(open)_
|
|
|
|
### Analyzers-005
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Design-document adherence |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:33-43` |
|
|
| Status | Open |
|
|
|
|
**Description:** `CapabilityInvoker`'s XML doc (`src/Core/.../Resilience/CapabilityInvoker.cs:15-17`) enumerates the routed capability surface as `IReadable`, `IWritable`, `ITagDiscovery`, `ISubscribable`, `IHostConnectivityProbe`, `IAlarmSource`, and all four `IHistoryProvider` reads — matching the analyzer's `GuardedInterfaces` set. However `IHistoryProvider` exposes five async methods, and two of them (`ReadAtTimeAsync`, `ReadEventsAsync`) are C# default-interface-method implementations. When a driver does not override a DIM and a caller invokes it through a concrete driver reference, `FindImplementationForInterfaceMember` returns the interface's own default method symbol; the second equality branch (`method.OriginalDefinition` == `member`) still catches the interface-typed-receiver case, so detection holds — but this DIM interaction is undocumented and untested, and a future driver that overrides one DIM but not the other creates an asymmetric guarded surface that nobody has verified.
|
|
|
|
**Recommendation:** Add explicit test cases (see Analyzers-006) for `IHistoryProvider` calls via both an interface-typed receiver and a concrete driver that (a) overrides and (b) inherits the default `ReadAtTimeAsync` / `ReadEventsAsync`. If a gap is found, handle DIM members explicitly. Add a short remark to the analyzer XML doc noting the default-interface-method consideration.
|
|
|
|
**Resolution:** _(open)_
|
|
|
|
### Analyzers-006
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Medium |
|
|
| Category | Testing coverage |
|
|
| Location | `tests/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers.Tests/UnwrappedCapabilityCallAnalyzerTests.cs` |
|
|
| Status | Open |
|
|
|
|
**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:
|
|
|
|
- No test for `ISubscribable`, `IHostConnectivityProbe`, `IAlarmSource`, or `IHistoryProvider` — four of seven guarded interfaces, including the two (`IAlarmSource`, `IHistoryProvider`) with the most subtle wrapping story.
|
|
- No test that a synchronous guarded-type member is NOT flagged — `IHostConnectivityProbe.GetHostStatuses()` is explicitly called out in the source comment (lines 75-77) as something the `IsAsyncReturningType` filter must let through, yet there is no regression test pinning that.
|
|
- No test for a concrete driver class implementing the interface (the receiver is always the interface type `IReadable driver`); the `FindImplementationForInterfaceMember` branch of `ImplementsGuardedInterface` — the entire reason the source comment claims an unusually-named method implementing `IReadable.ReadAsync` still trips the rule — is never executed by a test.
|
|
- No test for `ExecuteWriteAsync` (only `ExecuteAsync` is covered) and no test for `AlarmSurfaceInvoker`.
|
|
- No test for nested lambdas or for the generated-code exclusion (`ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None)`).
|
|
- The `StubSources` constant omits `ISubscribable` / `IAlarmSource` / `IHistoryProvider` / `IHostConnectivityProbe` and `AlarmSurfaceInvoker` entirely, so those paths cannot be tested without extending it.
|
|
|
|
**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)_
|
|
|
|
### Analyzers-007
|
|
|
|
| Field | Value |
|
|
|---|---|
|
|
| Severity | Low |
|
|
| Category | Documentation & comments |
|
|
| Location | `src/Tooling/ZB.MOM.WW.OtOpcUa.Analyzers/UnwrappedCapabilityCallAnalyzer.cs:21-26` |
|
|
| Status | Open |
|
|
|
|
**Description:** The `<remarks>` block states the analyzer "matches by receiver-interface identity using Roslyn's semantic model, not by method name". This is accurate for the guarded-call detection (`ImplementsGuardedInterface` uses symbols), but the wrapper detection in `IsInsideWrapperLambda` is described in the same block as walking the syntax tree and checking enclosing invocations by containing type — and that detection is in fact looser than the prose implies (see Analyzers-001): it does not verify the lambda is bound to the resilience `callSite` parameter. The XML doc reads as if the wrapper match is precise. The `<remarks>` also notes the rule does not enforce the capability argument matches the method, but omits the more important current limitation — that a lambda in any argument position of a wrapper-typed call suppresses the diagnostic.
|
|
|
|
**Recommendation:** Tighten the `<remarks>` to state precisely what `IsInsideWrapperLambda` checks today (textual containment within a lambda argument of a `CapabilityInvoker` / `AlarmSurfaceInvoker`-typed invocation), and note the known limitation that it does not bind the lambda to the `callSite` parameter. Keep the doc in sync if Analyzers-001 is fixed.
|
|
|
|
**Resolution:** _(open)_
|