Skip to content

Conversation

@nitin4real
Copy link
Contributor

Screenshot 2026-01-16 at 1 41 20 AM

@claude
Copy link

claude bot commented Jan 15, 2026

PR Review: Add Murf TTS Extension

Strengths

  • Excellent test coverage (2100+ lines across 6 test files)
  • Clean architecture with proper separation of concerns
  • Good error handling using ModuleVendorException pattern
  • Proper security for sensitive data in logging

Critical Issues

1. API Key Exposure (murf_tts.py:72) - API key passed as query param in WebSocket URL will be logged by servers/proxies. Use headers if possible.

2. Race Condition (extension.py:123-217) - current_request_id accessed without synchronization across async contexts. Use asyncio.Lock.

3. Memory Leak (extension.py:139-143) - asyncio.create_task() without storing reference. Store tasks or await directly.

Important Issues

4. Type Annotation (extension.py:47) - Use: response_msgs: asyncio.Queue[...] = asyncio.Queue()

5. Missing Validation (config.py:22-32) - update_params() needs type checking before setattr()

6. Chinese Comment (extension.py:340) - Replace with English

7. WebSocket Reconnection (murf_tts.py:94-155) - Loop continues indefinitely after max retries

Code Quality

  • Extract magic number max_size=100_000_000 to constant
  • Add type info in exception logging
  • Document INVALID_TEXT_LIST rationale
  • Add queue size limits (maxsize=100)

Documentation

  • Complete README.md (currently has placeholders)
  • Document params in manifest.json
  • Add header validation docs for first chunk

Must Fix Before Merge

  1. API key security issue
  2. Race condition fix
  3. Orphaned asyncio tasks
  4. Chinese comment

Great work overall! Strong implementation with excellent tests. Main concerns are thread safety and security.

@claude
Copy link

claude bot commented Jan 15, 2026

PR Review: Murf TTS Extension

This PR adds a new Murf TTS extension with WebSocket-based streaming support. The implementation follows TEN Framework patterns and includes comprehensive test coverage (2,044 lines of tests across 6 test files).

Strengths

  • Correct base class usage (AsyncTTS2BaseExtension)
  • Well-structured with clear separation of concerns
  • Comprehensive error handling with vendor-specific exceptions
  • Excellent test coverage (6 test files)
  • Proper security practices (API key in headers, sensitive data masking)
  • Good features (TTFB metrics, audio dumping, reconnection logic)

Critical Issues to Fix

1. Race Condition in get_current_request_id (extension.py:130-134)

The value is used after the lock is released, creating a TOCTOU race condition. Either hold the lock for the entire critical section or simplify by removing the lock if string assignment is atomic.

2. Unsafe Queue Clearing in cancel_tts (murf_tts.py:509-513)

The empty() check and get_nowait() are not atomic. Use: while True: try: self.response_msgs.get_nowait() except asyncio.QueueEmpty: break

3. Unbounded Task Growth (extension.py:137-139)

PCM write tasks could grow unbounded. Add backpressure and limit max concurrent tasks.

Medium Priority Issues

  1. Inconsistent empty text handling - only complete when text_input_end is true
  2. Missing WebSocket health checks - add periodic ping/pong
  3. Potential memory leak in recorder_map - add size limit or time-based cleanup
  4. No timeout on WebSocket send operations - use asyncio.wait_for

Minor Improvements

  1. Extract magic number 44 to MURF_AUDIO_HEADER_SIZE constant
  2. Use Enum for event types instead of int
  3. Use _format_exception helper consistently
  4. Add stricter config validation with Pydantic field_validator
  5. More consistent log levels

Overall Assessment: 4/5 stars

  • Architecture: 5/5
  • Code Quality: 4/5
  • Testing: 5/5
  • Security: 4/5
  • Performance: 4/5

Recommendation: Approve with required changes to fix critical concurrency issues. Great work nitin4real! Once the race conditions are addressed, this will be a solid addition to the TEN Framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants