Repository navigation
fix(cli): play mp3 TTS on Linux via real decoders, not paplay (#1138) - #1180
Conversation
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 3 (src/cli/utils/audioPlayer.ts, src/lib/types/cli.ts, test/continuous-test-suite-bugfixes.ts)
New issues raised:
- 🔒 1 CRITICAL — potential shell injection through unescaped
filePathpassed to media players / PowerShell. ⚠️ 2 MAJOR — internal use of the deprecatedAudioFormatalias instead ofTTSAudioFormat; non-ENOENT player failures are silently swallowed and retried.- 💡 3 MINOR/SUGGESTION —
getAudioExtensionduplicates existing format mapping; temp filename is predictable and can collide; test could tighten the first-candidate assertion.
Decision: REQUEST CHANGES
The CRITICAL shell-injection risk in getPlayerCandidates / playAudio blocks the PR. Even though filePath is normally generated internally, getPlayerCandidates is exported and platform-injectable, and several invoked players interpret shell metacharacters. Please sanitize/validate filePath before building command arguments, and properly escape the path for the PowerShell invocation.
Also please address the MAJOR issues:
- Replace
AudioFormatwithTTSAudioFormatinaudioPlayer.ts(AudioFormatis a deprecated backward-compat alias insrc/lib/types/tts.ts). - Distinguish recoverable (missing binary) from unrecoverable (permission denied, decode failure, signal) player errors instead of unconditionally retrying every non-ENOENT failure.
Once these are fixed, the remaining MINOR items can be addressed at your discretion.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
ee9e9a7 to
16e3600
Compare
✅ Verification & gap-bridging (hardened)Rebased onto current Test gap → closed
Why not a full end-to-end
|
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesTTS playback
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Documentation Validation Results🚀 Documentation validation passed!
📦 Build artifact uploaded successfully. Ready for deployment preview. Commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/continuous-test-suite-bugfixes.ts (1)
3858-3934: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise the actual retry loop.
These tests validate candidate lists and messages, but not that
playAudiocontinues after a failed player. Add an injectable command executor or mockexecFileand cover first-fails/second-succeeds plus all-fail attempt aggregation.🤖 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 `@test/continuous-test-suite-bugfixes.ts` around lines 3858 - 3934, Extend the audio playback tests around playAudio to exercise the actual retry loop by injecting or mocking the command executor/execFile. Add coverage where the first candidate fails and the second succeeds, then verify all candidates failing produces an aggregated error containing each attempted player and its failure. Keep the existing candidate-order and message assertions unchanged.
🤖 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 `@docs/features/tts.md`:
- Around line 318-320: Update the WAV playback statements in the TTS
documentation to avoid implying zero dependencies or out-of-the-box support.
State that WAV requires an available aplay, paplay, or ffplay executable in
PATH, while clarifying that it does not require a compressed-format decoder.
In `@src/cli/utils/audioPlayer.ts`:
- Around line 153-157: Update the Linux non-WAV error message in the audio
playback format branch to apply the “paplay/aplay cannot decode” limitation and
mpg123 recommendation only when format is MP3. Preserve the existing
format-specific guidance for OGG/Opus from the earlier playback logic, while
retaining the WAV message and fallback player recommendations for other formats.
---
Nitpick comments:
In `@test/continuous-test-suite-bugfixes.ts`:
- Around line 3858-3934: Extend the audio playback tests around playAudio to
exercise the actual retry loop by injecting or mocking the command
executor/execFile. Add coverage where the first candidate fails and the second
succeeds, then verify all candidates failing produces an aggregated error
containing each attempted player and its failure. Keep the existing
candidate-order and message assertions unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eafd20ef-fd55-428b-a220-65bb1fc91dc4
📒 Files selected for processing (4)
docs/features/tts.mdsrc/cli/utils/audioPlayer.tssrc/lib/types/cli.tstest/continuous-test-suite-bugfixes.ts
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 4 (docs/features/tts.md, src/cli/utils/audioPlayer.ts, src/lib/types/cli.ts, test/continuous-test-suite-bugfixes.ts)
New issues raised this run:
- 🔒 1 CRITICAL — potential shell injection through unescaped
filePathpassed to media players / PowerShell in the exportedgetPlayerCandidateshelper. ⚠️ 2 MAJOR — internal use of the deprecatedAudioFormatalias instead ofTTSAudioFormat; external player processes have no timeout, so a hung decoder can block the CLI indefinitely.- 💡 1 MINOR — temp filename uses only
Date.now()and can collide under rapid/concurrent calls.
Existing review comments already covered: I did not duplicate points already raised by prior reviewers (deprecated AudioFormat, temp-file predictability, swallowed non-ENOENT errors, test coverage gaps, docs wording). The issues above are the ones I found independently or that remain unaddressed.
Decision: REQUEST CHANGES
The CRITICAL shell-injection risk in getPlayerCandidates / playAudio blocks the PR. Even though filePath is normally generated internally, getPlayerCandidates is exported and platform-injectable, and several invoked players interpret shell metacharacters. Please sanitize/validate filePath before building command arguments, and properly escape the path for the PowerShell invocation.
Also please address the MAJOR issues:
- Replace
AudioFormatwithTTSAudioFormatinaudioPlayer.ts(AudioFormatis a deprecated backward-compat alias insrc/lib/types/tts.ts). - Add a timeout to
execFileAsync(e.g.timeout: 30_000) so a hungffplay/mpv/cvlccannot block the CLI forever.
Once these are fixed, the remaining MINOR item can be addressed at your discretion.
paplay (PulseAudio) and aplay (ALSA) only decode libsndfile/PCM formats and cannot decode mp3, yet --tts-format defaults to mp3 — so 'neurolink generate ... --tts --tts-play' silently failed playback on Linux with a misleading 'install PulseAudio/ALSA' message. audioPlayer now builds an ordered candidate list per platform+format and tries each until one succeeds. On Linux, compressed formats (mp3/ogg/opus) lead with real decoders (ffplay/mpv/mpg123 for mp3/cvlc); paplay/aplay remain last-resort fallbacks and still handle wav. The failure message is format-aware (names ffmpeg/mpv/mpg123/VLC or --tts-format wav). getPlayerCandidates(file, format, platform) and buildPlaybackErrorMessage are exported and platform-injectable for deterministic tests. Bugfixes-suite tests assert: Linux mp3 leads with a decoder (not paplay/aplay), mpg123 offered for mp3 not opus, wav routes to aplay, macOS uses afplay; and the user-facing error names the decoders + wav fallback rather than the misleading PulseAudio message. Docs (docs/features/tts.md) updated with the Linux decoder requirement.
16e3600 to
05d60f9
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Reviewed all 4 changed files in PR #1180.
Files examined:
docs/features/tts.mdsrc/cli/utils/audioPlayer.tssrc/lib/types/cli.tstest/continuous-test-suite-bugfixes.ts
New issues raised this run: None.
Existing review threads: All prior review comments are resolved (including the AudioFormat → TTSAudioFormat migration, PowerShell single-quote escaping, temp-file collision avoidance, player timeout wiring, and format-aware error messages). No duplicates or re-raises.
Assessment: This is a focused CLI-only fix for Linux mp3 TTS playback. The implementation correctly routes compressed formats through real decoders before falling back to paplay/aplay, uses execFile (not a shell) for player invocation, escapes PowerShell single quotes, caps player execution with a timeout, and includes deterministic regression tests. No blocking security, architecture, or backward-compatibility concerns remain.
Approving.
|
🎉 This PR is included in version 9.94.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What & why
Fixes #1138 —
--tts-playmp3 playback is broken on Linux.src/cli/utils/audioPlayer.tsrouted all non-wav audio topaplay, but PulseAudio'spaplay(and ALSA'saplay) only decode libsndfile/PCM formats (WAV/FLAC/Ogg/AIFF) — they cannot decode mp3/AAC. Since--tts-formatdefaults to mp3, the defaultneurolink generate "..." --tts --tts-playfails playback on Linux, with a misleading "Install PulseAudio (paplay) or ALSA (aplay)" message (PulseAudio is present; the format is simply undecodable).Fix
playAudionow builds an ordered candidate list per platform + format and tries each until one succeeds (a missing binary or a decode failure advances to the next).ffplay(ffmpeg) →mpv→mpg123(mp3 only) →cvlc(VLC) — thenpaplay/aplayas last-resort fallbacks (which still handle wav).aplayfirst, thenpaplay, thenffplay.afplaydecodes everything; PowerShell paths preserved).--tts-format wav, and lists what was tried.Testing / proof
getPlayerCandidates(file, format, platform)is exported and platform-injectable, so the behavior is deterministically testable off-Linux.paplay/aplay);mpg123is offered for mp3 but not opus; Linux wav still routes toaplayfirst; macOS usesafplay.pnpm run check✅ ·pnpm run lint✅ (0 errors) · bugfixes suite 90/90 PASS ·pnpm run build:cli✅.Notes
src/lib/types/cli.tsasCliAudioPlayerCommandto satisfy the types-location lint rule.Summary by CodeRabbit
Bug Fixes
Documentation