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.