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

# Code Review — Driver.AbCip.Contracts

<!-- Template for a per-module findings file. Copy to code-reviews/<Module>/findings.md.
     See ../../REVIEW-PROCESS.md for the full process. The base README.md is generated
     from the per-module `findings.md` files by regen-readme.py — do not edit README.md by hand. -->

| 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

<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
     never reused. Findings are never deleted — close them by changing Status and
     completing Resolution. -->

### 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.:

```csharp
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` + `arrayLength` absent — same canonical rule; not tested.
- `"dataType": "Structure"` input — no test (see Driver.AbCip.Contracts-001).
- The `Writable: true` hardcode is not asserted anywhere.
- A `tagPath` that is present as a JSON string but contains only whitespace — returns `false`; not tested.
- Malformed JSON input (the `JsonException` catch) — 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).