diff --git a/code-reviews/Driver.AbLegacy.Contracts/findings.md b/code-reviews/Driver.AbLegacy.Contracts/findings.md new file mode 100644 index 00000000..299e6506 --- /dev/null +++ b/code-reviews/Driver.AbLegacy.Contracts/findings.md @@ -0,0 +1,142 @@ +# Code Review — Driver.AbLegacy.Contracts + + + +| Field | Value | +|---|---| +| Module | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts` | +| Reviewer | Claude Code | +| Review date | 2026-06-19 | +| Commit reviewed | `a19b0f86` | +| Status | Reviewed | +| Open findings | 3 | + +## 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 | +| 2 | OtOpcUa conventions | 1 finding (Driver.AbLegacy.Contracts-003) | +| 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 | 1 finding (Driver.AbLegacy.Contracts-004) | +| 9 | Testing coverage | No issues found (contracts exercised by Driver.AbLegacy.Tests and AdminUI.Tests) | +| 10 | Documentation & comments | 2 findings (Driver.AbLegacy.Contracts-001 fixed, Driver.AbLegacy.Contracts-002 open) | + +## Findings + + + +### Driver.AbLegacy.Contracts-001 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs:48` | +| Status | Resolved | + +**Description:** The `` XML doc on `AbLegacyTagDefinition` stated +`1 reads as a scalar.` This was incorrect. Both the parser (`AbLegacyEquipmentTagParser`, +line 44: `if (rawLength >= 1)`) and the driver (`AbLegacyDriver.IsArrayTag`, checks `len >= 1`) +treat `ArrayLength: 1` as a valid one-element array, exposing a `[1]` OPC UA array node -- not a +scalar. The in-source comment at `AbLegacyEquipmentTagParser.cs:36` correctly stated +"A 1-element array (isArray:true, arrayLength:1) is a valid [1] array." The XML doc contradicted this. + +**Recommendation:** Change the offending sentence from `1 reads as a scalar.` to +`1 is a valid one-element array exposed as a [1] OPC UA array node.` + +**Resolution:** Fixed in same review session, 2026-06-19. Changed the misleading sentence in +`AbLegacyDriverOptions.cs:48` to correctly describe ArrayLength 1 as a one-element array; +verified by build (no test project in this module). + +--- + +### Driver.AbLegacy.Contracts-002 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Documentation & comments | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyEquipmentTagParser.cs:15` | +| Status | Open | + +**Description:** `AbLegacyEquipmentTagParser.TryParse` has an undocumented edge case: when +`isArray` is the JSON literal `true` but `arrayLength` is absent, zero, or negative, +`ReadInt` returns `0`, the `rawLength >= 1` guard fails, and `arrayLength` stays +`null` -- producing a scalar tag despite `isArray:true`. This is intentional (the in-source +comment at lines 33-39 references "review C-2"), but it is invisible from the public +`` and `` XML docs, and there is no unit test covering this case. +An operator who hand-edits the raw TagConfig JSON with `{"isArray":true}` and no +`arrayLength` will silently get a scalar OPC UA node instead of the expected array shape. + +**Recommendation:** Add a `` element to the XML doc noting that `isArray:true` +without a valid positive `arrayLength` silently produces a scalar (null `ArrayLength`). +Optionally add a unit test to `AbLegacyEquipmentTagTests` covering +`isArray:true, arrayLength:0/absent -> null` for regression protection. + +**Resolution:** _(empty until closed)_ + +--- + +### Driver.AbLegacy.Contracts-003 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | OtOpcUa conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:10` | +| Status | Open | + +**Description:** `AbLegacyPlcFamilyProfile.MaxTagBytes` is a record constructor parameter +populated with distinct values per family (240/232/240/240), but a global search finds zero +call sites that read this property anywhere in the codebase -- not in `AbLegacyDriver.cs`, +the probe, the CLI, or any test. The field is dead weight in the public contract surface. +Keeping it may mislead future implementors into assuming it is enforced: a caller that adds +array-size clamping logic on `MaxTagBytes` without knowing the values are approximate +(e.g. SLC 5/05 240 is the PCCC packet payload cap, not a libplctag fragment limit) could +produce off-by-one sizing. If reserved for future clamping, the intent is undocumented. + +**Recommendation:** Either (a) add XML doc noting it is reserved for future array-length +clamping and is not currently enforced, or (b) remove the field and its four initialisers +in a dedicated cleanup PR (signature change is out of scope for this review). + +**Resolution:** _(empty until closed)_ + +--- + +### Driver.AbLegacy.Contracts-004 + +| Field | Value | +|---|---| +| Severity | Low | +| Category | Code organization & conventions | +| Location | `src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyPlcFamilyProfile.cs:22` | +| Status | Open | + +**Description:** `AbLegacyPlcFamilyProfile.ForFamily` has a catch-all arm `_ => Slc500` +that silently returns the SLC 500 profile for any unrecognised `AbLegacyPlcFamily` value +(e.g. an integer cast). The XML doc on `ForFamily` does not mention the fallback. A +misconfigured device (or a future enum member added without updating the switch) will +silently receive SLC 500 libplctag attributes (`plc="slc500"`, `path="1,0"`) and a +`MaxTagBytes` of 240, which may produce libplctag session errors or wrong data only at +runtime, far from where the enum value was set. The parallel FOCAS driver throws on +unexpected values. + +**Recommendation:** Either (a) document the silent fallback in the XML doc and note it is +intentional (e.g. for forward-compatibility with configs authored before a new family was +added), or (b) replace `_ => Slc500` with +`_ => throw new ArgumentOutOfRangeException(nameof(family), family, null)` and apply it +in a cleanup PR. Semantic change -- defer. + +**Resolution:** _(empty until closed)_ diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs index 34cb4294..a1d28ace 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Contracts/AbLegacyDriverOptions.cs @@ -45,7 +45,7 @@ public sealed record AbLegacyDeviceOptions( /// Element count when the tag addresses a multi-element span of a PCCC data file (e.g. an /// N7 integer file is inherently up to 256 words); for a scalar. /// A PCCC data file holds at most (256) elements, so a -/// value above that is clamped where it is materialised/read. 1 reads as a scalar. +/// value above that is clamped where it is materialised/read. 1 is a valid one-element array exposed as a [1] OPC UA array node. /// public sealed record AbLegacyTagDefinition( string Name,