feat(mcp): add voicebox.speak_audio for headless callers - #888
Open
AlexStephenLytton wants to merge 2 commits into
Open
feat(mcp): add voicebox.speak_audio for headless callers#888AlexStephenLytton wants to merge 2 commits into
AlexStephenLytton wants to merge 2 commits into
Conversation
voicebox.speak only triggers playback via an open Voicebox browser tab (the floating pill reacting to the speak-start event) and returns just a generation id / poll URL - no audio ever reaches the MCP caller. That makes it unusable for headless/background agents (e.g. a chat agent with no Voicebox UI open), since there is nothing to actually play the generated audio. voicebox.speak_audio wraps the same _speak() resolution logic (profile/ engine/personality), polls the generation until it completes or fails, then reads the resulting file off disk and returns it as base64 audio bytes (audio_base64, audio_format) directly in the tool result. It still fires the same speak-start event, so an already-open Voicebox tab keeps working exactly as before and the generation is still saved to Captures / History. Verified end-to-end on both a CUDA/pytorch backend and an Apple Silicon/MLX backend (base64-decoded output played back correctly).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe MCP tools now provide headless callers with a timed, blocking ChangesMCP audio generation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MCPCaller
participant voicebox_speak_audio
participant _speak
participant GenerationStatus
participant AudioFile
MCPCaller->>voicebox_speak_audio: Provide text and timeout
voicebox_speak_audio->>_speak: Start generation
_speak-->>voicebox_speak_audio: Return generation result
loop Until completion, failure, or timeout
voicebox_speak_audio->>GenerationStatus: Poll status
GenerationStatus-->>voicebox_speak_audio: Return completed or failed status
end
voicebox_speak_audio->>AudioFile: Read generated audio
AudioFile-->>voicebox_speak_audio: Return audio bytes
voicebox_speak_audio-->>MCPCaller: Return base64 audio or loopback output path
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mcp_server/tools.py`:
- Around line 151-183: Extract the shared profile, MCPClientBinding, engine,
personality, and use_persona resolution currently duplicated in voicebox_speak
and the shown tool into a reusable helper. Update both callers to use that
helper while preserving existing validation, binding precedence, profile
fallback, and returned values; keep tool-specific behavior outside the shared
resolution.
- Around line 156-238: The database session in the MCP speech flow remains open
during the entire generation wait. Update the code around `_speak()` and the
`while True` polling loop to close the initial `db` session immediately after
`_speak()` returns, then create and release a short-lived session for each
`DBGeneration` status query, preserving the existing polling, timeout, and
completion behavior.
🪄 Autofix (Beta)
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
Run ID: 1906a2fe-1907-4ac9-9d83-f5c2e8ca3fb1
📒 Files selected for processing (1)
backend/mcp_server/tools.py
Comment on lines
+151
to
+183
| from ..database.models import MCPClientBinding | ||
|
|
||
| if timeout_seconds <= 0: | ||
| raise ValueError("`timeout_seconds` must be > 0.") | ||
|
|
||
| db = next(get_db()) | ||
| try: | ||
| client_id = current_client_id.get() | ||
| vp = resolve_profile(profile, client_id, db) | ||
| if vp is None: | ||
| raise ValueError( | ||
| "No voice profile resolved. Pass `profile=` with a " | ||
| "voice profile name or id, or set a default voice in " | ||
| "Voicebox → Settings → MCP." | ||
| ) | ||
|
|
||
| binding = None | ||
| if client_id: | ||
| binding = ( | ||
| db.query(MCPClientBinding) | ||
| .filter(MCPClientBinding.client_id == client_id) | ||
| .first() | ||
| ) | ||
|
|
||
| resolved_personality = personality | ||
| if resolved_personality is None and binding is not None: | ||
| resolved_personality = bool(binding.default_personality) | ||
|
|
||
| resolved_engine = engine | ||
| if resolved_engine is None and binding is not None: | ||
| resolved_engine = binding.default_engine | ||
|
|
||
| use_persona = bool(resolved_personality) and bool(vp.personality) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract shared profile/engine/personality resolution instead of copy-pasting it.
Lines 151-183 duplicate the exact resolution block from voicebox_speak (lines 75-104) verbatim: the MCPClientBinding import, resolve_profile call, binding lookup, resolved_personality, resolved_engine, and use_persona derivation. Any future fix to this logic (e.g., binding precedence, personality fallback) now has to be applied in two places.
♻️ Proposed extraction
+async def _resolve_speak_params(
+ profile: str | None,
+ engine: str | None,
+ personality: bool | None,
+ client_id: str | None,
+ db,
+):
+ from ..database.models import MCPClientBinding
+
+ vp = resolve_profile(profile, client_id, db)
+ if vp is None:
+ raise ValueError(
+ "No voice profile resolved. Pass `profile=` with a "
+ "voice profile name or id, or set a default voice in "
+ "Voicebox → Settings → MCP."
+ )
+
+ binding = None
+ if client_id:
+ binding = (
+ db.query(MCPClientBinding)
+ .filter(MCPClientBinding.client_id == client_id)
+ .first()
+ )
+
+ resolved_personality = personality
+ if resolved_personality is None and binding is not None:
+ resolved_personality = bool(binding.default_personality)
+
+ resolved_engine = engine
+ if resolved_engine is None and binding is not None:
+ resolved_engine = binding.default_engine
+
+ use_persona = bool(resolved_personality) and bool(vp.personality)
+ return vp, resolved_engine, use_personaThen both tools reduce to:
- client_id = current_client_id.get()
- vp = resolve_profile(profile, client_id, db)
- if vp is None:
- raise ValueError(...)
- binding = None
- if client_id:
- binding = (...)
- resolved_personality = personality
- if resolved_personality is None and binding is not None:
- resolved_personality = bool(binding.default_personality)
- resolved_engine = engine
- if resolved_engine is None and binding is not None:
- resolved_engine = binding.default_engine
- use_persona = bool(resolved_personality) and bool(vp.personality)
+ client_id = current_client_id.get()
+ vp, resolved_engine, use_persona = await _resolve_speak_params(
+ profile, engine, personality, client_id, db
+ )📝 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.
Suggested change
| from ..database.models import MCPClientBinding | |
| if timeout_seconds <= 0: | |
| raise ValueError("`timeout_seconds` must be > 0.") | |
| db = next(get_db()) | |
| try: | |
| client_id = current_client_id.get() | |
| vp = resolve_profile(profile, client_id, db) | |
| if vp is None: | |
| raise ValueError( | |
| "No voice profile resolved. Pass `profile=` with a " | |
| "voice profile name or id, or set a default voice in " | |
| "Voicebox → Settings → MCP." | |
| ) | |
| binding = None | |
| if client_id: | |
| binding = ( | |
| db.query(MCPClientBinding) | |
| .filter(MCPClientBinding.client_id == client_id) | |
| .first() | |
| ) | |
| resolved_personality = personality | |
| if resolved_personality is None and binding is not None: | |
| resolved_personality = bool(binding.default_personality) | |
| resolved_engine = engine | |
| if resolved_engine is None and binding is not None: | |
| resolved_engine = binding.default_engine | |
| use_persona = bool(resolved_personality) and bool(vp.personality) | |
| async def _resolve_speak_params( | |
| profile: str | None, | |
| engine: str | None, | |
| personality: bool | None, | |
| client_id: str | None, | |
| db, | |
| ): | |
| from ..database.models import MCPClientBinding | |
| vp = resolve_profile(profile, client_id, db) | |
| if vp is None: | |
| raise ValueError( | |
| "No voice profile resolved. Pass `profile=` with a " | |
| "voice profile name or id, or set a default voice in " | |
| "Voicebox → Settings → MCP." | |
| ) | |
| binding = None | |
| if client_id: | |
| binding = ( | |
| db.query(MCPClientBinding) | |
| .filter(MCPClientBinding.client_id == client_id) | |
| .first() | |
| ) | |
| resolved_personality = personality | |
| if resolved_personality is None and binding is not None: | |
| resolved_personality = bool(binding.default_personality) | |
| resolved_engine = engine | |
| if resolved_engine is None and binding is not None: | |
| resolved_engine = binding.default_engine | |
| use_persona = bool(resolved_personality) and bool(vp.personality) | |
| return vp, resolved_engine, use_persona | |
| if timeout_seconds <= 0: | |
| raise ValueError("`timeout_seconds` must be > 0.") | |
| db = next(get_db()) | |
| try: | |
| client_id = current_client_id.get() | |
| vp, resolved_engine, use_persona = await _resolve_speak_params( | |
| profile, engine, personality, client_id, db | |
| ) |
🤖 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/tools.py` around lines 151 - 183, Extract the shared
profile, MCPClientBinding, engine, personality, and use_persona resolution
currently duplicated in voicebox_speak and the shown tool into a reusable
helper. Update both callers to use that helper while preserving existing
validation, binding precedence, profile fallback, and returned values; keep
tool-specific behavior outside the shared resolution.
Comment on lines
+156
to
+238
| db = next(get_db()) | ||
| try: | ||
| client_id = current_client_id.get() | ||
| vp = resolve_profile(profile, client_id, db) | ||
| if vp is None: | ||
| raise ValueError( | ||
| "No voice profile resolved. Pass `profile=` with a " | ||
| "voice profile name or id, or set a default voice in " | ||
| "Voicebox → Settings → MCP." | ||
| ) | ||
|
|
||
| binding = None | ||
| if client_id: | ||
| binding = ( | ||
| db.query(MCPClientBinding) | ||
| .filter(MCPClientBinding.client_id == client_id) | ||
| .first() | ||
| ) | ||
|
|
||
| resolved_personality = personality | ||
| if resolved_personality is None and binding is not None: | ||
| resolved_personality = bool(binding.default_personality) | ||
|
|
||
| resolved_engine = engine | ||
| if resolved_engine is None and binding is not None: | ||
| resolved_engine = binding.default_engine | ||
|
|
||
| use_persona = bool(resolved_personality) and bool(vp.personality) | ||
| speak_result = await _speak( | ||
| profile_id=vp.id, | ||
| profile_name=vp.name, | ||
| text=text, | ||
| engine=resolved_engine, | ||
| language=language, | ||
| personality=use_persona, | ||
| db=db, | ||
| ) | ||
|
|
||
| generation_id = speak_result.get("generation_id") | ||
| if not generation_id: | ||
| raise ValueError( | ||
| "Generation failed to start; no generation_id returned." | ||
| ) | ||
|
|
||
| deadline = time.monotonic() + timeout_seconds | ||
| gen = None | ||
| while True: | ||
| db.expire_all() | ||
| gen = db.query(DBGeneration).filter_by(id=generation_id).first() | ||
| status = (gen.status if gen else None) or "generating" | ||
| if status == "completed": | ||
| break | ||
| if status == "failed": | ||
| error_detail = gen.error if gen else "unknown error" | ||
| raise ValueError( | ||
| f"Generation {generation_id} failed: {error_detail}" | ||
| ) | ||
| if time.monotonic() >= deadline: | ||
| raise ValueError( | ||
| f"Generation {generation_id} did not complete within " | ||
| f"{timeout_seconds}s (last status: {status}). Pass a " | ||
| "higher `timeout_seconds` for long text or a " | ||
| "slow/cold-loading model." | ||
| ) | ||
| await asyncio.sleep(0.5) | ||
|
|
||
| audio_path = config.resolve_storage_path(gen.audio_path) | ||
| if audio_path is None or not audio_path.is_file(): | ||
| raise ValueError( | ||
| f"Generation {generation_id} completed but its audio " | ||
| "file is missing on disk." | ||
| ) | ||
|
|
||
| audio_bytes = await asyncio.to_thread(audio_path.read_bytes) | ||
|
|
||
| return { | ||
| **speak_result, | ||
| "status": "completed", | ||
| "duration": gen.duration, | ||
| "audio_base64": b64.b64encode(audio_bytes).decode("ascii"), | ||
| "audio_format": audio_path.suffix.lstrip("."), | ||
| } | ||
| finally: |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tools.py context ==\n'
sed -n '120,280p' backend/mcp_server/tools.py
printf '\n== get_db definitions ==\n'
rg -n "def get_db|yield.*Session|sessionmaker|scoped_session|expire_all\(|close\(\)" backend -g '*.py'
printf '\n== database/session setup ==\n'
rg -n "create_engine|pool_size|max_overflow|poolclass|SessionLocal|sessionmaker" backend -g '*.py'
printf '\n== DBGeneration / MCPClientBinding references ==\n'
rg -n "class DBGeneration|class MCPClientBinding|voicebox\.list_captures|voicebox\.list_profiles" backend -g '*.py'Repository: jamiepine/voicebox
Length of output: 11001
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== backend/database/session.py ==\n'
cat -n backend/database/session.py
printf '\n== backend/mcp_server/tools.py (surrounding relevant lines) ==\n'
sed -n '150,245p' backend/mcp_server/tools.py
printf '\n== engine/pool defaults in repository ==\n'
rg -n "create_engine\\(" backend -g '*.py'Repository: jamiepine/voicebox
Length of output: 6814
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== SQLite/WAL pragmas or journal mode ==\n'
rg -n "journal_mode|WAL|PRAGMA|busy_timeout|check_same_thread" backend -g '*.py'
printf '\n== generation status write path ==\n'
rg -n "status\s*=\s*['\"](completed|failed|generating)|DBGeneration.*status|update\\(DBGeneration|generation.status|commit\\(" backend -g '*.py'Repository: jamiepine/voicebox
Length of output: 6660
Release the DB session before the polling loop
db stays open for the entire while True wait, so each caller holds a SQLite connection/read transaction idle for up to timeout_seconds. That can block concurrent writers from committing generation status updates. Close the session after _speak() returns and use a short-lived session for each poll.
🤖 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/tools.py` around lines 156 - 238, The database session in
the MCP speech flow remains open during the entire generation wait. Update the
code around `_speak()` and the `while True` polling loop to close the initial
`db` session immediately after `_speak()` returns, then create and release a
short-lived session for each `DBGeneration` status query, preserving the
existing polling, timeout, and completion behavior.
Base64-encoding audio in the tool result works but scales badly - a few seconds of audio is tens of KB of base64 text, which some MCP clients/agents truncate when they cap tool-result size (to protect their own context budget). That silently corrupts the audio with no clear error. output_path is a generic escape hatch for local callers who don't want to pay that cost: pass an absolute path and the server writes the finished audio there directly, returning only the small string instead of the base64 blob. Restricted to loopback callers, mirroring the exact same restriction already used for audio_path on voicebox.transcribe (and for the same reason - a Voicebox instance bound on 0.0.0.0 shouldn't double as a remote arbitrary-local-file-write primitive). When output_path is omitted, behavior is unchanged (base64 response). Verified end-to-end on both backends from the previous commit (CUDA and MLX): loopback call with output_path writes a valid file at the given path and returns just the path; a LAN-address call (non- loopback) with output_path is correctly rejected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
voicebox.speakonly triggers playback via an open Voicebox browser tab -- the floating pill reacts to thespeak-startevent and plays the audio there. The MCP tool itself returns just ageneration_id/ poll URL, never the actual audio. That makes it unusable for headless/background MCP callers (e.g. a chat agent with no Voicebox UI open anywhere) -- there's simply nothing listening to play the generated audio.Fix
New tool
voicebox.speak_audio:_speak()resolution logic asvoicebox.speak(profile/engine/personality resolution -- identical behavior, no duplicated logic)timeout_seconds, default 60s) until it reachescompletedorfailedconfig.resolve_storage_path(same helperroutes/audio.pyuses) and returns it as base64 (audio_base64,audio_format) directly in the tool resultspeak-startevent, so an already-open Voicebox tab keeps working exactly as before, and the generation is still saved to Captures / History -- this is purely additive,voicebox.speakis unchanged in behaviorAlso added a short cross-reference to
voicebox.speak's description pointing headless callers at the new tool.Testing
Verified end-to-end via raw MCP JSON-RPC calls (
tools/call) against both:afplay, confirmed real speech audio, correct RIFF/WAVE headerAlso confirmed the timeout path works correctly: a cold model load past the timeout returns a clear, actionable error message rather than hanging or a bare exception.
Summary by CodeRabbit