TTS-025: Add cross-platform CLI audio playback (--tts-play)#983
Conversation
…S-025] Add audioPlayer utility that plays TTS audio through the system speaker using platform-native tools (afplay on macOS, paplay/aplay on Linux, PowerShell on Windows). Wire playback into the generate command's handleTTSOutput handler — playback errors are non-fatal (warn only).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdded cross-platform CLI audio playback functionality with a new Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant Factory as Command Factory
participant Player as Audio Player
participant TempFS as Temp Filesystem
participant OSPlayer as OS Audio Player
CLI->>Factory: Execute with --tts-play flag
Factory->>Player: playAudio(buffer, format)
Player->>TempFS: Write audio buffer to temp file
TempFS-->>Player: File created
Player->>OSPlayer: Execute platform-specific player (afplay/paplay/aplay/PowerShell)
OSPlayer-->>Player: Audio playback initiated
Player->>TempFS: Clean up temp file
TempFS-->>Player: File deleted
Player-->>Factory: Playback complete/failed
Factory-->>CLI: Continue or emit warning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 a new CLI capability to play generated TTS audio directly on the user’s machine via --tts-play, while keeping existing --tts-output save-to-file behavior intact. This fits into the CLI’s TTS workflow by extending handleTTSOutput to optionally trigger playback after generation.
Changes:
- Introduces a new cross-platform audio playback utility (
playAudio) that writes a temp file, invokes an OS-native player, and cleans up. - Updates
CLICommandFactory.handleTTSOutputto support--tts-playand--tts-outputindependently or together, with non-fatal playback errors. - Updates streaming flow to warn when
--tts-play/--tts-outputare used (streaming TTS audio not yet supported).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/cli/utils/audioPlayer.ts |
New cross-platform playback helper (macOS/Linux/Windows) using native CLI tools + temp file lifecycle. |
src/cli/factories/commandFactory.ts |
Wires --tts-play into the generate flow and updates streaming warning gating for --tts-play/--tts-output. |
Comments suppressed due to low confidence (1)
src/cli/factories/commandFactory.ts:3269
- When
--tts-playis used withstream, the warning still says "TTS audio output for streaming is not yet available". Since this branch now also covers playback, the message should mention that streaming playback is not available either (or adjust wording to cover both--tts-outputand--tts-play).
// Handle TTS audio output/playback if --tts-output or --tts-play is provided
// Note: For streaming, TTS audio is collected during the stream
// and saved at the end if available
const ttsOutputPath = options.ttsOutput as string | undefined;
const ttsPlay = options.ttsPlay as boolean | undefined;
if (ttsOutputPath || ttsPlay) {
// For now, streaming TTS output is not yet available
// This will be enabled when the TTS streaming infrastructure is complete
if (!options.quiet) {
logger.always(
chalk.yellow(
"⚠️ TTS audio output for streaming is not yet available. Use 'generate' command for TTS output.",
),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await execFileAsync("powershell", [ | ||
| "-Command", | ||
| `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 30`, | ||
| ]); |
There was a problem hiding this comment.
The non-WAV Windows playback path uses Start-Sleep -Seconds 30, which will cut off longer audio (and may stop playback when PowerShell exits). This makes --tts-play unreliable for anything longer than ~30s. Consider a blocking approach that waits until playback completes (e.g., poll WMP playState/duration) before returning and cleaning up the temp file.
| await execFileAsync("powershell", [ | ||
| "-Command", | ||
| `(New-Object Media.SoundPlayer '${filePath}').PlaySync()`, | ||
| ]); | ||
| } else { | ||
| await execFileAsync("powershell", [ | ||
| "-Command", | ||
| `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 30`, | ||
| ]); |
There was a problem hiding this comment.
The PowerShell -Command strings interpolate filePath inside single quotes. If the temp directory contains an apostrophe (valid on Windows/macOS/Linux) or if TMP/TEMP is set to a path with ', the command will break and could potentially allow command injection. Escape single quotes for PowerShell single-quoted strings (replace ' with '') or pass the path via a safer mechanism (e.g., -File with arguments).
| const ext = getExtension(format); | ||
| const tmpFile = join(tmpdir(), `nl-tts-${Date.now()}.${ext}`); | ||
| try { |
There was a problem hiding this comment.
Temp file naming uses only Date.now() which can collide if multiple playbacks are triggered within the same millisecond (e.g., parallel invocations). Use a more collision-resistant suffix (e.g., crypto.randomUUID(), fs.mkdtemp, or include process.pid + a counter) to avoid accidental overwrites/races.
| // Linux: try paplay (PulseAudio) first, fallback to aplay (ALSA) for WAV | ||
| if (format === "wav") { | ||
| await execFileAsync("aplay", [filePath]).catch(() => | ||
| execFileAsync("paplay", [filePath]), | ||
| ); |
There was a problem hiding this comment.
The Linux comment says "try paplay first, fallback to aplay" but the implementation tries aplay first and falls back to paplay. Please align the comment with the actual behavior (or swap the order) to avoid misleading future maintenance.
| // Play audio through system speaker if --tts-play is provided | ||
| if (ttsPlay) { | ||
| try { | ||
| if (!options.quiet) { | ||
| logger.always(chalk.cyan("🔊 Playing audio...")); | ||
| } | ||
| await playAudio(audio.buffer, audio.format); | ||
| } catch (err) { | ||
| // Non-fatal: warn but don't crash the command | ||
| if (!options.quiet) { | ||
| logger.always( | ||
| chalk.green( | ||
| `🔊 Audio saved to: ${saveResult.path} (${formatFileSize(saveResult.size)})`, | ||
| 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 behavior for --tts-play is introduced here, but there doesn’t appear to be a corresponding CLI test in the existing test/continuous-test-suite-tts.ts coverage for flags. Consider adding a test that runs generate with --tts-play but without --tts and asserts a graceful warning + exit code 0 (and/or a test that playback failures are non-fatal), so regressions don’t silently break the flag.
| "-Command", | ||
| `(New-Object Media.SoundPlayer '${filePath}').PlaySync()`, | ||
| ]); | ||
| } else { | ||
| await execFileAsync("powershell", [ |
There was a problem hiding this comment.
On Windows, the PowerShell type name Media.SoundPlayer is likely invalid (the standard type is System.Media.SoundPlayer). As written, WAV playback may fail immediately on win32. Update the command to use the correct fully-qualified type name (and consider adding -NoProfile/-NonInteractive flags to reduce environmental variance).
| "-Command", | |
| `(New-Object Media.SoundPlayer '${filePath}').PlaySync()`, | |
| ]); | |
| } else { | |
| await execFileAsync("powershell", [ | |
| "-NoProfile", | |
| "-NonInteractive", | |
| "-Command", | |
| `(New-Object System.Media.SoundPlayer '${filePath}').PlaySync()`, | |
| ]); | |
| } else { | |
| await execFileAsync("powershell", [ | |
| "-NoProfile", | |
| "-NonInteractive", |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli/factories/commandFactory.ts (1)
952-1005:⚠️ Potential issue | 🟠 MajorDon’t hide requested TTS failure warnings behind default quiet mode.
quietdefaults totrue, sogenerate --tts-playwithout--tts, playback failures, andstream --tts --tts-playemit no warning by default. Keep progress messages quiet, but print warnings for requested TTS side effects unconditionally.🔧 Proposed fix
if (!audio) { - if (!options.quiet) { - logger.always( - chalk.yellow( - "⚠️ No audio available in result. TTS may not be enabled for this request.", - ), - ); - } + logger.always( + chalk.yellow( + "⚠️ No audio available in result. TTS may not be enabled for this request.", + ), + ); return; } @@ } catch (err) { // Non-fatal: warn but don't crash the command - if (!options.quiet) { - 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.", - ), - ); - } + 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 || ttsPlay) { // For now, streaming TTS output is not yet available // This will be enabled when the TTS streaming infrastructure is complete - if (!options.quiet) { - logger.always( - chalk.yellow( - "⚠️ TTS audio output for streaming is not yet available. Use 'generate' command for TTS output.", - ), - ); - } + logger.always( + chalk.yellow( + "⚠️ TTS audio output/playback for streaming is not yet available. Use 'generate' command for TTS output.", + ), + ); }Also applies to: 3257-3271
🤖 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 952 - 1005, The code currently gates TTS side-effect warnings behind options.quiet; change the behavior so warnings/errors for requested TTS actions are always shown regardless of options.quiet: in the ttsOutputPath branch (involving saveAudioToFile, saveResult, handleError) keep progress/success messages subject to options.quiet but ensure failures call handleError or logger.always unconditionally so the user is notified when they explicitly requested --tts-output; in the ttsPlay branch (involving ttsPlay, playAudio) keep the "Playing audio..." progress message conditional on options.quiet but move the playback failure logs (the chalk.yellow warnings and tip) out of the options.quiet guard so they always print for requested --tts-play; make analogous changes in the other occurrence noted (around play/save code at the other lines).
🤖 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/utils/audioPlayer.ts`:
- Around line 54-92: The executePlayer function lacks subprocess timeouts, is
vulnerable to PowerShell injection, uses a hardcoded Start-Sleep, and constructs
plain Errors; fix executePlayer by wrapping every execFileAsync call with the
repo's withTimeout helper (use a sensible PLAYBACK_TIMEOUT constant), replace
plain new Error(...) with ErrorFactory.create(...) calls, and avoid Start-Sleep
by spawning PowerShell/WMPlayer/afplay/paplay/aplay and awaiting the child
process exit to detect completion; for Windows calls, prevent injection by using
PowerShell's argument-friendly invocation (use the --% parameter or pass the
file path via a safe argument/quoted double-quote escaping) instead of
interpolating filePath into single-quoted strings; update both WAV and non-WAV
Windows branches and the Linux fallback paths to use withTimeout + process-exit
waiting and ErrorFactory for all failure cases.
- Around line 30-37: The temp-file handling and player invocation need fixes:
replace the predictable tmp filename using Date.now() with a unique temp
dir/file (use fs.promises.mkdtemp or a UUID/nanoid when creating tmpFile) and
consolidate creation/cleanup so tmpFile is always removed in the finally (catch
unlink errors). In executePlayer(), wrap every execFileAsync call in
withTimeout(...) to prevent hangs, remove the arbitrary PowerShell Start-Sleep
and instead await the spawned player process to finish, and fix PowerShell path
quoting by passing the file path as a properly quoted argument (use double
quotes or pass as a separate argument rather than interpolating inside single
quotes). Replace raw throws (throw new Error(...)) with the project ErrorFactory
API (e.g., ErrorFactory.create(...)) at the sites currently throwing errors so
errors are typed and consistent.
---
Outside diff comments:
In `@src/cli/factories/commandFactory.ts`:
- Around line 952-1005: The code currently gates TTS side-effect warnings behind
options.quiet; change the behavior so warnings/errors for requested TTS actions
are always shown regardless of options.quiet: in the ttsOutputPath branch
(involving saveAudioToFile, saveResult, handleError) keep progress/success
messages subject to options.quiet but ensure failures call handleError or
logger.always unconditionally so the user is notified when they explicitly
requested --tts-output; in the ttsPlay branch (involving ttsPlay, playAudio)
keep the "Playing audio..." progress message conditional on options.quiet but
move the playback failure logs (the chalk.yellow warnings and tip) out of the
options.quiet guard so they always print for requested --tts-play; make
analogous changes in the other occurrence noted (around play/save code at the
other lines).
🪄 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: d1cfe6c2-1f0d-4162-bbca-f8cc1753b32d
📒 Files selected for processing (2)
src/cli/factories/commandFactory.tssrc/cli/utils/audioPlayer.ts
| const ext = getExtension(format); | ||
| const tmpFile = join(tmpdir(), `nl-tts-${Date.now()}.${ext}`); | ||
| try { | ||
| await fs.promises.writeFile(tmpFile, buffer); | ||
| await executePlayer(tmpFile, format); | ||
| } finally { | ||
| try { | ||
| await fs.promises.unlink(tmpFile); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether audio playback temp paths are timestamp-only
# and whether exclusive temp-directory creation is used.
rg -n -C2 'nl-tts-\$\{Date\.now\(\)\}|mkdtemp|randomUUID|fs\.promises\.rm' src/cli/utils/audioPlayer.tsRepository: juspay/neurolink
Length of output: 252
🏁 Script executed:
cat -n src/cli/utils/audioPlayer.tsRepository: juspay/neurolink
Length of output: 3443
🏁 Script executed:
# Verify if ErrorFactory, withTimeout, and logger are imported or used in the file
rg -n 'ErrorFactory|withTimeout|logger' src/cli/utils/audioPlayer.tsRepository: juspay/neurolink
Length of output: 42
🏁 Script executed:
# Check if ErrorFactory and withTimeout utilities exist in the codebase
fd ErrorFactory -type f
fd withTimeout -type fRepository: juspay/neurolink
Length of output: 401
🏁 Script executed:
# Search for ErrorFactory and withTimeout definitions in the codebase
rg -l 'ErrorFactory|withTimeout' --type ts --type tsxRepository: juspay/neurolink
Length of output: 87
🏁 Script executed:
# Search for ErrorFactory and withTimeout in the codebase
rg 'ErrorFactory|withTimeout' -A 2 | head -40Repository: juspay/neurolink
Length of output: 1994
🏁 Script executed:
# Check the errorHandling.js file to see the signatures of ErrorFactory and withTimeout
cat src/lib/utils/errorHandling.js | head -100Repository: juspay/neurolink
Length of output: 122
🏁 Script executed:
# Find the errorHandling file
fd errorHandlingRepository: juspay/neurolink
Length of output: 90
🏁 Script executed:
# Read the errorHandling.ts file to see ErrorFactory and withTimeout signatures
cat -n src/lib/utils/errorHandling.ts | head -150Repository: juspay/neurolink
Length of output: 5829
🏁 Script executed:
# Continue reading errorHandling.ts to see the full ErrorFactory and withTimeout implementation
cat -n src/lib/utils/errorHandling.ts | tail -200Repository: juspay/neurolink
Length of output: 6458
🏁 Script executed:
# Search for the withTimeout function definition
rg -n 'export.*function withTimeout' src/lib/utils/errorHandling.ts -A 20Repository: juspay/neurolink
Length of output: 649
Consolidate temp directory handling, add timeout protection, use ErrorFactory for errors, and fix PowerShell path escaping.
The temp file uses predictable Date.now() timestamps that collide across concurrent calls. Additionally, executePlayer has three separate violations:
-
Missing timeout wrapper: All six
execFileAsync()calls (lines 61, 65–66, 69, 78, 83) can hang indefinitely. Wrap withwithTimeout()to prevent CLI hangs. -
Missing ErrorFactory: Lines 70–72 and 89–91 use raw
throw new Error(...). Replace withErrorFactoryfor typed, structured error handling per coding guidelines. -
PowerShell path escaping: Lines 80 and 85 interpolate
${filePath}in single quotes, which breaks on paths containing spaces. Escape with double quotes or single-quote the path argument separately. -
Fixed sleep duration: Line 85's
Start-Sleep -Seconds 30is arbitrary and incompatible with variable-length audio. Wait for WMPlayer playback to complete instead.
🛡️ Proposed fix
import { execFile } from "node:child_process";
import fs from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import { promisify } from "node:util";
+import { ErrorFactory, withTimeout } from "../../lib/utils/errorHandling.js";
import type { AudioFormat } from "../../lib/types/index.js";
const execFileAsync = promisify(execFile);
+const PLAYER_TIMEOUT_MS = 120_000; // 2-minute timeout for playback
export async function playAudio(
buffer: Buffer,
format: AudioFormat,
): Promise<void> {
const ext = getExtension(format);
- const tmpFile = join(tmpdir(), `nl-tts-${Date.now()}.${ext}`);
+ const tmpDir = await fs.promises.mkdtemp(join(tmpdir(), "nl-tts-"));
+ const tmpFile = join(tmpDir, `audio.${ext}`);
try {
await fs.promises.writeFile(tmpFile, buffer);
await executePlayer(tmpFile, format);
} finally {
try {
- await fs.promises.unlink(tmpFile);
+ await fs.promises.rm(tmpDir, { recursive: true, force: true });
} catch {
/* ignore cleanup errors */
}
}
}
async function executePlayer(
filePath: string,
format: AudioFormat,
): Promise<void> {
const platform = process.platform;
if (platform === "darwin") {
- await execFileAsync("afplay", [filePath]);
+ await withTimeout(
+ execFileAsync("afplay", [filePath]),
+ PLAYER_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("afplay", PLAYER_TIMEOUT_MS)
+ );
} else if (platform === "linux") {
if (format === "wav") {
- await execFileAsync("aplay", [filePath]).catch(() =>
- execFileAsync("paplay", [filePath]),
+ await withTimeout(
+ execFileAsync("aplay", [filePath]).catch(() =>
+ execFileAsync("paplay", [filePath]),
+ ),
+ PLAYER_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("aplay/paplay", PLAYER_TIMEOUT_MS)
+ );
} else {
- await execFileAsync("paplay", [filePath]).catch(() => {
- throw new Error(
- "Linux audio playback failed. Install PulseAudio (paplay) or use WAV format with ALSA (aplay).",
- );
+ await withTimeout(
+ execFileAsync("paplay", [filePath]).catch(() => {
+ throw ErrorFactory.toolExecutionFailed(
+ "paplay",
+ "Install PulseAudio (paplay) or use WAV format with ALSA (aplay).",
+ );
+ }),
+ PLAYER_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("paplay", PLAYER_TIMEOUT_MS)
+ );
}
} else if (platform === "win32") {
if (format === "wav") {
- await execFileAsync("powershell", [
- "-Command",
- `(New-Object Media.SoundPlayer '${filePath}').PlaySync()`,
+ await withTimeout(
+ execFileAsync("powershell", [
+ "-Command",
+ `(New-Object Media.SoundPlayer "${filePath}").PlaySync()`,
+ ]),
+ PLAYER_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("SoundPlayer", PLAYER_TIMEOUT_MS)
+ );
} else {
- await execFileAsync("powershell", [
- "-Command",
- `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 30`,
+ await withTimeout(
+ execFileAsync("powershell", [
+ "-Command",
+ `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = "${filePath}"; do { Start-Sleep -Milliseconds 100 } while ($player.playState -eq 1)`,
+ ]),
+ PLAYER_TIMEOUT_MS,
+ ErrorFactory.toolTimeout("WMPlayer", PLAYER_TIMEOUT_MS)
+ );
}
} else {
- throw new Error(
- `Unsupported platform for audio playback: ${platform}. Supported: macOS, Linux, Windows.`,
+ throw ErrorFactory.toolExecutionFailed(
+ "audioPlayer",
+ `Unsupported platform for audio playback: ${platform}. Supported: macOS, Linux, Windows.`,
);
}
}🤖 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 30 - 37, The temp-file handling
and player invocation need fixes: replace the predictable tmp filename using
Date.now() with a unique temp dir/file (use fs.promises.mkdtemp or a UUID/nanoid
when creating tmpFile) and consolidate creation/cleanup so tmpFile is always
removed in the finally (catch unlink errors). In executePlayer(), wrap every
execFileAsync call in withTimeout(...) to prevent hangs, remove the arbitrary
PowerShell Start-Sleep and instead await the spawned player process to finish,
and fix PowerShell path quoting by passing the file path as a properly quoted
argument (use double quotes or pass as a separate argument rather than
interpolating inside single quotes). Replace raw throws (throw new Error(...))
with the project ErrorFactory API (e.g., ErrorFactory.create(...)) at the sites
currently throwing errors so errors are typed and consistent.
| async function executePlayer( | ||
| filePath: string, | ||
| format: AudioFormat, | ||
| ): Promise<void> { | ||
| const platform = process.platform; | ||
| if (platform === "darwin") { | ||
| // macOS: afplay is built-in, supports mp3/wav/aac/flac | ||
| await execFileAsync("afplay", [filePath]); | ||
| } else if (platform === "linux") { | ||
| // Linux: try paplay (PulseAudio) first, fallback to aplay (ALSA) for WAV | ||
| if (format === "wav") { | ||
| await execFileAsync("aplay", [filePath]).catch(() => | ||
| execFileAsync("paplay", [filePath]), | ||
| ); | ||
| } else { | ||
| await execFileAsync("paplay", [filePath]).catch(() => { | ||
| throw new Error( | ||
| "Linux audio playback failed. Install PulseAudio (paplay) or use WAV format with ALSA (aplay).", | ||
| ); | ||
| }); | ||
| } | ||
| } else if (platform === "win32") { | ||
| // Windows: PowerShell SoundPlayer for WAV, WMPlayer for other formats | ||
| if (format === "wav") { | ||
| await execFileAsync("powershell", [ | ||
| "-Command", | ||
| `(New-Object Media.SoundPlayer '${filePath}').PlaySync()`, | ||
| ]); | ||
| } else { | ||
| await execFileAsync("powershell", [ | ||
| "-Command", | ||
| `$player = New-Object -ComObject WMPlayer.OCX; $player.URL = '${filePath}'; $player.controls.play(); Start-Sleep -Seconds 30`, | ||
| ]); | ||
| } | ||
| } else { | ||
| throw new Error( | ||
| `Unsupported platform for audio playback: ${platform}. Supported: macOS, Linux, Windows.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the repo's timeout utility and demonstrate that an apostrophe
# in the temp path breaks the current PowerShell single-quoted command string.
echo "withTimeout definitions/usages:"
rg -n -C3 '\bwithTimeout\b' --glob '*.ts'
echo
echo "Current unbounded player calls:"
rg -n -C2 '\bexecFileAsync\(' src/cli/utils/audioPlayer.ts
echo
python - <<'PY'
file_path = r"C:\Users\O'Brien\AppData\Local\Temp\nl-tts-1.mp3"
cmd = f"(New-Object Media.SoundPlayer '{file_path}').PlaySync()"
print(cmd)
print("single quote count:", cmd.count("'"))
print("Expected: odd quote count demonstrates the current interpolation breaks PowerShell quoting.")
PYRepository: juspay/neurolink
Length of output: 50374
🏁 Script executed:
cat -n src/cli/utils/audioPlayer.ts | head -100Repository: juspay/neurolink
Length of output: 3443
🏁 Script executed:
# Check if ErrorFactory is exported from lib/utils/errorHandling.js or lib/core/infrastructure/baseError.js
rg -n 'export.*ErrorFactory' src/lib/utils/errorHandling.ts src/lib/core/infrastructure/baseError.tsRepository: juspay/neurolink
Length of output: 241
Harden external player execution against subprocess hangs and PowerShell injection.
The executePlayer function has three critical gaps:
-
No timeout protection: All
execFileAsynccalls (lines 61, 65-66, 69, 78-81, 83-86) can hang indefinitely if a player subprocess stalls. -
PowerShell string injection: Lines 80 and 85 interpolate
filePathinto single-quoted PowerShell strings without escaping. A file path containing an apostrophe (e.g.,C:\Users\O'Brien\...) breaks the quoting and allows command injection. -
Fixed 30-second sleep: Line 85 uses
Start-Sleep -Seconds 30unconditionally, which delays short clips and may cut off longer clips if the temp file is cleaned up mid-playback. -
Plain error constructors: Lines 70 and 89 use
new Error()instead ofErrorFactory.
Wrap all execFileAsync calls with the repo's withTimeout utility, escape PowerShell literals using the --% parameter separator or double-quote escaping, replace the fixed sleep with actual event-based completion detection, and use ErrorFactory for error construction.
🤖 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 54 - 92, The executePlayer
function lacks subprocess timeouts, is vulnerable to PowerShell injection, uses
a hardcoded Start-Sleep, and constructs plain Errors; fix executePlayer by
wrapping every execFileAsync call with the repo's withTimeout helper (use a
sensible PLAYBACK_TIMEOUT constant), replace plain new Error(...) with
ErrorFactory.create(...) calls, and avoid Start-Sleep by spawning
PowerShell/WMPlayer/afplay/paplay/aplay and awaiting the child process exit to
detect completion; for Windows calls, prevent injection by using PowerShell's
argument-friendly invocation (use the --% parameter or pass the file path via a
safe argument/quoted double-quote escaping) instead of interpolating filePath
into single-quoted strings; update both WAV and non-WAV Windows branches and the
Linux fallback paths to use withTimeout + process-exit waiting and ErrorFactory
for all failure cases.
|
Closing as superseded. |
|
Reopening — my closure was incorrect. 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
I've reviewed PR #983 which adds cross-platform CLI audio playback (--tts-play). This is a useful feature that enables users to play TTS audio directly through system speakers.
Files Reviewed
src/cli/utils/audioPlayer.ts(new file, 93 lines)src/cli/factories/commandFactory.ts(modifications to integrate playback)
Assessment
Existing Review Coverage: There are already 8 review comments from copilot-pull-request-reviewer and CodeRabbit that comprehensively cover the key issues:
- Windows 30-second sleep limitation - Fixed sleep duration cuts off longer audio
- PowerShell path escaping vulnerability - Single-quote interpolation risks command injection with paths containing apostrophes
- Temp file collision risk -
Date.now()can collide under concurrent invocations - Comment/code mismatch - Linux comment says paplay first but code tries aplay first
- Missing test coverage - No tests for
--tts-playflag behavior - Incorrect PowerShell type name -
Media.SoundPlayershould beSystem.Media.SoundPlayer - Missing timeout protection -
execFileAsynccalls can hang indefinitely - Missing ErrorFactory usage - Raw
Errorthrows instead of typed errors
Recommendation
The feature design is sound (cross-platform native tools, zero npm dependencies, non-fatal error handling), but the implementation needs the issues above addressed before merging. The existing review comments provide actionable fixes.
No additional blocking issues found. The security scan shows no hardcoded secrets, and the architecture follows CLI patterns established in the codebase.
Once the existing review comments are addressed (particularly the PowerShell injection risk and timeout protection), this should be ready for merge.
Summary
src/cli/utils/audioPlayer.ts— cross-platform audio playback utility using platform-native CLI tools (no npm dependencies): macOS (afplay), Linux (paplay/aplay), Windows (PowerShell)src/cli/factories/commandFactory.ts— wiredplayAudio()intohandleTTSOutputso both--tts-output(save) and--tts-play(playback) work independently or together--tts-playis used (streaming TTS not yet available)Files Changed
src/cli/utils/audioPlayer.tssrc/cli/factories/commandFactory.tsplayAudio, updatehandleTTSOutputto support--tts-playDesign Decisions
os.tmpdir(), play, clean up infinallyblocksrc/cli/utils/audioFileUtils.tsTest Plan
neurolink generate "Hello" --tts --tts-playon macOS — verify audio plays through speakers--tts-play --tts-output output.mp3— verify both save and playback work--tts-playbut without--tts— verify graceful "no audio available" warningneurolink stream "Hello" --tts --tts-play— verify warning about streaming TTS not yet availableDiscussion Thread
https://slack.com/archives/D070Z4F0XNZ/p1776706127566189
Summary by CodeRabbit
--tts-playflag to trigger automatic TTS audio playback from the command line.