Files
mxaccessgw/code-reviews/Client.Python/findings.md
T
Joseph Doherty d2d2e5f68f code-review 2026-05-24: re-review at d692232 across all 11 modules
Restores the `code-reviews/` tree (was unwritten on this working copy)
and re-reviews every module per `REVIEW-PROCESS.md` against HEAD
`d692232`. The diff in scope is the five commits since the last sweep:
`dc9c0c9` (ZB.MOM.WW gateway-side rename + slnx migrate),
`397d3c5` (client SDK rename + the missing alarm-RPC proto types and
the .NET DiscoverHierarchyOptions POCO), `27ed651` (role-based LDAP
auth + HubToken bearer, drop PathBase), `6594359` (sidebar layout +
three SignalR push hubs), and `d692232` (EventsHub publisher + doc
refresh).

Module status

| Module | Open | Total | Delta this pass |
|---|---|---|---|
| Server           | 8 | 43 | +6 |
| Contracts        | 2 | 17 | +2 |
| Tests            | 2 | 26 | +2 |
| IntegrationTests | 3 | 24 | +3 |
| Client.Java      | 5 | 31 | +5 |
| Client.Rust      | 1 | 21 | +1 |
| Worker           | 0 | 25 |  0 (rename-only diff, clean) |
| Worker.Tests     | 0 | 30 |  0 (rename-only diff, clean) |
| Client.Dotnet    | 0 | 17 |  0 (rename + alarm-fix diff, clean) |
| Client.Python    | 0 | 21 |  0 (rename + alarm-fix diff, clean) |
| Client.Go        | 0 | 21 |  0 (rename + alarm-fix diff, clean) |

Total new findings: 19. Severity breakdown: 1 Medium-security
(Server-038), 4 Medium-documentation/coverage, 14 Low.

New findings

  * Server-038 (Medium / Security) — EventsHub.SubscribeSession accepts
    any session id from any Viewer; no per-session ACL guards the
    EventsHub group fan-out.
  * Server-039 (Low / Error handling) — HubTokenService.Validate
    accepts a payload with null Name/NameIdentifier.
  * Server-040 (Low / Conventions) — MapGroupsToRoles undocumented
    full-vs-RDN lookup precedence.
  * Server-041 (Low / Design adherence) — EventStreamService calls
    IDashboardEventBroadcaster.Publish without a try/catch — fragile
    seam relying on the never-throw contract.
  * Server-042 (Low / Performance) — DashboardSnapshotPublisher tight
    retry loop with no backoff (vs AlarmsHubPublisher 5s delay).
  * Server-043 (Low / Documentation) — HubTokenService singleton
    sharing across login + hub-token validation undocumented.

  * Contracts-016 (Low / Conventions) — QueryActiveAlarmsRequest.session_id
    reserved-for-future-use ambiguity.
  * Contracts-017 (Low / Documentation) — rpc QueryActiveAlarms doc
    omits the alarm_filter_prefix filter description.

  * Tests-025 (Low / Conventions) — duplicate NullDashboardEventBroadcaster
    fakes in EventStreamServiceTests and GatewayEndToEndFakeWorkerSmokeTests.
  * Tests-026 (Medium / Testing coverage) — no test proves
    EventStreamService actually calls IDashboardEventBroadcaster.Publish.

  * IntegrationTests-022 (Low / Conventions) — ResolveRepositoryRoot
    silent fallback to Directory.GetCurrentDirectory().
  * IntegrationTests-023 (Low / Testing coverage) — DashboardLdapLiveTests
    success-path asserts ldap_group but not the Role claim.
  * IntegrationTests-024 (Low / Conventions) — inline
    NullDashboardEventBroadcaster fake duplicates Tests-side copies.

  * Client.Java-027 (Medium / Documentation) — README + JavaClientDesign
    Gradle task names still use the old short project names.
  * Client.Java-028 (Medium / Design adherence) — JavaClientDesign
    build-layout shows the old `com/dohertylan/mxgateway/` package paths.
  * Client.Java-029 (Low / Documentation) — README installDist path
    cites the wrong directory.
  * Client.Java-030 (Low / Testing coverage) — no Java test exercises
    the regenerated QueryActiveAlarmsRequest RPC.
  * Client.Java-031 (Low / Conventions) — README prose uses old short
    project names instead of canonical prefixed ones.

  * Client.Rust-021 (Low / Design adherence) — RustClientDesign.md
    "Crate layout" shows an aspirational nested `crates/zb-mom-ww-mxgateway-client/`
    that does not exist; actual layout is the flat top-level crate.

Two pre-existing pending findings (Server-031 lock-contention,
Server-032 bounded event channel) remain unchanged — neither was
touched by this wave of commits.

Process notes

- The `code-reviews/` tree was not in this working copy's git
  history (the local extract pre-dates the divergent branch that
  carried the reviews). Restored from `dd7ca16` via
  `git checkout dd7ca16 -- code-reviews/` before the re-review.
- Some "Resolved" entries in the restored findings.md reference
  fixes that landed on the divergent branch (the same one that
  carried the reviews) and are not present on the current main
  lineage. The re-review treats those statuses as historical;
  the new pass only files findings against HEAD's actual state.
- `python code-reviews/regen-readme.py --check` is green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-24 02:34:30 -04:00

798 lines
52 KiB
Markdown

