docs: address review feedback on feature gating, forward refs, and test paths

Author: allenporter

.agents/skills/review/SKILL.md

@@ -7,7 +7,7 @@ description: Conduct authoritative code review on python-roborock changes agains
 
 Use this skill when reviewing pull requests, inspecting code changes, or verifying new device traits in `python-roborock`.
 
-All reviews are evaluated against the engineering conventions defined in [AGENTS.md](../../AGENTS.md).
+All reviews are evaluated against the engineering conventions defined in [AGENTS.md](../../../AGENTS.md).
 
 ---
 
@@ -23,23 +23,25 @@ All reviews are evaluated against the engineering conventions defined in [AGENTS
    - Are trait models subclassing `RoborockBase` with `@dataclass`?
    - Is `TypedDict` completely avoided for domain data models?
    - Is `Any` strictly prohibited on public trait boundaries?
+   - Are forward references (`"ClassName"`) and `typing.TYPE_CHECKING` guards avoided? (Needing them usually indicates circular dependencies or coupling that should be refactored).
+   - Are non-universal traits gated by device feature flags (`device.features`) and supported categories?
    - Are trait abstractions consumer-agnostic (not hardcoded exclusively to Home Assistant entities)?
    - Are transport details (keys, tokens, sockets) cleanly isolated from traits?
 
 3. **Evaluate Layer 2: Lifecycle, State & Simplification**:
    - **Value-Returning Helpers**: Do helper functions return values instead of modifying `self` via hidden side effects?
    - **Direct Flow**: Is the execution path straightforward and direct, avoiding unnecessary indirection or ping-ponging between methods?
-   - **Concurrency**: Does 1:1 async command-response matching use `asyncio.Future` mapped by `msg_id`?
+   - **Concurrency**: Does 1:1 async command-response matching use `asyncio.Future` correlated by protocol request/sequence ID (`request_id` or `msg_id`)?
    - **Teardown**: Are all tasks, event listeners, and channels cleanly unhooked in `RoborockDevice.close()`?
-   - **Capability Gating**: Is feature gating checking BOTH protocol version (`pv`) and product category (`RoborockCategory`)?
+   - **Capability Gating**: Is capability gating checking protocol version, category (appropriate for device family), and device feature flags?
 
 4. **Evaluate Layer 3: Wire Protocols, Codecs & Resilient Enums**:
    - Do status and error code enums subclass `RoborockEnum` (with lowercase `unknown = -1` member) or `RoborockModeEnum` (with `from_code_optional()`)?
    - Are unexpected wire codes prevented from crashing via unhandled `ValueError`?
    - Is wire-contained `Any` properly scoped to genuine protocol polymorphism (e.g. Tuya DPS maps)?
 
 5. **Evaluate Tests & Fixtures**:
-   - **Test Mirroring & Colocation**: Are tests colocated in the matching mirror path under `tests/` for the module being tested (e.g., tests for `roborock/devices/traits/battery.py` in `tests/devices/traits/test_battery.py`; new modules have corresponding new test suites under the mirror directory)?
+   - **Test Mirroring & Colocation**: Are tests colocated in the matching mirror path under `tests/` for the module being tested (e.g., tests for `roborock/devices/traits/v1/status.py` in `tests/devices/traits/v1/test_status.py`; new modules have corresponding new test suites under the mirror directory)?
    - **No One-Off Bugfix Test Files**: Flag any fragmented single-bug or single-PR test files (e.g., `tests/test_battery_low_voltage_fix.py`). Tests must be integrated into the module's corresponding test suite.
    - Are tests using `fake_channel` and `@pytest.mark.parametrize` rather than hand-rolled mocks or duplicate methods?
    - Do assertions verify public behavior rather than private `_state` variables?

AGENTS.md

@@ -22,17 +22,17 @@ uv venv
 uv sync
 
 # Run tests
-pytest
+uv run pytest
 
 # Run tests for a specific trait or protocol
-pytest tests/devices/traits/test_battery.py
-pytest tests/protocols/test_v1.py
+uv run pytest tests/devices/traits/v1/test_status.py
+uv run pytest tests/protocols/test_v1_protocol.py
 
 # Lint and typecheck
-pre-commit run --all-files
+uv run pre-commit run --all-files
 # or directly:
-ruff check roborock tests
-mypy roborock tests
+uv run ruff check roborock tests
+uv run mypy roborock tests
 ```
 
 ---
@@ -49,7 +49,7 @@ Review every proposed change against the repository's three core architectural l
    - **Radical Simplification & Linear Data Flow**: Keep logic simple and readable. Avoid unnecessary indirection, multi-layered helper cascades, or back-and-forth ping-ponging across methods.
    - **Value-Returning Helpers Over Side Effects**: Helper functions MUST be pure transformations that compute and return values (e.g., dictionaries, tuples, or dataclasses) rather than mutating internal object state as a side effect. Make state assignments explicit and visible at the call site (e.g., `new_data = self._parse_response(response, segment_map); self._update_trait_values(new_data)`).
    - **Cohesive Lifecycle**: All event listeners, callbacks, background tasks, and channels cleanly torn down in `close()`.
-   - **Concurrency**: Request/response matching across async push channels uses `asyncio.Future` mapped by message ID (`msg_id`).
+   - **Concurrency**: Request/response matching across async push channels uses `asyncio.Future` correlated by protocol request or sequence ID (`request_id` for V1 RPC, `msg_id` for B01/Tuya).
    - **Presentation vs State**: Transform data in render pipelines; never mutate cached telemetry state or raw packets for presentation effects.
 
 3. **Priority 3: Wire Protocol Parsers, Codecs & Cryptography**
@@ -72,18 +72,21 @@ Review every proposed change against the repository's three core architectural l
 - **Strongly Type What You Know; Contain `Any` to the Wire**:
   - Public trait APIs, method signatures, properties, and domain models MUST declare concrete types (`RoborockBase` dataclasses, enums, or primitives). Never use `Any` or raw `dict` as a lazy shortcut for domain models that can be typed.
   - `Any` is natural, expected, and accepted where the underlying wire protocol or transport is genuinely dynamic or polymorphic (such as Tuya DPS maps, low-level channel RPC dispatch, serialization helpers, or evolving cloud API schemas).
+- **Avoid Forward References & `TYPE_CHECKING`**: Avoid stringified forward references (`"ClassName"`) and `if typing.TYPE_CHECKING:` guards wherever possible. Needing `TYPE_CHECKING` or forward references is typically a code smell indicating circular dependencies or architectural coupling that should be resolved by refactoring shared models or reorganizing module boundaries.
 - **Explicit Parameter Requirements**: Do NOT mark arguments or dataclass fields as `Optional[...]` or default them to `None` if they are always required by the protocol or caller.
 - **Trust Type Annotations**: Prohibit defensive runtime `isinstance` checks on statically typed parameters in business and trait logic (e.g. `def parse_map(data: Q10MapInfo): if not isinstance(data, Q10MapInfo): ...`). Do not add redundant runtime guards where static types suffice. (Note: dynamic payload unpacking and wire-level type narrowing on untyped raw inputs in `from_dict` or RPC deserializers is legitimate and expected).
 
 ### 2. Protocol & Device Communications
 - **Cryptographic & Transport Layer Decoupling**: Encapsulate all AES encryption, MD5 hashing, protocol salts, raw TCP sockets, and MQTT credentials strictly within `roborock.protocols` and `roborock.devices.transport` / `roborock.devices.rpc`.
 - **Channel Trait Boundary**: Traits MUST only receive an abstract communication channel (e.g., `Channel`, `RpcChannel`). NEVER pass device local keys, security tokens, IP addresses, or transport sockets into `Trait` instances.
-- **RPC Correlation via `asyncio.Future`**: Asynchronous 1:1 request/response matching across MQTT push channels MUST use `asyncio.Future` mapped by message sequence ID (`msg_id`). The channel creates a loop future, registers unsubscription cleanup, and sets the future result/exception upon message arrival. (Multi-packet query streaming in `a01_channel.py` legitimately uses `asyncio.Event` with a result accumulator).
+- **RPC Correlation via `asyncio.Future`**: Asynchronous 1:1 request/response matching across MQTT push channels MUST use `asyncio.Future` mapped by the protocol's sequence or request ID (`request_id` for V1 JSON-RPC, `msg_id` for B01 Tuya DPS). The channel creates a loop future, registers unsubscription cleanup, and sets the future result/exception upon message arrival. (Multi-packet query streaming in `a01_channel.py` legitimately uses `asyncio.Event` with a result accumulator).
 - **Protocol Version Isolation**: Keep V1 (vacuum JSON-RPC), B01 (Q7/Q10 Tuya DPS), and A01 (Dyad/Zeo Tuya DPS) parser pipelines strictly isolated. Do not bleed Tuya DPS decoding logic into V1 or vice versa.
 - **Rendering Transformations vs State Mutation**: When changing how data is displayed or rendered (such as clearing map traces while docked), transform the values within the render pipeline. NEVER mutate or discard cached protocol packet state to achieve rendering effects.
 
 ### 3. Trait Design & Client Integration Idioms
-- **Compound Capability Gating**: Capability and trait availability MUST be gated by BOTH protocol version AND `RoborockCategory` using authentic codebase attributes: protocol version string `device.pv == "1.0"` (or `device.pv == DeviceVersion.V1`) and product category `product.category == RoborockCategory.VACUUM`. Never assume protocol V1 ("1.0") implies a vacuum robot; mowers and wet/dry vacuums also share protocol variants.
+- **Compound Capability & Feature Gating**: Capability and trait availability MUST be gated by protocol version, category, and device feature flags:
+  - Check both protocol version string (`device.pv == "1.0"` or `device.pv == DeviceVersion.V1`) AND the appropriate product category (`product.category == RoborockCategory.VACUUM` for robot vacuums, or `RoborockCategory.WET_DRY_VAC` / `WASHING_MACHINE` for A01 appliances). Never assume protocol V1 ("1.0") implies a vacuum robot; mowers and wet/dry vacuums also share protocol variants.
+  - Non-universal capabilities and traits must be gated by device feature flags (`device.features`) or supported schema flags rather than assumed to be present on all devices.
 - **Value-Returning Helpers Over Mutating Side Effects**: Methods that parse responses, extract mappings, or transform telemetry MUST compute and return values (e.g., returning a dict, tuple, or dataclass) rather than mutating internal object state as a side effect. Keep state assignment and notifications explicit and visible in the calling method.
 - **Aggressive Simplification & Direct Flow**: Avoid convoluted back-and-forth between helper methods or redundant checks for default properties. Strive for simple, linear data flow that makes all side effects immediately visible.
 - **Exhaustive Lifecycle Teardown**: All background listeners, event subscriptions, state callbacks, and channel connections MUST be cleanly unhooked and canceled inside `RoborockDevice.close()`.
@@ -93,13 +96,14 @@ Review every proposed change against the repository's three core architectural l
 ### 4. Error Handling & Exceptions
 - **Hierarchy Rooting**: All custom exceptions raised within the library MUST inherit from `roborock.exceptions.RoborockException`.
 - **Specific Exception Narrowing**: Catch only specific, expected exception classes (`RoborockTimeout`, `RoborockConnectionException`, `json.JSONDecodeError`).
+  - *Cleanup Exception*: Catching broad `Exception` is permitted only in teardown or subscription-cleanup handlers (e.g. `device.py`) to guarantee leaked channels or resources are cleanly unsubscribed before re-raising or logging.
 - **Context-Rich Parsing Exceptions**: Wrap deserialization, decoding, and schema mapping errors into `RoborockParsingException` providing rich context: `RoborockParsingException(trait_name=..., command=..., payload=..., inner_error=err)`.
 - **Guard Clauses & Early Exits**: Flatten deeply nested conditional branches by using guard clauses (`if not condition: return`). Keep the primary execution path at the lowest possible indentation level.
 
 ### 5. Test Patterns & Fixtures
-- **Test Mirroring & Module Colocation**: Place and maintain unit tests in the matching mirror path under `tests/` corresponding to the module under test (e.g. tests for `roborock/devices/traits/battery.py` belong in `tests/devices/traits/test_battery.py`; a new parser `roborock/map/q10.py` belongs in `tests/map/test_q10.py`). When extending existing functionality, augment the canonical test file rather than creating separate one-off test files.
+- **Test Mirroring & Module Colocation**: Place and maintain unit tests in the matching mirror path under `tests/` corresponding to the module under test (e.g. tests for `roborock/devices/traits/v1/status.py` belong in `tests/devices/traits/v1/test_status.py`; tests for `roborock/map/b01_q10_map_parser.py` belong in `tests/map/test_b01_q10_map_parser.py`). When extending existing functionality, augment the canonical test file rather than creating separate one-off test files.
 - **Avoid One-Off Test Files (Anti-Pattern)**: Do NOT create fragmented, single-bug or single-PR test files (such as `tests/test_battery_low_voltage_fix.py`). Integrate tests into the module's corresponding test suite.
-- **Standard Fixture Reuse**: Reuse existing shared fixtures (`fake_channel`, `message_builder`, `device_fixture`, `v1_simulator`) in `tests/`.
+- **Standard Fixture Reuse**: Reuse existing shared fixtures (`fake_channel`, `message_builder`, `device_fixture`) in `tests/`.
 - **Table-Driven Parametrization**: Use `@pytest.mark.parametrize` for testing multi-dock variants, work modes, error codes, and protocol matrices. Avoid copy-pasting duplicate test methods.
 - **Behavior-Driven Assertions**: Assert on public trait methods, return values, and emitted channel commands. Do NOT assert on private object attributes (`_state`) or internal implementation flags.
 - **State Injection via Fixtures**: Construct test scenarios by feeding `HomeData` or payload fixtures through `device_fixture`, rather than monkeypatching trait internals in test functions.