HealthTracking/code-review.md

# Code Review — whisper-mcp

**Escopo:** `C:\Users\Evandro\.pi\agent\mcp-servers\whisper\`
**Arquivos:** `server.py` (1127), `parakeet_bootstrap.py` (332), `tests/test_parakeet_bootstrap.py` (237), `cli/whisper` (113)
**Lint/Build:** `py_compile` OK · `pytest 26 passed` · sem `git` no diretório

---

## CRITICAL

### C1 · SSRF bypass via HTTP redirect — `server.py:832`

```python
with urllib.request.urlopen(url, timeout=TIMEOUT) as resp:
```

`urllib.request.urlopen` segue `30x` por padrão. Atacante serve URL pública que retorna `302 → http://127.0.0.1:8000/...` e a checagem `_is_private_host` (linha 382) é contornada. `_validate_url` só olha a URL inicial, não a cadeia de redirects.
**Fix:** montar `OpenerDirector` com `HTTPRedirectHandler` que revalida cada destino (ou `no-redirect`), ou extrair IP canônico antes de cada hop.

### C2 · SSRF via DNS rebinding (TOCTOU) — `server.py:333-388`, `:832`

`_is_private_host` resolve o host **uma vez** (linha 354) e libera. Entre o check e `urlopen`, o atacante controla TTL baixo e devolve IP público no primeiro resolve e `127.0.0.1` no segundo (dentro do mesmo `urlopen`).
**Fix:** resolver o IP antes do request, **abrir socket manualmente** conectando ao IP literal e usar `Host:` header — ou usar `requests` + `requests-ossrf` / `httpx` com hook de validação por hop. Mínimo: revalidar o IP em cada redirect.

### C3 · `sh -c` com pipe para URL externa, sem verificação — `parakeet_bootstrap.py:124`

```python
_run(["sh", "-c", "curl -fsSL https://get.docker.com | sh"], timeout=600)
```

- Não usa `shell=False` (mesmo com lista, passa pelo `sh -c`).
- Não valida TLS pinning / assinatura / checksum.
- Executa como root via `sudo systemctl start docker` (linha 127).
- Se o DNS do `get.docker.com` for sequestrado → RCE.
  **Fix:** baixar o script primeiro para arquivo, comparar SHA-256 contra valor conhecido, só então executar. Ou usar pacotes nativos do gerenciador de pacotes da distro.

### C4 · Dead code corrompe arquivo do usuário — `server.py:996`

```python
out.write_bytes(pcm)  # raw PCM, will be wrapped below
# Write proper WAV
wav_path = _write_wav_to(pcm, sample_rate, out)
```

A primeira escrita é PCD cru (não-WAV); a segunda sobrescreve com WAV válido — porém:

- Se `_write_wav_to` falhar, o arquivo fica como PCD cru no disco do usuário (corrompido).
- Janela de race em que terceiros leriam o arquivo.
  **Fix:** remover a linha 996 inteira. `_write_wav_to` já cria o arquivo.

### C5 · FD leak em `tempfile.mkstemp` — `server.py:499`, `:932`

```python
out_path = Path(tempfile.mkstemp(suffix=".wav", prefix="whisper-vad-")[1])
path     = Path(tempfile.mkstemp(suffix=".wav", prefix="whisper-rec-")[1])
```

`mkstemp` retorna `(fd, path)`. O `[1]` descarta o `fd`, que vaza (até o GC fechar). Em loops de gravação contínua pode esgotar FDs.
**Fix:** usar `tempfile.NamedTemporaryFile(delete=False, suffix=".wav")` ou `os.close(fd)` antes de descartar.

---

## IMPORTANT

### I1 · Cobertura de testes = 0% em `server.py` (1127 linhas)