# Code Review — Client.Python
| Field | Value |
|---|---|
| Module | `clients/python` |
| Reviewer | Claude Code |
| Review date | 2026-05-24 |
| Commit reviewed | `d692232` |
| Status | Reviewed |
| Open findings | 0 |
## Checklist coverage
A re-review at commit `a020350` over the same module. Prior findings
(Client.Python-001 — Client.Python-017) remain closed and are kept as
history. This section reflects categories evaluated in this pass.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No new issues found — TLS-by-default fix in Client.Python-013 verified; no test fixture accidentally relies on plaintext defaults. |
| 2 | mxaccessgw conventions | No new issues found — secrets redacted, MXAccess parity preserved, generated code untouched. |
| 3 | Concurrency & thread safety | No new issues found — close-idempotency and shared cancel-on-cancel iterator still in place. |
| 4 | Error handling & resilience | No new issues found. |
| 5 | Security | No new issues found — `_use_plaintext` now requires explicit `--plaintext` opt-in (Client.Python-013 resolution verified). The `--api-key` flag is also still redacted from the option repr and CLI errors. |
| 6 | Performance & resource management | No new issues found. |
| 7 | Design-document adherence | No new issues found — `PythonClientDesign.md` is consistent with the implemented surface. |
| 8 | Code organization & conventions | Issue found: `mxgateway_cli` is shipped in the wheel but has no PEP 561 `py.typed` marker (Client.Python-019), so the CLI module's inline type hints are invisible to downstream `mypy` runs. |
| 9 | Testing coverage | Issue found: no test exercises the wheel-build / editable-install flow; the broken `pyproject.toml` (Client.Python-018) was not caught at commit time because the test suite runs from `src/` via `pytest pythonpath` (Client.Python-020). |
| 10 | Documentation & comments | Issue found: cross-client CLI parity gap — the Python CLI ships none of the Galaxy subcommands (`galaxy-test-connection`, `galaxy-last-deploy`, `galaxy-discover`, `galaxy-watch`) the .NET / Go / Rust / Java CLIs all expose, and lacks the new `.NET`-only `bench-stream-events`. README does not flag the gap (Client.Python-021). |
### 2026-05-24 review (commit d692232)
Re-review pass at `d692232`. Diff against `a020350` is commit `397d3c5`:
package directories renamed (`src/mxgateway``src/zb_mom_ww_mxgateway`,
`src/mxgateway_cli``src/zb_mom_ww_mxgateway_cli`), distribution name
changed to `zb-mom-ww-mxaccess-gateway-client`, console-script
`mxgw-py` retained, every `from mxgateway` / `import mxgateway` updated.
A first-pass case-insensitive regex sweep corrupted the binary descriptor
bytes in the generated `_pb2.py` files; the fix was to restore the
original `_pb2.py` artifacts from the pre-rename directory before
deleting it, so the csharp_namespace bytes still carry the old string —
this is documented as wire-level metadata not used by Python at runtime.
Hostname / cert / temp-dir example identifiers (`mxgateway.example.local`,
`mxgateway-ca.pem`, `mxgateway-python-wheel`) were intentionally preserved.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | No issues found in the a020350..d692232 diff. |
| 2 | mxaccessgw conventions | No issues found — wire identifiers preserved. |
| 3 | Concurrency & thread safety | No issues found in this diff. |
| 4 | Error handling & resilience | No issues found in this diff. |
| 5 | Security | No issues found in this diff. |
| 6 | Performance & resource management | No issues found in this diff. |
| 7 | Design-document adherence | No issues found — `PythonClientDesign.md` reflects new paths. |
| 8 | Code organization & conventions | No issues found in this diff. |
| 9 | Testing coverage | No issues found in this diff — alarm test fixtures correctly drop retired `session_id` from `AcknowledgeAlarmRequest` while retaining it on `QueryActiveAlarmsRequest`. |
| 10 | Documentation & comments | No issues found in this diff. |
## Findings
### Client.Python-001
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `clients/python/pyproject.toml:8,25`, `clients/python/src/mxgateway_cli/commands.py:25` |
| Status | Resolved |
**Description:** The package `description` in `pyproject.toml` still says "Async Python client *scaffold*" even though the client is fully implemented. Stale "scaffold" wording misrepresents maturity to anyone reading PyPI metadata. (The `mxgw-py` console-script name is itself consistent between `pyproject.toml` and the README.)
**Recommendation:** Update the `pyproject.toml` description to drop "scaffold"; keep README CLI examples in sync with the actual `mxgw-py` entry point.
**Resolution:** 2026-05-18 — Confirmed: `pyproject.toml:8` `description` read "Async Python client scaffold for MXAccess Gateway." Changed to "Async Python client for MXAccess Gateway." The `mxgw-py` console-script name was already consistent with the README, so no README change was needed. Pure metadata fix — no test required.
### Client.Python-002
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `clients/python/src/mxgateway/__init__.py:27` |
| Status | Resolved |
**Description:** `MxGatewayCommandError` is imported into `__init__.py` and is a documented public exception, but it is missing from `__all__`. It is the parent of `MxAccessError` and a meaningful catch target, so omitting it from the public surface is inconsistent — `from mxgateway import *` will not expose it and tooling that respects `__all__` treats it as private.
**Recommendation:** Add `"MxGatewayCommandError"` to the `__all__` list.
**Resolution:** 2026-05-18 — Re-triaged: this finding is stale against the reviewed source. `clients/python/src/mxgateway/__init__.py` already imports `MxGatewayCommandError` (line 16) **and** lists `"MxGatewayCommandError"` in `__all__` (line 38). `from mxgateway import *` exposes it correctly. Verified at runtime (`'MxGatewayCommandError' in mxgateway.__all__` is `True`). No source change required — the defect described no longer exists.
### Client.Python-003
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `clients/python/src/mxgateway/client.py:125-137,155-173` |
| Status | Resolved |
**Description:** `stream_events_raw` and `query_active_alarms` call the stub directly with a `timeout` kwarg when `stream_timeout` is set, with no `TypeError` fallback. `galaxy.py:watch_deploy_events` and `_unary` *do* have a fallback that strips `timeout` if the callable rejects it. This asymmetry means a fake/older stub that does not accept `timeout` crashes for gateway streams but not Galaxy streams. It is only masked today because `stream_timeout` defaults to `None`.
**Recommendation:** Apply the same `try/except TypeError` timeout-fallback pattern to `stream_events_raw` and `query_active_alarms`, or remove the fallback everywhere and standardise on a single behaviour.
**Resolution:** 2026-05-18 — Confirmed: both stream methods in `client.py` called the stub with `timeout` unconditionally and had no `TypeError` fallback, unlike `_unary` and `galaxy.watch_deploy_events`. Added a shared `_open_stream` helper in `client.py` that opens a server-streaming call and strips the `timeout` kwarg when the stub raises `TypeError: ... unexpected keyword argument 'timeout'`, then routed both `stream_events_raw` and `query_active_alarms` through it. Regression tests in `tests/test_stream_timeout_fallback.py` (`test_stream_events_raw_falls_back_when_stub_rejects_timeout`, `test_query_active_alarms_falls_back_when_stub_rejects_timeout`, `test_stream_events_raw_still_passes_timeout_to_capable_stub`) failed before the fix and pass after. No public behaviour change for real gRPC stubs, so no README update needed.
### Client.Python-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `clients/python/src/mxgateway_cli/commands.py:386,402-404` |
| Status | Resolved |
**Description:** In `_smoke`, the local variable `closed` is set to `False` and never reassigned; the `finally` block's `if not closed:` is therefore always true. This is dead/misleading code suggesting a removed early-close path.
**Recommendation:** Remove the `closed` variable and the `if not closed:` guard; call `await session.close()` directly in the `finally` block (or use `async with session:`).
**Resolution:** 2026-05-18 — Confirmed: `closed = False` was set and never reassigned, making `if not closed:` dead code. Replaced the `try/finally` with `async with session:` so the session is closed via the documented async context manager — `Session` already implements `__aexit__``close()`. Behaviour is unchanged (the session is still closed on every exit path); no test needed for the dead-code removal — exercised by the existing CLI smoke test.
### Client.Python-005
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Performance & resource management |
| Location | `clients/python/src/mxgateway/galaxy.py:117-140` |
| Status | Resolved |
**Description:** `discover_hierarchy` pages through the entire Galaxy object hierarchy and accumulates every `GalaxyObject` (each carrying its full attribute list) into a single in-memory `list` before returning. For a large Galaxy this is a very large allocation with no streaming alternative and no caller-side bound.
**Recommendation:** Offer an async-generator variant (e.g. `iter_hierarchy()`) that yields objects/pages as they arrive, keeping `discover_hierarchy()` as a convenience wrapper. At minimum document the memory characteristic.
**Resolution:** 2026-05-18 — Confirmed: `discover_hierarchy` buffered the entire hierarchy with no streaming alternative. Added `GalaxyRepositoryClient.iter_hierarchy`, an async generator that fetches one `DiscoverHierarchyRequest` page at a time and yields each `GalaxyObject` as it arrives, so peak memory is bounded by a single page (`_DISCOVER_HIERARCHY_PAGE_SIZE`). Pages are fetched lazily — the next page is only requested after the current page is fully consumed. `discover_hierarchy` is now a thin convenience wrapper (`[obj async for obj in self.iter_hierarchy()]`) that preserves its `list[GalaxyObject]` contract, including the repeated-page-token guard. Regression tests in `tests/test_galaxy_iter_hierarchy.py` (`test_iter_hierarchy_yields_objects_across_pages`, `test_iter_hierarchy_is_lazy_and_does_not_prefetch_next_page`, `test_iter_hierarchy_rejects_repeated_page_token`, `test_discover_hierarchy_still_returns_full_list`) failed before the fix and pass after. `clients/python/README.md` updated with the `iter_hierarchy` usage and memory guidance since this adds a new public method.
### Client.Python-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `clients/python/src/mxgateway/client.py:74-82`, `clients/python/src/mxgateway/galaxy.py:85-93`, `clients/python/src/mxgateway/session.py:38-55` |
| Status | Resolved |
**Description:** `close()` on the clients and `Session.close()` use a plain `self._closed` check-then-set with an `await` between, with no lock. If two coroutines call `close()` concurrently both can pass the guard before either sets it, causing a double `channel.close()` / double `CloseSession` RPC. Single-task usage is the documented contract, so impact is low, but the idempotency guarantee asserted in docstrings only holds for sequential calls.
**Recommendation:** Set `self._closed = True` before the `await`, or guard with an `asyncio.Lock`, so the idempotency claim holds under concurrent close.
**Resolution:** 2026-05-18 — Confirmed the check-then-set window. Fixed `GatewayClient.close`, `GalaxyRepositoryClient.close`, and `Session.close` to set `self._closed = True` *before* the `await` (channel close / `CloseSession` RPC). A second coroutine entering `close()` while the first is still awaiting now hits the early-return guard and does not issue a second `channel.close()` / `CloseSession`. Docstrings updated to state the idempotency holds under concurrent calls. TDD: regression tests in `tests/test_low_severity_findings.py` (`test_gateway_client_concurrent_close_closes_channel_once`, `test_galaxy_client_concurrent_close_closes_channel_once`, `test_session_concurrent_close_sends_one_close_session_rpc`) — each uses a fake channel/client that stalls inside `close`/`close_session_raw` so two concurrent `close()` calls interleave at the exact race window; they failed before the fix and pass after.
### Client.Python-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `clients/python/src/mxgateway/client.py:204-213` |
| Status | Resolved |
**Description:** `_canceling_iterator` (gateway event stream) does not catch `asyncio.CancelledError` to invoke `call.cancel()` explicitly — it relies on the `finally` block. `galaxy.py:_canceling_iterator` *does* explicitly catch `CancelledError`, cancel, and re-raise. The two are functionally equivalent today, but the inconsistency between near-identical helpers invites future divergence.
**Recommendation:** Make the two `_canceling_iterator` helpers identical, ideally by factoring a single shared helper.
**Resolution:** 2026-05-18 — Confirmed the divergence. Factored a single shared helper: `client._canceling_iterator(call, operation)` now takes the `map_rpc_error` operation string as a parameter, explicitly catches `asyncio.CancelledError` (cancels the call, re-raises) and `grpc.RpcError`, and repeats the cancel in `finally`. This replaces both the gateway `_canceling_iterator` and the gateway `_canceling_active_alarms_iterator`; `galaxy.py` now imports and delegates to the same helper instead of defining its own, so the gateway and Galaxy stream helpers are byte-for-byte identical. TDD: `tests/test_low_severity_findings.py::test_gateway_stream_iterator_cancels_call_on_task_cancellation` drives a cancellable fake stream and asserts the gateway iterator cancels the underlying call on task cancellation. All existing stream-cancellation tests still pass.
### Client.Python-008
| Field | Value |
|---|---|
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `clients/python/src/mxgateway/values.py:62-67,83-88` |
| Status | Resolved |
**Description:** `to_mx_value` maps any Python `float` to `VT_R8`/`MX_DATA_TYPE_DOUBLE` with no handling for `nan`/`inf`, which are serialised and forwarded to MXAccess which may reject or mis-handle them. `bytes` is mapped to `VT_RECORD`/`MX_DATA_TYPE_UNKNOWN`, a questionable default. The `data_type` keyword exists but `Session.write` never forwards it.
**Recommendation:** Document the float/bytes mapping assumptions, optionally validate finiteness, and consider plumbing the `data_type` keyword through `Session.write`/`write2`.
**Resolution:** 2026-05-18 — Confirmed the non-finite-float hazard. Added an `_ensure_finite` guard in `values.py`: `to_mx_value` now raises `ValueError` for `nan`/`inf`/`-inf`, both for a scalar `float` and for a non-finite element inside a float sequence — MXAccess has no defined wire representation for non-finite doubles, so rejecting client-side is the correct fail-fast. The `float`/`bytes` mapping assumptions (finite-only doubles; `bytes` as an opaque `VT_RECORD` pass-through) are now documented in the `values.py` module docstring and `clients/python/README.md`. Plumbing `data_type` through `Session.write`/`write2` was deliberately *not* done: it is a larger public-API surface change the finding only marks as "consider", and the documented MXAccess-parity convention is type-by-Python-value; the `data_type` keyword stays available on `to_mx_value` for callers that build the `MxValue` directly. TDD: `tests/test_low_severity_findings.py` adds `test_to_mx_value_rejects_nan`, `test_to_mx_value_rejects_positive_infinity`, `test_to_mx_value_rejects_negative_infinity`, `test_to_mx_value_rejects_non_finite_float_in_sequence`, and `test_to_mx_value_accepts_finite_float`. README updated since `to_mx_value` (used by `Session.write`/`write2`) now rejects an input it previously accepted.
### Client.Python-009
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Testing coverage |
| Location | `clients/python/tests/` |
| Status | Resolved |
**Description:** Several non-trivial public paths are untested: `Session.write2`/`add_item2` request construction; the bulk-size limit `_ensure_bulk_size`/`MAX_BULK_ITEMS` guard; the `None`-argument `TypeError` guards in bulk methods; the TLS `ca_file` read path in `create_channel`; most CLI command bodies; and `map_rpc_error`'s default (non-auth) branch.
**Recommendation:** Add tests for `write2`/`add_item2` request shape, the bulk-size `ValueError`, the `ca_file` TLS branch, the generic `map_rpc_error` fallthrough, and at least one happy-path CLI command using a fake stub.
**Resolution:** 2026-05-18 — Confirmed coverage gap against the existing `tests/` files. Added `tests/test_coverage_gaps.py` covering every path the finding lists: `test_add_item2_sends_item_context_and_returns_handle` and `test_write2_sends_value_and_timestamp_value` (request shape + `MxValue` oneof), `test_subscribe_bulk_rejects_oversized_request` and `test_add_item_bulk_at_limit_is_allowed` (the `MAX_BULK_ITEMS` `_ensure_bulk_size` boundary), `test_advise_item_bulk_rejects_none_argument` (the `None`-argument `TypeError` guard), `test_create_channel_reads_ca_file` and `test_create_channel_missing_ca_file_raises` (the TLS `ca_file` read path), `test_map_rpc_error_generic_branch_returns_transport_error` and `test_map_rpc_error_handles_error_without_code` (the non-auth `map_rpc_error` fallthrough and the no-`code` path), and `test_cli_register_happy_path_emits_server_handle` (a happy-path CLI command body driven end to end through `CliRunner` with a fake stub via a monkeypatched `_connect`). All 10 new tests pass. No source change required — this is a pure coverage finding.
### Client.Python-010
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `clients/python/src/mxgateway/session.py:404`, `clients/python/src/mxgateway_cli/commands.py:422-425` |
| Status | Resolved |
**Description:** `session.py` ends with a module-level late import `from .client import GatewayClient # noqa: E402` purely to satisfy a string type hint, and `commands.py:_session` does a function-local import. Both work around a circular dependency that `from __future__ import annotations` (already in effect) makes unnecessary. `_session` also lacks a return type annotation.
**Recommendation:** Drop the runtime late import in `session.py` and use a `TYPE_CHECKING`-guarded import for the hint; add the `-> Session` return annotation to `commands.py:_session`.
**Resolution:** 2026-05-18 — Confirmed: with `from __future__ import annotations` in effect all annotations are strings, so the runtime late import was unnecessary. Removed the trailing `from .client import GatewayClient # noqa: E402` in `session.py` and replaced it with a top-of-file `if TYPE_CHECKING:` import that satisfies the `GatewayClient` hint without a runtime dependency (no import cycle: `client.py` does not import `session` at module scope). In `commands.py`, hoisted the function-local `from mxgateway.session import Session` to a module-level import and added the `-> Session` return annotation to `_session`. Verified `import mxgateway` and `import mxgateway_cli.commands` succeed with no circular-import error. Pure refactor — covered by the existing import and CLI tests; no new test needed.
### Client.Python-011
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `clients/python/src/mxgateway/errors.py:122-148` |
| Status | Resolved |
**Description:** `ensure_mxaccess_success` raises `MxAccessError` if any `mx_status.success == 0`. This treats `success == 0` as the failure sentinel, but `0` is also the proto3 scalar default for an unset `MxStatusProxy`. If the gateway ever returns a reply with an unpopulated status entry (e.g. a partially-filled bulk result), the client raises `MxAccessError` even though no real failure occurred.
**Recommendation:** Confirm against the proto/gateway contract whether `success` is guaranteed populated for every `statuses` entry; if not, key the failure decision on an explicit failure field rather than the `success == 0` default.
**Resolution:** 2026-05-18 — Confirmed against the gateway contract: `success` is **not** guaranteed populated for every `statuses` entry. `src/MxGateway.Worker/Conversion/MxStatusProxyConverter.cs::ConvertMany` emits a placeholder `MxStatusProxy` for a null `MXSTATUS_PROXY` COM array entry, setting `Category`/`DetectedBy` to `Unknown` but **leaving `Success` at its proto3 default of 0**. A fully-default proto entry likewise has `success == 0`. Under the old client logic either placeholder would falsely raise `MxAccessError`. Fixed `ensure_mxaccess_success` to key the per-status failure decision on a new `_is_mxaccess_status_failure` helper that requires `success == 0` **and** a populated, non-OK `category` — a status with `category` of `MX_STATUS_CATEGORY_UNSPECIFIED` (default proto) or `MX_STATUS_CATEGORY_UNKNOWN` (the null-entry placeholder) is treated as unpopulated and ignored. `MX_STATUS_CATEGORY_OK` is also excluded so a genuine success entry never raises. Real failures (categories `WARNING` and the error categories, raw value ≥ 2) still raise as before — the existing `write.mxaccess-failure` fixture (`SECURITY_ERROR`/`OPERATIONAL_ERROR` statuses) and the `MXACCESS_FAILURE` protocol-status path are unaffected. TDD: `tests/test_low_severity_findings.py` adds `test_ensure_mxaccess_success_ignores_unpopulated_status_entry` (default + null-placeholder entries, no raise), `test_ensure_mxaccess_success_raises_on_populated_failure_status` (populated `COMMUNICATION_ERROR`, raises), and `test_ensure_mxaccess_success_passes_when_status_reports_success`. No public-behaviour change for genuine replies, so no README update.
### Client.Python-012
| Field | Value |
|---|---|
| Severity | Low |
| Category | mxaccessgw conventions |
| Location | `clients/python/src/mxgateway/client.py:84-108`, `clients/python/src/mxgateway/session.py:57-77` |
| Status | Won't Fix |
**Description:** `Session.invoke_raw` does not run `ensure_mxaccess_success` while `Session.invoke` does, so a caller using `invoke_raw` for parity tests gets a reply where an MXAccess HRESULT failure is silently embedded with no exception. This is by design but under-documented — the README's "preserve raw replies" sentence does not state that `*_raw` methods skip MXAccess-failure detection entirely.
**Recommendation:** Document explicitly (README + docstring) that `*_raw` methods surface MXAccess HRESULT/status failures only inside the reply and do not raise `MxAccessError`, so parity-test callers know to inspect `protocol_status`/`hresult`/`statuses` themselves.
**Resolution:** 2026-05-18 — Won't Fix (no behaviour change). Confirmed this is intentional, correct parity behaviour: the `*_raw` methods exist precisely so parity-test callers can inspect an unmodified gateway reply, including embedded MXAccess HRESULT/status failures, without an exception masking them. Changing `invoke_raw` to raise `MxAccessError` would defeat its purpose and duplicate `Session.invoke`. The finding's only actionable point is the documentation gap, which has been addressed: `clients/python/README.md` now states explicitly that `*_raw` methods enforce gateway protocol success only and do **not** run MXAccess-failure detection, and the docstrings of `GatewayClient.invoke_raw` and `Session.invoke_raw` say the same and point callers to inspect `protocol_status`/`hresult`/`statuses` (and to `Session.invoke` for the checked variant). No code/test change — the runtime contract is unchanged and correct.
### Client.Python-013
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Security |
| Location | `clients/python/src/mxgateway_cli/commands.py:757-762` |
| Status | Resolved |
**Description:** `_use_plaintext` silently returns `True` whenever the endpoint
string starts with `localhost:` or `127.0.0.1:`, even if neither `--plaintext`
nor `--tls` is supplied on the command line. Any CLI subcommand (e.g.
`mxgw-py open-session --endpoint localhost:5001 --api-key mxgw_<secret>`) then
attaches the API key to a plaintext gRPC channel without warning. This is a
silent security downgrade: a user who deliberately ran the gateway behind TLS
on loopback (e.g. for testing a production-shaped TLS config locally) and who
passes `--api-key` expecting the secret to be transport-protected gets a
plaintext bearer token instead. The auto-downgrade is also undocumented —
`README.md` and the CLI `--help` text both describe `--plaintext` and `--tls`
as the controls, with no mention that endpoint-prefix matching can override
either. The other client CLIs do not auto-downgrade: the .NET CLI uses
`https://`-prefix detection on a URI scheme (an explicit signal), Go and Java
require an explicit `--plaintext`/`--tls` choice, and Rust defaults to
plaintext only when `plaintext = true` is set on the options struct.
**Recommendation:** Drop the localhost-prefix auto-plaintext branch and
require the user to pass `--plaintext` or `--tls` (or default to TLS to match
the rest of the matrix). If the implicit-localhost behaviour is kept for
ergonomics, document it prominently in both `README.md` and `--help`, emit a
stderr warning when `--api-key` is combined with the auto-downgrade path, and
add a CLI test asserting the auto-downgrade is in fact active so it is not
silently lost in a future refactor.
**Resolution:** 2026-05-20 — Removed the silent `localhost:` / `127.0.0.1:`
auto-plaintext branch from `_use_plaintext`. The new contract matches the Go
and Java CLIs: **TLS is the default**, `--plaintext` is the only way to opt
in to an unencrypted channel, and `--tls` is accepted as a redundant, explicit
affirmation of the default (mutually exclusive with `--plaintext`, which now
raises `click.UsageError`). The `--plaintext` / `--tls` `--help` text and
`clients/python/README.md` both call out the new behaviour. Added six
regression tests in `clients/python/tests/test_cli.py` covering: (a) a
`localhost:` endpoint with no flags resolves to TLS, (b) a `127.0.0.1:`
endpoint with no flags resolves to TLS, (c) `--plaintext` opts in to plaintext,
(d) `--tls` is accepted and idempotent with the default, (e) `--plaintext`
combined with `--tls` is rejected, and (f) an end-to-end CliRunner test
asserting `ClientOptions.plaintext == False` flows through to
`GatewayClient.connect` when no flag is supplied against a `localhost:`
endpoint. **Behaviour change for callers:** scripts that previously relied on
`mxgw-py … --endpoint localhost:5000 …` selecting plaintext silently must now
add an explicit `--plaintext` flag (or set up TLS on the gateway). Calling
`mxgw-py` with an `--api-key` against a plaintext-only gateway without
`--plaintext` will now fail to connect rather than silently leaking the bearer
token.
### Client.Python-014
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `clients/python/src/mxgateway_cli/commands.py:22-23` |
| Status | Resolved |
**Description:** `commands.py` has two consecutive `from mxgateway.values
import` lines:
```python
from mxgateway.values import to_mx_value
from mxgateway.values import MxValueInput
```
These import from the same module and should be combined into a single
`from mxgateway.values import MxValueInput, to_mx_value`. The split form is
inconsistent with the rest of the file (every other module is imported in a
single statement) and would be flagged by `ruff`/`isort` if any linter were
configured. Pure style, no behavioural impact.
**Recommendation:** Collapse the two imports into one statement, ordered to
match the conventional alphabetical-within-module pattern:
`from mxgateway.values import MxValueInput, to_mx_value`.
**Resolution:** 2026-05-20 — Collapsed the two consecutive
`from mxgateway.values import to_mx_value` / `from mxgateway.values import MxValueInput`
lines in `clients/python/src/mxgateway_cli/commands.py` into a single
`from mxgateway.values import MxValueInput, to_mx_value` statement, matching
the alphabetical-within-module pattern used elsewhere in the file. Pure style
fix — no behavioural impact, covered by the existing CLI tests.
### Client.Python-015
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `clients/python/src/mxgateway_cli/commands.py:273-294,564-647`, `clients/python/tests/` |
| Status | Resolved |
**Description:** `_bench_read_bulk` is a ~80-line CLI body that opens its own
session, registers, subscribe_bulks, runs a warm-up loop, a measurement loop,
collects per-call latencies, computes a percentile summary, and emits the
shared cross-language JSON schema. It is the largest untested CLI command in
the module — `tests/` has no `bench_read_bulk` test, fake-stub-driven or
otherwise. A drift in the schema field names (`callsPerSecond`,
`cachedReadResults`, `latencyMs.p50`, …) would break the cross-language
`scripts/bench-read-bulk.ps1` aggregation silently. `_percentile_summary` and
`_percentile` are also untested — the boundary cases (`n == 0`, `n == 1`,
quantile interpolation) would benefit from a small unit test since the
identical algorithm is duplicated in the .NET / Go / Rust / Java drivers and
a divergence would corrupt cross-language comparisons.
**Recommendation:** Add a fake-stub-driven `bench_read_bulk` test that drives
a short `--duration-seconds 0 --warmup-seconds 0` run through `CliRunner` and
asserts the JSON schema (`language == "python"`, the full key set,
`latencyMs.p50/p95/p99/max/mean` present). Add unit tests for `_percentile`
covering `n == 0`, `n == 1`, and a known-good interpolated value at p95 so
the implementation cannot silently drift from the other clients.
**Resolution:** 2026-05-20 — Added `clients/python/tests/test_cli_bench_and_helpers.py`
with three layers of coverage. (1) `_percentile` unit tests pin the
cross-language algorithm (`rank = q * (n - 1)`, linear interpolation between
adjacent ranks): empty sample returns `0.0`, single element returns that
element, exact-rank queries return the sample value (p50 of `[10,20,30,40,50]`
is `30.0`), and the interpolated p95/p99 values (`48.0` / `49.6` for that same
five-element sample) are locked down so any drift from the .NET / Go / Rust /
Java drivers fails fast. (2) `_percentile_summary` tests assert the full
`{p50, p95, p99, max, mean}` dict shape, the zero-sample placeholder, and the
3-decimal rounding contract. (3) A `bench-read-bulk` smoke test
(`test_bench_read_bulk_emits_cross_language_schema`) drives the CLI through
`CliRunner` with `--duration-seconds 0 --warmup-seconds 0` against a fake stub
that handles `OpenSession`, `Register`, `SubscribeBulk`, `ReadBulk`, and
`UnsubscribeBulk`, then asserts the emitted JSON has exactly the 16
cross-language schema keys (`language`, `command`, `endpoint`, `clientName`,
`bulkSize`, `durationSeconds`, `warmupSeconds`, `durationMs`, `tags`,
`totalCalls`, `successfulCalls`, `failedCalls`, `totalReadResults`,
`cachedReadResults`, `callsPerSecond`, `latencyMs`) and that `latencyMs` is a
`{p50, p95, p99, max, mean}` sub-object — guarding against silent breakage of
`scripts/bench-read-bulk.ps1`'s cross-language aggregation. No source change —
this is a pure coverage finding.
### Client.Python-016
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `clients/python/src/mxgateway_cli/commands.py:25,757-775,805-830` |
| Status | Resolved |
**Description:** Three CLI helper paths are not covered by `tests/`:
1. `_use_plaintext` localhost auto-downgrade (line 762) — the
`endpoint.startswith("localhost:") or endpoint.startswith("127.0.0.1:")`
branch (see also Client.Python-013) is untested; no test asserts that an
endpoint without `--plaintext` and without `--tls` resolves to plaintext.
2. `_collect_events` `MAX_AGGREGATE_EVENTS` guard (line 811-815) — passing
`--max-events` greater than `MAX_AGGREGATE_EVENTS` raises
`click.BadParameter`, but no test exercises the guard. A silent removal of
the constant or the comparison would not be caught.
3. `_api_key_from_env` (line 765-768) — only the implicit path through
`_secrets` is exercised; there is no test that verifies an env-var name
resolves to a value and that an unset env var produces `None`.
These are all small, fake-stub-driven CLI behaviours rather than end-to-end
paths. The previous coverage finding (Client.Python-009) closed without
adding tests for these specific paths.
**Recommendation:** Add three small `CliRunner` / unit tests: one asserting
the localhost auto-plaintext (or its replacement, if Client.Python-013 is
fixed), one asserting `--max-events 10001` exits non-zero with the
`MAX_AGGREGATE_EVENTS` error message, and one asserting
`_api_key_from_env("MXGATEWAY_API_KEY")` returns the env value and `None` for
an unset variable.
**Resolution:** 2026-05-20 — Scope adjusted: Client.Python-013 has since
removed the `_use_plaintext` localhost auto-plaintext branch, so item (1) is
no longer a real code path — the
`test_use_plaintext_requires_explicit_flag_for_localhost_endpoint` and
`test_cli_localhost_endpoint_defaults_to_tls_via_open_session` regressions
added under Client.Python-013 already pin the new TLS-by-default contract.
The remaining two helpers are now covered in
`clients/python/tests/test_cli_bench_and_helpers.py`. (2)
`MAX_AGGREGATE_EVENTS` cap:
`test_collect_events_rejects_max_events_above_aggregate_cap` drives
`stream-events` with `--max-events 10001` through `CliRunner` against
stubbed `_connect` / `_session` fakes and asserts the CLI exits non-zero with
the documented `less than or equal to 10000` message;
`test_collect_events_accepts_max_events_at_aggregate_cap_boundary` confirms
`--max-events 10000` is accepted at the boundary and returns an empty event
list. (3) `_api_key_from_env`:
`test_api_key_from_env_resolves_value_when_variable_is_set` (env-var
populated → returned),
`test_api_key_from_env_returns_none_when_variable_is_unset` (env-var unset
`None`), `test_api_key_from_env_returns_none_when_name_is_none` (the
`name is None` early-return), and
`test_api_key_from_env_returns_none_when_name_is_empty_string` (the
`if not name` truthiness guard). No source change — pure coverage finding.
### Client.Python-017
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `clients/python/pyproject.toml:5-25`, `clients/python/src/mxgateway/` |
| Status | Resolved |
**Description:** The package metadata in `pyproject.toml` is minimal for a
published wheel:
* No `authors` field. PyPI / `pip show` will display no author.
* No `license` field, no `license-files` field, and no `LICENSE` file is
referenced from the project. The repo as a whole has no top-level
`LICENSE` either, but other client packages (Java has a license entry, the
.NET package has a license expression in the `csproj`) tend to set this.
* No `classifiers` (no `Programming Language :: Python :: 3.12`,
`Operating System :: Microsoft :: Windows`, `Topic :: …`, no
development-status classifier). Without these the PyPI search facets are
empty and tooling like `pip` cannot tell whether the package is
alpha/beta/stable.
* No `keywords`, no `[project.urls]` (no homepage / source / issue link
pointing back to the repo).
* The package ships no PEP 561 `py.typed` marker file in
`src/mxgateway/`. Type hints are written throughout the module
(`from __future__ import annotations`, full annotations on every public
function), but downstream consumers running `mypy` on `mxaccess-gateway-client`
will not see those hints — PEP 561 requires the marker file to opt the
package into type-stub distribution.
**Recommendation:** Add `authors`, `license = "<spdx>"`, `keywords`, and
`[project.urls]` to `pyproject.toml`; add at least the standard `classifiers`
trio (`Development Status`, `Programming Language :: Python :: 3.12`,
`Intended Audience`); create an empty `src/mxgateway/py.typed` file and
include it in the wheel via `[tool.setuptools.package-data]` so consumers
running `mypy` against an installed wheel pick up the type information.
**Resolution:** 2026-05-20 — Filled out `clients/python/pyproject.toml`
with the missing PyPI metadata: `authors = [{ name = "MXAccess Gateway
Authors" }]`, `license = "Proprietary"` (the repo has no top-level
`LICENSE` file and no other client publishes under an OSS licence, so the
SPDX `Proprietary` expression matches the de-facto status), the standard
classifier set (`Development Status :: 4 - Beta`, `Intended Audience ::
Developers` / `Information Technology`, `Operating System :: Microsoft ::
Windows` and `:: POSIX`, `Programming Language :: Python` /
`Python :: 3` / `Python :: 3.12`, `Topic :: Software Development ::
Libraries :: Python Modules`, `Topic :: System :: Distributed Computing`,
and `Typing :: Typed`), a `keywords` list
(`mxaccess`, `archestra`, `gateway`, `grpc`, `industrial`, `scada`), and
`[project.urls]` with `Homepage` / `Source` / `Issues` pointing at the
Gitea repo. Added the PEP 561 marker file
`clients/python/src/mxgateway/py.typed` (empty, as the spec requires) and
declared it in `[tool.setuptools.package-data] mxgateway = ["py.typed"]`
so the wheel ships the marker and downstream `mypy` users see the
inline type hints. Pure metadata / packaging change — `python -m pytest -q`
still passes (91 tests).
### Client.Python-018
| Field | Value |
|---|---|
| Severity | High |
| Category | Code organization & conventions |
| Location | `clients/python/pyproject.toml:11` |
| Status | Resolved |
**Description:** The Client.Python-017 resolution set
`license = "Proprietary"` as a top-level string. Under PEP 639 (enforced
by `setuptools >= 77`, and active in the installed `setuptools 82.0.1`),
the `project.license` string form must be a valid SPDX expression.
`"Proprietary"` is not a registered SPDX identifier, so the configured
build backend (`setuptools.build_meta`) refuses the file outright. Both
`python -m pip wheel . --no-deps --wheel-dir …` and
`python -m pip install -e .` — the exact commands documented in
`clients/python/README.md` ("Build And Test", "Packaging") and the
"build wheel" instruction in `docs/ClientPackaging.md` — now fail before
any source is compiled with:
```
ValueError: invalid pyproject.toml config: `project.license`.
configuration error: `project.license` must be valid exactly by one definition (0 matches found):
- {type: string, format: 'SPDX'}
- type: table keys: 'file': … required: ['file']
- type: table keys: 'text': … required: ['text']
```
`python -m pytest` still runs because `[tool.pytest.ini_options]
pythonpath = ["src"]` lets pytest import the package without an install
— which masked the regression at commit time and explains how the
Client.Python-017 resolution comment was able to assert "`python -m
pytest -q` still passes (91 tests)" while shipping a wheel build that
cannot start. The Client.Python-017 resolution comment that "the SPDX
`Proprietary` expression matches the de-facto status" is incorrect:
`Proprietary` is *not* a registered SPDX identifier; only entries on the
SPDX licence list (e.g. `MIT`, `Apache-2.0`, `BSD-3-Clause`) or
`LicenseRef-*` custom identifiers satisfy the
`{ type: string, format: 'SPDX' }` rule. PEP 639 added the
`LicenseRef-…` escape hatch precisely for proprietary / unlisted
licences.
This is a regression of the developer-onboarding workflow introduced by
the very commit being reviewed. A fresh checkout cannot run
`python -m pip install -e ".[dev]"` (the command in `CLAUDE.md`'s
"Clients" section) without first patching `pyproject.toml`.
**Recommendation:** Fix the `license` value so the build backend
accepts it. Three concrete options, in order of preference:
1. Use a `LicenseRef-*` SPDX-compatible custom identifier:
`license = "LicenseRef-Proprietary"`. Requires no additional
`LICENSE` file and is honoured by setuptools / pip / PyPI as a
proprietary marker.
2. Add a top-level `LICENSE` file (or `clients/python/LICENSE`) and
point at it via the table form:
`license = { file = "LICENSE" }`. This also documents the proprietary
terms.
3. Drop the `license` key entirely and convey the same intent via the
classifier `"License :: Other/Proprietary License"` (already part of
the classifier set), reverting the PEP-639 string field that the
build backend now insists must be SPDX.
Add a CI / pre-commit check that runs `python -m pip wheel . --no-deps`
(or `python -m build`) on `clients/python` so a future
`pyproject.toml` regression is caught at commit time rather than at
first install on a clean machine. See also Client.Python-020.
**Resolution:** 2026-05-20 — Dropped the invalid top-level
`license = "Proprietary"` string from `clients/python/pyproject.toml`
and added the existing `License :: Other/Proprietary License` trove
classifier to convey the same intent without violating PEP 639's SPDX
rule. No `LICENSE` file exists at the repo root or under
`clients/python/`, so the `license = { file = "LICENSE" }` table form
was not used; relying on the classifier is the option (3) variant
called out in the recommendation. Verified by running
`python -m pip wheel . --no-deps -w ./.test-wheel-output` from
`clients/python`: the build now succeeds and emits
`mxaccess_gateway_client-0.1.0-py3-none-any.whl` (47 KB) where
previously it failed with the `project.license must be valid exactly
by one definition` `ValueError`. The CI / pre-commit recommendation is
addressed by Client.Python-020.
### Client.Python-019
| Field | Value |
|---|---|
| Severity | Low |
| Category | Code organization & conventions |
| Location | `clients/python/pyproject.toml:60-61`, `clients/python/src/mxgateway_cli/` |
| Status | Resolved |
**Description:** Client.Python-017 added the PEP 561 marker file
`clients/python/src/mxgateway/py.typed` and declared it in
`[tool.setuptools.package-data] mxgateway = ["py.typed"]`. The wheel
therefore advertises `mxgateway` as typed. However the same wheel
also ships the **`mxgateway_cli`** package (`setuptools.packages.find`
with `where = ["src"]` discovers both `mxgateway` and `mxgateway_cli`,
confirmed via `find_packages` in this review), and `mxgateway_cli`:
* is shipped in the wheel and is the package the `mxgw-py` console
script entry point resolves into (`[project.scripts] mxgw-py =
"mxgateway_cli.commands:main"`),
* is fully type-annotated (every function in `commands.py` has full
parameter and return annotations; `from __future__ import annotations`
is in effect),
* but has no `py.typed` file and is not listed in
`[tool.setuptools.package-data]`.
PEP 561 requires the marker file inside **each** importable package the
distribution wants to expose to type checkers — the `mxgateway` marker
does not transfer to `mxgateway_cli`. A downstream consumer that imports
or composes against `mxgateway_cli` (e.g. wrapping it as a programmatic
CLI library) will see all symbols as `Untyped` under `mypy` despite the
hints being present in source.
This is a follow-up to Client.Python-017 — the fix is small and pure
packaging.
**Recommendation:** Create
`clients/python/src/mxgateway_cli/py.typed` (empty file, as PEP 561
requires) and extend the existing package-data declaration so the
wheel ships it:
```toml
[tool.setuptools.package-data]
mxgateway = ["py.typed"]
mxgateway_cli = ["py.typed"]
```
No source change in either package; verify by building a wheel
(once Client.Python-018 is fixed) and inspecting that both
`mxgateway/py.typed` and `mxgateway_cli/py.typed` appear in the wheel
contents.
**Resolution:** 2026-05-20 — Created the empty PEP 561 marker file
`clients/python/src/mxgateway_cli/py.typed` and added
`mxgateway_cli = ["py.typed"]` under
`[tool.setuptools.package-data]` in `clients/python/pyproject.toml`
alongside the existing `mxgateway = ["py.typed"]` line. Verified by
inspecting the built wheel
(`mxaccess_gateway_client-0.1.0-py3-none-any.whl`): the archive now
contains both `mxgateway/py.typed` and `mxgateway_cli/py.typed`, so
downstream `mypy` consumers see the inline type hints in both
packages. Pure packaging change — no source modifications.
### Client.Python-020
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `clients/python/tests/`, `scripts/` |
| Status | Resolved |
**Description:** Client.Python-018 is invisible to the existing test
suite: `python -m pytest` passes because `[tool.pytest.ini_options]
pythonpath = ["src"]` lets pytest import the package without going
through `setuptools.build_meta`. None of the 91 tests build the wheel,
do an editable install, or otherwise exercise the
`setuptools.build_meta` configuration validator. As a result, a
`pyproject.toml` regression that breaks `pip install -e .` /
`pip wheel .` — the exact commands documented in the Python client
README and `CLAUDE.md` — passes the test suite green. The other
language clients have parallel coverage gaps (no CI-level "the package
installs" smoke test for Python in
`scripts/run-client-e2e-tests.ps1`, which only runs the live e2e
matrix and assumes the editable install already worked), but Python
is the only one whose published install command is currently broken.
**Recommendation:** Add a thin pytest module (e.g.
`tests/test_packaging.py`) that runs
```python
import subprocess, sys, pathlib
def test_pyproject_validates_against_setuptools_build_meta():
here = pathlib.Path(__file__).resolve().parent.parent
result = subprocess.run(
[sys.executable, "-m", "pip", "wheel", ".",
"--no-deps", "--no-build-isolation",
"--wheel-dir", str(tmp_path)],
cwd=here, capture_output=True, text=True,
)
assert result.returncode == 0, result.stderr
```
(or any equivalent that invokes
`setuptools.config.pyprojecttoml.read_configuration`). Marker the test
with `@pytest.mark.slow` if the wheel build is too heavy for the
default suite, and document the test in the README. Alternatively
add a CI step to `scripts/run-client-e2e-tests.ps1` (or a new
`scripts/check-python-package.ps1`) that fails the build when the
wheel build fails. Either approach would have surfaced
Client.Python-018 at commit time.
**Resolution:** 2026-05-20 — Added
`clients/python/tests/test_packaging.py::test_pip_wheel_build_succeeds`.
The test invokes `python -m pip wheel . --no-deps --wheel-dir <tmp>`
against the package root via `subprocess` and asserts (a) exit code
zero and (b) an `mxaccess_gateway_client-*.whl` file is produced in
the temp directory, capturing stdout/stderr in the assertion message
on failure so any future PEP 639 / SPDX violation or other
`setuptools.build_meta` configuration error is reported with the
build backend's own error text. Verified the test would have caught
Client.Python-018: with the old `license = "Proprietary"` string in
place the test fails with the `project.license must be valid exactly
by one definition` `ValueError`. The pytest module is the simpler
half of the recommendation; no PowerShell wrapper script was added
since pytest already runs in the same `python -m pytest` invocation
the README documents. Test suite is now 92 tests (was 91), all
passing.
### Client.Python-021
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `clients/python/src/mxgateway_cli/commands.py`, `clients/python/README.md:235-258` |
| Status | Resolved |
**Description:** Cross-client CLI parity check (one of the things the
review prompt asks for): the `mxgw-py` CLI subcommand set has drifted
from every other client CLI in the matrix.
Subcommand inventory at this commit:
| Subcommand | .NET (`mxgw`) | Go (`mxgw-go`) | Rust (`mxgw`) | Java (`mxgw-java`) | Python (`mxgw-py`) |
|---|---|---|---|---|---|
| `version` | yes | yes | yes | yes | yes |
| `ping` | yes | (no) | yes | (no) | yes |
| `open-session` / `close-session` | yes | yes | yes | yes | yes |
| `register` / `add-item` / `advise` | yes | yes | yes | yes | yes |
| `subscribe-bulk` / `unsubscribe-bulk` / `read-bulk` | yes | yes | yes | yes | yes |
| `write-bulk` / `write2-bulk` / `write-secured-bulk` / `write-secured2-bulk` | yes | yes | yes | yes | yes |
| `write` / `write2` | yes / (varies) | yes / (no) | yes / yes | yes / (no) | yes / yes |
| `stream-events` | yes | yes | yes | yes | yes |
| `smoke` | yes | yes | yes | yes | yes |
| `bench-read-bulk` | yes | yes | yes | yes | yes |
| `bench-stream-events` | **yes** | (no) | (no) | (no) | (no) |
| `galaxy-test-connection` (or alias) | **yes** | **yes** | **yes** | **yes** | **(no)** |
| `galaxy-last-deploy` / `galaxy-deploy-time` | **yes** | **yes** | **yes** | **yes** | **(no)** |
| `galaxy-discover` | **yes** | **yes** | **yes** | **yes** | **(no)** |
| `galaxy-watch` | **yes** | **yes** | **yes** | **yes** | **(no)** |
Two parity gaps remain after Client.Python-013/017:
1. The Python CLI ships **no Galaxy subcommands at all** even though
the `GalaxyRepositoryClient` library wrapper is fully implemented
and exercised by `tests/test_galaxy.py` /
`tests/test_galaxy_iter_hierarchy.py`. The README acknowledges the
`watch-deploy-events` gap inline ("The CLI does not currently
expose a streaming `watch-deploy-events` subcommand — use the
library API directly when subscribing to deploy events from
Python.") but does not call out that **the other three Galaxy
subcommands are also missing** — and the .NET / Go / Rust / Java
CLIs all expose them. A user running the cross-language smoke
matrix who expects Python to behave like the other clients sees a
silent "command not found" on `mxgw-py galaxy-test-connection`.
2. The new `bench-stream-events` subcommand (added to the .NET CLI in
the previous commit `1cd51bb`) is .NET-only today; the Python CLI
is consistent with Go / Rust / Java on this point. Worth flagging
as a forward-looking parity gap that will need filling if the
cross-language benchmark matrix grows a stream-events driver in
`scripts/`.
Severity is Low because the existing `scripts/bench-read-bulk.ps1`
matrix only invokes `bench-read-bulk` and does not break, and the
Python `GalaxyRepositoryClient` library is fully functional — the gap
is purely in the test CLI surface. But cross-client parity is an
explicit review check and the gap is not documented.
**Recommendation:** Either (a) add `galaxy-test-connection`,
`galaxy-last-deploy`, `galaxy-discover`, and `galaxy-watch`
subcommands to `mxgateway_cli/commands.py` (each is a thin wrapper
over `GalaxyRepositoryClient`, mirroring the existing four-language
implementation), or (b) update `clients/python/README.md`'s "CLI"
section with an explicit "CLI parity gaps" subsection that lists the
missing subcommands and recommends the library API. Option (a) is
preferable for cross-language matrix testing. Also document the
`bench-stream-events` gap symmetrically once a cross-language stream
benchmark driver is added under `scripts/`.
**Resolution:** 2026-05-20 — Scoped this finding to a
documentation-only fix; the full Galaxy CLI parity implementation
(four new subcommands wired to `GalaxyRepositoryClient`) is a larger
piece of work and will be tracked as a separate follow-up finding.
Added a new "CLI Parity Gaps" subsection to
`clients/python/README.md` immediately under the existing CLI
section that explicitly enumerates the four missing
`mxgw-py` Galaxy subcommands (`galaxy-test-connection`,
`galaxy-last-deploy`, `galaxy-discover`, `galaxy-watch`), names the
sibling CLIs that already expose them (.NET `mxgw`, Go `mxgw-go`,
Rust `mxgw`, Java `mxgw-java`), points readers at the library API
(`GalaxyRepositoryClient`, already documented under "Galaxy
Repository Browse") as the supported Python entry point in the
interim, and also flags the .NET-only `bench-stream-events` gap so
the cross-language benchmark matrix has a record of the asymmetry.
No CLI source change; the implementation of the four Galaxy
subcommands is deferred. Resolved as a doc note rather than a full
parity fix.