From fd5bf311df63129e043b19637008235ae72f1c86 Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Sat, 12 Sep 2026 17:26:44 +0000 Subject: [PATCH 1/4] docs: add repository engineering guidelines, review skill, and contributor standards (#953) --- .agents/skills/review/SKILL.md | 52 ++++++++++++++ AGENTS.md | 124 +++++++++++++++++++++++++++++++++ CLAUDE.md | 1 + CONTRIBUTING.md | 4 +- 4 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 .agents/skills/review/SKILL.md create mode 100644 AGENTS.md create mode 120000 CLAUDE.md diff --git a/.agents/skills/review/SKILL.md b/.agents/skills/review/SKILL.md new file mode 100644 index 000000000..a6d9b4069 --- /dev/null +++ b/.agents/skills/review/SKILL.md @@ -0,0 +1,52 @@ +--- +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 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`? + - **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`)? + +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)? + - **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/AGENTS.md b/AGENTS.md new file mode 100644 index 000000000..5028835c1 --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,124 @@ +# 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 development tools and coding agents (**OpenAI Codex**, **Google Antigravity**, **Anthropic Claude Code**, and **GitHub Copilot**). + +--- + +## Environment & Tooling Standards +- **Python Target**: Python 3.11+ +- **Build Backend**: Hatchling +- **Package & Dependency Manager**: `uv` +- **Linting & Formatting**: Ruff (line length 120) +- **Type Checking**: Mypy (`check_untyped_defs = true`) +- **Testing**: pytest (`pytest-asyncio`) + +### Key Developer Commands +```bash +# Environment setup +uv venv +uv sync + +# Run tests +pytest + +# Run tests for a specific trait or protocol +pytest tests/devices/traits/test_battery.py +pytest tests/protocols/test_v1.py + +# Lint and typecheck +pre-commit run --all-files +# or directly: +ruff check roborock tests +mypy roborock tests +``` + +--- + +## Core Architectural Hierarchy & Review Priorities +Review every proposed change against the repository's three core architectural layers, in strict order of priority: + +1. **Priority 1: Public Trait APIs & Consumer Contracts** (Highest Priority) + - Clean, stable user-facing abstractions suitable for downstream consumers (e.g., Home Assistant, CLI, or standalone scripts) without coupling the library's design to any single framework. + - Strictly 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** + - **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`). + - **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** + - Isolate 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`**: All serializable cloud API payloads, JSON-RPC models, and Tuya DPS data containers MUST subclass `roborock.data.containers.RoborockBase` and be decorated with `@dataclass`. Rely on `RoborockBase` built-in camelCase <-> snake_case translation (`from_dict`, `as_dict`). + - *Exemptions*: Internal binary protocol map packets (`Q10MapPacket`), transport message envelopes (`RoborockMessage`), map layers (`GridLayers`), transport parameters (`MqttParams`, `UserParams`), and push payloads (`CleanRecordPush`). +- **Prefer `RoborockBase` Over `TypedDict`**: Do not use `TypedDict` or loose dictionaries for structured domain data; always define a `@dataclass` subclassing `RoborockBase`. +- **Enum Fallback Resilience**: All enum types representing device status, firmware modes, error codes, and wire protocol integer codes where unknown values arrive from firmware MUST inherit from `RoborockEnum` or `RoborockModeEnum`. + - For `RoborockEnum` (an `IntEnum` subclass), define a lowercase fallback member (e.g. `unknown = -1` or `unknown = 0`); `RoborockEnum._missing_` automatically returns `cls.unknown` with a deduplicated warning without raising `ValueError`. + - For `RoborockModeEnum` (pairing string names with integer codes), use `from_code_optional(code) -> Self | None` for optional fallback. + - Standard internal repository enums (e.g. `RoborockCommand`, `DeviceVersion`, `RoborockCategory`) that do not decode unknown codes from firmware remain standard `Enum`, `IntEnum`, or `StrEnum`. +- **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). +- **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). +- **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. +- **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()`. +- **Trait Boundary Simplicity**: Design traits around cohesive, general-purpose consumer concepts (e.g., `CleanHistoryTrait`, `DockTrait`, `ConsumableTrait`), suitable for downstream consumers like Home Assistant or CLI tools, not low-level firmware registers or raw Tuya DPS numbers. +- **Diagnostic Safety & Privacy**: Exclude sensitive user information (such as Wi-Fi SSIDs, passwords, cloud tokens, or encryption keys) from device diagnostics and logs to prevent privacy leaks in downstream clients. + +### 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`). +- **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. +- **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/`. +- **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. + +### 6. PR Hygiene & Sizing +- **Scoping & Splitting Large Pull Requests**: Large pull requests (>500–1,000 LOC or mixing protocol parsers, device traits, and consumer fixes) take much longer to review and block releases. + - Keep PRs focused on a single logical change or subsystem where possible (e.g. implementing a parser module with its tests in one PR, then introducing or updating traits that consume it in a follow-up PR). + - When reviewing large PRs, maintainers prioritize how consumer traits and public APIs are shaped before digging into parser internals. Keep public APIs and state management clean and readable even if parser internals are complex. +- **CI Requirements**: PRs must pass automated GitHub Actions CI (`.github/workflows/ci.yml`): + - **Commitlint**: Commit messages and PR titles MUST follow Conventional Commits (`feat:`, `fix:`, `refactor:`, `chore:`, `docs:`) to support automated releases. + - **Pre-commit**: Ruff, Mypy (`check_untyped_defs = true`), and Codespell must pass (`pre-commit run --all-files`). + - **Pytest**: All unit tests must pass (`pytest`). +- **Logical Module Placement**: Place constants, enums, and utility functions in the module where they are consumed or in `roborock.data.code_mappings`. Do not scatter domain constants across unrelated modules. + +--- + +## 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 000000000..47dc3e3d8 --- /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 bfa8e8b28..8776efe13 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. From 9e1be67a2d292494f84d6a6f3a0c7d6889509416 Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Sat, 12 Sep 2026 17:28:10 +0000 Subject: [PATCH 2/4] docs: add .claude/skills symlink for Claude Code discovery --- .claude/skills | 1 + AGENTS.md | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 120000 .claude/skills diff --git a/.claude/skills b/.claude/skills new file mode 120000 index 000000000..2b7a412b8 --- /dev/null +++ b/.claude/skills @@ -0,0 +1 @@ +../.agents/skills \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 5028835c1..4ecfed016 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -120,5 +120,5 @@ Review every proposed change against the repository's three core architectural l 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`): +- **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). From 63957a4a849969a3df732a7976843826758c1553 Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Sat, 12 Sep 2026 19:34:37 +0000 Subject: [PATCH 3/4] docs: address review feedback on feature gating, forward refs, and test paths --- .agents/skills/review/SKILL.md | 10 ++++++---- AGENTS.md | 26 +++++++++++++++----------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/.agents/skills/review/SKILL.md b/.agents/skills/review/SKILL.md index a6d9b4069..6eaec6c7b 100644 --- a/.agents/skills/review/SKILL.md +++ b/.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,15 +23,17 @@ 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()`)? @@ -39,7 +41,7 @@ All reviews are evaluated against the engineering conventions defined in [AGENTS - 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? diff --git a/AGENTS.md b/AGENTS.md index 4ecfed016..7f088c4fe 100644 --- a/AGENTS.md +++ b/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. From b7299ed547f91ffce5beafbb7ad6962d119efb59 Mon Sep 17 00:00:00 2001 From: Allen Porter Date: Sat, 12 Sep 2026 19:37:27 +0000 Subject: [PATCH 4/4] docs: streamline AGENTS.md by removing redundancies --- AGENTS.md | 107 ++++++++++++++++++++---------------------------------- 1 file changed, 40 insertions(+), 67 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7f088c4fe..d81bd0a06 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -3,28 +3,22 @@ ## 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 development tools and coding agents (**OpenAI Codex**, **Google Antigravity**, **Anthropic Claude Code**, and **GitHub Copilot**). +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 -- **Python Target**: Python 3.11+ -- **Build Backend**: Hatchling -- **Package & Dependency Manager**: `uv` -- **Linting & Formatting**: Ruff (line length 120) -- **Type Checking**: Mypy (`check_untyped_defs = true`) -- **Testing**: pytest (`pytest-asyncio`) +- **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 +uv venv && uv sync # Run tests uv run pytest - -# Run tests for a specific trait or protocol uv run pytest tests/devices/traits/v1/test_status.py uv run pytest tests/protocols/test_v1_protocol.py @@ -38,22 +32,20 @@ uv run mypy roborock tests --- ## Core Architectural Hierarchy & Review Priorities -Review every proposed change against the repository's three core architectural layers, in strict order of priority: +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 suitable for downstream consumers (e.g., Home Assistant, CLI, or standalone scripts) without coupling the library's design to any single framework. - - Strictly decoupled from wire protocol details, Tuya DPS keys, encryption keys, or transport sockets. + - 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** - - **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` 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. + - 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** - - Isolate cryptography (AES, MD5), protocol framing, and raw sockets to `roborock.protocols` and `roborock.devices.transport` / `roborock.devices.rpc`. + - 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. @@ -62,61 +54,42 @@ Review every proposed change against the repository's three core architectural l ## Detailed Engineering Conventions ### 1. Typing & Data Models -- **Subclass `RoborockBase`**: All serializable cloud API payloads, JSON-RPC models, and Tuya DPS data containers MUST subclass `roborock.data.containers.RoborockBase` and be decorated with `@dataclass`. Rely on `RoborockBase` built-in camelCase <-> snake_case translation (`from_dict`, `as_dict`). - - *Exemptions*: Internal binary protocol map packets (`Q10MapPacket`), transport message envelopes (`RoborockMessage`), map layers (`GridLayers`), transport parameters (`MqttParams`, `UserParams`), and push payloads (`CleanRecordPush`). -- **Prefer `RoborockBase` Over `TypedDict`**: Do not use `TypedDict` or loose dictionaries for structured domain data; always define a `@dataclass` subclassing `RoborockBase`. -- **Enum Fallback Resilience**: All enum types representing device status, firmware modes, error codes, and wire protocol integer codes where unknown values arrive from firmware MUST inherit from `RoborockEnum` or `RoborockModeEnum`. - - For `RoborockEnum` (an `IntEnum` subclass), define a lowercase fallback member (e.g. `unknown = -1` or `unknown = 0`); `RoborockEnum._missing_` automatically returns `cls.unknown` with a deduplicated warning without raising `ValueError`. - - For `RoborockModeEnum` (pairing string names with integer codes), use `from_code_optional(code) -> Self | None` for optional fallback. - - Standard internal repository enums (e.g. `RoborockCommand`, `DeviceVersion`, `RoborockCategory`) that do not decode unknown codes from firmware remain standard `Enum`, `IntEnum`, or `StrEnum`. -- **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). +- **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 -- **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 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 & 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()`. -- **Trait Boundary Simplicity**: Design traits around cohesive, general-purpose consumer concepts (e.g., `CleanHistoryTrait`, `DockTrait`, `ConsumableTrait`), suitable for downstream consumers like Home Assistant or CLI tools, not low-level firmware registers or raw Tuya DPS numbers. -- **Diagnostic Safety & Privacy**: Exclude sensitive user information (such as Wi-Fi SSIDs, passwords, cloud tokens, or encryption keys) from device diagnostics and logs to prevent privacy leaks in downstream clients. +- **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 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. +- **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 & 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`) 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. +- **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 & Splitting Large Pull Requests**: Large pull requests (>500–1,000 LOC or mixing protocol parsers, device traits, and consumer fixes) take much longer to review and block releases. - - Keep PRs focused on a single logical change or subsystem where possible (e.g. implementing a parser module with its tests in one PR, then introducing or updating traits that consume it in a follow-up PR). - - When reviewing large PRs, maintainers prioritize how consumer traits and public APIs are shaped before digging into parser internals. Keep public APIs and state management clean and readable even if parser internals are complex. -- **CI Requirements**: PRs must pass automated GitHub Actions CI (`.github/workflows/ci.yml`): - - **Commitlint**: Commit messages and PR titles MUST follow Conventional Commits (`feat:`, `fix:`, `refactor:`, `chore:`, `docs:`) to support automated releases. - - **Pre-commit**: Ruff, Mypy (`check_untyped_defs = true`), and Codespell must pass (`pre-commit run --all-files`). - - **Pytest**: All unit tests must pass (`pytest`). -- **Logical Module Placement**: Place constants, enums, and utility functions in the module where they are consumed or in `roborock.data.code_mappings`. Do not scatter domain constants across unrelated modules. +- **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. ---