Conversation
|
test |
|
placeholder being replaced |
|
edit test |
Code Review: xAI ASR and TTS ExtensionsGood overall implementation - both extensions follow the TEN Framework patterns well, have bounded reconnect logic, proper fatal/non-fatal error classification, config redaction, and solid test coverage (17 ASR + 23 TTS tests). The graph wiring and README updates are clean. Below are the issues worth addressing before merge, ordered by severity. |
Code Review: xAI ASR and TTS ExtensionsGood overall implementation - both extensions follow the TEN Framework patterns well, have bounded reconnect logic, proper fatal/non-fatal error classification, config redaction, and solid test coverage (17 ASR + 23 TTS tests). The graph wiring and README updates are clean. Below are the issues worth addressing before merge, ordered by severity. Bugs / Correctness1. XAIASRConfig.to_json() returns Python repr, not JSON In 2. _message_handler re-fires on_open() on duplicate transcript.created In 3. Redundant copy.deepcopy after json.loads in _flush_buffered_audio_frames
4. _ensure_connection does not verify the socket is still open (TTS)
API Key Validation Leaks Test Logic into Production5. xai_tts_python/config.py validate() accepts "test" prefix The validation accepts Missing Manifest Properties6. xai_asr_python/manifest.json omits dump, dump_path, and finalize_timeout_ms These three properties are present in Fatal-Error String Matching is Fragile7. ASR _is_fatal_connection_error and on_error Both use substring scans on free-form error strings, including the literal string Minor / Style8. Missing license header in 9. Missing newlines at end of files: 10. 11. Duplicated 12. Stale test log string in Summary
The top priorities before merge are items 1 (invalid JSON from |
|
PR Review: xAI ASR and TTS Extensions - test post |
|
PR Review: xAI ASR and TTS Extensions. Issues to fix: (1) on_open called twice in recognition.py - both start() and _message_handler call the callback on transcript.created, causing duplicate metrics and buffer flushes if server re-sends; (2) buf potentially unbound in xai_asr_python send_audio - if lock_buf raises, buf is undefined in the except handler, initialize buf=None before the try block; (3) property_en_hotwords.json is identical to property_en.json with no hotword impl - remove or implement. Issues to address: (4) XAIASRConfig.validate() shadows Pydantic class method, rename to validate_config(); (5) TTS api key startswith('test') check is too broad; (6) turn_status values 1 and 2 are undocumented magic numbers; (7) isinstance str check for text_normalization is dead code since the field is typed bool; (8) missing trailing newlines in requirements.txt and tests/bin/start; (9) test_basic.py copy-paste error says Deepgram not xAI. Positives: great reconnect logic with backoff and fatal escalation, API key redaction, _contains_spoken_content guard, comprehensive test coverage (timeout, flush, punctuation, reconnect-after-error, auth failure), clean agent.py update for the new event types. Generated by Claude Code |
Code Review: xAI ASR + TTS ExtensionsGreat overall structure. The two extensions follow existing repo conventions closely and come with thorough test coverage (17 ASR + 23 TTS tests). The reconnection logic, error classification, and sensitive-data redaction are all solid. A few issues worth addressing before merge. Bugs1. Potential If buf = frame.lock_buf()
try:
audio_data = bytes(buf)
...
return True
except Exception as e:
self.ten_env.log_error(f"Error sending audio: {e}")
return False
finally:
frame.unlock_buf(buf)Code Quality2.
3.
4. Empty The override only calls 5.
Completeness / Consistency6.
7. Both files in Minor Nits8. Misleading validation error message for TTS API key (
9. Copy-paste error in TTS test ( The basic test sends Summary
|
| def _calculate_request_event_interval_ms(self) -> int: | ||
| if self.sent_ts is None: | ||
| return 0 | ||
| return int((datetime.now() - self.sent_ts).total_seconds() * 1000) |
There was a problem hiding this comment.
The interval time is formulate is "current time - first audio receive time", not first request send time
| code=ModuleErrorCode.NON_FATAL_ERROR, | ||
| vendor_info=ModuleErrorVendorInfo(vendor=self.vendor()), | ||
| ) | ||
| await self._finalize_request(TTSAudioEndReason.ERROR, error=error) |
There was a problem hiding this comment.
_finalize_request only for handling last request of the turn.
if t.text_input_end:
| if start_ms <= 0: | ||
| start_ms = int(datetime.now().timestamp() * 1000) | ||
|
|
||
| transcript_result = TTSTextResult( |
There was a problem hiding this comment.
TTSTextResult is not necessary. Actually, we only use it when we can get a word-level timestamp. if this tts vendor cannot provide, we can remove it
| message=e.body, | ||
| ), | ||
| ) | ||
| await self._finalize_request(TTSAudioEndReason.ERROR, error=error) |
There was a problem hiding this comment.
_finalize_request only for handling last request of the turn.
if t.text_input_end:
Code Review: PR #2146 — Add xAI ASR and TTS ExtensionsOverviewThis PR adds two well-structured TEN extensions — IssuesBug:
|
Summary
xai_asr_pythonandxai_tts_pythonTEN extensionsvoice-assistantexampleWhat Changed
ai_agents/agents/ten_packages/extension/xai_asr_pythonaudio.donefinalize flowai_agents/agents/ten_packages/extension/xai_tts_pythonAsyncTTS2BaseExtensionai_agents/agents/examples/voice-assistant/tenapp/manifest.jsonai_agents/agents/examples/voice-assistant/tenapp/property.jsonvoice_assistant_xai_asrvoice_assistant_xai_ttsvoice_assistant_xai_fullai_agents/agents/examples/voice-assistant/README.mdai_agents/.env.exampleXAI_API_KEYTesting
23 passed17 passedvoice_assistant_xai_fullNotes
docs/progressive-disclosurein TEN Frameworkimprove/tts-extension-guidein TEN Portal