diff --git a/code-reviews/Driver.Calculation.Contracts/findings.md b/code-reviews/Driver.Calculation.Contracts/findings.md new file mode 100644 index 00000000..c17b915b --- /dev/null +++ b/code-reviews/Driver.Calculation.Contracts/findings.md @@ -0,0 +1,55 @@ +# Code Review — Driver.Calculation.Contracts + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts` | +| Reviewer | Claude (automated review agent) | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 1 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found. `TryReadTagConfig`'s field-by-field reading (scriptId/scriptSource/changeTriggered/timerIntervalMs) matches its own documented lenient-defaulting rules exactly, and `Inspect`'s warnings track the same rules one-for-one — verified by cross-reading both against `TagConfigNonEnumParityTests.cs` in AdminUI.Tests. | +| 2 | OtOpcUa conventions | No issues found. This is a pure JSON-reading leaf with no transport, no gateway, and no Galaxy-specific surface — the relevant conventions here are the deploy-time TagConfig-gate architecture (G-7: one authority shared by the runtime read and the Inspect gate), which this module satisfies by construction (`CalculationTagDefinition.TryFromTagConfig`, Driver.Calculation, calls `CalculationTagDefinitionFactory.TryReadTagConfig`, this module, rather than re-implementing the blob read). | +| 3 | Concurrency & thread safety | No issues found. Purely functional (`JsonDocument.Parse` + local reads), no shared mutable state. | +| 4 | Error handling & resilience | No issues found. Both `TryReadTagConfig` and `Inspect` are documented never to throw and both live up to it: every `JsonDocument.Parse` call is wrapped in `try/catch (JsonException)`, and a malformed blob is a `false`/warning result rather than an exception escaping to the deploy pipeline or the runtime driver. | +| 5 | Security | No issues found. No secrets, no I/O beyond parsing an in-memory string; nothing here can be used for injection (script *execution* is a Roslyn-sandboxed concern owned by `Core.Scripting`, not this module). | +| 6 | Performance & resource management | 1 finding (Driver.Calculation.Contracts-001), Open, Low — a deliberate double-parse in `Inspect`, at deploy-time only (not a runtime hot path). | +| 7 | Design-document adherence | No issues found. The class-level remarks explicitly state the "one authority" split (this module owns what the BLOB says; `Driver.Calculation` owns identity + dependency-ref extraction) and the "Inspect is never stricter than TryReadTagConfig" invariant (pinned by a falsification-control test in AdminUI.Tests, per the project memory on this exact pattern) — both hold under inspection of the actual code. | +| 8 | Code organization & conventions | No issues found. Single-file, single-responsibility leaf; the split rationale (why `NamespaceMap`-style SDK-typed helpers don't belong here, why the dependency-ref extraction stays in `Driver.Calculation`) is documented inline and matches the actual dependency graph. | +| 9 | Testing coverage | No issues found. There is no dedicated `Driver.Calculation.Contracts.Tests` project, but this matches the fleet-wide convention — no `*.Contracts` driver library anywhere in the repo has one (`ls tests/Drivers \| grep -i Contracts` is empty). `TryReadTagConfig` is exercised indirectly via `CalculationTagDefinitionTests` in `Driver.Calculation.Tests`, and `Inspect` is directly tested in `tests/Server/ZB.MOM.WW.OtOpcUa.AdminUI.Tests/Uns/TagConfigNonEnumParityTests.cs` (`A_Calculation_blob_that_draws_an_advisory_but_parses_clean_is_accepted_at_authoring`), which specifically pins the "Inspect never stricter than the runtime read" invariant with a falsification control. | +| 10 | Documentation & comments | No issues found. The XML docs are accurate and unusually explicit about WHY the design is shaped the way it is (the authority split, the deliberately-weaker-than-AdminUI Inspect semantics called out in the review brief). | + +## Findings + +### Driver.Calculation.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Performance & resource management | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Contracts/CalculationTagDefinitionFactory.cs:107-160` (`Inspect`) | +| Status | Open | + +**Description:** `Inspect` parses `reference` with `JsonDocument.Parse` once directly (to check the root +kind and read `changeTriggered`/`timerIntervalMs` for the "present-but-discarded" warnings), and then — +when the object parsed cleanly — calls `TryReadTagConfig(reference, out _)` (line 133), which parses the +SAME string a second time from scratch to decide whether a usable `scriptId` is present. This is a +deliberate consequence of the module's own "one authority" design (`Inspect` must consult +`TryReadTagConfig`, not reimplement its scriptId rule), so it is not a bug, but it is a real double-parse +of every calc tag's `TagConfig` blob on every deploy-time gate pass. + +**Recommendation:** If this ever needs to scale beyond a deploy-time gate (e.g. if `Inspect` starts being +called from a hot path, which it is not today), consider giving `TryReadTagConfig` an internal overload +that accepts an already-parsed `JsonElement` so `Inspect` can pass its own `root` through without a +second `JsonDocument.Parse`. Not pursued in this review: `TryReadTagConfig`'s public string-based +signature is consumed by other modules (`Driver.Calculation`'s `CalculationTagDefinition.TryFromTagConfig`, +and `AdminUI`'s `CalculationTagConfigModel.cs`), so widening it safely is a small cross-module change, not +a same-file one-liner, and the current cost is one extra JSON parse per calc tag at deploy time — not a +runtime cost. + +**Resolution:** _(Open — Low severity, record-only per review policy; deploy-time only, not a hot path.)_ diff --git a/code-reviews/Driver.Calculation/findings.md b/code-reviews/Driver.Calculation/findings.md new file mode 100644 index 00000000..1b4c31a5 --- /dev/null +++ b/code-reviews/Driver.Calculation/findings.md @@ -0,0 +1,225 @@ +# Code Review — Driver.Calculation + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation` | +| Reviewer | Claude (automated review agent) | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 1 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | 3 findings (Driver.Calculation-001, -002, -003), all Resolved | +| 2 | OtOpcUa conventions | No issues found. Galaxy/MXAccess-specific conventions do not apply to this pseudo-driver (no transport, no gateway). It correctly implements the capability-interface seams (IDriver, IDependencyConsumer, ISubscribable, IReadable, ISkippedTagReporter), .NET 10/AnyCPU, and authorization stays out of driver-level code. | +| 3 | Concurrency & thread safety | 1 finding (Driver.Calculation-005), Open (cross-module) | +| 4 | Error handling & resilience | Covered by Driver.Calculation-002 (script-evaluation faults never throw out of the driver; SkippedTagLedger tolerates a single bad tag without failing the whole rebuild). No further issues found. | +| 5 | Security | No issues found. No secrets, no I/O, no SQL; the sandboxed Roslyn evaluator (shared Core.Scripting sandbox) is the security boundary and lives outside this module. | +| 6 | Performance & resource management | Covered by Driver.Calculation-005 (blocking evaluation under a shared lock). CalculationTimerScheduler correctly bounds outstanding work to one tick per interval group (skip-if-in-flight) and CalculationEvaluator.ClearCompiledScripts() is invoked on every ReinitializeAsync, so compiled-script AssemblyLoadContexts do not accrete across script edits. No further issues found. | +| 7 | Design-document adherence | No issues found. The class-level XML docs cite the "mini-design" sections directly and the implementation matches them (change-gate, once-per-transition Bad, force-published recovery, GetHealth() per Gitea #517, ISkippedTagReporter per Gitea #541). | +| 8 | Code organization & conventions | 1 finding (Driver.Calculation-003, also an organization smell: three near-duplicate JsonSerializerOptions definitions across the module). Otherwise clean: one construction routine (RebuildTagTable) shared by the ctor and ReinitializeAsync so the two paths cannot drift. | +| 9 | Testing coverage | 1 finding (Driver.Calculation-004), Resolved (tests added). CalculationTimerScheduler.SkippedTickCount remains untested — see note under -004. | +| 10 | Documentation & comments | No issues found. XML docs are extensive, accurate, and were kept in sync with the fixes below. | + +## Findings + +### Driver.Calculation-001 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:103-106` (pre-fix), `CalculationEvaluator.cs:47,56-59` (pre-fix) | +| Status | Resolved | + +**Description:** `CalculationDriver`'s constructor built the evaluator's logger as +`_logger is ILogger el ? el : NullLogger.Instance`, where +`_logger` is typed `ILogger`. `ILogger` and +`ILogger` are unrelated closed generic interfaces (no inheritance relationship +between `CalculationDriver` and `CalculationEvaluator`), so the concrete logger instance the host injects +(a `Logger` from `ILoggerFactory.CreateLogger()`, or +`NullLogger.Instance`) never actually implements `ILogger`. The +`is` check was therefore **always false**, and every `CalculationEvaluator` silently ran with +`NullLogger.Instance` regardless of what logger the driver itself was constructed +with — swallowing the evaluator's host-side diagnostics (Roslyn compile failures, sandbox violations, +unexpected execution throws, the single-tag `SetVirtualTag`-drop Debug line) with no way to route them to +the real host log sink. (The actual script-failure *reason* still reached the operator via the +`script-logs` DPS topic through `CalculationDriver.PublishScriptLog`, so this was a silent loss of +redundant host-side diagnostics rather than a loss of the primary operator signal — hence Medium, not +High.) + +**Recommendation:** Have `CalculationEvaluator` accept the non-generic `ILogger` (every `ILogger` +already implements it) instead of the closed generic `ILogger`, and pass the +driver's own `_logger` straight through. + +**Resolution:** Fixed 2026-08-10 (branch `review/calculation-session`). Changed +`CalculationEvaluator`'s constructor parameter/field from `ILogger` to `ILogger`, +and `CalculationDriver`'s constructor now does `_evaluator = new CalculationEvaluator(_logger, _scriptRoot, +_options.RunTimeout);` with no type gymnastics. Regression test +`CalculationDriverTests.Evaluator_compile_diagnostics_flow_through_to_the_drivers_own_logger` (a +capturing `ILogger` passed into the driver, a deliberately broken timer-triggered +script, asserting the capturing logger receives the "Roslyn compile failed" Warning) fails against the +pre-fix code and passes after. + +--- + +### Driver.Calculation-002 + +| Field | Value | +|---|---| +| Severity | High | +| Category | Correctness & logic bugs | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:374-410` (pre-fix `EvaluateTag`) | +| Status | Resolved | + +**Description:** `EvaluateTag` computed the change-gate flag (`allArrived`) but only used it to decide +whether to *publish* the result — it called `_evaluator.Evaluate(...)` **before** checking `allArrived`, +against a dependency snapshot that only contains the deps that HAVE arrived. `VirtualTagContext.GetTag` +(shared with VirtualTag scripts) returns a Bad snapshot with `.Value == null` for a key not present in +the read cache. The idiomatic calc-tag script shape used throughout this module's own tests and comments — +`return (double)ctx.GetTag("src/a").Value + (double)ctx.GetTag("src/b").Value;` — casts that `null` to a +non-nullable value type, which throws `NullReferenceException` at the unboxing conversion. That exception +was caught by `CalculationEvaluator.Evaluate`'s generic `catch (Exception ex)` and reported as a +`Failure("script threw: Object reference not set to an instance of an object.")`, which +`OnEvaluationFailed` then unconditionally logged to the `script-logs` topic as a Warning — **on every +partial-dependency arrival**, for any multi-dependency change-triggered calc tag whose declared deps +don't all arrive on the same tick. This is not a corner case: it fires on ordinary warm-up (every deploy +and every driver restart) for the standard multi-input calc-tag shape, producing a false-positive +"script threw" warning that looks exactly like a real script bug even though nothing is wrong, plus a +wasted Roslyn/compiled-cache invocation per partial arrival. + +**Recommendation:** Move the `allArrived` check before the call to `_evaluator.Evaluate` so the script +never runs against a snapshot it wasn't declared against, matching the class's own documented intent +("publish nothing until every declared dep has arrived at least once"). + +**Resolution:** Fixed 2026-08-10 (branch `review/calculation-session`). `EvaluateTag` now returns before +building the dependency snapshot / calling the evaluator when `!def.DependencyRefs.All(_arrivedDeps.Contains)`. +`OnEvaluationFailed`'s now-always-true `allArrived` parameter was removed (the caller only reaches it once +every dep has arrived). Regression test +`CalculationDriverTests.No_evaluation_or_script_log_while_a_declared_dep_is_still_missing` reproduces the +exact scenario from `No_publish_until_every_declared_dep_has_arrived` but also asserts `logs.ShouldBeEmpty()`; +it fails with the captured `"script threw: Object reference not set..."` entry against the pre-fix code +and passes after. The pre-existing `No_publish_until_every_declared_dep_has_arrived` and +`Failure_publishes_Bad_once_per_transition_emits_script_log_and_recovers` tests are unaffected (neither +exercises a multi-dep tag mid-warm-up while asserting on `logs`). + +--- + +### Driver.Calculation-003 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Correctness & logic bugs / Code organization & conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs:16-21` (pre-fix) | +| Status | Resolved | + +**Description:** The module defines its config-parsing `JsonSerializerOptions` in three places — +`CalculationDriverFactoryExtensions.JsonOptions` (spawn), `CalculationDriver.ReinitJsonOptions` (in-place +ApplyDelta), and `CalculationDriverProbe.Opts` (Test-Connect) — and the first two both set +`ReadCommentHandling = JsonCommentHandling.Skip` and `AllowTrailingCommas = true` while the probe's did +not. A config blob the real spawn/reinit path tolerates (a trailing comma or a `//` comment, plausible for +a hand-edited config used during troubleshooting) could therefore fail Test-Connect with "Config JSON is +invalid" even though deploying the identical JSON would succeed — the probe was **stricter** than the path +it exists to stand in for, the opposite of its documented contract ("Never throws; returns Ok with +negligible latency" / parse-only parity with the real path). + +**Recommendation:** Align `CalculationDriverProbe.Opts` with the other two option sets (or extract a +single shared `JsonSerializerOptions` constant the three call sites reuse, to remove the duplication risk +entirely for future changes). + +**Resolution:** Fixed 2026-08-10 (branch `review/calculation-session`). Added +`ReadCommentHandling = JsonCommentHandling.Skip` and `AllowTrailingCommas = true` to `Opts`, with a +comment naming the two option sets it must stay in parity with. New test file +`CalculationDriverProbeTests.cs` (previously nonexistent — see Driver.Calculation-004) pins this with +`Tolerates_trailing_commas_like_the_real_spawn_and_reinit_paths` and +`Tolerates_comments_like_the_real_spawn_and_reinit_paths`, both of which fail against the pre-fix `Opts` +and pass after. + +--- + +### Driver.Calculation-004 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Testing coverage | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs`, `CalculationDriver.cs` (`ISkippedTagReporter`/`GetSkippedTags()`), `CalculationTimerScheduler.cs` (`SkippedTickCount`) | +| Status | Resolved | + +**Description:** Two behaviours in this module had zero test coverage before this review: + +1. `CalculationDriverProbe` had no test file at all — the Test-Connect surface for this driver type was + completely unverified (and, per Driver.Calculation-003, was in fact wrong). +2. `CalculationDriver`'s implementation of `ISkippedTagReporter`/`GetSkippedTags()` (the Gitea #541 + authored-vs-mapped-tag visibility feature — the exact class of bug that let a driver silently ignore + 20 of 22 tags while still reporting Healthy) had no test verifying a raw tag with no `scriptId` is + actually reported skipped, that `AuthoredCount`/`SkippedCount`/`Sample` are correct, or that the report + is rebuilt (not stale) after `ReinitializeAsync`. + +`CalculationTimerScheduler.SkippedTickCount` (the in-flight-tick-skip counter) remains untested — left +open rather than force-fitted, since reliably forcing a same-group timer overlap without a sleep-based +flaky test would need either an injectable clock/scheduler or an artificially slow evaluator hook, neither +of which exists yet in this module. + +**Recommendation:** Add a dedicated `CalculationDriverProbeTests.cs`; add `GetSkippedTags()` coverage to +`CalculationDriverTests.cs`. For `SkippedTickCount`, a follow-up could add a test-only evaluator delay hook +if the counter ever needs to be pinned by a test rather than only exercised implicitly. + +**Resolution:** Fixed 2026-08-10 (branch `review/calculation-session`). Added +`tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverProbeTests.cs` (6 tests, +including the two parity regressions from -003). Added to `CalculationDriverTests.cs`: +`GetSkippedTags_reports_a_raw_tag_with_no_scriptId_alongside_the_authored_total`, +`GetSkippedTags_has_no_skips_when_every_authored_tag_maps`, and +`GetSkippedTags_updates_after_reinitialize`. `SkippedTickCount` is intentionally left without a dedicated +test — noted above, not silently dropped. + +--- + +### Driver.Calculation-005 + +| Field | Value | +|---|---| +| Severity | Medium | +| Category | Concurrency & thread safety | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs:300-314` (`OnDependencyValue`), `:374-410` (`EvaluateTag`); cross-module: `src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs` | +| Status | Open | + +**Description:** `OnDependencyValue` evaluates every change-triggered dependent tag **inline, under +`_lock`**, and `EvaluateTag`'s call into `CalculationEvaluator.Evaluate` ultimately runs +`TimedScriptEvaluator<...>.RunAsync(context).GetAwaiter().GetResult()` — a synchronous blocking wait bounded +by `RunTimeout` (default 2 s, configurable via `RunTimeoutMs`, with no enforced ceiling). `OnDependencyValue` +is invoked directly and synchronously from `DependencyConsumerMuxAdapter.OnDependencyValueChanged` +(`src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/DependencyConsumerMuxAdapter.cs:66-70`), an Akka +`ReceiveActor`'s message handler — i.e. on the actor's dispatcher thread. A calc script that runs close to +its timeout budget therefore blocks that thread (and, transitively, that specific driver's dependency-value +mailbox) for up to `RunTimeout` on every dependency arrival, and — because `TimedScriptEvaluator` offloads +the actual run via `Task.Run` while the caller blocks synchronously waiting on it — occupies a pool thread +for the duration rather than yielding it, which is a known thread-pool-starvation risk under Akka's +fork-join dispatcher if enough calc drivers are evaluating slow scripts concurrently. The in-line comment +("the compile cache makes this a bounded method invocation — the same 'fast enough to run inline' contract +as the VT evaluator") is not actually enforced by anything faster than `RunTimeout` itself — the existence +of the timeout mechanism is evidence a script CAN run slow, not evidence it never will. + +This exact pattern (`_evaluator.Evaluate(...)` called unconditionally, inline, from an Akka actor's +synchronous message handler, with the identical `GetAwaiter().GetResult()` blocking shape) is also present +in `VirtualTagActor.OnDependencyChanged` (`src/Server/ZB.MOM.WW.OtOpcUa.Runtime/VirtualTags/VirtualTagActor.cs:179-199`), +which `CalculationDriver`'s own docs explicitly say this module mirrors — so this is a systemic, +pre-existing trade-off in the scripting engine, not something introduced by the Calculation driver alone. +Recorded here because the blocking call itself lives in this module's code (`CalculationDriver.cs`), even +though a full fix spans `Core.Abstractions` (the synchronous `IDependencyConsumer.OnDependencyValue` +contract) and `Runtime` (the adapter actor) — both out of this review's module scope. + +**Recommendation:** Not a fix for this review (cross-module: the synchronous `IDependencyConsumer` contract +lives in `Core.Abstractions`, and the actor calling it lives in `Runtime`). If this risk is judged worth +closing, options include: (a) giving `IDependencyConsumer.OnDependencyValue` an async signature so the actor +can `PipeTo` the result instead of blocking its own thread; (b) having `CalculationDriver` dispatch evaluation +onto a dedicated bounded scheduler (e.g. `Task.Run` from `OnDependencyValue` itself, publishing the result +asynchronously) so the actor thread returns immediately; or (c) tightening `RunTimeout`'s effective ceiling +independent of the `RunTimeoutMs` config knob. Any of these should be scoped and reviewed together with the +equivalent `VirtualTagActor` behaviour so the two mirrors do not diverge. + +**Resolution:** _(Open — cross-module, not fixed as part of this review.)_ diff --git a/code-reviews/Driver.OpcUaClient.Session/findings.md b/code-reviews/Driver.OpcUaClient.Session/findings.md new file mode 100644 index 00000000..f7d38d20 --- /dev/null +++ b/code-reviews/Driver.OpcUaClient.Session/findings.md @@ -0,0 +1,29 @@ +# Code Review — Driver.OpcUaClient.Session + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Session` | +| Reviewer | Claude (automated review agent) | +| Review date | 2026-08-10 | +| Commit reviewed | `b990adae` | +| Status | Reviewed | +| Open findings | 0 | + +## Checklist coverage + +| # | Category | Result | +|---|---|---| +| 1 | Correctness & logic bugs | No issues found. `ToStableReference`/`TryResolve` correctly implement the namespace-URI-stable NodeId encoding described in `docs/v2/driver-specs.md` §8: namespace 0 keeps the compact `i=…` form (spec-fixed, never reorders), a nonzero namespace is rewritten to `nsu=;=`, an out-of-range namespace index falls back to the raw form rather than throwing, and the static session-based `TryResolve` re-binds a captured `nsu=…` reference against the CURRENT session's namespace table (correcting a post-connect reorder) while still accepting a plain `ns=N;…` form for hand-authored config. `BuildRawPathTable`'s per-entry skip rules (blank RawPath, unreadable/absent `nodeId`) match what the class doc promises ("the driver surfaces such a reference as `BadNodeIdInvalid`") and what `OpcUaClientNamespaceTests.cs` exercises. | +| 2 | OtOpcUa conventions | No issues found. Uses the OPC Foundation UA .NET Standard stack's own types (`Opc.Ua.NodeId`, `NamespaceTable`, `ISession`) exactly as intended rather than re-deriving namespace semantics by hand; .NET 10/AnyCPU; no Galaxy/MXAccess surface (not applicable to this module). | +| 3 | Concurrency & thread safety | No issues found. `NamespaceMap` is effectively immutable after construction (`_uris`, `_uriToIndex`, `_rawPathToNode` are all populated once in the constructor from freshly-built collections that are never exposed for external mutation), so concurrent reads from multiple threads after a connect are safe with no locking needed. A reconnect is expected to build a fresh `NamespaceMap` instance (out of this module's scope to verify the call site, but nothing in this class assumes otherwise). | +| 4 | Error handling & resilience | No issues found. Every public entry point follows the `TryXxx` contract consistently: `TryResolve` (both the instance RawPath overload and the static session-based overload) returns `false` rather than throwing for null/blank/unknown input, and the static overload's `NodeId.Parse` call is wrapped in a `try/catch` that treats any parse failure as a clean `false` — including a concurrently-torn-down session, which the XML doc calls out explicitly as an expected caller scenario on the reconnect path. | +| 5 | Security | No issues found. No secrets, no network I/O of its own (session I/O is owned by the consuming driver), no injection surface — this is a pure in-memory table + string-encoding helper. | +| 6 | Performance & resource management | No issues found. `Count`/`UriForIndex`/`IndexForUri`/`TryResolve`/`TryGetBinding` are all O(1) dictionary/array lookups; `FromSession`/`FromTable` do one O(n) pass over the namespace table and one O(m) pass over the authored raw tags, both proportional to input size with no redundant work. No `IDisposable` state to manage (nothing here owns a session, socket, or other unmanaged resource). | +| 7 | Design-document adherence | No issues found. The "why this type sits in `.Session` and not `.Contracts`" remark accurately describes the actual dependency shape (`.Contracts` stays free of the `OPCFoundation.NetStandard.Opc.Ua.Client` package; the blob read (`OpcUaClientTagDefinitionFactory.TryReadNodeId`) stays in `.Contracts` as the one authority the deploy-time gate and this table share) — confirmed by reading the referenced `.Contracts` factory and its call site here (`BuildRawPathTable`, line 134). | +| 8 | Code organization & conventions | No issues found. Single-file, single-responsibility extraction exactly as described in `CLAUDE.md`'s summary of this module ("a small shared-helper project ... so the OpcUaClient Browser and driver could share namespace-mapping logic without the Browser referencing the runtime driver") — confirmed the module has no dependency on `Driver.OpcUaClient` itself, only on `Core.Abstractions` (`RawTagEntry`) and the OPC UA SDK. | +| 9 | Testing coverage | No issues found. `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.OpcUaClient.Tests/OpcUaClientNamespaceTests.cs` covers the encoding contract (namespace-0 compact form, nonzero-namespace URI encoding, unknown-index fallback, bidirectional index/URI lookup), the v3 RawPath resolution table (successful resolution, unknown/blank RawPath, `WriteIdempotent` threading, an entry with no usable `nodeId` being skipped, legacy PascalCase `NodeId` key tolerance, an empty-raw-tags map), and the null-session negative case of the static session-based `TryResolve`. The success path of the static session-based `TryResolve` (a real, non-null `ISession`) is not exercised in this module's own test file — reasonably so, since constructing a live/fake `ISession` from the SDK is impractical to do in isolation here; it is exercised end-to-end via the consuming driver's integration test suites (`Driver.OpcUaClient.IntegrationTests`, `Driver.OpcUaClient.Browser.IntegrationTests`), which is out of this review's module scope to inspect further. | +| 10 | Documentation & comments | No issues found. The class- and member-level XML docs are thorough and verifiably accurate against the implementation (the namespace-reorder rationale, the `nsu=…`/`ns=N;…` dual-form acceptance, the RawPath v3 resolution table's relationship to `TagConfig.nodeId`, and the `.Session`-vs-`.Contracts` placement rationale all match what the code actually does). | + +## Findings + +_No findings recorded — no defects, coverage gaps, or design-adherence issues were found in this module at the reviewed commit. See the checklist notes above for the review evidence per category._ diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs index 3c1d9f2f..4e5c2065 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriver.cs @@ -100,10 +100,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba _driverInstanceId = driverInstanceId; _logger = logger ?? NullLogger.Instance; _scriptRoot = scriptRoot ?? new ScriptRootLogger(new LoggerConfiguration().CreateLogger()); - _evaluator = new CalculationEvaluator( - _logger is ILogger el ? el : NullLogger.Instance, - _scriptRoot, - _options.RunTimeout); + // _logger (ILogger) implements the non-generic ILogger, so it is passed straight + // through — the evaluator's host-side diagnostics (compile errors, sandbox violations, execution + // throws) land on the SAME logger the driver itself was constructed with, not a separate one. + _evaluator = new CalculationEvaluator(_logger, _scriptRoot, _options.RunTimeout); RebuildTagTable(_options); } @@ -379,7 +379,14 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba var st = _stateByRawPath[rawPath]; // Change-gate (mini-design §4): publish nothing until every declared dep has arrived at least once. - var allArrived = def.DependencyRefs.All(_arrivedDeps.Contains); + // MUST be checked BEFORE the evaluator runs, not just before the publish: a partially-warmed-up + // dependency snapshot is missing entries for the not-yet-arrived deps, and VirtualTagContext.GetTag + // returns a Bad snapshot (.Value == null) for those — so the common `(double)ctx.GetTag("x").Value` + // idiom throws NullReferenceException on every warm-up tick for any multi-dependency script whose + // deps don't all arrive in the same tick. Evaluating anyway (and discarding the result afterwards) + // used to turn ordinary warm-up into a spurious "script threw" script-log Warning on every partial + // arrival, plus a wasted Roslyn/compiled-cache invocation. + if (!def.DependencyRefs.All(_arrivedDeps.Contains)) return; // still warming up — do not evaluate yet // Build the per-tag dependency snapshot (only its declared reads, current values). var deps = new Dictionary(StringComparer.Ordinal); @@ -390,12 +397,10 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba if (!result.Success) { - OnEvaluationFailed(def, st, result.Reason ?? "evaluator failure", allArrived, timestampUtc); + OnEvaluationFailed(def, st, result.Reason ?? "evaluator failure", timestampUtc); return; } - if (!allArrived) return; // change-trigger warm-up: hold until inputs are ready - // Value dedup — but bypass it while recovering from Bad so a recovery whose value equals the // pre-failure value still republishes Good (otherwise the node would stay Bad forever). if (!st.LastPublishedBad && st.HasValue && Equals(st.LastValue, result.Value)) @@ -411,12 +416,13 @@ public sealed class CalculationDriver : IDriver, IDependencyConsumer, ISubscriba /// Failure tail (mirrors VirtualTagActor 02/S13): always emits the per-failure /// script-log; additionally publishes a Bad snapshot — carrying the last-known value — once per - /// Good→Bad transition and only after every declared dependency has arrived. - private void OnEvaluationFailed( - CalculationTagDefinition def, TagState st, string reason, bool allArrived, DateTime timestampUtc) + /// Good→Bad transition. The caller () only reaches here once every declared + /// dependency has arrived — the warm-up gate returns before the evaluator ever runs — so there is no + /// separate "inputs ready" condition to re-check here. + private void OnEvaluationFailed(CalculationTagDefinition def, TagState st, string reason, DateTime timestampUtc) { PublishScriptLog(def, reason); - if (st.LastPublishedBad || !allArrived) return; + if (st.LastPublishedBad) return; st.LastPublishedBad = true; st.LastTimestampUtc = timestampUtc; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs index 060cf5bd..0a34b959 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationDriverProbe.cs @@ -13,10 +13,16 @@ namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation; /// public sealed class CalculationDriverProbe : IDriverProbe { + // Mirrors CalculationDriverFactoryExtensions.JsonOptions / CalculationDriver.ReinitJsonOptions + // (AllowTrailingCommas + ReadCommentHandling included) so a config blob the real spawn/reinit path + // tolerates never fails Test-Connect as "invalid JSON" — the probe must never be STRICTER than the + // path it is standing in for. private static readonly JsonSerializerOptions Opts = new() { PropertyNameCaseInsensitive = true, UnmappedMemberHandling = JsonUnmappedMemberHandling.Skip, + ReadCommentHandling = JsonCommentHandling.Skip, + AllowTrailingCommas = true, Converters = { new JsonStringEnumConverter() }, }; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs index 481ce36e..b3f069cf 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation/CalculationEvaluator.cs @@ -44,17 +44,20 @@ public sealed class CalculationEvaluator : IScriptCacheOwner, IDisposable // that dispose each evaluator's collectible AssemblyLoadContext. ClearCompiledScripts() — driven by the // WP7 driver per deploy generation — is what stops ALCs accreting across script edits. private readonly CompiledScriptCache _cache = new(); - private readonly ILogger _logger; + private readonly ILogger _logger; private readonly SerilogLogger _scriptRoot; private readonly TimeSpan _runTimeout; private bool _disposed; /// Initializes a new . - /// Logger for host-side compilation/execution diagnostics. + /// Logger for host-side compilation/execution diagnostics. Deliberately the + /// non-generic (rather than ILogger<CalculationEvaluator>) so the + /// caller can forward its OWN ILogger<T> — every ILogger<T> already + /// implements this — instead of needing a second, separately-obtained logger instance. /// Root script logger; user ctx.Logger.* output flows through this to the Script-log page. /// Wall-clock budget per script run; defaults to 2 seconds (parity with the VT evaluator). public CalculationEvaluator( - ILogger logger, + ILogger logger, ScriptRootLogger scriptRoot, TimeSpan? runTimeout = null) { diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverProbeTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverProbeTests.cs new file mode 100644 index 00000000..5d1be8f4 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverProbeTests.cs @@ -0,0 +1,72 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests; + +/// +/// Unit tests for — the Calculation pseudo-driver's Test-Connect +/// probe (parse-only, since there is no backend to reach). +/// +public sealed class CalculationDriverProbeTests +{ + private static readonly CalculationDriverProbe Sut = new(); + + [Fact] + public async Task Empty_config_json_fails() + { + var result = await Sut.ProbeAsync("", TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeFalse(); + result.Message.ShouldNotBeNullOrWhiteSpace(); + } + + [Fact] + public async Task Malformed_json_fails() + { + var result = await Sut.ProbeAsync("not json", TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeFalse(); + result.Message.ShouldNotBeNullOrWhiteSpace(); + } + + [Fact] + public async Task A_config_with_no_rawTags_array_is_still_valid() + { + var result = await Sut.ProbeAsync("{}", TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeTrue(result.Message); + } + + [Fact] + public async Task A_well_formed_rawTags_config_is_valid() + { + const string json = """{"rawTags":[{"rawPath":"calc/e/x","tagConfig":"{}","writeIdempotent":false}]}"""; + var result = await Sut.ProbeAsync(json, TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeTrue(result.Message); + } + + /// + /// The probe must not be STRICTER than the real spawn/reinit path: both + /// CalculationDriverFactoryExtensions.JsonOptions and CalculationDriver.ReinitJsonOptions + /// set AllowTrailingCommas + ReadCommentHandling.Skip. Before this pin the probe's own + /// options omitted both, so a config a real deploy would accept could fail Test-Connect as "invalid". + /// + [Fact] + public async Task Tolerates_trailing_commas_like_the_real_spawn_and_reinit_paths() + { + const string json = """{"rawTags":[{"rawPath":"calc/e/x","tagConfig":"{}","writeIdempotent":false},],}"""; + var result = await Sut.ProbeAsync(json, TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeTrue(result.Message); + } + + /// Same parity claim for comments, which the spawn/reinit path also tolerates. + [Fact] + public async Task Tolerates_comments_like_the_real_spawn_and_reinit_paths() + { + const string json = """ + { + // a hand-edited config comment + "rawTags": [] + } + """; + var result = await Sut.ProbeAsync(json, TimeSpan.FromSeconds(1), TestContext.Current.CancellationToken); + result.Ok.ShouldBeTrue(result.Message); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs index 6846d8f4..276b7f13 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationDriverTests.cs @@ -1,4 +1,5 @@ using System.Collections.Concurrent; +using Microsoft.Extensions.Logging; using Serilog; using Serilog.Events; using Shouldly; @@ -74,6 +75,27 @@ public sealed class CalculationDriverTests e.Snapshot.StatusCode.ShouldBe(StatusGood); } + /// + /// The change-gate must hold BEFORE the script ever runs, not just before the publish. A + /// multi-dependency script that casts a missing dep's .Value to a non-nullable type (the + /// common idiom used throughout this file) throws when evaluated against a partial snapshot, so + /// evaluating anyway during ordinary warm-up produced a spurious "script threw" script-log Warning + /// on every partial dependency arrival — even though nothing is actually wrong with the script. + /// + [Fact] + public async Task No_evaluation_or_script_log_while_a_declared_dep_is_still_missing() + { + var (driver, published, logs) = Build( + CalcTag("calc/e/sum", "return (double)ctx.GetTag(\"src/a\").Value + (double)ctx.GetTag(\"src/b\").Value;")); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + driver.OnDependencyValue("src/a", 10.0, StatusGood, DateTime.UtcNow); + + published.ShouldBeEmpty(); + logs.ShouldBeEmpty("a still-missing declared dependency must hold the gate BEFORE evaluation, " + + "not merely before the publish"); + } + // ── dedup + recompute ───────────────────────────────────────────────────────────────────── [Fact] @@ -209,6 +231,102 @@ public sealed class CalculationDriverTests driver.GetHealth().State.ShouldBe(DriverState.Unknown); } + // ── evaluator diagnostics reach the driver's own logger ────────────────────────────────────── + + /// + /// The the driver constructs internally must log its host-side + /// diagnostics (compile errors, sandbox violations, execution throws) through the SAME + /// ILogger<CalculationDriver> the driver itself was given — not silently into a + /// no-op logger. A timer-triggered tag with a deliberately broken script is the vehicle: it + /// evaluates on its own cadence with no dependency needed to arrive first. + /// + [Fact] + public async Task Evaluator_compile_diagnostics_flow_through_to_the_drivers_own_logger() + { + var logger = new CapturingLogger(); + var scriptRoot = new ScriptRootLogger(new LoggerConfiguration().CreateLogger()); + var driver = new CalculationDriver( + new CalculationDriverOptions + { + RawTags = [CalcTag("calc/e/bad", "this is not valid C#;", changeTriggered: false, timerMs: 50)], + }, + "calc-drv", scriptRoot, logger); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var fired = await WaitUntil( + () => logger.Entries.Any(e => e.Level == LogLevel.Warning && e.Message.Contains("Roslyn compile failed")), + TimeSpan.FromSeconds(3)); + + fired.ShouldBeTrue("the evaluator's compile-failure diagnostic should reach the driver-supplied logger"); + + await driver.ShutdownAsync(TestContext.Current.CancellationToken); + } + + // ── skipped-tag reporting (Gitea #541) ──────────────────────────────────────────────────────── + + [Fact] + public async Task GetSkippedTags_reports_a_raw_tag_with_no_scriptId_alongside_the_authored_total() + { + var noScriptId = new RawTagEntry("calc/e/nogood", "{\"foo\":\"bar\"}", WriteIdempotent: false); + var good = CalcTag("calc/e/good", "return 1.0;", changeTriggered: false); + var (driver, _, _) = Build(noScriptId, good); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var report = driver.GetSkippedTags(); + + report.ShouldNotBeNull(); + report!.AuthoredCount.ShouldBe(2); + report.SkippedCount.ShouldBe(1); + report.Sample.ShouldContain(s => s.RawPath == "calc/e/nogood"); + } + + [Fact] + public async Task GetSkippedTags_has_no_skips_when_every_authored_tag_maps() + { + var (driver, _, _) = Build(CalcTag("calc/e/good", "return 1.0;", changeTriggered: false)); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + + var report = driver.GetSkippedTags(); + + report.ShouldNotBeNull(); + report!.AuthoredCount.ShouldBe(1); + report.SkippedCount.ShouldBe(0); + report.HasSkips.ShouldBeFalse(); + } + + /// The skip ledger must be REBUILT on ReinitializeAsync, not just at construction — an + /// ApplyDelta that drops a tag's scriptId (or adds a new tag with none) must be reflected immediately, + /// since RebuildTagTable is the one routine both the ctor and reinit share. + [Fact] + public async Task GetSkippedTags_updates_after_reinitialize() + { + var (driver, _, _) = Build(CalcTag("calc/e/x", "return 1.0;", changeTriggered: false)); + await driver.InitializeAsync("{}", TestContext.Current.CancellationToken); + driver.GetSkippedTags()!.SkippedCount.ShouldBe(0); + + const string reinitJson = """{"rawTags":[{"rawPath":"calc/e/x","tagConfig":"{}","writeIdempotent":false}]}"""; + await driver.ReinitializeAsync(reinitJson, TestContext.Current.CancellationToken); + + var report = driver.GetSkippedTags(); + report!.AuthoredCount.ShouldBe(1); + report.SkippedCount.ShouldBe(1, "the reinit payload's tag has no scriptId, so it is no longer a calc tag"); + } + + /// Minimal capturing so a test can assert on what the driver's own + /// logger received (mirrors the equivalent helper in CalculationEvaluatorTests). + private sealed class CapturingLogger : ILogger + { + public List<(LogLevel Level, string Message)> Entries { get; } = []; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => + Entries.Add((logLevel, formatter(state, exception))); + } + private sealed class CapturingPublisher(List sink) : IScriptLogPublisher { private readonly object _lock = new();