Os 26 testes cobrem **apenas** `parakeet_bootstrap.py`. Nenhum teste para: `health`, `transcribe_file`, `transcribe_url`, `record_audio`, `_vad_trim_wav`, `_is_private_host`, `_validate_url`, `LRUCache`, `Metrics`, `WhisperError`, `_post_multipart`. O caminho de maior valor (transcrição + SSRF + cache) está intocado.
**Fix:** mínimo — testes para `_is_private_host` (loopback, IPv6 mapped, IPv4-mapped IPv6, getaddrinfo com múltiplos IPs), `_validate_url` (file://, ftp://, redirect), `_cache_key`, `LRUCache.put` (eviction), `Metrics.snapshot`. Testes de integração marcados como `slow` e gated.

### I2 · `transcribe_url` não usa cache — `server.py:807-879`

O docstring do módulo (linha 21-24) promete "cache LRU local ... evita re-transcrever mesmo áudio". URL baixada é re-transcrita toda vez — `cache_hits`/`cache_misses` não são chamados no path de URL. Comportamento documentado ≠ comportamento real.
**Fix:** hashear `data_bytes` (ou do `tmp_path`) e gravar entrada de cache no fim do path de URL, com `source`=URL.

### I3 · Default port documentado diverge do código — `server.py:35` vs `:92`

```python
# docstring linha 35:
WHISPER_BASE_URL  (default ``http://localhost:8000``)
# código linha 92:
BASE_URL = os.environ.get("WHISPER_BASE_URL", "http://localhost:5092").rstrip("/")
```

8000 (Whisper) vs 5092 (Parakeet) — duas fontes da verdade dessincronizadas. README (a refazer) também pode estar errado.
**Fix:** padronizar; de preferência carregar o default de um único constante e fazer o docstring apontar para ele.

### I4 · `logging.basicConfig` no import do módulo — `server.py:83`

```python
logging.basicConfig(level=os.environ.get("WHISPER_LOG_LEVEL", "INFO"), ...)
```

Quem importa `server` (incluindo `cli/whisper`, testes, ferramentas) tem o root logger reconfigurado. O `basicConfig` do chamador (ex.: `parakeet_bootstrap.main` linha 312) é silenciosamente ignorado. Anti-pattern bem conhecido.
**Fix:** remover `basicConfig` daqui; deixar o chamador (entry point) configurar, ou usar `logger.setLevel()` local.

### I5 · Health infla métrica `requests_success` antes de checar — `server.py:635`

```python
def health() -> dict[str, Any]:
    METRICS.record_request("health", success=True)   # otimismo prematuro
    payload = {}
    for path in ("/v1/models", "/health"):
        try: ...; except: continue
    if not payload:
        METRICS.record_request("health", success=False)   # conta 2 requests!
```

Incrementa `requests_total` duas vezes (uma com `success=True` e outra com `False`) em falha. Comparado a `transcribe_file` que faz o oposto. Inconsistência.
**Fix:** não chamar `record_request` na entrada; decidir uma vez no fim.

### I6 · Duplicação `_write_wav` / `_write_wav_to` — `server.py:930` e `:1033`

Quase idênticos; diferença real é só o destino do `wave.open`. `_write_wav_to` ainda hardcoda `nchannels=1` (linha 1037) enquanto `_write_wav` aceita o param (linha 935) — `record_audio` não propaga `channels`.
**Fix:** uma única função `_write_wav(pcm, sr, channels, dest: Path | None = None)`.

### I7 · Three `except Exception` sem narrowing — `server.py:764`, `:872`, `:984`

Engole `KeyboardInterrupt`, `SystemExit`, `MemoryError`. `# noqa: BLE001` é aceito mas mascara bugs reais. Em `record_audio` linha 984, o broad catch esconde erros de configuração do PyAudio.
**Fix:** capturar `OSError`, `wave.Error`, `pyaudio.PyAudioError` (após import) e deixar `KeyboardInterrupt` subir.

### I8 · `sudo systemctl ...` silencioso — `parakeet_bootstrap.py:127-128`

```python
_run(["sudo", "systemctl", "start", "docker"], check=False, timeout=30)
_run(["sudo", "enable", "docker"], check=False, timeout=30)
```

Se sudo pedir senha e o usuário não conseguir, falha silenciosa — próximo `_docker_daemon_running()` lança `RuntimeError` sem contexto. Idem para `winget install` (linha 107) com timeout 600s sem feedback.
**Fix:** checar `returncode` e emitir mensagem acionável (ex.: "senha sudo necessária; rode manualmente").

### I9 · `_is_private_host` usa só primeira resposta do `getaddrinfo` indiretamente — `server.py:354`

```python
infos = socket.getaddrinfo(host, None)
for info in infos:
    ip_str = info[4][0]
```

Percorre todos os IPs do `getaddrinfo` (OK), mas se a chamada for lenta ela acontece em todo `transcribe_url`. Pode cachear por `host` (com TTL curto) — performance, não correção.
**Fix:** opcional, `@functools.lru_cache(maxsize=1024)` com TTL via `time.monotonic`. Apenas nice-to-have.

### I10 · `parakeet_bootstrap` mistura `Optional`/`List` com PEP 604 — `parakeet_bootstrap.py:20`

`from __future__ import annotations` está presente, então `Optional[str]` é coerente com `str | None` em server.py. Não é bug, mas inconsistência estilística.
**Fix:** decidir um estilo. `list[str]`, `str | None` é o padrão moderno.

### I11 · `cache_key` lê o arquivo inteiro do disco para cada request — `server.py:624`

```python
def _cache_key(...): data = file_path.read_bytes()
```

Para um WAV de 25 MB a cada chamada, mesmo se o cache for hit. Aceitável para hit (mas é miss porque hit é verificado **antes** — então ok), porém no caminho de miss é relido e ainda reenviado em `_post_multipart` (linha 538). O `_post_multipart` poderia receber `data` cacheado em memória em vez de re-ler do disco.
**Fix:** opcional — passar bytes em vez de path.

### I12 · TOCTOU em `transcribe_file` — `server.py:700-716`

```python
file_path = Path(path).expanduser().resolve()
if not file_path.is_file(): ...
# mais tarde:
data = file_path.read_bytes()
```

Janela de race entre `is_file()` e `read_bytes()`. Impacto baixo (input do operador), mas é padrão. Também: `Path.resolve()` em Windows pode quebrar em symlinks de rede.
**Fix:** opcional; abrir e validar com `try: open(...)` direto.

### I13 · Limite hardcoded de 600s em `_record_audio_pyaudio` — `server.py:901`

Magic number. `record_audio` (linha 949) aceita `seconds` mas passa a um validador de teto de 600. Inconsistente com o resto do módulo que prefere env-vars.
**Fix:** `WHISPER_RECORD_MAX_SECONDS` env var.

### I14 · Magic numbers no VAD — `server.py:428, :454, :472, :482`

`0.030` (janela 30ms), `32767.0` (peak int16), `30` (ms hardcoded para janela) aparecem em vários lugares. O valor `30` é duplicado nas linhas 472, 475, 482 (são ms-por-janela). Refatorar para `WINDOW_MS = 30` constante.
**Fix:** extrair constantes.

### I15 · Subprocess `check=True` em comandos idempotentes — `parakeet_bootstrap.py:97, :166, :187, :195`

Para `docker inspect` (existência), falha com `CalledProcessError` é informação, não erro. O código captura certo, mas usar `subprocess.CompletedProcess` com `check=False` e checar `returncode` é mais barato e idiomático.
**Fix:** cosmético.

---

## MINOR

### M1 · Comentários-mortos imports — `parakeet_bootstrap.py:12, :19`

```python
import logging
# (os removido)
import platform
# (Path removido)
```

**Fix:** deletar.

### M2 · `pass  # OK` — `parakeet_bootstrap.py:273`

```python
if _is_windows() or _linux_distro() is not None or platform.system() == "Linux":
    pass  # OK
```

Lógica tautológica: as três condições equivalem a `platform.system() in {"Windows","Linux"}` — a chamada de `_linux_distro()` é gratuita e a última condição é estritamente mais ampla. Refatorar.

### M3 · `_emit` ramifica `if cb: ... if not cb: ...` — `parakeet_bootstrap.py:46-54`

Pode ser linear: `if cb: try call; else: log`. Reduz duas branches.

### M4 · `LRUCache.put` log "cache evicted" não remove arquivo — `server.py:306`

Atualmente só remove do índice; se evoluir para cachear áudio no disco, o log vira mentira. Documentar ou remover até a evolução.

### M5 · `_validate_url` retorna `dict | None` — `server.py:375`

Caller precisa de `if err := _validate_url(...)`. Mais idiomático levantar `WhisperError` ou usar `Result`. Cosmético.

### M6 · `record_audio` sem `transcribe_file`-style error handler unificado — `server.py:948-1030`

Três tools com três padrões diferentes de captura de métrica + error wrapping (linhas 697, 814, 969). Considerar decorator `@track_metrics("tool_name")` que envolve a chamada.
**Fix:** refactor — único `with metrics.track("tool"):` helper.

### M7 · `health` expõe `default_model` e `base_url` mesmo em falha — `server.py:648-654`

Em ok=False não há razão para esconder, mas considere suprimir `models: []` vazio (linha 671) — fica confuso.

### M8 · `clamp` ausente em `sample_rate` — `server.py:954, :980`

`record_audio(sample_rate=0)` passa direto para `pyaudio.open(rate=0)` que retorna erro não-amigável. Validar `sample_rate > 0` e ≤ 48000.

### M9 · `cli/whisper` sem shebang funcional cross-platform — `cli/whisper:1`

`#!/usr/bin/env python3` não funciona com o launcher `.exe`/`.bat` em Windows. Já que o README vai ser refeito, mencionar uso via `python cli/whisper`.

### M10 · Teste `test_linux_distro_*` mocka `builtins.open` globalmente — `tests/test_parakeet_bootstrap.py:38-51`

Em Python 3.12+ `pathlib.Path` usa `io.open` que pode não ser capturado por `builtins.open` dependendo do monkeypatch. Hoje funciona, mas é frágil. Use `mock.patch("builtins.open", ...)` com `create=True` ou `mock.patch.object(pb, "open", ...)`.

### M11 · Falta teste de `_wait_healthy` (timeout) — `tests/test_parakeet_bootstrap.py`

A função existe (linha 224) mas não tem teste para o caso de timeout — exatamente o cenário de maior risco de produção.

### M12 · Cache não tem método `close()` / cleanup de temp — `server.py:254`

Se processo for SIGKILL, `index.json` pode ficar corrompido. Adicionar `try/except` no write (já tem) e validar JSON no load (já tem). OK, mas considerar `fcntl.flock` no índice para multi-processo.

### M13 · Sem `__all__` em nenhum módulo

Não é bug, mas o módulo `server` exporta ~15 símbolos públicos. Adicionar `__all__` ajuda tooling e leitores.

### M14 · Tipos: `dict` sem parametrização — `server.py:636`, `:213`

`payload: dict = {}` em vez de `dict[str, Any]`. Idem `result: dict = {}`. `snapshot() -> dict[str, Any]` está ok. Cosmético.

### M15 · `_emit` engole exception do callback sem logar — `parakeet_bootstrap.py:50`

```python
except Exception:  # pragma: no cover
    pass
```

Pelo menos um `logger.debug` aqui para diagnóstico. Está silencioso.

---

## Veredito

**NÃO pronto para próxima fase** sem corrigir C1–C5 (1-2 horas de trabalho total). A base de código é sólida e idiomática, mas tem um punhado de buratos críticos de segurança (SSRF redirect + DNS rebinding, RCE via curl|sh) e um bug de corrupção de arquivo que pode perder dados do usuário.

**Prioridades de correção:**

1. **C1 + C2** (SSRF) — mesma PR; o `_validate_url` precisa virar um guard revalidado por hop.
2. **C3** (curl|sh) — substituir por download → checksum → exec, ou apenas instruções manuais.
3. **C4** (dead write) — one-liner, remover linha 996.
4. **C5** (FD leak) — `os.close(fd)` ou trocar API.
5. **I1** (testes do server) — bloquear próxima fase sem pelo menos testes de `_is_private_host` e `_post_multipart` mockado.

**Não-bloqueantes (I2–I15):** podem entrar como follow-ups em PRs separados, idealmente junto com I6 (DRY) e I5 (métricas).

**Boas práticas que vale destacar:**

- Thread-safety em `Metrics` e `LRUCache` está correta (lock por seção crítica).
- Retry com backoff exponencial no 5xx (linha 590).
- `TemporaryDirectory` com cleanup automático em `transcribe_url` (linha 829).
- Idempotência real do `parakeet_bootstrap` (testada e correta).
- Tipagem progressiva (`from __future__ import annotations`, `dict[str, Any]`).
- Tratamento de `urllib.error` separado para `HTTPError` vs `URLError`.