diff --git a/CHANGELOG.md b/CHANGELOG.md index d165dbb..d2fb6a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] ### Added +- **DeviceCommand — one seam for server→firmware MCP tool calls** (`custom-providers/xiaozhi-patches/device_command.py`, mounted at `core/utils/device_command.py`) — twelve call sites across the `/xiaozhi/admin/*` handlers and `receiveAudioHandle.py` each hand-rolled the same JSON-RPC envelope, so every shared defect was twelve defects (2026-06-06 audit): request ids were `int(time.time()*1000) % 0x7FFFFFFF` (same-millisecond calls collided, with zero reply correlation to notice), and every site fired `conn.websocket.send()` uncoordinated against the other senders on the same connection. The seam owns **monotonic per-connection request ids**, **the MCP envelope**, and **per-connection serialized sends** (an asyncio.Lock — play-asset's opus frames now route through it too, so a concurrent head-turn can't interleave mid-frame). Admin handlers also share one `_dotty_resolve_conn()` instead of eight copies of the device-lookup block (which fixes the audit's `inject-text` missing-`or {}` headers nit as a side effect). Reply correlation is deliberately still absent — `call_tool` returns the request id so it can be added behind this interface without touching the callers again. **Bench: needs a live-device smoke (LED pips, head-turn, take-photo, play-asset) before release sign-off.** - **Bridge systemd unit loads API keys from `${BRIDGE_DIR}/.env`** (#15) — `zeroclaw-bridge.service.template` and `scripts/install-bridge.sh` now emit `EnvironmentFile=-${BRIDGE_DIR}/.env`. `install-bridge.sh` creates a mode-0600 stub `.env` containing `OPENROUTER_API_KEY=` (and commented `VISION_API_KEY` / `VLM_API_KEY` placeholders) when one isn't already present, so the missing-vision-key failure surfaces as the bridge's existing ERROR ("camera offline") instead of a silent confabulation. Existing `.env` files are preserved. ### Changed diff --git a/CLAUDE.md b/CLAUDE.md index eae237d..78f2b1e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -199,3 +199,17 @@ For hardware specs, protocol details, model internals, latent capabilities, and - xiaozhi-esp32 firmware (upstream): https://github.com/78/xiaozhi-esp32 - StackChan (hardware + firmware patches): https://github.com/m5stack/StackChan - Emotion protocol: https://xiaozhi.dev/en/docs/development/emotion/ + +## Agent skills + +### Issue tracker + +Issues live as GitHub issues on `BrettKinny/dotty-stackchan` (the `origin` remote), managed via the `gh` CLI. See `docs/agents/issue-tracker.md`. + +### Triage labels + +Five canonical triage roles use their default label strings (`needs-triage`, `needs-info`, `ready-for-agent`, `ready-for-human`, `wontfix`), orthogonal to the existing `status:*` / `area:*` labels. See `docs/agents/triage-labels.md`. + +### Domain docs + +Single-context: one `CONTEXT.md` + `docs/adr/` at the repo root. See `docs/agents/domain.md`. diff --git a/compose.all-in-one.yml b/compose.all-in-one.yml index 0631953..624e089 100644 --- a/compose.all-in-one.yml +++ b/compose.all-in-one.yml @@ -102,6 +102,9 @@ services: - ./receiveAudioHandle.py:/opt/xiaozhi-esp32-server/core/handle/receiveAudioHandle.py:ro - ./dances.py:/opt/xiaozhi-esp32-server/core/handle/dances.py:ro - ./custom-providers/textUtils.py:/opt/xiaozhi-esp32-server/core/utils/textUtils.py:ro + # DOTTY DeviceCommand seam: MCP request ids + envelope + per-conn + # serialized sends, shared by http_server.py + receiveAudioHandle.py + - ./custom-providers/xiaozhi-patches/device_command.py:/opt/xiaozhi-esp32-server/core/utils/device_command.py:ro - ./songs:/opt/xiaozhi-esp32-server/config/assets/songs:ro # DOTTY portal + admin routes + active-connection registry - ./custom-providers/xiaozhi-patches/portal_bridge.py:/opt/xiaozhi-esp32-server/core/portal_bridge.py:ro diff --git a/custom-providers/xiaozhi-patches/device_command.py b/custom-providers/xiaozhi-patches/device_command.py new file mode 100644 index 0000000..8d5e21a --- /dev/null +++ b/custom-providers/xiaozhi-patches/device_command.py @@ -0,0 +1,94 @@ +"""DeviceCommand — the single seam for server→firmware MCP tool calls. + +Mounted into the xiaozhi container at `core/utils/device_command.py` +(importable as `core.utils.device_command`) so both patch surfaces that +talk MCP to the device — the `/xiaozhi/admin/*` handlers in +`http_server.py` and the chat-pipeline helpers in +`receiveAudioHandle.py` — build and send tool calls through one module. + +Before this seam, twelve call sites each hand-rolled the same JSON-RPC +envelope, and every shared defect was twelve defects (2026-06-06 +audit): request ids were `int(time.time()*1000) % 0x7FFFFFFF`, so two +calls in the same millisecond collided; and every site fired +`conn.websocket.send()` with no coordination, racing the other senders +on the same ServerConnection (the websockets library does not allow +interleaved sends). + +What this module owns: + + * **Monotonic per-connection request ids** — a plain counter stored + on the conn, so ids are unique for the life of the connection and + a future reply-correlation layer has something to correlate on. + * **The MCP envelope** — one place to change the wire shape. + * **Serialized device-bound sends** — a per-connection asyncio.Lock; + every send routed through here is mutually exclusive with every + other send routed through here. (Upstream xiaozhi's own chat-path + writer does not take this lock — full serialization would mean + patching upstream send sites; this seam is where that lands when + it does.) + +Reply correlation is deliberately NOT implemented yet: device-side MCP +replies are still fire-and-forget. `call_tool` returns the request id +so a correlation layer can be added behind this interface without +touching the twelve callers again. + +State is attached to the conn object (`_dotty_mcp_next_id`, +`_dotty_send_lock`) rather than a side table so it lives and dies with +the connection — no leak across reconnects, no weakref bookkeeping. +All attachment happens on the event-loop thread with no awaits between +check and set, so initialisation cannot race. +""" + +import asyncio +import json + +_ID_ATTR = "_dotty_mcp_next_id" +_LOCK_ATTR = "_dotty_send_lock" + + +def next_request_id(conn) -> int: + """Monotonic JSON-RPC id, unique per connection lifetime.""" + current = getattr(conn, _ID_ATTR, 1) + setattr(conn, _ID_ATTR, current + 1) + return current + + +def mcp_envelope(conn, tool: str, arguments: dict, request_id: int) -> str: + """Serialize one MCP tools/call frame for `conn`.""" + return json.dumps({ + "session_id": getattr(conn, "session_id", ""), + "type": "mcp", + "payload": { + "jsonrpc": "2.0", + "method": "tools/call", + "params": {"name": tool, "arguments": arguments}, + "id": request_id, + }, + }) + + +def _send_lock(conn) -> asyncio.Lock: + lock = getattr(conn, _LOCK_ATTR, None) + if lock is None: + lock = asyncio.Lock() + setattr(conn, _LOCK_ATTR, lock) + return lock + + +async def send_serialized(conn, message) -> None: + """Send one frame (str or bytes) on the device WebSocket, mutually + exclusive with every other send routed through this module.""" + async with _send_lock(conn): + await conn.websocket.send(message) + + +async def call_tool(conn, tool: str, arguments: dict) -> int: + """Build + send one MCP tools/call. Returns the request id. + + Fire-and-forget at the protocol level (no reply wait — see module + docstring), but the send itself is serialized against other + device-bound sends from this module. + """ + request_id = next_request_id(conn) + await send_serialized(conn, mcp_envelope(conn, tool, arguments, request_id)) + return request_id diff --git a/custom-providers/xiaozhi-patches/http_server.py b/custom-providers/xiaozhi-patches/http_server.py index c09997a..0290f20 100644 --- a/custom-providers/xiaozhi-patches/http_server.py +++ b/custom-providers/xiaozhi-patches/http_server.py @@ -7,6 +7,10 @@ # and consumed by the /xiaozhi/admin/inject-text route below. Lets the # Dotty admin dashboard fire `startToChat` against an active device WS. from core.portal_bridge import active_connections as _dotty_active_connections +# DOTTY-PATCH: DeviceCommand seam — monotonic MCP request ids + the +# envelope + per-conn serialized sends live in one module shared with +# receiveAudioHandle.py (mounted at core/utils/device_command.py). +from core.utils import device_command as _dotty_device_command TAG = __name__ @@ -88,6 +92,27 @@ async def _dotty_admin_auth_middleware(request, handler): return await handler(request) +# DOTTY-PATCH: shared conn resolution for every admin route. Named device +# first, else the first available connection; (None, 503-response) when no +# device is connected. Was copy-pasted into all eight handlers. +def _dotty_resolve_conn(device_id: str): + if device_id: + conn = _dotty_active_connections.get(device_id) + else: + conn = next(iter(_dotty_active_connections.values()), None) + if conn is None: + return None, web.json_response( + {"error": "no device connected", + "known": list(_dotty_active_connections)}, + status=503, + ) + return conn, None + + +def _dotty_conn_device_id(conn, fallback: str = "") -> str: + return (getattr(conn, "headers", {}) or {}).get("device-id", "") or fallback + + class SimpleHttpServer: def __init__(self, config: dict): self.config = config @@ -114,21 +139,15 @@ async def _dotty_inject_text(self, request: "web.Request") -> "web.Response": device_id = data.get("device_id", "") or "" if not text: return web.json_response({"error": "text required"}, status=400) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", "known": list(_dotty_active_connections)}, - status=503, - ) + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err # Lazy import to avoid pulling the chat pipeline at server startup. from core.handle.receiveAudioHandle import startToChat _spawn(startToChat(conn, text), name="inject_text_chat") return web.json_response({ "ok": True, - "device_id": getattr(conn, "headers", {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), }) async def _dotty_list_devices(self, request: "web.Request") -> "web.Response": @@ -146,21 +165,14 @@ async def _dotty_abort(self, request: "web.Request") -> "web.Response": except Exception: data = {} device_id = (data.get("device_id") or "").strip() if isinstance(data, dict) else "" - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err from core.handle.abortHandle import handleAbortMessage _spawn(handleAbortMessage(conn), name="inject_abort") return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), }) async def _dotty_set_head_angles(self, request: "web.Request") -> "web.Response": @@ -183,35 +195,19 @@ async def _dotty_set_head_angles(self, request: "web.Request") -> "web.Response" speed = int(data.get("speed", 250)) except (TypeError, ValueError): return web.json_response({"error": "yaw/pitch/speed must be ints"}, status=400) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - import json - import time - msg = json.dumps({ - "session_id": getattr(conn, "session_id", ""), - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_head_angles", - "arguments": {"yaw": yaw, "pitch": pitch, "speed": speed}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - _spawn(conn.websocket.send(msg), name="set_head_angles_send") + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + _spawn( + _dotty_device_command.call_tool( + conn, "self.robot.set_head_angles", + {"yaw": yaw, "pitch": pitch, "speed": speed}, + ), + name="set_head_angles_send", + ) return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), "yaw": yaw, "pitch": pitch, "speed": speed, }) @@ -231,35 +227,18 @@ async def _dotty_set_state(self, request: "web.Request") -> "web.Response": state = (data.get("state") or "").strip() if state not in ("idle", "talk", "story_time", "security", "sleep", "dance"): return web.json_response({"error": f"unknown state: {state!r}"}, status=400) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - import json - import time - msg = json.dumps({ - "session_id": getattr(conn, "session_id", ""), - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_state", - "arguments": {"state": state}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - _spawn(conn.websocket.send(msg), name="set_state_send") + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + _spawn( + _dotty_device_command.call_tool( + conn, "self.robot.set_state", {"state": state}, + ), + name="set_state_send", + ) return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), "state": state, }) @@ -279,35 +258,18 @@ async def _dotty_set_toggle(self, request: "web.Request") -> "web.Response": if name not in ("kid_mode", "smart_mode"): return web.json_response({"error": f"unknown toggle: {name!r}"}, status=400) enabled = bool(data.get("enabled")) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - import json - import time - msg = json.dumps({ - "session_id": getattr(conn, "session_id", ""), - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_toggle", - "arguments": {"name": name, "enabled": enabled}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - _spawn(conn.websocket.send(msg), name="set_toggle_send") + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + _spawn( + _dotty_device_command.call_tool( + conn, "self.robot.set_toggle", {"name": name, "enabled": enabled}, + ), + name="set_toggle_send", + ) return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), "name": name, "enabled": enabled, }) @@ -325,35 +287,18 @@ async def _dotty_set_face_identified(self, request: "web.Request") -> "web.Respo except Exception: data = {} device_id = (data.get("device_id") or "").strip() - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - import json - import time - msg = json.dumps({ - "session_id": getattr(conn, "session_id", ""), - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_face_identified", - "arguments": {}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - _spawn(conn.websocket.send(msg), name="set_face_identified_send") + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + _spawn( + _dotty_device_command.call_tool( + conn, "self.robot.set_face_identified", {}, + ), + name="set_face_identified_send", + ) return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), }) async def _dotty_take_photo(self, request: "web.Request") -> "web.Response": @@ -375,35 +320,18 @@ async def _dotty_take_photo(self, request: "web.Request") -> "web.Response": question = (data.get("question") or "").strip() if not question: return web.json_response({"error": "question required"}, status=400) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - import json - import time - msg = json.dumps({ - "session_id": getattr(conn, "session_id", ""), - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.camera.take_photo", - "arguments": {"question": question}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - _spawn(conn.websocket.send(msg), name="take_photo_send") + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + _spawn( + _dotty_device_command.call_tool( + conn, "self.camera.take_photo", {"question": question}, + ), + name="take_photo_send", + ) return web.json_response({ "ok": True, - "device_id": (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id, + "device_id": _dotty_conn_device_id(conn, device_id), "question": question, }) @@ -455,17 +383,10 @@ async def _dotty_play_asset(self, request: "web.Request") -> "web.Response": return web.json_response( {"error": f"asset not found: {asset_path}"}, status=404 ) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) - resolved_id = (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err + resolved_id = _dotty_conn_device_id(conn, device_id) # DOTTY-PATCH: don't clobber a live chat turn. play-asset is timer-driven # on the first available device, so a purr/song landing mid-conversation @@ -539,8 +460,12 @@ def _collect(b: bytes) -> None: conn.client_abort = False conn.client_is_speaking = True sent = 0 + # DOTTY-PATCH: frame sends routed through the DeviceCommand + # per-conn send lock so a concurrent admin MCP call (e.g. a + # sound-turner head-turn) can't interleave mid-frame. + _send = _dotty_device_command.send_serialized try: - await conn.websocket.send(_json.dumps({ + await _send(conn, _json.dumps({ "type": "tts", "state": "sentence_start", "text": "", @@ -552,7 +477,7 @@ def _collect(b: bytes) -> None: f"play-asset aborted after {sent}/{len(pkts)} packets" ) break - await conn.websocket.send(pkt) + await _send(conn, pkt) sent += 1 await asyncio.sleep(0.06) except Exception as exc: @@ -560,7 +485,7 @@ def _collect(b: bytes) -> None: finally: conn.client_is_speaking = False try: - await conn.websocket.send(_json.dumps({ + await _send(conn, _json.dumps({ "type": "tts", "state": "stop", "session_id": conn.session_id, @@ -604,21 +529,14 @@ async def _dotty_say(self, request: "web.Request") -> "web.Response": device_id = (data.get("device_id") or "").strip() if not text: return web.json_response({"error": "text required"}, status=400) - if device_id: - conn = _dotty_active_connections.get(device_id) - else: - conn = next(iter(_dotty_active_connections.values()), None) - if conn is None: - return web.json_response( - {"error": "no device connected", - "known": list(_dotty_active_connections)}, - status=503, - ) + conn, err = _dotty_resolve_conn(device_id) + if err is not None: + return err if not getattr(conn, "tts", None): return web.json_response( {"error": "device has no tts provider"}, status=503, ) - resolved_id = (getattr(conn, "headers", {}) or {}).get("device-id", "") or device_id + resolved_id = _dotty_conn_device_id(conn, device_id) # Lazy imports — keep module load cheap and the dependency on # xiaozhi-server's TTS DTO module local to this handler. diff --git a/docker-compose.yml.template b/docker-compose.yml.template index df5380a..d176b74 100644 --- a/docker-compose.yml.template +++ b/docker-compose.yml.template @@ -68,6 +68,9 @@ services: - ./receiveAudioHandle.py:/opt/xiaozhi-esp32-server/core/handle/receiveAudioHandle.py:ro - ./dances.py:/opt/xiaozhi-esp32-server/core/handle/dances.py:ro - ./custom-providers/textUtils.py:/opt/xiaozhi-esp32-server/core/utils/textUtils.py:ro + # DOTTY DeviceCommand seam: MCP request ids + envelope + per-conn + # serialized sends, shared by http_server.py + receiveAudioHandle.py + - ./custom-providers/xiaozhi-patches/device_command.py:/opt/xiaozhi-esp32-server/core/utils/device_command.py:ro - ./songs:/opt/xiaozhi-esp32-server/config/assets/songs:ro # DOTTY portal: admin /inject-text route + active-connection registry - ./custom-providers/xiaozhi-patches/portal_bridge.py:/opt/xiaozhi-esp32-server/core/portal_bridge.py:ro diff --git a/docs/agents/domain.md b/docs/agents/domain.md new file mode 100644 index 0000000..a4f449f --- /dev/null +++ b/docs/agents/domain.md @@ -0,0 +1,38 @@ +# Domain Docs + +How the engineering skills should consume this repo's domain documentation when exploring the codebase. + +This is a **single-context** repo: one `CONTEXT.md` + `docs/adr/` at the repo root cover the whole Dotty stack. (Note: `CLAUDE.md` at the root is the always-on architecture reference and the authoritative starting point regardless — `CONTEXT.md` is the lazily-grown domain glossary the producer skills maintain.) + +## Before exploring, read these + +- **`CONTEXT.md`** at the repo root. +- **`docs/adr/`** — read ADRs that touch the area you're about to work in. + +If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The producer skill (`/grill-with-docs`) creates them lazily when terms or decisions actually get resolved. + +## File structure + +Single-context repo: + +``` +/ +├── CLAUDE.md ← always-on architecture reference (already exists) +├── CONTEXT.md ← domain glossary (created lazily by /grill-with-docs) +├── docs/adr/ ← architectural decision records (created lazily) +│ ├── 0001-....md +│ └── 0002-....md +└── ... +``` + +## Use the glossary's vocabulary + +When your output names a domain concept (in an issue title, a refactor proposal, a hypothesis, a test name), use the term as defined in `CONTEXT.md`. Don't drift to synonyms the glossary explicitly avoids. + +If the concept you need isn't in the glossary yet, that's a signal — either you're inventing language the project doesn't use (reconsider) or there's a real gap (note it for `/grill-with-docs`). + +## Flag ADR conflicts + +If your output contradicts an existing ADR, surface it explicitly rather than silently overriding: + +> _Contradicts ADR-0007 (...) — but worth reopening because…_ diff --git a/docs/agents/issue-tracker.md b/docs/agents/issue-tracker.md new file mode 100644 index 0000000..afd0d0f --- /dev/null +++ b/docs/agents/issue-tracker.md @@ -0,0 +1,32 @@ +# Issue tracker: GitHub + +Issues and PRDs for this repo live as GitHub issues on `BrettKinny/dotty-stackchan`. Use the `gh` CLI for all operations. + +## Conventions + +- **Create an issue**: `gh issue create --title "..." --body "..."`. Use a heredoc for multi-line bodies. +- **Read an issue**: `gh issue view --comments`, filtering comments by `jq` and also fetching labels. +- **List issues**: `gh issue list --state open --json number,title,body,labels,comments --jq '[.[] | {number, title, body, labels: [.labels[].name], comments: [.comments[].body]}]'` with appropriate `--label` and `--state` filters. +- **Comment on an issue**: `gh issue comment --body "..."` +- **Apply / remove labels**: `gh issue edit --add-label "..."` / `--remove-label "..."` +- **Close**: `gh issue close --comment "..."` + +Infer the repo from `git remote -v` — `gh` does this automatically when run inside a clone. Note this clone has two remotes: `origin` → `BrettKinny/dotty-stackchan` (the canonical repo) and `fork` → `pboushy/dotty-stackchan`. Target `origin` unless told otherwise (e.g. `gh issue list --repo BrettKinny/dotty-stackchan`). + +## Existing label conventions + +This repo already uses workflow labels that the triage labels complement, not replace: + +- `status:active`, `status:bench-pending`, `status:blocked`, `status:speculative` — lifecycle state (see the `dotty-task-runner` skill). +- `area:firmware`, `area:bridge`, `area:xiaozhi`, `area:dashboard`, `area:infra`, `area:docs`, `area:behaviour` — which part of the stack. +- `safety` — child-safety / correctness bug. + +Keep applying these alongside the triage roles in `docs/agents/triage-labels.md`. + +## When a skill says "publish to the issue tracker" + +Create a GitHub issue. + +## When a skill says "fetch the relevant ticket" + +Run `gh issue view --comments`. diff --git a/docs/agents/triage-labels.md b/docs/agents/triage-labels.md new file mode 100644 index 0000000..f22fe45 --- /dev/null +++ b/docs/agents/triage-labels.md @@ -0,0 +1,19 @@ +# Triage Labels + +The skills speak in terms of five canonical triage roles. This file maps those roles to the actual label strings used in this repo's issue tracker. + +| Label in mattpocock/skills | Label in our tracker | Meaning | +| -------------------------- | -------------------- | ---------------------------------------- | +| `needs-triage` | `needs-triage` | Maintainer needs to evaluate this issue | +| `needs-info` | `needs-info` | Waiting on reporter for more information | +| `ready-for-agent` | `ready-for-agent` | Fully specified, ready for an AFK agent | +| `ready-for-human` | `ready-for-human` | Requires human implementation | +| `wontfix` | `wontfix` | Will not be actioned | + +`wontfix` already exists in this repo. The other four are created on first use (`gh label create ` or auto-created when applied via the API). + +These triage roles are **orthogonal** to the repo's existing `status:*` lifecycle labels (`status:active`, `status:bench-pending`, `status:blocked`, `status:speculative`) and `area:*` labels — apply both as appropriate rather than treating them as alternatives. + +When a skill mentions a role (e.g. "apply the AFK-ready triage label"), use the corresponding label string from this table. + +Edit the right-hand column to match whatever vocabulary you actually use. diff --git a/receiveAudioHandle.py b/receiveAudioHandle.py index 94d11c9..e24cf4c 100644 --- a/receiveAudioHandle.py +++ b/receiveAudioHandle.py @@ -13,6 +13,10 @@ from core.handle.intentHandler import handle_user_intent from core.utils.output_counter import check_device_output_limit from core.handle.sendAudioHandle import send_stt_message, SentenceType +# DOTTY-PATCH: DeviceCommand seam — monotonic MCP request ids + the envelope +# + per-conn serialized sends, shared with the admin routes in http_server.py +# (mounted at core/utils/device_command.py). +from core.utils.device_command import call_tool as _mcp_call_tool TAG = __name__ @@ -268,20 +272,9 @@ def _is_help_request(text: str) -> bool: async def _send_led_color(conn: "ConnectionHandler", r: int, g: int, b: int) -> None: try: - msg = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_led_color", - "arguments": {"r": r, "g": g, "b": b}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(msg) + await _mcp_call_tool( + conn, "self.robot.set_led_color", {"r": r, "g": g, "b": b}, + ) except Exception: pass # Phase 4 — kid_mode + smart_mode pips are firmware-owned (StateManager @@ -306,20 +299,10 @@ async def _send_led_multi( so we don't noisily spam. """ try: - msg = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_led_multi", - "arguments": {"index": index, "r": r, "g": g, "b": b}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(msg) + await _mcp_call_tool( + conn, "self.robot.set_led_multi", + {"index": index, "r": r, "g": g, "b": b}, + ) except Exception as exc: # The firmware may simply not support set_led_multi yet (old # build); log warn-once per connection so we know without @@ -336,20 +319,10 @@ async def _send_led_multi( async def _send_head_angles(conn: "ConnectionHandler", yaw: int, pitch: int, speed: int = 150) -> None: try: - msg = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_head_angles", - "arguments": {"yaw": yaw, "pitch": pitch, "speed": speed}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(msg) + await _mcp_call_tool( + conn, "self.robot.set_head_angles", + {"yaw": yaw, "pitch": pitch, "speed": speed}, + ) except Exception: pass @@ -360,20 +333,7 @@ async def _send_set_state(conn: "ConnectionHandler", state: str) -> None: StateManager handles the transition (pip update + idle profile + state_changed event back to the bridge).""" try: - msg = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_state", - "arguments": {"state": state}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(msg) + await _mcp_call_tool(conn, "self.robot.set_state", {"state": state}) except Exception as exc: try: conn.logger.bind(tag=TAG).warning(f"set_state {state} failed: {exc}") @@ -386,20 +346,9 @@ async def _send_set_toggle(conn: "ConnectionHandler", name: str, enabled: bool) kid_mode (warm pink pip on right ring index 8) and smart_mode (orange pip on right ring index 9). Toggles compose freely with state.""" try: - msg = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.robot.set_toggle", - "arguments": {"name": name, "enabled": enabled}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(msg) + await _mcp_call_tool( + conn, "self.robot.set_toggle", {"name": name, "enabled": enabled}, + ) except Exception as exc: try: conn.logger.bind(tag=TAG).warning(f"set_toggle {name}={enabled} failed: {exc}") @@ -443,20 +392,7 @@ async def _handle_vision(conn: "ConnectionHandler", text: str) -> str | None: device_id = conn.headers.get("device-id", "unknown") - mcp_call = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.camera.take_photo", - "arguments": {"question": text}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(mcp_call) + await _mcp_call_tool(conn, "self.camera.take_photo", {"question": text}) conn.logger.bind(tag=TAG).info(f"Vision: sent take_photo MCP call, device={device_id}") try: @@ -546,20 +482,10 @@ async def _capture_room_description_async( # tasks.md §Layer 4 v1.5 for the full diagnosis. body: dict | None = None for attempt in range(3): - mcp_call = json.dumps({ - "session_id": conn.session_id, - "type": "mcp", - "payload": { - "jsonrpc": "2.0", - "method": "tools/call", - "params": { - "name": "self.camera.take_photo", - "arguments": {"question": _ROOM_VIEW_VLM_QUESTION}, - }, - "id": int(time.time() * 1000) % 0x7FFFFFFF, - }, - }) - await conn.websocket.send(mcp_call) + await _mcp_call_tool( + conn, "self.camera.take_photo", + {"question": _ROOM_VIEW_VLM_QUESTION}, + ) if attempt == 0: conn.logger.bind(tag=TAG).info( f"room_view: capture started device={device_id}" diff --git a/tests/test_admin_auth_middleware.py b/tests/test_admin_auth_middleware.py index 41e1692..1fc5d75 100644 --- a/tests/test_admin_auth_middleware.py +++ b/tests/test_admin_auth_middleware.py @@ -16,12 +16,25 @@ for _n in ( "config", "config.logger", "core", "core.api", "core.api.ota_handler", - "core.api.vision_handler", "core.portal_bridge", + "core.api.vision_handler", "core.portal_bridge", "core.utils", ): sys.modules.setdefault(_n, MagicMock()) sys.modules["config.logger"].setup_logging = lambda: MagicMock() # type: ignore[attr-defined] sys.modules["core.portal_bridge"].active_connections = {} # type: ignore[attr-defined] +# The DOTTY DeviceCommand seam is a real, dependency-free module — load it by +# path and install it at its container import path so http_server.py binds the +# real id/lock logic instead of a MagicMock attribute. +_DC_PY = ( + pathlib.Path(__file__).parent.parent + / "custom-providers" / "xiaozhi-patches" / "device_command.py" +) +_dc_spec = _ilu.spec_from_file_location("core.utils.device_command", _DC_PY) +_dc_mod = _ilu.module_from_spec(_dc_spec) # type: ignore[arg-type] +_dc_spec.loader.exec_module(_dc_mod) # type: ignore[union-attr] +sys.modules["core.utils"].device_command = _dc_mod # type: ignore[attr-defined] +sys.modules["core.utils.device_command"] = _dc_mod + _SERVER_PY = ( pathlib.Path(__file__).parent.parent / "custom-providers" / "xiaozhi-patches" / "http_server.py" diff --git a/tests/test_device_command.py b/tests/test_device_command.py new file mode 100644 index 0000000..549934b --- /dev/null +++ b/tests/test_device_command.py @@ -0,0 +1,247 @@ +"""Tests for the DeviceCommand seam (xiaozhi-patches/device_command.py) +and its wiring into the http_server admin handlers. + +The seam replaces twelve hand-rolled MCP envelopes (2026-06-06 audit): +ids were `int(time.time()*1000) % 0x7FFFFFFF` (same-millisecond calls +collided) and every site called `conn.websocket.send()` with no +coordination (the websockets library forbids interleaved sends). +""" +import asyncio +import importlib.util as _ilu +import json +import pathlib +import sys +import types +import unittest +from unittest.mock import MagicMock + +_PATCHES = pathlib.Path(__file__).parent.parent / "custom-providers" / "xiaozhi-patches" + +_dc_spec = _ilu.spec_from_file_location("device_command_under_test", _PATCHES / "device_command.py") +dc = _ilu.module_from_spec(_dc_spec) # type: ignore[arg-type] +_dc_spec.loader.exec_module(dc) # type: ignore[union-attr] + + +class _RecordingWS: + def __init__(self): + self.sent: list = [] + + async def send(self, message): + self.sent.append(message) + + +class _SlowWS: + """Records how many sends overlap — the lock must keep it at 1.""" + + def __init__(self): + self.sent: list = [] + self._in_send = 0 + self.max_concurrent = 0 + + async def send(self, message): + self._in_send += 1 + self.max_concurrent = max(self.max_concurrent, self._in_send) + await asyncio.sleep(0.005) + self.sent.append(message) + self._in_send -= 1 + + +class _FakeConn: + def __init__(self, ws=None): + self.session_id = "sess-1" + self.websocket = ws or _RecordingWS() + + +class TestRequestIds(unittest.TestCase): + + def test_ids_are_monotonic_and_collision_free(self): + conn = _FakeConn() + ids = [dc.next_request_id(conn) for _ in range(1000)] + self.assertEqual(ids, list(range(1, 1001))) + self.assertEqual(len(set(ids)), 1000) + + def test_counters_are_per_connection(self): + a, b = _FakeConn(), _FakeConn() + self.assertEqual(dc.next_request_id(a), 1) + self.assertEqual(dc.next_request_id(a), 2) + self.assertEqual(dc.next_request_id(b), 1, "fresh conn restarts at 1") + + +class TestEnvelope(unittest.TestCase): + + def test_wire_shape_matches_the_old_hand_rolled_envelope(self): + conn = _FakeConn() + frame = json.loads(dc.mcp_envelope( + conn, "self.robot.set_state", {"state": "sleep"}, 7, + )) + self.assertEqual(frame, { + "session_id": "sess-1", + "type": "mcp", + "payload": { + "jsonrpc": "2.0", + "method": "tools/call", + "params": { + "name": "self.robot.set_state", + "arguments": {"state": "sleep"}, + }, + "id": 7, + }, + }) + + def test_missing_session_id_degrades_to_empty_string(self): + conn = types.SimpleNamespace(websocket=_RecordingWS()) + frame = json.loads(dc.mcp_envelope(conn, "t", {}, 1)) + self.assertEqual(frame["session_id"], "") + + +class TestCallTool(unittest.TestCase): + + def test_sends_envelope_and_returns_id(self): + async def go(): + conn = _FakeConn() + rid1 = await dc.call_tool(conn, "self.camera.take_photo", {"question": "q"}) + rid2 = await dc.call_tool(conn, "self.robot.set_state", {"state": "idle"}) + self.assertEqual((rid1, rid2), (1, 2)) + frames = [json.loads(m) for m in conn.websocket.sent] + self.assertEqual( + [f["payload"]["id"] for f in frames], [1, 2], + ) + self.assertEqual( + frames[0]["payload"]["params"]["name"], "self.camera.take_photo", + ) + asyncio.run(go()) + + def test_concurrent_sends_are_serialized(self): + async def go(): + ws = _SlowWS() + conn = _FakeConn(ws=ws) + await asyncio.gather(*[ + dc.call_tool(conn, "self.robot.set_head_angles", {"yaw": i}) + for i in range(5) + ]) + self.assertEqual(ws.max_concurrent, 1, "sends must never overlap") + ids = [json.loads(m)["payload"]["id"] for m in ws.sent] + self.assertEqual(sorted(ids), [1, 2, 3, 4, 5]) + asyncio.run(go()) + + def test_send_serialized_mixes_with_call_tool_under_one_lock(self): + async def go(): + ws = _SlowWS() + conn = _FakeConn(ws=ws) + await asyncio.gather( + dc.call_tool(conn, "t", {}), + dc.send_serialized(conn, b"\x01\x02"), + dc.call_tool(conn, "t", {}), + ) + self.assertEqual(ws.max_concurrent, 1) + self.assertEqual(len(ws.sent), 3) + asyncio.run(go()) + + +class TestHttpServerWiring(unittest.TestCase): + """The admin MCP handlers route through the seam: monotonic ids on + the wire, shared conn resolution, fire-and-forget HTTP semantics.""" + + @classmethod + def setUpClass(cls): + stubbed = ( + "config", "config.logger", "core", "core.api", + "core.api.ota_handler", "core.api.vision_handler", + "core.portal_bridge", "core.utils", "core.utils.device_command", + ) + missing = object() + cls._saved = {k: sys.modules.get(k, missing) for k in stubbed} + cls._missing = missing + + cls.active: dict = {} + portal = MagicMock() + portal.active_connections = cls.active + logger_mod = MagicMock() + logger_mod.setup_logging = lambda: MagicMock() + for n in ("config", "core", "core.api", "core.api.ota_handler", + "core.api.vision_handler"): + sys.modules[n] = MagicMock() + sys.modules["config.logger"] = logger_mod + sys.modules["core.portal_bridge"] = portal + core_utils = MagicMock() + core_utils.device_command = dc + sys.modules["core.utils"] = core_utils + sys.modules["core.utils.device_command"] = dc + + spec = _ilu.spec_from_file_location( + "http_server_dc_under_test", + _PATCHES / "http_server.py", + ) + cls.mod = _ilu.module_from_spec(spec) # type: ignore[arg-type] + spec.loader.exec_module(cls.mod) # type: ignore[union-attr] + + @classmethod + def tearDownClass(cls): + for k, v in cls._saved.items(): + if v is cls._missing: + sys.modules.pop(k, None) + else: + sys.modules[k] = v + + def setUp(self): + type(self).active.clear() + + def _server(self): + return self.mod.SimpleHttpServer( + {"server": {"ip": "0.0.0.0", "http_port": 8003}} + ) + + @staticmethod + def _request(data): + class _Req: + async def json(self): + return data + return _Req() + + def test_set_state_sends_seam_envelope_with_monotonic_ids(self): + async def go(): + conn = _FakeConn() + conn.headers = {"device-id": "dev-1"} + type(self).active["dev-1"] = conn + srv = self._server() + r1 = await srv._dotty_set_state(self._request({"state": "sleep"})) + r2 = await srv._dotty_set_state(self._request({"state": "idle"})) + self.assertEqual((r1.status, r2.status), (200, 200)) + # Sends are _spawn()-ed fire-and-forget; let the tasks run. + await asyncio.sleep(0.02) + frames = [json.loads(m) for m in conn.websocket.sent] + self.assertEqual([f["payload"]["id"] for f in frames], [1, 2]) + self.assertEqual( + [f["payload"]["params"]["arguments"]["state"] for f in frames], + ["sleep", "idle"], + ) + asyncio.run(go()) + + def test_resolve_conn_falls_back_to_first_device_and_503s_when_empty(self): + conn = _FakeConn() + conn.headers = {"device-id": "dev-a"} + type(self).active["dev-a"] = conn + got, err = self.mod._dotty_resolve_conn("") + self.assertIs(got, conn) + self.assertIsNone(err) + type(self).active.clear() + got, err = self.mod._dotty_resolve_conn("") + self.assertIsNone(got) + self.assertEqual(err.status, 503) + + def test_no_ms_truncated_ids_left_anywhere(self): + # The collision-prone id pattern must not reappear in either + # patch surface (the audit found it at twelve sites). + for path in ( + _PATCHES / "http_server.py", + pathlib.Path(__file__).parent.parent / "receiveAudioHandle.py", + ): + src = path.read_text(encoding="utf-8") + self.assertNotIn( + "% 0x7FFFFFFF", src, + f"ms-truncated MCP id pattern resurfaced in {path.name}", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_play_asset_route.py b/tests/test_play_asset_route.py index 81080c8..3d93ec8 100644 --- a/tests/test_play_asset_route.py +++ b/tests/test_play_asset_route.py @@ -30,6 +30,7 @@ "core.api.ota_handler", "core.api.vision_handler", "core.portal_bridge", + "core.utils", ): sys.modules.setdefault(_n, MagicMock()) @@ -39,6 +40,19 @@ # ── Load module under test via importlib (not on the normal package path) ───── import importlib.util as _ilu +# The DOTTY DeviceCommand seam is a real, dependency-free module — load it by +# path and install it at its container import path so http_server.py binds the +# real id/lock logic instead of a MagicMock attribute. +_DC_PY = ( + pathlib.Path(__file__).parent.parent + / "custom-providers" / "xiaozhi-patches" / "device_command.py" +) +_dc_spec = _ilu.spec_from_file_location("core.utils.device_command", _DC_PY) +_dc_mod = _ilu.module_from_spec(_dc_spec) # type: ignore[arg-type] +_dc_spec.loader.exec_module(_dc_mod) # type: ignore[union-attr] +sys.modules["core.utils"].device_command = _dc_mod # type: ignore[attr-defined] +sys.modules["core.utils.device_command"] = _dc_mod + _SERVER_PY = ( pathlib.Path(__file__).parent.parent / "custom-providers"