diff --git a/code-reviews/Driver.Galaxy/findings.md b/code-reviews/Driver.Galaxy/findings.md index c9bf73e..6598492 100644 --- a/code-reviews/Driver.Galaxy/findings.md +++ b/code-reviews/Driver.Galaxy/findings.md @@ -63,13 +63,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `Runtime/StatusCodeMap.cs:86` | -| Status | Open | +| Status | Resolved | **Description:** `FromMxStatus` returns `Good` whenever `status.Success != 0`. The intent (per the surrounding comment "Honors the success flag") is that a non-zero `Success` means success. But if `MxStatusProxy.Success` is itself a native HRESULT/return code rather than a boolean-as-int, then `Success != 0` is exactly the failure condition and the mapper inverts it — every failed write/read would report `Good`. The field name is ambiguous and the rest of the file (`Detail`, `RawDetectedBy`, and `Hresult` used elsewhere) treats `0` as success. `GatewayGalaxyAlarmAcknowledger.cs:62` uses the opposite convention for the sibling field (`reply.Hresult != 0` means failure). **Recommendation:** Verify the semantics of `MxStatusProxy.Success` against the gateway proto contract. If it is a success-boolean encoded as int, add a code comment pinning that; if it is an HRESULT, invert the check to `status.Success == 0 => Good`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — replaced `status.Success != 0` with `status.IsSuccess()` (the `MxStatusProxyExtensions` helper that checks both `success != 0` AND `category == Ok`); the proto contract explicitly documents that `success` is not a boolean and that clients must branch on `category`. Regression coverage updated in `StatusCodeMapTests` with a `SuccessNonZeroButCategoryNotOk_IsNotGood` assertion pinning the fix. ### Driver.Galaxy-004 diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs index 7e05714..0b4058b 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy/Runtime/StatusCodeMap.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.Logging; +using MxGateway.Client; using MxGateway.Contracts.Proto; namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; @@ -75,18 +76,20 @@ internal static class StatusCodeMap }; /// - /// Map a gateway-reported to OPC UA StatusCode. Honors - /// the success flag, then the detail byte (treated as a quality substatus), with a - /// transport-error fallback for status rows whose detected_by indicates the failure - /// happened before the MXAccess call ran. + /// Map a gateway-reported to OPC UA StatusCode. Uses + /// (category == OK AND success != 0) + /// as the canonical success test — the proto contract explicitly documents that + /// success is NOT a boolean and must not be checked in isolation; category is + /// the authoritative indicator. On failure, the detail byte (OPC DA quality substatus) + /// drives the specific code, with a transport-error fallback for pre-MXAccess failures. /// public static uint FromMxStatus(MxStatusProxy? status, ILogger? logger = null) { if (status is null) return Good; - if (status.Success != 0) return Good; + if (status.IsSuccess()) return Good; // Detail field carries the substatus when the worker translated MX-style codes; - // when zero, infer from category + detected_by. + // when zero, infer from detected_by. var detail = (byte)(status.Detail & 0xFF); if (detail != 0) return FromQualityByte(detail, logger); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs index 5b25004..e446273 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests/Runtime/StatusCodeMapTests.cs @@ -2,6 +2,7 @@ using MxGateway.Contracts.Proto; using Shouldly; using Xunit; using ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Runtime; +// MxStatusCategory needed for Driver.Galaxy-003 regression tests. namespace ZB.MOM.WW.OtOpcUa.Driver.Galaxy.Tests.Runtime; @@ -46,6 +47,12 @@ public sealed class StatusCodeMapTests else mapped.ShouldBe(StatusCodeMap.Bad); } + // ===== Driver.Galaxy-003 regression: FromMxStatus uses IsSuccess() (category + success) ===== + // The proto doc: "clients should branch on category, not on a specific success value." + // IsSuccess() requires BOTH success != 0 AND category == Ok — checking success alone + // would invert the mapping when the worker sets success=1 for an error code (the numeric + // value is NOT a boolean). + [Fact] public void FromMxStatus_NullStatus_IsGood() { @@ -53,16 +60,26 @@ public sealed class StatusCodeMapTests } [Fact] - public void FromMxStatus_SuccessNonZero_IsGood() + public void FromMxStatus_SuccessNonZeroAndCategoryOk_IsGood() { - var s = new MxStatusProxy { Success = 1 }; + // Both conditions required — this is the canonical OK path. + var s = new MxStatusProxy { Success = 1, Category = MxStatusCategory.Ok }; StatusCodeMap.FromMxStatus(s).ShouldBe(StatusCodeMap.Good); } [Fact] - public void FromMxStatus_SuccessZero_DetailKnown_MapsToSpecificCode() + public void FromMxStatus_SuccessNonZeroButCategoryNotOk_IsNotGood() { - var s = new MxStatusProxy { Success = 0, Detail = 8 /* Bad_NotConnected */ }; + // Bug fixed by Driver.Galaxy-003: success != 0 alone used to return Good even when + // category indicates an error. The proto says success is NOT a boolean — category wins. + var s = new MxStatusProxy { Success = 1, Category = MxStatusCategory.CommunicationError }; + StatusCodeMap.FromMxStatus(s).ShouldNotBe(StatusCodeMap.Good); + } + + [Fact] + public void FromMxStatus_SuccessZeroAndCategoryNotOk_DetailKnown_MapsToSpecificCode() + { + var s = new MxStatusProxy { Success = 0, Category = MxStatusCategory.OperationalError, Detail = 8 /* Bad_NotConnected */ }; StatusCodeMap.FromMxStatus(s).ShouldBe(StatusCodeMap.BadNotConnected); }