diff --git a/.agents/skills/review/SKILL.md b/.agents/skills/code-review/SKILL.md similarity index 64% rename from .agents/skills/review/SKILL.md rename to .agents/skills/code-review/SKILL.md index 6eaec6c7b..87094053d 100644 --- a/.agents/skills/review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -1,5 +1,5 @@ --- -name: review +name: code-review description: Conduct authoritative code review on python-roborock changes against repository architectural hierarchy, typing standards, and maintainer conventions. --- @@ -13,11 +13,13 @@ All reviews are evaluated against the engineering conventions defined in [AGENTS ## Review Workflow -1. **Classify Changed Files by Layer**: - Group changed files according to the 3-tier architectural hierarchy: +1. **Classify Changed Files & Scope**: + Group changed files according to the 3-tier architectural hierarchy and evaluate PR scoping: - **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 2: Data Lifecycle & State Management** (trait state, callbacks, `RoborockDevice.close()`, map/render pipelines) - **Priority 3: Wire Protocols, Codecs & Cryptography** (`roborock/protocols/`, `roborock/devices/transport/`, `roborock/devices/rpc/`) + - **PR Scoping**: Flag PRs that mix unrelated subsystems or combine large protocol/parser additions with trait consumption when they should be split into focused PRs. + - **Automated Conformance**: Structural invariants (`RoborockBase` inheritance, `RoborockEnum` fallback members, `RoborockException` rooting, and trait boundary types) are enforced in `tests/conformance/`. Flag any unjustified allowlist exemptions added to `tests/conformance/` and focus deeper review energy on semantic, lifecycle, and design issues. 2. **Evaluate Layer 1: Public Trait APIs & Consumer Contracts**: - Are trait models subclassing `RoborockBase` with `@dataclass`? @@ -28,9 +30,11 @@ All reviews are evaluated against the engineering conventions defined in [AGENTS - 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**: +3. **Evaluate Layer 2: Lifecycle, State & Concurrency**: - **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? + - **Render & Cache Immutability**: Are cached telemetry state and raw packets kept immutable during presentation/rendering? When caching derived state (e.g., parsed maps or calibration), is cache invalidation handled accurately across out-of-order or concurrent updates? + - **Event Loop Safety**: Are CPU-intensive operations (e.g., heavy map decompression or PIL image rendering) kept off the `asyncio` event loop (such as via `asyncio.to_thread`) with proper concurrency/staleness guards? - **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? @@ -47,8 +51,6 @@ All reviews are evaluated against the engineering conventions defined in [AGENTS - 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** + Keep feedback high-signal: focus on architectural violations, state/concurrency bugs, protocol safety, and test gaps; avoid nitpicking formatting or style already enforced by Ruff and Mypy. + - **Interactive / Summary Reviews**: Group findings by Priority Layer (1 → 3) with file references, the anti-pattern identified, grounded rationale (citing protocol resilience, consumer stability, or maintainability), and a concrete before/after diff or suggested fix. + - **Inline PR Review Comments**: State the issue, grounded rationale (citing `AGENTS.md` conventions), and suggested fix concisely right on the affected lines. diff --git a/.github/skills b/.github/skills new file mode 120000 index 000000000..2b7a412b8 --- /dev/null +++ b/.github/skills @@ -0,0 +1 @@ +../.agents/skills \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 474bdfe90..d3cea4d81 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -97,5 +97,5 @@ Evaluate every change against the repository's three core layers in strict prior 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). +- **Code Review Skill** (`.agents/skills/code-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/code-review/SKILL.md](.agents/skills/code-review/SKILL.md).