Repository navigation
feat(cli): add audio playback support for TTS output - #982
Conversation
TTS-025: Add platform-specific audio playback when --tts-play flag is used. Supports macOS (afplay), Linux (paplay/aplay), and Windows (PowerShell). Playback failures are non-fatal and display a helpful warning message.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded audio playback capability to the CLI's TTS system. Extended Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command Handler
participant Player as Audio Player Utility
participant TempFS as Temp File System
participant OS as OS Audio System<br/>(paplay/aplay/afplay)
CLI->>Player: playAudio(buffer, format)
activate Player
Player->>TempFS: Write buffer to unique temp file
activate TempFS
TempFS-->>Player: File path
deactivate TempFS
Player->>Player: Select platform-specific player command
Player->>OS: execFile(playerCommand, [filepath])
activate OS
alt Playback Successful
OS-->>Player: Audio played
else Player Binary Missing
Player->>Player: Detect ENOENT error
alt Linux with Fallback
Player->>OS: Try fallback aplay command
OS-->>Player: Audio played (or error)
else No Fallback
Player-->>Player: Throw descriptive error
end
end
deactivate OS
Player->>TempFS: Cleanup temp file (finally block)
deactivate Player
Player-->>CLI: Promise resolved/rejected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
✅ 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 |
🤖 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 |
There was a problem hiding this comment.
Pull request overview
Adds CLI support to automatically play generated TTS audio (--tts-play) using platform-specific native tools, while keeping playback failures non-fatal.
Changes:
- Added a new cross-platform audio playback utility (
playAudio,getAudioExtension). - Extended CLI TTS handling to optionally play audio after generation when
--tts-playis set. - Updated streaming-mode messaging to account for
--tts-playas well.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/cli/utils/audioPlayer.ts |
New platform-specific playback helper that writes a temp file, executes a player, and cleans up. |
src/cli/factories/commandFactory.ts |
Integrates playback into handleTTSOutput and adjusts streaming warning gating for --tts-play. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Play audio if --tts-play is provided | ||
| if (shouldPlay) { | ||
| try { | ||
| if (!options.quiet) { | ||
| logger.always(chalk.blue("Playing audio...")); | ||
| } | ||
| await playAudio(audio.buffer, audio.format); | ||
| } catch (err) { | ||
| // Non-fatal: warn but don't crash | ||
| logger.always( | ||
| chalk.yellow(`Audio playback failed: ${(err as Error).message}`), | ||
| ); | ||
| logger.always( | ||
| chalk.yellow( | ||
| " Tip: Save the audio with --tts-output <file> and play manually.", | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
New --tts-play behavior is introduced here but doesn’t appear to be covered by the existing CLI TTS integration tests (e.g., test/continuous-test-suite-tts.ts covers --tts-output but not --tts-play). Please add coverage that at least verifies the flag is recognized and that playback failures remain non-fatal (ideally by stubbing/guarding actual playback in CI).
| case "linux": | ||
| if (format === "wav") { | ||
| return { command: "aplay", args: [filePath] }; | ||
| } | ||
| return { command: "paplay", args: [filePath] }; | ||
|
|
There was a problem hiding this comment.
On Linux, selecting paplay for non-wav formats is likely incorrect: paplay/aplay generally handle PCM/WAV (and aplay is WAV/RAW only) and won’t reliably play the default mp3/ogg/opus TTS output. This makes --tts-play fail on many Linux setups. Consider switching to tools that actually decode these formats (e.g., ffplay, mpg123, ogg123, play/sox), or constrain Linux playback to wav and emit a clear error prompting --tts-format wav when --tts-play is used.
| command: "powershell", | ||
| args: [ | ||
| "-NoProfile", | ||
| "-Command", | ||
| `(New-Object System.Media.SoundPlayer '${filePath}').PlaySync()`, | ||
| ], |
There was a problem hiding this comment.
The PowerShell -Command strings embed filePath inside single quotes. If os.tmpdir() (or user profile path) contains an apostrophe, this will break the command and can lead to unexpected PowerShell parsing. Escape single quotes for PowerShell literals (or pass the path via a parameter / use -LiteralPath) before embedding it.
| command: "powershell", | ||
| args: [ | ||
| "-NoProfile", | ||
| "-Command", | ||
| `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 1; while ($player.playState -eq 3) { Start-Sleep -Milliseconds 100 }; $player.close()`, | ||
| ], |
There was a problem hiding this comment.
Same quoting issue here: filePath is interpolated into a single-quoted PowerShell string for the WMPlayer COM object. Paths containing ' will break the script; escape appropriately or pass as an argument to PowerShell instead of string interpolation.
| const ext = getAudioExtension(format); | ||
| const tempFile = path.join(os.tmpdir(), `nl-tts-${Date.now()}.${ext}`); | ||
|
|
There was a problem hiding this comment.
Temp file names based only on Date.now() can collide if playAudio() is called multiple times in the same millisecond (e.g., parallel requests), causing races between write/play/unlink. Consider using a stronger unique suffix (e.g., crypto.randomUUID()), or fs.promises.mkdtemp() to create a dedicated temp directory per playback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/cli/utils/audioPlayer.ts (1)
26-39: Unreachable default branch — simplify.If
AudioFormatis the strict union"mp3" | "wav" | "ogg" | "opus", the switch/default is dead code and the extension equals the format string. Consider justreturn format;(with an exhaustive check if you want compile-time safety).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/utils/audioPlayer.ts` around lines 26 - 39, The switch in getAudioExtension is redundant because AudioFormat is the union "mp3" | "wav" | "ogg" | "opus"; replace the switch with a direct return of format in getAudioExtension to simplify, or if you want compile-time exhaustiveness add a type guard/assertion (e.g., a never-based exhaustCheck) to ensure format is one of the expected values before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 963-1005: The save-audio block currently calls handleError (which
exits) on save failures, preventing playback when both --tts-output
(ttsOutputPath) and --tts-play (shouldPlay) are used; fix by rearranging the
logic to attempt playback first (call playAudio with audio.buffer/format and log
via options.quiet/logger.always) and then save to disk (call saveAudioToFile) so
a save failure won't short-circuit play, or alternatively make the save failure
non-fatal in this context by catching saveAudioToFile errors and logging a
warning instead of invoking handleError when shouldPlay is true; update
references to saveAudioToFile, handleError, playAudio, ttsOutputPath,
shouldPlay, and options.quiet accordingly.
In `@src/cli/utils/audioPlayer.ts`:
- Line 116: The temp filename for playAudio (tempFile variable created via
path.join(os.tmpdir(), `nl-tts-${Date.now()}.${ext}`)) can collide when
Date.now() is identical; change the temp-file creation to produce a
cryptographically-unique name (e.g., include crypto.randomUUID() or use
fs.mkdtemp/ mkdtempSync to create a unique temp directory and then write the
file inside it) and update imports accordingly; ensure the unique name is used
wherever tempFile is referenced and cleanup/unlink logic still targets the
generated unique path.
- Around line 130-152: The current fallback in the execFileAsync error handler
wrongly tries ALSA's aplay when a missing paplay occurs even for non-wav formats
(mp3/ogg/opus) — change the logic in the error handling inside audio playback
(the block handling err.code === "ENOENT" where `command`, `tempFile`,
`fallbackError` and `execError` are in scope) so that you only attempt `aplay`
as a fallback when the audio format is WAV (use the same format check used by
getPlayerCommand); for non-wav formats, do not call `aplay` — instead surface a
clear error telling the user to install `paplay` or provide an appropriate
decoder (e.g., `ffplay`/`mpg123`) or implement a fallback that invokes a decoder
for compressed formats; ensure thrown errors reference the original
`execError`/`fallbackError` as cause and update the error message to explicitly
mention the required player for the detected format.
- Around line 64-82: The PowerShell commands currently interpolate filePath into
the -Command string using single quotes (e.g., '(New-Object
System.Media.SoundPlayer '${filePath}').PlaySync()'), which breaks when the path
contains a single quote; change the Windows branch to pass the path as a
separate argument to powershell instead of embedding it: build a -Command script
that accepts a parameter (e.g., param($p) ...) or references $args[0], and then
supply filePath via the args array (add it after the -Command entry) so execFile
invokes powershell with the path as data rather than as interpolated script
text; update both the wav branch (SoundPlayer) and the WMPlayer branch
accordingly, referencing the case "win32", format, and filePath symbols to
locate where to change.
---
Nitpick comments:
In `@src/cli/utils/audioPlayer.ts`:
- Around line 26-39: The switch in getAudioExtension is redundant because
AudioFormat is the union "mp3" | "wav" | "ogg" | "opus"; replace the switch with
a direct return of format in getAudioExtension to simplify, or if you want
compile-time exhaustiveness add a type guard/assertion (e.g., a never-based
exhaustCheck) to ensure format is one of the expected values before returning.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b60978b-e179-4d90-b885-e1403360eec4
📒 Files selected for processing (2)
src/cli/factories/commandFactory.tssrc/cli/utils/audioPlayer.ts
| // Save audio to file if --tts-output is provided | ||
| if (ttsOutputPath) { | ||
| try { | ||
| const saveResult = await saveAudioToFile(audio, ttsOutputPath); | ||
|
|
||
| if (saveResult.success) { | ||
| if (!options.quiet) { | ||
| logger.always( | ||
| chalk.green( | ||
| `🔊 Audio saved to: ${saveResult.path} (${formatFileSize(saveResult.size)})`, | ||
| ), | ||
| if (saveResult.success) { | ||
| if (!options.quiet) { | ||
| logger.always( | ||
| chalk.green( | ||
| `🔊 Audio saved to: ${saveResult.path} (${formatFileSize(saveResult.size)})`, | ||
| ), | ||
| ); | ||
| } | ||
| } else { | ||
| handleError( | ||
| new Error(saveResult.error || "Failed to save audio file"), | ||
| "TTS Output", | ||
| ); | ||
| } | ||
| } else { | ||
| handleError( | ||
| new Error(saveResult.error || "Failed to save audio file"), | ||
| "TTS Output", | ||
| } catch (error) { | ||
| handleError(error as Error, "TTS Output"); | ||
| } | ||
| } | ||
|
|
||
| // Play audio if --tts-play is provided | ||
| if (shouldPlay) { | ||
| try { | ||
| if (!options.quiet) { | ||
| logger.always(chalk.blue("Playing audio...")); | ||
| } | ||
| await playAudio(audio.buffer, audio.format); | ||
| } catch (err) { | ||
| // Non-fatal: warn but don't crash | ||
| logger.always( | ||
| chalk.yellow(`Audio playback failed: ${(err as Error).message}`), | ||
| ); | ||
| logger.always( | ||
| chalk.yellow( | ||
| " Tip: Save the audio with --tts-output <file> and play manually.", | ||
| ), | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| handleError(error as Error, "TTS Output"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether handleError exits the process or just logs
ast-grep --pattern $'export function handleError($$$) { $$$ }'
rg -nP -C2 '\bhandleError\b' src/cli/errorHandler.tsRepository: juspay/neurolink
Length of output: 282
🏁 Script executed:
cat -n src/cli/errorHandler.tsRepository: juspay/neurolink
Length of output: 2640
🏁 Script executed:
# Find the TTS handler context and session setup
rg -n "globalSession\|getCurrentSessionId" src/cli/factories/commandFactory.ts | head -20Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check the broader context around the TTS code to see if we're in a session
sed -n '930,965p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 1137
🏁 Script executed:
# Find where handleTTSOutput is called
rg -n "handleTTSOutput" src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 173
🏁 Script executed:
# Check the broader command execution context to understand session lifecycle
rg -n "globalSession\|getCurrentSessionId" src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check context around the handleTTSOutput call at line 2480
sed -n '2450,2490p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 1513
🏁 Script executed:
# Also check if there's any session management around the generate command execution
rg -n "async.*generate|async.*execute" src/cli/factories/commandFactory.ts | head -10Repository: juspay/neurolink
Length of output: 710
🏁 Script executed:
# Check the executeGenerate method to see session context
sed -n '2511,2600p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 3272
🏁 Script executed:
# Search for where globalSession is initialized/managed
rg -n "globalSession\|getCurrentSessionId" src/cli --include="*.ts" | head -20Repository: juspay/neurolink
Length of output: 501
🏁 Script executed:
# Fix the rg command syntax and search for globalSession initialization
rg -n "globalSession" src/cli --type ts | head -20Repository: juspay/neurolink
Length of output: 1762
🏁 Script executed:
# Check CLI entry point and main command handler
rg -n "globalSession\|executeGenerate\|executeStream" src/cli/index.tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check parser.ts to understand session initialization
sed -n '1,100p' src/cli/parser.tsRepository: juspay/neurolink
Length of output: 4039
🏁 Script executed:
# Check where commands are dispatched from
rg -n "executeGenerate\|executeStream\|handleError" src/cli/parser.ts -A 3 -B 3Repository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if session ID is set anywhere for single commands vs loop
rg -n "setLoopSession\|setSessionId" src/lib/session/globalSessionState.tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check globalSessionState to understand when session ID is set
sed -n '1,100p' src/lib/session/globalSessionState.tsRepository: juspay/neurolink
Length of output: 3170
🏁 Script executed:
# Check getCurrentSessionId implementation to confirm it returns null for single commands
rg -n "getCurrentSessionId" src/lib/session/globalSessionState.ts -A 5Repository: juspay/neurolink
Length of output: 283
🏁 Script executed:
# Verify the complete control flow: single command → executeGenerate → handleTTSOutput → handleError
sed -n '2568,2590p' src/cli/factories/commandFactory.tsRepository: juspay/neurolink
Length of output: 1036
Save-failure short-circuits playback in single-command mode.
When saveAudioToFile fails or throws, handleError(...) calls process.exit(1) (since no loop session is active in typical single-command execution), preventing the --tts-play block from executing even though the audio buffer is available in memory. Given playback failures are intentionally non-fatal, save failures should be treated the same when shouldPlay is also set—or at minimum, attempt playback before saving.
Suggested ordering
- // Save audio to file if --tts-output is provided
- if (ttsOutputPath) {
- try {
- const saveResult = await saveAudioToFile(audio, ttsOutputPath);
- ...
- } catch (error) {
- handleError(error as Error, "TTS Output");
- }
- }
-
- // Play audio if --tts-play is provided
- if (shouldPlay) { ... }
+ // Play audio first so save failures cannot block playback
+ if (shouldPlay) {
+ try {
+ if (!options.quiet) logger.always(chalk.blue("Playing audio..."));
+ await playAudio(audio.buffer, audio.format);
+ } catch (err) {
+ logger.always(chalk.yellow(`Audio playback failed: ${(err as Error).message}`));
+ logger.always(chalk.yellow(" Tip: Save the audio with --tts-output <file> and play manually."));
+ }
+ }
+
+ if (ttsOutputPath) {
+ try {
+ const saveResult = await saveAudioToFile(audio, ttsOutputPath);
+ ...
+ } catch (error) {
+ handleError(error as Error, "TTS Output");
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/factories/commandFactory.ts` around lines 963 - 1005, The save-audio
block currently calls handleError (which exits) on save failures, preventing
playback when both --tts-output (ttsOutputPath) and --tts-play (shouldPlay) are
used; fix by rearranging the logic to attempt playback first (call playAudio
with audio.buffer/format and log via options.quiet/logger.always) and then save
to disk (call saveAudioToFile) so a save failure won't short-circuit play, or
alternatively make the save failure non-fatal in this context by catching
saveAudioToFile errors and logging a warning instead of invoking handleError
when shouldPlay is true; update references to saveAudioToFile, handleError,
playAudio, ttsOutputPath, shouldPlay, and options.quiet accordingly.
| case "win32": | ||
| if (format === "wav") { | ||
| return { | ||
| command: "powershell", | ||
| args: [ | ||
| "-NoProfile", | ||
| "-Command", | ||
| `(New-Object System.Media.SoundPlayer '${filePath}').PlaySync()`, | ||
| ], | ||
| }; | ||
| } | ||
| return { | ||
| command: "powershell", | ||
| args: [ | ||
| "-NoProfile", | ||
| "-Command", | ||
| `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 1; while ($player.playState -eq 3) { Start-Sleep -Milliseconds 100 }; $player.close()`, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
PowerShell command uses single-quoted interpolation — breaks/injects if the temp path contains '.
filePath is embedded via '${filePath}' inside the PS -Command string. Although execFile bypasses the OS shell, PowerShell itself parses -Command as script text, so a path containing a single quote (e.g., Windows usernames like O'Brien → C:\Users\O'Brien\AppData\Local\Temp\...) will terminate the string early, causing a parse error or arbitrary PS execution. Your temp filename prefix is safe, but os.tmpdir() is not controlled.
Safer options: pass the path as a parameter/argument instead of interpolating, or escape ' → '' before interpolation.
Proposed fix (argument passing)
case "win32":
+ // Pass path as a PowerShell argument to avoid quoting/injection issues
if (format === "wav") {
return {
command: "powershell",
args: [
"-NoProfile",
"-Command",
- `(New-Object System.Media.SoundPlayer '${filePath}').PlaySync()`,
+ "param($p) (New-Object System.Media.SoundPlayer $p).PlaySync()",
+ "-p",
+ filePath,
],
};
}
return {
command: "powershell",
args: [
"-NoProfile",
"-Command",
- `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 1; while ($player.playState -eq 3) { Start-Sleep -Milliseconds 100 }; $player.close()`,
+ "param($p) $player = New-Object -ComObject WMPlayer.OCX; $player.URL = $p; $player.controls.play(); Start-Sleep -Seconds 1; while ($player.playState -eq 3) { Start-Sleep -Milliseconds 100 }; $player.close()",
+ "-p",
+ filePath,
],
};PowerShell execFile pass file path as parameter safely to avoid single-quote escaping in -Command
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/utils/audioPlayer.ts` around lines 64 - 82, The PowerShell commands
currently interpolate filePath into the -Command string using single quotes
(e.g., '(New-Object System.Media.SoundPlayer '${filePath}').PlaySync()'), which
breaks when the path contains a single quote; change the Windows branch to pass
the path as a separate argument to powershell instead of embedding it: build a
-Command script that accepts a parameter (e.g., param($p) ...) or references
$args[0], and then supply filePath via the args array (add it after the -Command
entry) so execFile invokes powershell with the path as data rather than as
interpolated script text; update both the wav branch (SoundPlayer) and the
WMPlayer branch accordingly, referencing the case "win32", format, and filePath
symbols to locate where to change.
| format: AudioFormat, | ||
| ): Promise<void> { | ||
| const ext = getAudioExtension(format); | ||
| const tempFile = path.join(os.tmpdir(), `nl-tts-${Date.now()}.${ext}`); |
There was a problem hiding this comment.
Temp filename based only on Date.now() can collide.
Two near-simultaneous playAudio calls within the same ms (possible under loop/batch use) will share the path, causing one run to overwrite/unlink the other's file mid-playback. Use crypto.randomUUID() or fs.mkdtemp for uniqueness.
-import path from "node:path";
+import path from "node:path";
+import { randomUUID } from "node:crypto";
@@
- const tempFile = path.join(os.tmpdir(), `nl-tts-${Date.now()}.${ext}`);
+ const tempFile = path.join(
+ os.tmpdir(),
+ `nl-tts-${Date.now()}-${randomUUID()}.${ext}`,
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/utils/audioPlayer.ts` at line 116, The temp filename for playAudio
(tempFile variable created via path.join(os.tmpdir(),
`nl-tts-${Date.now()}.${ext}`)) can collide when Date.now() is identical; change
the temp-file creation to produce a cryptographically-unique name (e.g., include
crypto.randomUUID() or use fs.mkdtemp/ mkdtempSync to create a unique temp
directory and then write the file inside it) and update imports accordingly;
ensure the unique name is used wherever tempFile is referenced and
cleanup/unlink logic still targets the generated unique path.
| if (err.code === "ENOENT") { | ||
| if (process.platform === "linux" && command === "paplay") { | ||
| // Fallback to aplay on Linux | ||
| try { | ||
| await execFileAsync("aplay", [tempFile]); | ||
| return; | ||
| } catch (fallbackError) { | ||
| const fbErr = fallbackError as NodeJS.ErrnoException; | ||
| if (fbErr.code === "ENOENT") { | ||
| throw new Error( | ||
| "Neither paplay nor aplay found. Install PulseAudio (paplay) or ALSA (aplay) for audio playback.", | ||
| { cause: fallbackError }, | ||
| ); | ||
| } | ||
| throw fallbackError; | ||
| } | ||
| } | ||
|
|
||
| throw new Error( | ||
| `Audio player '${command}' not found. Ensure it is installed and available in PATH.`, | ||
| { cause: execError }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Linux aplay fallback will fail for non-wav formats.
The fallback only triggers when the primary command is paplay, which per getPlayerCommand is used for non-wav formats (mp3/ogg/opus). aplay is an ALSA raw-PCM/WAV player and will not decode mp3/ogg/opus — the fallback will either error out or produce noise. Either gate the fallback to wav only (and emit a clearer "install paplay for mp3/ogg/opus" error otherwise), or pick a decoder like ffplay/mpg123 for compressed formats.
Proposed fix
if (err.code === "ENOENT") {
- if (process.platform === "linux" && command === "paplay") {
- // Fallback to aplay on Linux
- try {
- await execFileAsync("aplay", [tempFile]);
- return;
- } catch (fallbackError) {
- const fbErr = fallbackError as NodeJS.ErrnoException;
- if (fbErr.code === "ENOENT") {
- throw new Error(
- "Neither paplay nor aplay found. Install PulseAudio (paplay) or ALSA (aplay) for audio playback.",
- { cause: fallbackError },
- );
- }
- throw fallbackError;
- }
- }
+ if (
+ process.platform === "linux" &&
+ command === "paplay" &&
+ format === "wav"
+ ) {
+ // aplay only decodes WAV/PCM; only safe to fall back for wav
+ try {
+ await execFileAsync("aplay", [tempFile]);
+ return;
+ } catch (fallbackError) {
+ const fbErr = fallbackError as NodeJS.ErrnoException;
+ if (fbErr.code === "ENOENT") {
+ throw new Error(
+ "Neither paplay nor aplay found. Install PulseAudio or ALSA for audio playback.",
+ { cause: fallbackError },
+ );
+ }
+ throw fallbackError;
+ }
+ }Note: the case "linux" branch in getPlayerCommand already routes wav to aplay directly, so this guard effectively only engages when paplay is missing on wav (which is unreachable today). Consider instead routing wav to paplay first too and relying on this consolidated fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/utils/audioPlayer.ts` around lines 130 - 152, The current fallback in
the execFileAsync error handler wrongly tries ALSA's aplay when a missing paplay
occurs even for non-wav formats (mp3/ogg/opus) — change the logic in the error
handling inside audio playback (the block handling err.code === "ENOENT" where
`command`, `tempFile`, `fallbackError` and `execError` are in scope) so that you
only attempt `aplay` as a fallback when the audio format is WAV (use the same
format check used by getPlayerCommand); for non-wav formats, do not call `aplay`
— instead surface a clear error telling the user to install `paplay` or provide
an appropriate decoder (e.g., `ffplay`/`mpg123`) or implement a fallback that
invokes a decoder for compressed formats; ensure thrown errors reference the
original `execError`/`fallbackError` as cause and update the error message to
explicitly mention the required player for the detected format.
|
Closing as superseded. CLI TTS audio playback already shipped to |
|
Reopening — my closure was incorrect. Release declares the |
🤖 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
Files reviewed: 2
src/cli/utils/audioPlayer.ts(new file, 164 lines)src/cli/factories/commandFactory.ts(modifications, +48/-21)
New issues raised in this review: 2
| Severity | Count | Description |
|---|---|---|
| CRITICAL | 0 | None |
| MAJOR | 0 | None |
| MINOR | 1 | Missing timeout for execFile call |
| SUGGESTION | 1 | Emoji inconsistency in warning message |
Pre-existing review comments: 9 unresolved comments from prior reviews covering:
⚠️ PowerShell command injection vulnerability (CRITICAL security issue)- Linux
paplay/aplayformat incompatibility (MAJOR logic issue) - Save-failure short-circuits playback due to
handleErrorexiting (MAJOR logic issue) - Temp file collision risk with
Date.now()(MINOR reliability issue) - Missing test coverage for
--tts-play(MINOR testing gap)
Overall assessment:
The PR introduces a useful CLI audio playback feature with good cross-platform support. The new code follows existing patterns and includes proper error handling with non-fatal playback failures as designed.
Action required:
The 9 pre-existing review comments (particularly the CRITICAL PowerShell injection and MAJOR Linux format issues) should be addressed before merging. My 2 new comments are non-blocking suggestions for improvement.
No new blocking issues introduced in this review.
| await execFileAsync(command, args); | ||
| } catch (execError) { | ||
| const err = execError as NodeJS.ErrnoException; | ||
|
|
There was a problem hiding this comment.
execFile - potential indefinite hang
The execFileAsync call has no timeout option. Long audio files or stalled player processes could hang indefinitely.
Suggested fix:
// Add timeout option to prevent indefinite hangs
await execFileAsync(command, args, { timeout: 60000 }); // 60s timeoutThis aligns with the project's timeout handling patterns seen in other CLI utilities.
| const shouldPlay = options.ttsPlay as boolean | undefined; | ||
| if (ttsOutputPath || shouldPlay) { | ||
| // For now, streaming TTS output is not yet available | ||
| // This will be enabled when the TTS streaming infrastructure is complete |
There was a problem hiding this comment.
💡 SUGGESTION: Emoji inconsistency in warning message
The modified warning message removed the ⚠️ emoji that was present in the original:
- Original:
"⚠️ TTS audio output for streaming is not yet available..." - Modified:
"TTS audio for streaming is not yet available..."
This is inconsistent with the established codebase pattern where warning messages use ⚠️ (warning emoji + two spaces). See other examples in the codebase like logger.always(chalk.yellow("⚠️ No providers selected...")).
Suggested fix:
logger.always(
chalk.yellow(
"⚠️ TTS audio for streaming is not yet available. Use 'generate' command for TTS output.",
),
);
|
🎉 This PR is included in version 9.82.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
src/cli/utils/audioPlayer.ts: Platform-specific audio playback utility that plays TTS audio using native CLI tools (macOS:afplay, Linux:paplay/aplay, Windows: PowerShellSoundPlayer/WMPlayer.OCX)src/cli/factories/commandFactory.ts: IntegratedplayAudiointohandleTTSOutputso--tts-playflag triggers automatic audio playback after TTS generation--tts-outputChanges
src/cli/utils/audioPlayer.tsplayAudio()andgetAudioExtension()exportssrc/cli/factories/commandFactory.tsplayAudio, extendhandleTTSOutputfor--tts-playHow it works
--tts-playis passed, audio buffer is written to a temp file inos.tmpdir()child_process.execFilepaplayis not found, falls back toaplayfinallyblockTest plan
neurolink generate "Hello world" --tts --tts-playon macOS — verify audio plays--tts-play --tts-output ./test.mp3— verify both save and play work--tts-play— verify warning message shown--tts-play— verify "not yet available" messageReferences
Summary by CodeRabbit
New Features
--tts-playflag to play text-to-speech audio output directly from the CLI.Bug Fixes