test: add evaluation coverage and pre-commit tooling - #1033
Conversation
…ption Whisper's encoder only accepts 30s of audio per pass (3000 mel frames). Feeding longer audio in a single processor+generate call silently transcribed just the first 30 seconds. Repro: a 6:43 FLAC produced only the first two sentences. - Split input PCM into 30s chunks at 16kHz and join decoded segments - Force return_timestamps=True: without it Whisper intermittently emits an early <|endoftext|> after the first sentence of a window, truncating multi-sentence chunks (observed on medium and turbo) - Load model weights in bfloat16 to halve VRAM on modern GPUs (avoids float16 c10::Half dtype bug on RTX 50-series) - Move inputs to model dtype to prevent float/bias dtype mismatch Tests: backend/tests/test_whisper_chunking.py covers single-chunk audio, multi-chunk joining, the timestamps flag and language forcing (mocked, no GPU needed).
Two GPU-stability fixes on 8 GB cards (e.g. RTX 5060 Laptop): - After each generation, unload_all_models() (TTS+STT+LLM + empty CUDA cache) instead of only the engine used. Whisper stayed resident and chained tada generations died with CUDA OOM (7.4/7.5 GiB in use). - tada degenerates on long inputs (produces 1s of audio or hallucinated text). GenerationRequest defaults max_chunk_chars=800, so effective_max_chunk_chars() caps tada at 250 chars per chunk unless the caller explicitly requests smaller. Repro: 650-char Spanish paragraph via /speak engine=tada returned a 1.0s clip containing only 'y'; with the cap it yields full 33s speech. Tests: backend/tests/test_tada_chunk_cap.py covers the cap, the 800 default bypass and per-engine passthrough.
Add regression coverage for model unloading and MCP mount paths, opt-in GPU/E2E evaluation harnesses, and Husky pre-commit checks for the desktop repository. Generated with Codebuff 🤖 Co-Authored-By: Codebuff <noreply@codebuff.com>
📝 WalkthroughWalkthroughThe pull request adds backend model cleanup, TADA chunk limits, Whisper long-audio chunking, MCP slash compatibility, test fixtures, evaluation harnesses, developer tooling, and Linux WebKit initialization. ChangesBackend reliability and evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/backends/pytorch_backend.py`:
- Around line 376-403: Serialize model lifecycle operations with transcription
by adding or reusing one shared guard around model loading, unloading, and the
entire chunk-inference loop. Update the transcription path to retain stable
model and processor references while the guard is held, and make unload_model()
wait for active transcriptions to complete before clearing them.
- Around line 302-305: Update _load_model_sync to query _get_device before
loading Whisper and resolve a supported torch dtype per device instead of
hard-coding torch.bfloat16. Add a dedicated device-specific dtype resolver with
a tested Whisper fallback, ensuring DirectML, XPU, CPU, and fallback CPU avoid
unsupported bfloat16 while preserving bfloat16 only where supported.
In `@backend/mcp_server/server.py`:
- Around line 41-47: Update the bare-mount normalization in the HTTP scope
handling to rewrite only scope["path"] to "/" while preserving the original
scope["raw_path"] value. Remove the raw_path assignment from this block and
leave the surrounding scope-copy behavior unchanged.
In `@backend/services/generation.py`:
- Around line 331-339: Move TTS model cleanup out of the setup `finally` and
into a `finally` enclosing synchronous inference and WAV encoding, so cleanup
occurs only after `generate_chunked()` completes. Reuse the local `tts_model`
instance for the loaded check and unload operation, while keeping
`bg_db.close()` in the existing setup cleanup. Add a regression test covering
the synchronous persist=false speech path and confirming inference runs before
unloading.
In `@backend/tests/conftest.py`:
- Around line 49-56: Update the _read helper to catch FileNotFoundError and
subprocess.CalledProcessError from subprocess.run, then call pytest.skip with a
clear message when nvidia-smi is unavailable or fails. Preserve the existing
parsing and integer return behavior when the command succeeds.
- Around line 117-127: Update the exception handling around _wait_for_health in
live_backend so that failures after the backend has been spawned are reported as
test failures rather than skipped. Preserve pytest.skip only for the
subprocess.Popen precondition failure, while retaining the existing process
cleanup and server-log-tail diagnostics for health-check errors.
In `@backend/tests/fixtures/generate_fixtures.sh`:
- Around line 73-80: Update the fixture generation flow around the long `ffmpeg`
invocation for `cv_6m43s.wav` to validate the generated WAV duration is at least
403 seconds after encoding. Make the script exit nonzero when the duration is
shorter, using the repository’s available media-inspection tooling, while
preserving the existing 10-second and 30-second fixture generation.
In `@docs/evaluation/whisper-chunking.md`:
- Around line 30-32: Update the WER verification description in
whisper-chunking.md to accurately describe
backend/tests/test_whisper_long_audio_e2e.py as a transcription smoke test,
unless the test is enhanced with reference text and an explicit WER threshold.
Do not claim WER quality-matrix validation while the test only checks non-empty
transcription output and audio duration.
In `@justfile`:
- Around line 337-352: Update the initial pytest marker expression in the
eval-local recipe to exclude gpu tests alongside e2e and slow tests. Keep the
separate GPU=1 conditional suite unchanged so GPU tests run only through that
gated path and are not executed twice.
In `@scripts/test_dictate_e2e.sh`:
- Around line 157-172: Update the test harness around run_dictate to create and
expose a controlled pw-record stub before the host binaries in PATH, matching
the existing arecord stub’s SIGTERM handling and WAV-output behavior. Adjust
recorder-related assertions to accept whichever recorder, arecord or pw-record,
the installed dictate script selects while preserving controlled failure and
output checks.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07285b11-63ae-482a-bd24-429e834546e4
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.husky/pre-commit.lintstagedrc.mcp.json.prettierrcCHANGELOG.mdbackend/app.pybackend/backends/__init__.pybackend/backends/pytorch_backend.pybackend/mcp_server/README.mdbackend/mcp_server/server.pybackend/services/generation.pybackend/tests/conftest.pybackend/tests/fixtures/README.mdbackend/tests/fixtures/generate_fixtures.shbackend/tests/test_fixture_contract.pybackend/tests/test_generation_unload.pybackend/tests/test_mcp_mount_slashes.pybackend/tests/test_tada_chunk_cap.pybackend/tests/test_tts_vram_churn.pybackend/tests/test_whisper_chunking.pybackend/tests/test_whisper_long_audio_e2e.pydocs/evaluation/TEMPLATE.mddocs/evaluation/appimage-backend-detection.mddocs/evaluation/local-setup.mddocs/evaluation/mcp-trailing-slash.mddocs/evaluation/tada-chunk-cap.mddocs/evaluation/vram-unload.mddocs/evaluation/whisper-chunking.mdjustfilepackage.jsonscripts/test_backend_detection.shscripts/test_dictate_e2e.shtauri/src-tauri/src/main.rs
| self.model = WhisperForConditionalGeneration.from_pretrained( | ||
| model_name, | ||
| torch_dtype=torch.bfloat16, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the official PyTorch and torch-directml documentation for the versions used by this repository, which of CUDA, MPS, DirectML, CPU, and XPU support loading and running Whisper in torch.bfloat16?
💡 Result:
Support for loading and running the Whisper model in torch.bfloat16 varies by device, depending on hardware capabilities and PyTorch backend implementation. CUDA: Support depends on the underlying GPU architecture. NVIDIA GPUs with Compute Capability 8.0 or higher (e.g., Ampere architecture and newer) support bfloat16 natively. While PyTorch's torch.bfloat16 can be used on these devices, Whisper's specific implementation (as provided by openai-whisper) is primarily optimized for float16 inference [1], and using bfloat16 may require manual conversion and handling of input features to match the model's dtype [2]. CPU: PyTorch supports bfloat16 on CPUs, particularly on Intel Xeon processors with AVX-512_BF16 or AMX support [3]. There is community-driven support to enable Whisper with bfloat16 on Intel hardware [4]. MPS (Metal Performance Shaders): PyTorch support for bfloat16 on MPS is limited and generally requires newer macOS versions (e.g., macOS 14.0+) [5]. While some operations might function, it is not broadly supported for full model execution for all architectures. DirectML: The torch-directml backend does not natively support bfloat16 for most operations [6][7]. It is primarily focused on supporting standard operators in float32 and float16 (where available), and users frequently encounter issues when attempting to use specialized dtypes or autocasting with this backend [8][6]. XPU: PyTorch provides explicit support for bfloat16 on XPU devices [9], leveraging Intel's hardware acceleration for these types [3]. Summary Table Device | bfloat16 Support | Notes --- | --- | --- CUDA | Yes | Requires Ampere (CC 8.0) or newer. CPU | Yes | Requires instruction set support (e.g., AVX-512_BF16/AMX). MPS | Limited | Requires macOS 14.0+; support is platform-dependent. DirectML | No | Generally not supported by the current torch-directml implementation. XPU | Yes | Supported via Intel extensions/PyTorch XPU. For all devices, note that Whisper's default inference code is designed for float32 or float16; running in bfloat16 often necessitates manual casting of the model and all input tensors to ensure compatibility [2].
Citations:
- 1: Shouldn't FP16 = True give faster transcription time ? openai/whisper#622
- 2: https://huggingface.co/openai/whisper-large-v3/discussions/42
- 3: https://pytorch.org/blog/empowering-pytorch-on-intel-xeon-scalable-processors-with-bfloat16/
- 4: added support for bfloat16 datatype to be used for Intel Xeon processors openai/whisper#1605
- 5: https://github.com/pytorch/pytorch/blob/79bdafe5/torch/_dynamo/device_interface.py
- 6: torch-directml: Is autocast unavailable on DirectML devices? microsoft/DirectML#454
- 7: https://pypi.org/project/torch-directml/0.2.3.dev240715/
- 8: Running with DirectML gives "RuntimeError: The parameter is incorrect." cubiq/ComfyUI_IPAdapter_plus#10
- 9: https://docs.pytorch.org/docs/stable/generated/torch.xpu.is_bf16_supported.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^backend/backends/pytorch_backend\.py$|requirements|pyproject|setup|poetry|environment)' || true
echo
echo "== pytorch_backend outline =="
ast-grep outline backend/backends/pytorch_backend.py --view compact || true
echo
echo "== relevant lines 260-330 =="
sed -n '260,330p' backend/backends/pytorch_backend.py | nl -ba -v260
echo
echo "== device/get dtype references =="
rg -n "from_pretrained|torch_dtype|_get_device|get_device|device|bfloat16|torch\.|mps|cuda|xpu|directml|DirectML" backend/backends/pytorch_backend.py backend -S
echo
echo "== package version hints =="
for f in $(git ls-files | rg '(^|/)(requirements.*\.txt|pyproject\.toml|setup\.py|setup\.cfg|Pipfile|poetry\.lock|environment\.ya?ml)$'); do
echo "--- $f"
rg -n "torch|transformers|whisper|huggingface_hub|directml|xpu|mkl|xla" "$f" -i || true
doneRepository: jamiepine/voicebox
Length of output: 719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant pytorch_backend lines =="
awk '{printf "%6d\t%s\n", NR, $0}' backend/backends/pytorch_backend.py | sed -n '240,335p'
echo
echo "== device/dtype/import references in pytorch_backend.py =="
rg -n "from_pretrained|torch_dtype|_get_device|get_device|device_id|get_device_id|device|xpu|mps|cuda|DirectML|directml|torch\b" backend/backends/pytorch_backend.py -S
echo
echo "== device implementation candidates =="
rg -n "class .*Device|def _get_device|def get_device|device_id|device_id" backend/backends/pytorch_backend.py backend/services backend -S
echo
echo "== dependency version hints =="
for f in backend/pyproject.toml backend/requirements.txt requirements.txt; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n "torch|transformers|openai-whisper|huggingface_hub|directml|xpu|xla" "$f" -i || true
fi
doneRepository: jamiepine/voicebox
Length of output: 9508
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== utils containing get_torch_device =="
rg -n "def get_torch_device|allow_xpu|allow_directml|mps|mkl|torch_directml|is_bf16_supported|torch\.bfloat16" backend -S
echo
echo "== file containing get_torch_device with line numbers =="
f="$(rg -l "def get_torch_device" backend || true)"
if [ -n "${f:-}" ]; then
awk '{printf "%6d\t%s\n", NR, $0}' "$f" | sed -n '1,220p'
fiRepository: jamiepine/voicebox
Length of output: 17141
Query the device before requesting torch.bfloat16.
_get_device() can return DirectML, XPU, CPU, or fallback CPU when MPS is unavailable, but _load_model_sync() hard-codes torch_dtype=torch.bfloat16. Add a device-specific dtype resolver with a tested fallback for Whisper, especially for DirectML where torch.bfloat16 is not generally supported by the backend.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/backends/pytorch_backend.py` around lines 302 - 305, Update
_load_model_sync to query _get_device before loading Whisper and resolve a
supported torch dtype per device instead of hard-coding torch.bfloat16. Add a
dedicated device-specific dtype resolver with a tested Whisper fallback,
ensuring DirectML, XPU, CPU, and fallback CPU avoid unsupported bfloat16 while
preserving bfloat16 only where supported.
| transcriptions = [] | ||
| for chunk in chunks: | ||
| # Process audio | ||
| inputs = self.processor( | ||
| chunk, | ||
| sampling_rate=16000, | ||
| return_tensors="pt", | ||
| ) | ||
| # Mover al device y convertir al mismo dtype que el modelo para | ||
| # evitar el error "Input type (float) and bias type (...) should be | ||
| # the same" en GPUs como la RTX 5060. | ||
| inputs = inputs.to(device=self.device, dtype=self.model.dtype) | ||
|
|
||
| with torch.no_grad(): | ||
| predicted_ids = self.model.generate( | ||
| inputs["input_features"], | ||
| **generate_kwargs, | ||
| ) | ||
|
|
||
| # Decode | ||
| transcriptions.append( | ||
| self.processor.batch_decode( | ||
| predicted_ids, | ||
| skip_special_tokens=True, | ||
| )[0] | ||
| ) | ||
|
|
||
| # Decode | ||
| transcription = self.processor.batch_decode( | ||
| predicted_ids, | ||
| skip_special_tokens=True, | ||
| )[0] | ||
|
|
||
| return transcription.strip() | ||
| return " ".join(t.strip() for t in transcriptions).strip() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Serialize model unload with active transcription.
The loop repeatedly reads self.model and self.processor without retaining stable references or preventing unload_model(). backend/services/generation.py unloads all models after generation. If it overlaps a long transcription, a later chunk can access None and fail. The new multi-chunk path increases this exposure window.
Protect model load, unload, and inference with one shared lifecycle guard. Make unload_model() wait until active transcriptions finish.
Based on learnings: serialize model swap with in-flight operations because reload/unload can race inference references.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/backends/pytorch_backend.py` around lines 376 - 403, Serialize model
lifecycle operations with transcription by adding or reusing one shared guard
around model loading, unloading, and the entire chunk-inference loop. Update the
transcription path to retain stable model and processor references while the
guard is held, and make unload_model() wait for active transcriptions to
complete before clearing them.
Source: Learnings
| if scope["type"] == "http" and scope.get("path") == "": | ||
| scope = dict(scope) | ||
| scope["path"] = "/" | ||
| # Keep path and raw_path coherent for ASGI routers that inspect | ||
| # both fields (Starlette normally supplies raw_path from the | ||
| # original request, e.g. b"/mcp"). | ||
| scope["raw_path"] = b"/" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
if [ -f backend/mcp_server/server.py ]; then
cat -n backend/mcp_server/server.py | sed -n '1,120p'
else
echo "backend/mcp_server/server.py not found"
fi
echo
echo "== ASGI path/raw_path usages =="
rg -n --hidden --glob '!*.pyc' 'raw_path|scope\["path"\]|scope\["raw_path"\]|\["raw_path"\]' backend || true
echo
echo "== repository dependencies mentioning starlette/fastmcp/ASGI (if present) =="
for f in pyproject.toml backend/pyproject.toml requirements.txt backend/requirements.txt uv.lock backend/uv.lock poetry.lock backend/poetry.lock; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "starlette|fastmcp|asgi" "$f" || true
fi
done
echo
echo "== current git status/diff summary =="
git diff --stat || true
git diff -- backend/mcp_server/server.py | sed -n '1,220p' || trueRepository: jamiepine/voicebox
Length of output: 5976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import pathlib
import re
path = pathlib.Path("backend/mcp_server/server.py")
if not path.exists():
print("backend/mcp_server/server.py missing")
raise SystemExit
text = path.read_text()
m = re.search(r'def MountRootSlashRewrite\(app\):.*?(?=\ndef |\Z)', text, re.S)
print("MountRootSlashRewrite function present:", bool(m))
if m:
body = m.group(0)
print("contains scope copy:", bool(re.search(r'scope\s*=\s*dict\(scope\)', body)))
print("contains path rewrite:", bool(re.search(r'scope\["path"\]\s*=\s*"/"', body)))
print("contains raw_path rewrite:", bool(re.search(r'scope\["raw_path"\]\s*=\s*b"/"', body)))
# Deterministic probe of the exact transformation for a bare mounted request.
initial = {"type": "http", "path": "", "raw_path": b"/mcp"}
if initial["type"] == "http" and initial.get("path") == "":
scope = dict(initial)
scope["path"] = "/"
scope["raw_path"] = b"/"
else:
scope = initial
print("initial raw_path:", initial["raw_path"])
print("transformed raw_path:", scope["raw_path"])
print("child_match_path:", scope["path"])
PYRepository: jamiepine/voicebox
Length of output: 275
🌐 Web query:
ASGI scope raw_path specification original path
💡 Result:
In the ASGI specification, raw_path is an optional field within the connection scope [1][2]. It is defined as a byte string representing the original HTTP path component exactly as it was received by the web server, without modification, unquoting, or percent-decoding [1][3]. Key characteristics of raw_path include: 1. Data Type: It is provided as a byte string, distinct from the path field, which is a Unicode string that has been fully decoded from both UTF-8 and percent-encoded sequences [1][2]. 2. Scope: It excludes the query string [1][3]. 3. Optionality: Because some web server implementations may be unable to provide the original raw bytes or may perform their own path normalization before the application layer is reached, raw_path is optional [1][4]. If a server cannot provide it, the field may be missing or set to None [1][3]. The addition of raw_path to the ASGI specification was intended to allow applications to access the path without the information loss that occurs when a server automatically performs percent-decoding on the standard path field [4][5]. However, developers should note that because intermediate proxies or the underlying web server might modify the path before it reaches the ASGI interface, the availability and exact content of raw_path can depend on the specific server implementation [1][5].
Citations:
- 1: https://asgi.readthedocs.io/en/latest/specs/www.html
- 2: https://github.com/django/asgiref/blob/master/specs/www.rst
- 3: https://dokk.org/documentation/django-asgiref/3.2.10/specs/www/
- 4: Provide raw path / URI django/asgiref#87
- 5: Provide the un-%-decoded path django/asgiref#51
Preserve the original ASGI raw_path.
For a bare mount request, raw_path is the unmodified client request path. Rewriting it to b"/" makes request inspection, logging, auditing, and signatures return a path the client did not send. Keep raw_path unchanged and rewrite only scope["path"] to / for the child application.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/mcp_server/server.py` around lines 41 - 47, Update the bare-mount
normalization in the HTTP scope handling to rewrite only scope["path"] to "/"
while preserving the original scope["raw_path"] value. Remove the raw_path
assignment from this block and leave the surrounding scope-copy behavior
unchanged.
| finally: | ||
| bg_db.close() | ||
| # Unload TTS model to free VRAM after speak | ||
| try: | ||
| tts_backend = get_tts_backend_for_engine(engine) | ||
| if tts_backend.is_loaded(): | ||
| tts_backend.unload_model() | ||
| except Exception: | ||
| logger.warning("Failed to unload TTS model after speak", exc_info=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move TTS cleanup after synchronous inference.
This finally runs before generate_chunked() at lines 356-358. It unloads the loaded tts_model before audio generation starts. The synchronous persist=false speech path will then generate with an unloaded backend.
Keep bg_db.close() in the setup cleanup. Put model cleanup in a finally that encloses inference and WAV encoding. Reuse the local tts_model instance. Add a regression test for this path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/services/generation.py` around lines 331 - 339, Move TTS model
cleanup out of the setup `finally` and into a `finally` enclosing synchronous
inference and WAV encoding, so cleanup occurs only after `generate_chunked()`
completes. Reuse the local `tts_model` instance for the loaded check and unload
operation, while keeping `bg_db.close()` in the existing setup cleanup. Add a
regression test covering the synchronous persist=false speech path and
confirming inference runs before unloading.
| def _read() -> int: | ||
| out = subprocess.run( | ||
| ["nvidia-smi", "--query-gpu=memory.used", "--format=csv,noheader,nounits"], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| ) | ||
| return int(out.stdout.strip().splitlines()[0]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)backend/tests/(conftest|test_tts_vram_churn)\.py$' || true
echo "== conftest excerpt =="
if [ -f backend/tests/conftest.py ]; then
nl -ba backend/tests/conftest.py | sed -n '1,120p'
fi
echo "== test excerpt =="
if [ -f backend/tests/test_tts_vram_churn.py ]; then
nl -ba backend/tests/test_tts_vram_churn.py | sed -n '1,120p'
fi
echo "== vram_mb usages =="
rg -n "vram_mb\(|nvidia-smi|pytest\.skip|skip" backend/tests || trueRepository: jamiepine/voicebox
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== conftest excerpt =="
if [ -f backend/tests/conftest.py ]; then
cat -n backend/tests/conftest.py | sed -n '1,120p'
fi
echo "== test excerpt =="
if [ -f backend/tests/test_tts_vram_churn.py ]; then
cat -n backend/tests/test_tts_vram_churn.py | sed -n '1,140p'
fi
echo "== vram_mb usages =="
grep -RInE "vram_mb\(|nvidia-smi|pytest\.skip|skip|skipif|marked|gpu" backend/tests || true
echo "== behavioral probe of vram_sm subprocess failure semantics =="
python3 - <<'PY'
import subprocess
def vram_mb():
out = subprocess.run(
["nvidia-smi", "--query-gpu=memory.used", "--format=csv,noheader,nounits"],
capture_output=True,
text=True,
check=True,
)
return int(out.stdout.strip().splitlines()[0])
try:
vram_mb()
except Exception as e:
print(type(e).__name__, repr(str(e)))
PYRepository: jamiepine/voicebox
Length of output: 14648
Skip VRAM checks when nvidia-smi is unavailable or fails.
backend/tests/test_tts_vram_churn.py:72, 77 calls vram_mb(), but backend/tests/conftest.py:49-56 raises on missing nvidia-smi or subprocess errors. Wrap the subprocess.run(..., check=True) call, catch FileNotFoundError and subprocess.CalledProcessError, and call pytest.skip with a clear message.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 49-54: Command coming from incoming request
Context: subprocess.run(
["nvidia-smi", "--query-gpu=memory.used", "--format=csv,noheader,nounits"],
capture_output=True,
text=True,
check=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/conftest.py` around lines 49 - 56, Update the _read helper to
catch FileNotFoundError and subprocess.CalledProcessError from subprocess.run,
then call pytest.skip with a clear message when nvidia-smi is unavailable or
fails. Preserve the existing parsing and integer return behavior when the
command succeeds.
| try: | ||
| _wait_for_health(base_url, proc, timeout=HEALTH_TIMEOUT) | ||
| except Exception as exc: | ||
| proc.kill() | ||
| proc.wait(timeout=10) | ||
| log_fh.close() | ||
| with contextlib.suppress(OSError): | ||
| tail = "\n".join(log_path.read_text(errors="replace").splitlines()[-40:]) | ||
| if "tail" not in locals(): | ||
| tail = "" | ||
| pytest.skip(f"backend did not become healthy: {exc}\n--- server log tail ---\n{tail}") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate conftest =="
fd -a 'conftest.py$' . | sed 's#^\./##'
echo "== backend/tests/conftest.py outline =="
if [ -f backend/tests/conftest.py ]; then
wc -l backend/tests/conftest.py
ast-grep outline backend/tests/conftest.py 2>/dev/null | sed -n '1,200p' || true
echo "== relevant conftest section =="
sed -n '1,220p' backend/tests/conftest.py
fi
echo "== health/failure fixture usages =="
if [ -f backend/tests/conftest.py ]; then
rg -n "backend_health|_wait_for_health|pytest\.skip|pytest\.fail|subprocess\.Popen|HEALTH_TIMEOUT" backend/tests backend || true
fiRepository: jamiepine/voicebox
Length of output: 8164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== live_backend usages in backend/tests =="
rg -n "def .*\(.*live_backend|live_backend|e2e|backend did not become healthy|could not spawn backend process" backend/tests backend/tests/conftest.py
echo "== test file sections likely receiving fixture =="
for f in backend/tests/test_tts_vram_churn.py backend/tests/test_whisper_long_audio_e2e.py backend/tests/test_all_models_e2e.py; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "def .*\(.*live_backend|live_backend|`@pytest`|`@mark`" "$f" || true
sed -n '1,120p' "$f"
fi
done
echo "== pytest skip/fail semantics =="
python3 - <<'PY'
try:
import pytest
except Exception as exc:
print(f"pytest unavailable: {type(exc).__name__}: {exc}")
else:
print(f"pytest available: {pytest.__version__}")
PYRepository: jamiepine/voicebox
Length of output: 14884
🌐 Web query:
Pytest skip vs fail documentation pytest.skip test skips pytest.fail test fails
💡 Result:
In pytest, the primary difference between skip and fail lies in whether the test is executed and how the test suite's outcome is reported [1][2]. pytest.skip (or @pytest.mark.skip) Skipping a test means that the test is not executed at all [1][2]. It is used when you expect a test to be relevant only under specific conditions (e.g., specific platforms or availability of external resources) [1][3]. Because the test is not run, it does not contribute to the "fail" count of your test suite, keeping the overall suite "green" [1][4]. You can call pytest.skip imperatively within a test or setup phase, or use the @pytest.mark.skip or @pytest.mark.skipif decorators to skip tests declaratively [5][4]. pytest.fail Calling pytest.fail explicitly forces an executing test to fail [5][6]. Unlike skipping, where the test is bypassed, pytest.fail triggers an immediate failure and is typically used within test logic to signal that a specific condition or assertion has been met which should cause the test to fail [6]. This will be counted as a failure in your test summary and will affect the exit status of your test suite [5]. Note on xfail While not the same as skipping or failing, pytest provides the xfail (expected fail) mechanism [1][7]. Tests marked with @pytest.mark.xfail are executed, but they are expected to fail; if they do fail as expected, they are reported as XFAIL (not a test suite failure) [1][2]. If they unexpectedly pass, they are reported as XPASS [1][7]. Unlike skip, the test code is still run [2].
Citations:
- 1: https://docs.pytest.org/en/stable/how-to/skipping.html
- 2: https://stackoverflow.com/questions/76838648/pytest-mark-skip-vs-pytest-mark-xfail-in-pytest
- 3: https://docs.pytest.org/en/8.3.x/how-to/skipping.html
- 4: https://pytest.org/en/stable/how-to/skipping.html
- 5: https://docs.pytest.org/en/stable/reference/reference.html
- 6: https://docs.pytest.org/en/6.2.x/_modules/_pytest/outcomes.html
- 7: https://pytest.org/en/latest/how-to/skipping.html
Fail when the spawned backend does not become healthy.
live_backend is an e2e fixture used by GPU-backed E2E tests. A subprocess.Popen() failure is a precondition, so pytest.skip() is appropriate only there. Once the backend is spawned, a health-check timeout or early process exit should report a failure to avoid masking live backend regressions.
Proposed fix
- pytest.skip(f"backend did not become healthy: {exc}\n--- server log tail ---\n{tail}")
+ pytest.fail(f"backend did not become healthy: {exc}\n--- server log tail ---\n{tail}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| _wait_for_health(base_url, proc, timeout=HEALTH_TIMEOUT) | |
| except Exception as exc: | |
| proc.kill() | |
| proc.wait(timeout=10) | |
| log_fh.close() | |
| with contextlib.suppress(OSError): | |
| tail = "\n".join(log_path.read_text(errors="replace").splitlines()[-40:]) | |
| if "tail" not in locals(): | |
| tail = "" | |
| pytest.skip(f"backend did not become healthy: {exc}\n--- server log tail ---\n{tail}") | |
| try: | |
| _wait_for_health(base_url, proc, timeout=HEALTH_TIMEOUT) | |
| except Exception as exc: | |
| proc.kill() | |
| proc.wait(timeout=10) | |
| log_fh.close() | |
| with contextlib.suppress(OSError): | |
| tail = "\n".join(log_path.read_text(errors="replace").splitlines()[-40:]) | |
| if "tail" not in locals(): | |
| tail = "" | |
| pytest.fail(f"backend did not become healthy: {exc}\n--- server log tail ---\n{tail}") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/conftest.py` around lines 117 - 127, Update the exception
handling around _wait_for_health in live_backend so that failures after the
backend has been spawned are reported as test failures rather than skipped.
Preserve pytest.skip only for the subprocess.Popen precondition failure, while
retaining the existing process cleanup and server-log-tail diagnostics for
health-check errors.
| # The concat stream is longer than 403 seconds; -t makes all three outputs | ||
| # exactly 6:43 (or fails rather than silently producing a short fixture). | ||
| ffmpeg -hide_banner -loglevel error -y -f concat -safe 0 -i "$concat_list" \ | ||
| -t 10 -ac 1 -ar 16000 "$OUT_DIR/cv_10s.wav" | ||
| ffmpeg -hide_banner -loglevel error -y -f concat -safe 0 -i "$concat_list" \ | ||
| -t 30 -ac 1 -ar 16000 "$OUT_DIR/cv_30s.wav" | ||
| ffmpeg -hide_banner -loglevel error -y -f concat -safe 0 -i "$concat_list" \ | ||
| -t 403 -ac 1 -ar 16000 "$OUT_DIR/cv_6m43s.wav" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)backend/tests/fixtures/generate_fixtures\.sh$|backend/tests/test_whisper_long_audio_e2e\.py$' || true
echo
echo "== generate fixture relevant section =="
if [ -f backend/tests/fixtures/generate_fixtures.sh ]; then
wc -l backend/tests/fixtures/generate_fixtures.sh
sed -n '1,140p' backend/tests/fixtures/generate_fixtures.sh | cat -n
fi
echo
echo "== e2e test relevant section =="
if [ -f backend/tests/test_whisper_long_audio_e2e.py ]; then
wc -l backend/tests/test_whisper_long_audio_e2e.py
sed -n '1,120p' backend/tests/test_whisper_long_audio_e2e.py | cat -n
fi
echo
echo "== search fixture duration usages =="
rg -n "cv_6m43s|whisper_long_audio|400|403|duration|duration" backend/tests tests . -g '!node_modules' -g '!dist' -g '!build' || trueRepository: jamiepine/voicebox
Length of output: 50374
🌐 Web query:
FFmpeg duration option -t output shorter input exits normally documentation
💡 Result:
When using the -t option in FFmpeg, the software is designed to stop processing as soon as the specified duration limit is reached, regardless of whether the input file is shorter or longer than that duration [1][2]. If the input file is shorter than the duration specified with -t, FFmpeg will reach the end of the input file before the duration is met. In this scenario, FFmpeg will exit normally [1][2][3]. It does not throw an error simply because the input ended prematurely; rather, it completes the task based on the available input data and terminates the process as expected [1][2]. To summarize: - Input shorter than -t duration: FFmpeg reads until the end of the file and exits normally [1][2]. - Input longer than -t duration: FFmpeg stops reading the input and writing the output once the specified -t duration is reached [1][2]. The -t option is a limit on the duration of data read (when used as an input option before -i) or the duration of the output file (when used as an output option) [1][4]. It is distinct from the -shortest option, which instructs FFmpeg to stop muxing at the end of the shortest available input stream [5][6].
Citations:
- 1: https://www.ffmpeg.org/ffmpeg.html
- 2: https://manpages.debian.org/unstable/ffmpeg/ffmpeg.1.en.html
- 3: https://ffmpeg.org/pipermail/ffmpeg-trac/2023-April/065599.html
- 4: https://manpagez.com/man/1/ffmpeg/
- 5: https://www.ffmpeg.org/ffmpeg-formats.html
- 6: https://github.com/FFmpeg/FFmpeg/blob/3137d337/doc/formats.texi
Validate the generated long-fixture duration.
ffmpeg -t 403 only caps how long FFmpeg writes; if the concat input is shorter than 403 seconds, FFmpeg exits normally and produces a short cv_6m43s.wav. This can still satisfy the downstream E2E pass condition, which only asserts duration >= 400.0. Fail the fixture script if the output is below 403 seconds, or keep adding normalized clips until the concat input is long enough.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/fixtures/generate_fixtures.sh` around lines 73 - 80, Update the
fixture generation flow around the long `ffmpeg` invocation for `cv_6m43s.wav`
to validate the generated WAV duration is at least 403 seconds after encoding.
Make the script exit nonzero when the duration is shorter, using the
repository’s available media-inspection tooling, while preserving the existing
10-second and 30-second fixture generation.
| La matriz de calidad WER para audio real de 6:43, formatos WAV/FLAC/MP3 y | ||
| carga en frío/caliente requiere `pytest -m gpu` con fixtures locales generadas | ||
| por `backend/tests/fixtures/generate_fixtures.sh`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the WER verification claim.
backend/tests/test_whisper_long_audio_e2e.py does not calculate WER or compare against reference text. It only checks non-empty text and audio duration. Either add a reference transcript with a WER threshold or describe this command as a transcription smoke test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/evaluation/whisper-chunking.md` around lines 30 - 32, Update the WER
verification description in whisper-chunking.md to accurately describe
backend/tests/test_whisper_long_audio_e2e.py as a transcription smoke test,
unless the test is enhanced with reference text and an explicit WER threshold.
Do not claim WER quality-matrix validation while the test only checks non-empty
transcription output and audio duration.
| # Run local evaluation harnesses; GPU/model tests remain opt-in via --gpu. | ||
| [unix] | ||
| eval-local: _ensure-venv | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m "not e2e and not slow" -v | ||
| bash scripts/test_backend_detection.sh | ||
| if [[ -f "${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" ]]; then | ||
| VOICEBOX_DICTATE_SCRIPT="${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" bash scripts/test_dictate_e2e.sh | ||
| else | ||
| echo "Skipping dictation harness: no installed script found." | ||
| fi | ||
| if [[ "${GPU:-0}" == "1" ]]; then | ||
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m gpu -v | ||
| else | ||
| echo "Skipping GPU suite. Re-run with GPU=1 after model/fixture setup." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude GPU tests from the initial eval-local run.
Line 342 selects every test except e2e and slow. It therefore runs gpu tests before the GPU=1 gate. On GPU hosts, GPU=1 runs the GPU suite twice. Add not gpu to the initial marker expression.
Proposed fix
- {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m "not e2e and not slow" -v
+ {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m "not e2e and not slow and not gpu" -v📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run local evaluation harnesses; GPU/model tests remain opt-in via --gpu. | |
| [unix] | |
| eval-local: _ensure-venv | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m "not e2e and not slow" -v | |
| bash scripts/test_backend_detection.sh | |
| if [[ -f "${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" ]]; then | |
| VOICEBOX_DICTATE_SCRIPT="${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" bash scripts/test_dictate_e2e.sh | |
| else | |
| echo "Skipping dictation harness: no installed script found." | |
| fi | |
| if [[ "${GPU:-0}" == "1" ]]; then | |
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m gpu -v | |
| else | |
| echo "Skipping GPU suite. Re-run with GPU=1 after model/fixture setup." | |
| # Run local evaluation harnesses; GPU/model tests remain opt-in via --gpu. | |
| [unix] | |
| eval-local: _ensure-venv | |
| #!/usr/bin/env bash | |
| set -euo pipefail | |
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m "not e2e and not slow and not gpu" -v | |
| bash scripts/test_backend_detection.sh | |
| if [[ -f "${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" ]]; then | |
| VOICEBOX_DICTATE_SCRIPT="${VOICEBOX_DICTATE_SCRIPT:-$HOME/.local/bin/voicebox-dictate.sh}" bash scripts/test_dictate_e2e.sh | |
| else | |
| echo "Skipping dictation harness: no installed script found." | |
| fi | |
| if [[ "${GPU:-0}" == "1" ]]; then | |
| {{ venv_bin }}/python -m pytest {{ backend_dir }}/tests -m gpu -v | |
| else | |
| echo "Skipping GPU suite. Re-run with GPU=1 after model/fixture setup." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@justfile` around lines 337 - 352, Update the initial pytest marker expression
in the eval-local recipe to exclude gpu tests alongside e2e and slow tests. Keep
the separate GPU=1 conditional suite unchanged so GPU tests run only through
that gated path and are not executed twice.
| # Runs the installed dictate script in a fully controlled environment. | ||
| run_dictate() { | ||
| local rc=0 | ||
| env -i \ | ||
| HOME="$FAKE_HOME" \ | ||
| PATH="$STUB_DIR:/usr/bin:/bin" \ | ||
| WAYLAND_DISPLAY="wayland-vbtest" \ | ||
| CALLS_LOG="$CALLS_LOG" \ | ||
| CLIP_FILE="$CLIP_FILE" \ | ||
| VB_FIXTURE_WAV="$FIXTURE" \ | ||
| VB_STUB_ARECORD_FAIL="${VB_STUB_ARECORD_FAIL:-0}" \ | ||
| VB_STUB_ARECORD_NOWAV="${VB_STUB_ARECORD_NOWAV:-0}" \ | ||
| VB_STUB_CURL_FAIL="${VB_STUB_CURL_FAIL:-0}" \ | ||
| VB_STUB_WLCOPY_FAIL="${VB_STUB_WLCOPY_FAIL:-0}" \ | ||
| VB_STUB_YDOTOOL_FAIL="${VB_STUB_YDOTOOL_FAIL:-0}" \ | ||
| "$DICTATE_SCRIPT" >/dev/null 2>&1 || rc=$? |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Stub pw-record before exposing host binaries.
The isolated PATH includes /usr/bin, but the harness only stubs arecord. If the installed script selects pw-record, it can invoke the host recorder and access real audio hardware. Add a pw-record stub with the same controlled SIGTERM and WAV behavior, then make recorder assertions accept the selected recorder.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test_dictate_e2e.sh` around lines 157 - 172, Update the test harness
around run_dictate to create and expose a controlled pw-record stub before the
host binaries in PATH, matching the existing arecord stub’s SIGTERM handling and
WAV-output behavior. Adjust recorder-related assertions to accept whichever
recorder, arecord or pw-record, the installed dictate script selects while
preserving controlled failure and output checks.
1 participant
Summary
/mcpand/mcp/without redirecting POST requestsVerification
bun run typecheck✅bunx lint-staged✅python3 -m pytest backend/tests/test_fixture_contract.py -q✅ (2 passed)scripts/test_backend_detection.sh --case B/C✅scripts/test_dictate_e2e.sh✅ (24 passed)The full backend suite and real GPU WER/VRAM thresholds require the project backend environment and local model/GPU dependencies; those are intentionally not downloaded by this PR.
Summary by CodeRabbit
Bug Fixes
/mcpand/mcp/endpoint formats.Documentation