diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 808f059..292202f 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -112,8 +112,8 @@ Full design + implementation for each row lives in the linked domain doc under i |---|---|:-:|:-:|---|---|---| | CLI-35 | Medium | P0 | S | GWC-25 (coord) | Done | Python CLI `stream-events` crashes on a ReplayGap | | CLI-36 | Medium | P0 | S | GWC-25 (coord) | Done | Go CLI `stream-events` silently destroys the ReplayGap signal | -| CLI-37 | Medium | P1 | M | CLI-38 (co-land) | Not started | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | -| CLI-38 | Medium | P1 | S | — | Not started | Align .NET/Go/Java on `hresult < 0` — lands old CLI-08, cures design-doc drift | +| CLI-37 | Medium | P1 | M | CLI-38 (co-land) | Done | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | +| CLI-38 | Medium | P1 | S | — | Done | Align .NET/Go/Java on `hresult < 0` — lands old CLI-08, cures design-doc drift | | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 (land last) | Not started | Bump client versions off published 0.1.2 (converge on 0.2.0); registry-collision guard in pack-clients.ps1 | | CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET | | CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | @@ -149,7 +149,7 @@ Sequence these together rather than piecemeal — several are one change set spa ## Old-tracker actions ([`../../remediation/00-tracking.md`](../../remediation/00-tracking.md)) - Close **CLI-24** and **CLI-34** as `Done` (incidentally fixed; evidence in [../50-clients.md](../50-clients.md)). -- When CLI-38 lands, close old **CLI-08** with a pointer here. +- ~~When CLI-38 lands, close old **CLI-08** with a pointer here.~~ Done 2026-08-07: CLI-38 landed and old CLI-08 is now `Done` in the first-cycle tracker, pointing at [CLI-38](50-clients.md#cli-38--align-netgojava-on-hresult--0-lands-prior-cli-08-cures-the-doc-drift---medium--p1). - When WRK-26 lands, its doc section also discharges the WorkerFrameProtocol gap; when TST-25 lands, revisit old **TST-05** (scheduled live smoke) and **TST-24** (client wire tests), which it unlocks. ## Change log @@ -166,3 +166,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **ReplayGap end-to-end cluster (GWC-25 + CLI-35 + CLI-36) → `Done`** on `fix/gwc-25-replaygap-trio`. GWC-25: `SessionEventDistributor.RegisterWithReplay`'s empty-ring branch now reports `oldestAvailableSequence = _highestSequenceSeen + 1` when `gap == true` (still `0` when no gap), so the universal `oldest - 1` resume formula no longer wraps to `ulong.MaxValue` and dead-stream the subscriber; `docs/Sessions.md` documents the empty-ring value. CLI-35: the Python CLI renders a `ReplayGap` as a `{"replayGap": {...}}` row via a new `_event_row` helper instead of crashing in `MessageToDict`. CLI-36: the Go CLI branches on `result.IsReplayGap()` and prints the typed `REPLAY_GAP requested_after= oldest_available=` line / `replayGap` JSON row instead of formatting the library's cleared `Event`. `docs/CrossLanguageSmokeMatrix.md` gained a per-CLI gap-rendering table (one edit covering both client findings). Four new tests as designed (3 × `SessionEventDistributorTests`, `GatewayEndToEndReconnectReplayTests.ReconnectAfterFullAgeEvictionResumesWithSentinelFormula`) plus `test_stream_events_renders_replay_gap` (Python) and `TestRunStreamEventsPrintsReplayGap` (Go); all written red first and each reproducing its defect verbatim. **Deferred:** GWC-25's `ReplayGap.oldest_available_sequence` proto-comment amendment is **not** in this change — it is comment-only but triggers the full five-client regen fan-out, so it lands with the later codegen wave (alongside IPC-23's proto-comment edits) rather than forcing a regen for one sentence. Note for that wave: the fake-worker gateway e2e suite cannot run on the macOS worktree without `TMPDIR` shortened (macOS caps the Unix-domain-socket path backing .NET named pipes at 104 chars; `TMPDIR=/tmp dotnet test …` works and was used here). | | 2026-08-07 | **GWC-27 → `Done`, GWC-26 → `Done`** (branch `fix/gwc-26-27-alarm-attach`). GWC-27: `GatewaySession.AttachInternalEventSubscriber` now mirrors `AttachEventSubscriber`'s readiness gate under `_syncRoot`, before `EnsureDistributorCreated`, so a premature attach can no longer latch a poisoned distributor. GWC-26: the alarm monitor takes its internal lease directly from the session **before** `SubscribeAlarms` and drains it after the first reconcile; `ISessionManager.ReadAlarmEventsAsync` removed (zero remaining callers); `ApplyReconcile` now broadcasts an `Acknowledge` feed transition for a both-present alarm whose state advanced to `ActiveAcked` (feed-level repair on `AlarmFeedMessage`, not `MxEvent` synthesis). New tests `GatewaySessionTests.AttachInternalEventSubscriberBeforeReadyThrowsAndDoesNotPoisonDistributor` and `GatewayAlarmMonitorAttachOrderTests` (`TransitionsDuringSubscribeWindow_StillReachTheAlarmFeed`, `ApplyReconcileBroadcastsAcknowledgeDelta`); the alarm-monitor fakes now hand the monitor a real Ready `GatewaySession` with a dashboard mirror so the window is actually reproducible. Verification: NonWindows build 0 warnings/0 errors; `GatewayAlarmMonitor` 16 passed, `SessionManagerTests` 38 passed, `GatewaySessionTests` 19 passed, `AlarmFailoverEndToEndTests` 2 passed. | | 2026-08-07 | Code review of `fix/gwc-26-27-alarm-attach` surfaced a **known pre-existing characteristic, now documented**: the alarm monitor's reconcile-derived feed repairs are **at-least-once, not exactly-once**. A reconcile reads the worker's current state while the matching live transition may still be buffered in the monitor's internal lease, so both broadcast and the duplicates are indistinguishable on the alarm feed (`StreamAlarms` + dashboard alarm hub). This pre-dates GWC-26 — the Raise/Clear presence repair has always had it, since nothing serializes a reconcile pass against the in-flight live stream — so closing it (reconcile/live serialization or transition-timestamp dedup) was ruled out of scope for a P2 fix. Documented instead in `GatewayAlarmMonitor.ApplyReconcile`, `gateway.md`, and `docs/Sessions.md`, with the consumer-side contract stated explicitly (apply transitions idempotently — "set this alarm to this state", never increment/toggle). **Candidate finding for the next review cycle.** | +| 2026-08-07 | **CLI-37 + CLI-38 -> `Done`** (branch `fix/cli-37-38-conformance`), one cross-client conformance commit; **closes old-tracker CLI-08**. Canonical rules landed everywhere: an `MxStatusProxy` entry fails iff `category != MX_STATUS_CATEGORY_OK` (`success` is the raw COM member, diagnostics only; absent entry = success, present entry with `UNSPECIFIED` = failure), and a reply fails on HRESULT iff `hresult` is present and `< 0` (so `S_FALSE = 1` passes). Edits: .NET `MxStatusProxyExtensions.IsSuccess` (drop the `Success != 0` conjunct) + `MxCommandReplyExtensions` (`!= 0` -> `< 0`); Go `StatusSucceeded` (category) + `errors.go` (`< 0`); Java `MxStatuses.succeeded` (category, Javadoc corrected) + `MxGatewayErrors` (`< 0`); Python `errors.py` (category); Rust `ensure_mxaccess_success` (category, doc comment corrected). Four shared fixtures added under `clients/proto/fixtures/behavior/command-replies/` (`write.status-category-{error-success-set,ok-success-zero}.reply.json`, `write.hresult-{s-false,e-fail}.reply.json`) + manifest + `docs/ClientBehaviorFixtures.md`; each of the five suites now runs all four fixture-driven, plus a per-language table test for the two edges fixtures cannot express (nil/null entry, `UNSPECIFIED` category). Docs same commit: `ClientLibrariesDesign.md` per-item rule sentence (its existing HRESULT `< 0` claim is now true), .NET/Go/Java README error sections. Also fixed a Java test fake that built a status with a bare `setSuccess(1)` and no category. Verification: dotnet build 0 warnings + 110 passed/1 skipped; `gofmt -l` clean, `go build ./...`, `go test ./...` all ok; `gradle test` BUILD SUCCESSFUL with **no** generated-file churn to revert this time (no `.proto` changed and `generateProto` stayed up to date); `python -m pytest` 155 passed/1 skipped; `cargo fmt` (no unrelated reformat), `cargo check`, `cargo test --workspace` 100 passed, `cargo clippy --all-targets -- -D warnings` clean. Gateway-side `ClientBehaviorFixtureTests` 8/8 re-run because the new fixtures are validated there. | diff --git a/archreview/2026-07-12/remediation/50-clients.md b/archreview/2026-07-12/remediation/50-clients.md index 41d0d34..b9123e5 100644 --- a/archreview/2026-07-12/remediation/50-clients.md +++ b/archreview/2026-07-12/remediation/50-clients.md @@ -18,8 +18,8 @@ Operating constraints carried from prior work: |----|-----|------|-----|-----|--------|-------| | CLI-35 | Medium | P0 | S | — | Done | Python CLI `stream-events` crashes on a ReplayGap | | CLI-36 | Medium | P0 | S | — | Done | Go CLI `stream-events` silently destroys the ReplayGap signal | -| CLI-37 | Medium | P1 | M | CLI-38 | Not started | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | -| CLI-38 | Medium | P1 | S | — | Not started | Align .NET/Go/Java on `hresult < 0` — lands prior CLI-08 and cures the design-doc drift | +| CLI-37 | Medium | P1 | M | CLI-38 | Done | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | +| CLI-38 | Medium | P1 | S | — | Done | Align .NET/Go/Java on `hresult < 0` — lands prior CLI-08 and cures the design-doc drift | | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 | Not started | Bump client versions off the already-published 0.1.2 before the next publish; add registry-collision guard | | CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET | | CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | diff --git a/archreview/remediation/00-tracking.md b/archreview/remediation/00-tracking.md index 1720557..f6f6ba4 100644 --- a/archreview/remediation/00-tracking.md +++ b/archreview/remediation/00-tracking.md @@ -181,7 +181,7 @@ Full design + implementation for each row lives in the linked domain doc under i | CLI-05 | Medium | — | S | — | Not started | .NET session cannot be re-attached to an existing session id | | CLI-06 | Medium | — | S | — | Not started | .NET `DisposeAsync` blocks/throws on unreachable gateway | | CLI-07 | Medium | — | S | — | Not started | .NET retry budget self-defeats on `DeadlineExceeded` | -| CLI-08 | Medium | — | S | CLI-03 | Not started | .NET/Go/Java treat any nonzero HRESULT as failure (should be `< 0`) | +| CLI-08 | Medium | — | S | CLI-03 | Done | .NET/Go/Java treat any nonzero HRESULT as failure (should be `< 0`) — landed via 2026-07-12 [CLI-38](../2026-07-12/remediation/50-clients.md#cli-38--align-netgojava-on-hresult--0-lands-prior-cli-08-cures-the-doc-drift---medium--p1) | | CLI-09 | Medium | — | M | — | Not started | Go has no typed auth-error mapping (Unauthenticated vs PermissionDenied) | | CLI-10 | Medium | — | M | — | Not started | Go uses deprecated `grpc.DialContext` + `grpc.WithBlock()` | | CLI-11 | Medium | — | S | — | Not started | Go CLI cannot opt into strict TLS validation | diff --git a/clients/dotnet/README.md b/clients/dotnet/README.md index ebdd94c..4846525 100644 --- a/clients/dotnet/README.md +++ b/clients/dotnet/README.md @@ -163,6 +163,12 @@ can keep the full `MxCommandReply`, HRESULT, and status array when MXAccess itself rejects a command. `MxAccessException.Reply` contains the raw generated reply. +`EnsureMxAccessSuccess()` follows COM semantics: only a **negative** HRESULT is +a failure, so positive success codes such as `S_FALSE` (1) pass. A status entry +fails only when `Category` is not `MxStatusCategory.Ok` — `MxStatusProxy.Success` +mirrors the raw COM member for diagnostics and never decides the verdict, which +is why `IsSuccess()` branches on the category alone. + ## Write Semantics And Common Pitfalls These are MXAccess parity behaviors that surprise new callers. The gateway diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxCommandReplyExtensionsTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxCommandReplyExtensionsTests.cs index 545019b..f1c9383 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxCommandReplyExtensionsTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxCommandReplyExtensionsTests.cs @@ -32,6 +32,57 @@ public sealed class MxCommandReplyExtensionsTests Assert.Contains("0x80040200", exception.Message); } + /// Verifies that a non-OK status category fails even when the raw success member is set. + [Fact] + public void EnsureMxAccessSuccess_WithNonOkCategoryAndSuccessSet_Throws() + { + MxCommandReply reply = ReadReplyFixture( + "write.status-category-error-success-set.reply.json"); + + reply.EnsureProtocolSuccess(); + MxAccessException exception = Assert.Throws( + reply.EnsureMxAccessSuccess); + + Assert.Equal(1, Assert.Single(exception.Statuses).Success); + Assert.Contains("CommunicationError", exception.Message, StringComparison.Ordinal); + } + + /// Verifies that an Ok status category succeeds even when the raw success member is zero. + [Fact] + public void EnsureMxAccessSuccess_WithOkCategoryAndZeroSuccess_ReturnsReply() + { + MxCommandReply reply = ReadReplyFixture( + "write.status-category-ok-success-zero.reply.json"); + + Assert.Equal(0, Assert.Single(reply.Statuses).Success); + Assert.Same(reply, reply.EnsureProtocolSuccess()); + Assert.Same(reply, reply.EnsureMxAccessSuccess()); + } + + /// Verifies that a positive HResult (S_FALSE) is a COM success code, not a failure. + [Fact] + public void EnsureMxAccessSuccess_WithPositiveHResult_ReturnsReply() + { + MxCommandReply reply = ReadReplyFixture("write.hresult-s-false.reply.json"); + + Assert.Equal(1, reply.Hresult); + Assert.Same(reply, reply.EnsureProtocolSuccess()); + Assert.Same(reply, reply.EnsureMxAccessSuccess()); + } + + /// Verifies that a negative HResult fails even when every status entry is Ok. + [Fact] + public void EnsureMxAccessSuccess_WithNegativeHResult_Throws() + { + MxCommandReply reply = ReadReplyFixture("write.hresult-e-fail.reply.json"); + + reply.EnsureProtocolSuccess(); + MxAccessException exception = Assert.Throws( + reply.EnsureMxAccessSuccess); + + Assert.Equal(-2147467259, exception.HResultCode); + } + /// Verifies that session-not-found protocol failures throw the correct gateway exception. [Fact] public void EnsureProtocolSuccess_WithSessionFailure_ThrowsSessionException() diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs index aec4195..cd417a2 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs @@ -19,9 +19,8 @@ public sealed class MxStatusProxyExtensionsTests { MxStatusProxy status = JsonParser.Default.Parse( testCase.GetProperty("status").GetRawText()); - int success = testCase.GetProperty("status").GetProperty("success").GetInt32(); - Assert.Equal(success != 0 && status.Category is MxStatusCategory.Ok, status.IsSuccess()); + Assert.Equal(status.Category is MxStatusCategory.Ok, status.IsSuccess()); Assert.Equal( testCase.GetProperty("status").GetProperty("rawCategory").GetInt32(), status.RawCategory); @@ -31,6 +30,22 @@ public sealed class MxStatusProxyExtensionsTests } } + /// Verifies that the raw success member never overrides the authoritative category. + [Theory] + [InlineData(MxStatusCategory.Ok, 0, true)] + [InlineData(MxStatusCategory.Ok, 1, true)] + [InlineData(MxStatusCategory.CommunicationError, 1, false)] + [InlineData(MxStatusCategory.Unspecified, 1, false)] + public void IsSuccess_BranchesOnCategoryOnly( + MxStatusCategory category, + int success, + bool expected) + { + MxStatusProxy status = new() { Category = category, Success = success }; + + Assert.Equal(expected, status.IsSuccess()); + } + private static string ReadFixture(string category, string fileName) { DirectoryInfo directory = new(AppContext.BaseDirectory); diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxCommandReplyExtensions.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxCommandReplyExtensions.cs index 5715904..0bf4bf5 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxCommandReplyExtensions.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxCommandReplyExtensions.cs @@ -23,7 +23,11 @@ public static class MxCommandReplyExtensions throw CreateProtocolException(reply, code); } - /// Validates that the reply indicates MXAccess success (no HResult or status failures), throwing MxAccessException if not. + /// + /// Validates that the reply indicates MXAccess success, throwing MxAccessException if not. + /// Following COM semantics, only a negative HResult is a failure — positive success codes + /// such as S_FALSE pass — and a status entry fails only when its category is not Ok. + /// /// The command reply to check. /// The same reply, for chaining. public static MxCommandReply EnsureMxAccessSuccess(this MxCommandReply reply) @@ -31,7 +35,7 @@ public static class MxCommandReplyExtensions ArgumentNullException.ThrowIfNull(reply); bool mxAccessFailure = reply.ProtocolStatus?.Code is ProtocolStatusCode.MxaccessFailure; - bool hResultFailure = reply.HasHresult && reply.Hresult != 0; + bool hResultFailure = reply.HasHresult && reply.Hresult < 0; bool statusFailure = reply.Statuses.Any(status => !status.IsSuccess()); if (!mxAccessFailure && !hResultFailure && !statusFailure) diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs index 318ad8a..5223b1b 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs @@ -5,15 +5,18 @@ namespace ZB.MOM.WW.MxGateway.Client; /// Extension methods for MxStatusProxy values. public static class MxStatusProxyExtensions { - /// Returns whether the status indicates success (success flag set and category is Ok). + /// + /// Returns whether the status indicates success, which the wire contract defines as + /// . The raw Success member is a verbatim COM + /// diagnostic, not a boolean, so it never participates in the verdict. + /// /// The status to check. /// if the status indicates success; otherwise . public static bool IsSuccess(this MxStatusProxy status) { ArgumentNullException.ThrowIfNull(status); - return status.Success != 0 - && status.Category is MxStatusCategory.Ok; + return status.Category is MxStatusCategory.Ok; } /// Returns a formatted summary of the status for diagnostic output. diff --git a/clients/go/README.md b/clients/go/README.md index c76cbbe..dbf608d 100644 --- a/clients/go/README.md +++ b/clients/go/README.md @@ -94,6 +94,12 @@ goroutine cleanup. Raw protobuf messages remain available through the `errors.As` for `GatewayError`, `CommandError`, and `MxAccessError`; command errors preserve the raw reply. +`EnsureMxAccessSuccess` follows COM semantics: only a **negative** HRESULT is a +failure, so positive success codes such as `S_FALSE` (1) pass. `StatusSucceeded` +judges each `MXSTATUS_PROXY` entry by its category — an entry fails when +`Category` is not `MX_STATUS_CATEGORY_OK`, and the raw `Success` member is a +diagnostic that never decides the verdict. A nil entry is success. + ### Reconnect-replay gap Each `EventResult` carries exactly one of `Event`, `ReplayGap`, or `Err`. When diff --git a/clients/go/mxgateway/conversion_test.go b/clients/go/mxgateway/conversion_test.go index c7633ec..a259d28 100644 --- a/clients/go/mxgateway/conversion_test.go +++ b/clients/go/mxgateway/conversion_test.go @@ -65,7 +65,8 @@ func TestStatusConversionFixtures(t *testing.T) { if err := protojson.Unmarshal(tc.Status, &status); err != nil { t.Fatalf("parse status: %v", err) } - if got, want := StatusSucceeded(&status), status.GetSuccess() != 0; got != want { + want := status.GetCategory() == pb.MxStatusCategory_MX_STATUS_CATEGORY_OK + if got := StatusSucceeded(&status); got != want { t.Fatalf("StatusSucceeded() = %v, want %v", got, want) } }) diff --git a/clients/go/mxgateway/errors.go b/clients/go/mxgateway/errors.go index 572716d..70c9f59 100644 --- a/clients/go/mxgateway/errors.go +++ b/clients/go/mxgateway/errors.go @@ -180,11 +180,15 @@ func EnsureProtocolSuccess(op string, status *ProtocolStatus, reply *MxCommandRe // EnsureMxAccessSuccess returns a typed MxAccessError for failing HRESULTs or // MXSTATUS_PROXY entries. +// +// Following COM semantics, only a negative HRESULT is a failure — positive +// success codes such as S_FALSE (1) pass. Status entries are judged by +// StatusSucceeded, which branches on the authoritative category. func EnsureMxAccessSuccess(op string, reply *MxCommandReply) error { if reply == nil { return nil } - if reply.Hresult != nil && reply.GetHresult() != 0 { + if reply.Hresult != nil && reply.GetHresult() < 0 { return &MxAccessError{Reply: reply} } for _, status := range reply.GetStatuses() { diff --git a/clients/go/mxgateway/protofixtures_test.go b/clients/go/mxgateway/protofixtures_test.go index dc5014f..f4b7e60 100644 --- a/clients/go/mxgateway/protofixtures_test.go +++ b/clients/go/mxgateway/protofixtures_test.go @@ -48,6 +48,89 @@ func TestGeneratedGoldenFixturesParse(t *testing.T) { } } +// TestCommandReplyValidationFixtures locks the shared reply-validation rules to +// the behavior fixtures: a status entry fails iff its category is not OK (the +// raw success member is diagnostics only), and an HRESULT fails iff it is +// present and negative (S_FALSE and other positive COM success codes pass). +func TestCommandReplyValidationFixtures(t *testing.T) { + tests := []struct { + fixture string + wantFailure bool + }{ + {fixture: "register.ok.reply.json", wantFailure: false}, + {fixture: "write.mxaccess-failure.reply.json", wantFailure: true}, + {fixture: "write.status-category-error-success-set.reply.json", wantFailure: true}, + {fixture: "write.status-category-ok-success-zero.reply.json", wantFailure: false}, + {fixture: "write.hresult-s-false.reply.json", wantFailure: false}, + {fixture: "write.hresult-e-fail.reply.json", wantFailure: true}, + } + + for _, tt := range tests { + t.Run(tt.fixture, func(t *testing.T) { + data, err := os.ReadFile(filepath.Join( + "..", "..", "proto", "fixtures", "behavior", "command-replies", tt.fixture)) + if err != nil { + t.Fatalf("read fixture: %v", err) + } + + var reply pb.MxCommandReply + if err := protojson.Unmarshal(data, &reply); err != nil { + t.Fatalf("parse fixture: %v", err) + } + + err = EnsureMxAccessSuccess("invoke", &reply) + if got := err != nil; got != tt.wantFailure { + t.Fatalf("EnsureMxAccessSuccess() failed = %v (err %v), want %v", got, err, tt.wantFailure) + } + }) + } +} + +// TestStatusSucceededBranchesOnCategory pins the per-entry rule directly, +// including the two edges the fixtures cannot express: a nil entry is success +// and a present entry with an unspecified category is a failure. +func TestStatusSucceededBranchesOnCategory(t *testing.T) { + tests := []struct { + name string + status *MxStatusProxy + want bool + }{ + {name: "nil entry", status: nil, want: true}, + { + name: "ok category with zero success", + status: &pb.MxStatusProxy{ + Success: 0, + Category: pb.MxStatusCategory_MX_STATUS_CATEGORY_OK, + }, + want: true, + }, + { + name: "error category with success set", + status: &pb.MxStatusProxy{ + Success: 1, + Category: pb.MxStatusCategory_MX_STATUS_CATEGORY_COMMUNICATION_ERROR, + }, + want: false, + }, + { + name: "unspecified category with success set", + status: &pb.MxStatusProxy{ + Success: 1, + Category: pb.MxStatusCategory_MX_STATUS_CATEGORY_UNSPECIFIED, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := StatusSucceeded(tt.status); got != tt.want { + t.Fatalf("StatusSucceeded() = %v, want %v", got, tt.want) + } + }) + } +} + func TestOpenSessionFixtureProtocolVersions(t *testing.T) { data, err := os.ReadFile(filepath.Join("..", "..", "proto", "fixtures", "golden", "open-session-reply.ok.json")) if err != nil { diff --git a/clients/go/mxgateway/status.go b/clients/go/mxgateway/status.go index 3ea60c6..0d1b113 100644 --- a/clients/go/mxgateway/status.go +++ b/clients/go/mxgateway/status.go @@ -1,6 +1,17 @@ package mxgateway +import ( + pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" +) + // StatusSucceeded reports whether an MXSTATUS_PROXY entry represents success. +// +// The wire contract makes Category authoritative: an entry succeeds only when +// its category is MX_STATUS_CATEGORY_OK. The Success member mirrors the raw +// 16-bit COM value verbatim for diagnostics and is not a boolean, so it takes +// no part in the verdict. A nil entry is success (nothing was reported); a +// present entry with an unspecified category is a failure, because the worker +// always maps a category and an unmapped one is not proven OK. func StatusSucceeded(status *MxStatusProxy) bool { - return status == nil || status.GetSuccess() != 0 + return status == nil || status.GetCategory() == pb.MxStatusCategory_MX_STATUS_CATEGORY_OK } diff --git a/clients/java/README.md b/clients/java/README.md index 2e0b117..2f07fe9 100644 --- a/clients/java/README.md +++ b/clients/java/README.md @@ -139,7 +139,12 @@ commands, so you do not need to build raw `MxCommand` messages: All of them run the same MXAccess reply validation as the bulk helpers (protocol status plus HRESULT/`MxStatusProxy` check) via the shared `invoke` path, so an -MXAccess COM-side failure surfaces as `MxAccessException`. +MXAccess COM-side failure surfaces as `MxAccessException`. That validation +follows COM semantics: only a **negative** HRESULT is a failure, so positive +success codes such as `S_FALSE` (1) pass. `MxStatuses.succeeded` judges each +entry by its category — an entry fails when its category is not +`MX_STATUS_CATEGORY_OK`, and the raw `success` member is a diagnostic that never +decides the verdict. A `null` entry is success. **Secret redaction.** Credentials passed to `authenticateUser` (and the credential-sensitive values passed to `writeSecured`/`writeSecured2`) travel diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayErrors.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayErrors.java index 065e3af..638ad9c 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayErrors.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayErrors.java @@ -47,7 +47,9 @@ final class MxGatewayErrors { if (reply == null) { return; } - if (reply.hasHresult() && reply.getHresult() != 0) { + // COM semantics: only a negative HRESULT is a failure. Positive success + // codes such as S_FALSE (1) pass. + if (reply.hasHresult() && reply.getHresult() < 0) { throw new MxAccessException(operation, reply); } for (var status : reply.getStatusesList()) { diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxStatuses.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxStatuses.java index 3ff45e2..56daf4b 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxStatuses.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxStatuses.java @@ -8,8 +8,11 @@ import mxaccess_gateway.v1.MxaccessGateway.MxStatusSource; * Helpers for inspecting {@link MxStatusProxy} values returned by the gateway. * *

