docs: add repository engineering guidelines, review skill, and contributor standards (#953)

[allenporter]

Summary

Codifies code quality standards and engineering conventions to help agents write higher quality code and improve automated code review across Google Antigravity, OpenAI Codex, GitHub Copilot, and Anthropic Claude Code.

Key Additions

1. AGENTS.md:
   • Single source of truth for guidelines, standards, environment commands, and architectural review hierarchy:
     • Priority 1: Public Trait APIs & Consumer Contracts (clean, strongly typed with RoborockBase dataclasses, consumer-agnostic, decoupled from transport/crypto).
     • Priority 2: Data Lifecycle & State Management (pure value-returning helpers, explicit state assignments, simple linear flow, exhaustive teardown in close(), asyncio.Future RPC correlation).
     • Priority 3: Wire Protocol Parsers, Codecs & Cryptography (protocol isolation, resilient enums with RoborockEnum / RoborockModeEnum fallback).
   • Core engineering conventions: RoborockBase over TypedDict, wire-contained Any, compound capability gating (pv + RoborockCategory), test mirroring and module colocation (no one-off test files).
   • Practical PR scoping, sizing, and automated CI check requirements.
2. CLAUDE.md: Symlink pointing to AGENTS.md for Claude Code compatibility.
3. .agents/skills/review/SKILL.md: Portable review skill implementing the Agent Skills specification.
4. CONTRIBUTING.md: Points contributors to AGENTS.md under Code Style & Architecture.

Validation

• uv run pre-commit run --all-files passed cleanly (Ruff, Mypy with check_untyped_defs = true, Codespell, Commitlint).
• uv run pytest passed cleanly (983 passed).

Related issue #953

[Copilot]

🟡 Changes recommended

Address the unresolved documentation and review-guidance inaccuracies, including broken links and protocol-specific rules.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds repository-wide engineering standards, a portable review skill, and contributor guidance.

Changes:

• Adds AGENTS.md with architecture, tooling, testing, and workflow standards.
• Adds .agents/skills/review/SKILL.md.
• Updates CONTRIBUTING.md to reference the guidelines.

File summaries

File	Summary	Final review comments
CONTRIBUTING.md	Links contributors to AGENTS.md.	No comments.
AGENTS.md	Defines repository engineering standards.	Nit findings: missing CLAUDE.md symlink (3 votes); protocol-specific msg_id wording (1); overly broad VACUUM gating (1); invalid mirroring paths (1); unconditional exception rule needs a cleanup exception (1); nonexistent v1_simulator fixture (1).
.agents/skills/review/SKILL.md	Adds structured review guidance.	Nit findings: broken AGENTS.md link (3 votes); msg_id-only correlation rule should support protocol-specific IDs (1).

Review details

Suppressed comments (8)

.agents/skills/review/SKILL.md:32

• The skill repeats the same msg_id-only requirement, but V1 correlates responses with request_id (roborock/devices/rpc/v1_channel.py:123-143). Make this check protocol-neutral so the review skill does not flag the existing V1 pattern.

   - **Concurrency**: Does 1:1 async command-response matching use `asyncio.Future` mapped by `msg_id`?

AGENTS.md:22

• This setup creates and syncs .venv but never activates it, while the commands below invoke bare pytest, pre-commit, ruff, and mypy. Following this file alone can therefore run system tools or fail with command-not-found; add the activation step here (as CONTRIBUTING.md:25-28 does) or switch the commands to uv run.

# Environment setup
uv venv
uv sync

AGENTS.md:29

• Neither example test path exists in this checkout: the trait suite is under tests/devices/traits/v1/, and the protocol file is test_v1_protocol.py. As written, copying either command fails before running any tests; please point these examples at the canonical files.

pytest tests/devices/traits/test_battery.py
pytest tests/protocols/test_v1.py

AGENTS.md:52

• The overview hard-codes msg_id, but the V1 RPC implementation correlates responses with request_id (roborock/devices/rpc/v1_channel.py:123-143), while B01 Q7 uses msg_id. This wording would cause reviewers to reject a valid protocol-specific implementation; describe the correlation as using each protocol's request/sequence ID.

   - **Concurrency**: Request/response matching across async push channels uses `asyncio.Future` mapped by message ID (`msg_id`).

AGENTS.md:86

• This makes RoborockCategory.VACUUM the universal category gate, but A01 traits are intentionally created for WET_DRY_VAC and WASHING_MACHINE (roborock/devices/traits/a01/__init__.py:425-429). Following this rule would reject supported non-robot devices; scope VACUUM to V1 vacuum traits and require the protocol-appropriate category for each family.

- **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.

AGENTS.md:100

• The mirroring examples repeat the same nonexistent battery.py path, and roborock/map/q10.py is not the parser module here. This makes the canonical placement guidance point contributors at files they cannot run or extend; use the repository's versioned trait and B01 Q10 parser paths.

- **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.

AGENTS.md:95

• Specific Exception Narrowing is stated as an unconditional ban on broad catches, but roborock/devices/device.py:216-223 deliberately catches Exception to unsubscribe a leaked channel before re-raising. Enforcing this rule literally would reintroduce the resource leak the existing code guards against; document the cleanup exception.

- **Specific Exception Narrowing**: Catch only specific, expected exception classes (`RoborockTimeout`, `RoborockConnectionException`, `json.JSONDecodeError`).

AGENTS.md:102

• v1_simulator is not a pytest fixture in this repository; it is a simulator module/class used directly by tests (for example, tests/testing/test_v1_simulator.py). There is no fixture with that name to reuse, so this list gives contributors a nonexistent fixture; remove it or replace it with an actual fixture name.

- **Standard Fixture Reuse**: Reuse existing shared fixtures (`fake_channel`, `message_builder`, `device_fixture`, `v1_simulator`) in `tests/`.

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Lash-L]

Very nice - could likely be trimmed in some areas but I don't think it is meaningful enough to matter. Good addition all around

[allenporter]

Very nice - could likely be trimmed in some areas but I don't think it is meaningful enough to matter. Good addition all around

I addressed the feedback, as well as took a pass to slim some areas (cut agents file by 40%)