# Javis Slave & Slave-MCP — Code Audit

**Scope:** [javis-slave](../javis-slave) (satellite app: touch GUI + headless/Raspberry Pi
mode, ~3.6 k lines) and [javis-slave-mcp](../javis-slave-mcp) (MCP server exposing the
satellites' controls to the master, ~1 k lines), including `pyproject.toml`,
`setup-pi.sh`, the systemd unit and the READMEs.

**Method** — same as [AUDIT.md](AUDIT.md): every file read line by line, `ruff
--select F,E9,B` baseline (8 findings), and each claim reproduced where possible:

* the MCP tool failure was reproduced by calling the tool through FastMCP
  (`mcp` 1.29.0 in `javis-slave-mcp/.venv`);
* the unauthenticated control API was reproduced with FastAPI's `TestClient`;
* the Windows volume bug was reproduced in a scratch venv with the current `pycaw`
  (20260927, comtypes 1.4.17).

Line numbers refer to the audited revision `0246cbb6`.

---

## 1. Architecture

```mermaid
flowchart LR
    subgraph Satellite["javis-slave (tablet / Raspberry Pi)"]
        MIC[MicStream + resampler] --> WW[openWakeWord]
        MIC --> REC[record_utterance + VAD]
        ENG[AudioEngine] --> SPK[play_pcm]
        CTRL["Control API :8766<br/>/mcp/v1/*"] --> SYS[volume / brightness / power]
        UI[Touch UI or headless log]
    end
    WW --> ENG
    REC --> ENG
    ENG -- "POST /api/v1/turn (Bearer key)" --> MASTER[Javis master :8765]
    ENG -- "GET /api/v1/config (60 s poll)" --> MASTER
    MCP[javis-slave-mcp<br/>FastMCP stdio] -- HTTP --> CTRL
    MASTER -- spawns --> MCP
    CTRL -- "pass-through /api/v1/slaves" --> MASTER
```

* The satellite owns the wake word and end-pointing; the master does STT → LLM → TTS
  and returns WAV.
* The control API is a FastAPI app served by uvicorn inside the same event loop
  (qasync in GUI mode, `asyncio.run` headless).
* `javis-slave-mcp` is spawned by the master's MCP host and calls each configured
  satellite's control API.

---

## 2. Findings summary

| ID | Sev. | Component | Title | Status |
|---|---|---|---|---|
| [MCP-S1](#mcp-s1) | Critical | slave-mcp | Every device tool fails: `asyncio.run()` inside the server's event loop | Fixed |
| [SLV-01](#slv-01) | Critical | slave / security | Control API (power, mic, volume, master pass-through) has no authentication | Fixed |
| [SLV-02](#slv-02) | High | slave / Windows | Volume control broken with current `pycaw` | Fixed |
| [SLV-03](#slv-03) | Medium | slave | Turn upload / pairing retried after a read timeout → duplicate turn, burnt code | Fixed |
| [SLV-04](#slv-04) | Medium | slave | Failed engine start leaks the open microphone (ALSA: busy forever) | Fixed |
| [SLV-05](#slv-05) | Medium | slave | Playback thread keeps writing while the stream is closed | Fixed |
| [SLV-06](#slv-06) | Medium | slave / security | Master-supplied wake-word name used as a file path | Fixed |
| [SLV-07](#slv-07) | Medium | slave | Corrupt config silently replaced → device_id and pairing lost | Fixed |
| [SLV-08](#slv-08) | Medium | slave / security | `slave_config.json` (API key) world-readable on Linux | Fixed |
| [MCP-S2](#mcp-s2) | Medium | slave-mcp | `slave_get_ip_address` looks up the alias instead of the device UUID | Fixed |
| [MCP-S3](#mcp-s3) | Medium | slave-mcp | Power tools not marked destructive | Fixed (annotations) |
| [SLV-09](#slv-09) | Low | slave / Windows | Power commands report success regardless of exit code | Fixed |
| [SLV-10](#slv-10) | Low | slave | Retry counter double-counted; misleading "re-pair manually" error | Fixed |
| [SLV-11](#slv-11) | Low | slave | 30 ms chunks zero-padded to Silero's 512-sample window | Fixed |
| [SLV-12](#slv-12) | Low | slave | Mic callback can raise during shutdown | Fixed |
| [SLV-13](#slv-13) | Low | slave | Fire-and-forget tasks without strong references | Fixed |
| [SLV-14](#slv-14) | Low | slave / GUI | Transcript bubbles render LLM text as rich text | Fixed |
| [MCP-S4](#mcp-s4) | Low | slave-mcp | `JAVIS_SLAVE_DEVICES` parsing fragile | Fixed |
| [MCP-S5](#mcp-s5) | Low | slave-mcp | 25 copy-pasted tool bodies, client per call, README drift | Fixed |
| [LINT-S](#lint-s) | Low | both | Ruff F/B findings | Fixed |
| [SLV-15](#slv-15) | Info | slave | `windows_control.py` is an unused copy of `system_control.py` | Open (delete?) |
| [SLV-16](#slv-16) | Info | slave | uvicorn's signal capture overrides the headless SIGTERM handler | Fixed (pin) |
| [SLV-17](#slv-17) | Info | slave / security | Plain HTTP; the satellite does not authenticate the master | Open (recommendation) |
| [SLV-18](#slv-18) | Info | Pi deployment | systemd unit has no sandboxing | Open (recommendation) |

---

## 3. Detailed findings

### javis-slave-mcp

#### MCP-S1
**Every device tool fails: `asyncio.run()` inside the server's event loop** — *Critical*

All 24 device tools are plain `def` functions that call
`asyncio.run(_slaves[id].get_status())` (server.py L319–L816). FastMCP 1.x calls a
synchronous tool **directly on its running event loop**
(`FuncMetadata.call_fn_with_arg_validation`: `return fn(**args)`), and `asyncio.run`
refuses to start a second loop there. Reproduced:

```
slave_get_volume → ERROR: asyncio.run() cannot be called from a running event loop
```

So "mach die Küche leiser" could never work; the master only ever got `ERROR:` back.
**Fix:** all tools are `async def` and await the client directly; one `_run()` helper
raises failures as `ToolError`, which FastMCP returns as a result with
`isError: true` (the master's MCP host turns that into its usual `ERROR:` text).

#### MCP-S2
**`slave_get_ip_address` looks up the alias instead of the device UUID** — *Medium*

`get_slave_ip(device_id)` passes the MCP alias (`"localhost"`, `"kitchen"`) to
`/mcp/v1/master/slaves/{id}/ip`, but the master keys satellites by their UUID, so the
lookup always returned 404. **Fix:** the tool first reads the satellite's real
`device_id` from `/mcp/v1/status`.

#### MCP-S3
**Power tools not marked destructive** — *Medium*

`slave_shutdown`, `slave_restart_system`, `slave_sleep` are indistinguishable from
read-only tools, and the shipped `mcp_servers.json` sets `"approval": "allow"` for this
server, so a mis-heard request can power off a satellite without confirmation.
**Fix:** MCP tool annotations (`readOnlyHint`, `destructiveHint`, `idempotentHint`)
on every tool. The master's MCP host now asks for approval for `destructiveHint`
tools even on `allow` servers (`mcp.confirm_destructive`, default `true`; added in the
review pass after [AUDIT_MCP.md](AUDIT_MCP.md)).

#### MCP-S4
**`JAVIS_SLAVE_DEVICES` parsing fragile** — *Low*

Keys were not stripped (`"a=…, kitchen=…"` registered `" kitchen"`), and one malformed
entry aborted parsing of all following ones. **Fix:** per-entry parsing with stripped
keys and a warning per bad entry; optional token per entry.

#### MCP-S5
**Duplication and drift** — *Low*

24 copies of the same try/`asyncio.run`/except block, a new `httpx.AsyncClient` per
call, unused imports, and README entries that don't match the code
(`slave_sleep` has no `delay`; brightness steps default to 5, not 10).
**Fix:** one shared client per satellite (`trust_env=False`, so the token is never sent
through an environment proxy), one `_run()` helper, README corrected.

### javis-slave

#### SLV-01
**Control API has no authentication** — *Critical*

`build_http_app` (http_server.py) registers `/mcp/v1/power/{sleep,shutdown,restart}`,
mic mute, volume, brightness, engine restart and the master pass-through
`/mcp/v1/master/slaves` without any auth, and `api_host` defaults to `0.0.0.0`.
Reproduced: `POST /mcp/v1/power/cancel` from an unauthenticated client → **200**.

* Anyone on the LAN can power off / reboot the satellite, silence its microphone or
  read the master's full device list (names, rooms, IPs) through the pass-through,
  which uses the satellite's own master credentials.
* Even on loopback, any web page open on the device can trigger them: a body-less
  `POST` is a CORS "simple request", so the browser sends it without a preflight
  (classic CSRF).

**Fix:**
* each satellite gets a random `control_token` (generated on first start, stored in
  its config; `--control-token` sets a household-wide one, `--show-control-token`
  prints it, the settings dialog shows it);
* every endpoint except `/mcp/v1/health` requires `Authorization: Bearer <token>`
  (constant-time compare);
* zero-config compatibility: callers on **loopback** may omit the token but must send
  `X-Javis-Control: 1`. A browser cannot add that header cross-origin without a
  preflight, which this server never grants, so CSRF stays blocked;
* `javis-slave-mcp` sends the header always and the token from `JAVIS_SLAVE_TOKEN`
  or per device.

#### SLV-02
**Volume control broken with current `pycaw`** — *High*

`VolumeController` calls `AudioUtilities.GetSpeakers().Activate(IAudioEndpointVolume._iid_, 0, None)`
and uses the result directly. Since pycaw 2024 `GetSpeakers()` returns an
`AudioDevice` wrapper **without** `Activate` (it exposes `.EndpointVolume`); on older
pycaw the returned `IUnknown` pointer also needed a `cast` to `IAudioEndpointVolume`.
Reproduced: `AttributeError: 'AudioDevice' object has no attribute 'Activate'` → the
controller disables itself and every volume endpoint fails on Windows.
**Fix:** use `EndpointVolume` when present, else the documented
`cast(Activate(..., CLSCTX_ALL, None), POINTER(IAudioEndpointVolume))`. The endpoint is
resolved per call, so it follows default-device changes and no COM pointer is shared
between `asyncio.to_thread` workers (calling from workers was verified to work).

#### SLV-03
**Turn upload / pairing retried after a read timeout** — *Medium*

`_retryable_request` retries on any `httpx.HTTPError`, including `ReadTimeout` /
`RemoteProtocolError`, i.e. after the master already received the request.
`send_turn` (`retries=1`) then re-runs the whole STT → LLM → tools turn ("add milk to
the list" twice), and `pair` re-submits a single-use code that was already consumed
(the user sees "code invalid" although pairing succeeded on the master).
**Fix:** non-idempotent calls only retry when the connection could not be established
(`ConnectError`, `ConnectTimeout`).

#### SLV-04
**Failed engine start leaks the open microphone** — *Medium*

`AudioEngine.start()` opens the mic, then builds the VAD and loads/downloads the wake
model. If the latter fails (custom model not on the master, ONNX error), the started
`InputStream` is never closed. Both the GUI and headless runners retry every 10 s with
a new engine: each retry opens another stream; on a Raspberry Pi with an ALSA `hw:`
device the second open fails with "Device or resource busy", so the satellite never
recovers without a restart. **Fix:** `start()` stops everything it opened on failure.

#### SLV-05
**Playback thread keeps writing while the stream is closed** — *Medium*

Same defect as master AGT-06: `play_pcm` awaits `asyncio.to_thread(_writer)`. Restart,
re-pair or shutdown during a reply cancels the engine task; `to_thread` returns at
once while the thread keeps calling `stream.write()`, and the `finally` block calls
`stop()/close()` concurrently. **Fix:** cancellation-safe block writer that stops and
joins the writer thread before the stream is closed.

#### SLV-06
**Master-supplied wake-word name used as a file path** — *Medium*

`_load_wake` downloads to `config_dir()/models/f"{name}.onnx"` with `name` taken from
the master's `/config` response. Over plain HTTP (SLV-17), a spoofed or compromised
master could send `../../.config/autostart/x` and write files outside the models
folder. **Fix:** names containing path separators or `..` are rejected.

#### SLV-07
**Corrupt config silently replaced** — *Medium*

`load_config()` swallows any parse error and returns fresh defaults, including a
**new random `device_id`**; both entry points immediately `save_config()` it. A
truncated `slave_config.json` (SD card + power loss — `tmp.replace` without `fsync`
is not durable) therefore wipes the pairing and the satellite reappears as a new device.
**Fix:** unreadable files are preserved as `slave_config.json.corrupt` with an error
log; saves `fsync` before the atomic replace; values from JSON are type-checked.

#### SLV-08
**`slave_config.json` (API key) world-readable on Linux** — *Medium*

The file holds the master API key (and now the control token) but is created with the
default umask (0644): every local user on the Pi can read it. **Fix:** `chmod 600` on
POSIX after every write.

#### SLV-09
**Windows power commands report success regardless of exit code** — *Low*

`subprocess.run(["shutdown", ...], check=False)` → always `True`, e.g.
`/mcp/v1/power/cancel` said "cancelled" when nothing was pending (exit 1116), and a
console window flashed in GUI mode. **Fix:** shared runner with timeout,
`CREATE_NO_WINDOW` and return-code check (exit 1116 "no shutdown in progress" counts as
success for *cancel*). `sleep` is started without waiting: `SetSuspendState` only
returns after resume, which held the HTTP reply until the machine woke up.

#### SLV-10
**Retry counter double-counted; misleading error** — *Low*

`_retryable_request` already calls `mark_disconnected()` per attempt; `_handle_turn`
and `_config_poll_loop` called it again, so the counter grew twice as fast. After 10
the UI said "Maximale Wiederverbindungsversuche erreicht. Bitte manuell neu koppeln."
although polling continues and recovers automatically. **Fix:** single bookkeeping
point; the message now says reconnection keeps being attempted.

#### SLV-11
**30 ms chunks zero-padded** — *Low*

Same as master AUD-02. **Fix:** 32 ms (512 samples @ 16 kHz).

#### SLV-12
**Mic callback can raise during shutdown** — *Low*

Same as master AUD-04. **Fix:** guarded.

#### SLV-13
**Fire-and-forget tasks** — *Low*

`ensure_future()` results discarded for chimes, wake-model swaps, pairing, mute,
HTTP-server start, engine retry. **Fix:** small `spawn()` helper keeping references
and logging failures.

#### SLV-14
**Transcript bubbles render LLM text as rich text** — *Low*

`QLabel(f"🤖 {reply_text}")` uses `Qt::AutoText`; replies containing HTML (e.g.
echoed from web search results) are rendered, including `<img src="file:…">`.
**Fix:** `Qt.PlainText` for user/assistant/error bubbles.

#### SLV-15
**`windows_control.py` is an unused copy** — *Info*

It differs from `system_control.py` only in the logger import and is imported nowhere.
Recommend deleting it (left in place pending your confirmation).

#### SLV-16
**uvicorn overrides the headless SIGTERM handler** — *Info*

`Server.serve()` installs its own SIGINT/SIGTERM handlers after the headless runner
registered `stop.set`. uvicorn ≥ 0.29 re-raises captured signals when it exits, so
shutdown still works — older versions (the spec allowed `>=0.24`) would leave
`systemctl stop` hanging until SIGKILL. **Fix:** `uvicorn>=0.29`.

#### SLV-17
**Plain HTTP; the satellite does not authenticate the master** — *Info*

API key, transcripts and reply audio travel unencrypted, and anything answering on the
master's IP is trusted (wake-word settings, model downloads). 📌 Run the master behind
TLS (Caddy) or keep satellites on a trusted VLAN; SLV-06 limits the worst case.

#### SLV-18
**systemd unit has no sandboxing** — *Info*

📌 Consider `ProtectSystem=full`, `ProtectHome=read-only`, `PrivateTmp=yes`,
`ReadWritePaths=/opt/javis-slave/config` (sudo for `shutdown` keeps working;
`NoNewPrivileges=yes` would not). Not applied: it can't be tested here, and
`ProtectHome` breaks installs under `/home`; see §6 for the options that must stay off.

#### LINT-S
Unused imports (`SlaveConfig`, `Any`, `Context`, `Field`), missing exception chaining
(B904 ×2), f-strings without placeholders (F541 ×2). **Fixed.**

---

## 4. Reviewed and rejected

| Claim | Why rejected |
|---|---|
| Stateful resampler drops/duplicates samples between blocks | Phase carry-over and the `_last` sample are handled correctly; output length tracks the ratio. |
| `MicStream.stop()` can block a consumer forever | `_drain_and_sentinel` evicts until the sentinel fits. |
| Mute race between API and UI | `_mute_lock` + `_unmuted` event serialise it. |
| Pairing code brute force from the satellite side | Codes are validated and rate-limited on the master (see master API audit). |

---

## 5. Verification

| Check | Result |
|---|---|
| `ruff check javis-slave/src javis-slave-mcp/src --select F,E9,B` | clean (was 8 findings) |
| `pytest tests` (main venv) | 59 passed, 1 skipped (`test_slave_mcp.py` needs mcp 1.x) |
| `javis-slave-mcp/.venv` → `pytest tests/test_slave_mcp.py` | 5 passed |
| Offscreen GUI smoke (`SlaveWindow`, bubbles, settings dialog) | plain-text bubbles, token field masked |

New regression tests:

* [tests/test_slave_audit.py](../tests/test_slave_audit.py):
  * Config: a corrupt file is kept as `.corrupt`, loading is type-checked, and the token round-trips.
  * Control API:
    * LAN callers need the Bearer token.
    * Loopback callers need `X-Javis-Control`.
    * Plain loopback requests (CSRF) and empty tokens are refused.
    * `/docs` is gone.
  * Master client: `send_turn` does not retry after `ReadTimeout`, but retries a `ConnectError` once.
  * Audio:
    * A failed engine start releases the mic.
    * Wake-model names with path parts are rejected before any download.
    * Cancelled playback stops writing before the stream can be closed.
  * System control: the pycaw `EndpointVolume` path and Windows exit-code handling.
* [tests/test_slave_mcp.py](../tests/test_slave_mcp.py):
  * Env parsing skips bad entries and never puts the token in `repr`.
  * A tool called through `FastMCP.call_tool` on a running loop resolves the real UUID and sends both auth headers.
  * 401 responses and unknown devices become tool errors.
  * Power tools carry `destructiveHint`.

No test calls a real power, volume or brightness endpoint. The controllers are faked, or the
auth middleware rejects the request before the handler runs.

---

## 6. Improvements and best practices

Researched after the fix pass; ✅ = applied, 📌 = recommendation.

| Source | Guidance | Status |
|---|---|---|
| [MCP spec — Tools](https://modelcontextprotocol.io/specification/2025-06-18/server/tools) | Tool execution failures belong in the result with `isError: true`. Protocol errors are for unknown tools and bad arguments. | ✅ `ToolError` from `_run()` |
| MCP spec — Tools | Annotations describe behaviour, but clients **MUST** treat them as untrusted unless the server is trusted. A human **SHOULD** be able to deny invocations. | ✅ Annotations on all 23 tools. ✅ The master's MCP host asks before `destructiveHint` tools, even on `approval: allow` servers. |
| MCP spec — Tools, security | Servers **MUST** validate inputs and implement access control, and **SHOULD** rate-limit. | ✅ FastAPI `Query(ge/le)` bounds + auth. 📌 No rate limit on power endpoints yet. |
| [OWASP CSRF cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html) | Custom request headers are a valid CSRF defence for APIs because they force a CORS preflight. Never change state via `GET`. Compare secrets in constant time. | ✅ `X-Javis-Control` on loopback, no CORS middleware, all state changes use `POST`, `hmac.compare_digest`. 📌 Optionally also reject `Sec-Fetch-Site: cross-site`. |
| [uvicorn 0.29.0 release](https://github.com/encode/uvicorn/releases/tag/0.29.0) | "Cooperative signal handling" (#1600): captured signals are re-raised after shutdown. | ✅ `uvicorn>=0.29` |
| [pycaw README](https://github.com/AndreMiras/pycaw) | Current API: `AudioUtilities.GetSpeakers().EndpointVolume`. | ✅ Used, with an `Activate`+`cast` fallback for old pycaw |
| [systemd.exec(5)](https://man7.org/linux/man-pages/man5/systemd.exec.5.html) | Enable `ProtectSystem=` for all long-running services and `ProtectHome=` for network-facing ones. `NoNewPrivileges=` blocks setuid, `ProtectKernelTunables=` makes `/sys` read-only, and `PrivateDevices=` removes physical devices. | 📌 Safe set for this unit: `ProtectSystem=full`, `PrivateTmp=yes`, `ProtectHome=read-only` (only for `/opt` installs). These must stay **off**: `NoNewPrivileges` (sudo power control), `ProtectKernelTunables` (backlight), `PrivateDevices` (audio). |
| systemd.exec(5), *Environment=* | Environment variables are not suitable for secrets: they are visible via D-Bus and inherited by child processes. | 📌 For remote satellites, prefer `"token"` in `~/.javis/slave_mcp_config.json` (keep it 0600) over `JAVIS_SLAVE_TOKEN` in `mcp_servers.json`. |
| Python `hmac.compare_digest` / `secrets.token_urlsafe` | Use constant-time comparison and CSPRNG tokens. | ✅ 24-byte URL-safe token, constant-time check |

Observations (no action):

* `mcp` 1.29 with the current `pydantic-settings` prints an `IncompleteFieldDefinitionWarning`
  (`lifespan`) when `FastMCP()` is constructed. This is an upstream SDK issue and harmless.
* `mcp` 2.x removed `mcp.server.fastmcp`, so `javis-slave-mcp` now pins `mcp>=1.10,<2.0`
  (tool annotations need ≥ 1.10). The master's MCP host already supports both.
* Still open:
  * SLV-15: delete `windows_control.py`?
  * SLV-17: TLS.
  * SLV-18: sandboxing.
  * Rate limiting.