# Code Review — Driver.MTConnect.Contracts | Field | Value | |---|---| | Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts` | | Reviewer | Claude (per-module review) | | Review date | 2026-08-10 | | Commit reviewed | `b990adae` | | Status | Complete | | Open findings | 0 | ## 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 | No issues found. `TryFromTagConfig` cannot throw for any input: the only reachable failure of `JsonSerializer.Deserialize` over this all-scalar DTO is `JsonException`, which is caught, and the null/blank input is guarded before it. `SeparatorInsensitiveComparer.Equals` and `GetHashCode` hash and compare exactly the same characters (the classic bucket-mismatch bug is explicitly avoided and commented). `Infer`'s `(uint)sampleCount.Value` cannot overflow — `sampleCount` is an `int` the probe parser already constrained, and the cast is guarded by `> 0`. The `DATA_SET`/`TABLE` demotion is correctly checked **before** the category, so a `DATA_SET` `SAMPLE` cannot escape as `Float64`. | | 2 | OtOpcUa conventions | No issues found. Zero-dependency leaf over `Core.Abstractions` only, mirroring `Driver.Modbus.Contracts` / `Driver.AbCip.Contracts`; net10.0, nullable enabled, `TreatWarningsAsErrors`. No `JsonStringEnumConverter` on `JsonOptions` — the type-carrying field stays `string?` and is parsed by NAME, with a digits-only pre-check that rejects the numerically-serialized-enum shape before `Enum.TryParse` can accept `"8"` as an ordinal. The absent `GenerateDocumentationFile` matches every other `*.Contracts` project in the fleet (checked against Modbus/AbCip/S7/Mqtt), so it is convention rather than an omission. | | 3 | Concurrency & thread safety | No issues found. Every type here is immutable or stateless: two `sealed record`s, a `readonly record struct`, a `FrozenSet` built once in a static initialiser, and a stateless singleton comparer. No mutable static state, no I/O, no locks needed. | | 4 | Error handling & resilience | No issues found. `TryFromTagConfig` is total (never throws, returns a verdict), `Inspect` is total, and `IsAcceptedDataTypeName` treats blank as accepted so an unauthored type falls through to the Agent's own declaration rather than rejecting a tag. A present-but-invalid type rejects rather than silently defaulting — the sibling drivers' strict-enum posture, and the fix for Gitea #549. | | 5 | Security | No issues found. No I/O, no credentials, no logging, no reflection over untrusted input; the JSON reader is bounded by a fixed DTO shape. `Inspect`'s messages echo the operator's own authored value and the valid enum names, which is the intended operator-facing detail and carries nothing secret. | | 6 | Performance & resource management | No issues found. `MTConnectInferredType` is a `readonly record struct` so the discovery hot path allocates nothing; `NumericEventTypes` is a `FrozenSet` consulted through a comparer rather than by normalising the input, so the lookup needs no temporary string; `Infer` trims via `ReadOnlySpan`. Nothing here is disposable. | | 7 | Design-document adherence | 1 finding — `Driver.MTConnect.Contracts-001`. The accept-gate-first contract from the #549–#559 arc is correctly implemented: `Inspect` asks `TryFromTagConfig` for the verdict and only explains a rejection, so it can never be stricter than the runtime; `IsAcceptedDataTypeName` is the single authority both the runtime and `MTConnectTagConfigModel.Validate` consult; and `FirstNonBlank(DriverDataType, DataType)` is deliberate, mirrored by the AdminUI model's load order and its normalise-onto-`dataType`-and-drop-the-alias save (#550). The `reference.TrimStart().FirstOrDefault() != '{'` early return in `Inspect` is likewise fleet convention — all ten driver factories carry the identical guard — so it was reviewed and not raised. | | 8 | Code organization & conventions | No issues found. The parse lives here as one authority rather than being mirrored into the AdminUI, which is exactly the drift the arc's design argues against; the DTO is private to the factory; the Options pattern is honoured and every collection defaults to empty rather than null. | | 9 | Testing coverage | No issues found. The project has no test project of its own (fleet convention for `*.Contracts`); it is covered from `Driver.MTConnect.Tests` (`MTConnectDataTypeInferenceTests` is a golden table including a probe-vs-streams spelling-parity case; `MTConnectRawTagBindingTests` and `MTConnectDriverOptionsTests` drive the factory) and from `AdminUI.Tests`, whose TagConfig parity corpus consumes `IsAcceptedDataTypeName` / `Inspect` through `MTConnectTagConfigModel`. Both suites were run after this review's changes: 538 and 1466 pass. | | 10 | Documentation & comments | 1 finding — `Driver.MTConnect.Contracts-001`. The XML documentation is otherwise excellent and records the *reason* for each rule — why the digits-only pre-check exists, why the alias out-ranks the canonical key, why the comparer is separator-insensitive (the probe document's `PART_COUNT` vs the streams document's `PartCount`), and why blank is accepted. | ## Findings ### Driver.MTConnect.Contracts-001 | Field | Value | |---|---| | Severity | Low | | Category | Documentation & comments | | Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDataTypeInference.cs:31` · `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.MTConnect.Contracts/MTConnectDriverOptions.cs:86` | | Status | Resolved | **Description:** Both public types documented a call graph that no longer exists, and in one case the false premise concealed a live defect rather than merely misinforming a reader. 1. `MTConnectDataTypeInference`'s class remarks justify living in `.Contracts` because "three call sites must agree byte-for-byte": `ITagDiscovery.DiscoverAsync`, "the universal browser's commit path (writes the browsed leaf into `TagConfig`)", and "the AdminUI typed tag editor (shows the inferred type and offers an override)". At HEAD a `grep` for `MTConnectDataTypeInference` outside this project returns only the driver: `RawBrowseCommitMapper` never calls it, and `MTConnectTagConfigModel` explicitly documents that it does **not** recompute the inference. Two of the three named callers do not exist. 2. `MTConnectDriverOptions.RawTags` describes the coercion-type precedence and states that the `/probe`-inference step "is what makes a browse-committed tag work: `RawBrowseCommitMapper` writes only the address into the blob, so without it every value from the address picker would publish as text under a numerically-typed OPC UA node." That premise is false at HEAD — the mapper writes a full `MTConnectTagConfigModel` blob whose `dataType` defaults to `String` — and the sentence therefore asserts as *prevented* precisely the outcome that now happens on every browse commit. The second is what makes this worth recording rather than a tidy-up: a reader auditing the browse path would have taken that remark as evidence the case was handled. The underlying defect is recorded as `Driver.MTConnect-003` (Resolved `64be41d8`, 2026-08-10, in `RawBrowseCommitMapper`). **Recommendation:** Correct both remarks to describe the callers that exist, and — rather than deleting the precedence rule, which is still correct for hand-authored, CSV-imported and API-authored blobs — state explicitly that the step is not reached by a browse-committed tag today, with a pointer to the finding that tracks the cause. **Resolution:** Fixed `6c63bf8f`, 2026-08-10 — `MTConnectDataTypeInference` now names its two real callers (`DiscoverAsync` and `MTConnectDriver.InferDefinition`) and carries a warning that the two it used to name do not call it; `MTConnectDriverOptions.RawTags` carries a warning that the `/probe` step is unreachable for a browse-committed tag at HEAD. Both point at `Driver.MTConnect-003`. Comment-only: `Driver.MTConnect.Tests` (538) and `AdminUI.Tests` (1466) both re-run green.