diff --git a/code-reviews/Core.ScriptedAlarms/findings.md b/code-reviews/Core.ScriptedAlarms/findings.md index 8d953af..093ba48 100644 --- a/code-reviews/Core.ScriptedAlarms/findings.md +++ b/code-reviews/Core.ScriptedAlarms/findings.md @@ -4,16 +4,18 @@ |---|---| | Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-05-23 | +| Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 0 | +| Open findings | 1 | ## Checklist coverage A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank. +### 2026-05-22 review (commit `76d35d1`) + | # | Category | Result | |---|---|---| | 1 | Correctness & logic bugs | Core.ScriptedAlarms-002 | @@ -27,6 +29,26 @@ a category produced nothing rather than leaving it blank. | 9 | Testing coverage | Core.ScriptedAlarms-012 | | 10 | Documentation & comments | Core.ScriptedAlarms-003 | +### 2026-05-23 re-review (commit `a9be809`) + +Focused re-review of the Core.ScriptedAlarms-009 resolution (commit `0001cdd`) — +new `AlarmScratch` class, `_scratchByAlarmId` ConcurrentDictionary, `RefillReadCache` +helper, and internal test accessors. Only the changed/new code since `76d35d1` was +re-examined; existing closed findings stay as audit trail. + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found | +| 2 | OtOpcUa conventions | No issues found | +| 3 | Concurrency & thread safety | No issues found | +| 4 | Error handling & resilience | No issues found | +| 5 | Security | No issues found | +| 6 | Performance & resource management | No issues found | +| 7 | Design-document adherence | No issues found | +| 8 | Code organization & conventions | No issues found | +| 9 | Testing coverage | No issues found | +| 10 | Documentation & comments | Core.ScriptedAlarms-013 | + ## Findings ### Core.ScriptedAlarms-001 @@ -224,3 +246,21 @@ green (was 63). **Recommendation:** Add engine-level tests that inject a controllable `Func` clock to drive `RunShelvingCheck`, cover the remaining Part 9 engine methods end-to-end, assert subscriber-exception isolation, and add a store-failure fake to lock in the chosen persistence-failure semantics from finding 007. **Resolution:** Resolved 2026-05-22 — added 8 new engine-level tests covering all 6 gap areas: injectable-clock timed-shelve expiry via `RunShelvingCheckForTest`, `ConfirmAsync`/`TimedShelveAsync`/`UnshelveAsync`/`EnableAsync` end-to-end, subscriber-exception isolation, store-failure invariant, second-`LoadAsync` timer-leak regression, and `AreInputsReady` Bad/Uncertain guard; exposed `RunShelvingCheckForTest()` internal hook on the engine. + +### Core.ScriptedAlarms-013 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `ScriptedAlarmEngine.cs:66-81` | +| Status | Open | + +**Description:** The new internal test accessors `TryGetScratchReadCacheForTest` and `TryGetScratchContextForTest` (introduced by the Core.ScriptedAlarms-009 resolution at `0001cdd`) return the *live* per-alarm scratch — the same `Dictionary` instance the engine clears and refills in `RefillReadCache` under `_evalGate`, plus the `AlarmPredicateContext` that wraps it by reference. The XML docs describe the intended use ("assert the scratch is reused across evaluations (two reads return the same instance)") but do not explicitly warn that: + +1. The returned `IReadOnlyDictionary` is the engine's mutable working set. Enumerating it from a test thread while the engine is mid-evaluation (e.g. during a `ReevaluateAsync` queued by `OnUpstreamChange`, or a `ShelvingCheckAsync` callback) is a concurrent-read-while-writer scenario against a plain `Dictionary` — undefined behaviour, can throw `InvalidOperationException` or return torn data. +2. Reference-equality comparisons (`ReferenceEquals(a, b)`) and single-key indexer reads (`dict["Temp"]`) on a quiesced engine are the only safe uses. The existing regression tests stay within those bounds, but a future test author has no in-code signal that broader reads are unsafe. + +The engine itself is correct — `RefillReadCache` runs only under `_evalGate`, so the engine never tears its own state. The risk is purely on the test-side contract. + +**Recommendation:** Add a `` block to both `TryGetScratchReadCacheForTest` and `TryGetScratchContextForTest` stating that the returned references point at live engine state, that reads are only safe when the engine is known to be idle (no in-flight `ReevaluateAsync`/`ShelvingCheckAsync`/`LoadAsync`), and that the intended uses are reference-identity assertions plus single-key lookups against a quiesced engine — never enumeration. No code change required; the engine's correctness depends on `_evalGate`, which is already documented. diff --git a/code-reviews/Core.Scripting/findings.md b/code-reviews/Core.Scripting/findings.md index 19cf939..e493cd9 100644 --- a/code-reviews/Core.Scripting/findings.md +++ b/code-reviews/Core.Scripting/findings.md @@ -4,28 +4,33 @@ |---|---| | Module | `src/Core/ZB.MOM.WW.OtOpcUa.Core.Scripting` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-05-23 | +| Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 0 | +| Open findings | 5 | ## Checklist coverage A comprehensive review completes every category, recording "No issues found" where a category produced nothing rather than leaving it blank. -| # | Category | Result | -|---|---|---| -| 1 | Correctness & logic bugs | Core.Scripting-004, Core.Scripting-005 | -| 2 | OtOpcUa conventions | No issues found | -| 3 | Concurrency & thread safety | Core.Scripting-006 | -| 4 | Error handling & resilience | Core.Scripting-007 | -| 5 | Security | Core.Scripting-001, Core.Scripting-002, Core.Scripting-003 | -| 6 | Performance & resource management | Core.Scripting-008 | -| 7 | Design-document adherence | Core.Scripting-009 | -| 8 | Code organization & conventions | No issues found | -| 9 | Testing coverage | Core.Scripting-010, Core.Scripting-011 | -| 10 | Documentation & comments | No issues found | +The 2026-05-23 re-review only covers code touched between commits `76d35d1` and +`a9be809` (primarily the Core.Scripting-008 ALC rewrite + the broadened BCL +references). Categories where the new code surface produced no issues are +recorded as "No new issues" for that pass. + +| # | Category | Result (76d35d1) | Result (a9be809, new code only) | +|---|---|---|---| +| 1 | Correctness & logic bugs | Core.Scripting-004, Core.Scripting-005 | Core.Scripting-015 | +| 2 | OtOpcUa conventions | No issues found | No new issues | +| 3 | Concurrency & thread safety | Core.Scripting-006 | Core.Scripting-014 | +| 4 | Error handling & resilience | Core.Scripting-007 | No new issues | +| 5 | Security | Core.Scripting-001, Core.Scripting-002, Core.Scripting-003 | Core.Scripting-012, Core.Scripting-013 | +| 6 | Performance & resource management | Core.Scripting-008 | Core.Scripting-016 | +| 7 | Design-document adherence | Core.Scripting-009 | No new issues | +| 8 | Code organization & conventions | No issues found | No new issues | +| 9 | Testing coverage | Core.Scripting-010, Core.Scripting-011 | No new issues | +| 10 | Documentation & comments | No issues found | No new issues | ## Findings @@ -362,3 +367,294 @@ a script logging at Error level produces both a `scripts-*.log` event and a comp Warning event. **Resolution:** Resolved 2026-05-23 — added three new test files: `ScriptSandboxBuildTests` covers the `Build` null / non-`ScriptContext` / base-class / concrete-subclass paths; `ScriptContextTests` locks `Deadband` boundary semantics (equal-to-tolerance returns false; just-over returns true; symmetric in direction; zero-tolerance returns true only on non-equal; negative tolerance trips on any non-equal); the new `Factory_plus_companion_sink_integration_surfaces_script_error_in_both_logs` test in `ScriptLogCompanionSinkTests` wires `ScriptLoggerFactory` + the companion sink together end-to-end and asserts an Error emission lands in both the scripts sink (at Error) and the main sink (at Warning), each tagged with `ScriptName`. Suite now 101 green (was 85 before). + +### Core.Scripting-012 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Security | +| Location | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | +| Status | Open | + +**Description:** The Core.Scripting-008 rewrite broadened the BCL references list +from a narrow allow-list (`System.Private.CoreLib` + `System.Linq` only) to the +full `TRUSTED_PLATFORM_ASSEMBLIES` set filtered to `System.*` + `netstandard` + +`Microsoft.Win32.Registry`. This change correctly delegates the security gate to +`ForbiddenTypeAnalyzer` (the new comment in `ScriptSandbox` calls this out +explicitly), but the analyzer's deny-list has not been expanded to match the new +attack surface, and three categories of dangerous BCL types in the `System.*` +allow-listed assemblies are now reachable from script source: + +1. **`System.Threading.ThreadPool`** (in namespace `System.Threading`). The + Core.Scripting-003 fix added `System.Threading.Tasks` to deny `Task.Run` / + `Parallel` fan-out because background work that outlives the per-evaluation + timeout is the explicit threat. `ThreadPool.QueueUserWorkItem`, + `ThreadPool.UnsafeQueueUserWorkItem`, and `ThreadPool.RegisterWaitForSingleObject` + are exactly the same threat — they schedule background work that outlives the + `WaitAsync(Timeout)` budget and tie up worker threads — but `System.Threading` + itself is allowed (because `CancellationToken` / `SemaphoreSlim` / `Volatile` + live there). The Core.Scripting-003 resolution is incomplete on the new + reference surface. +2. **`System.Threading.Timer`** (same namespace). Schedules a background + callback; the script returns control to the engine but the timer keeps + firing past the evaluation budget. Same threat as `Task.Run`. +3. **`System.Runtime.Loader.AssemblyLoadContext`** (in namespace + `System.Runtime.Loader`, which is not denied — only `System.Runtime.InteropServices` + is). The constructor + `LoadFromAssemblyPath` / `LoadFromStream` / + `LoadFromAssemblyName` let a script load an arbitrary DLL into the host + process. Pass (1) of the analyzer resolves the receiver type + (`AssemblyLoadContext`, allowed) + the invocation symbol's containing type + (also `AssemblyLoadContext`, allowed) and lets the call through. Pass (2) + only inspects `TypeSyntax` nodes — if the script discards the returned + `Assembly` (e.g. `alc.LoadFromAssemblyPath(@"C:\evil.dll");`) there is no + `TypeSyntax` for the analyzer to walk and the call is accepted. Triggering + execution of the loaded code from inside the sandbox is hard (most of + `Assembly`'s surface is in `System.Reflection`, which is denied) but the + defense-in-depth gap is real: an attacker who can author a script also + typically controls a file path on the server (Admin UI uploads, share + mounts) and loading an assembly is the prerequisite to every chained + escape — module initializers, type-resolve handlers, and a future analyzer + slip would all become exploitable. + +In addition, two lower-impact `System.*` types are reachable that arguably +shouldn't be: **`System.Console.SetOut`** / **`Console.SetError`** could +redirect the host's console streams (requires constructing a +`System.IO.TextWriter`, which is blocked, so the practical exploit is +`Console.WriteLine` log-spam only), and **`System.Globalization.CultureInfo.DefaultThreadCurrentCulture`** +could perturb the entire process's formatting behavior (subtle but real cross-script +side effect). + +The original Core.Scripting-001 finding called out the model: when an allow-listed +namespace contains dangerous types, those types must be denied type-granularly. +The new reference surface introduces several more such types and the deny-list +has not been kept in sync. + +**Recommendation:** Add `System.Threading.ThreadPool` and `System.Threading.Timer` +to `ForbiddenFullTypeNames`. Add `System.Runtime.Loader` as a namespace prefix +to `ForbiddenNamespacePrefixes` (every type in `System.Runtime.Loader` — +`AssemblyLoadContext`, `AssemblyDependencyResolver`, `AssemblyLoadEventArgs` — is +out of script scope). Consider adding `System.Console` to `ForbiddenFullTypeNames` +to stop log-spam through the host's console streams, and at minimum document +`CultureInfo.DefaultThreadCurrentCulture` as an accepted cross-script side +effect. Each addition must have a regression test in `ScriptSandboxTests` +mirroring the Core.Scripting-010 vector style. Update +`docs/v2/implementation/phase-7-scripting-and-alarming.md` decision #6 + the +"Sandbox escape" compliance-check row to enumerate the additions, per the +Core.Scripting-009 doc-sync convention. + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ + +### Core.Scripting-013 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | +| Status | Open | + +**Description:** The synthesized wrapper pastes the user's source verbatim +between `{` and `}` braces inside a static method body, with a `#line 1` +directive and no escaping. The legacy `CSharpScript.CreateDelegate` path was +robust to this because Roslyn's scripting compiler parses script source as a +top-level statement sequence; the new hand-rolled path is parsing ordinary C# in +a method body, so a script that injects matching `{` / `}` braces can extend the +synthesized compilation unit with additional methods, classes, or `#line` +directives. For example, a script body of +`return 0; } public static int Evil() { return 0; }} public static class CompiledScript2 { public static void M() {` +ends the `Run` method early, declares a sibling `Evil` method (and even a +sibling `CompiledScript2` class) inside the synthesized namespace, then opens an +unclosed method that consumes the wrapper's trailing `}\n}`. With matching brace +counts the script parses cleanly and compiles. + +`ForbiddenTypeAnalyzer` walks every descendant of every syntax tree, so any +forbidden BCL types named inside the injected methods are still caught — the +finding is **not** a direct sandbox escape. However: + +- It silently relaxes the operator-visible authoring contract documented in + `docs/VirtualTags.md` ("scripts are statement bodies that end with an + explicit `return …;`") to "scripts can be any compilable C# inside the + `CompiledScript` namespace" — operators have access to features the design + did not intend to expose (local types defined as siblings of `Run`, custom + module initializers via attributes, etc.). +- A script can embed its own `#line` directives that override the + `#line 1` we emit just above the user source, producing misleading error + locations in compiler diagnostics surfaced to the operator. +- Future hardening that relies on syntactic-shape assumptions (e.g. + "every script has exactly one method") would silently fail. +- It widens the analyzer's surface: the analyzer's correctness now depends on + Pass (2) correctly walking every conceivable C# construct that can name a + type, including ones a normal script body would never contain + (`UnmanagedCallersOnly` attribute, function pointer types `delegate*<...>`, + pattern types, switch arm types, …). + +**Recommendation:** Either (a) reject scripts whose parsed body contains +declarations other than statements — walk the wrapper's syntax tree after parse +and require that the only members of `CompiledScript` are the single `Run` +method, raising a `CompilationErrorException` if anything else appears — or +(b) parse the user source independently as a `BlockSyntax` and inject the +parsed block as the method body via the Roslyn syntax API, which makes +brace-mismatched / class-injecting source unparseable. Add a regression test +covering at least the brace-injection vector +(`return 0; } public static int Evil() { return 0;`). + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ + +### Core.Scripting-014 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Concurrency & thread safety | +| Location | `CompiledScriptCache.cs:91-103` (`Clear`) | +| Status | Open | + +**Description:** `Clear()` snapshots `_cache.Keys.ToArray()` then iterates, +calling `TryRemove(key, out var lazy)` on each — the key-only overload, not +the value-scoped one used in `GetOrCompile`'s catch block. Between the +snapshot and a given `TryRemove`, a concurrent `GetOrCompile(scriptSource)` +call that hashes to the same key can re-insert a fresh `Lazy` whose `.Value` +the caller already retained. The unconditional `TryRemove` then removes that +fresh `Lazy` and `DisposeLazyIfMaterialised(lazy)` calls `Dispose()` on its +evaluator — unloading the ALC while the concurrent caller still holds a +reference to the evaluator and intends to invoke it. + +This is exactly the race-window pattern the Core.Scripting-006 resolution +fixed in `GetOrCompile`'s catch block (the test +`Failed_compile_eviction_does_not_remove_a_concurrent_retry_entry` locks it +there). `Clear()` carries the same shape but uses the older, value-blind +overload, so the same race that finding-006 addresses is still latent on the +publish-replace path. + +In current production wiring `Clear()` is intended for config-publish + tests +— neither overlaps steady-state evaluation under the documented design — so +the in-practice impact is low. But the cache is checked in as the +forward-looking compile cache for the engines (per `Script.SourceHash`'s docs +and the cache's own remarks); a future wiring that calls `Clear()` from +publish while evaluations are in flight would dispose live evaluators. + +**Recommendation:** Replace the snapshot + `TryRemove(key, out var lazy)` +sequence with an enumeration that captures the `Lazy` reference at snapshot +time and uses the value-scoped `TryRemove(KeyValuePair<,>)` overload, mirroring +the Core.Scripting-006 fix: + +```csharp +foreach (var entry in _cache.ToArray()) +{ + if (_cache.TryRemove(entry)) + DisposeLazyIfMaterialised(entry.Value); +} +``` + +Add a regression test that races `GetOrCompile` against `Clear` and asserts +the caller's evaluator is still usable. + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ + +### Core.Scripting-015 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Correctness & logic bugs | +| Location | `ScriptEvaluator.cs:234-270` (`ToCSharpTypeName`) | +| Status | Open | + +**Description:** `ToCSharpTypeName` is documented to handle nested types +(`Outer+Inner` → `Outer.Inner`) via `Replace('+', '.')` for the +non-generic path (line 269) but the generic path (line 263-266) constructs the +name from `def.FullName!` then takes a substring up to the backtick. For a +**nested generic** type — e.g. `Outer.Inner` whose `FullName` is +`Outer+Inner`1` — `Replace('+', '.')` is applied first, then `Substring(0, IndexOf('`'))` +on `"Outer.Inner`1"` produces `"Outer.Inner"`, which is correct. Good. + +However, the generic branch does NOT handle the case where the OPEN generic +type itself is nested with `+` inside the parent's name when the parent is +also generic (`Outer.Inner` — `FullName` is +`Outer`1+Inner`1[[TOuter,TInner]]`). For that shape `Substring(0, IndexOf('`'))` +truncates at the first backtick — yielding `"Outer.Inner"` — silently dropping +the closed type arguments of `Outer`. The resulting source string is +syntactically valid but semantically wrong: `global::Outer.Inner` does +not name `Outer.Inner`. + +The production code never hits this shape — `TResult` is always one of +`object?`, `bool`, `int`, `double`, `string?`, `DateTime` across the +virtual-tag engine, the alarm engine, the test-harness, and the test suite, +and `ScriptGlobals` is always a top-level generic over a top-level +`ScriptContext` subclass. The bug is latent. But it is a foot-gun for a +future caller (e.g. a Phase-8 driver that wires a context type defined as a +nested generic for grouping reasons) and the XML-doc comment claims +"handles nested types" without qualifying it. + +A second smaller correctness gap on the same path: the comment claims +`global::`-qualified FQNs prevent accidental capture by the wrapper's `using` +directives, which is true for the generic / non-generic branches, but the +primitive aliases (`bool`, `int`, `string`, `object`, …) are emitted unqualified. +A script that defines a local `class bool` (now possible per Core.Scripting-013) +would shadow the alias. Probably benign, but worth a comment. + +**Recommendation:** Add a check in the generic branch that walks the FullName +backtick-by-backtick — or use `INamedTypeSymbol`-style name composition from +`def.DeclaringType` recursively — so multi-arity-nested generics emit +correctly. At minimum update the XML doc to qualify "handles nested types" as +"handles single-level nesting; nested generics whose parent is itself generic +are not supported". Add a `ToCSharpTypeName` unit test (currently nothing +exercises this method directly — coverage relies on the end-to-end compile path, +so the bug surfaces only as a misleading Roslyn diagnostic). + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ + +### Core.Scripting-016 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Performance & resource management | +| Location | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:74-117`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:139-182` | +| Status | Open | + +**Description:** The Core.Scripting-008 resolution introduced +`ScriptEvaluator.IDisposable` + `CompiledScriptCache.Clear()` that disposes +each materialised evaluator before dropping its dictionary entry, so per-publish +ALC accretion is no longer process-lifetime rooted **inside the cache**. But +neither production consumer of `ScriptEvaluator` uses the cache — both +`VirtualTagEngine.Load` and `ScriptedAlarmEngine.LoadAsync` call +`ScriptEvaluator.Compile(...)` directly (lines 105 / 160 +respectively), store the evaluator inside an internal `VirtualTagState` / +`AlarmState` record, and on the next `Load` simply call `_tags.Clear()` / +`_alarms.Clear()`. The dropped `ScriptEvaluator` references never have +`Dispose()` called on them, so the underlying `ScriptAssemblyLoadContext` +instances are never `Unload()`-ed. The .NET runtime guarantees that a +collectible ALC stays alive until `Unload()` is called explicitly — having +"no strong references" is necessary but not sufficient. So the publish-replace +cycle leaks every prior generation's emitted assembly exactly as before the +fix, even though the fix's infrastructure is in place. + +The Core.Scripting-008 regression tests in `CompiledScriptCacheTests` +(`Dispose_unloads_compiled_script_assembly_load_context` / +`Clear_disposes_every_materialised_evaluator`) prove the contract on +`CompiledScriptCache`, but neither engine uses that class. There is no +integration test exercising the actual publish path — i.e. that calling +`VirtualTagEngine.Load(...)` twice with different definitions makes the prior +generation's ALC eligible for GC. As a result the fix's headline guarantee +("Server restarts are no longer required to reclaim compiled-script memory" — +`docs/VirtualTags.md`) is not actually delivered to the production engines. + +This is the same observable behavior the original Core.Scripting-008 finding +described, surfacing on a different code path that the resolution did not touch. + +**Recommendation:** Either route the engines' compile path through +`CompiledScriptCache` (the documented design — the cache +already returns the same evaluator instance for identical source, and its +`Clear()` now performs the right disposal — and `Script.SourceHash`'s doc-comment +already names this as the cache key), or make the engines' `Load` methods +dispose the previous `ScriptEvaluator` instances before reassigning. The +former is the cleaner change because it also collapses redundant compiles +across publishes for unchanged scripts. Add an integration test along the +lines of `CompiledScriptCacheTests.Clear_disposes_every_materialised_evaluator` +for each engine: snapshot the per-evaluator emitted assembly via +`WeakReference`, call `Load(...)` with a different definition set, and assert +the prior generation's assemblies become collectable. + +**Resolution:** _(empty until closed; on close, record the fixing commit SHA, the date, and a one-line description of the fix)_ diff --git a/code-reviews/Driver.Cli.Common/findings.md b/code-reviews/Driver.Cli.Common/findings.md index 8adc23a..ea772f6 100644 --- a/code-reviews/Driver.Cli.Common/findings.md +++ b/code-reviews/Driver.Cli.Common/findings.md @@ -4,10 +4,10 @@ |---|---| | Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-05-23 | +| Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 0 | +| Open findings | 2 | ## Checklist coverage @@ -16,7 +16,7 @@ a category produced nothing rather than leaving it blank. | # | Category | Result | |---|---|---| -| 1 | Correctness & logic bugs | Driver.Cli.Common-001, Driver.Cli.Common-002 | +| 1 | Correctness & logic bugs | Driver.Cli.Common-001, Driver.Cli.Common-002, Driver.Cli.Common-007 | | 2 | OtOpcUa conventions | No issues found | | 3 | Concurrency & thread safety | Driver.Cli.Common-003 | | 4 | Error handling & resilience | Driver.Cli.Common-004 | @@ -24,9 +24,47 @@ a category produced nothing rather than leaving it blank. | 6 | Performance & resource management | No issues found | | 7 | Design-document adherence | No issues found | | 8 | Code organization & conventions | No issues found | -| 9 | Testing coverage | Driver.Cli.Common-005 | +| 9 | Testing coverage | Driver.Cli.Common-005, Driver.Cli.Common-008 | | 10 | Documentation & comments | Driver.Cli.Common-006 | +## Re-review 2026-05-23 (commit `a9be809`) + +Delta scope: commit `5a9c459` extends the `FormatStatus` shortlist with five +`Bad*` codes (`BadInternalError` 0x80020000, `BadNotWritable` 0x803B0000, +`BadOutOfRange` 0x803C0000, `BadNotSupported` 0x803D0000, `BadDeviceFailure` +0x80550000) the FOCAS / AbCip / AbLegacy native-protocol mappers emit. Tests +extended with parallel `[InlineData]` rows on the well-known Theory plus a new +`FormatStatus_names_native_driver_emitted_codes` Theory. + +Cross-checked the five new hex literals against the OPC Foundation +`Opc.Ua.StatusCodes` table via DeepWiki: + +| Name added | Code in shortlist | Spec value | Verdict | +|---|---|---|---| +| `BadInternalError` | `0x80020000` | `0x80020000` | Correct | +| `BadNotWritable` | `0x803B0000` | `0x803B0000` | Correct | +| `BadOutOfRange` | `0x803C0000` | `0x803C0000` | Correct | +| `BadNotSupported` | `0x803D0000` | `0x803D0000` | Correct | +| `BadDeviceFailure` | `0x80550000` | **`0x808B0000`** | **WRONG — `0x80550000` is `BadSecurityPolicyRejected`** | + +The `BadDeviceFailure` mismapping is the same shape of bug as the original +Driver.Cli.Common-001 (wrong hex literal copied into the shortlist); recorded +as Driver.Cli.Common-007. The wrong constant also lives in +`FocasStatusMapper.cs`, `AbCipStatusMapper.cs`, `AbLegacyStatusMapper.cs`, +`TwinCATStatusMapper.cs`, `S7Driver.cs`, and `ModbusDriver.cs` — those are in +other modules' review scope but are noted here so future re-reviewers know +this isn't isolated. (`StatusCodeMap.cs` in Driver.Galaxy + the Wonderware +historian mappers use the correct `0x808B0000`, confirming the discrepancy.) + +Testing observation: the new `FormatStatus_names_native_driver_emitted_codes` +Theory is fully redundant with the well-known Theory (the five rows were also +added there in the same commit) and uses `ShouldContain` rather than +`ShouldBe` — recorded as Driver.Cli.Common-008. + +Other categories (concurrency, security, performance, design-doc adherence, +code organisation, documentation) are unchanged by this delta — no new +issues found. + ## Findings ### Driver.Cli.Common-001 @@ -205,3 +243,92 @@ to state the source-time column is the right-most one and intentionally not measured/padded, calling out the null-timestamp `"-"` case explicitly. (2) FOCAS was added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff356b` (landed alongside the -003 work). + +### Driver.Cli.Common-007 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:129` | +| Status | Open | + +**Description:** Commit `5a9c459` added `0x80550000u => "BadDeviceFailure"` to the +`FormatStatus` shortlist, but `0x80550000` is the canonical OPC UA spec value for +`BadSecurityPolicyRejected`, not `BadDeviceFailure`. The correct spec value for +`BadDeviceFailure` is `0x808B0000` (verified against the OPC Foundation +`Opc.Ua.StatusCodes` table via DeepWiki; corroborated locally by +`src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs:40` +(`BadDeviceFailure = 0x808B0000u`) and the two Wonderware historian quality mappers, +which all hand-pin the correct value). + +This is the same shape of bug Driver.Cli.Common-001 closed: a wrong hex literal +in the shortlist that the test theory (`SnapshotFormatterTests.cs:42`) blindly +asserts against the same wrong value, so the bug is invisible to CI. + +Practical impact, two-sided: +1. A driver that returns the real spec `BadDeviceFailure` (`0x808B0000`) — e.g. + the `Driver.Galaxy.StatusCodeMap` path on a deploy-time device fault — falls + through the named shortlist entirely. Since Driver.Cli.Common-002 added the + severity-class fallback, it now renders as `0x808B0000 (Bad)` instead of + `0x808B0000 (BadDeviceFailure)` — operators lose the specific class label + `docs/Driver.FOCAS.Cli.md:153` tells them to read off the output. +2. A driver that returns `0x80550000` (which `FocasStatusMapper`, `AbCipStatusMapper`, + `AbLegacyStatusMapper`, `TwinCATStatusMapper`, `S7Driver`, and `ModbusDriver` all + misuse as "BadDeviceFailure") now renders as `0x80550000 (BadDeviceFailure)` — + matching driver intent but contradicting the OPC UA spec, which says any client + that decodes the same payload using the OPC Foundation stack will see + `BadSecurityPolicyRejected`. A security-monitoring tool keying on + `BadSecurityPolicyRejected` will fire on a CPU fault, while real + `BadSecurityPolicyRejected` returns from the secure-channel layer would be + mislabelled as a device fault. Operator-facing CLI output and machine-readable + status semantics disagree. + +The deeper bug is the wrong constant in the native-protocol mappers (out of scope +for this module), but the `SnapshotFormatter` shortlist is its own +spec-authoritative reference point — Driver.Cli.Common-001 explicitly framed the +shortlist as canonical, with the in-line "keep [these literals] in sync with [the +Opc.Ua.StatusCodes] table" comment at `SnapshotFormatter.cs:112-113`. That +contract is now broken. + +**Recommendation:** Change line 129 to `0x808B0000u => "BadDeviceFailure"`. Update +the matching `[InlineData]` rows in `SnapshotFormatterTests.cs` (line 42 in the +well-known Theory; line 60 in the redundant Theory — see Driver.Cli.Common-008). +Also note in the resolution that the native-protocol mappers (FOCAS / AbCip / +AbLegacy / TwinCAT / S7 / Modbus) need the same fix recorded against their own +module reviews — the constant `0x80550000` should be replaced with `0x808B0000` +everywhere it claims to mean `BadDeviceFailure`. Consider Driver.Cli.Common-001's +original recommendation again: add a CI test that cross-checks every shortlist +entry against `Opc.Ua.StatusCodes` reflection so this class of bug stops +recurring. + +### Driver.Cli.Common-008 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Testing coverage | +| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:50-64` | +| Status | Open | + +**Description:** Commit `5a9c459` adds a new +`FormatStatus_names_native_driver_emitted_codes` `[Theory]` whose five +`[InlineData]` rows are identical to five rows added to the existing +`FormatStatus_names_well_known_status_codes` `[Theory]` in the same commit +(lines 32, 39, 40, 41, 42). The new Theory therefore adds no coverage. It is +also weaker than the Theory it duplicates: it asserts +`output.ShouldContain($"({expectedName})")` (substring match) where the +well-known Theory asserts `output.ShouldBe($"0x{status:X8} ({expectedName})")` +(exact match including the hex prefix). The substring form would not catch a +regression where the hex literal renders wrong but the name is correct. + +This is not a correctness problem — both Theories pass — but it's a +copy-paste inconsistency that costs maintainer attention every time someone +reads the test file and wonders which Theory is authoritative. + +**Recommendation:** Either (a) delete the new Theory entirely — its five rows +are already covered by the well-known Theory in the same commit — or (b) keep +it but switch to `ShouldBe($"0x{status:X8} ({expectedName})")` so its +assertion strength matches the rest of the file. Option (a) is cleaner: the +commit's "operator workflow" intent is documented well enough in the +well-known Theory comment block; the redundant Theory is dead weight. diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index e8ae0f2..8135c74 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -4,10 +4,10 @@ |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy` | | Reviewer | Claude Code | -| Review date | 2026-05-22 | -| Commit reviewed | `76d35d1` | +| Review date | 2026-05-23 | +| Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 0 | +| Open findings | 4 | ## Checklist coverage @@ -17,12 +17,39 @@ | 2 | OtOpcUa conventions | Driver.Galaxy-005 | | 3 | Concurrency & thread safety | Driver.Galaxy-006, Driver.Galaxy-007 | | 4 | Error handling & resilience | Driver.Galaxy-001, Driver.Galaxy-008, Driver.Galaxy-009 | -| 5 | Security | Driver.Galaxy-010 | -| 6 | Performance & resource management | Driver.Galaxy-011, Driver.Galaxy-012 | -| 7 | Design-document adherence | Driver.Galaxy-013 | +| 5 | Security | Driver.Galaxy-010, Driver.Galaxy-015 | +| 6 | Performance & resource management | Driver.Galaxy-011, Driver.Galaxy-012, Driver.Galaxy-016 | +| 7 | Design-document adherence | Driver.Galaxy-013, Driver.Galaxy-017 | | 8 | Code organization & conventions | No issues found | | 9 | Testing coverage | Driver.Galaxy-014 | -| 10 | Documentation & comments | Driver.Galaxy-005, Driver.Galaxy-013 | +| 10 | Documentation & comments | Driver.Galaxy-005, Driver.Galaxy-013, Driver.Galaxy-018 | + +## Re-review 2026-05-23 (commit `a9be809`) + +The only code-affecting change since `76d35d1` was commit `994997b` — the +sibling `mxaccessgw` repo restructured (the `clients/dotnet/MxGateway.Client` +project path and the `MxGateway.Contracts.Proto` namespace both moved), and +the driver's path-based `ProjectReference` started producing 87 build errors +solution-wide. The fix is build-time only: the broken `ProjectReference` was +replaced with `` items pointing at vendored +binary copies of `MxGateway.Client.dll` (99 KB, May 2026 known-good build) +and `MxGateway.Contracts.dll` (490 KB), and five `PackageReference`s that +the dropped project was previously providing transitively (`Google.Protobuf`, +`Grpc.Core.Api`, `Grpc.Net.Client`, `Microsoft.Extensions.Logging.Abstractions`, +`Polly`) were declared explicitly. The matching `Tests` csproj got the same +binary `` for `MxGateway.Contracts` (replacing its own broken +`ProjectReference`). A `libs/README.md` documents what is vendored and the +two unwinding paths (sibling restores a client library, or driver migrates +to the new `ZB.MOM.WW.MxGateway.Contracts.Proto` namespace + reimplements +the `MxGatewayClient` / `MxGatewaySession` / `GalaxyRepositoryClient` +wrapper, ~2,200 LoC). + +No `*.cs` file changed; the re-review walked only the categories that apply +to a build-time/packaging change. Categories with no new findings: +Correctness (1), OtOpcUa conventions (2), Concurrency (3), Error handling +(4), Code organization (8), Testing coverage (9). Four new findings are +recorded below (Driver.Galaxy-015..018) — none Critical, none High; two +Medium, two Low. ## Findings @@ -235,3 +262,59 @@ **Recommendation:** Add unit/parity tests covering: (a) stream fault -> supervisor reopen -> EventPump restart -> `OnDataChange` resumes; (b) `ReplayAsync` updates `SubscriptionRegistry` with new handles; (c) `StatusCodeMap.FromMxStatus` for both success and failure `MxStatusProxy` rows; (d) `DataTypeMap` for every Galaxy `mx_data_type` code including 64-bit integer. **Resolution:** Resolved 2026-05-22 — added `GalaxyDriverInfrastructureTests` covering `GetMemoryFootprint` (Driver.Galaxy-011) and `IAsyncDisposable` (Driver.Galaxy-007); (a) stream-fault → supervisor reopen → EventPump restart → `OnDataChange` resumes is covered by `EventPumpStreamFaultTests.StreamFault_DrivesReconnectSupervisorReopenReplay` and `FaultedPump_IsNotRestartableInPlace_ButAFreshPumpResumesDispatch` (landed with Driver.Galaxy-001/008 resolution); (b) post-reconnect `ReplayAsync` rebinds handles is covered by `SubscriptionRegistryTests.Rebind_*` suite; (c) `StatusCodeMap.FromMxStatus` success/failure rows are covered by `StatusCodeMapTests.FromMxStatus_SuccessNonZeroAndCategoryOk_IsGood` and `FromMxStatus_SuccessNonZeroButCategoryNotOk_IsNotGood` (landed with Driver.Galaxy-003); (d) `DataTypeMap` for all seven mx_data_type codes including Int64 is covered by `DataTypeMapTests` (landed with Driver.Galaxy-002). + +### Driver.Galaxy-015 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Security | +| Location | `libs/MxGateway.Client.dll`, `libs/MxGateway.Contracts.dll`, `libs/README.md` | +| Status | Open | + +**Description:** Commit `994997b` checks in two binary DLLs (`MxGateway.Client.dll`, 99 840 bytes; `MxGateway.Contracts.dll`, 489 984 bytes) under `src/Drivers/.../Driver.Galaxy/libs/` and references them via ``. These are the only checked-in binary build artefacts in the entire repo (a repo-wide `find` for non-`bin/`/`obj/` `*.dll` under `libs/` returns only these two), so the change sets a precedent. The accompanying `libs/README.md` states the DLLs are "byte-for-byte the build output" of the OtOpcUa team's own code against the gateway's open proto contracts, but there is no recorded provenance — no source-commit SHA from the sibling `mxaccessgw` repo that produced the build, no SHA-256/SHA-512 checksum, no `.gitattributes` rule marking these paths as binary (so a future churn-in-place will balloon the pack file). Without a recorded source commit + checksum it is impossible for a future reviewer/auditor to verify the binaries match a specific revision of the sibling repo — the assertion "we built them, not external" is unverifiable after the fact. Tampering or accidental swap (e.g. someone drops in a different DLL of the same name under the same path) would not be detectable. + +**Recommendation:** (a) Pin the source provenance: add the sibling `mxaccessgw` commit SHA used to build each DLL to `libs/README.md`. (b) Record a SHA-256 of each `.dll` in `libs/README.md` so a future tamper or accidental update is detectable by running `Get-FileHash`/`sha256sum`. (c) Add a `.gitattributes` rule under `libs/` declaring `*.dll binary` (and consider `filter=lfs diff=lfs merge=lfs -text` if/when these need to be updated, to avoid bloating the pack file on every refresh). (d) Optional: a `dotnet test` time-check that compares the on-disk hash to the recorded hash, so a CI run notices if the file drifts from what the README claims. + +### Driver.Galaxy-016 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Performance & resource management | +| Location | `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:43-47`, `libs/README.md:32-37` | +| Status | Open | + +**Description:** The five new `PackageReference` versions declared in the csproj (`Google.Protobuf` 3.34.1, `Grpc.Core.Api` 2.76.0, `Grpc.Net.Client` 2.71.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.0, `Polly` 8.5.2) do not all match what the vendored `MxGateway.Client.dll` was built against. The DLL's PE metadata (extracted via `System.Reflection.Metadata`) shows references to `Grpc.Net.Client v2.0.0.0`, `Microsoft.Extensions.Logging.Abstractions v10.0.0.0`, and notably `Polly.Core v8.0.0.0` — and the source csproj just before the sibling-repo rename (commit `bd4a09a` from 2026-04-27) declared `Grpc.Net.Client` 2.76.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.7, and `Polly.Core` 8.6.6 — *not* the meta-package `Polly`. Our driver pulls `Polly` 8.5.2 (which transitively pins `Polly.Core` 8.5.2 per its nuspec dependency), so the vendored client actually loads `Polly.Core` 8.5.2 at runtime against code compiled against 8.6.6. Across an 8.5 ↔ 8.6 minor delta this is usually safe (assembly-version is `v8.0.0.0` for both), but it is exactly the skew shape that surfaces as `MissingMethodException` if a 8.6-only API was used in the client. `libs/README.md` claims "versions match what the sibling repo's `ZB.MOM.WW.MxGateway.Contracts.csproj` uses so the gRPC + proto runtime stays binary-compatible" — that statement is correct only for `Google.Protobuf` and `Grpc.Core.Api`; the other three packages do not match. + +**Recommendation:** Reconcile the declared package versions with what the vendored DLLs were built against — bump to `Grpc.Net.Client` 2.76.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.7, swap `Polly` for `Polly.Core` 8.6.6 (the driver does not import the `Polly` legacy v7 surface, only Polly.Core via the client). Alternatively, rebuild the vendored DLLs against the same versions the csproj declares and refresh the binaries. Update `libs/README.md` to record the exact versions the DLLs were built against, so the next vendoring refresh has an authoritative reference. + +### Driver.Galaxy-017 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Design-document adherence | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/` (no source change), gateway proto contract | +| Status | Open | + +**Description:** The vendored `MxGateway.Contracts.dll` only carries the OLD `MxGateway.Contracts.Proto[.Galaxy]` namespace (PE-namespace dump confirms — `MxGateway.Client`, `MxGateway.Contracts`, `MxGateway.Contracts.Proto`, `MxGateway.Contracts.Proto.Galaxy` only). The sibling `mxaccessgw` repo's live `Protos/mxaccess_gateway.proto`, `mxaccess_worker.proto`, and `galaxy_repository.proto` files now generate into `ZB.MOM.WW.MxGateway.Contracts.Proto.*`. The proto wire format itself can still evolve (new RPCs, renamed fields, removed fields) and the driver has no contract-version handshake (a repo-wide search for `ContractVersion|ProtocolVersion|ApiVersion|WireVersion` in the driver returns nothing) — so a gateway service that evolves its proto past what the vendored client knows will fail silently at runtime: gRPC `UNIMPLEMENTED` for a renamed RPC, default-value reads for a removed scalar field, or worse, a wire-tag collision if a field number is reused. The risk surface grew with vendoring: previously the `ProjectReference` would have hard-failed at build time if the proto changed shape; now the driver builds green against a frozen contract that may not match the running gateway. + +**Recommendation:** (a) Add a single `Ping`/`GetVersion` RPC call at gateway-session open, comparing the gateway's reported contract version against a string baked into `libs/README.md` (or a `GatewayContractVersion` const) and refusing the session on mismatch with a clear log. (b) Document in `libs/README.md` the exact mxaccessgw commit SHA (and proto-file SHA-256s) the vendored DLLs were built from, so a parity-rig operator can grep the live gateway for the matching commit. (c) Add a soak/parity test that asserts the live gateway's proto descriptor still matches what the vendored DLL expects — fail loud rather than degrade. + +### Driver.Galaxy-018 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `libs/README.md:32-37`, `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:40-47` | +| Status | Open | + +**Description:** Several small documentation issues in the vendoring artefacts: +1. `libs/README.md` says "Versions match what the sibling repo's `ZB.MOM.WW.MxGateway.Contracts.csproj` uses" — but `ZB.MOM.WW.MxGateway.Contracts.csproj` only declares `Google.Protobuf` 3.34.1 and `Grpc.Core.Api` 2.76.0; the other three packages (`Grpc.Net.Client`, `Microsoft.Extensions.Logging.Abstractions`, `Polly`) come from the (now-deleted) `MxGateway.Client.csproj`, not the contracts csproj. The README points at the wrong source-of-truth file. See Driver.Galaxy-016 for the related version-skew issue. +2. `libs/README.md` says the DLLs "are built against net10.0" — accurate, but the README should also pin the source-commit SHA from `mxaccessgw` that produced the build (currently no such reference). Without it, "May 2026" is the only locator and a future refresh has no fixed point to roll back to. +3. The two `` items in the csproj omit `false`. The vendored DLLs carry `AssemblyVersion 1.0.0.0`; MSBuild's default for `` items is `SpecificVersion=true` only when the `Include` attribute contains version info, which it does not here, so this is benign — but spelling it out (`false`) would make a future refresh that bumps the AssemblyVersion robust without csproj edits. +4. The csproj `` value relies on the bare assembly simple-name; an explicit `` plus `false` would document the contract surface inside the csproj where a reviewer reads it. + +**Recommendation:** (a) Update `libs/README.md` to (i) point at `MxGateway.Client.csproj` for the `Grpc.Net.Client`/`Microsoft.Extensions.Logging.Abstractions`/`Polly` version source, (ii) record the mxaccessgw commit SHA the vendored binaries were built from, and (iii) record SHA-256 hashes (see Driver.Galaxy-015). (b) Add `false` to both `` items in the csproj to make the intent explicit and refresh-robust. diff --git a/code-reviews/README.md b/code-reviews/README.md index 00869eb..1fa8078 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -19,17 +19,17 @@ Each module's `findings.md` is the source of truth; this file is generated from | [Core](Core/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Core.Abstractions](Core.Abstractions/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | | [Core.AlarmHistorian](Core.AlarmHistorian/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 | -| [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | -| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 11 | +| [Core.ScriptedAlarms](Core.ScriptedAlarms/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 1 | 13 | +| [Core.Scripting](Core.Scripting/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 5 | 16 | | [Core.VirtualTags](Core.VirtualTags/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbCip](Driver.AbCip/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 15 | | [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | | [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 | -| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 6 | +| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 2 | 8 | | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 | -| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 14 | +| [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 4 | 18 | | [Driver.Historian.Wonderware](Driver.Historian.Wonderware/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.Historian.Wonderware.Client](Driver.Historian.Wonderware.Client/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 10 | | [Driver.Modbus](Driver.Modbus/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | @@ -46,7 +46,20 @@ Each module's `findings.md` is the source of truth; this file is generated from Findings with status `Open` or `In Progress`, ordered by severity. -_No pending findings._ +| ID | Severity | Category | Location | Description | +|---|---|---|---|---| +| Core.Scripting-012 | High | Security | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | The Core.Scripting-008 rewrite broadened the BCL references list from a narrow allow-list (`System.Private.CoreLib` + `System.Linq` only) to the full `TRUSTED_PLATFORM_ASSEMBLIES` set filtered to `System.*` + `netstandard` + `Microsoft.Win… | +| Driver.Cli.Common-007 | High | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:129` | Commit `5a9c459` added `0x80550000u => "BadDeviceFailure"` to the `FormatStatus` shortlist, but `0x80550000` is the canonical OPC UA spec value for `BadSecurityPolicyRejected`, not `BadDeviceFailure`. The correct spec value for `BadDeviceF… | +| Core.Scripting-013 | Medium | Security | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | The synthesized wrapper pastes the user's source verbatim between `{` and `}` braces inside a static method body, with a `#line 1` directive and no escaping. The legacy `CSharpScript.CreateDelegate` path was robust to this because Roslyn's… | +| Core.Scripting-014 | Medium | Concurrency & thread safety | `CompiledScriptCache.cs:91-103` (`Clear`) | `Clear()` snapshots `_cache.Keys.ToArray()` then iterates, calling `TryRemove(key, out var lazy)` on each — the key-only overload, not the value-scoped one used in `GetOrCompile`'s catch block. Between the snapshot and a given `TryRemove`,… | +| Core.Scripting-016 | Medium | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:74-117`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:139-182` | The Core.Scripting-008 resolution introduced `ScriptEvaluator.IDisposable` + `CompiledScriptCache.Clear()` that disposes each materialised evaluator before dropping its dictionary entry, so per-publish ALC accretion is no longer process-li… | +| Driver.Galaxy-015 | Medium | Security | `libs/MxGateway.Client.dll`, `libs/MxGateway.Contracts.dll`, `libs/README.md` | Commit `994997b` checks in two binary DLLs (`MxGateway.Client.dll`, 99 840 bytes; `MxGateway.Contracts.dll`, 489 984 bytes) under `src/Drivers/.../Driver.Galaxy/libs/` and references them via ``. These are the onl… | +| Driver.Galaxy-016 | Medium | Performance & resource management | `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:43-47`, `libs/README.md:32-37` | The five new `PackageReference` versions declared in the csproj (`Google.Protobuf` 3.34.1, `Grpc.Core.Api` 2.76.0, `Grpc.Net.Client` 2.71.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.0, `Polly` 8.5.2) do not all match what the vendo… | +| Core.ScriptedAlarms-013 | Low | Documentation & comments | `ScriptedAlarmEngine.cs:66-81` | The new internal test accessors `TryGetScratchReadCacheForTest` and `TryGetScratchContextForTest` (introduced by the Core.ScriptedAlarms-009 resolution at `0001cdd`) return the *live* per-alarm scratch — the same `Dictionary