fix(models): decode packed consumable alert lists (#79) - #80
Conversation
Every maintain/replace alert has been silently discarded since the
feature shipped. protobuf packs repeated scalars, and blackboxprotobuf
surfaces a packed field as a str whose code points are the encoded bytes.
_enum_int_list() accepted "an int, or a list of ints", so
int('\x04\x06\x08\n') raised, the per-item except swallowed it, and both
lists came back empty.
Empty means "nothing needs attention", so the failure was invisible: a
robot asking for six parts was reported as perfectly healthy. Live
capture from a Flow (AX12, v01.08.03.07):
{'1': {'1': '\x04\x06\x08\n', '2': '\x03\x14'}}
maintainItems = [4, 6, 8, 10] wash ribs, universal wheel,
side distance sensor, anti-winding brush
replaceItems = [3, 20] side brush, station bag
Decodes str/bytes as packed varints, properly rather than one byte per
value, so values above 127 survive. Both callers are consumable lists, so
there is no text field to misread.
The existing test fed [1, 9] and 8 — the shapes a hand-written payload
takes, not the shape a robot sends, which is exactly how this survived a
green suite. New tests use the verbatim capture, plus the single-item case
(where an off-by-one decoder still looks right), a bytes blob, a
multi-byte varint, and an empty blob still meaning healthy.
Verified against the live robot: maintain_items and replace_items now
populate from the same payload that previously parsed to nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YAo9szBPifvDJrsag2oW6Y
Greptile SummaryThe PR fixes consumable maintenance and replacement alerts by decoding blackboxprotobuf string/byte blobs as packed protobuf varints in both client copies.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking opportunity to reject overlong malformed varints before they consume excessive resources. The intended packed consumable payloads are decoded correctly and comprehensively tested, while only malformed continuation sequences remain insufficiently bounded. Files Needing Attention: narwal_client/models.py and custom_components/narwal/narwal_client/models.py
|
| Filename | Overview |
|---|---|
| narwal_client/models.py | Adds packed consumable-list decoding; normal payload handling is covered, but malformed overlong varints remain unbounded. |
| custom_components/narwal/narwal_client/models.py | Mirrors the client decoder exactly and therefore shares its malformed-varint hardening concern. |
| tests/test_models.py | Adds focused regression tests covering live packed payloads and important representation and boundary cases. |
Reviews (1): Last reviewed commit: "fix(models): decode packed consumable al..." | Re-trigger Greptile
| for byte in raw: | ||
| acc |= (byte & 0x7F) << shift | ||
| if byte & 0x80: | ||
| shift += 7 |
There was a problem hiding this comment.
If a malformed consumable response contains a long sequence of continuation bytes, this loop repeatedly expands an arbitrary-precision integer without enforcing protobuf's maximum varint width, consuming disproportionate CPU and memory and potentially producing an enormous bogus consumable identifier. Rejecting overlong varints would make this device-controlled parsing path fail safely.
Bumps the manifest, adds release notes, and updates the README's release banner and status table. Headline is the consumable alert fix (#80): both alert sensors had reported "no problem" on every robot since the feature shipped, because packed varints parsed to nothing and an empty list means healthy. Also ships Freo Z Ultra support (#76) and the fan-tier rename with the AX26 gate. Verified before tagging: 255 tests, CI green, deployed to a live Home Assistant on real hardware with all 28 entities correct and both alert sensors reporting their item lists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YAo9szBPifvDJrsag2oW6Y
Fixes the confirmed bug in #79: every consumable maintain/replace alert has been silently discarded since the feature shipped.
The bug
consumable/get_consumable_inforeturns its two lists as packed repeated varints, and blackboxprotobuf surfaces a packed field as astrwhose code points are the encoded bytes._enum_int_list()accepted "an int, or a list of ints", soint('\x04\x06\x08\n')raised, the per-itemexcept: continueswallowed it, and both lists came back empty.Empty means nothing needs attention. That's what made this invisible — it failed as good news. Live capture from my Flow (AX12,
v01.08.03.07), a robot that has been running for months:binary_sensor.maintenance_requiredandbinary_sensor.replacement_requiredreportedoffthroughout, on a robot asking for six parts.The fix
Decode
str/bytesas packed varints before the int conversion. Properly, not one byte per value — enum ids are all small today, but a byte-per-value shortcut would misread anything above 127 and hide the next surprise.Both callers of
_enum_int_listare consumable lists, so there is no genuine text field that could be mangled by treating astras a blob.Why a green suite missed it
The existing test fed
{"1": [1, 9], "2": 8}— the shapes a hand-written payload takes, not the shape a robot sends. The test and the code shared an assumption, so they agreed with each other and not with the device.New tests use the verbatim capture, plus the cases that would let a subtly wrong decoder pass:
bytesblob rather thanstr, so a bbp version change doesn't reintroduce it255 passed, and bothnarwal_clientcopies verified identical.Verification
Re-ran the probe against the live robot with the patched code — the same payload that previously parsed to nothing now yields
maintain_items = [4, 6, 8, 10]andreplace_items = [3, 20].Scope
This closes the first checkbox in #79 only. The disputed field 38, the unverified field 41 → detergent mapping, the absent fields 21/35, and the tank-state thresholds behind #77 all still need captures from other people's hardware — the asks are listed in that issue.