feat: support ElevenLabs voice cloning and audio previews#866
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for audio file previews in the frontend and persistent voice cloning capabilities using ElevenLabs in the backend. On the frontend, it adds native audio playback controls to the file preview component, handles legacy workspace audio paths, and refines model capability badges to keep sound effects and music distinct. On the backend, it implements a new clone_tts_voice tool, adds persistent voice cloning to the ElevenLabs TTS provider, and improves error handling for invalid voice IDs. The review feedback highlights two important issues: a potential concurrency race condition in aclose where temporary voice clones might leak if deleted during an active cloning operation, and a logic bug in getProviderDisplayCapabilities where returning early for certain audio categories skips processing other valid model abilities.
rogercloud
left a comment
There was a problem hiding this comment.
Summary
This PR adds ElevenLabs voice-cloning support to the audio synthesis stack. Concretely it: (1) enables temporary, task-local voice cloning so speech can be synthesized directly from a reference audio clip, with clones reused within a task and cleaned up on aclose(); (2) adds a new provider-scoped clone_tts_voice tool for creating persistent ElevenLabs voice clones; (3) makes list_tts_voices provider-aware (preserving ElevenLabs voice categories and giving clearer guidance on invalid voice IDs); (4) surfaces per-provider TTS/ASR/sound-effect/music capabilities on the frontend provider cards; and (5) renders MP3/audio workspace artifacts with native <audio> playback controls instead of image previews.
Re-review update
This is a re-review. Since the previous review there are two commits: 662f1e84 (original feature) and cd55d8df "fix: address ElevenLabs review feedback". The fixup commit addresses both findings raised by the earlier gemini-code-assist review — the aclose() race condition and the early-return capability bug — and adds a regression test for each. Both are confirmed fixed (see below).
Design verdict: SOUND
Blank-slate design assessment came out sound:
- The two-tier clone lifecycle (temporary task-local vs. persistent) is coherent. Double-checked locking in
_get_or_create_cloned_voiceplus the lock inaclose()correctly handles both concurrent reuse and an in-flight clone during teardown (tested). - Provider-scoping is consistent across
list_tts_voicesandclone_tts_voice: both gate through_get_provider_tts_model, both enforce the provider-mismatch guard, and both gate tool registration on capability. Non-ElevenLabs providers degrade gracefully — verified by tests, no regression to existing providers. getProviderDisplayCapabilitiesextraction is a clean, testable improvement.- One avoidable-duplication note:
InlineAudioPreviewreimplementsInlineImagePreview's blob-fetch/revoke mechanism instead of sharing a hook (see Simplifications). Not wrong, just duplicated.
Prior findings — both FIXED
| # | Finding | Status | Evidence |
|---|---|---|---|
| 1 | Race condition in aclose() — a temporary clone could leak if aclose() ran concurrently with an in-flight _get_or_create_cloned_voice (src/xagent/core/model/tts/elevenlabs.py) |
✅ Fixed | aclose() now wraps its whole body in async with self._voice_clone_lock: (elevenlabs.py:234-262), matching the lock held across the clone-and-register operation in _get_or_create_cloned_voice (elevenlabs.py:402-420). New regression test test_aclose_waits_for_inflight_temporary_voice_clone (tests/core/model/tts/test_elevenlabs.py:278-319) simulates the exact race and passes; full suite 26 passed. |
| 2 | Early-return bug in getProviderDisplayCapabilities — early return for sound_effect/music skipped all other legitimate abilities, not just generate (frontend/src/components/pages/model-display-capabilities.ts) |
✅ Fixed | Code (lines 16-32) no longer returns early: it adds the category tag then iterates abilities, skipping only generate for sound_effect/music via `if (ability !== "generate" |
New findings this round
No criticals or majors — all findings below are minor / optional.
-
F1 (efficiency) —
src/xagent/core/model/tts/elevenlabs.py,_reference_audio_file/_get_or_create_cloned_voice(~L372-398): the reference audio is fully read into memory viaread_bytes()on everysynthesize()call before the cache-hit check, even on a hit. The cache key (resolved_path:size:mtime_ns) only needsstat(), not the bytes — wasted I/O and memory in batch synthesis reusing one reference across many segments. Suggest deferring the read until after a cache miss is confirmed. -
F2 (resource leak) —
src/xagent/core/model/tts/elevenlabs.py,aclose()(~L234-262): the delete-clones branch is guarded byasync_client is not None. Onceself._async_clientis nulled at the end of anyaclose(), there is no retry path at all — if a clone delete fails once (network blip; currently logged and swallowed), that voice is never retried and leaks permanently in the ElevenLabs account. Consider retaining un-deleted clone IDs for a later retry rather than dropping them. -
F3 (documentation) —
src/xagent/core/model/tts/elevenlabs.py(~L514-517): when bothvoiceandreference_audioare supplied,reference_audiosilently overrides the explicitvoice_idwith the clone. The precedence is reasonable but undocumented — neitherSYNTHESIZE_SPEECH_DESCRIPTIONinaudio_tool_descriptions.pynor the tool docstring mentions it, so an agent passing both is overridden with no signal. Document the precedence. -
F4 (consistency) —
frontend/src/components/file/inline-file-preview.tsx(~L149 vs L49):InlineAudioPreviewonly attempts an authenticated fetch whensource.fileIdcontains/(legacy path form), whereasInlineImagePreviewalways attempts authenticated fetch for anyfileId. For a managed audio file (bare-UUIDfileId), the public preview endpoint is hit unconditionally with zero auth attempt — bypassing the auth layer that the image component's own comment (~L73-76) exists to handle. No stated reason for the divergence. -
F5 (resilience) —
frontend/src/components/file/inline-file-preview.tsx(~L184 vs L77/87): on fetch failure, audio setsloadError=true(dead end — no playback control rendered), while image falls back to the publicpreviewUrl(best-effort). The image path's inline comment states the rationale ("a spinner/dead end with no recovery path is worse than attempting the anonymous endpoint"); audio does the opposite with no comment explaining the different choice. -
F6 (cosmetic) —
frontend/src/components/file/inline-file-preview.tsx(~L206): the audio preview header uses theFileTexticon.Volume2/Musicare already used for audio elsewhere (file-attachment.tsx,models.tsx,default-models.tsx) and would be clearer here. -
F7 (test-coverage) —
src/xagent/core/tools/core/audio_tool.py: no test exercises the "No {provider} TTS model is configured" branch (tts_model resolves toNone) for eitherlist_tts_voices(~L1024-1033) or the newclone_tts_voice(~L1111-1118) — existing tests cover only the provider-mismatch branch, not not-configured-at-all. (The related sub-claim thatreference_audiotool-level pass-through is untested is false —test_synthesize_speech_passes_structured_tts_optionsalready covers it.)
Simplification opportunities
frontend/src/components/file/inline-file-preview.tsxL133-195:InlineAudioPreviewduplicates ~90% ofInlineImagePreview's auth-fallback fetch →.blob()→createObjectURL→ cleanup effect. A shareduseAuthenticatedPreviewUrlhook would need to parameterize both the fallback-trigger condition (F4) and the failure-recovery behavior (F5), so it's more than a two-arg extraction — but the collapse is real.src/xagent/core/tools/core/audio_tool.pyL1024-1049 vs L1111-1131:clone_tts_voice's "not configured" and "provider mismatch" error blocks structurally duplicatelist_tts_voices's. A shared_resolve_provider_tts_model_or_error(...)helper is feasible but needs a caller-name parameter (error text embeds the method name) and per-call extra-field handling (field sets andprovider-key placement differ asymmetrically).
net: -45 to -55 lines possible
Testing performed
- Backend:
pytest tests/core/model/tts/test_elevenlabs.py tests/core/tools/test_audio_tool.py— all passed (matches the PR's claimed 48/49, confirmed on re-run). - Frontend:
vitest run src/components/pages/model-display-capabilities.test.ts— 4/4 passed.
Recommendation
Approve. Both previously-blocking issues are fixed with regression coverage, the design is sound, and this round surfaced zero criticals or majors — only minor, optional nits. F1 (redundant file read on cache hit) and F2 (clones never retried on delete failure) are the most worthwhile follow-ups but neither blocks merge; the rest are documentation, consistency, and cosmetic polish that can land here or in a follow-up.
|
Addressed the optional re-review findings in
I intentionally left the broader hook/error-helper deduplication ideas out of this PR because they are behavior-neutral refactors with a wider review surface. Validation: 52 backend tests, 36 frontend tests, TypeScript type-check, Ruff/format, and the full pre-commit suite passed. |
rogercloud
left a comment
There was a problem hiding this comment.
Re-review — round 2 (commit d4f90d9)
What changed since last review
Since my review at cd55d8d, the author pushed one new commit, d4f90d9 ("fix: address audio review findings"), which addresses all 7 minor findings (F1-F7) from that round, each backed by a new regression test. The author also explicitly declined the 2 simplification suggestions (shared preview hook + shared TTS-model-resolution helper), stating they are behavior-neutral refactors with a wider review surface that they preferred to keep out of this PR. No behavioral surprises in the diff; the fixes are targeted and well-tested.
Design verdict (fresh Round 0): SOUND (unchanged)
- The temporary-vs-persistent clone split remains coherent. Re-confirmed that TTS model instances are created fresh per task, so teardown cleanup cannot delete voices in use by a concurrent task.
_get_provider_tts_modelunifies provider + capability lookup cleanly and now returns explicitNonerather than silently falling back to a wrong-provider default — a good direction.
Scope clarification (not a finding): frontend/src/app/globals.css and frontend/src/app/layout.tsx show up in a two-dot diff against the base but are not part of this PR — git log <base>...HEAD -- globals.css layout.tsx is empty. They belong to the base branch tip (an unrelated scrollbar fix, #864) having advanced past this PR's actual merge-base. No action needed; flagging only so it isn't mistaken for scope creep in a later round.
Prior findings (F1-F7) — all FIXED
| # | Type | Status | Evidence |
|---|---|---|---|
| F1 | efficiency | Fixed | Reference-audio read deferred until after cache-miss. New _reference_audio_metadata does stat-only cache-key computation; full read_bytes() now runs only after the locked cache-miss re-check in _get_or_create_cloned_voice. New test asserts the file is read exactly once across two synthesize() calls. |
| F2 | resource leak | Fixed | aclose() now spins up a fresh async client via _create_async_client() when retrying cleanup of previously-failed clone IDs, which are retained in _cloned_voice_ids instead of dropped on failure. New test_aclose_retries_failed_temporary_voice_cleanup covers fail-then-retry. |
| F3 | documentation | Fixed | SYNTHESIZE_SPEECH_DESCRIPTION now states "When provided, it takes precedence over voice." — surfaced to the agent via the tool schema. |
| F4 | consistency | Fixed | InlineAudioPreview fallback condition changed from source.fileId?.includes('/') to Boolean(source.fileId), matching InlineImagePreview; any managed audio fileId now triggers authenticated fetch. Test confirms. |
| F5 | resilience | Fixed | Both !response.ok and catch branches now fall back to previewUrl (matching image best-effort behavior); the dead-end loadError state/prop was removed entirely. Test confirms fallback to public preview URL on auth failure. |
| F6 | cosmetic | Fixed | Icon changed from FileText to Volume2. |
| F7 | test-coverage | Fixed | Two new tests (test_list_tts_voices_reports_missing_elevenlabs_model, test_clone_tts_voice_reports_missing_elevenlabs_model) cover the "no TTS model configured at all" branch, distinct from the pre-existing provider-mismatch tests. |
New findings this round
No criticals, majors, or minors of consequence. Only the following optional low/nit items:
- Low / info —
src/xagent/core/tools/core/audio_tool.py:clone_tts_voice's except block returns rawstr(e)to the caller, which can include an unredacted workspace-resolved absolute file path surfaced by_reference_audio_metadata(not routed throughredact_sensitive_text, unlike the SDK-error path inclone_voice). This is not new to this PR — the same "resolve to absolute path + rawstr(e)passthrough" pattern already exists pre-PR intranscribe_audioandsynthesize_speechin the same file. Local path only, no credentials. Best handled as a cross-tool fix (redact inside_reference_audio_metadata, or centrally in the except handlers) rather than a single-spot patch. Not a blocker. - Nit —
src/xagent/core/tools/core/audio_tool.py(~L1160-1164):clone_tts_voice's except block recomputesactual_model_idwith simpler logic than_get_provider_tts_model(misses the default-modelmodel_namefallback match), so an error response'smodel_usedfield could report"default"instead of the precise model id in a narrow edge case (default model configured but not registered under its own id in_tts_models). Affects only an informational field in error responses, not behavior. - Nit —
src/xagent/core/model/tts/elevenlabs.py(~L466-468):clone_voicere-parsesPath(reference_audio).nameafter_reference_audio_filealready resolved the path. Harmless (values are always equal on the reachable path); not worth changing unless_reference_audio_file's return signature grows to expose the name.
Note on two investigated-but-rejected "shrink" ideas
Two candidate simplification findings — removing the post-read empty-file checks in _reference_audio_file and _get_or_create_cloned_voice on the grounds that _reference_audio_metadata already checks stat().st_size == 0 — were investigated and should not be applied. There is a genuine (if narrow) TOCTOU race: in _get_or_create_cloned_voice an async with self._voice_clone_lock yield point sits between the stat-based check and the actual read, widening the window in which the file could be truncated or replaced. These checks are defense-in-depth, not dead code. Flagging so they are not re-raised in a future round.
On the two declined simplification suggestions — verdict: WAIVED (legitimate)
The author declined (1) extracting a shared useAuthenticatedPreviewUrl-style hook for the InlineAudioPreview/InlineImagePreview blob-fetch effect, and (2) extracting a shared _resolve_provider_tts_model_or_error helper for the list_tts_voices/clone_tts_voice validation blocks.
I agree these should be waived, not pushed as required follow-ups. Both are pure code-cleanliness suggestions — no correctness, security, or performance impact — and "defer a behavior-neutral refactor to keep this PR's review surface focused" is a sound engineering call, not an evasion. The reasoning holds up on its own terms:
- The refactors touch code that is shared with the image-preview and TTS-listing paths, so doing them here would pull unrelated components into a PR whose stated scope is voice cloning and audio previews. That widens the blast radius precisely when the PR is already two review rounds deep and otherwise converged.
- The duplication is bounded and visible (two call sites each), not a metastasizing pattern. It carries low ongoing cost and is cheap to extract later in a dedicated PR where the shared abstraction can be reviewed on its own merits — including the design question of whether audio and image previews should actually converge on one hook, which is easy to get subtly wrong when rushed into an unrelated change.
- Nothing about merging as-is makes the future refactor harder; it is not accruing debt that compounds.
The only thing I would ask is that these not simply evaporate: a short follow-up issue capturing both extractions keeps them from being silently lost. That is a nice-to-have, not a merge gate.
Testing performed
- Backend:
pytest tests/core/model/tts/test_elevenlabs.py tests/core/tools/test_audio_tool.py— all pass (27 + 25 verified individually across runs; consistent with the PR's 52-backend-test claim). - Frontend:
npx vitest run src/components/file/inline-file-preview.test.tsx(21/21) plus other targeted suites — all pass (consistent with the PR's 36-frontend-tests-this-round / 57-cumulative claim).
Recommendation
Approve. All 7 prior findings are fixed with regression coverage, the design remains sound, and the only open items this round are optional low/nit polish (the raw-path passthrough is a pre-existing cross-tool concern better handled separately). The two declined simplifications are legitimately waived. Optional: file a follow-up issue for the shared hook/helper extractions and the cross-tool path-redaction cleanup so they aren't lost. None of these block merge.
d4f90d9 to
ef29c91
Compare
Summary
clone_tts_voicetool for persistent ElevenLabs voice IDslist_tts_voicesprovider-aware, preserve ElevenLabs voice categories, and improve invalid voice ID guidanceWhy
ElevenLabs voice IDs are account-specific opaque identifiers. Agents could previously invent generic voice names, and the audio tool could not create reusable provider-side clones. Generated MP3 workspace links were also interpreted as images in legacy markdown output, leaving users unable to play or open them.
Impact
Users can now clone an ElevenLabs voice either temporarily for one task or persistently for later reuse, discover exact voice IDs from their account, and play generated audio directly in the task UI. The public tool surface remains explicitly provider-scoped while only ElevenLabs is supported.
Validation
PYTHONPATH=src pytest -q tests/core/model/tts/test_elevenlabs.py tests/core/tools/test_audio_tool.py(48 passed)npm run type-check