Resolve Client.Python-001/002/004/006/007/008/010/011/012 findings

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) <noreply@anthropic.com>

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.
 

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 = [

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:
+        async for item in call:
-            yield event
+            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
+        raise map_rpc_error(operation, 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
     finally:
         cancel = getattr(call, "cancel", None)
         if cancel is not None:

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

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()

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

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}",
+        )

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
+        async with session:
-        try:
             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):
+def _session(client: GatewayClient, session_id: str) -> Session:
-    from mxgateway.session import Session
-
     return Session(client=client, session_id=session_id)
 
 

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

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.