Conversation
Rollback blind-restored the whole shadow and every command's parameters from a pre-send snapshot, even though the exact-send path never mutates the shadow before commit and the transaction only ever mutates the one command _prepare() actually touched. The awscrt MQTT callback runs on its own thread outside the dispatcher's lock and can update those same objects while send_exact is awaited; on failure, the old rollback threw that authoritative update away. Shadow is no longer restored on rollback (dispatch never owns it pre-commit). Command parameters now use compare-and-restore: a second snapshot taken right after _prepare() (before the await) lets rollback tell the transaction's own write apart from a concurrent one, and only undoes what it can prove is still its own.
…rsal observe_mqtt_update matched the first FIFO pending command sharing ANY expected key with an incoming push, so an older command sharing only a mandatory field (common to every command) could consume a push that belonged to a newer, more fully-confirmed one. It now scores every pending entry by how many expected key/value pairs the push actually confirms and keeps the best match, with FIFO order breaking ties. emit_command_event ran two full, unbounded redact_identity passes plus a full unbounded set-materialization pass BEFORE _bound ever applied its depth/size limits, so a cyclic or very deep payload silently dropped the event (RecursionError) and a large mapping/set paid for a full sort before being trimmed. _bound is now the single traversal: it caps recursion depth, breaks cycles via a path-scoped id set, and samples every collection through islice before sorting, so only a bounded slice of a huge collection is ever touched. redact_identity (a widely shared helper, left untouched) now runs once, after bounding, on data already guaranteed small.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @tis24dev, your pull request is larger than the review limit of 150000 diff characters
📝 WalkthroughWalkthroughThis PR adds air purifier capability discovery, sensors and controls, transactional command dispatch, MQTT diagnostics, experimental feature gating, beta tag support, translations, fixtures, and broad regression coverage. It also records campaign progress, validation constraints, and repository tooling changes. ChangesAir purifier support
Transactional dispatch and diagnostics
Validation and repository support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds full air purifier (AP) support and experimental diagnostics/controls, wires those features through the transactional command dispatcher and MQTT correlation/diagnostics, tightens Home Assistant stubs and options handling, extends diagnostics and release-tag policy, and bumps the integration to v5.10.0-beta. Sequence diagram for AP command dispatch, diagnostics, and MQTT correlationsequenceDiagram
actor User
participant HA as HomeAssistant_platform
participant APMod as air_purifier.ap_patch
participant Disp as CommandDispatcher
participant HC as HonClient
participant Cmd as HonCommand
participant App as HonAppliance
participant MQTT as MqttTransport
participant CD as command_diagnostics
User->>HA: change entity state (e.g. HonAirPurifierFan.async_turn_on)
HA->>APMod: ap_patch(action, capabilities, values)
APMod-->>HA: CommandPatch
HA->>HC: dispatch_patch_sync(appliance, CommandPatch)
HC->>Disp: dispatch(appliance, CommandPatch)
Disp->>CD: emit_command_event(command_intent)
Disp->>Cmd: canonical_exact_payload(params)
Disp->>Cmd: send_exact(payload)
Cmd->>App: appliance.api.send_command(...)
App-->>Cmd: result True
Cmd-->>Disp: True
Disp->>App: sync_payload_to_params(payload)
Disp->>CD: record_expected_update(appliance, action, payload)
Disp->>CD: emit_command_event(command_result)
par later MQTT push
MQTT->>App: apply parameter updates
MQTT->>CD: observe_mqtt_update(appliance, observed_values)
CD->>CD: match pending action by key/value coverage
CD->>CD: emit_command_event(shadow_update)
CD->>CD: emit_command_event(contract_check)
end
Disp-->>HC: True
HC-->>HA: True
HA-->>User: state updated (after coordinator refresh)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
PR Summary by QodoRelease v5.10.0-beta: transactional command dispatch + air purifier support
AI Description
Diagram
High-Level Assessment
Files changed (39)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
.gitignore (1)
11-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
.superpowers/ignore rule.This PR tracks campaign documentation under
.superpowers/sdd/...; ignoring the whole tree means future documentation there is silently skipped bygit addunless force-added. Ignore only generated artifacts, or move this rule to a local/global ignore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.gitignore around lines 11 - 17, Narrow the .superpowers/ entry in .gitignore so tracked campaign documentation under .superpowers/sdd/... remains discoverable by git add. Ignore only the specific generated artifacts, or remove this repository rule and rely on local/global ignore configuration.tests/conftest.py (1)
142-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
_install_fan_stubsto match its actual scope.It now installs
fan,light,switch,select,sensor,binary_sensor,numberandentity_platformstubs; the name reads as fan-only and will mislead the next person deciding where to add a platform stub.♻️ Suggested rename
-def _install_fan_stubs() -> None: +def _install_platform_stubs() -> None: """Shared `fan`, `light`, `switch`, `select` and `number` platform stubs.And at the call site (Line 409):
-_install_fan_stubs() +_install_platform_stubs()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/conftest.py` around lines 142 - 149, Rename _install_fan_stubs to a name reflecting that it installs shared platform stubs for fan, light, switch, select, sensor, binary_sensor, number, and entity_platform, then update its call site and any references consistently.tests/test_air_purifier_entities.py (1)
1762-1764: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the leftover
ON = Noneplaceholder.Nothing reads it, and the comment ("set in setUp-free helpers below") describes an approach that was not taken —
_labelpasses_experimental(True)directly.🧹 Proposed cleanup
class ExperimentalAirQualityLabelTest(unittest.IsolatedAsyncioTestCase): - ON = None # set in setUp-free helpers below - async def _label(self, raw: str | None):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_air_purifier_entities.py` around lines 1762 - 1764, Remove the unused ON = None placeholder and its accompanying comment from ExperimentalAirQualityLabelTest; leave the existing _label and _experimental behavior unchanged.custom_components/addhon/command_dispatch.py (2)
278-311: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAdd a comment for selector-free callbacks, no change needed.
prepareis only aCommandPatchslot here and none of the in-repo callers pass it, so there’s no current selected-category callback mutation path to fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/command_dispatch.py` around lines 278 - 311, Add a concise comment adjacent to the patch.prepare call documenting that selector-free callbacks are the supported path and that no selected-category callback mutation is currently performed. Do not change the behavior of CommandPatch handling or parameter mutation.
26-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog swallowed diagnostic failures at debug level.
Diagnostics must never break a dispatch, but bare
except: pass(repeated at Lines 85, 116, 143, 179) makes a systematically broken diagnostics path invisible. A_LOGGER.debug(..., exc_info=True)keeps the guarantee and preserves observability.♻️ Suggested change
def _emit_safely(event: str, fields: Mapping[str, object]) -> None: try: emit_command_event(event, fields) - except Exception: - pass + except Exception: # diagnostics must never break a dispatch + _LOGGER.debug("Dispatch debug: emit %s failed", event, exc_info=True) def _record_expected_safely( appliance: Appliance, action: str, payload: Mapping[str, object], ) -> None: try: record_expected_update(appliance, action, payload) - except Exception: - pass + except Exception: # correlation is best-effort + _LOGGER.debug("Dispatch debug: expected-update record failed", exc_info=True)(requires a module-level
_LOGGER = logging.getLogger(__name__))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/command_dispatch.py` around lines 26 - 41, Replace the silent exception handling in _emit_safely and _record_expected_safely, plus the other referenced diagnostic wrappers, with module-level logger debug calls using _LOGGER.debug(..., exc_info=True). Keep all diagnostic failures suppressed so command dispatch behavior remains unchanged, and initialize _LOGGER with logging.getLogger(__name__).Source: Linters/SAST tools
custom_components/addhon/client/transport/mqtt.py (1)
460-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSilent
except Exception: passhides a diagnostics regression; log at DEBUG instead.
observe_mqtt_updateis already internally failure-safe (broadtry/except+_log_failure), so anything escaping it is a real bug in the diagnostics module — and this barepassmakes it invisible. A DEBUG line keeps the callback thread protected while staying diagnosable, and clears Ruff S110/BLE001.♻️ Suggested change
try: observe_mqtt_update(appliance, observed_values) - except Exception: - pass + except Exception as err: # pragma: no cover - defensive + # Diagnostics must never break the push path, but a failure + # here means the module's own guard leaked: record it. + _LOGGER.debug("MQTT: command correlation failed: %s", err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/client/transport/mqtt.py` around lines 460 - 463, Replace the bare exception suppression around observe_mqtt_update in the MQTT update callback with a DEBUG-level log that records the escaped exception, while preserving the callback thread’s failure-safe behavior and avoiding propagation. Use the module’s existing logger and exception logging conventions.Source: Linters/SAST tools
tests/test_log_identity_redaction.py (1)
466-484: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWall-clock assertion is a latent CI flake.
The margin is large (~0.0007s vs the 0.2s bound), but a contended or emulated runner can still stall a single statement past 200ms, and the failure would be unrelated to the behavior under test. Consider taking the best of a few runs, or asserting the observable bound instead of elapsed time (e.g. counting
_sort_keyinvocations via a patch, which is what "no full sort before trimming" actually means).♻️ Cheap mitigation: best-of-three
- with self.assertLogs(self._LOGGER_NAME, level="DEBUG") as captured: - started = time.monotonic() - emit_command_event("command_payload", {"payload": huge}) - elapsed = time.monotonic() - started + with self.assertLogs(self._LOGGER_NAME, level="DEBUG") as captured: + elapsed = min( + self._time_emit(emit_command_event, huge) for _ in range(3) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_log_identity_redaction.py` around lines 466 - 484, Remove the wall-clock assertion from test_command_event_bounds_a_very_large_mapping_quickly, which can flake on slow or contended runners. Validate the bounded traversal behavior directly by patching or instrumenting _sort_key and asserting it is not invoked for every item before the payload is limited, while preserving the existing payload-size and serialized-record bounds.custom_components/addhon/diagnostics.py (1)
493-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReusing
_FUTURE_MAX_VALUESas a character budget conflates two units.
_FUTURE_MAX_VALUESis documented as "max values per unhandled delta" (an item count), but here it is multiplied by 4 to bound a string length. A dedicated constant would keep the two bounds independently tunable.♻️ Suggested tweak
+# Character budget for a single reported live value (a raw shadow scalar). +_FUTURE_MAX_VALUE_CHARS = 80- unhandled_state[name] = text[:_FUTURE_MAX_VALUES * 4] + unhandled_state[name] = text[:_FUTURE_MAX_VALUE_CHARS]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/diagnostics.py` at line 493, Replace the `_FUTURE_MAX_VALUES * 4` character bound in the unhandled-state assignment with a dedicated constant for the maximum text length, keeping `_FUTURE_MAX_VALUES` exclusively as the item-count limit. Define and name the new character-budget constant consistently with the surrounding constants, and use it when slicing `text`.custom_components/addhon/light.py (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the class-level set to silence RUF012.
- _attr_supported_color_modes = {ColorMode.BRIGHTNESS} + _attr_supported_color_modes: ClassVar[set[ColorMode]] = {ColorMode.BRIGHTNESS}Requires
from typing import ClassVar.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/light.py` around lines 84 - 85, Import ClassVar from typing and annotate the class-level _attr_supported_color_modes set as ClassVar[set[ColorMode]] to satisfy RUF012, leaving _attr_color_mode unchanged.Source: Linters/SAST tools
custom_components/addhon/air_purifier.py (1)
537-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSort
__all__to satisfy RUF022.
AP_CO_ALARM_RAWsits afterAP_WRITABLE_MODES; if RUF022 is enforced in CI this fails lint.♻️ Proposed ordering
__all__ = [ "AP_AIR_QUALITY_LABELS", "AP_AROMA_TO_OPTION", "AP_BRIGHTNESS_TO_LIGHT", + "AP_CO_ALARM_RAW", + "AP_ENTITY_PARAMS", + "AP_HANDLED_VALUES", "AP_LIGHT_TO_BRIGHTNESS", "AP_MODE_TO_PRESET", "AP_OPTION_TO_AROMA", "AP_PRESET_TO_MODE", "AP_WRITABLE_MODES", - "AP_CO_ALARM_RAW", - "AP_ENTITY_PARAMS", - "AP_HANDLED_VALUES", "AirPurifierCapabilities",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/air_purifier.py` around lines 537 - 559, Reorder the entries in the module-level __all__ list alphabetically to satisfy RUF022, specifically moving AP_CO_ALARM_RAW before AP_ENTITY_PARAMS and AP_HANDLED_VALUES while preserving all existing exports.Source: Linters/SAST tools
custom_components/addhon/number.py (1)
629-633: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse
air_purifier._raw()for AP state reads.
custom_activestill compares againststr(raw), while AP writes use_raw(value)for canonical codes, and other read paths also handle barestr(raw). Use the same exported raw normalizer here, or add a shared read-side raw helper for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/number.py` around lines 629 - 633, Update the _custom_active property to normalize the _AROMA_ATTR value with the exported air_purifier._raw() helper before comparing it with AP_CUSTOM_AROMA, replacing the direct str(raw) comparison and preserving the existing None handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/release-policy.sh:
- Line 50: Synchronize the invalid-trigger-tag guidance in the release-intake
workflow with the validation message in die, ensuring it documents numbered beta
tags in the pr-vX.Y.Z-betaN form as well as unnumbered beta tags. Update the
workflow’s hard-coded expectation or reuse a shared message so both validation
paths remain consistent.
In `@custom_components/addhon/command_diagnostics.py`:
- Around line 267-271: Normalize observed MQTT scalar values with
diagnostics._scalar_text in the observed mapping, and apply the same helper when
record_expected_update stores expected values, so numerically equivalent
payloads use identical text representations and match correctly.
In `@custom_components/addhon/hon_client.py`:
- Around line 680-684: Update dispatch_patch_sync and the corresponding async
command-dispatch path to preserve the boolean result from
CommandDispatcher.dispatch; when dispatch returns False for a cloud rejection,
raise or propagate the existing localized command error instead of allowing
entity callers to complete normally, while keeping successful dispatch behavior
unchanged.
In `@custom_components/addhon/select.py`:
- Around line 1093-1114: Update HonAirPurifierAromaSelect.async_select_option to
reject writes when the appliance is powered off, before constructing or
dispatching the aroma patch, matching the guard behavior in
HonAirPurifierTimeNumber.async_set_native_value. Raise HomeAssistantError using
a new translation key for the stopped-power condition, and add matching English
and Italian entries in the translation files.
In `@custom_components/addhon/translations/it.json`:
- Around line 607-608: Update the Italian co_alarm name translation to use the
full wording “Indicazione di monossido di carbonio” while preserving the
existing experimental and certification disclaimer, matching the established co
sensor label.
In `@tests/contract_fixtures.py`:
- Around line 25-32: Normalize each case’s id to a string before duplicate
detection in the fixture validation loop: derive the normalized value once, use
it for the membership check, and store that same value in ids. Keep the existing
missing-field validation and duplicate-id error behavior unchanged.
In `@tests/test_air_purifier_entities.py`:
- Around line 490-492: Move the existing if __name__ == "__main__":
unittest.main() block from its current position to the end of
tests/test_air_purifier_entities.py, after all test classes and definitions, so
direct execution discovers every test.
---
Nitpick comments:
In @.gitignore:
- Around line 11-17: Narrow the .superpowers/ entry in .gitignore so tracked
campaign documentation under .superpowers/sdd/... remains discoverable by git
add. Ignore only the specific generated artifacts, or remove this repository
rule and rely on local/global ignore configuration.
In `@custom_components/addhon/air_purifier.py`:
- Around line 537-559: Reorder the entries in the module-level __all__ list
alphabetically to satisfy RUF022, specifically moving AP_CO_ALARM_RAW before
AP_ENTITY_PARAMS and AP_HANDLED_VALUES while preserving all existing exports.
In `@custom_components/addhon/client/transport/mqtt.py`:
- Around line 460-463: Replace the bare exception suppression around
observe_mqtt_update in the MQTT update callback with a DEBUG-level log that
records the escaped exception, while preserving the callback thread’s
failure-safe behavior and avoiding propagation. Use the module’s existing logger
and exception logging conventions.
In `@custom_components/addhon/command_dispatch.py`:
- Around line 278-311: Add a concise comment adjacent to the patch.prepare call
documenting that selector-free callbacks are the supported path and that no
selected-category callback mutation is currently performed. Do not change the
behavior of CommandPatch handling or parameter mutation.
- Around line 26-41: Replace the silent exception handling in _emit_safely and
_record_expected_safely, plus the other referenced diagnostic wrappers, with
module-level logger debug calls using _LOGGER.debug(..., exc_info=True). Keep
all diagnostic failures suppressed so command dispatch behavior remains
unchanged, and initialize _LOGGER with logging.getLogger(__name__).
In `@custom_components/addhon/diagnostics.py`:
- Line 493: Replace the `_FUTURE_MAX_VALUES * 4` character bound in the
unhandled-state assignment with a dedicated constant for the maximum text
length, keeping `_FUTURE_MAX_VALUES` exclusively as the item-count limit. Define
and name the new character-budget constant consistently with the surrounding
constants, and use it when slicing `text`.
In `@custom_components/addhon/light.py`:
- Around line 84-85: Import ClassVar from typing and annotate the class-level
_attr_supported_color_modes set as ClassVar[set[ColorMode]] to satisfy RUF012,
leaving _attr_color_mode unchanged.
In `@custom_components/addhon/number.py`:
- Around line 629-633: Update the _custom_active property to normalize the
_AROMA_ATTR value with the exported air_purifier._raw() helper before comparing
it with AP_CUSTOM_AROMA, replacing the direct str(raw) comparison and preserving
the existing None handling.
In `@tests/conftest.py`:
- Around line 142-149: Rename _install_fan_stubs to a name reflecting that it
installs shared platform stubs for fan, light, switch, select, sensor,
binary_sensor, number, and entity_platform, then update its call site and any
references consistently.
In `@tests/test_air_purifier_entities.py`:
- Around line 1762-1764: Remove the unused ON = None placeholder and its
accompanying comment from ExperimentalAirQualityLabelTest; leave the existing
_label and _experimental behavior unchanged.
In `@tests/test_log_identity_redaction.py`:
- Around line 466-484: Remove the wall-clock assertion from
test_command_event_bounds_a_very_large_mapping_quickly, which can flake on slow
or contended runners. Validate the bounded traversal behavior directly by
patching or instrumenting _sort_key and asserting it is not invoked for every
item before the payload is limited, while preserving the existing payload-size
and serialized-record bounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 991e1a22-2fae-40e0-941a-f74ac12c000f
⛔ Files ignored due to path filters (3)
tests/fixtures/ap/schema.jsonis excluded by!tests/fixtures/**tests/fixtures/contracts/air_purifier.jsonis excluded by!tests/fixtures/**tests/fixtures/contracts/dispatcher.jsonis excluded by!tests/fixtures/**
📒 Files selected for processing (68)
.github/scripts/release-policy.sh.gitignore.superpowers/sdd/2026-07-27-air-purifier-support/OPEN-ITEMS.md.superpowers/sdd/2026-07-27-air-purifier-support/progress.md.superpowers/sdd/2026-07-27-air-purifier-support/task-1-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-10-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-11-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-12-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-13-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-2-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-3-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-4-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-5-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-6-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-7-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-8-report.md.superpowers/sdd/2026-07-27-air-purifier-support/task-9-report.md.superpowers/sdd/2026-07-27-dispatcher-aggregate-fixes/progress.md.superpowers/sdd/2026-07-27-dispatcher-aggregate-fixes/task-1-report.md.superpowers/sdd/2026-07-27-dispatcher-aggregate-fixes/task-2-report.md.superpowers/sdd/2026-07-27-dispatcher-aggregate-fixes/task-3-report.mdcustom_components/addhon/__init__.pycustom_components/addhon/air_purifier.pycustom_components/addhon/binary_sensor.pycustom_components/addhon/client/engine/appliance.pycustom_components/addhon/client/engine/commands.pycustom_components/addhon/client/interfaces.pycustom_components/addhon/client/transport/mqtt.pycustom_components/addhon/command_diagnostics.pycustom_components/addhon/command_dispatch.pycustom_components/addhon/config_flow.pycustom_components/addhon/const.pycustom_components/addhon/diagnostics.pycustom_components/addhon/fan.pycustom_components/addhon/hon_client.pycustom_components/addhon/light.pycustom_components/addhon/manifest.jsoncustom_components/addhon/number.pycustom_components/addhon/param_rollback.pycustom_components/addhon/select.pycustom_components/addhon/sensor.pycustom_components/addhon/switch.pycustom_components/addhon/translations/en.jsoncustom_components/addhon/translations/it.jsontests/conftest.pytests/contract_fixtures.pytests/test_ac_write_path.pytests/test_air_purifier_contracts.pytests/test_air_purifier_entities.pytests/test_client_interfaces.pytests/test_command_dispatch.pytests/test_diagnostics.pytests/test_engine_cluster.pytests/test_entity_availability.pytests/test_entity_translation_keys.pytests/test_hon_client_realtime.pytests/test_log_identity_redaction.pytests/test_number_setpoints.pytests/test_options_flow.pytests/test_program_options.pytests/test_release_policy.pytests/test_sensor_per_type.pytests/test_stub_hygiene.pytests/test_switch_params.pytests/test_tier2_sensors.pytests/test_translations.pytests/test_transport_mqtt.pytests/test_wash_option_params.py
Comment on lines
+1093
to
+1114
| async def async_select_option(self, option: str) -> None: | ||
| if option not in self._attr_options: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="invalid_setpoint", | ||
| translation_placeholders={ | ||
| "value": option, | ||
| "allowed": ", ".join(self._attr_options), | ||
| }, | ||
| ) | ||
| appliance = self._appliance | ||
| client = self._hon_client | ||
| if not appliance or not client: | ||
| raise HomeAssistantError( | ||
| translation_domain=DOMAIN, | ||
| translation_key="appliance_or_client_unavailable", | ||
| ) | ||
| raw = AP_OPTION_TO_AROMA[option] | ||
| values: dict[str, object] = {"value": raw} | ||
| if raw == AP_CUSTOM_AROMA: | ||
| values["time_on"] = self._custom_time(_AROMA_TIME_ON_ATTR) | ||
| values["time_off"] = self._custom_time(_AROMA_TIME_OFF_ATTR) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
No power guard on the write path, unlike the sibling AP number entity.
available hides the select while the purifier is stopped, but a service call still reaches async_select_option, which then dispatches an aroma patch — exactly what the class docstring says must not happen. HonAirPurifierTimeNumber.async_set_native_value refuses instead of trusting the UI; mirror that here.
🛡️ Proposed guard
async def async_select_option(self, option: str) -> None:
+ if not environment_available(self._attributes):
+ raise HomeAssistantError(
+ translation_domain=DOMAIN,
+ translation_key="appliance_not_running",
+ )
if option not in self._attr_options:Needs a matching translation key in translations/en.json / it.json.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def async_select_option(self, option: str) -> None: | |
| if option not in self._attr_options: | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="invalid_setpoint", | |
| translation_placeholders={ | |
| "value": option, | |
| "allowed": ", ".join(self._attr_options), | |
| }, | |
| ) | |
| appliance = self._appliance | |
| client = self._hon_client | |
| if not appliance or not client: | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="appliance_or_client_unavailable", | |
| ) | |
| raw = AP_OPTION_TO_AROMA[option] | |
| values: dict[str, object] = {"value": raw} | |
| if raw == AP_CUSTOM_AROMA: | |
| values["time_on"] = self._custom_time(_AROMA_TIME_ON_ATTR) | |
| values["time_off"] = self._custom_time(_AROMA_TIME_OFF_ATTR) | |
| async def async_select_option(self, option: str) -> None: | |
| if not environment_available(self._attributes): | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="appliance_not_running", | |
| ) | |
| if option not in self._attr_options: | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="invalid_setpoint", | |
| translation_placeholders={ | |
| "value": option, | |
| "allowed": ", ".join(self._attr_options), | |
| }, | |
| ) | |
| appliance = self._appliance | |
| client = self._hon_client | |
| if not appliance or not client: | |
| raise HomeAssistantError( | |
| translation_domain=DOMAIN, | |
| translation_key="appliance_or_client_unavailable", | |
| ) | |
| raw = AP_OPTION_TO_AROMA[option] | |
| values: dict[str, object] = {"value": raw} | |
| if raw == AP_CUSTOM_AROMA: | |
| values["time_on"] = self._custom_time(_AROMA_TIME_ON_ATTR) | |
| values["time_off"] = self._custom_time(_AROMA_TIME_OFF_ATTR) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@custom_components/addhon/select.py` around lines 1093 - 1114, Update
HonAirPurifierAromaSelect.async_select_option to reject writes when the
appliance is powered off, before constructing or dispatching the aroma patch,
matching the guard behavior in HonAirPurifierTimeNumber.async_set_native_value.
Raise HomeAssistantError using a new translation key for the stopped-power
condition, and add matching English and Italian entries in the translation
files.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
`air_purifier._raw` was the module's canonicalization rule but stayed private,
so seven air-purifier read paths hand-rolled a weaker `str(raw)` instead:
fan.is_on and fan._raw_mode, light._raw_level, the lock/tone switch, the aroma
select, the aroma-timing number, and the eco binary through the shared platform
comparison. `environment_available` already used the rule, so the same
`onOffStatus` was resolved two different ways.
This is NOT a live bug fix. The engine's HonAttribute.value routes through
str_to_float, which folds bool, int and integral float onto an int before a
platform sees the value, so `str(raw)` matches the schema for every spelling the
cloud actually uses. The reachable divergence is a decimal-spelled numeric
string ("1.0"), which str_to_float keeps as a float; the worst case is the aroma
timings, which would hide AND refuse the write immediately after the user
selected Custom.
- rename `_raw` to `raw_text`, export it, and rewrite its docstring: it used to
justify itself with the bool case, which the engine makes unreachable, and
that is what made the hazard look bigger than it is.
- add `is_engaged` so the eco binary resolves its own value in this module
instead of through the platform's generic `on_value` comparison, which is
shared with every other appliance family.
The equivalent shared reads (binary_sensor.HonBinarySensor.is_on, sensor._mapped,
switch.HonSettingsSwitch.is_on) keep the older platform convention: they serve
eight appliance types whose behavior is pinned, and the engine protects them
identically.
Tests: ShadowSpellingTest drives all seven paths from real HonAttribute fixtures,
since a plain-string fixture cannot reproduce a divergence the engine creates.
RawTextRuleTest pins the helper's own branches. The structural guard is per
member via AST, not a whole-file grep, because three of these modules also serve
appliances that legitimately keep `str(raw)`. Reverting any one of the seven
sites fails both a behavioral and the structural test; a no-op control mutation
survives. 1575 passed.
`available` hid the aroma select and the two timing numbers while the purifier
was stopped, but neither write path re-checked it, so a service call still
dispatched. A hidden entity is not an unreachable one: a script keeps calling the
service, and the snapshot `available` reads can be a refresh behind the device.
The class docstring already stated the rule this now holds, that selecting a mode
must never implicitly start the appliance.
The timing numbers did have a write-path refusal, but on Custom rather than on
power, and that does not cover this: Custom is a SETTING the device can retain
while stopped, so aromaStatus=4 with onOffStatus=0 passed. The power check goes
first, since a stopped purifier outside Custom fails both and turning Custom on
would not help.
Unreported power stays a refusal. `environment_available` treats "not confirmed
on" as not running, which is the same rule the read side already applies to the
same attribute; letting the write path be more trusting than the read path is the
inconsistency, not the refusal.
The new key exposed a gap: nothing verified that a translation_key raised by the
code exists in the JSON at all. A missing one reaches the user as the raw key,
and only when the error fires. ExceptionKeyParityTest derives them by AST over
the whole component tree and checks both directions. Not by pattern: keyword
order is free, so translation_placeholders={"error": str(err)} before
translation_key hides the raise from any expression that cannot cross a bracket,
and a top-level-only scan would call a key that moved under client/ unused.
11 keys raised, 11 declared per language, exact match both ways.
Mutation: removing either guard, swapping the number's two checks, or deleting
the key from one language each fails; a no-op control survives. 1585 passed.
`test_air_purifier_entities.py` carried its guard at line 490 of 2561, with 54 module-level definitions below it, accumulated by appending a class per task. The reported failure mode is not real: `python3 tests/test_air_purifier_entities.py` crashes at import with "module 'homeassistant.exceptions' has no attribute 'HomeAssistantError'", because the module's own stub installer needs conftest's base stubs, which only pytest puts in place. Nothing was silently skipped; under pytest the guard is inert wherever it sits. What it was is a dead entry point that reads as the end of the file and strands everything after it, with no signal to the next reader appending a class. Moved to the end. Whether a guard WORKS is a separate, pre-existing and much wider question: a standalone sweep of the corpus shows many modules failing or crashing on their stub bootstrap, and curing one of them as a rider here would leave the others untouched. CI runs `python -m pytest tests/ -q`. MainGuardPlacementTest fences it. The check is the placement one only, and it is deliberately stricter than the defect: any trailing module-level STATEMENT counts, not just a definition, since the rule is that the guard is last. The operator must be `==` so an inverted `if __name__ != "__main__":` cannot be mistaken for a guard that a real one then hides behind, and both operand orders are recognized. The sweep asserts floors on modules read and guards seen, so an empty scan cannot pass as a clean one. Its synthetic sources are assembled from a constant rather than written literally, so this module's own text still carries exactly one real guard. Mutation: counting only classes, dropping the operator check, dropping the operand order, or pointing the sweep at nothing each fails; restoring the guard mid-file fails two. 1590 passed.
The trigger format was written out in four places. Widening the regexes to accept a numbered beta updated one of them, leaving release-intake.yml telling the operator to push a shape that was no longer the only accepted one, and leaving docs/release-workflow.md describing a pipeline that has not existed for several releases. - release-policy.sh owns the prose: RELEASE_TAG_FORMATS and PR_TAG_FORMATS, used by both die() messages and by the workflow. - docs/release-workflow.md described pushing a bare vX.Y.Z tag by hand. That is the PROTECTED tag the automation creates itself on the squash commit; the operator pushes pr-vX.Y.Z. Rewritten around the two tags, with the numbered beta, the push-dev-first ordering, the squash-only requirement and the reason the policy is read from origin/main. The interpolation is DEFAULTED, and must stay defaulted. The workflow body ships with the tag while the policy is sourced from origin/main, so every release runs a new body against the previous policy: a constant introduced and used in one change does not exist yet when it first runs. Under `set -euo pipefail` that is a fatal unbound variable, and it would have killed the invalid-trigger branch one statement before delete_remote_tag, leaving the rejected trigger on origin. A leftover trigger makes a re-push a no-op that never re-fires intake, so recovery would have been a manual deletion. Reproduced against origin/main's policy before fixing. Tests. The prose is no longer read, it is EXERCISED: concrete tags are built out of each constant and fed to the predicates, and each advertised trigger shape must map onto the release shape advertised beside it. SplitSourceContractTest pins the lag contract, both structurally (no workflow may read a policy variable without a default) and end to end (the rejection branch, run against a policy stripped of both constants, still reaches the cleanup). The operator doc's tag blocks must equal the advertised shapes, and no workflow message may spell a format itself. Mutation: dropping the default, deleting a constant, narrowing the regex under stale prose, advertising a shape the regex rejects, hardcoding the format in the workflow again, and adding or removing a shape in the doc each fail. 1598 passed.
The Italian binary read "Indicazione Monossido" while the sensor on the SAME coLevel attribute read "Monossido di Carbonio", so the pair looked like two different substances on one device page. English never diverged: both labels were built from "Carbon monoxide". SharedAttributeNamingTest fences it, with the pairs DERIVED from the description tables rather than listed, so an entity that reuses an existing attribute is covered the day it lands. That surfaced a third pair, `errors`, which is exempt for a stated reason: the PROBLEM binary takes its name from the Home Assistant device class, which is the platform's vocabulary and not this feature's to align. ENUM is deliberately not an exemption, since it declares the value type and not a name, and the carbon-monoxide binary is not exempt either because it carries no device class at all, precisely so it is never presented as a certified detector. The rule is EQUALITY of the referent, not overlap. Overlap was the first version, and it passed with the bug restored: "Indicazione Monossido" and "Monossido di Carbonio" share "monossido". A truncated referent is exactly the shape that reads as two things, so nothing short of equality catches it. That makes the noise list load-bearing, and the tempting way to silence a failure is to add the missing noun to it. Referent SIZES are therefore compared across languages, so hiding "carbonio" leaves Italian naming the substance with one word where English uses two, and fails there instead. The sweep also pins which pairs are exempt, so a future entity carrying a device class cannot drop its pair out unnoticed. What this cannot judge is whether the shared name is the right one: two labels that are identically wrong pass by construction. Said so in the docstring, and cross-referenced from the filter-label test, which keeps the overlap rule on purpose because those two read DIFFERENT attributes and must stay distinguishable. Mutation: the Italian truncation, the same truncation in English, a diverging air-quality pair, a device class on the monoxide binary, splitting a pair by attr_key, extending the noise list, and a new device-classed entity joining a swept attribute each fail. 1600 passed.
The two sides of the correlation are spelled differently and neither is wrong. What leaves the machine is a parameter's intern_value, typed str | float. What comes back is the cloud's raw parValue, taken verbatim from the MQTT frame, so it can be a string, a number or a bool, and the cloud may reformat it. A plain str() on both sides calls "60" against "60.0" a missing key. The report is the lesser half. `_match_coverage` uses the same comparison to pick WHICH pending command a push confirms, so a spelling difference does not just mis-report a field, it can hand the push to the wrong command. Both sides are therefore canonicalized where they ENTER, so every comparison downstream is consistent. `comparable_text` lives in debug_utils, the leaf module command_diagnostics already imports. Numbers compare numerically, everything else as trimmed text. Bools are taken first because the rest goes through str(), and "True" is not something a numeric parse accepts. A decimal comma is read as a decimal point for the same reason `client.helpers.str_to_float` reads it: the cloud sends that spelling and the engine already stored 5.5, so calling it a mismatch would contradict the value the integration holds. Deliberately more forgiving than `air_purifier.raw_text`, which prepares a value to be WRITTEN and must never invent a spelling the schema does not declare. Nothing here reaches the wire. Forgiving about spelling, though, never about value: overflow spellings keep their raw text, because collapsing them onto "inf" would make "1e400" and a 400-digit number compare equal. Three things the verification pass corrected in this change. The expected side had no test, and it is not defensive: a range parameter's intern_value really is a float. The bool branch was documented as guarding float(True), which is never evaluated since str() runs first; the real reason is the "True" literal. And the overflow branch was pinned by a test that passed with it deleted, because "nan" and "inf" render the same either way; it now uses "infinity" and "1e400", which do not. Mutation: un-normalizing either side, deleting the bool, overflow or decimal-comma handling, and losing the right-pending-command correlation each fail; an equivalent rewrite passes. 1611 passed.
`CommandDispatcher.dispatch` reports three outcomes: it raises, it returns True, or it returns False having rolled the transaction back. `async_dispatch_patch` discarded that False, so the entity went on to refresh and showed the user a success on a write the hOn service never accepted. But that branch is the one that never fires. A real refusal arrives as ApiError: api.send_command returns a literal True or False, and HonCommand._send_parameters raises on anything falsy. So the refusal that actually happens reached the entity as a GENERIC failure carrying the untranslated literal "Can't send command", which an Italian user read as "Comando non riuscito: Can't send command", while a carefully worded localized string sat on the unreachable branch. Both paths are handled now, and ApiError has a single raise site meaning exactly this, so the mapping is exact rather than a guess. Same defect in the diagnostics: outcome="cloud_rejected" was emitted only on the dead branch, so every real refusal was recorded as a generic "error", indistinguishable from a transport or preparation failure, and anyone counting service-side refusals in a downloaded diagnostic got zero. A refusal is now labelled as one however it arrives, and nothing else may carry the label. The acceptance check is `is not True`, the same rule the dispatcher applies, and by identity rather than equality: 1 equals True in Python, and a client answering with an int is not a client confirming a write. That identity is now pinned. `_run_on_hon_loop` is stubbed by every test that goes near it, so nothing observed what the real executor hop returns, and the one assertion that touched it used assertTrue, which passes for any truthy value. The new test drives the real hop on a real background loop and asserts the value comes back as the SAME object. It earns its place: an earlier revision of this branch carried a stray edit turning that return into `1 if _v is True else _v`, which made every successful purifier write raise "the service did not accept the command", with the whole suite green. The edit is gone and the test now fails on it. Mutation, whole suite: dropping the ApiError mapping fails 2; reverting the outcome label fails 1; labelling every failure as a refusal fails 1; removing the acceptance guard fails 9; loosening it to `is False`, `not accepted` or `!= True` fails 1 each; coercing the loop hop's True fails 1; deleting the Italian key fails 5. 1621 passed.
Five reviewer nitpicks, all introduced by this campaign, each with the drift fenced rather than just repaired. - .gitignore excluded `/.superpowers/`, the tree whose campaign records this branch TRACKS. Every new task report was ignored silently and landed only when someone remembered `git add -f`; the committed ones stayed visible purely because a tracked file outranks .gitignore. Now the CONTENTS are excluded with sdd/ re-included, since git never descends into an excluded directory and a negation inside one can never re-include anything. Some working copies also carry an untracked `.superpowers/sdd/.gitignore` holding `*`, written by local tooling and not part of this repository, which still wins there; noted in place. - conftest's `_install_fan_stubs` stubbed SEVEN platforms and its docstring listed five, having grown one per task. Renamed to `_install_entity_platform_stubs`, docstring corrected, and a test now asserts the docstring against the platforms the function actually stubs: a reader deciding whether their module needs its own stub was reading a list that had been wrong for weeks. - A dead `ON = None` in the experimental air-quality test, whose comment described an approach never taken. - diagnostics used `_FUTURE_MAX_VALUES * 4` to cap a string LENGTH, so a constant documented as a number of values read as "20 values" while meaning "80 characters". Split into its own `_FUTURE_MAX_VALUE_CHARS`. - air_purifier.__all__ had drifted out of order as each task appended to the end, and it was not the module's real surface: AP_CUSTOM_AROMA is imported by both the aroma select and the timing numbers while absent from the list. Sorted, the name added, and a test pins both the order and the direction that matters, that no module imports a name the module does not export. The reverse stays allowed: a constant may exist to state a rule, as AP_WRITABLE_MODES does. 1624 passed.
Four pre-existing reviewer nitpicks. None is a bug today; each is a place where a real failure would have looked like a healthy state. The contract fixture loader compared a case's RAW id against a set of normalized strings, so a duplicate slipped whenever the spellings differed: "1" recorded first, then a bare 1, matched nothing and collapsed onto the same entry. Two cases then shared an id and the second silently shadowed the first in any id-keyed lookup, across every contract matrix in the suite. Normalized once, then both compared and stored, and the new test covers both orders plus a control. The six diagnostics wrappers in command_dispatch and the MQTT correlation call swallowed every exception with a bare `pass`. Swallowing is correct and stays: diagnostics must never affect a command, and a broken diagnostic must not drop an appliance state update. What was wrong is that it left no trace, so a correlation that was dead for every single command was indistinguishable from one that simply never matched. The module had no logger at all; it has one now, and the new test asserts both halves, that the command still commits and that each wrapper that fired recorded why. The bounded-traversal test asserted an absolute 0.2s against a real measurement well under a millisecond. That pinned nothing about the shape of the work and would have gone red on a contended runner that merely stalled. It was also nearly blind in the other direction: restoring the unbounded pre-limit sort measures 0.205s here, a 2.5 percent margin over the old threshold, so on a slightly faster machine the regression it exists to catch would have passed. It now times the same call on a small collection in the same process and compares the ratio, which cancels machine speed and load: flat work stays within a wide factor while the unbounded version is a thousandfold apart, and it fails on that mutation with 7x of margin. The trimming assertions moved to their own test, since they were never about timing. 1627 passed, three consecutive runs.
A mutation sweep over the whole unpushed delta found one survivor: removing the character slice from the future-capability section entirely left all 1627 tests green. The section is passive EVIDENCE, so a firmware answering with a long blob must add a hint to the dump and never carry the blob into it, and nothing checked that. The bound is easy to lose because it had already been written twice: first as a count constant times four, reading as "20 values" while meaning 80 characters, and then split into its own `_FUTURE_MAX_VALUE_CHARS`. Neither spelling was covered. The test derives the expected length FROM the constant, so the mechanism stays pinned at whatever value it takes, and bounds the constant separately: a cap large enough to carry the blob would satisfy the mechanism while defeating its purpose. A control asserts a value that fits is not trimmed. The AP coordinator builder takes attribute overrides now, which no existing caller notices. Mutation: dropping the slice fails 1, widening the cap to 8000 fails 1. 1629 passed.
comparable_text promises the one thing a correlation cannot do without: forgiving about spelling, never about value. It already guarded the overflow band, where str() would render every too-large spelling as "inf" and make "1e400" and a 400-digit number compare EQUAL. Precision, though, is lost well before inf. A float holds every integer only up to 2**53; above that str(int(number)) renders the ROUNDED double, so 12345678901234567890 and 12345678901234567891 both land on 12345678901234567168 and _match_coverage would score a match that did not happen. Same rule as the overflow branch, applied one step earlier: past 2**53 keep the raw text. Below the bound nothing changes, so the reachable band, schema-declared settings a few digits wide, keeps the numeric comparison the delta added it for. Reachability today is nil, which is why this is a contract repair and not a bug fix: record_expected_update is fed prepared.payload, built only from active_parameters[key].intern_value, and transactionId, timestamp and macAddress join the envelope later in client/transport/api.py, never that payload. The docstring, however, stated the invariant absolutely, and a promise that holds only below an undocumented bound is the kind a later caller relies on. The test pins WHERE the bound is, not merely that large numbers survive: 2**53 + 1 has no float of its own and rounds onto 2**53, so that pair is the first one that collides. Mutation evidence, tests/test_debug_utils_redact.py: guard removed (if False) -> 1 failed bound widened to 2**63 -> 1 failed bound narrowed to 2**32 -> 1 failed Full suite 1630 passed, 1 skipped, 7 xfailed, 319 subtests.
Two holes, both found by an adversarial pass over the whole delta rather than by
the commit that added the cap.
The control, test_a_short_unhandled_state_value_is_untouched, had a body identical
to the pre-existing test_future_capability_reports_an_unhandled_live_state: same
_ap_block() with no overrides, same {"machMode": "3"} assertion. Two identical
bodies over one deterministic fixture have provably equal discriminating power, so
no mutation could fail one and spare the other. It read as an independent control
while adding nothing.
The cap test fed a blob of one repeated character, so `blob.startswith(captured)`
was satisfied by ANY window of the right length. A slice that kept the correct
NUMBER of characters and the wrong ones was invisible.
Both now use a counting run where no character equals its neighbour, and the
control sits ON the bound: a value exactly _FUTURE_MAX_VALUE_CHARS long must come
back whole. A short value cannot tell a slice at the cap from one a character
either side of it, which is why the old control could not fail alone.
Mutation evidence against custom_components/addhon/diagnostics.py:498:
text[1:cap + 1] shifted window -> 4 failed (was invisible to the old pair)
text[:cap - 1] -> 2 failed
text[:cap + 1] -> 1 failed (was invisible to the old pair)
text slice removed -> 1 failed
Production code untouched. Full suite 1630 passed, 1 skipped, 7 xfailed,
319 subtests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
custom_components/addhon/select.py (1)
1079-1087: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
raw_textinstead of re-spelling the canonicalization here.
raw_textis already imported in this module and is the declared single rule for schema spelling; lines 1087 hand-roll the same integral-float collapse. Routing through it keeps one rule for the whole feature (and keeps this site aligned ifraw_textever grows a case).♻️ Proposed refactor
if low <= number <= high: - return str(int(number)) if number.is_integer() else str(number) + return raw_text(number)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/select.py` around lines 1079 - 1087, Update the candidate normalization in the loop within the relevant select method to pass the accepted numeric value through the imported raw_text helper instead of manually converting integral floats and formatting other numbers. Preserve the existing candidate filtering, float parsing, range validation, and return behavior while using raw_text as the single canonicalization rule.custom_components/addhon/diagnostics.py (1)
494-505: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider flagging a value that was actually trimmed.
truncatedis currently driven only by_enum_deltas, so a state value clipped by_FUTURE_MAX_VALUE_CHARSis indistinguishable in the dump from a complete one.♻️ Optional
for name in sorted(handled): text = _scalar_text(attributes.get(name)) if text is not None and text not in handled[name]: + if len(text) > _FUTURE_MAX_VALUE_CHARS: + truncated = True unhandled_state[name] = text[:_FUTURE_MAX_VALUE_CHARS]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@custom_components/addhon/diagnostics.py` around lines 494 - 505, Update the unhandled-state processing around _FUTURE_MAX_VALUE_CHARS so truncating any state value also sets truncated to true. Preserve the existing clipped value in state_values_unhandled and the current enum-delta truncation behavior.tests/test_translations.py (1)
435-463: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe detector only sees
raise HomeAssistantError(translation_key=...)inline.A key passed to an exception that is constructed first and raised later (
err = HomeAssistantError(...)…raise err), or raised from a helper factory, is invisible here — which makestest_no_language_carries_an_unused_exceptionfail on a key that is genuinely used. Consider also walkingast.Callnodes whose func name ends inError(not justRaisenodes), or noting the limitation in the docstring so the next reader knows why a live key reads as unused.Also applies to: 482-488
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_translations.py` around lines 435 - 463, The _raised_keys detector only records localized errors created directly inside raise statements, missing exceptions constructed before raising or returned by helper factories. Extend _raised_keys to inspect relevant ast.Call nodes, including error constructors whose function name ends with “Error,” while preserving literal-key validation and avoiding duplicate handling of inline raises; alternatively, explicitly document this limitation if detection cannot be expanded.tests/test_hon_client_realtime.py (1)
158-165: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose the loop after joining the thread.
loop.stop()+joinleaves the event loop object open, so its selector fds stay allocated for the rest of the session (and Python may emit aResourceWarning).♻️ Suggested cleanup
thread = client._hon_thread if thread is not None: thread.join(timeout=5) + if loop is not None: + loop.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_hon_client_realtime.py` around lines 158 - 165, Update the cleanup in the finally block to close client._hon_loop after stopping it and joining client._hon_thread. Ensure the loop is closed only when it exists, while preserving the existing thread join timeout and shutdown sequence.tests/test_stub_hygiene.py (1)
275-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit:
ast/reare already available at module scope, and the docstring extraction fails opaquely.
astis imported at the top of this module (used by_main_guards), so the local imports are redundant. Also, if the installer ever loses its docstring or becomesasync def, this test dies withValueError/StopIterationrather than a readable assertion.♻️ Optional
`@staticmethod` def _installer_source() -> str: - import ast - source = (TESTS_DIR / "conftest.py").read_text(encoding="utf-8") tree = ast.parse(source) - function = next( + function = next( node for node in tree.body if isinstance(node, ast.FunctionDef) - and node.name == "_install_entity_platform_stubs" - ) + and node.name == "_install_entity_platform_stubs" + , None) + assert function is not None, "conftest lost _install_entity_platform_stubs" return ast.get_source_segment(source, function) or "" def test_the_docstring_lists_every_platform_it_stubs(self) -> None: - import re - body = self._installer_source()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_stub_hygiene.py` around lines 275 - 297, Update test_the_docstring_lists_every_platform_it_stubs and _installer_source to reuse the module-level ast and re imports instead of importing them locally. Make _installer_source explicitly assert that _install_entity_platform_stubs is found and is a regular function with a docstring, producing clear assertion failures before extracting the docstring; preserve the existing platform-list validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_air_purifier_entities.py`:
- Around line 2676-2678: Update the loop over self._ap_descriptions() to replace
the unused platform variable with an underscore placeholder, while preserving
the existing by_attribute population using description.attr_key and
description.key.
---
Nitpick comments:
In `@custom_components/addhon/diagnostics.py`:
- Around line 494-505: Update the unhandled-state processing around
_FUTURE_MAX_VALUE_CHARS so truncating any state value also sets truncated to
true. Preserve the existing clipped value in state_values_unhandled and the
current enum-delta truncation behavior.
In `@custom_components/addhon/select.py`:
- Around line 1079-1087: Update the candidate normalization in the loop within
the relevant select method to pass the accepted numeric value through the
imported raw_text helper instead of manually converting integral floats and
formatting other numbers. Preserve the existing candidate filtering, float
parsing, range validation, and return behavior while using raw_text as the
single canonicalization rule.
In `@tests/test_hon_client_realtime.py`:
- Around line 158-165: Update the cleanup in the finally block to close
client._hon_loop after stopping it and joining client._hon_thread. Ensure the
loop is closed only when it exists, while preserving the existing thread join
timeout and shutdown sequence.
In `@tests/test_stub_hygiene.py`:
- Around line 275-297: Update test_the_docstring_lists_every_platform_it_stubs
and _installer_source to reuse the module-level ast and re imports instead of
importing them locally. Make _installer_source explicitly assert that
_install_entity_platform_stubs is found and is a regular function with a
docstring, producing clear assertion failures before extracting the docstring;
preserve the existing platform-list validation.
In `@tests/test_translations.py`:
- Around line 435-463: The _raised_keys detector only records localized errors
created directly inside raise statements, missing exceptions constructed before
raising or returned by helper factories. Extend _raised_keys to inspect relevant
ast.Call nodes, including error constructors whose function name ends with
“Error,” while preserving literal-key validation and avoiding duplicate handling
of inline raises; alternatively, explicitly document this limitation if
detection cannot be expanded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f303809b-3eea-4b21-8c39-1c85f7931ddd
📒 Files selected for processing (30)
.github/scripts/release-policy.sh.github/workflows/release-intake.yml.gitignorecustom_components/addhon/air_purifier.pycustom_components/addhon/binary_sensor.pycustom_components/addhon/client/transport/mqtt.pycustom_components/addhon/command_diagnostics.pycustom_components/addhon/command_dispatch.pycustom_components/addhon/debug_utils.pycustom_components/addhon/diagnostics.pycustom_components/addhon/fan.pycustom_components/addhon/light.pycustom_components/addhon/number.pycustom_components/addhon/select.pycustom_components/addhon/switch.pycustom_components/addhon/translations/en.jsoncustom_components/addhon/translations/it.jsondocs/release-workflow.mdtests/conftest.pytests/contract_fixtures.pytests/test_air_purifier_entities.pytests/test_command_dispatch.pytests/test_debug_utils_redact.pytests/test_diagnostics.pytests/test_hon_client_realtime.pytests/test_log_identity_redaction.pytests/test_release_policy.pytests/test_stub_hygiene.pytests/test_translations.pytests/test_transport_mqtt.py
🚧 Files skipped from review as they are similar to previous changes (4)
- custom_components/addhon/translations/en.json
- custom_components/addhon/translations/it.json
- .github/scripts/release-policy.sh
- tests/conftest.py
greptile filed this as P1 and it was closed as accepted-by-design, on the grounds that the purifier exposes discrete modes and deliberately does not declare SET_SPEED. The first half is verifiable and true: _attr_supported_features carries only PRESET_MODE, TURN_ON and TURN_OFF, and no percentage, speed_count or percentage_step exists anywhere in the package. The second half does not follow. Not advertising a speed justifies not implementing one; it does not justify accepting a percentage and discarding it. Dropped silently, a percentage is the worst of the three outcomes: the purifier starts in the REMEMBERED mode, the service returns success, and the automation reads as though the requested speed had been applied. Refusing costs a visible error on a call that was never going to do what it asked, and it costs nothing on any call Home Assistant itself makes, since the UI and the voice intents offer no percentage for an entity without SET_SPEED. The parameter stays in the signature because the service passes it positionally. The second test is what keeps the refusal coherent: an entity that declared SET_SPEED and then refused every percentage would be worse than either choice alone, so the absence of the feature is now pinned rather than assumed. Mutation evidence: refusal removed (if False) -> 1 failed SET_SPEED added to the declared features -> 1 failed translation_key renamed to an undeclared one -> 3 failed Full suite 1632 passed, 1 skipped, 7 xfailed, 319 subtests.
The pair sweep at test_the_sweep_finds_the_pairs_it_is_meant_to reads only description.attr_key and description.key, so the platform half of the tuple was bound and never used. The sibling sweep above it does use both, which made the difference easy to miss. Named _platform rather than a bare underscore, the spelling the rest of the tree uses. No linter in CI enforces this; it is a readability change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Automated release PR for
v5.10.0-beta.Summary by Sourcery
Add transactional command dispatching, detailed command diagnostics, and air purifier (AP) support (fan, light, switches, aroma control, experimental entities, and diagnostics) while tightening options handling, Home Assistant stubs, and release tagging.
New Features:
Bug Fixes:
Enhancements:
CommandDispatcherand synchronous patch-dispatch helper that run patches on the internal event loop.Build:
Tests:
Summary by CodeRabbit
-beta1,-beta2).Greptile Summary
This release adds transactional command dispatch and comprehensive air-purifier support while refining diagnostics, options, test infrastructure, and beta-release handling.
-betaand numbered-betaNreleases.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the boolean power-state handling now canonicalizes unwrapped booleans, and explicit unsupported percentages are rejected without affecting ordinary turn-on calls.
Important Files Changed
Sequence Diagram
sequenceDiagram participant HA as Home Assistant Entity participant Intent as AP Intent Builder participant Dispatcher as Command Dispatcher participant Engine as Hon Command Engine participant Device as Cloud / Appliance HA->>Intent: Build capability-validated patch Intent-->>HA: Sparse command patch HA->>Dispatcher: Dispatch patch Dispatcher->>Engine: Apply transactional parameters Engine->>Device: Send canonical payload alt command succeeds Device-->>Engine: Success Engine-->>Dispatcher: Updated shadow Dispatcher-->>HA: Refresh coordinator else command fails Device-->>Engine: Error Engine-->>Dispatcher: Failure Dispatcher->>Engine: Roll back owned mutations Dispatcher-->>HA: Localized command error endReviews (3): Last reviewed commit: "chore: mark an unused loop variable as u..." | Re-trigger Greptile