Joseph Doherty
ab57e53b92
- Driver.AbCip.Contracts-001: parse 'writable' from TagConfig JSON (default true) instead of hardcoding - Driver.AbCip.Contracts-002/-003: Dt type comment; drop dead [Display]/[Range] annotations - Driver.AbCip.Contracts-004: dedicated AbCipEquipmentTagParser test class (+15) - Driver.AbCip-017: document Tick severity Low-fallback on Bad severity read - Driver.AbLegacy.Contracts-002/-003/-004: isArray-scalar remarks (+tests), MaxTagBytes/ForFamily docs - Driver.Galaxy.Browser-003 + Driver.Galaxy.Contracts-003: extract ResolveApiKey -> GalaxySecretRef (dedup) - Driver.Galaxy-019: cache buffered-interval only on Ok + ILogger warnings + ClassifyIntervalReply (+tests) - Driver.FOCAS.Contracts-002: thread WriteIdempotent through DiscoverAsync (+test)
13 KiB
Code Review — Driver.AbCip.Contracts
| Field | Value |
|---|---|
| Module | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts |
| Reviewer | Claude Code |
| Review date | 2026-06-19 |
| Commit reviewed | a19b0f86 |
| Status | Reviewed |
| 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 | Driver.AbCip.Contracts-001, Driver.AbCip.Contracts-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | No issues found |
| 4 | Error handling & resilience | No issues found |
| 5 | Security | No issues found |
| 6 | Performance & resource management | No issues found |
| 7 | Design-document adherence | No issues found |
| 8 | Code organization & conventions | Driver.AbCip.Contracts-003 |
| 9 | Testing coverage | Driver.AbCip.Contracts-004 |
| 10 | Documentation & comments | Driver.AbCip.Contracts-005, Driver.AbCip.Contracts-006 |
Findings
Driver.AbCip.Contracts-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs:42 |
| Status | Resolved |
Description: AbCipEquipmentTagParser.TryParse hard-codes Writable: true on every
equipment-tag definition it produces, regardless of any writable field in the TagConfig JSON.
The consequence is that an operator who intends a read-only equipment tag — relying on the PLC's
ExternalAccess attribute to block writes — will still receive a writable OPC UA node in the
address space: the server advertises the node as writable, and the OnWriteValue path in the
driver dispatches writes to the PLC (which the PLC may then reject). The node is never declared
BadNotWritable proactively. This is inconsistent with the pre-declared tag path, where
AbCipTagDefinition.Writable is explicitly authored per tag and controls the OPC UA
AccessLevel bit at materialization.
Additionally, TryParse does not guard against AbCipDataType.Structure in the "dataType"
JSON field. An equipment tag authored with "dataType":"Structure" succeeds and returns a
Structure-typed definition with Members: null. The driver treats this as a black-box
Structure (readable via dotted-path child addressing), but the address space emits a placeholder
String variable for it rather than a UDT folder. No error or warning is surfaced. The AdminUI
tag editor does not expose UDT member declarations, so a Structure equipment tag is essentially
a misconfiguration.
Recommendation: (1) Read a "writable" boolean field from the TagConfig JSON (default true
when absent) and thread it into AbCipTagDefinition.Writable. This matches the contract on the
record's Writable parameter. (2) Either return false from TryParse when dataType parses
to AbCipDataType.Structure (equipment-tag flow cannot declare members), or add an explicit
comment documenting the black-box dotted-path behaviour so the next reader understands the intent.
Resolution: Resolved 2026-06-20 — (1) TryParse now reads the optional "writable" boolean
field from the TagConfig JSON and threads it into AbCipTagDefinition.Writable, defaulting to
true when the field is absent. The <remarks> on TryParse was updated to document this
behaviour. (2) For the Structure concern, zero-behaviour-change option taken: an inline comment
was added at the dataType parse site in TryParse documenting that a Structure dataType on an
equipment tag is treated as a black-box dotted-path read (libplctag resolves the full path; the
equipment-tag flow does not enumerate UDT members). New tests in AbCipEquipmentTagParserTests
cover writable:false, writable absent, and the Structure path. Suite green (322 tests).
Driver.AbCip.Contracts-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDataType.cs:28 |
| Status | Resolved |
Description: The inline comment on AbCipDataType.Dt reads:
Dt, // Date/Time — Logix DT == DINT representing seconds-since-epoch per Rockwell conventions
This description is inaccurate in two ways. First, CipSymbolObjectDecoder (in the driver
module) maps both CIP type code 0xCD (Logix DATE — a 4-byte unsigned integer representing
days since 1 January 1984) and 0xCF (Logix DATE_AND_TIME / DT — an 8-byte unsigned
integer representing microseconds since 1 January 1970) to AbCipDataType.Dt. The comment
fits only the DT / DATE_AND_TIME form, not DATE. Second, even for DT proper, the unit
is microseconds, not seconds.
In practice the driver reads Dt via GetInt32(offset) with a 4-byte stride: this is correct
for DATE but truncates DATE_AND_TIME to its low 4 bytes, losing the upper 32 bits. That
underlying decode issue lives in LibplctagTagRuntime (the Driver.AbCip module), but the
inaccurate comment here perpetuates the misconception for anyone authoring or reading configs
that reference the Dt member.
Recommendation: Update the inline comment to describe both mapped CIP types and the 4-byte stride constraint, e.g.:
Dt, // Logix DATE (0xCD — 4-byte unsigned days since 1984-01-01) or DATE_AND_TIME / DT
// (0xCF — 8-byte unsigned microseconds since 1970-01-01). The driver reads 4 bytes
// via GetInt32; DATE decodes correctly, DATE_AND_TIME is truncated to the low 4 bytes
// (a known limitation tracked in LibplctagTagRuntime).
No behaviour change; documentation only.
Resolution: Resolved 2026-06-20 — replaced the inaccurate single-line comment on Dt with a
3-line comment describing both mapped CIP types (0xCD DATE / 0xCF DT), the 4-byte read stride,
and the truncation note for DATE_AND_TIME. Build green (0 errors, 0 warnings).
Driver.AbCip.Contracts-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs:84-85 |
| Status | Resolved |
Description: AbCipDriverOptions.ProbeTimeoutSeconds carries [Display] and [Range(1, 60)]
attributes from System.ComponentModel.DataAnnotations. No other driver contracts project
(Modbus, S7, AbLegacy, Galaxy, TwinCAT, FOCAS, OpcUaClient) annotates ProbeTimeoutSeconds
with data-annotation attributes. More importantly, AbCipDriverOptions is never bound directly
by an ASP.NET model binder: the AdminUI driver page (AbCipDriverPage.razor) deserializes it
via JsonSerializer.Deserialize<AbCipDriverOptions> and then transfers values into a separate
FormModel. Neither the [Display] grouping hint nor the [Range] validation is evaluated at
runtime — they are dead metadata. The using System.ComponentModel.DataAnnotations; directive
exists in this project solely to support these two unused attributes.
Recommendation: Remove [Display] and [Range(1, 60)] from ProbeTimeoutSeconds and
remove the using System.ComponentModel.DataAnnotations; directive. This brings the project in
line with every other driver contracts project and removes a superfluous framework dependency.
If the intent is to document the valid range, an <remarks> tag (e.g. "Valid range: 1–60
seconds; the AdminUI clamps to 60s server-side.") achieves the same goal without the attribute
dependency.
Resolution: Resolved 2026-06-20 — removed [Display] and [Range(1, 60)] from
ProbeTimeoutSeconds and removed the using System.ComponentModel.DataAnnotations; directive
(confirmed it was used only by those two attributes). Added a <remarks> element reading
"Valid range: 1–60 seconds; the AdminUI clamps to 60s server-side." to preserve the intent
in documentation. Build green (0 errors, 0 warnings).
Driver.AbCip.Contracts-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs (entire file) |
| Status | Resolved |
Description: The contracts module has no dedicated test project, and AbCipEquipmentTagParser.TryParse
is the module's only non-trivial logic. Its ReadArrayShape helper has four distinct outcome
branches; the method also has multiple early-return paths and one JSON exception path. None of
these are covered by targeted unit tests. Coverage gaps include:
isArray: true+arrayLength: 0— canonical rule says this is a scalar; not tested.isArray: true+arrayLengthabsent — same canonical rule; not tested."dataType": "Structure"input — no test (see Driver.AbCip.Contracts-001).- The
Writable: truehardcode is not asserted anywhere. - A
tagPaththat is present as a JSON string but contains only whitespace — returnsfalse; not tested. - Malformed JSON input (the
JsonExceptioncatch) — not tested.
Recommendation: Add a dedicated test class for AbCipEquipmentTagParser in the existing
driver test project (tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/). No new project
is needed. Cover: valid scalar round-trip, 1-element array, N-element array, each degenerate
array-shape combination, non-JSON and non-object input, missing/blank tagPath, the Structure
DataType path, and the Writable default.
Resolution: Resolved 2026-06-20 — added AbCipEquipmentTagParserTests.cs to
tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/ (15 tests). Covers: valid scalar
round-trip; 1-element array (isArray:true, arrayLength:1); N-element array; isArray:true + arrayLength:0 → scalar; isArray:true + arrayLength absent → scalar; non-JSON input → false;
non-object JSON array → false; non-object JSON string → false; missing tagPath → false; blank
tagPath → false; tagPath as number → false; writable:false honoured; writable absent
defaults to true; writable:true explicit; dataType:"Structure" accepted with Members:null
(documents current black-box behaviour). Suite green (322 tests).
Driver.AbCip.Contracts-005
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipDriverOptions.cs:141-145 |
| Status | Resolved |
Description: The <param name="IsArray"> doc for AbCipTagDefinition did not state that
the field is "Ignored for AbCipDataType.Structure", while the immediately preceding
<param name="ElementCount"> (line 140) explicitly carries that note. The asymmetry left readers
unable to determine from the XML doc whether IsArray = true on a Structure-typed definition
is meaningful or silently ignored.
Recommendation: Add "Ignored for <see cref="AbCipDataType.Structure"/>." to the
IsArray param doc, matching ElementCount.
Resolution: Resolved 2026-06-19 — added "Ignored for <see cref="AbCipDataType.Structure"/>."
to the IsArray <param> doc on AbCipTagDefinition. Build verified green (0 errors, 0 warnings).
Driver.AbCip.Contracts-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Contracts/AbCipEquipmentTagParser.cs:11-14 |
| Status | Resolved |
Description: The TryParse method summary and parameter docs were silent about the fact
that Writable is always set to true in the produced AbCipTagDefinition. This is a
non-obvious behavioral contract: callers who inspect the returned definition expecting Writable
to reflect an authored intent from the JSON will find it unconditionally enabled. The omission
made the hardcode invisible to readers of the public-facing doc comment.
Recommendation: Add a <remarks> element to TryParse stating that Writable is always
true in the produced definition and that the PLC's ExternalAccess attribute is the effective
write gate.
Resolution: Resolved 2026-06-19 — added a <remarks> element to TryParse documenting
that Writable is always true and that PLC ExternalAccess is the effective write gate.
Build verified green (0 errors, 0 warnings).