From d6b2f24c3fd4abd698abb6bc782aae4ed0eb5489 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:00:58 -0400 Subject: [PATCH] fix(CLI-37,CLI-38): make status/HRESULT reply validation conformant across all five clients MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One cross-client conformance pass; also closes first-cycle CLI-08. CLI-37: an MxStatusProxy entry is a failure iff `category != MX_STATUS_CATEGORY_OK`. The proto contract has always said so — `success` is the raw 16-bit COM member carried verbatim for diagnostics, not a boolean — but four clients branched on `success` alone and .NET required both, so the same gateway reply produced opposite verdicts per language. An absent entry stays success; 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. CLI-38: a reply fails on HRESULT iff `hresult` is present and negative, so positive COM success codes such as S_FALSE (1) pass. .NET/Go/Java used `!= 0`, which errored on a parity-preserving S_FALSE that Python and Rust accepted. This makes the existing ClientLibrariesDesign.md claim true rather than rewriting the doc to describe the divergence. Four shared fixtures pin both rules cross-client, and each language suite also carries a table test for the two edges a fixture cannot express (absent entry, UNSPECIFIED category). A Java test fake that built a status with a bare `setSuccess(1)` and no category is fixed — under the category rule that reply was never a success. --- .../2026-07-12/remediation/00-tracking.md | 7 +- .../2026-07-12/remediation/50-clients.md | 4 +- archreview/remediation/00-tracking.md | 2 +- clients/dotnet/README.md | 6 ++ .../MxCommandReplyExtensionsTests.cs | 51 ++++++++++ .../MxStatusProxyExtensionsTests.cs | 19 +++- .../MxCommandReplyExtensions.cs | 8 +- .../MxStatusProxyExtensions.cs | 9 +- clients/go/README.md | 6 ++ clients/go/mxgateway/conversion_test.go | 3 +- clients/go/mxgateway/errors.go | 6 +- clients/go/mxgateway/protofixtures_test.go | 83 ++++++++++++++++ clients/go/mxgateway/status.go | 13 ++- clients/java/README.md | 7 +- .../ww/mxgateway/client/MxGatewayErrors.java | 4 +- .../mom/ww/mxgateway/client/MxStatuses.java | 24 +++-- .../client/MxGatewayClientSessionTests.java | 11 ++- .../client/MxGatewayFixtureTests.java | 47 +++++++++ .../write.hresult-e-fail.reply.json | 29 ++++++ .../write.hresult-s-false.reply.json | 29 ++++++ ...atus-category-error-success-set.reply.json | 29 ++++++ ...status-category-ok-success-zero.reply.json | 29 ++++++ clients/proto/fixtures/behavior/manifest.json | 28 ++++++ .../python/src/zb_mom_ww_mxgateway/errors.py | 4 +- clients/python/tests/test_errors.py | 49 ++++++++++ clients/rust/src/error.rs | 25 +++-- clients/rust/src/value.rs | 6 +- clients/rust/tests/client_behavior.rs | 98 +++++++++++++++++++ docs/ClientBehaviorFixtures.md | 26 +++++ docs/ClientLibrariesDesign.md | 9 +- 30 files changed, 631 insertions(+), 40 deletions(-) create mode 100644 clients/proto/fixtures/behavior/command-replies/write.hresult-e-fail.reply.json create mode 100644 clients/proto/fixtures/behavior/command-replies/write.hresult-s-false.reply.json create mode 100644 clients/proto/fixtures/behavior/command-replies/write.status-category-error-success-set.reply.json create mode 100644 clients/proto/fixtures/behavior/command-replies/write.status-category-ok-success-zero.reply.json diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index c6ed0c2..d83e3ef 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 @@ -164,3 +164,4 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **TST-29 → `Done`:** migrated the Phase-5 (orphan-worker reattach) deferred-not-planned governance record and the settled Phase-4 Viewer-default decision from `oldtasks.md` into a new "Session-Resilience Epic Scope" entry in `docs/DesignDecisions.md`; repointed CLAUDE.md and `stillpending.md:7,165` from `oldtasks.md` to `docs/DesignDecisions.md` / `docs/plans/2026-06-15-session-resilience.md.tasks.json`; `git rm oldtasks.md`. The five untracked root docs-review artifacts (`MxAccessGateway-docs-{issues,fixed,final}.md`, `MxGatewayClient-docs-{issues,fixed}.md`) were absent from this worktree — delete from the main working tree separately. | | 2026-08-07 | **GWC-24 → `Done`** (branch `fix/gwc-24-staging-bound`). `WorkerClient._eventStaging` is now `Channel.CreateBounded` at `2 × EventChannelCapacity` (`Wait`, single reader/writer, no sync continuations); a rejected staging `TryWrite` faults the client `ProtocolViolation` with `QueueOverflow("worker-event-staging")` unless `IsTerminalState()` (shutdown stays a silent drop), so a consumer draining slower than its worker produces dies at a fixed ceiling instead of growing gateway memory. Queue-depth accounting moved from `EnqueueWorkerEventAsync` to `StageWorkerEvent`, so the single gauge reports staged + queued; the timed-write fault (`EventChannelFullModeTimeout` / `QueueOverflow("worker-events")`) is unchanged and still catches the full-stall case first. No new config key — total gateway-side buffering is `3 × MxGateway:Events:QueueCapacity`, derived; coordination with still-open old **GWC-21** (`EventChannelFullModeTimeout` configurability) remains open and was not blocked on. Docs same commit: `GatewayProcessDesign.md` (two overflow faults), `MxAccessWorkerInstanceDesign.md`, `GatewayConfiguration.md`, `Metrics.md`. Tests: `WorkerClientTests.StagingChannelOverflowFaultsWorkerWithoutWaitingForFullModeTimeout` and `.WorkerEventQueueDepthGaugeCountsStagedEvents`; `WorkerClientTests` 22/22 green, `NonWindows.slnx` builds with 0 warnings. | | 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 | **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.