From a7bf1ef95d689e9720251ac9491cd90fe29584e3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 18 May 2026 22:59:24 -0400 Subject: [PATCH] Resolve Client.Python-001/002/004/006/007/008/010/011/012 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client.Python-001: dropped "scaffold" from the stale pyproject description. Client.Python-002 (re-triaged): stale finding — MxGatewayCommandError is already exported and in __all__; no change needed. Client.Python-004: removed the dead `closed` variable in _smoke; the CLI smoke now uses `async with session`. Client.Python-006: close() on both clients and Session had an unlocked check-then-set race; `_closed` is now set before the await. Client.Python-007: gateway stream iterators now share one helper that explicitly catches CancelledError and cancels the call. Client.Python-008: to_mx_value now rejects nan/inf; float/bytes mapping documented. Client.Python-010: removed the circular-import-workaround late imports in favour of TYPE_CHECKING / module-scope imports. Client.Python-011: ensure_mxaccess_success no longer treats a proto3-default success==0 with an unset category as a failure. Client.Python-012 (Won't Fix): invoke_raw deliberately skips MXAccess-failure detection for parity tests; documented the contract instead. Co-Authored-By: Claude Opus 4.7 (1M context) --- clients/python/README.md | 16 ++ clients/python/pyproject.toml | 2 +- clients/python/src/mxgateway/client.py | 58 +++-- clients/python/src/mxgateway/errors.py | 24 +- clients/python/src/mxgateway/galaxy.py | 30 +-- clients/python/src/mxgateway/session.py | 30 ++- clients/python/src/mxgateway/values.py | 27 ++- clients/python/src/mxgateway_cli/commands.py | 11 +- .../tests/test_low_severity_findings.py | 228 ++++++++++++++++++ code-reviews/Client.Python/findings.md | 38 +-- 10 files changed, 385 insertions(+), 79 deletions(-) create mode 100644 clients/python/tests/test_low_severity_findings.py diff --git a/clients/python/README.md b/clients/python/README.md index d014bf8..42d2393 100644 --- a/clients/python/README.md +++ b/clients/python/README.md @@ -95,6 +95,22 @@ async with await GatewayClient.connect( events available for parity tests. `Session` helpers call the method-specific MXAccess commands and preserve raw replies on typed command exceptions. +`*_raw` methods (`GatewayClient.invoke_raw`, `Session.invoke_raw`) surface +gateway protocol failures by raising the typed `MxGateway*` exceptions, but +they deliberately do **not** run MXAccess-failure detection: an MXAccess +HRESULT or `MxStatusProxy` status failure is left embedded in the returned +reply and no `MxAccessError` is raised. `Session.invoke` adds that check on +top. Parity-test callers using `invoke_raw` must inspect the reply's +`protocol_status`, `hresult`, and `statuses` themselves. The non-raw `Session` +helpers (`register`, `add_item`, `write`, the bulk methods, etc.) run the +check and raise `MxAccessError`. + +Value conversion (`to_mx_value`, used by `Session.write`/`write2` and the +bulk helpers) rejects non-finite floats — `nan`, `inf`, and `-inf` raise +`ValueError` rather than being forwarded to MXAccess, which has no defined +wire representation for them. Python `bytes` values are an opaque +`VT_RECORD` pass-through that MXAccess does not interpret. + Canceling a Python task cancels the client-side gRPC call or stream wait. It does not abort an in-flight MXAccess COM call inside the worker process. diff --git a/clients/python/pyproject.toml b/clients/python/pyproject.toml index 3bc0d8d..ebb35e1 100644 --- a/clients/python/pyproject.toml +++ b/clients/python/pyproject.toml @@ -5,7 +5,7 @@ build-backend = "setuptools.build_meta" [project] name = "mxaccess-gateway-client" version = "0.1.0" -description = "Async Python client scaffold for MXAccess Gateway." +description = "Async Python client for MXAccess Gateway." readme = "README.md" requires-python = ">=3.12" dependencies = [ diff --git a/clients/python/src/mxgateway/client.py b/clients/python/src/mxgateway/client.py index f0e909e..3ec5135 100644 --- a/clients/python/src/mxgateway/client.py +++ b/clients/python/src/mxgateway/client.py @@ -72,14 +72,20 @@ class GatewayClient: await self.close() async def close(self) -> None: - """Close the owned gRPC channel.""" + """Close the owned gRPC channel. + + Idempotent, including under concurrent calls: ``_closed`` is set + before the ``await`` so a second coroutine entering ``close()`` + while the first is still awaiting the channel close returns + immediately instead of issuing a second ``channel.close()``. + """ if self._closed: return + self._closed = True if self._channel is not None: await self._channel.close() - self._closed = True async def open_session( self, @@ -117,7 +123,15 @@ class GatewayClient: return reply async def invoke_raw(self, request: pb.MxCommandRequest) -> pb.MxCommandReply: - """Send an `Invoke` RPC and return the raw reply.""" + """Send an `Invoke` RPC and return the raw reply. + + Enforces gateway protocol success only. MXAccess HRESULT/status + failures are left embedded in the reply and do not raise + `MxAccessError` — parity-test callers must inspect the reply's + `protocol_status`, `hresult`, and `statuses` themselves. Use + `Session.invoke` for the variant that also raises on MXAccess + failure. + """ reply = await self._unary("invoke", self.raw_stub.Invoke, request) ensure_protocol_success("invoke", reply.protocol_status, reply) return reply @@ -134,7 +148,7 @@ class GatewayClient: if self.options.stream_timeout is not None: kwargs["timeout"] = self.options.stream_timeout call = _open_stream(self.raw_stub.StreamEvents, request, kwargs) - return _canceling_iterator(call) + return _canceling_iterator(call, "stream events") async def acknowledge_alarm( self, @@ -170,7 +184,7 @@ class GatewayClient: if self.options.stream_timeout is not None: kwargs["timeout"] = self.options.stream_timeout call = _open_stream(self.raw_stub.QueryActiveAlarms, request, kwargs) - return _canceling_active_alarms_iterator(call) + return _canceling_iterator(call, "query active alarms") async def _unary( self, @@ -218,24 +232,26 @@ def _open_stream(method: Any, request: Any, kwargs: dict[str, Any]) -> Any: return method(request, **kwargs) -async def _canceling_iterator(call: Any) -> AsyncIterator[pb.MxEvent]: +async def _canceling_iterator(call: Any, operation: str) -> AsyncIterator[Any]: + """Yield from a server-streaming call and cancel it when iteration stops. + + Explicitly catches :class:`asyncio.CancelledError` to cancel the + underlying call before re-raising, then repeats the cancel in the + ``finally`` block so the call is also cancelled on a clean break or an + ``aclose()``. ``galaxy._canceling_iterator`` delegates here so the + gateway and Galaxy stream helpers stay identical. + """ + try: - async for event in call: - yield event + async for item in call: + yield item + except asyncio.CancelledError: + cancel = getattr(call, "cancel", None) + if cancel is not None: + cancel() + raise except grpc.RpcError as error: - raise map_rpc_error("stream events", error) from error - finally: - cancel = getattr(call, "cancel", None) - if cancel is not None: - cancel() - - -async def _canceling_active_alarms_iterator(call: Any) -> AsyncIterator[pb.ActiveAlarmSnapshot]: - try: - async for snapshot in call: - yield snapshot - except grpc.RpcError as error: - raise map_rpc_error("query active alarms", error) from error + raise map_rpc_error(operation, error) from error finally: cancel = getattr(call, "cancel", None) if cancel is not None: diff --git a/clients/python/src/mxgateway/errors.py b/clients/python/src/mxgateway/errors.py index 7f689af..bc223a0 100644 --- a/clients/python/src/mxgateway/errors.py +++ b/clients/python/src/mxgateway/errors.py @@ -138,7 +138,7 @@ def ensure_mxaccess_success(operation: str, reply: pb.MxCommandReply) -> pb.MxCo ) for mx_status in reply.statuses: - if mx_status.success == 0: + if _is_mxaccess_status_failure(mx_status): raise MxAccessError( _mxaccess_message(operation, reply), protocol_status=status, @@ -148,6 +148,28 @@ def ensure_mxaccess_success(operation: str, reply: pb.MxCommandReply) -> pb.MxCo return reply +def _is_mxaccess_status_failure(mx_status: pb.MxStatusProxy) -> bool: + """Return ``True`` only for a populated MXAccess status reporting failure. + + MXAccess uses ``success == 0`` as the failure flag, but ``0`` is also the + proto3 scalar default. The gateway emits placeholder ``MxStatusProxy`` + entries with ``success`` unset for null ``MXSTATUS_PROXY`` COM entries + (see ``MxStatusProxyConverter.ConvertMany``); such an entry has + ``category`` of ``UNSPECIFIED`` or ``UNKNOWN``. Treating it as a failure + would raise ``MxAccessError`` for a reply that carries no real failure, + so failure is keyed on ``success == 0`` together with a populated, + non-OK status category. + """ + + if mx_status.success != 0: + return False + return mx_status.category not in ( + pb.MX_STATUS_CATEGORY_UNSPECIFIED, + pb.MX_STATUS_CATEGORY_UNKNOWN, + pb.MX_STATUS_CATEGORY_OK, + ) + + def _mxaccess_message(operation: str, reply: pb.MxCommandReply) -> str: status_text = reply.protocol_status.message or "MXAccess command failed" hresult = reply.hresult if reply.HasField("hresult") else None diff --git a/clients/python/src/mxgateway/galaxy.py b/clients/python/src/mxgateway/galaxy.py index b258e6f..8114e40 100644 --- a/clients/python/src/mxgateway/galaxy.py +++ b/clients/python/src/mxgateway/galaxy.py @@ -18,6 +18,7 @@ import grpc from google.protobuf.timestamp_pb2 import Timestamp from .auth import merge_metadata +from .client import _canceling_iterator from .errors import MxGatewayError, map_rpc_error from .generated import galaxy_repository_pb2 as galaxy_pb from .generated import galaxy_repository_pb2_grpc as galaxy_pb_grpc @@ -83,14 +84,20 @@ class GalaxyRepositoryClient: await self.close() async def close(self) -> None: - """Close the owned gRPC channel.""" + """Close the owned gRPC channel. + + Idempotent, including under concurrent calls: ``_closed`` is set + before the ``await`` so a second coroutine entering ``close()`` + while the first is still awaiting the channel close returns + immediately instead of issuing a second ``channel.close()``. + """ if self._closed: return + self._closed = True if self._channel is not None: await self._channel.close() - self._closed = True async def test_connection(self) -> bool: """Return ``True`` when the gateway can reach the Galaxy Repository DB.""" @@ -189,7 +196,7 @@ class GalaxyRepositoryClient: kwargs.pop("timeout") call = self.raw_stub.WatchDeployEvents(request, **kwargs) - return _canceling_iterator(call) + return _canceling_iterator(call, "watch deploy events") async def _unary( self, @@ -218,20 +225,3 @@ class GalaxyRepositoryClient: raise except grpc.RpcError as error: raise map_rpc_error(operation, error) from error - - -async def _canceling_iterator(call: Any) -> AsyncIterator[galaxy_pb.DeployEvent]: - try: - async for event in call: - yield event - except asyncio.CancelledError: - cancel = getattr(call, "cancel", None) - if cancel is not None: - cancel() - raise - except grpc.RpcError as error: - raise map_rpc_error("watch deploy events", error) from error - finally: - cancel = getattr(call, "cancel", None) - if cancel is not None: - cancel() diff --git a/clients/python/src/mxgateway/session.py b/clients/python/src/mxgateway/session.py index 75f647b..905d295 100644 --- a/clients/python/src/mxgateway/session.py +++ b/clients/python/src/mxgateway/session.py @@ -3,11 +3,15 @@ from __future__ import annotations from collections.abc import AsyncIterator, Sequence +from typing import TYPE_CHECKING from .errors import ensure_mxaccess_success from .generated import mxaccess_gateway_pb2 as pb from .values import MxValueInput, to_mx_value +if TYPE_CHECKING: + from .client import GatewayClient + MAX_BULK_ITEMS = 1000 @@ -36,7 +40,13 @@ class Session: await self.close() async def close(self, *, client_correlation_id: str = "") -> pb.CloseSessionReply: - """Close the gateway session. Repeated calls return a local closed reply.""" + """Close the gateway session. Repeated calls return a local closed reply. + + Idempotent, including under concurrent calls: ``_closed`` is set + before the ``CloseSession`` RPC is awaited so a second coroutine + entering ``close()`` while the first RPC is in flight returns the + local closed reply instead of issuing a second ``CloseSession``. + """ if self._closed: return pb.CloseSessionReply( @@ -44,15 +54,14 @@ class Session: final_state=pb.SESSION_STATE_CLOSED, protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), ) + self._closed = True - reply = await self.client.close_session_raw( + return await self.client.close_session_raw( pb.CloseSessionRequest( session_id=self.session_id, client_correlation_id=client_correlation_id, ), ) - self._closed = True - return reply async def invoke(self, command: pb.MxCommand, *, correlation_id: str = "") -> pb.MxCommandReply: """Invoke a raw command and enforce gateway and MXAccess success.""" @@ -66,7 +75,15 @@ class Session: *, correlation_id: str = "", ) -> pb.MxCommandReply: - """Invoke a raw command and preserve the raw reply.""" + """Invoke a raw command and preserve the raw reply. + + Enforces gateway protocol success only — unlike :meth:`invoke`, it + does not run MXAccess-failure detection. An MXAccess HRESULT or + ``MxStatusProxy`` status failure is left embedded in the returned + reply and no ``MxAccessError`` is raised. Parity-test callers must + inspect ``protocol_status``, ``hresult``, and ``statuses`` on the + reply themselves. + """ return await self.client.invoke_raw( pb.MxCommandRequest( @@ -399,6 +416,3 @@ class Session: def _ensure_bulk_size(name: str, count: int) -> None: if count > MAX_BULK_ITEMS: raise ValueError(f"{name} bulk commands are limited to {MAX_BULK_ITEMS} item(s)") - - -from .client import GatewayClient # noqa: E402 diff --git a/clients/python/src/mxgateway/values.py b/clients/python/src/mxgateway/values.py index e9251bf..6c5c759 100644 --- a/clients/python/src/mxgateway/values.py +++ b/clients/python/src/mxgateway/values.py @@ -1,7 +1,20 @@ -"""MXAccess value conversion helpers.""" +"""MXAccess value conversion helpers. + +Value-mapping assumptions (see ``to_mx_value``): + +* A Python ``float`` maps to ``VT_R8`` / ``MX_DATA_TYPE_DOUBLE``. Only finite + values are accepted — ``nan``, ``inf`` and ``-inf`` raise ``ValueError`` + rather than being forwarded to MXAccess, which has no defined wire + representation for non-finite doubles. +* A Python ``bytes`` value maps to ``VT_RECORD`` / ``MX_DATA_TYPE_UNKNOWN`` + and is carried in ``raw_value``. This is an opaque pass-through: MXAccess + does not interpret the bytes. Pass ``data_type`` explicitly when a concrete + MXAccess type is required. +""" from __future__ import annotations +import math from collections.abc import Sequence from dataclasses import dataclass from datetime import datetime, timezone @@ -60,6 +73,7 @@ def to_mx_value(value: MxValueInput, *, data_type: str | None = None) -> pb.MxVa ) if isinstance(value, float): + _ensure_finite(value) return pb.MxValue( data_type=_data_type(data_type, pb.MX_DATA_TYPE_DOUBLE), variant_type="VT_R8", @@ -177,6 +191,8 @@ def _sequence_to_mx_value( return pb.MxValue(data_type=pb.MX_DATA_TYPE_INTEGER, array_value=array) if all(isinstance(item, float) for item in sequence): + for item in sequence: + _ensure_finite(item) array = pb.MxArray( element_data_type=pb.MX_DATA_TYPE_DOUBLE, variant_type="VT_ARRAY|VT_R8", @@ -232,3 +248,12 @@ def _data_type(name: str | None, default: int) -> int: if name is None: return default return pb.MxDataType.Value(name) + + +def _ensure_finite(value: float) -> None: + """Reject non-finite doubles, which MXAccess cannot represent on the wire.""" + + if not math.isfinite(value): + raise ValueError( + f"MxValue double inputs must be finite; got {value!r}", + ) diff --git a/clients/python/src/mxgateway_cli/commands.py b/clients/python/src/mxgateway_cli/commands.py index 93f3ba8..0aa89ee 100644 --- a/clients/python/src/mxgateway_cli/commands.py +++ b/clients/python/src/mxgateway_cli/commands.py @@ -18,6 +18,7 @@ from mxgateway.client import GatewayClient from mxgateway.errors import MxGatewayError from mxgateway.generated import mxaccess_gateway_pb2 as pb from mxgateway.options import ClientOptions +from mxgateway.session import Session from mxgateway.values import MxValueInput MAX_AGGREGATE_EVENTS = 10_000 @@ -383,8 +384,7 @@ async def _write2(**kwargs: Any) -> dict[str, Any]: async def _smoke(**kwargs: Any) -> dict[str, Any]: async with await _connect(kwargs) as client: session = await client.open_session(client_session_name=kwargs["client_name"]) - closed = False - try: + async with session: server_handle = await session.register(kwargs["client_name"]) item_handle = await session.add_item(server_handle, kwargs["item"]) await session.advise(server_handle, item_handle) @@ -399,9 +399,6 @@ async def _smoke(**kwargs: Any) -> dict[str, Any]: "itemHandle": item_handle, "events": [_message_dict(event) for event in events], } - finally: - if not closed: - await session.close() async def _connect(kwargs: dict[str, Any]) -> GatewayClient: @@ -419,9 +416,7 @@ async def _connect(kwargs: dict[str, Any]) -> GatewayClient: ) -def _session(client: GatewayClient, session_id: str): - from mxgateway.session import Session - +def _session(client: GatewayClient, session_id: str) -> Session: return Session(client=client, session_id=session_id) diff --git a/clients/python/tests/test_low_severity_findings.py b/clients/python/tests/test_low_severity_findings.py new file mode 100644 index 0000000..6616608 --- /dev/null +++ b/clients/python/tests/test_low_severity_findings.py @@ -0,0 +1,228 @@ +"""Regression tests for Client.Python low-severity code-review findings. + +Covers Client.Python-006 (concurrent-close idempotency), +Client.Python-007 (shared cancelling stream helper), +Client.Python-008 (non-finite float / bytes value mapping), and +Client.Python-011 (`success == 0` proto3-default ambiguity). +""" + +from __future__ import annotations + +import asyncio +import math +from typing import Any + +import pytest + +from mxgateway import ClientOptions, GalaxyRepositoryClient, GatewayClient +from mxgateway.errors import ensure_mxaccess_success, MxAccessError +from mxgateway.generated import mxaccess_gateway_pb2 as pb +from mxgateway.values import to_mx_value + + +# --- Client.Python-006: concurrent close() is idempotent ------------------- + + +class CountingChannel: + """A fake gRPC channel that records and stalls on close().""" + + def __init__(self) -> None: + self.close_calls = 0 + self._gate = asyncio.Event() + + async def close(self) -> None: + self.close_calls += 1 + # Yield control so a second concurrent close() can interleave at the + # exact point a check-then-set guard would have left the window open. + await self._gate.wait() + + +@pytest.mark.asyncio +async def test_gateway_client_concurrent_close_closes_channel_once() -> None: + channel = CountingChannel() + client = GatewayClient( + options=ClientOptions(endpoint="fake", plaintext=True), + stub=object(), + channel=channel, # type: ignore[arg-type] + ) + + first = asyncio.create_task(client.close()) + second = asyncio.create_task(client.close()) + await asyncio.sleep(0) # let both coroutines pass the guard if racy + + channel._gate.set() + await asyncio.gather(first, second) + + assert channel.close_calls == 1 + + +@pytest.mark.asyncio +async def test_galaxy_client_concurrent_close_closes_channel_once() -> None: + channel = CountingChannel() + client = GalaxyRepositoryClient( + options=ClientOptions(endpoint="fake", plaintext=True), + stub=object(), + channel=channel, # type: ignore[arg-type] + ) + + first = asyncio.create_task(client.close()) + second = asyncio.create_task(client.close()) + await asyncio.sleep(0) + + channel._gate.set() + await asyncio.gather(first, second) + + assert channel.close_calls == 1 + + +@pytest.mark.asyncio +async def test_session_concurrent_close_sends_one_close_session_rpc() -> None: + gate = asyncio.Event() + rpc_calls = 0 + + class StallingClient: + async def close_session_raw(self, request: Any) -> pb.CloseSessionReply: + nonlocal rpc_calls + rpc_calls += 1 + await gate.wait() + return pb.CloseSessionReply( + session_id=request.session_id, + final_state=pb.SESSION_STATE_CLOSED, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + ) + + from mxgateway.session import Session + + session = Session(client=StallingClient(), session_id="session-1") # type: ignore[arg-type] + + first = asyncio.create_task(session.close()) + second = asyncio.create_task(session.close()) + await asyncio.sleep(0) + + gate.set() + await asyncio.gather(first, second) + + assert rpc_calls == 1 + + +# --- Client.Python-007: shared cancelling stream helper -------------------- + + +@pytest.mark.asyncio +async def test_gateway_stream_iterator_cancels_call_on_task_cancellation() -> None: + """A cancelled gateway stream iterator must explicitly cancel the call.""" + + class CancellableStream: + def __init__(self) -> None: + self.cancelled = False + + def __aiter__(self) -> "CancellableStream": + return self + + async def __anext__(self) -> pb.MxEvent: + await asyncio.Event().wait() # blocks until cancelled + raise AssertionError("unreachable") + + def cancel(self) -> None: + self.cancelled = True + + from mxgateway.client import _canceling_iterator + + stream = CancellableStream() + iterator = _canceling_iterator(stream, "stream events") + + task = asyncio.create_task(anext(iterator)) + await asyncio.sleep(0) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + # aclose() unwinds the generator's finally block. + await iterator.aclose() + + assert stream.cancelled + + +# --- Client.Python-008: non-finite float and bytes value mapping ----------- + + +def test_to_mx_value_rejects_nan() -> None: + with pytest.raises(ValueError, match="finite"): + to_mx_value(float("nan")) + + +def test_to_mx_value_rejects_positive_infinity() -> None: + with pytest.raises(ValueError, match="finite"): + to_mx_value(float("inf")) + + +def test_to_mx_value_rejects_negative_infinity() -> None: + with pytest.raises(ValueError, match="finite"): + to_mx_value(float("-inf")) + + +def test_to_mx_value_accepts_finite_float() -> None: + assert to_mx_value(3.5).double_value == 3.5 + + +def test_to_mx_value_rejects_non_finite_float_in_sequence() -> None: + with pytest.raises(ValueError, match="finite"): + to_mx_value([1.0, math.inf]) + + +# --- Client.Python-011: success == 0 proto3-default ambiguity -------------- + + +def test_ensure_mxaccess_success_ignores_unpopulated_status_entry() -> None: + """A status entry left at proto3 defaults is not a real MXAccess failure. + + The gateway emits such a placeholder for a null MXSTATUS_PROXY COM entry + (``MxStatusProxyConverter.ConvertMany``): ``success`` stays 0 but the + entry carries no failure category. It must not raise ``MxAccessError``. + """ + + reply = pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_SUBSCRIBE_BULK, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + statuses=[ + pb.MxStatusProxy(), # all-default: success == 0, category UNSPECIFIED + pb.MxStatusProxy( # the gateway's null-entry placeholder + category=pb.MX_STATUS_CATEGORY_UNKNOWN, + detected_by=pb.MX_STATUS_SOURCE_UNKNOWN, + ), + ], + ) + + assert ensure_mxaccess_success("subscribe bulk", reply) is reply + + +def test_ensure_mxaccess_success_raises_on_populated_failure_status() -> None: + """A populated failure status (success == 0 with a failure category) raises.""" + + reply = pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_WRITE, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + statuses=[ + pb.MxStatusProxy( + success=0, + category=pb.MX_STATUS_CATEGORY_COMMUNICATION_ERROR, + ), + ], + ) + + with pytest.raises(MxAccessError): + ensure_mxaccess_success("write", reply) + + +def test_ensure_mxaccess_success_passes_when_status_reports_success() -> None: + reply = pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_WRITE, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + statuses=[ + pb.MxStatusProxy(success=1, category=pb.MX_STATUS_CATEGORY_OK), + ], + ) + + assert ensure_mxaccess_success("write", reply) is reply diff --git a/code-reviews/Client.Python/findings.md b/code-reviews/Client.Python/findings.md index d6a1233..ae27468 100644 --- a/code-reviews/Client.Python/findings.md +++ b/code-reviews/Client.Python/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `3cc53a8` | | Status | Reviewed | -| Open findings | 9 | +| Open findings | 0 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | Low | | Category | Documentation & comments | | Location | `clients/python/pyproject.toml:8,25`, `clients/python/src/mxgateway_cli/commands.py:25` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -48,13 +48,13 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `clients/python/src/mxgateway/__init__.py:27` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -78,13 +78,13 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `clients/python/src/mxgateway_cli/commands.py:386,402-404` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -108,13 +108,13 @@ | 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 | Open | +| 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:** _(open)_ +**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 @@ -123,13 +123,13 @@ | Severity | Low | | Category | Error handling & resilience | | Location | `clients/python/src/mxgateway/client.py:204-213` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -138,13 +138,13 @@ | Severity | Low | | Category | Correctness & logic bugs | | Location | `clients/python/src/mxgateway/values.py:62-67,83-88` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -168,13 +168,13 @@ | Severity | Low | | Category | Code organization & conventions | | Location | `clients/python/src/mxgateway/session.py:404`, `clients/python/src/mxgateway_cli/commands.py:422-425` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -183,13 +183,13 @@ | Severity | Low | | Category | Error handling & resilience | | Location | `clients/python/src/mxgateway/errors.py:122-148` | -| Status | Open | +| 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:** _(open)_ +**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 @@ -198,10 +198,10 @@ | Severity | Low | | Category | mxaccessgw conventions | | Location | `clients/python/src/mxgateway/client.py:84-108`, `clients/python/src/mxgateway/session.py:57-77` | -| Status | Open | +| 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:** _(open)_ +**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.