Skip to content

fix: guard relay_switch get_basic_info against short responses#509

Merged
bdraco merged 3 commits into
sblibs:masterfrom
bluetoothbot:koan/guard-relay-switch-parsers
Jun 20, 2026
Merged

fix: guard relay_switch get_basic_info against short responses#509
bdraco merged 3 commits into
sblibs:masterfrom
bluetoothbot:koan/guard-relay-switch-parsers

Conversation

@bluetoothbot

@bluetoothbot bluetoothbot commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

What

Guard relay_switch.get_basic_info (RELAY_SWITCH_1, RELAY_SWITCH_1PM, RELAY_SWITCH_2PM, PLUG_MINI_EU, GARAGE_DOOR_OPENER) against truncated BLE responses.

Why

Closes #369. The reporter saw IndexError: bytearray index out of range from _parse_common_data and ValueError: Insufficient data: need at least 4 bytes, got 1 from _parse_user_data (via parse_uint24_be) when a single-byte payload — b"\x02" — reached the parser. The base _get_basic_info only filters the exact bytes b"\x07" and b"\x00", so anything else slips through and crashes inside the parser. Same class of bug as #491/#495/#496/#499/#500 — this branch is the relay-switch arm of that sweep, which was skipped at the time.

How

  • SwitchbotRelaySwitch.get_basic_info: bail with a warning if _data < 17 (last index touched is [16]) or _channel1_data < 15 (last 2-byte power slot is at offset 13).
  • SwitchbotRelaySwitch2PM.get_basic_info: same guard for _channel2_data.
  • Warnings log the hex payload so the next short response is easy to identify in the wild.

Testing

  • tests/test_relay_switch.py — added test_get_basic_info_2PM_short_response (5 truncation scenarios) and test_get_basic_info_short_response parametrised across all four single-channel models × 2 truncation scenarios. Each case asserts get_basic_info() returns None instead of raising.
  • Full suite: 1226 passed.

Quality Report

Changes: 2 files changed, 124 insertions(+)

Code scan: clean

Tests: passed (1226 passed)

Branch hygiene: clean

Generated by Kōan post-mission quality pipeline

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
switchbot/devices/relay_switch.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens relay-switch get_basic_info() parsing against truncated BLE command responses so callers receive None (with a warning log) instead of raising IndexError/ValueError, addressing issue #369 for relay/plug/garage and 2PM variants.

Changes:

  • Add minimum-length guards in SwitchbotRelaySwitch.get_basic_info() for basic_info (≥17) and channel1_info (≥15), logging truncated payloads.
  • Add the same minimum-length guard for channel2_info in SwitchbotRelaySwitch2PM.get_basic_info() (≥15).
  • Add regression tests covering multiple truncation scenarios across single-channel models and the 2PM model.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
switchbot/devices/relay_switch.py Adds length checks + warning logs to prevent parsing truncated command responses.
tests/test_relay_switch.py Adds parametrized async tests asserting short responses return None instead of raising.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@bluetoothbot

Copy link
Copy Markdown
Collaborator Author

PR Review — fix: guard relay_switch get_basic_info against short responses

Correct, well-scoped fix for issue #369 — merge-ready.

Verified against the parsers: _parse_common_data reads raw_data[16] (guard ≥17 is exact) and _parse_user_data's last read is parse_power_data(raw_data, 13, ...) touching bytes 13–14 (guard ≥15 is exact). Guards sit after the existing falsy-check and before any indexing, so the \x02 repro and sub-minimum payloads now return None instead of raising IndexError/ValueError. The 2PM override correctly layers its channel2 guard on top of the inherited base guards. Test coverage is thorough — parametrized truncation scenarios across all four single-channel models plus five 2PM cases, each asserting None rather than an exception, and codecov reports 100% on the changed lines. warning log level matches the existing lock.py precedent for this firmware-bug class.

  • suggestion: warning messages omit self.name/model, which the PR's own stated goal (identify in the wild) would benefit from
  • suggestion: three near-identical guard blocks could share a small helper

🟢 Suggestions

1. Warnings omit device identity — harder to trace in the field
switchbot/devices/relay_switch.py:139-162

The PR description states the warnings exist so "the next short response is easy to identify in the wild," but the three new _LOGGER.warning calls log only the byte count and hex payload — not which device/model emitted it.

Other diagnostics in device.py (e.g. lines 416–434) prefix with self.name, which is available on these classes. When a user has multiple relay switches, a bare "Short channel1 response" line can't be attributed to a specific device.

Consider prefixing with self.name (and optionally self._model) to make field reports actionable:

_LOGGER.warning(
    "%s: Short basic-info response (%d bytes): %s",
    self.name, len(_data), _data.hex(),
)

Non-blocking — purely a debuggability nicety.

_LOGGER.warning(
    "Short basic-info response (%d bytes): %s", len(_data), _data.hex()
)

Checklist

  • Guards sized to max index + 1 (correctness)
  • No untested branches introduced
  • Edge/boundary coverage (sub-minimum, single-byte)
  • No backward-incompatible behavior change for valid payloads
  • Diff matches PR description
  • Error/diagnostic messages are actionable — suggestion #1

Automated review by Kōan (Claude) HEAD=f62aa33 2 min 1s

bluetoothbot and others added 3 commits June 20, 2026 14:45
Closes sblibs#369. `_parse_common_data` reads `raw_data[1]`, `[2]`, `[16]` and
`_parse_user_data` calls `parse_uint24_be(raw_data, 1)` plus
`parse_power_data(raw_data, 13, ...)`. The base `_get_basic_info` only
filters single-byte payloads matching `b"\\x07"` or `b"\\x00"`, so any
other truncated response (e.g. the reporter's `b"\\x02"`) bubbles up and
raises IndexError/ValueError inside the parsers.

Gate `get_basic_info` (1PM/garage door/plug mini EU) on `_data` ≥ 17
bytes and `_channel1_data` ≥ 15 bytes, and the 2PM override on
`_channel2_data` ≥ 15 bytes. Log a warning with the hex payload to aid
diagnosis. Add regression coverage for each of the three truncation
paths across all four single-channel models and the 2PM variant.
@bluetoothbot

Copy link
Copy Markdown
Collaborator Author

Rebase with requested adjustments

Branch koan/guard-relay-switch-parsers was rebased onto master and review feedback was applied.

Changes applied

  • Done. Applied reviewer suggestion press #1.
  • Prefixed all three _LOGGER.warning calls in relay_switch.py with self.name (basic-info, channel1, channel2 short-response guards) so field reports identify which device emitted the truncated payload.
  • Skipped suggestion Beta #2 (shared helper) — explicitly non-blocking style nicety, out of scope.

Stats

2 files changed, 130 insertions(+)
Actions performed

CI status

CI will be checked asynchronously.


Automated by Kōan

@bluetoothbot bluetoothbot force-pushed the koan/guard-relay-switch-parsers branch from f62aa33 to de7e562 Compare June 20, 2026 14:45
@bdraco bdraco marked this pull request as ready for review June 20, 2026 14:48
@bdraco bdraco merged commit 7b7b8f1 into sblibs:master Jun 20, 2026
6 checks passed
@bluetoothbot bluetoothbot deleted the koan/guard-relay-switch-parsers branch June 21, 2026 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Single data byte causing index out of range

3 participants