lmxopcua/code-reviews/Driver.Calculation.Contracts/findings.md

# 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 | 0 |

## 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 | Resolved |

**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 (2026-08-10):** Fixed in `73230a42` — and it turned out to be a same-file change after all,
because widening the *public* signature was never necessary. The field-by-field read moved into a private
`TryReadRoot(JsonElement root, out CalculationTagConfigFields fields)`; `TryReadTagConfig(string, …)` keeps
its exact signature (still the entry point `Driver.Calculation`'s `CalculationTagDefinition.TryFromTagConfig`
and AdminUI's `CalculationTagConfigModel` consume) and now does nothing but parse, guard `JsonException`, and
delegate. `Inspect` calls `TryReadRoot(root, …)` over the root it has already parsed, so the blob is parsed
exactly once per gate pass while the scriptId rule stays owned by one authority — the "one authority" design
the finding names is preserved, not traded away.

Equivalence is exact rather than argued: by the time `Inspect` reaches that call it has already returned early
for a blank blob, a non-`{`-prefixed blob, and a `JsonException`, and has already established
`root.ValueKind == Object` — precisely the three pre-checks `TryReadTagConfig` performs before its own
field read, over the same default-options parse of the same string.

Pinned by a new `tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Calculation.Tests/CalculationTagDefinitionFactoryInspectTests.cs`
(17 cases: exact warning wording, the rejection-vs-advisory split and its ordering, the blank/non-object
pre-filter, and a falsification control that every advisory-drawing blob is still ACCEPTED by
`TryReadTagConfig` — the "never stricter than the runtime read" invariant). `Inspect` previously had no
coverage outside the DB-backed AdminUI parity corpus, so these were written against the **pre-refactor**
code and verified to pass there (production change stashed) before the refactor landed — a pin, not a
description of the new code. Behaviour is byte-identical; no accept/reject/advisory outcome changed.