Repository navigation
Conversation
`*michnovka#4*ZONE*22##` crashes the gateway's thermoregulation daemon (bt_termo) and reboots an F461 about three minutes later. own-firmware-oracle lists `*michnovka#4*2*22##` and `*michnovka#4*#0*22##` as crash frames for F461 020011 (tools/expected_divergences.yaml), and the live F461 2.0.11 rebooted after each of three occurrences. OWNd parsed the frame as a valid OWNHeatingCommand and every send path wrote it. OWNCommandSession._locked_send (used by send, send_to_gateway, keepalive and AsyncTcpTransport.send) and AsyncSerialTransport.send now refuse any WHO 4 dimension 22 frame with OWNProtocolError(reason="destructive_frame") before parsing, reconnecting or writing. The frame is serialised once so the string checked is the string written. Parsing is unchanged: the frame can still be read from the bus. The detector is_who4_dimension_22() is exported from OWNd.message. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
GreenGrassBlueOcean
left a comment
There was a problem hiding this comment.
Refusing WHO 4 Dimension 22 inside OWNd's outbound send layer—while keeping OWNCommand.parse / OWNMessage.parse permissive for frames observed on the bus—is the right architectural boundary.
Requesting changes mainly for one critical false-positive bug in _WHO4_DIMENSION_22_REGEX, along with three transport/session consistency and edge-case issues:
-
[Blocker] Unanchored
_WHO4_DIMENSION_22_REGEX.search(chunk)rejects valid WHO 1, 2, 4, 16, and 22 commands (OWNd/message/heating.py)
Within a##-delimited frame,*#4*only meansWHO = 4at the first*of the frame. Mid-frame,*#4*/*#04*encodesWHERE = #4(Group 4 in WHO 1/2/16/22 or Central-Unit Zone 4 in WHO 4) orDIMENSION = #4(Dimension 4 write in any subsystem). Because.search(chunk)is unanchored, it slides past*#WHO*and falsely refuses valid frames emitted by at least 9OWNdbuilders (e.g.OWNAutomationCommand.set_shutter_level("#4", 22)->*#2*#4*#11#001*22##,OWNHeatingCommand.set_holiday_end_date("#4", 22, 10, 2026)->*#4*#4*#30*22*10*2026##,OWNSoundCommand.set_volume("#4", 22)->*#16*#4*#1*22##) as well as any rawDIMENSION = #4write whose second value is22(*#1*14*#4*1*22##). -
[Medium] Serial guard bypasses &
bytesinputs (OWNd/transport/serial.py,OWNd/message/heating.py)AsyncSerialTransport.sendhas noOWNMessage.parse/is_valid_framecheck and appendsb"##"if absent, so whitespace/sign forms ("*#4*2*22 ","*#4*2*22 ##","*#4*2* 22##","*#4*2*+22##") and malformed single-#prefixes ("*1*1*11#*#4*2*22##") bypass the current regex and are written to serial.is_who4_dimension_22(frame: object)acceptsbytes, which only matched previously by accident of unanchored.search()inside"b'*#4*2*22##'"; once anchored,(bytes, bytearray, memoryview)must be decoded explicitly before.split("##").
-
[Medium]
OWNTransport.send()Liskov contract split betweenAsyncTcpTransportandAsyncSerialTransport(OWNd/transport/serial.pyvsOWNd/transport/tcp.py)
Both implement abstractOWNTransport.send(message, is_status_request=False). On*#4*2*22##, connectedAsyncTcpTransport.sendlogsERRORand returnsNone(and raisesRuntimeErrorfirst if disconnected), whereasAsyncSerialTransport.senddoes not log and raisesOWNProtocolErroreven when disconnected. -
[Medium]
OWNCommandSession.send_to_gateway()connects and authenticates before refusing locally (OWNd/connection/command_session.py:64-81)
send_to_gatewayexecutesawait connection.connect(...)prior tosend(), opening a live TCP socket + nonce/HMAC handshake (occupying single-slot gateways like MH200N / F453AV, or stalling across 5 retries ~13.3 s and raisingOWNConnectionErroron unreachable hosts) for a frame that will never be written. -
[Low] Protocol attribution, single-serialization hygiene, and PR description note
- Per the OpenWebNet WHO 4 specification (pp. 66-68), Dimension 22 is published split-unit IR control (
*#4*3#Z#N*22##/*#4*3#Z#N*#22*MOD*SP*VEL*SWING##); worth noting in comments/changelog while clarifying error text onAsyncSerialTransport(Legrand 3578 USB/ZigBee dongle, which has nobt_termo). - Reuse the pre-serialized
frame/textstring inOWNProtocolError(frame=...), logs, andOWNMessage.parse(...). - (Minor note on the PR summary:
MyHOMEalso guardsws_bus_monitor_sendincustom_components/myhome/websocket.py, not onlysend_message—both can switch toOWNd.message.is_who4_dimension_22once this lands.)
- Per the OpenWebNet WHO 4 specification (pp. 66-68), Dimension 22 is published split-unit IR control (
Detailed explanations and concrete fix suggestions are inline below.
| # WHO 4 DIMENSION 22 (``*#4*ZONE*22##``) kills the gateway's thermoregulation | ||
| # daemon (bt_termo) and reboots an F461 about 3 minutes later (firmware oracle | ||
| # crash_frames; live F461 2.0.11). Every send path refuses it, in any shape: | ||
| # request, write (``*#22*``) or reply, with or without a dimension parameter. | ||
| _WHO4_DIMENSION_22_REGEX = re.compile(r"\*#0*4\*[^*]*\*#?0*22(?:[#*]|$)") | ||
|
|
||
|
|
||
| def is_who4_dimension_22(frame: object) -> bool: | ||
| """True if ``frame`` (or any frame in it) is a WHO 4 DIMENSION 22 frame.""" | ||
| return any( | ||
| _WHO4_DIMENSION_22_REGEX.search(chunk) for chunk in str(frame).split("##") | ||
| ) |
There was a problem hiding this comment.
[Blocker] Unanchored .search(chunk) falsely blocks valid WHO 1, 2, 4, 16, and 22 commands (+ whitespace/bytes edge cases)
In OpenWebNet frame syntax, * is both the leading delimiter of a frame and the intra-frame field separator. Within a ##-delimited chunk, *#4* is WHO = 4 only at the first * of the frame; mid-frame, *#4* (or *#04*) legitimately encodes:
WHERE = #4— Group 4 in Lighting (WHO 1), Automation (WHO 2), Sound (WHO 16 / WHO 22), or Central-Unit Zone 4 in Thermoregulation (WHO 4).DIMENSION = #4— a write to Dimension 4 in any subsystem (e.g. WHO 1 dimmer preset or WHO 22 Low tones / Bass).
Because _WHO4_DIMENSION_22_REGEX.search(chunk) is not anchored to the start of chunk, it slides past *#WHO* and falsely rejects valid commands produced by at least 9 OWNd builders:
OWNd Builder Call |
Emitted Wire Frame | False Match Substring |
|---|---|---|
OWNAutomationCommand.set_shutter_level("#4", 22) |
*#2*#4*#11#001*22## |
*#4*#11#001*22 |
OWNLightingCommand.set_variable_timer("#4", 22, 0, 0) |
*#1*#4*#2*22*0*0## |
*#4*#2*22* |
OWNLightingCommand.set_hsv_color("#4", 22, 100, 50) |
*#1*#4*#12*22*100*50## |
*#4*#12*22* |
OWNSoundCommand.set_volume("#4", 22) |
*#16*#4*#1*22## |
*#4*#1*22 |
OWNSoundDiffusionCommand.set_volume("#4", 22) |
*#22*#4*#1*22## |
*#4*#1*22 |
OWNSoundDiffusionCommand.set_preset("#4", 22) |
*#22*#4*#19*22## |
*#4*#19*22 |
OWNHeatingCommand.set_holiday_end_date("#4", 22, 10, 2026) |
*#4*#4*#30*22*10*2026## |
*#4*#30*22* |
OWNHeatingCommand.set_holiday_end_time("#4", 22, 30) |
*#4*#4*#31*22*30## |
*#4*#31*22* |
OWNHeatingCommand.set_timed_manual_end_time("#4", 22, 0) |
*#4*#4*#32*22*00## |
*#4*#32*22* |
Raw Dimension #4 write (WHO 1 / WHO 22) |
*#1*14*#4*1*22## |
*#4*1*22 |
Two additional detector edge cases to close at the same time:
- Whitespace /
+sign tokens on serial:AsyncSerialTransport.senddoes not runis_valid_frameand appendsb"##"if missing. Inputs like"*#4*2*22 ","*#4*2*22 ##","*#4*2* 22##", and"*#4*2*+22##"currently returnFalsefromis_who4_dimension_22(the character after22is' ', not[#*]), yet gateway Catoi/strtolparses" 22"/"22 "/"+22"as22. bytespassed toframe: object:str(b"*#4*2*22##")yields"b'*#4*2*22##'", which only matched previously because.search()was unanchored. Once anchored to the first*, decode(bytes, bytearray, memoryview)before.split("##").- Protocol attribution: Per the WHO 4 specification (pp. 66-68), Dimension 22 is published split-unit IR control (
*#4*3#Z#N*22##/*#4*3#Z#N*#22*MOD*SP*VEL*SWING##), worth noting in the comment alongside the F461bt_termocrash.
| # WHO 4 DIMENSION 22 (``*#4*ZONE*22##``) kills the gateway's thermoregulation | |
| # daemon (bt_termo) and reboots an F461 about 3 minutes later (firmware oracle | |
| # crash_frames; live F461 2.0.11). Every send path refuses it, in any shape: | |
| # request, write (``*#22*``) or reply, with or without a dimension parameter. | |
| _WHO4_DIMENSION_22_REGEX = re.compile(r"\*#0*4\*[^*]*\*#?0*22(?:[#*]|$)") | |
| def is_who4_dimension_22(frame: object) -> bool: | |
| """True if ``frame`` (or any frame in it) is a WHO 4 DIMENSION 22 frame.""" | |
| return any( | |
| _WHO4_DIMENSION_22_REGEX.search(chunk) for chunk in str(frame).split("##") | |
| ) | |
| # WHO 4 DIMENSION 22 (published split-unit control ``*#4*3#Z#N*22##``, or | |
| # ``*#4*ZONE*22##``) kills the gateway's thermoregulation daemon (bt_termo) | |
| # and reboots Linux gateways such as F461 2.0.11 ~3 minutes later (firmware | |
| # oracle crash_frames; live F461 2.0.11). Anchored to the first '*' of each | |
| # '##'-delimited frame so mid-frame WHERE=#4 or DIMENSION=#4 tokens never match. | |
| _WHO4_DIMENSION_22_REGEX = re.compile( | |
| r"^[^*]*\*#\s*\+?0*4\s*\*[^*]*\*\s*#?\s*\+?0*22(?:\s*$|[#*\s])" | |
| ) | |
| def is_who4_dimension_22(frame: object) -> bool: | |
| """True if ``frame`` (or any ``##``-delimited frame in it) is WHO 4 DIMENSION 22.""" | |
| raw = ( | |
| bytes(frame).decode("latin-1") | |
| if isinstance(frame, (bytes, bytearray, memoryview)) | |
| else str(frame) | |
| ) | |
| return any(_WHO4_DIMENSION_22_REGEX.search(chunk) for chunk in raw.split("##")) |
| text = str(message) | ||
| if is_who4_dimension_22(text): | ||
| raise OWNProtocolError( | ||
| f"Refusing to send `{message}`: WHO 4 dimension 22 crashes the " | ||
| "gateway's thermoregulation and reboots it", | ||
| frame=str(message), | ||
| reason="destructive_frame", | ||
| ) | ||
| if not self._connected or self._writer is None: | ||
| raise RuntimeError("Serial transport is not connected") |
There was a problem hiding this comment.
OWNTransport.send() contract divergence + missing serial frame validation
-
Liskov contract split vs
AsyncTcpTransport.send(OWNd/transport/tcp.py:93-98):
Both classes implement abstractOWNTransport.send(message, is_status_request=False) -> list[OWNMessage | str] | bool | None(tests/typing/consumer_send.py:59), yet they behave oppositely on*#4*2*22##:- Connected:
AsyncTcpTransport.sendlogsERROR, sets_command_session.last_error, and returnsNone, whereasAsyncSerialTransport.senddoes not log and raisesOWNProtocolError. - Disconnected:
AsyncTcpTransport.sendraisesRuntimeError("TCP transport is not connected")first, whereasAsyncSerialTransport.sendraisesOWNProtocolErrorbefore checkingself._connected. - Note that
AsyncSerialTransport.senddoes not always raise on send failures—it already returnsNoneon NACK (lines 120-121), unexpected signaling (lines 122-123), and command timeout (lines 167-173). Please align bothOWNTransportimplementations (e.g. logERRORand returnNoneby default, or add*, raise_on_error: bool = Falseconsistently acrossOWNTransport/AsyncTcpTransport/AsyncSerialTransport, with consistent ordering relative to the"not connected"RuntimeError).
- Connected:
-
Outbound frame validation on serial:
UnlikeOWNCommandSession._locked_send,AsyncSerialTransport.senddoes not validate outbound frames withOWNMessage.parse/is_valid_frame. A malformed single-#concatenation such as"*1*1*11#*#4*2*22##"is rejected on TCP (invalid_frame), but on serial.split("##")produces a single chunk starting with*1*that would bypass an anchored detector and be written to the port. Rejecting invalid outbound frames (or matchingOWNCommandSession'sis_valid_frameguard) closes this gap. -
Single-serialization & hardware wording:
- Reuse
texton line 154 (frame=text) and line 163 (OWNMessage.parse(text, gateway=self.port)) instead of callingstr(message)again. AsyncSerialTransporttargets USB/serial interfaces such as the Legrand 3578 ZigBee dongle (which has nobt_termodaemon); phrasing the error as "WHO 4 dimension 22 crashes gateway thermoregulation (bt_termo) on Linux IP gateways" keeps the reason accurate across transports.
- Reuse
| # Serialised once: the frame checked is the frame written. | ||
| frame = str(message) | ||
| if is_who4_dimension_22(frame): | ||
| # Never written, whatever the caller: it reboots the gateway. | ||
| err: OWNError = OWNProtocolError( | ||
| f"Refusing to send `{message}`: WHO 4 dimension 22 crashes the " | ||
| "gateway's thermoregulation and reboots it", | ||
| frame=str(message), | ||
| reason="destructive_frame", | ||
| ) | ||
| self._last_error = err | ||
| self._logger.error( | ||
| "%s Refusing to send `%s`: WHO 4 dimension 22 reboots the gateway.", | ||
| self._log_id, | ||
| message, | ||
| ) | ||
| if raise_on_error: | ||
| raise err | ||
| return None |
There was a problem hiding this comment.
Reuse frame consistently so the checked string is also the parsed and logged string
Line 230 notes # Serialised once: the frame checked is the frame written., and line 358 now writes frame.encode(). However, lines 235, 237 (frame=str(message)), 244, and below at lines 252 (OWNMessage.parse(str(message), gateway=self._gateway)), 258, 259, and 267 still re-evaluate str(message) / f"{message}". If a caller passes an object with a custom/non-idempotent __str__, OWNMessage.parse validates a different string than frame.encode(). Reusing frame in all of those call sites makes the single-serialization guarantee hold end-to-end.
| DIMENSION_22_FRAMES = ( | ||
| "*#4*2*22##", | ||
| "*#4*#0*22##", | ||
| "*#4*#1*22##", | ||
| "*#4*2#1*22##", | ||
| "*#4**22##", | ||
| "*#4*2*22#1##", | ||
| "*#4*2*#22*1##", | ||
| "*#4*2*22*1##", | ||
| "*#04*2*022##", | ||
| "*1*1*11##*#4*2*22##", | ||
| ) | ||
|
|
||
| # Zone 22, other dimensions, and dimension 22 of other WHOs stay sendable. | ||
| SAFE_FRAMES = ( | ||
| "*#4*22##", | ||
| "*#4*22*0##", | ||
| "*#4*#22*14##", | ||
| "*#4*2*20##", | ||
| "*#4*2*220##", | ||
| "*#4*2*0*0220##", | ||
| "*#13**22##", | ||
| "*#1*22*1##", | ||
| "*4*303*22##", | ||
| ) |
There was a problem hiding this comment.
Expand DIMENSION_22_FRAMES and SAFE_FRAMES to cover #4 builders, split addresses, WHO 1004, whitespace, and bytes
Please add regression cases for the false-positive and bypass vectors:
SAFE_FRAMES(currently falsely flagged by the unanchored regex):WHERE = #4commands:"*#2*#4*#11#001*22##","*#1*#4*#2*22*0*0##","*#1*#4*#12*22*100*50##","*#16*#4*#1*22##","*#22*#4*#1*22##","*#22*#4*#19*22##","*#4*#4*#30*22*10*2026##","*#4*#4*#31*22*30##","*#4*#4*#32*22*00##"(plus a test exercising the correspondingOWN*Commandbuilder methods directly),DIMENSION = #4write with second value22:"*#1*14*#4*1*22##",- Other subsystems with
22:"*#1004*2*22##"(WHO 1004 diagnostic fault mask) and"*#22*3#1#1*1##"(WHO 22 Sound Diffusion).
DIMENSION_22_FRAMES/ detector tests:- Canonical WHO 4 split-unit forms from the spec:
"*#4*3#2#1*22##","*#4*3#2#1*#22*0***##","*#4*45#1*#22*3**1*1##", - Whitespace / sign / un-terminated variants:
"*#4*2*22 ","*#4*2*22 ##","*#4*2* 22##","*#4*2*+22##", bytesinput (b"*#4*2*22##") in a detector test so both branches ofis_who4_dimension_22(frame: object)maintain 100% coverage.
- Canonical WHO 4 split-unit forms from the spec:
| @pytest.mark.asyncio | ||
| async def test_send_to_gateway_refuses() -> None: | ||
| gateway = OWNGateway({"address": "192.0.2.1", "port": 20000, "password": "12345"}) | ||
| writer = _Writer() | ||
|
|
||
| async def fake_connect(self: OWNCommandSession, *, raise_on_error: bool = False): | ||
| del raise_on_error | ||
| self._stream_reader = asyncio.StreamReader() | ||
| self._stream_writer = writer | ||
| return {"Success": True} | ||
|
|
||
| with patch.object(OWNCommandSession, "connect", fake_connect): | ||
| with pytest.raises(OWNProtocolError): | ||
| await OWNCommandSession.send_to_gateway( | ||
| "*#4*2*22##", gateway, raise_on_error=True | ||
| ) | ||
| assert await OWNCommandSession.send_to_gateway("*#4*2*22##", gateway) is None | ||
| assert writer.written == [] |
There was a problem hiding this comment.
OWNCommandSession.send_to_gateway() connects and authenticates before refusing locally (OWNd/connection/command_session.py:64-81)
Notice this test has to patch OWNCommandSession.connect with fake_connect: in OWNCommandSession.send_to_gateway (command_session.py:64-81), await connection.connect(...) runs before await connection.send(message, ...).
As a result, calling send_to_gateway("*#4*2*22##", gateway):
- Opens a live TCP socket, runs full Open/HMAC authentication, occupies a gateway command slot (which on single-session gateways like
MH200,MH200N,MH201,F453,F453AV,H4684can starve or drop concurrent clients), and immediately closes it without sending anything. - Blocks across
MAX_CONNECT_ATTEMPTS = 5(~13.3 s exponential backoff) and raisesOWNConnectionError/OWNTimeoutErrorif the gateway is unreachable withraise_on_error=True, instead of failing fast withOWNProtocolError(reason="destructive_frame").
Checking is_who4_dimension_22 (or running pre-send validation) before await connection.connect(...) in send_to_gateway lets this test assert connect.assert_not_called(), matching test_command_session_refuses_before_reconnecting above.
| - `WHAT 39` end of movement (`*1*39*WHERE##`) from WHO 1 motion detectors: `message_type` `motion_detected` with `motion` `False`, new `OWNLightingEvent.motion_end`, `is_sensor` `True`, `is_on` `None`. It was `unknown_state` `39` before (`MyHOME#777`). | ||
|
|
||
| ### Fixed | ||
| - **WHO 4 Dimension 22 is never sent**: `OWNCommandSession.send` (and so `AsyncTcpTransport.send` and `send_to_gateway`) and `AsyncSerialTransport.send` refuse any WHO 4 Dimension 22 frame with `OWNProtocolError(reason="destructive_frame")` before writing: `*#4*ZONE*22##` crashes the gateway's thermoregulation daemon and reboots an F461 (own-firmware-oracle `tools/expected_divergences.yaml` crash frames, live F461 2.0.11). The detector `is_who4_dimension_22()` is public. |
There was a problem hiding this comment.
Two small accuracy tweaks here once the transport behavior is aligned:
- As currently written,
AsyncTcpTransport.sendreturnsNone(loggingERRORand settinglast_error) rather than raisingOWNProtocolError, whereasAsyncSerialTransport.sendraises—please make the changelog description match whichever unifiedOWNTransport.sendbehavior you choose. - Worth noting that WHO 4 Dimension 22 is published split-unit IR control (
*#4*3#Z#N*22##) refused because it crashesbt_termoon Linux gateways (e.g. F461 2.0.11).
`_WHO4_DIMENSION_22_REGEX.search()` was unanchored, so a mid-frame `*michnovka#4*` (WHERE `michnovka#4`: group 4 in WHO 1/2/16/22 or central-unit zone 4 in WHO 4, or a DIMENSION `michnovka#4` write) followed by a value of 22 was refused as if it were WHO 4 dimension 22. Nine OWNd builders were affected, for example `OWNAutomationCommand.set_shutter_level("michnovka#4", 22)` -> `*michnovka#2*michnovka#4*OpenWebNet-HA#11#001*22##` and `OWNHeatingCommand.set_holiday_end_date("michnovka#4", 22, 10, 2026)` -> `*michnovka#4*michnovka#4*OpenWebNet-HA#30*22*10*2026##`, plus raw writes such as `*michnovka#1*14*michnovka#4*1*22##`. The detector is now anchored to the first `*` of each `##`-delimited frame, and the WHO / DIMENSION tokens accept what the gateway's atoi() reads as 4 / 22 (leading zeros, surrounding whitespace, a `+` sign), so `*michnovka#4*2*22 `, `*michnovka#4*2* 22##` and `*michnovka#4*2*+22##` are refused too. bytes, bytearray and memoryview are decoded as latin-1 before splitting instead of going through `str()`. The comment names the published meaning of the dimension: split-unit IR control (`*michnovka#4*3#Z#N*22##`, WHO 4 specification pp. 66-68), refused because it crashes bt_termo on Linux IP gateways. Tests: the nine `michnovka#4` builder frames, `*michnovka#1*14*michnovka#4*1*22##`, `*#1004*2*22##` and `*OpenWebNet-HA#22*3#1#1*1##` are safe (and the builders are still written); the split-unit forms, the whitespace / sign / unterminated variants and buffer inputs are refused. Before this change 50 of the 115 cases fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ontract `AsyncSerialTransport.send` wrote whatever it was given (appending `##` when missing) without the `OWNMessage.parse` / `is_valid` check that `OWNCommandSession._locked_send` runs, so a malformed single-`#` concatenation such as `*1*1*11#*michnovka#4*2*22##` or a signaling frame reached the port. It also diverged from `AsyncTcpTransport.send` on a refused frame: it raised `OWNProtocolError` even when not connected, while the TCP transport raises `RuntimeError` first, then logs at ERROR, records `last_error` and returns `None`. `OWNTransport.send` now carries the same `raise_on_error` keyword as `OWNCommandSession.send` and documents one contract for both implementations: `RuntimeError` when not connected, then a locally refused frame (WHO 4 dimension 22, or an invalid / signaling frame) is logged at ERROR, kept in the new `OWNTransport.last_error` and answered with `None`, or raised as `OWNProtocolError(reason="destructive_frame" / "invalid_frame")` with `raise_on_error=True`. `AsyncTcpTransport` passes the keyword to its command session and exposes the session's `last_error`. The serial transport serialises the frame once, with its terminator, and that string is checked, parsed, logged and written. The refusal text is shared by the session and the serial transport (`destructive_frame_error`) and names the actual failure, "crashes the thermoregulation daemon (bt_termo) on Linux IP gateways", which is accurate on a Legrand 3578 serial interface too. Tests: serial default / raise_on_error behaviour on every dimension 22 form and on a built `OWNHeatingCommand`, serial refusal of the single-`#` concatenation, a signaling frame, text and an empty string, not-connected ordering, safe frames still written (terminator appended), TCP `raise_on_error` passthrough and `last_error` delegation. Before this change 28 of the 122 cases fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…connecting `OWNCommandSession.send_to_gateway()` ran `connect()` (TCP socket, nonce / HMAC handshake, a command slot on single-session gateways such as MH200N or F453AV, and up to 5 attempts of backoff on an unreachable host) before `send()` refused the frame. The detector now runs before `connect()`: the helper fails fast with `OWNProtocolError(reason="destructive_frame")` under `raise_on_error`, or logs at ERROR and returns `None`, and never opens a socket. The refusal (error, `last_error`, ERROR log) is shared with `_locked_send` through `_refuse_destructive_frame()`. Tests: `send_to_gateway` with a string and with a built `OWNHeatingCommand` in both modes asserts `connect.assert_not_called()` and that `_locked_send` is never reached; a safe frame still connects and sends. Before this change both refusal cases fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`_locked_send` serialised the message once into `frame` for the
dimension 22 check and the write, but still re-evaluated `str(message)`
/ `f"{message}"` for `OWNMessage.parse`, the `OWNProtocolError` and
`OWNNackError` frames and every log line. A message object with a
non-idempotent `__str__` would have been validated and logged as a
different string from the one written. Every one of those call sites
now uses `frame`, so the string checked is the string parsed, logged,
raised and written.
Test: a counting `OWNHeatingCommand` subclass is serialised exactly
once on the ACK, NACK (retried once) and destructive paths. Before this
change the ACK and NACK cases fail (several serialisations).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…on 22 attribution The `[Unreleased]` entries now match the behaviour of the previous commits: one refusal contract on `OWNCommandSession.send`, `send_to_gateway` (before `connect()`), `AsyncTcpTransport.send` and `AsyncSerialTransport.send` (`None` + ERROR log + `last_error`, or `OWNProtocolError` with `raise_on_error=True`), the new `OWNTransport.send(raise_on_error=...)` keyword and `OWNTransport.last_error`, serial outbound validation, and the published meaning of WHO 4 Dimension 22 (split-unit IR control, `*michnovka#4*3#Z#N*22##`, WHO 4 specification pp. 66-68) refused because it crashes bt_termo on Linux IP gateways such as the F461 2.0.11. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ast_error wording Cross-check follow-up. `*#4junk*2*22##`, `*michnovka#4*2*22junk##`, `*michnovka#4.0*2*22##` and `*michnovka#4*2*22.5##` are what a C atoi() would still read as WHO 4 / dimension 22 but are not OpenWebNet tokens: the detector leaves them to the frame validation, which `OWNMessage.parse` refuses, so the command session and the serial transport answer `invalid_frame` and write nothing. The test pins that on both paths. `OWNTransport.last_error` is documented as the error recorded by the last `send()`: a local refusal on both transports, plus NACK / timeout / connection errors on the TCP transport through its command session; a serial NACK or timeout still returns `None` without recording one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The MyHOME conformance gate (MyHOME's suite against this checkout) asserts the command session's log calls with the message object (`tests/test_infeasible_commands.py`, `tests/test_phase1_architecture.py` in OpenWebNet-HA/MyHOME), so passing the serialised `frame` to the logger broke two of its tests. Log lines take the caller's object again and format it lazily, as before this PR; `frame` stays the one string that is checked, parsed, carried by every error and written, which is the correctness point of the single serialisation. The serialisation-count test mocks the logger so it counts the code path only, and asserts the recorded error carries the written frame. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A NACKed status request returns None after one write, is reported at DEBUG only, and records OWNNackError with the written frame; a NACKed command is retried once, reported at ERROR, and raises OWNNackError with raise_on_error=True. The log calls receive the message object, as MyHOME's suite (run by the conformance gate against this checkout) asserts. The test fails on the earlier branch head 269acdf, which passed the serialised string to the logger, and passes with d40588d. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
All five points are on the branch (head 06fc3ef, one commit per point plus two follow-ups), CI green including MyHOME's suite against this checkout. An independent second-opinion review of the diff was run after the rework; its two remaining remarks (log argument identity, NACK contract pin) are the last two commits.
Full suite 2039 passed / 1 skipped at 100 % statement coverage, mypy and ruff clean, |
GreenGrassBlueOcean
left a comment
There was a problem hiding this comment.
All five points from the earlier review are cleanly addressed on 06fc3ef6 (anchored is_who4_dimension_22 detector + buffer decoding, outbound frame validation on AsyncSerialTransport with a unified OWNTransport.send / last_error contract, pre-connect() refusal in send_to_gateway, single serialization while preserving lazy message logger arguments for MyHOME, and WHO 4 split-unit IR attribution), and all CI / MyHOME conformance checks are green.
Approved from my side — @fedem95 could you also take a look and review before we merge?
fedem95
left a comment
There was a problem hiding this comment.
Reviewed the latest commit. The WHO 4 Dimension 22 guard now covers the command and transport send paths, with regression tests for safe frames and refusal cases. All checks are passing. Approved.
Requested by A
Summary
*#4*ZONE*22##(WHO 4, Dimension 22) crashes the gateway's thermoregulation daemon (bt_termo) and reboots an F461 about three minutes later. OWNd parsed*#4*2*22##as a validOWNHeatingCommandand wrote it: onmaster,OWNCommandSession.send("*#4*2*22##")against a fake writer returnsTrueand writesb'*#4*2*22##';AsyncSerialTransport.sendhad no validation at all. Only MyHOME refuses the frame today (itssend_messageservice andws_bus_monitor_sendincustom_components/myhome/websocket.py); any other caller of OWNd (scripts, other integrations) can reboot the gateway. Both MyHOME guards can switch toOWNd.message.is_who4_dimension_22once this is released.Evidence:
*#4*2*22##and*#4*#0*22##as crash frames for F461020011(tools/expected_divergences.yaml,crash_frames: "Known bt_termo daemon crash on WHO=4 DIMENSION 22 in F461 020011").tests/fixtures/traces/f461_live_2026-10-15, runprobe_dim22_confirm(isolated*#4*2*22##: WHO 4 refused in 3 ms, gateway reboot 3 min 12 s later, uptime 0:00:41 afterwards). That fixture holds five dimension-22 sends in total (3×*#4*2*22##, 2×*#4*#0*22##); each time every poll NACKed for about three minutes and the gateway came back with its uptime reset.Refs
OpenWebNet-HA/MyHOME#799(the 2026-10-23 and 2026-10-24 captures sent no dimension 22 on purpose).Changes
OWNd/message/heating.py:_WHO4_DIMENSION_22_REGEXand the publicis_who4_dimension_22(frame), exported fromOWNd.message. Dimension 22 is the published split-unit IR control (*#4*3#Z#N*22##/*#4*3#Z#N*#22*MOD*SP*VEL*SWING##, WHO 4 specification pp. 66-68); the detector flags any WHO 4 frame whose dimension is 22: request, write (*#22*) or reply form, with or without a dimension parameter, with the tokens a gatewayatoi()reads as 4 / 22 (leading zeros*#04*2*022##, whitespace*#4*2* 22##, sign*#4*2*+22##, missing terminator*#4*2*22), and anywhere in a concatenated string (*1*1*11##*#4*2*22##). It is anchored to the first*of each##-delimited frame, so a mid-frame WHERE#4/ DIMENSION#4(*#2*#4*#11#001*22##,*#4*#4*#30*22*10*2026##,*#1*14*#4*1*22##) is not flagged; neither are zone 22 (*#4*22##,*#4*#22*14##), dimension 220, a value0220,*#1004*2*22##or dimension 22 of WHO 13 / WHO 1.bytes,bytearrayandmemoryvieware decoded as latin-1 before splitting.OWNd/connection/command_session.py: the check is the first thing_locked_senddoes, before parsing, before any reconnect and before anything is written._locked_sendis the only writer of the command session, sosend(),keepalive()andAsyncTcpTransport.sendall inherit it.send_to_gateway()runs the check beforeconnect(): no socket, no handshake, no command slot on a single-session gateway, no 5-attempt backoff on an unreachable host. On refusal (_refuse_destructive_frame) it logs at ERROR, setslast_errorand returnsNone; withraise_on_error=Trueit raisesOWNProtocolError(reason="destructive_frame"). The frame is serialised once (frame = str(message)) and that one string is checked, parsed, logged, put in every error and written.OWNd/transport/base.py,tcp.py,serial.py: oneOWNTransport.send()contract. The abstract signature gains theraise_on_errorkeyword ofOWNCommandSession.send, andOWNTransport.last_errorexposes the recorded error (the TCP transport delegates to its command session). Both transports raiseRuntimeErrorwhen not connected, then refuse a frame locally the same way: ERROR log,last_error,None, orOWNProtocolErrorwithraise_on_error=True.AsyncSerialTransport.sendnow also validates outbound frames like_locked_send(OWNMessage.parse,is_valid, notOWNSignaling): the single-#concatenation*1*1*11#*#4*2*22##, a signaling frame or plain text are refused withreason="invalid_frame"instead of being written to the port. The serial frame is serialised once, terminator included (*1*1*12is still written as*1*1*12##), and that string is checked, parsed, logged and written. The refusal text is shared (destructive_frame_error) and accurate on a Legrand 3578 too: "crashes the thermoregulation daemon (bt_termo) on Linux IP gateways".OWNCommand.parse("*#4*2*22##")still returns a validOWNHeatingCommand, because the frame can appear on the bus and must stay readable. Only sending is refused.*99*0##,*#*1##, HMAC, the event keepalive) take no caller input.OWNd/__main__.pyopens an event session only and has no send path.CHANGELOG.md: entries under[Unreleased]/Changed(transport contract) andFixed(the guard with its split-unit attribution, serial validation).On the
Nonereturn withraise_on_error=False.Nonehere is not "unverified": it is OWNd's existing contract for a send that was refused locally. The pre-existinginvalid_framebranch of_locked_send(command_session.py, "Invalid OpenWebNet frame") already doesself._last_error = errandreturn Nonefor a frame that never leaves the process; this PR only re-types itserrvariable and adds the destructive-frame branch in front of it with the same shape.True/Falsestay reserved for an ACK / NACK from the gateway; a local refusal is logged at ERROR, kept inlast_error, and raised withraise_on_error=True.Review follow-up (review 5639882172)
#4/ DIMENSION#4frames; whitespace /+/ leading-zero tokens andbytesinputef5f003fix(message): anchor the detector to the frame startOWNTransport.send()contract split between TCP and serial, error wording117404bfix(transport): validate serial frames and unify the send() refusal contractsend_to_gateway()connected and authenticated before refusing7c002a7fix(connection): refuse in send_to_gateway before connecting_locked_send06dd2e6refactor(connection): serialise the outbound frame once;d40588dkeeps the message object in the log arguments (MyHOME's suite asserts on them: the conformance gate failed withframethere),framestays the string checked, parsed, raised and written;06fc3efpins the NACK contract (status request: one write, DEBUG,None; command: one retry, ERROR,OWNNackError; log calls with the message object)bb66882docs(changelog) (theheating.pycomment is inef5f003)atoi()prefix spellings (*#4junk*2*22##,*#4*2*22junk##) are not detector matches butOWNMessage.parserefuses them, so both paths answerinvalid_frame;last_errorwording narrowed (a serial NACK / timeout records nothing)269acdftest(transport)Builder note:
OWNSoundDiffusionCommand.set_preset("#4", 22)emits*#22*#4*#6*22##(dimension 6), not*#22*#4*#19*22##; both strings are inSAFE_FRAMES, and the builder test asserts the emitted frame.State Diagram
stateDiagram-v2 direction TB state "1. OWNCommandSession._locked_send(message) (guard first, before parse / reconnect / write)" as Send { [*] --> Check Check --> Refused : is_who4_dimension_22, str, message\n*#4*2*22##, *#4*#0*22##, *#04*2*022##, *#4*2*#22*...##\nOWNProtocolError, reason=destructive_frame\nlast_error set - return None, or raise with raise_on_error Check --> Invalid : not a valid frame\nreason=invalid_frame , unchanged, same shape Check --> Written : every other frame\n*#4*2*19##, *#4*22##, *#4*#22*14##, *#13**22##\nwritten as before } state "2. F461 2.0.11 if the frame reaches it (f461_live_2026-10-15, probe_dim22_confirm)" as Gateway { [*] --> Running Running --> Crashed : *#4*2*22## or *#4*#0*22##\nWHO 4 NACK in 3 ms, bt_termo dies Crashed --> Crashed : every poll NACKs for ~3 min Crashed --> Running : reboot 3 min 12 s after the frame\nuptime 0 -00 -41 afterwards }Where are the traces
tests/fixtures/traces/f461_live_2026-10-15(merged), runprobe_dim22_confirm(and the five sends across the fixture)*#4*2*22##, 2×*#4*#0*22##: WHO 4 refused in 3 ms, gateway reboot 3 min 12 s later, uptime 0:00:41 afterwardsOpenWebNet-HA/MyHOME#799(merged),f461_live_2026-10-23andmyhomeserver1_live_2026-10-24probe_dim22_confirmNo emulation claim is made for other models: the oracle excludes the frame rather than running it.
What does the Oracle say
No oracle stimulus exists for this frame:
tools/expected_divergences.yaml(crash_frames) excludes*#4*2*22##and*#4*#0*22##for F461020011with the reason "Known bt_termo daemon crash on WHO=4 DIMENSION 22 in F461 020011", so there is no per-gateway TSV row to cite. The physical F461 2.0.11 fixture above is the evidence.Testing
tests/test_who4_dimension22_guard.py(new, 132 cases): the detector on 20 Dimension 22 forms (including the spec split-unit forms*#4*3#2#1*22##,*#4*3#2#1*#22*0***##,*#4*45#1*#22*3**1*1##, the whitespace / sign / unterminated variants) and 26 safe frames (the nine WHERE#4builder outputs,*#1*14*#4*1*22##,*#1004*2*22##,*#22*3#1#1*1##, the single-#concatenation),bytes/bytearray/memoryviewinput, the nineOWN*Commandbuilders (frame asserted, detectorFalse, still written by the session); the command session with a raw string in bothraise_on_errormodes; withOWNCommand.parse,OWNMessage.parseandOWNHeatingCommand(...)objects; a counting__str__serialised exactly once on the ACK, NACK and refused paths (logger mocked: log lines keep the message object, formatted lazily); refusal before reconnecting (connectis never called);send_to_gatewayrefusing beforeconnect()(connect.assert_not_called(),_locked_sendnever reached) and still connecting for a safe frame; the TCP transport (None+last_error,raise_on_errorpassthrough); the serial transport on all 20 forms in both modes, on a built command, on invalid frames (invalid_frame), not-connected ordering, safe frames still written with the terminator appended; theatoi()prefix spellings refused as invalid on both paths.masterthe module fails at collection (is_who4_dimension_22does not exist) and the session writes the frame; on the first PR commit (27876c6) 50 of the 115 detector / builder cases fail (anchoring), 28 of the 122 transport cases (contract), 2send_to_gatewaycases (connects first) and 2 serialisation cases (severalstr()calls); on this branch all 132 pass.2039 passed, 1 skipped(master:1907 passed, 1 skipped); coverage 100 % of statements (scripts/verify_coverage.py, 6,629 / 6,629; the 14 partial branches are pre-existing insession.py/sound.py),mypy OWNd tests/typingclean (tests/typing/consumer_send.pyexercises the new keyword on all three transports),ruff checkclean,scripts/sync_documentation.py --checkandscripts/verify_library_standards.pypass.tests/test_transport_extended.py::test_tcp_transport_full_lifecycle_and_listen_loopnow expects the keyword in the mocked session call. MyHOME conformance: MyHOME's full suite (origin/v2-phase1-architecture,pytest tests/ -n 4) against this checkout at798d82c7:4523 passed, 1 skipped; the269acdfhead failed its two NACK log assertions because the log calls received the serialised string instead of the message object, the NACK return value and retry behaviour never changed.python -c "from OWNd.message import is_who4_dimension_22 as f; print(f('*#4*2*22##'))"printsTrue. No live test, deliberately: the frame is never sent.Live retest (2026-10-24)
OWNd layer not exercised live: MyHOME's service guard refused
*#4*2*22##first (myhome.send_messagereturned200 [], logRefusing to send *#4*2*22##), so nothing with dimension 22 reached OWNd or the bus (HA bus-monitor history, 146 frames, holds no dimension-22 frame). That confirms the upper guard only; this guard rests on its 132 unit tests.Companion
None. MyHOME's
send_messageservice andws_bus_monitor_sendalready refuse the frame; this guard sits underneath them, and both can switch toOWNd.message.is_who4_dimension_22once released.🤖 Generated with Claude Code