An {@code MxStatusProxy} mirrors the MXAccess COM {@code MXSTATUS_PROXY} - * struct. The success flag uses the MXAccess convention where any non-zero - * value indicates success. + * struct. Per the wire contract, {@code category} is the authoritative verdict: + * an entry succeeds only when its category is + * {@code MX_STATUS_CATEGORY_OK}. The {@code success} member carries the raw + * 16-bit COM value verbatim for diagnostics and is not a boolean, so it never + * decides success or failure. */ public final class MxStatuses { private MxStatuses() { @@ -18,12 +21,17 @@ public final class MxStatuses { /** * Returns whether the supplied status proxy reports success. * + *

A {@code null} status is success because nothing was reported. A + * present entry whose category is {@code MX_STATUS_CATEGORY_UNSPECIFIED} + * is a failure: the worker always maps a category, so an unmapped one is + * not proven OK. + * * @param status the status proxy, may be {@code null} - * @return {@code true} if {@code status} is {@code null} or its success - * flag is non-zero, {@code false} otherwise + * @return {@code true} if {@code status} is {@code null} or its category is + * {@code MX_STATUS_CATEGORY_OK}, {@code false} otherwise */ public static boolean succeeded(MxStatusProxy status) { - return status == null || status.getSuccess() != 0; + return status == null || status.getCategory() == MxStatusCategory.MX_STATUS_CATEGORY_OK; } /** @@ -44,9 +52,11 @@ public final class MxStatuses { */ public record MxStatusView(MxStatusProxy raw) { /** - * Returns the raw success flag (non-zero indicates success). + * Returns the raw {@code success} member exactly as MXAccess reported + * it. This is a diagnostic value, not a verdict — use + * {@link MxStatuses#succeeded(MxStatusProxy)} to decide success. * - * @return the success flag value + * @return the raw success member */ public int success() { return raw.getSuccess(); diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java index bfc3a2f..b6c04bf 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayClientSessionTests.java @@ -701,14 +701,17 @@ final class MxGatewayClientSessionTests { .setSessionId(request.getSessionId()) .setKind(request.getCommand().getKind()) .setProtocolStatus(ok()); + // `category` is the authoritative success indicator, so the fake + // must set it — a bare non-zero `success` is not a success. + var okStatus = mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy.newBuilder() + .setSuccess(1) + .setCategory(mxaccess_gateway.v1.MxaccessGateway.MxStatusCategory.MX_STATUS_CATEGORY_OK); if (request.getCommand().getKind() == MxCommandKind.MX_COMMAND_KIND_SUSPEND) { reply.setSuspend(mxaccess_gateway.v1.MxaccessGateway.SuspendReply.newBuilder() - .setStatus(mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy.newBuilder() - .setSuccess(1))); + .setStatus(okStatus)); } else if (request.getCommand().getKind() == MxCommandKind.MX_COMMAND_KIND_ACTIVATE) { reply.setActivate(mxaccess_gateway.v1.MxaccessGateway.ActivateReply.newBuilder() - .setStatus(mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy.newBuilder() - .setSuccess(1))); + .setStatus(okStatus)); } responseObserver.onNext(reply.build()); responseObserver.onCompleted(); diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayFixtureTests.java b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayFixtureTests.java index fbe093b..0663f3a 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayFixtureTests.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayFixtureTests.java @@ -4,6 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import com.google.gson.JsonArray; @@ -20,6 +21,8 @@ import mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy; import mxaccess_gateway.v1.MxaccessGateway.MxValue; import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatusCode; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; final class MxGatewayFixtureTests { @Test @@ -89,6 +92,50 @@ final class MxGatewayFixtureTests { throw new AssertionError("expected MxAccessException"); } + @ParameterizedTest + @CsvSource({ + "register.ok.reply.json,false", + "write.status-category-error-success-set.reply.json,true", + "write.status-category-ok-success-zero.reply.json,false", + "write.hresult-s-false.reply.json,false", + "write.hresult-e-fail.reply.json,true", + }) + void replyValidationFixturesBranchOnCategoryAndNegativeHresult(String fixture, boolean expectFailure) + throws Exception { + MxCommandReply.Builder builder = MxCommandReply.newBuilder(); + JsonFormat.parser().merge( + Files.readString(fixtureRoot().resolve("command-replies/" + fixture)), + builder); + MxCommandReply reply = builder.build(); + + if (expectFailure) { + assertThrows(MxAccessException.class, () -> MxGatewayErrors.ensureMxAccessSuccess("write", reply)); + } else { + MxGatewayErrors.ensureMxAccessSuccess("write", reply); + } + } + + @ParameterizedTest + @CsvSource({ + "MX_STATUS_CATEGORY_OK,0,true", + "MX_STATUS_CATEGORY_OK,1,true", + "MX_STATUS_CATEGORY_COMMUNICATION_ERROR,1,false", + "MX_STATUS_CATEGORY_UNSPECIFIED,1,false", + }) + void statusEntryVerdictIgnoresTheRawSuccessMember(String category, int success, boolean expectSucceeded) { + MxStatusProxy status = MxStatusProxy.newBuilder() + .setCategory(MxStatusCategory.valueOf(category)) + .setSuccess(success) + .build(); + + assertEquals(expectSucceeded, MxStatuses.succeeded(status)); + } + + @Test + void absentStatusEntryIsSuccess() { + assertTrue(MxStatuses.succeeded(null)); + } + @Test void grpcAuthErrorsAreClassifiedAndRedacted() { RuntimeException authError = MxGatewayErrors.fromGrpc( diff --git a/clients/proto/fixtures/behavior/command-replies/write.hresult-e-fail.reply.json b/clients/proto/fixtures/behavior/command-replies/write.hresult-e-fail.reply.json new file mode 100644 index 0000000..3fb72ea --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/write.hresult-e-fail.reply.json @@ -0,0 +1,29 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-write-e-fail", + "kind": "MX_COMMAND_KIND_WRITE", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "Write reached MXAccess." + }, + "hresult": -2147467259, + "returnValue": { + "dataType": "MX_DATA_TYPE_NO_DATA", + "variantType": "VT_EMPTY", + "isNull": true, + "rawDiagnostic": "MXAccess returned no value for the failed write.", + "rawDataType": 2 + }, + "statuses": [ + { + "success": 1, + "category": "MX_STATUS_CATEGORY_OK", + "detectedBy": "MX_STATUS_SOURCE_RESPONDING_LMX", + "detail": 0, + "rawCategory": 0, + "rawDetectedBy": 3, + "diagnosticText": "OK" + } + ], + "diagnosticMessage": "COM semantics: a negative HRESULT (E_FAIL, 0x80004005) is a failure even when every status entry is OK." +} diff --git a/clients/proto/fixtures/behavior/command-replies/write.hresult-s-false.reply.json b/clients/proto/fixtures/behavior/command-replies/write.hresult-s-false.reply.json new file mode 100644 index 0000000..6d77888 --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/write.hresult-s-false.reply.json @@ -0,0 +1,29 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-write-s-false", + "kind": "MX_COMMAND_KIND_WRITE", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "Write completed with S_FALSE." + }, + "hresult": 1, + "returnValue": { + "dataType": "MX_DATA_TYPE_NO_DATA", + "variantType": "VT_EMPTY", + "isNull": true, + "rawDiagnostic": "MXAccess returned no value for the write.", + "rawDataType": 2 + }, + "statuses": [ + { + "success": 1, + "category": "MX_STATUS_CATEGORY_OK", + "detectedBy": "MX_STATUS_SOURCE_RESPONDING_LMX", + "detail": 0, + "rawCategory": 0, + "rawDetectedBy": 3, + "diagnosticText": "OK" + } + ], + "diagnosticMessage": "COM semantics: a positive HRESULT such as S_FALSE (1) is a success code, not a failure." +} diff --git a/clients/proto/fixtures/behavior/command-replies/write.status-category-error-success-set.reply.json b/clients/proto/fixtures/behavior/command-replies/write.status-category-error-success-set.reply.json new file mode 100644 index 0000000..741740d --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/write.status-category-error-success-set.reply.json @@ -0,0 +1,29 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-write-category-error", + "kind": "MX_COMMAND_KIND_WRITE", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "Write reached MXAccess." + }, + "hresult": 0, + "returnValue": { + "dataType": "MX_DATA_TYPE_NO_DATA", + "variantType": "VT_EMPTY", + "isNull": true, + "rawDiagnostic": "MXAccess returned no value for the write.", + "rawDataType": 2 + }, + "statuses": [ + { + "success": 1, + "category": "MX_STATUS_CATEGORY_COMMUNICATION_ERROR", + "detectedBy": "MX_STATUS_SOURCE_RESPONDING_LMX", + "detail": 77, + "rawCategory": 5, + "rawDetectedBy": 3, + "diagnosticText": "Responding LMX lost communication mid-write." + } + ], + "diagnosticMessage": "Category is authoritative: a non-OK category is a failure even when the raw success member is non-zero." +} diff --git a/clients/proto/fixtures/behavior/command-replies/write.status-category-ok-success-zero.reply.json b/clients/proto/fixtures/behavior/command-replies/write.status-category-ok-success-zero.reply.json new file mode 100644 index 0000000..028d9b5 --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/write.status-category-ok-success-zero.reply.json @@ -0,0 +1,29 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-write-category-ok", + "kind": "MX_COMMAND_KIND_WRITE", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "Write completed." + }, + "hresult": 0, + "returnValue": { + "dataType": "MX_DATA_TYPE_NO_DATA", + "variantType": "VT_EMPTY", + "isNull": true, + "rawDiagnostic": "MXAccess returned no value for the write.", + "rawDataType": 2 + }, + "statuses": [ + { + "success": 0, + "category": "MX_STATUS_CATEGORY_OK", + "detectedBy": "MX_STATUS_SOURCE_RESPONDING_LMX", + "detail": 0, + "rawCategory": 0, + "rawDetectedBy": 3, + "diagnosticText": "OK, reported with a zero raw success member." + } + ], + "diagnosticMessage": "Category is authoritative: MX_STATUS_CATEGORY_OK is success even when the raw success member is zero." +} diff --git a/clients/proto/fixtures/behavior/manifest.json b/clients/proto/fixtures/behavior/manifest.json index aea5a5c..13fd66f 100644 --- a/clients/proto/fixtures/behavior/manifest.json +++ b/clients/proto/fixtures/behavior/manifest.json @@ -20,6 +20,34 @@ "path": "command-replies/write.mxaccess-failure.reply.json", "expectation": "MXAccess failures are data-bearing replies with HRESULT and status details, not transport failures." }, + { + "id": "command-reply.write.status-category-error-success-set", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/write.status-category-error-success-set.reply.json", + "expectation": "A status entry fails when its category is not MX_STATUS_CATEGORY_OK, even though the raw success member is non-zero." + }, + { + "id": "command-reply.write.status-category-ok-success-zero", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/write.status-category-ok-success-zero.reply.json", + "expectation": "A status entry succeeds when its category is MX_STATUS_CATEGORY_OK, even though the raw success member is zero." + }, + { + "id": "command-reply.write.hresult-s-false", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/write.hresult-s-false.reply.json", + "expectation": "A positive HRESULT such as S_FALSE (1) is a COM success code and does not fail the reply." + }, + { + "id": "command-reply.write.hresult-e-fail", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/write.hresult-e-fail.reply.json", + "expectation": "A negative HRESULT fails the reply even when every status entry reports MX_STATUS_CATEGORY_OK." + }, { "id": "event-stream.session-ordered", "category": "event_streams", diff --git a/clients/python/src/zb_mom_ww_mxgateway/errors.py b/clients/python/src/zb_mom_ww_mxgateway/errors.py index 7f689af..d7f65db 100644 --- a/clients/python/src/zb_mom_ww_mxgateway/errors.py +++ b/clients/python/src/zb_mom_ww_mxgateway/errors.py @@ -137,8 +137,10 @@ def ensure_mxaccess_success(operation: str, reply: pb.MxCommandReply) -> pb.MxCo raw_reply=reply, ) + # `category` is the authoritative verdict per the wire contract; `success` + # is the raw COM member carried verbatim for diagnostics only. for mx_status in reply.statuses: - if mx_status.success == 0: + if mx_status.category != pb.MX_STATUS_CATEGORY_OK: raise MxAccessError( _mxaccess_message(operation, reply), protocol_status=status, diff --git a/clients/python/tests/test_errors.py b/clients/python/tests/test_errors.py index f054c4d..36493a2 100644 --- a/clients/python/tests/test_errors.py +++ b/clients/python/tests/test_errors.py @@ -32,6 +32,55 @@ def test_write_failure_fixture_preserves_raw_reply() -> None: assert len(captured.value.raw_reply.statuses) == 2 +@pytest.mark.parametrize( + ("fixture", "expect_failure"), + [ + ("command-replies/register.ok.reply.json", False), + ("command-replies/write.status-category-error-success-set.reply.json", True), + ("command-replies/write.status-category-ok-success-zero.reply.json", False), + ("command-replies/write.hresult-s-false.reply.json", False), + ("command-replies/write.hresult-e-fail.reply.json", True), + ], +) +def test_reply_validation_fixtures_branch_on_category_and_negative_hresult( + fixture: str, + expect_failure: bool, +) -> None: + reply = _load_reply(fixture) + + if expect_failure: + with pytest.raises(MxAccessError): + ensure_mxaccess_success("write", reply) + else: + assert ensure_mxaccess_success("write", reply) is reply + + +@pytest.mark.parametrize( + ("category", "success", "expect_failure"), + [ + (pb.MX_STATUS_CATEGORY_OK, 0, False), + (pb.MX_STATUS_CATEGORY_OK, 1, False), + (pb.MX_STATUS_CATEGORY_COMMUNICATION_ERROR, 1, True), + (pb.MX_STATUS_CATEGORY_UNSPECIFIED, 1, True), + ], +) +def test_status_entry_verdict_ignores_the_raw_success_member( + category: int, + success: int, + expect_failure: bool, +) -> None: + reply = pb.MxCommandReply( + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + statuses=[pb.MxStatusProxy(success=success, category=category)], + ) + + if expect_failure: + with pytest.raises(MxAccessError): + ensure_mxaccess_success("write", reply) + else: + assert ensure_mxaccess_success("write", reply) is reply + + def test_session_status_maps_to_session_error() -> None: status = pb.ProtocolStatus( code=pb.PROTOCOL_STATUS_CODE_SESSION_NOT_FOUND, diff --git a/clients/rust/src/error.rs b/clients/rust/src/error.rs index 0c335b9..0a0bf74 100644 --- a/clients/rust/src/error.rs +++ b/clients/rust/src/error.rs @@ -308,10 +308,12 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result Result Result { let hresult_failure = reply.hresult.is_some_and(|hresult| hresult < 0); - let status_failure = reply.statuses.iter().any(|status| status.success == 0); + let status_failure = reply + .statuses + .iter() + .any(|status| status.category != MxStatusCategory::Ok as i32); if hresult_failure || status_failure { Err(Box::new(MxAccessError::new(reply)).into()) @@ -412,8 +418,10 @@ mod tests { let mut reply = ok_reply(); // Positive hresult (e.g. S_FALSE = 1) is a success, not a failure. reply.hresult = Some(1); + // A zero `success` member with an OK category is still a success: the + // category is authoritative and `success` is diagnostics only. reply.statuses = vec![MxStatusProxy { - success: 1, + success: 0, category: MxStatusCategory::Ok as i32, ..MxStatusProxy::default() }]; @@ -424,8 +432,9 @@ mod tests { #[test] fn ensure_mxaccess_success_flags_failing_status_entry() { let mut reply = ok_reply(); + // A non-OK category fails even though the raw `success` member is set. reply.statuses = vec![MxStatusProxy { - success: 0, + success: 1, category: MxStatusCategory::CommunicationError as i32, detail: 42, diagnostic_text: "write rejected for mxgw_visible_secret".to_owned(), diff --git a/clients/rust/src/value.rs b/clients/rust/src/value.rs index 94e43d1..51e3948 100644 --- a/clients/rust/src/value.rs +++ b/clients/rust/src/value.rs @@ -282,7 +282,11 @@ impl MxStatus { &self.raw } - /// `MXSTATUS_PROXY.Success` flag (0 = error, non-zero = good/warning). + /// Raw `MXSTATUS_PROXY.Success` member, carried verbatim from COM. + /// + /// This is a diagnostic value, not a verdict: the wire contract makes + /// [`Self::category`] authoritative, and `ensure_mxaccess_success` branches + /// on the category alone. pub fn success(&self) -> i32 { self.raw.success } diff --git a/clients/rust/tests/client_behavior.rs b/clients/rust/tests/client_behavior.rs index faf7fba..28f4a58 100644 --- a/clients/rust/tests/client_behavior.rs +++ b/clients/rust/tests/client_behavior.rs @@ -14,6 +14,7 @@ use tokio::sync::{mpsc, Mutex}; use tokio_stream::wrappers::{ReceiverStream, TcpListenerStream}; use tonic::transport::Server; use tonic::{Request, Response, Status}; +use zb_mom_ww_mxgateway_client::error::ensure_mxaccess_success; use zb_mom_ww_mxgateway_client::generated::mxaccess_gateway::v1::mx_access_gateway_server::{ MxAccessGateway, MxAccessGatewayServer, }; @@ -337,6 +338,57 @@ fn authentication_and_authorization_statuses_are_distinct_and_redacted() { assert!(!auth.to_string().contains("visible_secret")); } +#[test] +fn command_reply_validation_fixtures_branch_on_category_and_negative_hresult() { + // The shared behavior fixtures pin both reply-validation rules: a status + // entry fails iff its category is not OK (the raw `success` member is + // diagnostics only) and an HRESULT fails iff it is present and negative. + for (fixture, expect_failure) in [ + ("register.ok.reply.json", false), + ("write.status-category-error-success-set.reply.json", true), + ("write.status-category-ok-success-zero.reply.json", false), + ("write.hresult-s-false.reply.json", false), + ("write.hresult-e-fail.reply.json", true), + ] { + let reply = command_reply_fixture(fixture); + let result = ensure_mxaccess_success(reply); + + assert_eq!( + result.is_err(), + expect_failure, + "fixture {fixture} expected failure = {expect_failure}, got {result:?}" + ); + } +} + +#[test] +fn status_entry_verdict_ignores_the_raw_success_member() { + // Edges the fixtures cannot express: an OK category always passes and an + // unspecified category always fails, whatever `success` carries. + for (category, success, expect_failure) in [ + (MxStatusCategory::Ok, 0, false), + (MxStatusCategory::Ok, 1, false), + (MxStatusCategory::CommunicationError, 1, true), + (MxStatusCategory::Unspecified, 1, true), + ] { + let reply = MxCommandReply { + protocol_status: Some(ok_status("command ok")), + statuses: vec![MxStatusProxy { + success, + category: category as i32, + ..MxStatusProxy::default() + }], + ..MxCommandReply::default() + }; + + assert_eq!( + ensure_mxaccess_success(reply).is_err(), + expect_failure, + "category {category:?} with success {success} expected failure = {expect_failure}" + ); + } +} + #[test] fn command_error_display_keeps_raw_reply_accessible() { let reply = mxaccess_failure_reply(); @@ -1358,6 +1410,52 @@ fn event(sequence: u64) -> MxEvent { } } +/// Load a shared command-reply fixture into an [`MxCommandReply`]. +/// +/// The fixtures are protobuf JSON, which prost cannot parse directly, so this +/// reads the fields the reply-validation rules actually consume (`hresult` and +/// the status `success`/`category` pair) and rebuilds the message. Enum names +/// resolve through the generated `from_str_name`, so a fixture naming a +/// category the contract does not define fails the test rather than silently +/// degrading to `Unspecified`. +fn command_reply_fixture(file_name: &str) -> MxCommandReply { + let fixture = behavior_fixture(&format!("command-replies/{file_name}")); + + let statuses = fixture["statuses"] + .as_array() + .map(Vec::as_slice) + .unwrap_or_default() + .iter() + .map(|status| { + let category_name = status["category"].as_str().unwrap(); + MxStatusProxy { + success: status["success"].as_i64().unwrap() as i32, + category: MxStatusCategory::from_str_name(category_name) + .unwrap_or_else(|| panic!("unknown status category {category_name}")) + as i32, + detail: status["detail"].as_i64().unwrap_or_default() as i32, + diagnostic_text: status["diagnosticText"] + .as_str() + .unwrap_or_default() + .to_owned(), + ..MxStatusProxy::default() + } + }) + .collect(); + + MxCommandReply { + session_id: fixture["sessionId"].as_str().unwrap_or_default().to_owned(), + correlation_id: fixture["correlationId"] + .as_str() + .unwrap_or_default() + .to_owned(), + protocol_status: Some(ok_status("command ok")), + hresult: fixture["hresult"].as_i64().map(|hresult| hresult as i32), + statuses, + ..MxCommandReply::default() + } +} + fn behavior_fixture(path: &str) -> Value { let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) .join("../proto/fixtures/behavior") diff --git a/docs/ClientBehaviorFixtures.md b/docs/ClientBehaviorFixtures.md index 2cf6699..b8d058b 100644 --- a/docs/ClientBehaviorFixtures.md +++ b/docs/ClientBehaviorFixtures.md @@ -44,6 +44,32 @@ MXAccess failures remain command replies when the gateway reached the worker and the worker captured HRESULT or `MXSTATUS_PROXY` details. Client wrappers should map those replies to rich command errors without discarding the raw reply. +### Reply Validation Conformance + +Four command reply fixtures pin the two reply-validation rules every client +applies, because both rules have edges where a naive reading disagrees with the +wire contract: + +| Fixture | Reply | Expected verdict | +|---|---|---| +| `write.status-category-error-success-set.reply.json` | one status with `success = 1`, `category = MX_STATUS_CATEGORY_COMMUNICATION_ERROR` | failure | +| `write.status-category-ok-success-zero.reply.json` | one status with `success = 0`, `category = MX_STATUS_CATEGORY_OK` | success | +| `write.hresult-s-false.reply.json` | `hresult = 1` (`S_FALSE`), statuses OK | success | +| `write.hresult-e-fail.reply.json` | `hresult = -2147467259` (`E_FAIL`), statuses OK | failure | + +The rules those fixtures lock in are: + +- **Status entries.** An `MxStatusProxy` entry is a failure exactly when + `category != MX_STATUS_CATEGORY_OK`. `success` mirrors the raw 16-bit COM + member and is diagnostics only, so it never participates in the verdict — the + proto contract makes `category` authoritative. An absent entry is success + (nothing was reported); a present entry with + `MX_STATUS_CATEGORY_UNSPECIFIED` is a failure, because the worker always maps + a category and an unmapped one is not proven OK. +- **HRESULT.** A reply fails on HRESULT exactly when `hresult` is present and + negative. Positive COM success codes such as `S_FALSE` pass, matching COM + semantics. + ## Event Streams Event stream fixtures live in diff --git a/docs/ClientLibrariesDesign.md b/docs/ClientLibrariesDesign.md index 77356a0..9da39e4 100644 --- a/docs/ClientLibrariesDesign.md +++ b/docs/ClientLibrariesDesign.md @@ -115,7 +115,14 @@ remove/write family, the parity-critical single-item helpers are: `Activate`, and `Unregister`. Each is a thin wrapper over the same raw-command machinery the bulk helpers use — it adds no wire surface — and runs the same MXAccess-level reply validation (HRESULT `< 0` + per-item `MxStatusProxy`) as the -rest of the client. **MXAccess parity is preserved exactly**: e.g. `WriteSecured` +rest of the client. The per-item rule is **`MxStatusProxy` failure iff +`category != MX_STATUS_CATEGORY_OK`**; `success` is the raw COM member carried +for diagnostics only and never decides the verdict, so an absent entry is +success while a present entry with an unspecified category is a failure. The +shared fixtures in `clients/proto/fixtures/behavior/command-replies/` pin both +rules across all five clients (see +[Client Behavior Fixtures](./ClientBehaviorFixtures.md)). +**MXAccess parity is preserved exactly**: e.g. `WriteSecured` failing before a prior `AuthenticateUser` + `AdviseSupervisory` surfaces the native failure unchanged — the helper does not pre-validate or reorder it.