From 1f546c46ee328d14acfd76a07b8c2ee8c6887ab7 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 18 May 2026 21:50:57 -0400 Subject: [PATCH] Resolve Contracts-002 code-review finding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MxCommandReply.payload has no by-name ack case: MX_COMMAND_KIND_ACKNOWLEDGE_ ALARM_BY_NAME reuses the acknowledge_alarm reply payload. Verified the worker (MxAccessCommandExecutor.ExecuteAcknowledgeAlarmByName) and gateway (WorkerAlarmRpcDispatcher) already implement this correctly — the gap was purely undocumented contract asymmetry. Documented the reuse on the proto oneof case and the AcknowledgeAlarmReplyPayload message comment (regenerating the .NET contract), and in docs/AlarmClientDiscovery.md. Added ProtobufContractRoundTripTests.MxCommandReply_AcknowledgeAlarmByName_Reuses AcknowledgeAlarmPayloadCase to pin the contract. Co-Authored-By: Claude Opus 4.7 (1M context) --- code-reviews/Contracts/findings.md | 6 +-- docs/AlarmClientDiscovery.md | 14 ++++++ .../Generated/MxaccessGateway.cs | 27 +++++++--- .../Protos/mxaccess_gateway.proto | 25 +++++++--- .../ProtobufContractRoundTripTests.cs | 50 +++++++++++++++++++ 5 files changed, 107 insertions(+), 15 deletions(-) diff --git a/code-reviews/Contracts/findings.md b/code-reviews/Contracts/findings.md index f3e6b1b..b1890c3 100644 --- a/code-reviews/Contracts/findings.md +++ b/code-reviews/Contracts/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `6c64030` | | Status | Reviewed | -| Open findings | 8 | +| Open findings | 7 | ## Checklist coverage @@ -48,13 +48,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `src/MxGateway.Contracts/Protos/mxaccess_gateway.proto:384-385`, `:95` | -| Status | Open | +| Status | Resolved | **Description:** `MxCommandKind` includes `MX_COMMAND_KIND_ACKNOWLEDGE_ALARM_BY_NAME = 29` and `MxCommand.payload` carries `AcknowledgeAlarmByNameCommand acknowledge_alarm_by_name_command = 38`, but `MxCommandReply.payload` has only `acknowledge_alarm = 34` and `query_active_alarms = 35` — there is no by-name reply case. The by-name ack must reuse `AcknowledgeAlarmReplyPayload` or rely on the top-level `hresult`. The command/reply payload asymmetry is undocumented and easy to dispatch incorrectly. **Recommendation:** Either add an explicit comment to `MxCommandReply` stating that by-name ack reuses the `acknowledge_alarm` payload case, or add a dedicated payload case for symmetry, and document the chosen contract in `docs/Contracts.md` / `AlarmClientDiscovery.md`. -**Resolution:** _(open)_ +**Resolution:** _(2026-05-18)_ Verified against both the `.proto` and the dispatch code. The asymmetry is intentional and the code is correct: the worker's `MxAccessCommandExecutor.ExecuteAcknowledgeAlarmByName` builds `reply.AcknowledgeAlarm = new AcknowledgeAlarmReplyPayload { NativeStatus = rc }` — deliberately reusing the `acknowledge_alarm` payload case — and the gateway's `WorkerAlarmRpcDispatcher.AcknowledgeAsync` only reads the top-level `hresult`/`protocol_status`, so both ack arms work. The gap was documentation only. Took the finding's preferred option (a) — comment-only, no wire-format or generated-type change: added explicit comments to the `acknowledge_alarm` reply-payload case and to the `AcknowledgeAlarmReplyPayload` message in `mxaccess_gateway.proto` stating both ack kinds reuse this case and consumers must dispatch on `MxCommandReply.kind`, and documented the contract in `docs/AlarmClientDiscovery.md` section 4. Added regression test `ProtobufContractRoundTripTests.MxCommandReply_AcknowledgeAlarmByName_ReusesAcknowledgeAlarmPayloadCase` pinning the by-name-ack → `acknowledge_alarm` reuse and asserting no by-name-specific reply oneof case exists. ### Contracts-003 diff --git a/docs/AlarmClientDiscovery.md b/docs/AlarmClientDiscovery.md index da565bb..a74eca5 100644 --- a/docs/AlarmClientDiscovery.md +++ b/docs/AlarmClientDiscovery.md @@ -762,6 +762,20 @@ in the codebase for the forward-compat shape, but the gateway-side `AcknowledgeAlarmByName` when the public RPC supplies a recognizable `Provider!Group.Tag` reference. +**Command/reply payload reuse.** `MxCommand.payload` has a dedicated +`acknowledge_alarm_by_name_command` field, but `MxCommandReply.payload` +intentionally has **no** by-name-specific case. The by-name ack carries +no outcome detail beyond the native return code, so the worker's +`ExecuteAcknowledgeAlarmByName` sets the same `acknowledge_alarm` +(`AcknowledgeAlarmReplyPayload`) reply case used by the GUID arm, with +`native_status` = the `AlarmAckByName` return code (also echoed into the +top-level `MxCommandReply.hresult`). Reply consumers must dispatch on +`MxCommandReply.kind` (`MX_COMMAND_KIND_ACKNOWLEDGE_ALARM` vs. +`MX_COMMAND_KIND_ACKNOWLEDGE_ALARM_BY_NAME`), not on the payload oneof +case, to distinguish the two acks. `WorkerAlarmRpcDispatcher` reads only +the top-level `hresult`/`protocol_status`, so it handles both arms +without unpacking the payload. + ### 5. STA / threading — production fix needed The wnwrap COM is `ThreadingModel=Apartment`. The consumer's diff --git a/src/MxGateway.Contracts/Generated/MxaccessGateway.cs b/src/MxGateway.Contracts/Generated/MxaccessGateway.cs index ed8b018..8b64186 100644 --- a/src/MxGateway.Contracts/Generated/MxaccessGateway.cs +++ b/src/MxGateway.Contracts/Generated/MxaccessGateway.cs @@ -13388,6 +13388,17 @@ namespace MxGateway.Contracts.Proto { /// Field number for the "acknowledge_alarm" field. public const int AcknowledgeAlarmFieldNumber = 34; + /// + /// Reply payload for BOTH MX_COMMAND_KIND_ACKNOWLEDGE_ALARM (by GUID) + /// and MX_COMMAND_KIND_ACKNOWLEDGE_ALARM_BY_NAME. There is intentionally + /// no by-name-specific reply case: the by-name ack carries no outcome + /// detail beyond the native ack return code, so the worker reuses this + /// `acknowledge_alarm` payload for both command kinds (the worker's + /// MxAccessCommandExecutor sets `acknowledge_alarm` for the by-name arm + /// too). Consumers must dispatch on MxCommandReply.kind, not on the + /// payload case, to tell the two acks apart. The top-level `hresult` + /// mirrors AcknowledgeAlarmReplyPayload.native_status and is preferred. + /// [global::System.Diagnostics.DebuggerNonUserCodeAttribute] [global::System.CodeDom.Compiler.GeneratedCode("protoc", null)] public global::MxGateway.Contracts.Proto.AcknowledgeAlarmReplyPayload AcknowledgeAlarm { @@ -17339,12 +17350,16 @@ namespace MxGateway.Contracts.Proto { } /// - /// Reply payload for AcknowledgeAlarmCommand. Surfaces AVEVA's native - /// AlarmAckByGUID return code; 0 means success. The MxCommandReply's - /// hresult field carries the same value and is preferred for protocol - /// consumers — this payload exists so the gateway-side - /// WorkerAlarmRpcDispatcher can echo native_status into - /// AcknowledgeAlarmReply.hresult without unpacking the outer envelope. + /// Reply payload for AcknowledgeAlarmCommand AND + /// AcknowledgeAlarmByNameCommand — both ack command kinds reuse this + /// payload case (`MxCommandReply.acknowledge_alarm`); there is no + /// dedicated by-name reply case. Surfaces AVEVA's native ack return + /// code (AlarmAckByGUID for the GUID arm, AlarmAckByName for the + /// by-name arm); 0 means success. The MxCommandReply's hresult field + /// carries the same value and is preferred for protocol consumers — + /// this payload exists so the gateway-side WorkerAlarmRpcDispatcher + /// can echo native_status into AcknowledgeAlarmReply.hresult without + /// unpacking the outer envelope. /// [global::System.Diagnostics.DebuggerDisplayAttribute("{ToString(),nq}")] public sealed partial class AcknowledgeAlarmReplyPayload : pb::IMessage diff --git a/src/MxGateway.Contracts/Protos/mxaccess_gateway.proto b/src/MxGateway.Contracts/Protos/mxaccess_gateway.proto index fa71b6b..6d75b7d 100644 --- a/src/MxGateway.Contracts/Protos/mxaccess_gateway.proto +++ b/src/MxGateway.Contracts/Protos/mxaccess_gateway.proto @@ -381,6 +381,15 @@ message MxCommandReply { BulkSubscribeReply un_advise_item_bulk = 31; BulkSubscribeReply subscribe_bulk = 32; BulkSubscribeReply unsubscribe_bulk = 33; + // Reply payload for BOTH MX_COMMAND_KIND_ACKNOWLEDGE_ALARM (by GUID) + // and MX_COMMAND_KIND_ACKNOWLEDGE_ALARM_BY_NAME. There is intentionally + // no by-name-specific reply case: the by-name ack carries no outcome + // detail beyond the native ack return code, so the worker reuses this + // `acknowledge_alarm` payload for both command kinds (the worker's + // MxAccessCommandExecutor sets `acknowledge_alarm` for the by-name arm + // too). Consumers must dispatch on MxCommandReply.kind, not on the + // payload case, to tell the two acks apart. The top-level `hresult` + // mirrors AcknowledgeAlarmReplyPayload.native_status and is preferred. AcknowledgeAlarmReplyPayload acknowledge_alarm = 34; QueryActiveAlarmsReplyPayload query_active_alarms = 35; SessionStateReply session_state = 100; @@ -448,12 +457,16 @@ message DrainEventsReply { repeated MxEvent events = 1; } -// Reply payload for AcknowledgeAlarmCommand. Surfaces AVEVA's native -// AlarmAckByGUID return code; 0 means success. The MxCommandReply's -// hresult field carries the same value and is preferred for protocol -// consumers — this payload exists so the gateway-side -// WorkerAlarmRpcDispatcher can echo native_status into -// AcknowledgeAlarmReply.hresult without unpacking the outer envelope. +// Reply payload for AcknowledgeAlarmCommand AND +// AcknowledgeAlarmByNameCommand — both ack command kinds reuse this +// payload case (`MxCommandReply.acknowledge_alarm`); there is no +// dedicated by-name reply case. Surfaces AVEVA's native ack return +// code (AlarmAckByGUID for the GUID arm, AlarmAckByName for the +// by-name arm); 0 means success. The MxCommandReply's hresult field +// carries the same value and is preferred for protocol consumers — +// this payload exists so the gateway-side WorkerAlarmRpcDispatcher +// can echo native_status into AcknowledgeAlarmReply.hresult without +// unpacking the outer envelope. message AcknowledgeAlarmReplyPayload { int32 native_status = 1; } diff --git a/src/MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs b/src/MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs index f746766..e430326 100644 --- a/src/MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs +++ b/src/MxGateway.Tests/Contracts/ProtobufContractRoundTripTests.cs @@ -342,6 +342,56 @@ public sealed class ProtobufContractRoundTripTests Assert.True(parsed.HasHresult); } + /// + /// Pins the documented command/reply payload-reuse contract: an + /// ACKNOWLEDGE_ALARM_BY_NAME command's reply intentionally has no + /// by-name-specific payload case and instead reuses the + /// acknowledge_alarm () + /// case. A future change that adds a separate by-name reply case — or + /// drops the reuse — breaks this test. See Contracts-002 and + /// docs/AlarmClientDiscovery.md section 4. + /// + [Fact] + public void MxCommandReply_AcknowledgeAlarmByName_ReusesAcknowledgeAlarmPayloadCase() + { + // The reply oneof must NOT have a by-name-specific case. If a future + // edit adds one, this assertion fails and forces the doc/test contract + // to be revisited deliberately. + foreach (MxCommandReply.PayloadOneofCase value in + System.Enum.GetValues()) + { + Assert.NotEqual("AcknowledgeAlarmByName", value.ToString()); + } + + var original = new MxCommandReply + { + SessionId = "session-1", + CorrelationId = "gateway-correlation-7", + Kind = MxCommandKind.AcknowledgeAlarmByName, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + Hresult = 0, + // By-name ack reuses the acknowledge_alarm payload case; see the + // worker's MxAccessCommandExecutor.ExecuteAcknowledgeAlarmByName. + AcknowledgeAlarm = new AcknowledgeAlarmReplyPayload + { + NativeStatus = 0, + }, + }; + + var parsed = MxCommandReply.Parser.ParseFrom(original.ToByteArray()); + + Assert.Equal(original, parsed); + // Kind distinguishes the by-name ack; the payload case is shared. + Assert.Equal(MxCommandKind.AcknowledgeAlarmByName, parsed.Kind); + Assert.Equal(MxCommandReply.PayloadOneofCase.AcknowledgeAlarm, parsed.PayloadCase); + Assert.Equal(0, parsed.AcknowledgeAlarm.NativeStatus); + // The by-name command has its own command payload case — the asymmetry + // with the reply oneof is the documented contract under test. + Assert.Contains( + MxCommand.PayloadOneofCase.AcknowledgeAlarmByNameCommand, + System.Enum.GetValues()); + } + /// Verifies that ActiveAlarmSnapshot round-trips with current state and operator metadata. [Fact] public void ActiveAlarmSnapshot_RoundTripsAllFields()