dohertj2/mxaccessgw
1
0
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Resolve Client.Python-022..026: TLS-by-default, batch CLI, README

Browse Source 
Client.Python-022  README CLI examples for stream-alarms and
                   acknowledge-alarm now use the correct flags;
                   regression test parses every documented line through
                   Click.
Client.Python-023  Re-applied Client.Python-013 — _use_plaintext drops
                   the silent localhost / 127.0.0.1 auto-downgrade
                   branch; --plaintext and --tls are mutually exclusive
                   and TLS is the default.
Client.Python-024  batch dispatch routes through main.main(...,
                   standalone_mode=False) under a redirected stdout
                   instead of click.testing.CliRunner; recursive batch
                   lines are rejected outright.
Client.Python-025  Added behavioural tests for the five bulk SDK methods,
                   stream_alarms, and the new CLI subcommands.
Client.Python-026  _bench_read_bulk hoists 'import time' to module scope
                   and logs cleanup failures instead of swallowing them.

All resolved at 2026-05-24; python -m pytest is 65/65 green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Joseph Doherty
2026-05-24 08:50:27 -04:00
parent 4a0f88b17d
commit f5b50c4484
4 changed files with 1002 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
code-reviews/Client.Python/findings.md
+117 -6
View File
@@ -7,7 +7,7 @@
| Review date | 2026-05-24 |
| Commit reviewed | `42b0037` |
| Status | Re-reviewed |
| Open findings | 5 |
| Open findings | 0 |
## Checklist coverage
@@ -835,7 +835,7 @@ parity fix.
| Severity | High |
| Category | Documentation & comments |
| Location | `clients/python/README.md:201-202`, `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:389-420` |
| Status | Open |
| Status | Resolved |
**Description:** The README CLI examples added by commit `8738735` for the
new alarm subcommands cite flags the CLI does not accept:
@@ -868,6 +868,19 @@ rename the CLI option to `--alarm-reference` and add a test that copy-pastes
the README examples through `CliRunner` to assert they parse. Option (1) is
the smaller change.
**Resolution:** 2026-05-24 — Fixed the README examples to match the
implementation (option 1, smaller change). `clients/python/README.md:201-202`
now reads `mxgw-py stream-alarms --max-messages 1 --json` and
`mxgw-py acknowledge-alarm --reference "\\Galaxy\Area001.Pump001.PumpFault" --json`
— `--session-id` is dropped from both lines (the alarm feed is gateway-served,
session-less) and `--alarm-reference` is renamed to the real `--reference` flag.
Regression test
`tests/test_review_findings_022_to_026.py::test_readme_alarm_examples_parse_against_cli`
extracts every `mxgw-py …` line from the README, appends `--help` so only the
parser runs, and asserts that no example produces a `no such option` Click error.
Failed before the fix (the original `stream-alarms --session-id <id> …` line
emitted `Error: No such option: --session-id`), passes after.
### Client.Python-023
| Field | Value |
@@ -875,7 +888,7 @@ the smaller change.
| Severity | Medium |
| Category | Security |
| Location | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:901-906` |
| Status | Open |
| Status | Resolved |
**Description:** Client.Python-013 (severity Medium, Security) was marked
**Resolved** on 2026-05-20 with the explicit claim that the silent
@@ -919,6 +932,31 @@ is marked Resolved with a 2026-05-20 commit reference, do **not** silently
re-resolve this finding — keep it Open with a fresh ID so the regression
audit trail is preserved.
**Resolution:** 2026-05-24 — Re-applied the Client.Python-013 fix on the
renamed CLI module. Dropped the `endpoint.startswith("localhost:") or
endpoint.startswith("127.0.0.1:")` auto-plaintext branch from
`_use_plaintext` in `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py`.
TLS is now the default and `--plaintext` is the only way to opt in to
plaintext; `--tls` is accepted as a redundant affirmation and the two
flags combined raise `click.UsageError`. Regression tests live in
`tests/test_review_findings_022_to_026.py`:
`test_use_plaintext_does_not_auto_downgrade_for_localhost_endpoint` and
`test_use_plaintext_does_not_auto_downgrade_for_loopback_ipv4_endpoint`
exercise the bare-endpoint path,
`test_use_plaintext_requires_explicit_plaintext_flag` and
`test_use_plaintext_tls_flag_explicitly_disables_plaintext` pin the explicit
opt-in / opt-out contract,
`test_use_plaintext_rejects_plaintext_and_tls_combined` asserts mutual
exclusivity, and
`test_cli_localhost_endpoint_with_no_flags_uses_tls_channel` is an
end-to-end CliRunner test that intercepts `GatewayClient.connect` and
asserts the resolved `ClientOptions.plaintext` is `False` for a
`localhost:5000` endpoint without `--plaintext`. All five tests failed
against the pre-fix source and pass against the fix. **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).
### Client.Python-024
| Field | Value |
@@ -926,7 +964,7 @@ audit trail is preserved.
| Severity | Medium |
| Category | Code organization & conventions |
| Location | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:13,48-119` |
| Status | Open |
| Status | Resolved |
**Description:** The new `batch` subcommand (commit `71d2c39`) implements
the cross-language batch protocol by importing `click.testing.CliRunner`
@@ -965,6 +1003,33 @@ batch loop can interleave inner-command output with the
a regression test that drives `batch` with `batch\n` on stdin and asserts
recursive invocation is either rejected or correctly bounded.
**Resolution:** 2026-05-24 — Removed the `from click.testing import CliRunner`
import and the `CliRunner()` instantiation from
`clients/python/src/zb_mom_ww_mxgateway_cli/commands.py`. The `batch`
command body now dispatches each stdin line through a new
`_dispatch_batch_line` helper that calls `main.main(args=…,
standalone_mode=False, prog_name="mxgw-py")` directly and captures the
subcommand's stdout via `contextlib.redirect_stdout(io.StringIO())`. Click
exit conditions (`click.exceptions.Exit`, `click.ClickException`,
`SystemExit`) are caught and rendered as
`{"error": …, "type": …}` JSON; arbitrary exceptions are caught with a
broad `except Exception` so the batch loop never dies. A nested `batch`
line is rejected outright with a `RecursiveBatchError` JSON record before
the dispatcher runs, eliminating the silent-recursive-spawn footgun the
original `CliRunner.invoke(main, ["batch"], …)` path enabled. Regression
tests:
`tests/test_review_findings_022_to_026.py::test_batch_command_does_not_use_clirunner_in_production`
asserts the production module no longer imports `from click.testing` or
calls `CliRunner(`; and
`test_batch_recursive_batch_line_is_bounded` drives a `batch\nversion --json\n`
stdin payload and asserts the recursive `batch` line emits an error JSON
record rather than silently exiting. The pre-existing batch tests
(`test_batch_runs_version_command_and_writes_eor`,
`test_batch_terminates_on_empty_line`,
`test_batch_continues_after_error_line`) still pass against the new
implementation, confirming the wire-level contract (one EOR per line,
clean JSON error blocks) is preserved.
### Client.Python-025
| Field | Value |
@@ -972,7 +1037,7 @@ recursive invocation is either rejected or correctly bounded.
| Severity | Low |
| Category | Testing coverage |
| Location | `clients/python/tests/test_cli.py`, `clients/python/src/zb_mom_ww_mxgateway/{client.py,session.py}`, `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py` |
| Status | Open |
| Status | Resolved |
**Description:** Commits `6add4b4` and `828e3e6` added five new SDK
methods (`Session.read_bulk`, `Session.write_bulk`, `Session.write2_bulk`,
@@ -1020,6 +1085,32 @@ applied to the renamed bench). At minimum, add a request-shape test for
`write_secured_bulk` since the secured family is the highest-risk
parity surface.
**Resolution:** 2026-05-24 — Added behavioural test coverage for the five
new bulk SDK methods, `stream_alarms`, and the new CLI subcommand bodies
in `tests/test_review_findings_022_to_026.py`. Request-shape tests
(`test_session_read_bulk_sends_expected_request_shape`,
`test_session_write_bulk_sends_expected_request_shape`,
`test_session_write2_bulk_sends_expected_request_shape`,
`test_session_write_secured_bulk_sends_expected_request_shape`,
`test_session_write_secured2_bulk_sends_expected_request_shape`) drive
each `Session.*_bulk` method against a fake `Invoke` stub and assert
the captured `MxCommand`'s `kind`, sub-message, `server_handle`, and
per-entry fields (including `current_user_id` / `verifier_user_id`
on the secured family — the highest-risk parity surface the finding
calls out). `test_stream_alarms_yields_feed_messages_and_cancels_on_close`
covers the `GatewayClient.stream_alarms` happy path including the
`_canceling_alarm_feed_iterator` cancel-on-close contract and the
authorization metadata header. CLI happy-path tests
(`test_cli_read_bulk_happy_path`, `test_cli_write_bulk_happy_path`,
`test_cli_stream_alarms_happy_path`, `test_cli_acknowledge_alarm_happy_path`)
each drive their subcommand through `CliRunner` against a fake stub
injected via a monkeypatched `GatewayClient.connect` and assert the
emitted JSON shape and that the captured RPC request carries the
expected fields. The four CLI happy-path tests passed even before any
production fix (the implementations were correct; the finding is a
coverage gap), but they now exist as regression guards against future
drift. No source change — pure coverage finding.
### Client.Python-026
| Field | Value |
@@ -1027,7 +1118,7 @@ parity surface.
| Severity | Low |
| Category | Correctness & logic bugs |
| Location | `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py:674-738` |
| Status | Open |
| Status | Resolved |
**Description:** Two minor quality issues in the new `_bench_read_bulk`
body (commit `6add4b4`):
@@ -1060,3 +1151,23 @@ module-level `logger = logging.getLogger(__name__)`. No behavioural
change in the happy path; failure path becomes diagnosable. No new test
required for the import hoist; the logger change is exercised by the
existing bench smoke test once `caplog` is added to the test signature.
**Resolution:** 2026-05-24 — Hoisted `import time` to the module-level
import block in `clients/python/src/zb_mom_ww_mxgateway_cli/commands.py`
alongside the existing standard-library imports; the function-local
`import time` line at the top of `_bench_read_bulk` is gone. Added a
module-level `logger = logging.getLogger(__name__)` and rewrote the two
`finally` cleanup blocks to bind the swallowed exception and log it at
`WARNING` level — `unsubscribe_bulk` failures now emit
`"bench-read-bulk: unsubscribe_bulk cleanup failed: %s"` and the
`session.close()` failure path emits the equivalent — so a future
regression in the cleanup path is diagnosable at the next benchmark run
rather than silently corrupting subscription bookkeeping. Regression
tests in `tests/test_review_findings_022_to_026.py`:
`test_commands_module_imports_time_at_module_scope` uses
`inspect.getsource(_bench_read_bulk)` to assert no function-local
`import time` line, and asserts the module exposes `time` at module
scope; `test_commands_module_bench_read_bulk_does_not_use_bare_except_pass`
greps the function source for the `except Exception:\n pass` pattern
and rejects it. Both tests failed against the pre-fix source and pass
against the fix.