fix(CLI-37,CLI-38): make status/HRESULT reply validation conformant across all five clients

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.
This commit is contained in:
Joseph Doherty
2026-08-07 06:00:58 -04:00
parent cf66ebbcfb
commit d6b2f24c3f
30 changed files with 631 additions and 40 deletions
@@ -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-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-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-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 | — | Not started | Align .NET/Go/Java on `hresult < 0` — lands old CLI-08, cures design-doc drift | | 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-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-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 | | 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)) ## 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)). - 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. - 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 ## 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 | **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 | **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=<n> oldest_available=<n>` 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 | **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=<n> oldest_available=<n>` 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. |
@@ -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-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-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-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 | — | Not started | Align .NET/Go/Java on `hresult < 0` — lands prior CLI-08 and cures the design-doc drift | | 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-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-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 | | CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem |
+1 -1
View File
@@ -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-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-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-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-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-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 | | CLI-11 | Medium | — | S | — | Not started | Go CLI cannot opt into strict TLS validation |
+6
View File
@@ -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 itself rejects a command. `MxAccessException.Reply` contains the raw generated
reply. 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 ## Write Semantics And Common Pitfalls
These are MXAccess parity behaviors that surprise new callers. The gateway These are MXAccess parity behaviors that surprise new callers. The gateway
@@ -32,6 +32,57 @@ public sealed class MxCommandReplyExtensionsTests
Assert.Contains("0x80040200", exception.Message); Assert.Contains("0x80040200", exception.Message);
} }
/// <summary>Verifies that a non-OK status category fails even when the raw success member is set.</summary>
[Fact]
public void EnsureMxAccessSuccess_WithNonOkCategoryAndSuccessSet_Throws()
{
MxCommandReply reply = ReadReplyFixture(
"write.status-category-error-success-set.reply.json");
reply.EnsureProtocolSuccess();
MxAccessException exception = Assert.Throws<MxAccessException>(
reply.EnsureMxAccessSuccess);
Assert.Equal(1, Assert.Single(exception.Statuses).Success);
Assert.Contains("CommunicationError", exception.Message, StringComparison.Ordinal);
}
/// <summary>Verifies that an Ok status category succeeds even when the raw success member is zero.</summary>
[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());
}
/// <summary>Verifies that a positive HResult (S_FALSE) is a COM success code, not a failure.</summary>
[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());
}
/// <summary>Verifies that a negative HResult fails even when every status entry is Ok.</summary>
[Fact]
public void EnsureMxAccessSuccess_WithNegativeHResult_Throws()
{
MxCommandReply reply = ReadReplyFixture("write.hresult-e-fail.reply.json");
reply.EnsureProtocolSuccess();
MxAccessException exception = Assert.Throws<MxAccessException>(
reply.EnsureMxAccessSuccess);
Assert.Equal(-2147467259, exception.HResultCode);
}
/// <summary>Verifies that session-not-found protocol failures throw the correct gateway exception.</summary> /// <summary>Verifies that session-not-found protocol failures throw the correct gateway exception.</summary>
[Fact] [Fact]
public void EnsureProtocolSuccess_WithSessionFailure_ThrowsSessionException() public void EnsureProtocolSuccess_WithSessionFailure_ThrowsSessionException()
@@ -19,9 +19,8 @@ public sealed class MxStatusProxyExtensionsTests
{ {
MxStatusProxy status = JsonParser.Default.Parse<MxStatusProxy>( MxStatusProxy status = JsonParser.Default.Parse<MxStatusProxy>(
testCase.GetProperty("status").GetRawText()); 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( Assert.Equal(
testCase.GetProperty("status").GetProperty("rawCategory").GetInt32(), testCase.GetProperty("status").GetProperty("rawCategory").GetInt32(),
status.RawCategory); status.RawCategory);
@@ -31,6 +30,22 @@ public sealed class MxStatusProxyExtensionsTests
} }
} }
/// <summary>Verifies that the raw success member never overrides the authoritative category.</summary>
[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) private static string ReadFixture(string category, string fileName)
{ {
DirectoryInfo directory = new(AppContext.BaseDirectory); DirectoryInfo directory = new(AppContext.BaseDirectory);
@@ -23,7 +23,11 @@ public static class MxCommandReplyExtensions
throw CreateProtocolException(reply, code); throw CreateProtocolException(reply, code);
} }
/// <summary>Validates that the reply indicates MXAccess success (no HResult or status failures), throwing MxAccessException if not.</summary> /// <summary>
/// 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 <c>S_FALSE</c> pass — and a status entry fails only when its category is not Ok.
/// </summary>
/// <param name="reply">The command reply to check.</param> /// <param name="reply">The command reply to check.</param>
/// <returns>The same reply, for chaining.</returns> /// <returns>The same reply, for chaining.</returns>
public static MxCommandReply EnsureMxAccessSuccess(this MxCommandReply reply) public static MxCommandReply EnsureMxAccessSuccess(this MxCommandReply reply)
@@ -31,7 +35,7 @@ public static class MxCommandReplyExtensions
ArgumentNullException.ThrowIfNull(reply); ArgumentNullException.ThrowIfNull(reply);
bool mxAccessFailure = reply.ProtocolStatus?.Code is ProtocolStatusCode.MxaccessFailure; 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()); bool statusFailure = reply.Statuses.Any(status => !status.IsSuccess());
if (!mxAccessFailure && !hResultFailure && !statusFailure) if (!mxAccessFailure && !hResultFailure && !statusFailure)
@@ -5,15 +5,18 @@ namespace ZB.MOM.WW.MxGateway.Client;
/// <summary>Extension methods for MxStatusProxy values.</summary> /// <summary>Extension methods for MxStatusProxy values.</summary>
public static class MxStatusProxyExtensions public static class MxStatusProxyExtensions
{ {
/// <summary>Returns whether the status indicates success (success flag set and category is Ok).</summary> /// <summary>
/// Returns whether the status indicates success, which the wire contract defines as
/// <see cref="MxStatusCategory.Ok"/>. The raw <c>Success</c> member is a verbatim COM
/// diagnostic, not a boolean, so it never participates in the verdict.
/// </summary>
/// <param name="status">The status to check.</param> /// <param name="status">The status to check.</param>
/// <returns><see langword="true"/> if the status indicates success; otherwise <see langword="false"/>.</returns> /// <returns><see langword="true"/> if the status indicates success; otherwise <see langword="false"/>.</returns>
public static bool IsSuccess(this MxStatusProxy status) public static bool IsSuccess(this MxStatusProxy status)
{ {
ArgumentNullException.ThrowIfNull(status); ArgumentNullException.ThrowIfNull(status);
return status.Success != 0 return status.Category is MxStatusCategory.Ok;
&& status.Category is MxStatusCategory.Ok;
} }
/// <summary>Returns a formatted summary of the status for diagnostic output.</summary> /// <summary>Returns a formatted summary of the status for diagnostic output.</summary>
+6
View File
@@ -94,6 +94,12 @@ goroutine cleanup. Raw protobuf messages remain available through the
`errors.As` for `GatewayError`, `CommandError`, and `MxAccessError`; command `errors.As` for `GatewayError`, `CommandError`, and `MxAccessError`; command
errors preserve the raw reply. 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 ### Reconnect-replay gap
Each `EventResult` carries exactly one of `Event`, `ReplayGap`, or `Err`. When Each `EventResult` carries exactly one of `Event`, `ReplayGap`, or `Err`. When
+2 -1
View File
@@ -65,7 +65,8 @@ func TestStatusConversionFixtures(t *testing.T) {
if err := protojson.Unmarshal(tc.Status, &status); err != nil { if err := protojson.Unmarshal(tc.Status, &status); err != nil {
t.Fatalf("parse status: %v", err) 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) t.Fatalf("StatusSucceeded() = %v, want %v", got, want)
} }
}) })
+5 -1
View File
@@ -180,11 +180,15 @@ func EnsureProtocolSuccess(op string, status *ProtocolStatus, reply *MxCommandRe
// EnsureMxAccessSuccess returns a typed MxAccessError for failing HRESULTs or // EnsureMxAccessSuccess returns a typed MxAccessError for failing HRESULTs or
// MXSTATUS_PROXY entries. // 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 { func EnsureMxAccessSuccess(op string, reply *MxCommandReply) error {
if reply == nil { if reply == nil {
return nil return nil
} }
if reply.Hresult != nil && reply.GetHresult() != 0 { if reply.Hresult != nil && reply.GetHresult() < 0 {
return &MxAccessError{Reply: reply} return &MxAccessError{Reply: reply}
} }
for _, status := range reply.GetStatuses() { for _, status := range reply.GetStatuses() {
@@ -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) { func TestOpenSessionFixtureProtocolVersions(t *testing.T) {
data, err := os.ReadFile(filepath.Join("..", "..", "proto", "fixtures", "golden", "open-session-reply.ok.json")) data, err := os.ReadFile(filepath.Join("..", "..", "proto", "fixtures", "golden", "open-session-reply.ok.json"))
if err != nil { if err != nil {
+12 -1
View File
@@ -1,6 +1,17 @@
package mxgateway package mxgateway
import (
pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated"
)
// StatusSucceeded reports whether an MXSTATUS_PROXY entry represents success. // 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 { func StatusSucceeded(status *MxStatusProxy) bool {
return status == nil || status.GetSuccess() != 0 return status == nil || status.GetCategory() == pb.MxStatusCategory_MX_STATUS_CATEGORY_OK
} }
+6 -1
View File
@@ -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 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 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 **Secret redaction.** Credentials passed to `authenticateUser` (and the
credential-sensitive values passed to `writeSecured`/`writeSecured2`) travel credential-sensitive values passed to `writeSecured`/`writeSecured2`) travel
@@ -47,7 +47,9 @@ final class MxGatewayErrors {
if (reply == null) { if (reply == null) {
return; 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); throw new MxAccessException(operation, reply);
} }
for (var status : reply.getStatusesList()) { for (var status : reply.getStatusesList()) {
@@ -8,8 +8,11 @@ import mxaccess_gateway.v1.MxaccessGateway.MxStatusSource;
* Helpers for inspecting {@link MxStatusProxy} values returned by the gateway. * Helpers for inspecting {@link MxStatusProxy} values returned by the gateway.
* *
* <p>An {@code MxStatusProxy} mirrors the MXAccess COM {@code MXSTATUS_PROXY} * <p>An {@code MxStatusProxy} mirrors the MXAccess COM {@code MXSTATUS_PROXY}
* struct. The success flag uses the MXAccess convention where any non-zero * struct. Per the wire contract, {@code category} is the authoritative verdict:
* value indicates success. * 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 { public final class MxStatuses {
private MxStatuses() { private MxStatuses() {
@@ -18,12 +21,17 @@ public final class MxStatuses {
/** /**
* Returns whether the supplied status proxy reports success. * Returns whether the supplied status proxy reports success.
* *
* <p>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} * @param status the status proxy, may be {@code null}
* @return {@code true} if {@code status} is {@code null} or its success * @return {@code true} if {@code status} is {@code null} or its category is
* flag is non-zero, {@code false} otherwise * {@code MX_STATUS_CATEGORY_OK}, {@code false} otherwise
*/ */
public static boolean succeeded(MxStatusProxy status) { 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) { 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() { public int success() {
return raw.getSuccess(); return raw.getSuccess();
@@ -701,14 +701,17 @@ final class MxGatewayClientSessionTests {
.setSessionId(request.getSessionId()) .setSessionId(request.getSessionId())
.setKind(request.getCommand().getKind()) .setKind(request.getCommand().getKind())
.setProtocolStatus(ok()); .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) { if (request.getCommand().getKind() == MxCommandKind.MX_COMMAND_KIND_SUSPEND) {
reply.setSuspend(mxaccess_gateway.v1.MxaccessGateway.SuspendReply.newBuilder() reply.setSuspend(mxaccess_gateway.v1.MxaccessGateway.SuspendReply.newBuilder()
.setStatus(mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy.newBuilder() .setStatus(okStatus));
.setSuccess(1)));
} else if (request.getCommand().getKind() == MxCommandKind.MX_COMMAND_KIND_ACTIVATE) { } else if (request.getCommand().getKind() == MxCommandKind.MX_COMMAND_KIND_ACTIVATE) {
reply.setActivate(mxaccess_gateway.v1.MxaccessGateway.ActivateReply.newBuilder() reply.setActivate(mxaccess_gateway.v1.MxaccessGateway.ActivateReply.newBuilder()
.setStatus(mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy.newBuilder() .setStatus(okStatus));
.setSuccess(1)));
} }
responseObserver.onNext(reply.build()); responseObserver.onNext(reply.build());
responseObserver.onCompleted(); responseObserver.onCompleted();
@@ -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.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertInstanceOf; 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 static org.junit.jupiter.api.Assertions.assertTrue;
import com.google.gson.JsonArray; 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.MxValue;
import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatusCode; import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatusCode;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
final class MxGatewayFixtureTests { final class MxGatewayFixtureTests {
@Test @Test
@@ -89,6 +92,50 @@ final class MxGatewayFixtureTests {
throw new AssertionError("expected MxAccessException"); 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 @Test
void grpcAuthErrorsAreClassifiedAndRedacted() { void grpcAuthErrorsAreClassifiedAndRedacted() {
RuntimeException authError = MxGatewayErrors.fromGrpc( RuntimeException authError = MxGatewayErrors.fromGrpc(
@@ -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."
}
@@ -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."
}
@@ -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."
}
@@ -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."
}
@@ -20,6 +20,34 @@
"path": "command-replies/write.mxaccess-failure.reply.json", "path": "command-replies/write.mxaccess-failure.reply.json",
"expectation": "MXAccess failures are data-bearing replies with HRESULT and status details, not transport failures." "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", "id": "event-stream.session-ordered",
"category": "event_streams", "category": "event_streams",
@@ -137,8 +137,10 @@ def ensure_mxaccess_success(operation: str, reply: pb.MxCommandReply) -> pb.MxCo
raw_reply=reply, 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: for mx_status in reply.statuses:
if mx_status.success == 0: if mx_status.category != pb.MX_STATUS_CATEGORY_OK:
raise MxAccessError( raise MxAccessError(
_mxaccess_message(operation, reply), _mxaccess_message(operation, reply),
protocol_status=status, protocol_status=status,
+49
View File
@@ -32,6 +32,55 @@ def test_write_failure_fixture_preserves_raw_reply() -> None:
assert len(captured.value.raw_reply.statuses) == 2 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: def test_session_status_maps_to_session_error() -> None:
status = pb.ProtocolStatus( status = pb.ProtocolStatus(
code=pb.PROTOCOL_STATUS_CODE_SESSION_NOT_FOUND, code=pb.PROTOCOL_STATUS_CODE_SESSION_NOT_FOUND,
+17 -8
View File
@@ -308,10 +308,12 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result<MxCommandReply, E
/// This is the second reply check applied to the typed command path, after /// This is the second reply check applied to the typed command path, after
/// [`ensure_command_success`] confirms the protocol envelope is `Ok`. It /// [`ensure_command_success`] confirms the protocol envelope is `Ok`. It
/// enforces MXAccess parity: a reply can carry an `Ok` protocol envelope while /// enforces MXAccess parity: a reply can carry an `Ok` protocol envelope while
/// MXAccess itself rejected the operation. Following COM semantics (and the /// MXAccess itself rejected the operation. Following COM semantics, only a
/// Python client), only a **negative** `hresult` is a failure — positive codes /// **negative** `hresult` is a failure — positive codes such as `S_FALSE = 1`
/// such as `S_FALSE = 1` are success. A `MXSTATUS_PROXY` entry is treated as a /// are success. A `MXSTATUS_PROXY` entry is treated as a failure when its
/// failure when its `success` member is `0`. /// `category` is not [`MxStatusCategory::Ok`]; the `success` member mirrors the
/// raw COM value verbatim for diagnostics and never enters the verdict, so an
/// entry with an unspecified category fails even when `success` is non-zero.
/// ///
/// Per-item bulk failures are reported inside each result entry /// Per-item bulk failures are reported inside each result entry
/// (`was_successful = false`) rather than in the top-level `hresult`/`statuses` /// (`was_successful = false`) rather than in the top-level `hresult`/`statuses`
@@ -320,10 +322,14 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result<MxCommandReply, E
/// # Errors /// # Errors
/// ///
/// Returns [`Error::MxAccess`] when `reply.hresult` is negative or any /// Returns [`Error::MxAccess`] when `reply.hresult` is negative or any
/// `reply.statuses` entry reports a non-success `success` member. /// `reply.statuses` entry reports a category other than
/// [`MxStatusCategory::Ok`].
pub fn ensure_mxaccess_success(reply: MxCommandReply) -> Result<MxCommandReply, Error> { pub fn ensure_mxaccess_success(reply: MxCommandReply) -> Result<MxCommandReply, Error> {
let hresult_failure = reply.hresult.is_some_and(|hresult| hresult < 0); 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 { if hresult_failure || status_failure {
Err(Box::new(MxAccessError::new(reply)).into()) Err(Box::new(MxAccessError::new(reply)).into())
@@ -412,8 +418,10 @@ mod tests {
let mut reply = ok_reply(); let mut reply = ok_reply();
// Positive hresult (e.g. S_FALSE = 1) is a success, not a failure. // Positive hresult (e.g. S_FALSE = 1) is a success, not a failure.
reply.hresult = Some(1); 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 { reply.statuses = vec![MxStatusProxy {
success: 1, success: 0,
category: MxStatusCategory::Ok as i32, category: MxStatusCategory::Ok as i32,
..MxStatusProxy::default() ..MxStatusProxy::default()
}]; }];
@@ -424,8 +432,9 @@ mod tests {
#[test] #[test]
fn ensure_mxaccess_success_flags_failing_status_entry() { fn ensure_mxaccess_success_flags_failing_status_entry() {
let mut reply = ok_reply(); let mut reply = ok_reply();
// A non-OK category fails even though the raw `success` member is set.
reply.statuses = vec![MxStatusProxy { reply.statuses = vec![MxStatusProxy {
success: 0, success: 1,
category: MxStatusCategory::CommunicationError as i32, category: MxStatusCategory::CommunicationError as i32,
detail: 42, detail: 42,
diagnostic_text: "write rejected for mxgw_visible_secret".to_owned(), diagnostic_text: "write rejected for mxgw_visible_secret".to_owned(),
+5 -1
View File
@@ -282,7 +282,11 @@ impl MxStatus {
&self.raw &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 { pub fn success(&self) -> i32 {
self.raw.success self.raw.success
} }
+98
View File
@@ -14,6 +14,7 @@ use tokio::sync::{mpsc, Mutex};
use tokio_stream::wrappers::{ReceiverStream, TcpListenerStream}; use tokio_stream::wrappers::{ReceiverStream, TcpListenerStream};
use tonic::transport::Server; use tonic::transport::Server;
use tonic::{Request, Response, Status}; 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::{ use zb_mom_ww_mxgateway_client::generated::mxaccess_gateway::v1::mx_access_gateway_server::{
MxAccessGateway, MxAccessGatewayServer, MxAccessGateway, MxAccessGatewayServer,
}; };
@@ -337,6 +338,57 @@ fn authentication_and_authorization_statuses_are_distinct_and_redacted() {
assert!(!auth.to_string().contains("visible_secret")); 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] #[test]
fn command_error_display_keeps_raw_reply_accessible() { fn command_error_display_keeps_raw_reply_accessible() {
let reply = mxaccess_failure_reply(); 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 { fn behavior_fixture(path: &str) -> Value {
let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) let path = std::path::Path::new(env!("CARGO_MANIFEST_DIR"))
.join("../proto/fixtures/behavior") .join("../proto/fixtures/behavior")
+26
View File
@@ -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 the worker captured HRESULT or `MXSTATUS_PROXY` details. Client wrappers should
map those replies to rich command errors without discarding the raw reply. 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 Streams
Event stream fixtures live in Event stream fixtures live in
+8 -1
View File
@@ -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 `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 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 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 failing before a prior `AuthenticateUser` + `AdviseSupervisory` surfaces the
native failure unchanged — the helper does not pre-validate or reorder it. native failure unchanged — the helper does not pre-validate or reorder it.