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

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 records, 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<char>. 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.