# Javis — Code Audit

**Scope:** the main application `src/javis/**` (~19 k lines across 59 modules), plus
`pyproject.toml`, `requirements.txt`, `config.example.yaml` and `README.md`.
The satellite packages (`javis-slave`, `*-mcp`, `voice-ref-curator`) are only covered
where they interact with the master (API, MCP).

**Method**

1. Baseline tooling: `ruff check --select F,E9,B` (37 findings), `compileall` (clean),
   `pytest` (not installed in `.venv`, so no tests ran).
2. Line-by-line review of every subsystem (core, audio, STT, TTS/voices, LLM, MCP,
   API, agent, GUI).
3. Every finding below was **re-verified against the source** (and, where relevant,
   against the installed library versions: `mcp 2.0.0`, `fastapi 0.141`,
   `starlette 1.6`, `faster-whisper 1.2.1`, `silero-vad 6.2.1`, `PySide6 6.11`,
   `qasync 0.28`). Claims that did not survive verification are listed under
   [Reviewed and rejected](#4-reviewed-and-rejected-false-positives).

Line numbers in the findings refer to the audited revision `0246cbb6` (before the fixes).

**Severity scale**

| Level | Meaning |
|---|---|
| Critical | Crash or core feature broken for a typical user |
| High | Security weakness or significant malfunction |
| Medium | Robustness / correctness problem under plausible conditions |
| Low | Minor bug, hygiene, or edge case |
| Info | Design note / recommendation, no defect |

---

## 1. Architecture overview

```mermaid
flowchart LR
    subgraph Input
        MIC[MicStream<br/>sounddevice cb] --> WW[openWakeWord]
        MIC --> VAD[Silero VAD]
        VAD --> REC[record_utterance]
        TXT[GUI text input] --> ORCH
        SLAVE[javis-slave<br/>FastAPI /turn] --> ORCH
    end
    WW -->|wake| ORCH[Orchestrator<br/>state machine + turn lock]
    REC --> STT[faster-whisper] --> ORCH
    REC --> SPK[ECAPA speaker ID] --> ORCH
    ORCH --> QUICK[QuickEngine<br/>LLM bypass]
    ORCH --> MW[Middleware pipeline<br/>Privilege / HomeyGuard / Memory]
    MW --> LLM[Ollama client]
    LLM -->|tool calls| MCP[MCPHost<br/>stdio servers]
    QUICK --> MCP
    ORCH --> TTS[Piper / F5 backend] --> OUT[OutputStream]
    ORCH -. EventBus .-> GUI[PySide6 + qasync GUI]
```

* **Single event loop.** Everything runs on one asyncio loop (qasync in GUI mode,
  `asyncio.run` headless). Blocking work (STT, TTS, speaker ID, PCM writes) is pushed
  to threads with `asyncio.to_thread`.
* **Turn serialisation.** `Orchestrator._turn_lock` serialises wake-word, PTT, typed,
  remote (slave) turns, enrollment and timer announcements.
* **Middleware.** TanStack-style hooks (`on_before_llm` piped, `on_before_tool`
  first-win, the rest sequential). `PrivilegeMiddleware` is the per-speaker ACL.
* **Tool routing.** Keyword heuristics pick a subset of MCP servers per turn; a
  `javis__load_tools` meta-tool lets the model pull in un-routed servers.
* **GUI.** One `_MainWindow` class (defined inside `run_gui()`) receives EventBus
  events through Qt signals.

---

## 2. Findings summary

| ID | Sev. | Area | Title | Status |
|---|---|---|---|---|
| [MCP-01](#mcp-01) | Critical | MCP | MCP SDK 2.x field names → empty tool schemas, tool errors reported as success | Fixed |
| [GUI-01](#gui-01) | Critical | GUI | `NameError` when rendering voice-profile cards | Fixed |
| [SEC-01](#sec-01) | High | Repo / security | Git remote URL embeds a personal access token | **Action required (owner)** |
| [AGT-01](#agt-01) | High | Agent / security | Quick commands bypass speaker privileges | Fixed |
| [MCP-02](#mcp-02) | High | MCP / security | "Allow once" approval is remembered for the whole session | Fixed |
| [MCP-03](#mcp-03) | High | MCP / security | Bare server name executes that server's *first* tool | Fixed |
| [AGT-10](#agt-10) | Medium | Agent | One invalid stored regex quick command breaks every turn | Fixed |
| [MCP-08](#mcp-08) | Medium | MCP | No handshake timeout; servers connected one after another | Fixed |
| [VOI-02](#voi-02) | Medium | Voices | Quitting during scheduled training hangs exit and frees the lock early | Fixed |
| [AGT-02](#agt-02) | Medium | Agent | Quick-command repair pairs actions with the wrong errors | Fixed |
| [AGT-03](#agt-03) | Medium | Agent | `Orchestrator.stop()` runs twice / concurrently | Fixed |
| [AGT-04](#agt-04) | Medium | Agent | State stuck in THINKING (+ endless "thinking" ticks) on early errors | Fixed |
| [AGT-05](#agt-05) | Medium | Agent / GUI | Fire-and-forget tasks without strong references | Fixed |
| [AGT-06](#agt-06) | Medium | Audio out | PCM writer thread keeps writing while the stream is closed | Fixed |
| [MCP-04](#mcp-04) | Medium | MCP | Any tool error triggers server respawn + automatic retry | Fixed |
| [MCP-05](#mcp-05) | Medium | MCP | Exit stacks closed from a different task than they were opened in | Fixed |
| [LLM-01](#llm-01) | Medium | LLM | Streaming errors swallowed → silent empty/partial replies | Fixed |
| [AUD-01](#aud-01) | Medium | Audio in | `record_utterance` can block forever (holding the turn lock) | Fixed |
| [API-01](#api-01) | Medium | API / security | Large bodies parsed before auth and size check | Fixed |
| [GUI-02](#gui-02) | Medium | GUI | Theme registry leaks every rebuilt card widget | Fixed |
| [GUI-04](#gui-04) | Medium | GUI / security | `cmd /k` command line built with `list2cmdline` | Fixed |
| [VOI-01](#voi-01) | Medium | Voices / security | Imported voice archives may carry pickle checkpoints | Fixed (skipped by default) |
| [LLM-02](#llm-02) | Low | LLM | History trimming can drop the in-progress turn | Fixed |
| [AGT-07](#agt-07) | Low | Agent | Timer announcement can leave state SPEAKING | Fixed |
| [AGT-08](#agt-08) | Low | Agent / security | Privilege check fails open on exceptions | Fixed |
| [AUD-02](#aud-02) | Low | Audio in | 30 ms chunks are zero-padded to Silero's 512-sample window | Fixed |
| [AUD-03](#aud-03) | Low | Config | `audio.sample_rate ≠ 16000` silently breaks VAD / wake word | Fixed |
| [AUD-04](#aud-04) | Low | Audio in | Mic callback can raise during shutdown | Fixed |
| [MCP-06](#mcp-06) | Low | MCP | One leaked stderr temp file per server start | Fixed |
| [API-02](#api-02) | Low | API | Pair-attempt table grows forever; internals leaked in errors | Fixed |
| [GUI-05](#gui-05) | Low | GUI | Background tasks not cancelled on quit | Fixed |
| [CORE-01](#core-01) | Low | Logging | Log file date frozen at start; file I/O on the event loop | Fixed |
| [CORE-02](#core-02) | Low | Config | Missing validation (threshold range, YAML root type) | Fixed |
| [LINT-01](#lint-01) | Low | Hygiene | Ruff F/B findings (unused imports, duplicate set items, …) | Fixed |
| [DOC-01](#doc-01) | Low | Docs | README describes XTTS; code ships Piper/F5 | Fixed |
| [DOC-02](#doc-02) | Low | Packaging | Unpinned `mcp` major version; test deps missing | Fixed |
| [AGT-09](#agt-09) | Info | Agent | Substring keyword routing over-includes tool sets | Open (recommendation) |
| [ARCH-01](#arch-01) | Info | Maintainability | 5.3 k-line GUI closure, duplicated TTS pipelines | Open (recommendation) |
| [API-03](#api-03) | Info | API / security | Plain-HTTP LAN API | Open (recommendation) |
| [MCP-07](#mcp-07) | Info | MCP / security | MCP servers inherit the full environment | Open (recommendation) |

---

## 3. Detailed findings

### MCP

#### MCP-01
**MCP SDK 2.x field names → empty tool schemas, tool errors reported as success** — *Critical*

* `mcp_client/schema_bridge.py` L12: `getattr(tool, "inputSchema", None)`
* `mcp_client/host.py` L461: `getattr(result, "isError", False)`
* `mcp_client/host.py` L261: `page.next_cursor`

The dependency is declared as `mcp>=1.2`; the venv resolved **`mcp 2.0.0`**, whose
pydantic models expose *snake_case* attributes (`input_schema`, `is_error`,
`next_cursor`) — the camelCase names are only serialisation aliases and are **not**
attributes (verified: `getattr(Tool(...), "inputSchema", "NOATTR") == "NOATTR"`).

Impact with mcp 2.x:
1. Every MCP tool is advertised to the LLM as `{"type":"object","properties":{}}` — the
   model has to guess every argument name.
2. `isError` results are never prefixed with `ERROR:` → failures are shown as successes
   (✓ in the UI), cached by the cross-turn tool cache, and the quick-command repair
   path never triggers.

Conversely, with mcp 1.x `page.next_cursor` raises `AttributeError`, so the code only
half-works on either major version.

**Fix:** version-agnostic accessors (`input_schema`/`inputSchema`, `is_error`/`isError`,
`next_cursor`/`nextCursor`), and the dependency is now bounded `mcp>=1.2,<3`.

#### MCP-02
**"Allow once" is remembered for the whole session** — *High*

`MCPHost.call()` (L399-L420) adds `(server, tool)` to `_allow_cache` after **any**
positive approval. The GUI dialog's *Always allow* path already calls
`allow_tool()`; *Allow once* (and a spoken "ja") therefore silently behaves like
*Always allow*: subsequent calls of that tool — including ones with completely
different, possibly destructive arguments — run without asking.

**Fix:** a positive answer is now remembered only for the current turn
(`_turn_allow_cache`, cleared together with denials at every new turn); only
*Always allow* persists.

#### MCP-03
**Bare server name executes the server's first tool** — *High*

`validate_tool_name()` (schema_bridge.py L31-L57) "recovers" a tool name without `__`
that equals a server name by returning that server's **first** tool. `MCPHost.call()`
then executes it with whatever arguments the model sent. For a filesystem/Homey/Spotify
server, the first tool can be a write/actuating tool.

**Fix:** no guessing. The call returns
`ERROR: '<name>' is not a tool. Did you mean one of: …` so the model can retry with a
real name.

#### MCP-04
**Every tool exception triggers a respawn + automatic retry** — *Medium*

host.py L425-L445: any exception from `session.call_tool` (including JSON-RPC errors
such as *invalid params*, raised as `McpError`/`MCPError`) tears the server down,
respawns it and repeats the call. Non-idempotent tools (play, switch, write) can run
twice; heavy servers (`npx …`) are restarted for a simple argument error.

**Fix:** protocol errors are returned to the model immediately; reconnect + retry only
happens for transport failures (`CONNECTION_CLOSED`, closed/broken streams, `OSError`).

#### MCP-05
**Exit stacks closed from a different task than they were opened in** — *Medium*

Each server's `AsyncExitStack` is entered inside the task that ran `MCPHost.start()`
(the GUI `_bootstrap` task) and closed from another task (`_shutdown`, or the turn
task that triggers `_reconnect`). The MCP SDK uses anyio task groups/cancel scopes,
which must be exited by the task that entered them → `RuntimeError: Attempted to exit
cancel scope in a different task …`, which was logged and ignored, leaving cleanup of
the child process to chance.

**Fix:** each server is owned by a dedicated long-lived task that enters the contexts,
signals readiness, waits for a stop event, and exits the contexts itself (the pattern
recommended for the MCP Python SDK / anyio).

#### MCP-06
**One leaked stderr temp file per server start** — *Low*

The stderr `NamedTemporaryFile(delete=False)` is unlinked right after a successful
handshake while the child process still holds the handle; on Windows `unlink` fails
(swallowed), so every start/reconnect leaves a `mcp-<name>-*.stderr.log` in `%TEMP%`
that the child keeps appending to. The stderr content (useful for debugging) is lost.

**Fix:** stderr goes to `~/.javis/logs/mcp-<name>.stderr.log` (truncated per start).

#### MCP-07
**MCP servers inherit the full process environment** — *Info*


#### MCP-08
**No handshake timeout; servers connected one after another** — *Medium*

`session.initialize()` had no timeout, and `MCPHost.start()` connected servers
sequentially inside the boot `gather`. One server that hangs during start-up (e.g. an
`npx` package prompt, a dead network share) blocked "Loading models & MCP servers"
forever — and with it `orch.start()` and the whole agent. **Fix:** 120 s handshake
timeout per server, all servers connected concurrently.

### Repository

#### SEC-01
**Git remote URL embeds a personal access token** — *High*

`git remote -v` shows `https://<user>:<token>@<gitlab host>/…`. The token is stored in
plain text in `.git/config` and is printed by every `git remote -v`, CI log or support
screenshot. (The secret-bearing `config.yaml`, `mcp_servers.json` and
`spotify-config.json` are correctly git-ignored and were never committed.)
**Action for the owner:** revoke/rotate the token in GitLab, then
`git remote set-url origin https://<gitlab host>/<group>/javis.git` and let Git
Credential Manager store the credential. This was not changed automatically because it
affects your push setup.
`env = {**os.environ, **spec.env}` (L231) passes e.g. `JAVIS_HERMES_API_KEY` to every
server. This matches VS Code's behaviour and many servers need `PATH`, `APPDATA`, etc.
Treat `mcp_servers.json` as trusted code; consider an allow-list if third-party servers
are added.

### Agent / orchestrator

#### AGT-01
**Quick commands bypass speaker privileges** — *High*

`_handle_turn` computes `_privileges` from speaker ID (L1270-L1320) but calls
`_try_quick_response(tr.text)` (L1333) without them; `_try_quick_response` executes
each action via `self.mcp.call(...)` directly (L2190-L2205), never consulting
`PrivilegeMiddleware`. A guest/unknown speaker whose ACL denies `homey__*` can still
run every learned Homey quick command. The quick-command repair loop also creates a
`TurnContext` without privileges, so it runs with full rights. (Remote/slave turns do
not use quick commands, so they were not affected.)

**Fix:** privileges are passed to `_try_quick_response`; if any action is not allowed
the quick path is skipped (the LLM path then enforces the ACL), and the repair turn
inherits the speaker's privileges.

#### AGT-02
**Repair directive pairs actions with the wrong errors** — *Medium*

L2220-L2223: `zip(cmd.actions, errors)` — `errors` contains only the *failed* actions,
so with actions `[A ok, B failed]` the directive claims `A → <B's error>`. The LLM is
told to repair the wrong step.

**Fix:** failures are collected as `(action, error)` pairs.

#### AGT-03
**`stop()` runs twice / concurrently** — *Medium*

`run()` calls `stop()` in its `finally` (L660), the GUI calls it again from
`_shutdown()`, and `run_headless()` calls it again in its own `finally`. In GUI mode
both run concurrently (the loop resumes the cancelled `run()` while
`run_until_complete(_shutdown())` executes), so MCP stacks, the mic and the HTTP
clients are torn down twice in parallel.

**Fix:** `stop()` is idempotent and serialised (`_stopped` flag + lock); the main
loop task is cancelled and awaited before teardown.

#### AGT-04
**State stuck in THINKING (with endless ticks) on early errors** — *Medium*

* `submit_text` (L703-L716): `_start_thinking_cue()`, `_try_quick_response()`,
  `conv.add_user()` and `run_start()` run *before* the `try/finally` that resets state.
  If any raises, the state stays THINKING and the cue task loops
  `while self.state == THINKING: tick; sleep(4)` — a soft tick every 4 s until another
  turn happens.
* `submit_remote_turn` (L785-L800): an STT failure leaves the master in THINKING.
* `_handle_turn` leaked the cue task the same way when STT raised.

**Fix:** the whole turn body is inside `try/finally` (cue cancelled, state → IDLE).

#### AGT-05
**Fire-and-forget tasks without strong references** — *Medium*

`asyncio.ensure_future(...)`/`create_task(...)` results were discarded in ~25 places
(chimes, TTS pre-warm, background quick-suggest, `profiler.refine`, GUI actions, and
the long-lived `orch.run()`, training scheduler and API server tasks). The event loop
keeps only weak references (see the Python docs warning on `create_task`), and an
exception in such a task is only reported at garbage collection as
"Task exception was never retrieved" — e.g. a crash

#### AGT-10
**One invalid stored regex quick command breaks every turn** — *Medium*

`QuickEngine.add/update` accepted any `match_mode` and any regex — including ones
written by the LLM through `javis__quick_add`. `match()` then ran `re.search()` on every
utterance; an invalid pattern raised `re.error` on **every** voice/text turn (error
phrase, nothing works) until the entry was deleted by hand. **Fix:** modes and regexes
are validated on add/update (the LLM gets an `ERROR:` result and can retry; the GUI
shows a warning), and a bad pattern already on disk is logged and skipped. of `orch.run()` would have been
silent.

**Fix:** new `javis.core.tasks.spawn()` keeps a strong reference until completion and
logs exceptions; used everywhere a task is not awaited.

#### AGT-06
**PCM writer thread keeps writing while the stream is closed** — *Medium*

`_speak`, `_speak_stream` and `play_pcm` write audio with
`await asyncio.to_thread(_write_pcm, …)`. When the awaiting task is cancelled (turn
error, shutdown, barge-in cleanup), `to_thread` returns immediately but the **thread
keeps running**, while the `finally` block calls `stream.abort()/stop()/close()`.
PortAudio does not support closing a stream that another thread is writing to → errors,
hangs in `stop()` or native crashes. The writer code was also triplicated.

**Fix:** a single `write_pcm_blocks()` helper; on cancellation it raises a stop flag
and waits (bounded) for the writer thread to exit before the stream is closed.

#### AGT-07
**Timer announcement can leave state SPEAKING** — *Low*

`_on_timer_fire` (L1875-L1888) calls `_speak()` and then `set_state(IDLE)` without
`try/finally`. **Fix:** `finally` resets the state.

#### AGT-08
**Privilege check fails open** — *Low*

`run_before_tool` (middleware.py L1718-L1735) logs and ignores exceptions from every
middleware. For `PrivilegeMiddleware` this means *allow* on error (e.g. a malformed
`allowed_tools` value in `index.json`). **Fix:** an exception in the privilege
middleware now denies the tool.

#### AGT-09
**Substring keyword routing over-includes tool sets** — *Info*

`_route_tools` uses `k in text` for single words: `hell` ⊂ *hello*, `play` ⊂ *display*,
`ton` ⊂ *Anton*, and `möchte`/`wollen` (Emby set) match most German requests, so Emby
schemas are attached to nearly every turn. Substring matching is deliberate for German
compounds (*Wohnzimmerlicht*), so this is left as a recommendation: drop generic verbs
(`möchte`, `wollen`, `what is`) from the sets, or match short words on word boundaries
and keep substring matching for nouns ≥ 5 chars.

#### ARCH-01
**Maintainability hot-spots** — *Info*

* `gui/main_window.py` (~5.3 k lines) defines the whole window class inside
  `run_gui()`, capturing `config` from the closure; it cannot be imported or tested in
  isolation. Recommended split: one module per tab + a `MainWindow` class taking
  `config` explicitly.
* `orchestrator.py` (~3.3 k lines) embeds ~300 lines of JSON tool schemas; `_speak`
  and `_speak_stream` duplicate the synth→play pipeline. Recommended: `agent/builtin_tools.py`
  and a `tts_pipeline.py` with one producer/consumer implementation fed by a queue.

### LLM

#### LLM-01
**Streaming errors swallowed** — *Medium*

`OllamaClient.chat_streaming` (L326-L362) catches every exception (connect failure,
read timeout, dropped connection), logs it at **DEBUG** and returns whatever was
received. When Ollama is down the result is an *empty* response, which `_llm_loop`
interprets as "model returned nothing" and answers with nudges/loop fallbacks instead
of reporting the outage. The two `GeneratorExit` branches are dead code (a coroutine
never sees `GeneratorExit` from `aiter_lines`, and `except Exception` cannot catch it).

**Fix:** errors are logged at WARNING; if nothing was received the exception is
re-raised, a partial reply is returned with a warning.

#### LLM-02
**History trimming can drop the in-progress turn** — *Low*

`Conversation._trim()` runs after every `add_*` call and drops whole turns from the
front until under `max_chars`. If the current turn alone exceeds the budget (large
system prompt + several 2 kB tool results), the *current* user message and the
assistant tool-call message are dropped, leaving orphaned `tool` messages and a model
that no longer sees the question. **Fix:** the newest user turn is never trimmed.

### Audio / STT

#### AUD-01
**`record_utterance` can block forever** — *Medium*

capture.py L106 iterates `mic.chunks()` without `stale_timeout`. If the input device
disappears mid-utterance (USB unplug, driver reset), no chunk ever arrives, the
coroutine never returns and `_turn_lock` stays held — PTT, typed input and timer
announcements are blocked until restart. **Fix:** a 5 s stale timeout ends the
recording with what was captured.

#### AUD-02
**Chunk size vs. Silero window** — *Low*

The mic delivers 30 ms (480 samples) chunks; `SileroVAD.process` zero-pads each to
512 samples. Silero v5/v6 accepts exactly 512 samples @ 16 kHz and keeps the last 64
samples as context for the next call, so every call's context is half zeros.
**Fix:** 32 ms chunks (512 samples) — exact Silero window; openWakeWord buffers to its
own 1280-sample frame so it is unaffected.

#### AUD-03
**Non-16 kHz sample rate silently breaks detection** — *Low*

`audio.sample_rate` is configurable, but `make_vad()` is always built for 16 kHz and
openWakeWord only supports 16 kHz. **Fix:** `validate()` forces 16000 with a warning.

#### AUD-04
**Mic callback can raise during shutdown** — *Low*

`_cb` checks `loop.is_closed()` and then calls `call_soon_threadsafe` from the
PortAudio thread; if the loop closes in between, `RuntimeError` propagates into
PortAudio. **Fix:** guarded.

### API (slave devices)

#### API-01
**Large bodies parsed before auth and size check** — *Medium*

FastAPI parses the multipart body (`UploadFile`) before running the `Depends` auth
dependency, and `/turn` reads the whole upload into memory before comparing it with
`max_upload_bytes` (routes.py L152-L160). Any host on the LAN can make the master
spool arbitrarily large bodies to disk/RAM. **Fix:** a small ASGI guard rejects
requests whose `Content-Length` exceeds the limit (413) and requires a length for
`/turn`, before any parsing.

#### API-02
**Pair-attempt table grows forever; decoder internals leaked** — *Low*

`_pair_attempts` keeps one entry per source IP forever; `/turn` echoed soundfile's
internal decoder message; `_require_device` uses `assert` (removed under `python -O`).
**Fix:** stale entries are pruned; decode errors return a generic message (details
stay in the log); explicit check instead of `assert`. Turn errors are still reported
to the (authenticated) device, truncated to 200 characters, because the satellite
shows them to the user.

#### API-03
**Plain-HTTP LAN API** — *Info*

Bearer API keys travel unencrypted and the server binds `0.0.0.0` when enabled. Keys
are hashed at rest and compared in constant time (good). Recommendation: restrict the
Windows firewall rule to the private profile/subnet, or terminate TLS in a reverse
proxy (Caddy) if the LAN is not trusted.

### GUI

#### GUI-01
**`NameError` when rendering voice-profile cards** — *Critical*

`_make_voice_card` (main_window.py L2355-L2398) uses `name` and `v` that are never
defined (ruff F821) — the card's `QVBoxLayout` and `name = profile["name"]` were lost.
As soon as one speaker profile exists, opening the Intelligence tab (which refreshes
on every activation) raises inside the Qt slot and the voice panel never renders.

**Fix:** restored the layout and the name lookup.

#### GUI-02
**Theme registry leaks every rebuilt card widget** — *Medium*

`_th()` appends `(widget, factory)` to `self._themed` forever. Memory, quick-command
and voice cards are rebuilt on every Intelligence-tab activation (≈5 entries per
card), so the list grows without bound and `retheme()` iterates thousands of deleted
widgets. **Fix:** dead entries are pruned (`shiboken6.isValid`) with an amortised
threshold.

#### GUI-04
**`cmd /k` command line built with `list2cmdline`** — *Medium*

Training/export are launched with
`Popen(["cmd", "/k", subprocess.list2cmdline(cmd)])` (L4036-L4042, L4102-L4108).
`list2cmdline` quotes for the MSVC runtime, not for `cmd.exe`: an argument without
spaces containing `&`, `|`, `^` (voice names from imported archives, the free-text
base-checkpoint path) is executed as an extra command; and the outer `Popen` list
re-escapes inner quotes (`\"`), which breaks interpreter paths with spaces.
**Fix:** every argument is double-quoted for `cmd.exe` (arguments containing `"` are
rejected) and the command string is passed verbatim.

#### GUI-05
**Background tasks not cancelled on quit** — *Low*

After `loop.run_forever()` returns, only `orch.stop()` is awaited; the training
scheduler and API server tasks stay pending → "Task was destroyed but it is pending"
and the uvicorn socket is never closed cleanly. **Fix:** tracked and cancelled.

### Voices / TTS

#### VOI-02
**Quitting during scheduled training hangs exit and frees the lock early** — *Medium*

The overnight scheduler runs `run_pipeline()` via `asyncio.to_thread`; the thread waits
on the training subprocess for hours. Cancelling the scheduler task (quit) released
the training lock immediately while the thread kept training, and at interpreter exit
Python joins executor threads — so Javis hung on quit until training finished.
**Fix:** cancellation calls `piper_train.request_stop()`: the training process finishes
its step, saves its progress — within the epoch too: the next run continues with the
next batch — and exits (it is killed if that takes longer than
`STOP_GRACE_SECONDS`), and the scheduler waits for that (`_run` polls instead of
blocking in `wait()`). The lock (`training/.training.lock`, held by `run_pipeline`)
names the training processes and is only released once they have exited.

#### VOI-01
**Imported voice archives may carry pickle checkpoints** — *Medium (trust)*

`.javisvoice` import (voices/io.py) correctly blocks zip-slip and oversize archives,
but it extracts `training/**` including PyTorch-Lightning `*.ckpt` files and
`base_checkpoint.txt`. Training/export later load those checkpoints (pickle-based) →
importing an archive from an untrusted source and pressing *Train* executes arbitrary
code. **Fix:** checkpoint/pickle files (`.ckpt`, `.pt`, `.pth`, `.pkl`) are skipped on
import unless explicitly allowed, with a log warning.

### Core / config / logging

#### CORE-01
**Log file date frozen at start; file I/O on the event loop** — *Low*

`log_dir / f"javis_{datetime.now():%Y%m%d}.log"` is evaluated once; a process running
for a week writes into the start-day file (size rotation only). File writes run
synchronously on the event-loop thread, and loguru's default `diagnose=True` prints
local variable values in tracebacks (e.g. dicts holding MCP server env tokens).
**Fix:** loguru's `{time:YYYYMMDD}` filename placeholder with a midnight-or-10 MB
rotation predicate, `enqueue=True` (writes on a background thread) and
`diagnose=False` on both sinks.

#### CORE-02
**Missing validation** — *Low*

`speaker_id.threshold` and `speaker_id.refine_margin` were not range-checked; a YAML
file whose root is not a mapping crashed with `AttributeError`. **Fix:** validated.

#### LINT-01
**Ruff findings** — *Low*

Unused imports (`AgentState`, `PAL_LIGHT`, `json`, `pathlib`, `csv`, `asyncio`,
`parse_qualified_name`), duplicate set literals (B033 in `_SLAVE_KEYWORDS`,
`_EMBY_KEYWORDS`, `_SEARCH_KEYWORDS`, `_RECALL_STOPWORDS`), unused loop variable,
f-string without placeholders, missing exception chaining. **Fix:** cleaned up.

### Docs / packaging

#### DOC-01
README describes Coqui **XTTS v2** and requires Python ≤ 3.11 "because of XTTS"; the
code ships **Piper** (default) and optional **F5-TTS**. **Fix:** README updated.

#### DOC-02
`mcp>=1.2` allowed a breaking major (see MCP-01); `pytest` was not installed so the
test suite could not run. **Fix:** `mcp>=1.2,<3`; `pip install -e .[dev]` documented.

---

## 4. Reviewed and rejected (false positives)

These were raised during the automated pass and rejected after checking the code:

| Claim | Why rejected |
|---|---|
| `core/events.py` is truncated | File is complete; `publish()` ends after the logging loop. |
| Global hotkey never unregistered | `run_gui` calls `hk.uninstall()` after the loop stops. |
| GUI quits without `orch.stop()` | `_shutdown()` is run with `run_until_complete`. (It ran *twice* — see AGT-03.) |
| Startup-dialog mic meter touches Qt from the audio thread | Callback only stores a float; the `QTimer` tick on the GUI thread updates the bar. |
| Timer restore schedules wrong times | Remaining time is recomputed from the absolute `fire_at`; correct. |
| `Fraction(...).limit_denominator(1000)` causes pitch shift | 44100→16000 = 160/441 exactly; common rates reduce exactly. |
| Wake-word download endpoint path traversal | Path comes from the owner's config, not the client. |
| `asyncio.set_event_loop()` deprecated | Not deprecated on 3.10–3.13; required by qasync. |
| `blocksize` counted in samples vs frames | sounddevice `blocksize` is frames; code is correct. |
| `_abort_early` is a "marker" spoken by accident | It is the intended spoken abort reason. |

---

## 5. Verification

| Check | Result |
|---|---|
| `ruff check src/javis --select F,E9,B` | 37 findings → 0 (B008 is the idiomatic FastAPI `Depends` pattern and is ignored) |
| `pytest tests` | 21 existing + 18 new regression tests ([tests/test_audit_fixes.py](../tests/test_audit_fixes.py)) — **39 passed** |
| Offscreen GUI smoke test ([tests/_retheme_check.py](../tests/_retheme_check.py)) | Window builds, theme round-trip OK, voice-profile card renders (GUI-01) |
| Live MCP smoke test (local `duckduckgo-mcp`) | 3 tools with full parameter schemas (MCP-01), bare server name rejected (MCP-03), clean stop from a different task (MCP-05), stderr log under `logs/` (MCP-06) |
| Logging | New file `javis_YYYYMMDD.log` created, enqueue sink flushes |

---

## 6. Improvements and best practices

Researched against primary documentation; ✅ = implemented in this pass,
📌 = recommendation.

| # | Topic | Best practice (source) | Javis status |
|---|---|---|---|
| 1 | Background tasks | "Save a reference to the result of [`create_task`] … The event loop only keeps weak references to tasks"; prefer `asyncio.TaskGroup` for structured concurrency ([Python docs – Creating tasks](https://docs.python.org/3/library/asyncio-task.html#creating-tasks)). | ✅ `core.tasks.spawn()` keeps references + logs failures. 📌 After moving to Python ≥ 3.11, run per-turn helpers (chimes, cue, producer) in a `TaskGroup` owned by the turn. |
| 2 | Cancellation hygiene | Use `try/finally` for clean-up and re-raise `CancelledError` ([Python docs – Task cancellation](https://docs.python.org/3/library/asyncio-task.html#task-cancellation)). | ✅ Turn bodies restructured (AGT-04), PCM writer waits for its thread (AGT-06), scheduler propagates cancellation (VOI-02). |
| 3 | MCP client lifecycle | "Let the `async with` block own the connection"; "Check `result.is_error` rather than expecting a failing tool to raise"; SDK ≥ 2.0 uses snake_case fields ([MCP – Build a client](https://modelcontextprotocol.io/docs/develop/build-client)). | ✅ Owner task per server (MCP-05), `is_error`/`input_schema` compat (MCP-01). 📌 Consider migrating to the SDK 2.x `Client` wrapper once 1.x support is dropped. |
| 4 | Tool permissions | "Be cautious with tool permissions … connect only to servers and executable commands you trust" (same source). | ✅ Allow-once is per turn (MCP-02), no guessed tools (MCP-03), privileges on quick commands (AGT-01), fail-closed ACL (AGT-08). |
| 5 | Streaming tool calls | Ollama streams content **and** tool calls since May 2025 with a new incremental tool parser ([Ollama blog – Streaming responses with tool calling](https://ollama.com/blog/streaming-tool)). | 📌 `_llm_loop` still uses `stream=false` whenever tools are attached (comment cites old Ollama behaviour). Switching tool rounds to `chat_streaming(tools=…)` would start TTS on the first sentence of tool-free answers, a noticeable latency win. Needs a test pass with the models you use. |
| 6 | Context window | Ollama notes that a context of 32 k or higher improves tool-calling quality ([same source](https://ollama.com/blog/streaming-tool)); `num_ctx` is a per-request option ([Ollama API – chat](https://docs.ollama.com/api/chat)). | 📌 Default `llm.num_ctx` is 16 384 while the comment in `config.py` recommends 32 768 for 20+ tools; `Conversation.max_chars` (24 k chars ≈ 6 k tokens) is independent of `num_ctx` — derive it from `num_ctx`. |
| 7 | `keep_alive` | Accepts a duration string (`"5m"`) or number; `0` unloads ([Ollama API – chat](https://docs.ollama.com/api/chat)). | ✅ Example config no longer suggests the invalid `"infinity"` (use `-1`). 📌 Allow duration strings in `LLMConfig.keep_alive`. |
| 8 | VAD framing | Silero VAD supports 8/16 kHz; chunks of 30+ ms ([silero-vad](https://github.com/snakers4/silero-vad)); the v5+ models take a fixed 512-sample window at 16 kHz. | ✅ 32 ms (512-sample) mic chunks (AUD-02); 16 kHz enforced (AUD-03). |
| 9 | Wake-word robustness | Feed multiples of 80 ms; enable the built-in Silero `vad_threshold` gate to cut false activations; tune the threshold per room; consider custom verifier models for known voices ([openWakeWord README](https://github.com/dscripka/openWakeWord)). | ✅ New opt-in `wake_word.vad_gate`. 📌 A verifier model trained on the household's voices would also complement speaker ID. Note: bundled openWakeWord models are CC BY-NC-SA. |
| 10 | STT vocabulary | faster-whisper accepts `hotwords` / `initial_prompt` (verified in the installed 1.2.1). | ✅ New opt-in `stt.hotwords` for names, rooms and device names. |
| 11 | Logging | `enqueue=True` makes logging non-blocking; set `diagnose=False` in production "to avoid leaking sensitive data" ([loguru API](https://loguru.readthedocs.io/en/stable/api/logger.html)). | ✅ Both applied (CORE-01). |
| 12 | LAN API | Reject oversized bodies before parsing; plain HTTP exposes bearer tokens. | ✅ Body-size guard (API-01). 📌 Scope the firewall rule to the private profile, or put Caddy/TLS in front (API-03). |
| 13 | Supply chain | Pin majors of fast-moving deps; use a lock file. | ✅ `mcp>=1.2,<3`. 📌 Add a lock (`uv lock` / `pip-compile`) and pin `fastapi`/`starlette`, `faster-whisper`, `openwakeword`. |
| 14 | CI | Lint + tests on every push would have caught GUI-01 (`ruff F821`) and MCP-01. | 📌 Add a GitLab CI job: `ruff check src/javis --select F,E9,B` + `pytest`. |
| 15 | Maintainability | Split `main_window.py` per tab and inject `config`; extract built-in tool schemas and a single TTS pipeline from `orchestrator.py` (ARCH-01). | 📌 |
| 16 | Tool routing | Drop generic verbs (`möchte`, `wollen`, `what is`) from the routing sets or match short words on word boundaries (AGT-09). | 📌 |

### New configuration options (all off by default)

```yaml
stt:
  hotwords: "Javis, Esszimmer, Homey"   # bias Whisper towards these words
wake_word:
  vad_gate: 0.5                          # wake only when openWakeWord's VAD agrees
  voice_gate_ratio: 3.0                  # end recording when the *user* stops, not the music
```