You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review — fix: murf tts voice config with each tts request
Thanks for the fix. The core change (filter params into voice_config / advanced_settings, send voice config with each text chunk) looks sensible. A few items worth addressing before merge:
Correctness / API contract
Wire-format key rename from voiceConfig → voice_config (murf_tts.py:205). This changes the payload sent to Murf's websocket. Please confirm against Murf's current API spec and call it out in the PR description — if you deploy this while Murf still expects voiceConfig, TTS will silently break. Worth a note in the PR body and, ideally, a link to Murf's docs.
manifest.json dropped style/rate/pitch/variation from the schema, but VOICE_CONFIG_PARAM_KEYS in murf_tts.py still lists them. Result: users configuring those keys via property.json/runtime config may be rejected by the manifest validator before the code ever sees them. Either restore the manifest fields (plus pronunciation_dictionary) so configs can carry them, or drop them from VOICE_CONFIG_PARAM_KEYS too. The new test test_get_voice_config_from_params_filters_to_supported_keys passes a style in params, but that path is never exercised through real manifest validation, so the test gives a false sense of coverage.
Dual-alias keys in VOICE_CONFIG_PARAM_KEYS (voice_id + voiceId, multiNativeLocale + multi_native_locale, pronunciationDictionary + pronunciation_dictionary). If a user sets both, both land in the payload and Murf may reject it or prefer one unpredictably. Either document which form is canonical, or normalize (e.g. snake → camel) before sending.
Performance
_send_text_internal now attaches the full filtered voice_config to every chunk. For streaming responses with many small chunks this meaningfully inflates per-message size (especially if pronunciation_dictionary is present). Worth verifying Murf's API actually requires per-chunk voice config — or whether one-shot on connect + re-send only when it changes would suffice.
Logging
Many log_debug → log_info promotions throughout murf_tts.py and extension.py (lifecycle, "KEYPOINT Sending voice config...", websocket connect/close, client init/close). This will noticeably increase INFO-level log volume in production. If this was for local debugging, revert before merge; if intentional, consider whether connect/close chatter truly warrants INFO.
Inconsistent: _send_text_internal's "KEYPOINT Sending text to MURF TTS" remained log_debug while _send_voice_config's equivalent was promoted to log_info.
Tests
tests/test_voice_config.py uses a hardcoded Path(__file__).resolve().parents[6] sys.path hack. This is fragile — any directory rename breaks it. Existing tests in this extension rely on conftest.py + normal package imports; please follow the same pattern (or add a local conftest.py rather than path manipulation).
Tests use object.__new__(MurfTTSynthesizer) to bypass __init__. It works, but couples tests tightly to class internals; consider a lightweight constructor/factory if this pattern spreads.
Missing coverage for: config.params is None, conflicting aliases (voice_id + voiceId both set), and an empty voice_config in _send_text_internal.
Minor / nits
.env.example: extra blank line before # murf tts, and file still lacks a trailing newline.
manifest.json bump 0.1.0 → 0.1.1 is fine for a bug fix. If the schema removals are intentional (a breaking config change), this arguably warrants 0.2.0 under semver.
Blank line after class MurfTTSynthesizer: before def __init__ is unconventional — harmless, but other classes in this file don't do it.
Nice-to-have
A brief README note about the new advanced_settings params (min_buffer_size, max_buffer_delay_in_ms) and the per-chunk voice_config behavior would help downstream users.
Overall the direction looks right — the main blockers for me are confirming the voiceConfig → voice_config wire change is correct, and reconciling the manifest schema with VOICE_CONFIG_PARAM_KEYS.
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
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.
No description provided.