diff --git a/.agents/skills/review/SKILL.md b/.agents/skills/review/SKILL.md new file mode 100644 index 00000000..6eaec6c7 --- /dev/null +++ b/.agents/skills/review/SKILL.md @@ -0,0 +1,54 @@ +--- +name: review +description: Conduct authoritative code review on python-roborock changes against repository architectural hierarchy, typing standards, and maintainer conventions. +--- + +# Code Review Skill + +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). + +--- + +## Review Workflow + +1. **Classify Changed Files by Layer**: + Group changed files according to the 3-tier architectural hierarchy: + - **Priority 1: Public Trait APIs & Consumer Contracts** (`roborock/devices/traits/`, `roborock/data/containers.py`) + - **Priority 2: Data Lifecycle & State Management** (trait state, callbacks, `RoborockDevice.close()`) + - **Priority 3: Wire Protocols, Codecs & Cryptography** (`roborock/protocols/`, `roborock/devices/transport/`, `roborock/devices/rpc/`) + +2. **Evaluate Layer 1: Public Trait APIs & Consumer Contracts**: + - 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` 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 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/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? + +6. **Format Review Output**: + Structure review comments with clear rationale and actionable code recommendations: + - **Priority Level & File Reference** + - **Issue / Anti-Pattern Identified** + - **Grounded Rationale** (citing protocol resilience, consumer stability, or maintainability) + - **Concrete Before / After Diff or Suggested Fix** diff --git a/.claude/skills b/.claude/skills new file mode 120000 index 00000000..2b7a412b --- /dev/null +++ b/.claude/skills @@ -0,0 +1 @@ +../.agents/skills \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000..d81bd0a0 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,101 @@ +# Repository Engineering Guidelines: python-roborock + +## Overview & Scope +`python-roborock` is an asynchronous (`asyncio`) device integration library supporting multi-protocol Roborock vacuum and home appliances (V1 JSON-RPC over AES, B01 Tuya DPS for Q7/Q10, and A01 Tuya DPS for Dyad/Zeo). While Home Assistant Core is a primary downstream consumer, `python-roborock` is an independent client library whose public abstractions must remain general-purpose and consumer-agnostic. + +This document serves as the authoritative, single source of truth for engineering conventions, architectural standards, and automated code reviews across all contributors and coding agents. + +--- + +## Environment & Tooling Standards +- **Target**: Python 3.11+, Hatchling build backend, `uv` package manager. +- **Linters & Formatters**: Ruff (line length 120), Mypy (`check_untyped_defs = true`), Codespell. +- **Testing**: pytest (`pytest-asyncio`). + +### Key Developer Commands +```bash +# Environment setup +uv venv && uv sync + +# Run tests +uv run pytest +uv run pytest tests/devices/traits/v1/test_status.py +uv run pytest tests/protocols/test_v1_protocol.py + +# Lint and typecheck +uv run pre-commit run --all-files +# or directly: +uv run ruff check roborock tests +uv run mypy roborock tests +``` + +--- + +## Core Architectural Hierarchy & Review Priorities +Evaluate every change against the repository's three core layers in strict priority order: + +1. **Priority 1: Public Trait APIs & Consumer Contracts** (Highest Priority) + - Clean, stable user-facing abstractions decoupled from wire protocol details, Tuya DPS keys, encryption keys, or transport sockets. + - Strongly typed with concrete models subclassing `RoborockBase`; never expose raw protocol dictionaries or `Any` on public boundaries. + +2. **Priority 2: Data Lifecycle & State Management** + - Simple, linear data flow. Keep side effects visible: helper functions MUST be pure transformations that compute and return values rather than mutating object state as a side effect. + - Exhaustive lifecycle teardown: all listeners, background tasks, and channels cleanly unhooked in `RoborockDevice.close()`. + - Concurrency: 1:1 request/response matching across async push channels uses `asyncio.Future` correlated by protocol request/sequence ID (`request_id` for V1 RPC, `msg_id` for B01/Tuya). + - Transform data in render pipelines; never mutate cached telemetry state or raw packets for presentation effects. + +3. **Priority 3: Wire Protocol Parsers, Codecs & Cryptography** + - Encapsulate cryptography (AES, MD5), protocol framing, and raw sockets to `roborock.protocols` and `roborock.devices.transport` / `roborock.devices.rpc`. + - Keep protocol families (V1 JSON-RPC, B01 Tuya DPS, A01 Tuya DPS) strictly isolated; never conflate schemas across families. + - Enforce enum fallback resilience (`RoborockEnum` with lowercase `unknown = -1` or `RoborockModeEnum.from_code_optional()`); never crash on unexpected firmware codes. + +--- + +## Detailed Engineering Conventions + +### 1. Typing & Data Models +- **Subclass `RoborockBase`**: Define structured domain and wire data models as `@dataclass` subclassing `RoborockBase` (`from_dict`, `as_dict`). Avoid `TypedDict` or loose dicts. (Binary protocol packets, transport message envelopes, and map layers are exempt). +- **Enum Fallback Resilience**: All enums representing device status, firmware modes, error codes, and wire protocol integer codes MUST inherit from `RoborockEnum` (defining a lowercase `unknown = -1` or `0` member) or `RoborockModeEnum` (using `from_code_optional()`). Internal enums not decoding unknown firmware codes remain standard `Enum`/`StrEnum`. +- **Strongly Type What You Know; Contain `Any` to the Wire**: Public trait APIs, method signatures, properties, and domain models MUST declare concrete types. `Any` is accepted only where the underlying wire protocol is dynamic or polymorphic (Tuya DPS maps, low-level RPC dispatch, serialization helpers, evolving cloud schemas). +- **Avoid Forward References & `TYPE_CHECKING`**: Avoid stringified forward references (`"ClassName"`) and `if typing.TYPE_CHECKING:` guards wherever possible. They typically indicate circular dependencies or coupling that should be refactored by extracting shared models. +- **Explicit Parameters**: 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. (Dynamic wire payload unpacking and type narrowing in `from_dict` or RPC deserializers is legitimate and expected). + +### 2. Protocol & Device Communications +- **Channel Trait Boundary**: Traits receive only an abstract communication channel (e.g., `Channel`, `RpcChannel`). Never pass device local keys, security tokens, IP addresses, or transport sockets into `Trait` instances. +- **Version Isolation**: Keep V1, B01, and A01 parser pipelines strictly isolated. Do not bleed Tuya DPS decoding logic into V1 or vice versa. + +### 3. Trait Design & Client Integration +- **Compound Capability & Feature Gating**: Capability and trait availability MUST be gated by protocol version (`device.pv == "1.0"` or `DeviceVersion.V1`), appropriate category (`RoborockCategory.VACUUM`, `WET_DRY_VAC`, or `WASHING_MACHINE`), and device feature flags (`device.features`). +- **Linear State Flow**: Methods that parse responses or transform telemetry must return values to make state assignments explicit at the call site. Avoid multi-layered helper cascades. +- **Consumer Agnosticism**: Design traits around cohesive, general-purpose consumer concepts (`CleanHistoryTrait`, `DockTrait`, `ConsumableTrait`), not low-level firmware registers or raw Tuya DPS numbers. +- **Diagnostic Safety & Privacy**: Exclude sensitive user information (Wi-Fi SSIDs, passwords, cloud tokens, encryption keys) from device diagnostics and logs. + +### 4. Error Handling & Exceptions +- **Hierarchy Rooting**: All custom exceptions MUST inherit from `roborock.exceptions.RoborockException`. +- **Specific Exception Narrowing**: Catch only specific, expected exception classes (`RoborockTimeout`, `RoborockConnectionException`, `json.JSONDecodeError`). Catching broad `Exception` is permitted only in teardown/cleanup handlers (e.g. `device.py`) to guarantee resources are unsubscribed before re-raising or logging. +- **Context-Rich Parsing Exceptions**: Wrap deserialization, decoding, and schema mapping errors into `RoborockParsingException` with rich context (`trait_name`, `command`, `payload`, `inner_error`). +- **Guard Clauses**: Flatten nested conditional branches with early exit guard clauses (`if not condition: return`). + +### 5. Test Patterns & Fixtures +- **Test Mirroring & Colocation**: Place and maintain unit tests in the matching mirror path under `tests/` (e.g. `tests/devices/traits/v1/test_status.py` for `roborock/devices/traits/v1/status.py`; `tests/map/test_b01_q10_map_parser.py` for `roborock/map/b01_q10_map_parser.py`). +- **Avoid One-Off Test Files**: Do not create fragmented, single-bug test files (such as `tests/test_battery_low_voltage_fix.py`). Integrate test cases into the module's corresponding test suite. +- **Fixture Reuse & Parametrization**: Reuse shared fixtures (`fake_channel`, `message_builder`, `device_fixture`) in `tests/` and use table-driven `@pytest.mark.parametrize`. +- **Behavior-Driven Assertions**: Assert on public trait methods, return values, and emitted channel commands—never on private object attributes (`_state`). + +### 6. PR Hygiene & Sizing +- **Scoping & Sizing**: Keep pull requests focused on a single logical change or subsystem where possible (e.g. parser module with tests in one PR, traits consuming it in a follow-up PR). Large PRs (>500–1,000 LOC) take longer to review and block releases. +- **CI Requirements**: PRs must pass automated CI (`.github/workflows/ci.yml`): + - **Commitlint**: Commit messages and PR titles MUST follow Conventional Commits (`feat:`, `fix:`, `refactor:`, `chore:`, `docs:`). + - **Pre-commit**: Ruff, Mypy (`check_untyped_defs = true`), and Codespell must pass (`uv run pre-commit run --all-files`). + - **Pytest**: All unit tests must pass (`uv run pytest`). +- **Logical Placement**: Place domain constants and enums in the module where they are consumed or in `roborock.data.code_mappings`. Do not duplicate definitions across files. + +--- + +## Available Agent Skills + +Portable agent skills following the [Agent Skills specification](https://agentskills.io/specification) are located under `.agents/skills/`: + +- **Code Review Skill** (`.agents/skills/review/SKILL.md`): + A structured, interactive review workflow for evaluating pull requests and diffs against the repository's architectural hierarchy, typing rules, and testing standards. To invoke during interactive sessions, refer to [.agents/skills/review/SKILL.md](.agents/skills/review/SKILL.md). diff --git a/CLAUDE.md b/CLAUDE.md new file mode 120000 index 00000000..47dc3e3d --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bfa8e8b2..8776efe1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,7 +34,9 @@ and you certify that you have the right to submit it under that license. ## Development Workflow -### Code Style +### Code Style & Architecture + +Before writing code or opening pull requests, please review our [Repository Engineering Guidelines](AGENTS.md) for our standards on typing (`RoborockBase` dataclasses), value-returning state helpers, protocol isolation, and test suite consolidation. We use several tools to enforce code quality and consistency. These are configured via `pre-commit` and generally run automatically.