From 9430cb335959cb99e00c2ec1213a86780d9c5e64 Mon Sep 17 00:00:00 2001 From: Gotham-Zolio <18781106300@163.com> Date: Fri, 11 Sep 2026 16:56:05 -0400 Subject: [PATCH] docs(spec): correct six claims about behaviour the code does not have SPEC.md is the normative document, so a claim in it that the implementations do not honour is worse than the same claim anywhere else. An adversarial audit read every clause against the three codebases and six did not hold. The examples README's two-terminal recipe did not run as written, which matters more than it looks: that recipe is how a stranger checks their own client, so a broken one turns the conformance story into a claim again. Corrections are recorded in the document rather than applied silently, which is this specification's convention for anything it previously got wrong. Co-Authored-By: Claude Opus 5 (1M context) --- SPEC.md | 115 +++++++++++++++++++++++++++++++++++++-------- examples/README.md | 14 +++++- 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/SPEC.md b/SPEC.md index ac6ba0c..751ea6f 100644 --- a/SPEC.md +++ b/SPEC.md @@ -398,10 +398,30 @@ episode ended by the environment's own rules, `truncated` means it was cut short (time limit, external stop). A chunk is also flushed early when either flag is set, so a terminal transition is never buried inside a chunk. -`info` is a free-form map. When present, its values are expected to be -batched to `m` along a leading axis, and the server slices them per -environment; a value that is not `m`-shaped is passed through to every -environment unchanged. `{}` is valid and is what the reference clients send. +`info` is a free-form map. `{}` is valid and is what both reference clients +send. Anything else needs care, and the rule is narrower than it looks. + +> **Gap - a non-empty `info` can close the connection.** The server unbatches +> it by looking for the first value that is an ndarray and taking `m` from +> that array's leading axis, then slicing every ndarray value by it; a +> scalar or string sitting alongside is broadcast to all `m` environments, +> which is the documented behaviour. But if *no* value is an ndarray, the +> map is returned whole as a single per-environment entry regardless of `m` +> - `{"task": "pick"}` with `m` = 2 yields one entry, and so does a +> correctly batched msgpack *list* of length `m`, because a list is not an +> ndarray. The handler then asserts that the per-environment count equals +> the observation count, and an `AssertionError` there becomes a close with +> **1011** and the reason `Internal server error.` (the +> `assert len(info_list) == len(next_obs_list) or len(info_list) == 0` in +> `websocket_agent_server.py`'s connection handler, and `unbatch_aggregate` +> in `plugrl-server`'s `common/data_utils.py`). +> +> So a non-empty `info` is safe only when at least one of its values is an +> ndarray of length `m`, or when `m` is 1, where the mismatch cannot arise. +> The previous wording of this paragraph - that a value which is not +> `m`-shaped is passed through to every environment unchanged - is true only +> in the first of those cases. This has not bitten anyone because the +> reference clients send `{}`. #### Terminal observations @@ -532,6 +552,19 @@ environment it has no step state for. That condition has exactly one cause - the connection was replaced mid-run - and storing the transition silently puts a hole in the training data that nothing downstream can detect. +> **Gap - the reference client breaks this on one path.** +> `plugrl-env-client` run with `--reconnect-on-server-stop` resends the +> `feedback` it was holding instead of dropping it, when the close it hit was +> the server's `plugrl-server-stop`. In `websocket_env_client_agent.py`, +> `feedback()`'s `SERVER_STOP_REASON` branch `continue`s its retry loop, +> which opens a new connection, reads fresh metadata, and re-sends the same +> payload - the exact resend the historical note below says this section +> exists to prevent. Every other close path in that method returns and drops +> the transition, and `tests/test_reconnect_drops_feedback.py` covers three +> of them (a keepalive timeout, a plain 1000 close, a resync) but not this +> one. It is the only path in the reference client that violates the **MUST** +> above. + > **Where reconnects come from.** Nothing in this protocol causes them and > nothing in it can prevent them: a suspended laptop, a flaky link, an > operator restarting the server. The rule above is written in terms of the @@ -574,16 +607,38 @@ A client conforms to version 1 if it: `feedback` for an `action` that arrived on an earlier connection; - [ ] treats a text frame as a fatal error. -`examples/conformance_server.py` checks every clause above that is visible -from the server's side of the wire, and reports what it cannot enforce as a -note rather than a failure. Both reference clients pass it with one note: -they send `text` as a msgpack string array rather than a ` **Correction, 2026-09-11.** This paragraph used to say version 1 "has no +> version field on the wire" and attributed to section 5.1 a remark that the +> metadata message was the obvious place for one. Both halves went stale on +> 2026-09-10, when the metadata message gained contents: `protocol_version` +> has been in it on every connection since, in `plugrl-server` and in +> `examples/conformance_server.py` alike, and section 5.1 lists it as a key +> the server sends rather than as a suggestion. What survives is the absence +> of negotiation, which is what the sentence was reaching for. Changing any of the four `message_type` strings, the two close reasons, the ndarray key names, the action array's axis order, or the meaning of -`rewards` is a **breaking** change. `tests/test_wire_format.py` and -`tests/test_spec_conformance.py` exist so that such a change fails a test -rather than a deployment. +`rewards` is a **breaking** change. + +The first three are pinned in this repository, and a change to them fails a +test rather than a deployment: `tests/test_wire_format.py` holds the four +`message_type` strings and the two close reasons, +`tests/test_spec_conformance.py` the four ndarray key names. The last two +are not, and cannot be - this is the codec repository, and the axis order +and the chunk-sum rule are properties of how the two sides behave, not of +what the packer emits. Transposing the action chunk or redefining `rewards` +as the last step's reward passes every test here. They are covered instead +by `plugrl-env-client`'s +`tests/test_protocol_alternation.py::TestChunkSemantics`, which is the same +file section 8 points at. diff --git a/examples/README.md b/examples/README.md index b62f592..bcb57ab 100644 --- a/examples/README.md +++ b/examples/README.md @@ -16,11 +16,21 @@ broke. `plugrl-server` is deliberately forgiving — it validates the trade for a training run and the wrong one for someone bringing up a client in a new language. +The two Python files here need `websockets>=13`, which is **not** a +dependency of `plugrl-protocol` - installing the package does not get you +one. `conformance_server.py` imports `websockets.asyncio.server` and +`raw_client.py` imports `websockets.sync.client`; the `asyncio` layout +arrived in 13.0, so websockets 12 fails on the import. Ask for it +explicitly, from this directory: + ```bash -python conformance_server.py --port 8000 --steps 20 & +uv run --with 'websockets>=13' conformance_server.py --port 8000 --steps 20 & ./plugrl_client 127.0.0.1 8000 20 ``` +`pip install 'websockets>=13'` and then `python conformance_server.py ...` +works just as well. The C++ client needs nothing. + It exits non-zero on a violation, so it can sit in a CI job. Both clients here pass it with one note: they send `text` as a msgpack string array rather than the `=13' raw_client.py --host 127.0.0.1 --port 8000 --steps 120 ``` ## What the protocol asks of a client