Joseph Doherty
0d874f91ee
Code-review follow-up on the CLI-40/41/44 branch. ISSUE 1 (all five, critical): the message-only scrub still leaked the server-echoed credential through the redacted error's structured reply accessor (.NET Reply/Statuses, Java reply()/protocolStatus(), Go MxAccessError.Reply via errors.As, Rust reply()/into_reply(), Python raw_reply). The redacted error now carries a scrubbed clone of the reply (protocol_status.message, diagnostic_message, statuses[].diagnostic_text), with per-language tests asserting the reply accessor no longer contains the credential. ISSUE 2 (Rust, critical): ensure_command_success routed MXACCESS_FAILURE to Error::Command (unlike the other four clients), bypassing attach_secrets and leaking via derived Debug/Display. MXACCESS_FAILURE now routes to Error::MxAccess, fixing the cross-client inconsistency. ISSUE 3 (Go, important): the CLI-44 terminal send was unconditionally non-blocking, dropping a genuine terminal error under a full buffer on the never-drop SubscribeEvents path. It is now reserved-slot-non-blocking only for the cancel-on-overflow path and blocking for the never-drop path. New shared fixture authenticate-user.echoed-credential-mxaccess-failure.reply.json wired into all five suites. Minors: whitespace-secret guard on .NET/Java redact helpers; Java preserves exception subtype on redaction; redaction-helper unit tests (Go/Java/.NET). Docs (ClientBehaviorFixtures.md, ClientLibrariesDesign.md) updated to make the structured-field claim true.
173 lines
8.4 KiB
Markdown
173 lines
8.4 KiB
Markdown
# Client Behavior Fixtures
|
|
|
|
Client behavior fixtures define the shared expectations used by the official
|
|
.NET, Go, Rust, Python, and Java clients. They keep wrapper behavior aligned
|
|
while each language exposes idiomatic APIs over the same protobuf contract.
|
|
|
|
## Fixture Set
|
|
|
|
The fixture manifest is `clients/proto/fixtures/behavior/manifest.json`.
|
|
`clients/proto/proto-inputs.json` references the fixture root through
|
|
`behaviorFixtureRoot` so generators and client test projects can discover the
|
|
same files they use for descriptor inputs.
|
|
|
|
The fixture set contains:
|
|
|
|
- command reply protobuf JSON,
|
|
- ordered event stream protobuf JSON samples,
|
|
- `MxValue` conversion case sets,
|
|
- `MxStatusProxy` conversion case sets,
|
|
- authentication and authorization error expectations,
|
|
- timeout and cancellation behavior expectations.
|
|
|
|
Protobuf message fixtures use protobuf JSON field names and enum values. Files
|
|
that describe client wrapper behavior use explicit JSON fields instead of a
|
|
proto message because those expectations apply above the generated transport
|
|
types.
|
|
|
|
## Command Replies
|
|
|
|
Command reply fixtures live in
|
|
`clients/proto/fixtures/behavior/command-replies/`. They parse as
|
|
`mxaccess_gateway.v1.MxCommandReply`.
|
|
|
|
Clients use these fixtures to verify that successful and failed MXAccess
|
|
commands both carry the full reply details:
|
|
|
|
- `protocolStatus`,
|
|
- `hresult`,
|
|
- `returnValue`,
|
|
- repeated `statuses`,
|
|
- method-specific reply payloads when MXAccess returns out parameters.
|
|
|
|
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.
|
|
|
|
### Malformed-Reply And Credential-Redaction Conformance
|
|
|
|
Three further command reply fixtures pin the id/handle-extraction and
|
|
credential-redaction contracts for the credential-bearing helpers:
|
|
|
|
| Fixture | Reply | Expected behavior |
|
|
|---|---|---|
|
|
| `authenticate-user.echoed-credential.reply.json` | OK envelope, negative `hresult`, and the caller's credential echoed into `protocolStatus.message`, `statuses[0].diagnosticText`, and `diagnosticMessage` | the surfaced error redacts the exact secret from **both** the rendered message and the structured reply accessors (never leaks the verbatim value) |
|
|
| `authenticate-user.echoed-credential-mxaccess-failure.reply.json` | the same echo, but coded `PROTOCOL_STATUS_CODE_MXACCESS_FAILURE` | identical redaction; confirms every client routes the MXAccess-failure protocol code to its MXAccess error type and scrubs it |
|
|
| `authenticate-user.missing-payload.reply.json` | OK envelope, no `AuthenticateUser` payload, no `return_value` | a typed malformed-reply error, never a proto3 default `0` and never an NRE |
|
|
| `authenticate-user.return-value-only.reply.json` | OK envelope, `return_value.int32_value = 7`, no typed payload | the id resolves to `7` via the legacy `return_value` compatibility path |
|
|
|
|
The rules those fixtures lock in are:
|
|
|
|
- **Malformed-reply extraction (CLI-41).** Every helper that extracts a scalar
|
|
id/handle (`AuthenticateUser`, `ArchestrAUserToId`, `AddBufferedItem`, and the
|
|
handle extractors) prefers the typed payload; when it is absent it falls back
|
|
to `return_value` **only** when `return_value` is present with the expected
|
|
int32 variant; when neither is present it raises a typed malformed-reply error.
|
|
It never surfaces a proto3 default `0` and never throws a null-reference.
|
|
- **Credential redaction (CLI-40).** The credential-bearing helpers
|
|
(`AuthenticateUser`, `WriteSecured`/`WriteSecured2`) scrub the exact secret
|
|
values they were called with from any surfaced error — both the rendered
|
|
message text **and** the structured reply the error still exposes (a
|
|
server-echoed credential lives in `protocolStatus.message` and
|
|
`statuses[].diagnosticText`, which the error's raw-reply accessor would
|
|
otherwise re-expose to a logger dumping structured fields). The redacted error
|
|
therefore carries a scrubbed clone of the reply. This is defense-in-depth on
|
|
top of the by-construction guarantee that exceptions carry reply-derived text,
|
|
not the request. The marker is `<redacted>` in the Go, Rust, and Java clients
|
|
and `[redacted]` in the Python client and the .NET CLI; each suite asserts that
|
|
neither the surfaced message nor the exposed reply still contains the
|
|
credential, and that the message contains the client's marker.
|
|
|
|
## Event Streams
|
|
|
|
Event stream fixtures live in
|
|
`clients/proto/fixtures/behavior/event-streams/`. Each file contains an ordered
|
|
`events` array whose entries parse as `mxaccess_gateway.v1.MxEvent`.
|
|
|
|
Clients use these fixtures to verify that stream helpers preserve
|
|
`workerSequence` order and expose each native event family:
|
|
|
|
- `OnDataChange`,
|
|
- `OnWriteComplete`,
|
|
- `OperationComplete`,
|
|
- `OnBufferedDataChange`.
|
|
|
|
Wrappers must not reorder, coalesce, or drop events while reading the fixture.
|
|
|
|
## Value And Status Conversion
|
|
|
|
Value fixtures live in `clients/proto/fixtures/behavior/values/`. Each case
|
|
contains a `value` object that parses as `mxaccess_gateway.v1.MxValue`.
|
|
|
|
Status fixtures live in `clients/proto/fixtures/behavior/statuses/`. Each case
|
|
contains a `status` object that parses as
|
|
`mxaccess_gateway.v1.MxStatusProxy`.
|
|
|
|
Clients use these fixtures to verify typed projections and raw fallback
|
|
behavior. A language helper may expose native booleans, integers, strings,
|
|
arrays, and timestamps, but it must keep `rawDiagnostic`, raw data type fields,
|
|
and raw byte payloads accessible when conversion is incomplete.
|
|
|
|
Each status case also carries an independent `wantSuccess` boolean alongside its
|
|
`status` object. The success/failure conformance tests assert the helper's
|
|
verdict against this fixture-declared expectation rather than recomputing it from
|
|
`category` (the same formula under test), so a regression in the verdict rule
|
|
cannot hide behind a self-consistent computation.
|
|
|
|
## Auth, Timeout, And Cancel Behavior
|
|
|
|
Authentication fixtures live in `clients/proto/fixtures/behavior/auth/`. They
|
|
separate `UNAUTHENTICATED` from `PERMISSION_DENIED` so clients map missing or
|
|
invalid credentials differently from missing scopes. Expected output strings
|
|
contain only redacted credentials.
|
|
|
|
Timeout and cancellation fixtures live in
|
|
`clients/proto/fixtures/behavior/timeout-cancel/`. They document that canceling
|
|
or timing out a client call stops the client from waiting, but it does not abort
|
|
an in-flight MXAccess COM call on the worker STA. Clients should follow up with
|
|
`GetSessionState` or `CloseSession` before reusing handles after an uncertain
|
|
command timeout.
|
|
|
|
## Validation
|
|
|
|
Run the fixture validation tests after changing the behavior fixture set:
|
|
|
|
```bash
|
|
powershell -ExecutionPolicy Bypass -File scripts/validate-client-behavior-fixtures.ps1
|
|
```
|
|
|
|
The script runs the focused C# contract tests that parse all protobuf JSON
|
|
fixtures and validate deterministic wrapper expectation files.
|
|
|
|
## Related Documentation
|
|
|
|
- [Client Proto Generation](./ClientProtoGeneration.md)
|
|
- [Client Libraries Detailed Design](./ClientLibrariesDesign.md)
|
|
- [Protobuf Contracts](./Contracts.md)
|