diff --git a/code-reviews/Driver.Cli.Common/findings.md b/code-reviews/Driver.Cli.Common/findings.md index ea772f6..eed5d1d 100644 --- a/code-reviews/Driver.Cli.Common/findings.md +++ b/code-reviews/Driver.Cli.Common/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-23 | | Commit reviewed | `a9be809` | | Status | Reviewed | -| Open findings | 2 | +| Open findings | 0 | ## Checklist coverage @@ -251,7 +251,7 @@ added to the `DriverCommandBase` class-summary driver enumeration in commit `7ff | Severity | High | | Category | Correctness & logic bugs | | Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:129` | -| Status | Open | +| Status | Resolved | **Description:** Commit `5a9c459` added `0x80550000u => "BadDeviceFailure"` to the `FormatStatus` shortlist, but `0x80550000` is the canonical OPC UA spec value for @@ -302,6 +302,30 @@ original recommendation again: add a CI test that cross-checks every shortlist entry against `Opc.Ua.StatusCodes` reflection so this class of bug stops recurring. +**Resolution:** Resolved 2026-05-23 — corrected `SnapshotFormatter.FormatStatus` +to map `0x808B0000u => "BadDeviceFailure"` (was `0x80550000u`). Updated the +`InlineData` row in the well-known Theory accordingly; the redundant native- +emitted Theory was deleted entirely per Driver.Cli.Common-008. Added a regression +row to `FormatStatus_does_not_apply_pre_fix_wrong_names` pinning that +`0x80550000` no longer renders as `BadDeviceFailure` (mirroring the +Driver.Cli.Common-001 wrong-name guards). The underlying constant was also +corrected in all six native-protocol mappers as part of the same commit: +`FocasStatusMapper.BadDeviceFailure`, `AbCipStatusMapper.BadDeviceFailure`, +`AbLegacyStatusMapper.BadDeviceFailure`, `TwinCATStatusMapper.BadDeviceFailure`, +`ModbusDriver.StatusBadDeviceFailure`, `S7Driver.StatusBadDeviceFailure` — all +moved from `0x80550000u` to `0x808B0000u`. The three downstream Modbus tests +(`ModbusExceptionMapperTests` 3 InlineData rows + 1 ShouldBe assertion; +`ExceptionInjectionTests.StatusBadDeviceFailure` constant) updated to expect +the corrected code. **Behavior change:** OPC UA clients consuming the native +drivers now see the canonical `BadDeviceFailure` (0x808B0000) instead of the +misnamed `BadSecurityPolicyRejected` (0x80550000) on device-fault paths — +operator-facing CLI output and machine-readable status semantics now agree. +Suite totals after fix: Driver.Cli.Common.Tests 43 green (was 48 — minus 5 +redundant rows); Modbus.Tests 263; AbCip.Tests 262; AbLegacy.Tests 157; +FOCAS.Tests 178; S7.Tests 112; TwinCAT.Tests 131; all green. The Opc.Ua.StatusCodes +cross-check the recommendation suggested is recorded as a follow-up worth +considering but is out of scope for this fix. + ### Driver.Cli.Common-008 | Field | Value | @@ -309,7 +333,7 @@ recurring. | Severity | Low | | Category | Testing coverage | | Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs:50-64` | -| Status | Open | +| Status | Resolved | **Description:** Commit `5a9c459` adds a new `FormatStatus_names_native_driver_emitted_codes` `[Theory]` whose five @@ -332,3 +356,9 @@ it but switch to `ShouldBe($"0x{status:X8} ({expectedName})")` so its assertion strength matches the rest of the file. Option (a) is cleaner: the commit's "operator workflow" intent is documented well enough in the well-known Theory comment block; the redundant Theory is dead weight. + +**Resolution:** Resolved 2026-05-23 — took option (a): deleted the +`FormatStatus_names_native_driver_emitted_codes` Theory entirely. Its five +`InlineData` rows are covered by the well-known Theory's `ShouldBe` (strict +exact-match assertion), which is the authoritative shortlist test. Landed +alongside the Driver.Cli.Common-007 fix in the same commit. diff --git a/code-reviews/README.md b/code-reviews/README.md index 1fa8078..3727b20 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -26,7 +26,7 @@ Each module's `findings.md` is the source of truth; this file is generated from | [Driver.AbCip.Cli](Driver.AbCip.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 8 | | [Driver.AbLegacy](Driver.AbLegacy/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 13 | | [Driver.AbLegacy.Cli](Driver.AbLegacy.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 7 | -| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 2 | 8 | +| [Driver.Cli.Common](Driver.Cli.Common/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 0 | 8 | | [Driver.FOCAS](Driver.FOCAS/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 12 | | [Driver.FOCAS.Cli](Driver.FOCAS.Cli/findings.md) | Claude Code | 2026-05-22 | `76d35d1` | Reviewed | 0 | 5 | | [Driver.Galaxy](Driver.Galaxy/findings.md) | Claude Code | 2026-05-23 | `a9be809` | Reviewed | 4 | 18 | @@ -49,7 +49,6 @@ Findings with status `Open` or `In Progress`, ordered by severity. | ID | Severity | Category | Location | Description | |---|---|---|---|---| | Core.Scripting-012 | High | Security | `ForbiddenTypeAnalyzer.cs:60-76`, `ScriptSandbox.cs:96-126` | The Core.Scripting-008 rewrite broadened the BCL references list from a narrow allow-list (`System.Private.CoreLib` + `System.Linq` only) to the full `TRUSTED_PLATFORM_ASSEMBLIES` set filtered to `System.*` + `netstandard` + `Microsoft.Win… | -| Driver.Cli.Common-007 | High | Correctness & logic bugs | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/SnapshotFormatter.cs:129` | Commit `5a9c459` added `0x80550000u => "BadDeviceFailure"` to the `FormatStatus` shortlist, but `0x80550000` is the canonical OPC UA spec value for `BadSecurityPolicyRejected`, not `BadDeviceFailure`. The correct spec value for `BadDeviceF… | | Core.Scripting-013 | Medium | Security | `ScriptEvaluator.cs:202-225` (`BuildWrapperSource`) | The synthesized wrapper pastes the user's source verbatim between `{` and `}` braces inside a static method body, with a `#line 1` directive and no escaping. The legacy `CSharpScript.CreateDelegate` path was robust to this because Roslyn's… | | Core.Scripting-014 | Medium | Concurrency & thread safety | `CompiledScriptCache.cs:91-103` (`Clear`) | `Clear()` snapshots `_cache.Keys.ToArray()` then iterates, calling `TryRemove(key, out var lazy)` on each — the key-only overload, not the value-scoped one used in `GetOrCompile`'s catch block. Between the snapshot and a given `TryRemove`,… | | Core.Scripting-016 | Medium | Performance & resource management | `src/Core/ZB.MOM.WW.OtOpcUa.Core.VirtualTags/VirtualTagEngine.cs:74-117`, `src/Core/ZB.MOM.WW.OtOpcUa.Core.ScriptedAlarms/ScriptedAlarmEngine.cs:139-182` | The Core.Scripting-008 resolution introduced `ScriptEvaluator.IDisposable` + `CompiledScriptCache.Clear()` that disposes each materialised evaluator before dropping its dictionary entry, so per-publish ALC accretion is no longer process-li… | @@ -57,7 +56,6 @@ Findings with status `Open` or `In Progress`, ordered by severity. | Driver.Galaxy-016 | Medium | Performance & resource management | `ZB.MOM.WW.OtOpcUa.Driver.Galaxy.csproj:43-47`, `libs/README.md:32-37` | The five new `PackageReference` versions declared in the csproj (`Google.Protobuf` 3.34.1, `Grpc.Core.Api` 2.76.0, `Grpc.Net.Client` 2.71.0, `Microsoft.Extensions.Logging.Abstractions` 10.0.0, `Polly` 8.5.2) do not all match what the vendo… | | Core.ScriptedAlarms-013 | Low | Documentation & comments | `ScriptedAlarmEngine.cs:66-81` | The new internal test accessors `TryGetScratchReadCacheForTest` and `TryGetScratchContextForTest` (introduced by the Core.ScriptedAlarms-009 resolution at `0001cdd`) return the *live* per-alarm scratch — the same `Dictionary "BadNotWritable", 0x803C0000u => "BadOutOfRange", 0x803D0000u => "BadNotSupported", - 0x80550000u => "BadDeviceFailure", + 0x808B0000u => "BadDeviceFailure", 0x80740000u => "BadTypeMismatch", 0x40000000u => "Uncertain", _ => null, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs index f8adcaf..e21c9dc 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/AbCipStatusMapper.cs @@ -41,7 +41,7 @@ public static class AbCipStatusMapper public const uint BadNotWritable = 0x803B0000u; public const uint BadOutOfRange = 0x803C0000u; public const uint BadNotSupported = 0x803D0000u; - public const uint BadDeviceFailure = 0x80550000u; + public const uint BadDeviceFailure = 0x808B0000u; public const uint BadCommunicationError = 0x80050000u; public const uint BadTimeout = 0x800A0000u; public const uint BadTypeMismatch = 0x80730000u; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs index c6d442f..acb9124 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/AbLegacyStatusMapper.cs @@ -16,7 +16,7 @@ public static class AbLegacyStatusMapper public const uint BadNotWritable = 0x803B0000u; public const uint BadOutOfRange = 0x803C0000u; public const uint BadNotSupported = 0x803D0000u; - public const uint BadDeviceFailure = 0x80550000u; + public const uint BadDeviceFailure = 0x808B0000u; public const uint BadCommunicationError = 0x80050000u; public const uint BadTimeout = 0x800A0000u; public const uint BadTypeMismatch = 0x80730000u; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasStatusMapper.cs index 565e900..e5c3ddb 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.FOCAS/FocasStatusMapper.cs @@ -14,7 +14,7 @@ public static class FocasStatusMapper public const uint BadNotWritable = 0x803B0000u; public const uint BadOutOfRange = 0x803C0000u; public const uint BadNotSupported = 0x803D0000u; - public const uint BadDeviceFailure = 0x80550000u; + public const uint BadDeviceFailure = 0x808B0000u; public const uint BadCommunicationError = 0x80050000u; public const uint BadTimeout = 0x800A0000u; public const uint BadTypeMismatch = 0x80730000u; diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs index 088e044..dd106df 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus/ModbusDriver.cs @@ -1511,7 +1511,7 @@ public sealed class ModbusDriver private const uint StatusBadNotWritable = 0x803B0000u; private const uint StatusBadOutOfRange = 0x803C0000u; private const uint StatusBadNotSupported = 0x803D0000u; - private const uint StatusBadDeviceFailure = 0x80550000u; + private const uint StatusBadDeviceFailure = 0x808B0000u; private const uint StatusBadCommunicationError = 0x80050000u; /// diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs index 1ff4a5d..448f550 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.S7/S7Driver.cs @@ -69,7 +69,7 @@ public sealed class S7Driver(S7DriverOptions options, string driverInstanceId, I /// OPC UA StatusCode used for socket / timeout / protocol-layer faults. private const uint StatusBadCommunicationError = 0x80050000u; /// OPC UA StatusCode used for a genuine device fault (CPU error, hardware fault). - private const uint StatusBadDeviceFailure = 0x80550000u; + private const uint StatusBadDeviceFailure = 0x808B0000u; private readonly Dictionary _tagsByName = new(StringComparer.OrdinalIgnoreCase); private readonly Dictionary _parsedByName = new(StringComparer.OrdinalIgnoreCase); diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs index 22c4749..80e4e59 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.TwinCAT/TwinCATStatusMapper.cs @@ -16,7 +16,7 @@ public static class TwinCATStatusMapper public const uint BadNotWritable = 0x803B0000u; public const uint BadOutOfRange = 0x803C0000u; public const uint BadNotSupported = 0x803D0000u; - public const uint BadDeviceFailure = 0x80550000u; + public const uint BadDeviceFailure = 0x808B0000u; public const uint BadCommunicationError = 0x80050000u; public const uint BadTimeout = 0x800A0000u; public const uint BadTypeMismatch = 0x80730000u; diff --git a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs index f6d1d52..09f016f 100644 --- a/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs +++ b/tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common.Tests/SnapshotFormatterTests.cs @@ -39,7 +39,9 @@ public sealed class SnapshotFormatterTests [InlineData(0x803B0000u, "BadNotWritable")] [InlineData(0x803C0000u, "BadOutOfRange")] [InlineData(0x803D0000u, "BadNotSupported")] - [InlineData(0x80550000u, "BadDeviceFailure")] + // Driver.Cli.Common-007: corrected from 0x80550000 (which is actually + // BadSecurityPolicyRejected) to the canonical OPC UA spec value 0x808B0000. + [InlineData(0x808B0000u, "BadDeviceFailure")] [InlineData(0x80740000u, "BadTypeMismatch")] [InlineData(0x40000000u, "Uncertain")] public void FormatStatus_names_well_known_status_codes(uint status, string expectedName) @@ -47,22 +49,6 @@ public sealed class SnapshotFormatterTests SnapshotFormatter.FormatStatus(status).ShouldBe($"0x{status:X8} ({expectedName})"); } - [Theory] - // Driver.FOCAS.Cli-005 follow-up (Driver.Cli.Common): the FOCAS / AbCip / AbLegacy mappers - // emit these five codes — they previously rendered only as severity-class "Bad" because the - // shortlist did not name them, defeating the operator workflow that docs/Driver.*.Cli.md - // promise (read the BadDeviceFailure / BadNotWritable / ... name straight off probe/write - // output). Pin them named so the docs stay accurate. - [InlineData(0x80020000u, "BadInternalError")] - [InlineData(0x803B0000u, "BadNotWritable")] - [InlineData(0x803C0000u, "BadOutOfRange")] - [InlineData(0x803D0000u, "BadNotSupported")] - [InlineData(0x80550000u, "BadDeviceFailure")] - public void FormatStatus_names_native_driver_emitted_codes(uint status, string expectedName) - { - SnapshotFormatter.FormatStatus(status).ShouldContain($"({expectedName})"); - } - [Theory] // Regression for Driver.Cli.Common-001: these codes were previously mapped to the // wrong names. The hex values below are what the buggy shortlist used; they must @@ -71,6 +57,10 @@ public sealed class SnapshotFormatterTests [InlineData(0x80070000u, "BadNoCommunication")] // was mislabelled BadNoCommunication [InlineData(0x80080000u, "BadWaitingForInitialData")] // was mislabelled BadWaitingForInitialData [InlineData(0x80350000u, "BadNodeIdInvalid")] // was mislabelled BadNodeIdInvalid + // Driver.Cli.Common-007: 0x80550000 is BadSecurityPolicyRejected per spec, not + // BadDeviceFailure (which is 0x808B0000). The buggy shortlist + the six native- + // protocol mappers all had it wrong; this row pins it as a regression guard. + [InlineData(0x80550000u, "BadDeviceFailure")] public void FormatStatus_does_not_apply_pre_fix_wrong_names(uint status, string wrongName) { SnapshotFormatter.FormatStatus(status).ShouldNotContain(wrongName); diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/ExceptionInjectionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/ExceptionInjectionTests.cs index ecef883..7cd38d2 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/ExceptionInjectionTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.IntegrationTests/ExceptionInjectionTests.cs @@ -27,7 +27,7 @@ public sealed class ExceptionInjectionTests(ModbusSimulatorFixture sim) private const uint StatusGood = 0u; private const uint StatusBadOutOfRange = 0x803C0000u; private const uint StatusBadNotSupported = 0x803D0000u; - private const uint StatusBadDeviceFailure = 0x80550000u; + private const uint StatusBadDeviceFailure = 0x808B0000u; private const uint StatusBadCommunicationError = 0x80050000u; private void SkipUnlessInjectorLive() diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusExceptionMapperTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusExceptionMapperTests.cs index c329400..9862fb2 100644 --- a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusExceptionMapperTests.cs +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.Modbus.Tests/ModbusExceptionMapperTests.cs @@ -18,9 +18,9 @@ public sealed class ModbusExceptionMapperTests [InlineData((byte)0x01, 0x803D0000u)] // Illegal Function → BadNotSupported [InlineData((byte)0x02, 0x803C0000u)] // Illegal Data Address → BadOutOfRange [InlineData((byte)0x03, 0x803C0000u)] // Illegal Data Value → BadOutOfRange - [InlineData((byte)0x04, 0x80550000u)] // Server Failure → BadDeviceFailure - [InlineData((byte)0x05, 0x80550000u)] // Acknowledge (long op) → BadDeviceFailure - [InlineData((byte)0x06, 0x80550000u)] // Server Busy → BadDeviceFailure + [InlineData((byte)0x04, 0x808B0000u)] // Server Failure → BadDeviceFailure (Driver.Cli.Common-007) + [InlineData((byte)0x05, 0x808B0000u)] // Acknowledge (long op) → BadDeviceFailure + [InlineData((byte)0x06, 0x808B0000u)] // Server Busy → BadDeviceFailure [InlineData((byte)0x0A, 0x80050000u)] // Gateway path unavailable → BadCommunicationError [InlineData((byte)0x0B, 0x80050000u)] // Gateway target failed to respond → BadCommunicationError [InlineData((byte)0xFF, 0x80020000u)] // Unknown code → BadInternalError fallback @@ -61,7 +61,7 @@ public sealed class ModbusExceptionMapperTests [new WriteRequest("T", (short)42)], TestContext.Current.CancellationToken); - writes[0].StatusCode.ShouldBe(0x80550000u, "FC06 returning exception 04 (CPU in PROGRAM mode) maps to BadDeviceFailure"); + writes[0].StatusCode.ShouldBe(0x808B0000u, "FC06 returning exception 04 (CPU in PROGRAM mode) maps to BadDeviceFailure"); } private sealed class NonModbusFailureTransport : IModbusTransport