diff --git a/CHANGELOG.md b/CHANGELOG.md index 78c664c..e8be0a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Over-long text for ElevenLabs and xAI TTS no longer crashes sumvox when it contains Chinese or other multi-byte characters; it is cut at a character boundary. +- `--rate` is now optional: when omitted, the `rate` from your config (or the engine default) is used instead of being overwritten with 200. +- An unknown `--tts` value now fails with an error naming it, instead of silently running the whole fallback chain. +- `--provider` now keeps the matching config entry's `base_url` and `timeout`; `--model` only replaces the model. +- Concurrent playbacks no longer overwrite each other's temporary audio file. +- The Google Cloud TTS token request no longer uses system proxy detection, avoiding the macOS CoreFoundation crash. + ## [1.9.0] - 2026-09-04 ### Added diff --git a/src/audio/player.rs b/src/audio/player.rs index 450b068..9a03178 100644 --- a/src/audio/player.rs +++ b/src/audio/player.rs @@ -9,6 +9,7 @@ use std::io::Read; use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; +use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::mpsc; use std::time::Duration; @@ -17,6 +18,8 @@ use crate::error::{Result, VoiceError}; /// Wall-clock cap on a single playback. The cap exists to break a wedged /// player, not to police latency, so it is deliberately generous: any realistic /// notification or read-aloud summary finishes far inside it. +static TEMP_SEQ: AtomicU64 = AtomicU64::new(0); + const PLAYBACK_TIMEOUT: Duration = Duration::from_secs(120); /// Players we try, in descending order of "likely installed AND honours @@ -334,6 +337,38 @@ pub fn play_bytes(audio_data: &[u8], volume: u32, temp_file_prefix: &str) -> Res ) } +/// A temp path unique to this call, so concurrent playbacks (threads or +/// processes) never overwrite each other's audio. +pub(crate) fn unique_temp_path(prefix: &str, ext: &str) -> PathBuf { + std::env::temp_dir().join(format!( + "{prefix}_{}_{}.{ext}", + std::process::id(), + TEMP_SEQ.fetch_add(1, Ordering::Relaxed) + )) +} + +/// Write `data` to a fresh [`unique_temp_path`]. `create_new` refuses a path +/// that already exists (including a planted symlink) and the file is +/// owner-only, since the name is predictable in a shared temp dir. +pub(crate) fn write_temp_audio(prefix: &str, ext: &str, data: &[u8]) -> Result { + let path = unique_temp_path(prefix, ext); + write_exclusive(&path, data) + .map_err(|e| VoiceError::Voice(format!("Failed to write temp audio: {}", e)))?; + Ok(path) +} + +fn write_exclusive(path: &Path, data: &[u8]) -> std::io::Result<()> { + use std::io::Write; + use std::os::unix::fs::OpenOptionsExt; + + std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .mode(0o600) + .open(path) + .and_then(|mut f| f.write_all(data)) +} + /// [`play_bytes`] with the candidate list and timeout injected, for tests. pub fn play_bytes_with( candidates: &[&str], @@ -342,8 +377,6 @@ pub fn play_bytes_with( temp_file_prefix: &str, timeout: Duration, ) -> Result<()> { - use std::io::Write; - tracing::debug!( "Playing {} bytes, volume: {}, prefix: {}", audio_data.len(), @@ -351,10 +384,7 @@ pub fn play_bytes_with( temp_file_prefix ); - let tmp_path = std::env::temp_dir().join(format!("{}.wav", temp_file_prefix)); - std::fs::File::create(&tmp_path) - .and_then(|mut f| f.write_all(audio_data)) - .map_err(|e| VoiceError::Voice(format!("Failed to write temp WAV: {}", e)))?; + let tmp_path = write_temp_audio(temp_file_prefix, "wav", audio_data)?; // Capture the result before cleanup so the temp file is removed on every // path — including a spawn failure, which a bare `?` would have skipped, @@ -598,13 +628,50 @@ mod tests { crate::audio::wav_header::create_wav_file(&[0x00, 0x00], 24000, 1, 16) } + fn leftover_temp_files(prefix: &str) -> usize { + std::fs::read_dir(std::env::temp_dir()) + .unwrap() + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().starts_with(prefix)) + .count() + } + + #[test] + fn test_unique_temp_path_differs_per_call() { + let a = unique_temp_path("sumvox_test_unique", "wav"); + let b = unique_temp_path("sumvox_test_unique", "wav"); + assert_ne!(a, b); + assert_eq!(a.extension().unwrap(), "wav"); + } + + #[test] + fn test_write_temp_audio_refuses_existing_path_and_is_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let path = write_temp_audio("sumvox_test_write", "wav", b"x").unwrap(); + let mode = std::fs::metadata(&path).unwrap().permissions().mode(); + assert_eq!(mode & 0o777, 0o600); + + let planted = unique_temp_path("sumvox_test_planted", "wav"); + std::fs::write(&planted, b"planted").unwrap(); + assert!( + write_exclusive(&planted, b"x").is_err(), + "an existing path must not be reused" + ); + assert_eq!(std::fs::read(&planted).unwrap(), b"planted"); + + let _ = std::fs::remove_file(&path); + let _ = std::fs::remove_file(&planted); + } + #[test] fn test_play_bytes_with_removes_temp_file_on_success() { let prefix = "sumvox_test_bytes_ok"; let result = play_bytes_with(&["true"], &create_test_wav(), 50, prefix, FIVE_SECONDS); assert!(result.is_ok(), "unexpected error: {:?}", result.err()); - assert!( - !std::env::temp_dir().join(format!("{prefix}.wav")).exists(), + assert_eq!( + leftover_temp_files(prefix), + 0, "temp file must not survive a successful playback" ); } @@ -616,8 +683,9 @@ mod tests { .expect_err("a player exiting non-zero must never report success") .to_string(); assert!(err.contains("Audio playback failed"), "unexpected: {err}"); - assert!( - !std::env::temp_dir().join(format!("{prefix}.wav")).exists(), + assert_eq!( + leftover_temp_files(prefix), + 0, "temp file must be removed on the error path too" ); } diff --git a/src/cli.rs b/src/cli.rs index 4bb08e1..70aaba8 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -48,15 +48,15 @@ pub struct SayArgs { #[arg(long)] pub voice: Option, - /// Speech rate in words per minute + /// Speech rate in words per minute (overrides the configured rate) /// /// macos: passed to `say -r`. /// espeak: passed to `espeak-ng -s` (clamped to 80-450). /// piper: mapped onto --length-scale, where 200 is piper's default speed and /// a higher value speaks faster. /// Ignored by the cloud engines (google, cloud_tts, xai, elevenlabs, openai). - #[arg(long, default_value = "200")] - pub rate: u32, + #[arg(long)] + pub rate: Option, /// Volume level (0-100) #[arg(long)] @@ -99,15 +99,15 @@ pub struct SumArgs { #[arg(long)] pub voice: Option, - /// Speech rate in words per minute + /// Speech rate in words per minute (overrides the configured rate) /// /// macos: passed to `say -r`. /// espeak: passed to `espeak-ng -s` (clamped to 80-450). /// piper: mapped onto --length-scale, where 200 is piper's default speed and /// a higher value speaks faster. /// Ignored by the cloud engines (google, cloud_tts, xai, elevenlabs, openai). - #[arg(long, default_value = "200")] - pub rate: u32, + #[arg(long)] + pub rate: Option, /// Volume level (0-100) #[arg(long)] diff --git a/src/config.rs b/src/config.rs index cdb69f1..42cfb1c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -843,6 +843,7 @@ mod tests { #[test] fn test_get_openai_api_key_from_config() { + let _env = crate::test_support::env_guard(); let provider = openai_tts_provider(Some("sk-test".to_string())); assert_eq!(provider.get_openai_api_key(), Some("sk-test".to_string())); diff --git a/src/main.rs b/src/main.rs index f5dc3c4..97a62fe 100644 --- a/src/main.rs +++ b/src/main.rs @@ -11,6 +11,8 @@ mod notify_log; mod pipeline; mod provider_factory; mod queue; +#[cfg(test)] +mod test_support; mod transcript; mod tts; diff --git a/src/pipeline.rs b/src/pipeline.rs index 81c1be6..a0c3325 100644 --- a/src/pipeline.rs +++ b/src/pipeline.rs @@ -4,7 +4,7 @@ use std::time::Duration; use crate::config::{effective_disable_thinking, SumvoxConfig, TtsProviderConfig}; -use crate::error::Result; +use crate::error::{Result, VoiceError}; use crate::llm::{GenerationRequest, LlmProvider}; use crate::notify_log; use crate::provider_factory::ProviderFactory; @@ -18,7 +18,7 @@ use crate::tts::{ pub struct TtsOptions { pub engine: String, pub voice: Option, - pub rate: u32, + pub rate: Option, pub volume: Option, } @@ -27,7 +27,7 @@ impl Default for TtsOptions { Self { engine: "auto".to_string(), voice: None, - rate: 200, + rate: None, volume: None, } } @@ -119,6 +119,7 @@ pub async fn generate_summary( model_name, timeout, api_key.as_deref(), + matching_provider, ) { Ok(provider) => { if !provider.is_available() { @@ -215,8 +216,39 @@ async fn generate_with_fallback(attempts: &mut dyn Iterator) String::new() } +enum EngineChoice<'a> { + Engine(TtsEngine), + Named(&'a TtsProviderConfig), +} + +/// Interpret `--tts`: "auto"/empty, a known engine alias, or the exact name of a +/// configured provider. Anything else is an error rather than a silent fallback. +fn select_engine<'a>(name: &str, providers: &'a [TtsProviderConfig]) -> Result> { + if name.is_empty() { + return Ok(EngineChoice::Engine(TtsEngine::Auto)); + } + if let Ok(engine) = name.parse::() { + return Ok(EngineChoice::Engine(engine)); + } + providers + .iter() + .find(|p| p.name.to_lowercase() == name) + .map(EngineChoice::Named) + .ok_or_else(|| { + VoiceError::Config(format!( + "Unknown TTS engine '{}': not a known engine or a configured provider name", + name + )) + }) +} + /// Speak text using TTS pub async fn speak_text(config: &SumvoxConfig, tts_opts: &TtsOptions, text: &str) -> Result<()> { + // The raw engine name disambiguates entries that share one TtsEngine + // (cloud_tts vs gemini_tts); resolve_tts_provider matches it exactly first. + let engine_name = tts_opts.engine.to_lowercase(); + let choice = select_engine(&engine_name, &config.tts.providers)?; + // Record every agent voice report (even when muted) for the menu bar app. notify_log::record(text); if notify_log::is_muted() { @@ -224,35 +256,37 @@ pub async fn speak_text(config: &SumvoxConfig, tts_opts: &TtsOptions, text: &str return Ok(()); } - // The raw engine name disambiguates entries that share one TtsEngine - // (cloud_tts vs gemini_tts); resolve_tts_provider matches it exactly first. - let engine_name = tts_opts.engine.to_lowercase(); - let tts_engine = tts_opts.engine.parse().unwrap_or(TtsEngine::Auto); - // Create TTS provider: CLI override or config fallback chain - let provider: Box = match tts_engine { - TtsEngine::Auto => { + let provider: Box = match choice { + EngineChoice::Named(entry) => resolve_tts_provider( + &config.tts.providers, + &[entry.name.to_lowercase().as_str()], + tts_opts.voice.as_deref(), + tts_opts.rate, + tts_opts.volume, + )?, + EngineChoice::Engine(TtsEngine::Auto) => { // Use config fallback chain create_tts_from_config(&config.tts.providers)? } // An explicitly selected engine overrides which configured provider to use; // all attributes come from that config entry, with only explicit CLI/hook // voice/volume layered on top. Nothing is hardcoded. - TtsEngine::MacOS => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::MacOS) => resolve_tts_provider( &config.tts.providers, &["macos", "say"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::Google => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::Google) => resolve_tts_provider( &config.tts.providers, &["google", "google_tts", "gcloud", "gemini"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::CloudTts => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::CloudTts) => resolve_tts_provider( &config.tts.providers, &[ engine_name.as_str(), @@ -265,42 +299,42 @@ pub async fn speak_text(config: &SumvoxConfig, tts_opts: &TtsOptions, text: &str tts_opts.rate, tts_opts.volume, )?, - TtsEngine::AudioFile => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::AudioFile) => resolve_tts_provider( &config.tts.providers, &["audio_file", "audio", "file"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::Xai => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::Xai) => resolve_tts_provider( &config.tts.providers, &["xai", "xai_tts", "grok"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::ElevenLabs => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::ElevenLabs) => resolve_tts_provider( &config.tts.providers, &["elevenlabs", "eleven_labs", "11labs"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::OpenAi => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::OpenAi) => resolve_tts_provider( &config.tts.providers, &["openai", "openai_tts"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::Espeak => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::Espeak) => resolve_tts_provider( &config.tts.providers, &["espeak", "espeak_ng", "espeak-ng"], tts_opts.voice.as_deref(), tts_opts.rate, tts_opts.volume, )?, - TtsEngine::Piper => resolve_tts_provider( + EngineChoice::Engine(TtsEngine::Piper) => resolve_tts_provider( &config.tts.providers, &["piper", "piper_tts"], tts_opts.voice.as_deref(), @@ -321,8 +355,8 @@ pub async fn speak_text(config: &SumvoxConfig, tts_opts: &TtsOptions, text: &str } // Speak with error handling and fallback for Auto mode - match tts_engine { - TtsEngine::Auto => { + match choice { + EngineChoice::Engine(TtsEngine::Auto) => { // For Auto mode, try all providers in config order // Pass volume override so hook-level volume (stop_volume/notification_volume) is applied speak_with_provider_fallback(&config.tts.providers, text, tts_opts.volume).await diff --git a/src/pipeline/tests.rs b/src/pipeline/tests.rs index a0619ae..0668b13 100644 --- a/src/pipeline/tests.rs +++ b/src/pipeline/tests.rs @@ -233,3 +233,30 @@ async fn tts_audio_tag_stripped_only_for_providers_without_tag_support() { ["eleven:[satisfied] done", "plain:done"] ); } + +fn provider_named(name: &str) -> TtsProviderConfig { + toml::from_str(&format!("name = \"{name}\"")).unwrap() +} + +#[test] +fn test_select_engine() { + let providers = vec![provider_named("my_voice")]; + assert!(matches!( + select_engine("", &providers), + Ok(EngineChoice::Engine(TtsEngine::Auto)) + )); + assert!(matches!( + select_engine("auto", &providers), + Ok(EngineChoice::Engine(TtsEngine::Auto)) + )); + assert!(matches!( + select_engine("say", &providers), + Ok(EngineChoice::Engine(TtsEngine::MacOS)) + )); + assert!(matches!( + select_engine("my_voice", &providers), + Ok(EngineChoice::Named(p)) if p.name == "my_voice" + )); + let err = select_engine("nonsense", &providers).err().unwrap(); + assert!(err.to_string().contains("nonsense")); +} diff --git a/src/provider_factory.rs b/src/provider_factory.rs index ff8d361..e6af9b9 100644 --- a/src/provider_factory.rs +++ b/src/provider_factory.rs @@ -127,22 +127,42 @@ impl ProviderFactory { } } - /// Create a provider by name (for CLI override) + /// Create a provider by name (for CLI override). When `matching` is the + /// configured entry for `name`, its base_url, timeout and other settings are + /// kept and only the model is replaced. pub fn create_by_name( name: &str, model: &str, timeout: Duration, api_key: Option<&str>, + matching: Option<&LlmProviderConfig>, ) -> Result> { - let config = LlmProviderConfig { - name: name.to_string(), - model: model.to_string(), - api_key: api_key.map(|s| s.to_string()), - base_url: None, - timeout: timeout.as_secs(), - disable_thinking: None, - }; - Self::create_single(&config) + Self::create_single(&override_config(name, model, timeout, api_key, matching)) + } +} + +fn override_config( + name: &str, + model: &str, + timeout: Duration, + api_key: Option<&str>, + matching: Option<&LlmProviderConfig>, +) -> LlmProviderConfig { + if let Some(entry) = matching { + let mut config = entry.clone(); + config.model = model.to_string(); + if let Some(key) = api_key { + config.api_key = Some(key.to_string()); + } + return config; + } + LlmProviderConfig { + name: name.to_string(), + model: model.to_string(), + api_key: api_key.map(|s| s.to_string()), + base_url: None, + timeout: timeout.as_secs(), + disable_thinking: None, } } @@ -205,11 +225,13 @@ mod tests { #[test] fn test_create_by_name_google() { + let _env = crate::test_support::env_guard(); let result = ProviderFactory::create_by_name( "google", "gemini-2.5-flash", Duration::from_secs(10), Some("test-key"), + None, ); assert!(result.is_ok()); assert_eq!(result.unwrap().name(), "gemini"); @@ -217,11 +239,13 @@ mod tests { #[test] fn test_create_by_name_ollama() { + let _env = crate::test_support::env_guard(); let result = ProviderFactory::create_by_name( "ollama", "llama3.2", Duration::from_secs(10), None, // Ollama doesn't need API key + None, ); assert!(result.is_ok()); assert_eq!(result.unwrap().name(), "ollama"); @@ -229,6 +253,7 @@ mod tests { #[test] fn test_create_by_name_missing_api_key() { + let _env = crate::test_support::env_guard(); // Clear any env vars env::remove_var("GEMINI_API_KEY"); @@ -237,9 +262,46 @@ mod tests { "gemini-2.5-flash", Duration::from_secs(10), None, // No API key + None, ); assert!(result.is_err()); let err = result.err().unwrap(); assert!(err.to_string().contains("No API key")); } + + #[test] + fn test_override_config_keeps_configured_entry() { + let entry = LlmProviderConfig { + name: "ollama".to_string(), + model: "llama3.2".to_string(), + api_key: None, + base_url: Some("http://gpu-box:11434".to_string()), + timeout: 45, + disable_thinking: Some(true), + }; + let kept = override_config( + "ollama", + "llama3.2", + Duration::from_secs(10), + None, + Some(&entry), + ); + assert_eq!(kept.base_url.as_deref(), Some("http://gpu-box:11434")); + assert_eq!(kept.timeout, 45); + assert_eq!(kept.model, "llama3.2"); + + let remodeled = override_config( + "ollama", + "qwen3", + Duration::from_secs(10), + None, + Some(&entry), + ); + assert_eq!(remodeled.model, "qwen3"); + assert_eq!(remodeled.base_url.as_deref(), Some("http://gpu-box:11434")); + + let unconfigured = override_config("ollama", "m", Duration::from_secs(10), None, None); + assert_eq!(unconfigured.base_url, None); + assert_eq!(unconfigured.timeout, 10); + } } diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 0000000..d7d444d --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,11 @@ +//! Shared test-only helpers. + +use std::sync::{Mutex, MutexGuard}; + +/// Held by every test that reads or removes API-key environment variables, so +/// `remove_var` never races a concurrent `var` lookup. +pub static ENV_LOCK: Mutex<()> = Mutex::new(()); + +pub fn env_guard() -> MutexGuard<'static, ()> { + ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} diff --git a/src/tts/cloud_tts_auth.rs b/src/tts/cloud_tts_auth.rs index 288245e..0dd03f0 100644 --- a/src/tts/cloud_tts_auth.rs +++ b/src/tts/cloud_tts_auth.rs @@ -101,6 +101,7 @@ impl CloudTtsAuth { // Exchange JWT for access token let token_uri = sa.token_uri.as_deref().unwrap_or(TOKEN_URI); let client = Client::builder() + .no_proxy() // Disable system proxy detection to avoid CoreFoundation crash .timeout(Duration::from_secs(30)) .build() .map_err(|e| VoiceError::Voice(format!("Failed to create HTTP client: {}", e)))?; diff --git a/src/tts/elevenlabs.rs b/src/tts/elevenlabs.rs index d31aed0..1149760 100644 --- a/src/tts/elevenlabs.rs +++ b/src/tts/elevenlabs.rs @@ -5,7 +5,6 @@ use async_trait::async_trait; use reqwest::Client; use serde::Serialize; -use std::io::Write; use std::time::Duration; use super::TtsProvider; @@ -107,10 +106,8 @@ impl ElevenLabsProvider { self.volume ); - let tmp_path = std::env::temp_dir().join("sumvox_elevenlabs.mp3"); - std::fs::File::create(&tmp_path) - .and_then(|mut f| f.write_all(audio_data)) - .map_err(|e| VoiceError::Voice(format!("Failed to write temp MP3: {}", e)))?; + let tmp_path = + crate::audio::player::write_temp_audio("sumvox_elevenlabs", "mp3", audio_data)?; // Capture the result before cleanup so the temp file is removed on // every path, including a spawn failure (the pre-refactor `?` skipped it). @@ -142,10 +139,10 @@ impl TtsProvider for ElevenLabsProvider { let text = if text.len() > MAX_TEXT_LENGTH { tracing::warn!( - "Text exceeds {} chars, truncating to limit", + "Text exceeds {} bytes, truncating to limit", MAX_TEXT_LENGTH ); - &text[..MAX_TEXT_LENGTH] + truncate_to_limit(text, MAX_TEXT_LENGTH) } else { text }; @@ -223,8 +220,20 @@ impl TtsProvider for ElevenLabsProvider { } } +fn truncate_to_limit(text: &str, limit: usize) -> &str { + &text[..text.floor_char_boundary(limit)] +} + #[cfg(test)] mod tests { + #[test] + fn test_truncate_to_limit_respects_char_boundary() { + let text = format!("a{}", "你好".repeat(MAX_TEXT_LENGTH)); + let out = truncate_to_limit(&text, MAX_TEXT_LENGTH); + assert!(out.len() <= MAX_TEXT_LENGTH); + assert!(!out.is_empty()); + } + use super::*; #[test] diff --git a/src/tts/mod.rs b/src/tts/mod.rs index 2b32303..24af160 100644 --- a/src/tts/mod.rs +++ b/src/tts/mod.rs @@ -344,7 +344,7 @@ pub fn resolve_tts_provider( providers: &[TtsProviderConfig], aliases: &[&str], voice: Option<&str>, - rate: u32, + rate: Option, volume: Option, ) -> Result> { // Prefer the entry whose name exactly matches what the user asked for @@ -362,6 +362,19 @@ pub fn resolve_tts_provider( VoiceError::Config(format!("{} provider not found in config", aliases[0])) })?; + let resolved = apply_overrides(base, voice, rate, volume); + create_single_tts(&resolved) +} + +/// Layer explicit caller overrides on a configured entry; anything the caller +/// left unset keeps the configured value (and the engine's neutral default if +/// the config has none either). +fn apply_overrides( + base: &TtsProviderConfig, + voice: Option<&str>, + rate: Option, + volume: Option, +) -> TtsProviderConfig { let mut resolved = base.clone(); if let Some(v) = voice { resolved.voice = Some(v.to_string()); @@ -369,14 +382,28 @@ pub fn resolve_tts_provider( if let Some(vol) = volume { resolved.volume = Some(vol); } - resolved.rate = Some(rate); - create_single_tts(&resolved) + if let Some(r) = rate { + resolved.rate = Some(r); + } + resolved } #[cfg(test)] mod tests { use super::*; + #[test] + fn test_apply_overrides_rate_precedence() { + let mut base = espeak_config("espeak", None, Some(150)); + assert_eq!(apply_overrides(&base, None, None, None).rate, Some(150)); + assert_eq!( + apply_overrides(&base, None, Some(300), None).rate, + Some(300) + ); + base.rate = None; + assert_eq!(apply_overrides(&base, None, None, None).rate, None); + } + #[test] fn test_strip_leading_audio_tag() { assert_eq!( @@ -441,6 +468,7 @@ mod tests { #[test] fn test_factory_recognizes_gemini_tts_alias() { + let _env = crate::test_support::env_guard(); // gemini_tts routes through the cloud_tts branch; missing service // account surfaces the cloud_tts error, proving the alias is wired in // (not the "Unknown TTS provider" fallthrough). @@ -505,7 +533,7 @@ mod tests { &providers, &["gemini_tts", "cloud_tts", "gcp_tts", "google_cloud"], None, - 200, + Some(200), None, ); assert!( @@ -518,7 +546,7 @@ mod tests { #[test] fn test_dispatch_espeak_errors_when_absent_from_config() { let providers: Vec = vec![]; - let err = resolve_tts_provider(&providers, &["espeak"], None, 200, None) + let err = resolve_tts_provider(&providers, &["espeak"], None, Some(200), None) .err() .expect("an unconfigured engine must error") .to_string(); @@ -655,6 +683,7 @@ mod tests { #[test] fn test_openai_requires_voice() { + let _env = crate::test_support::env_guard(); let err = create_single_tts(&openai_config(Some("gpt-4o-mini-tts"), None)) .err() .expect("expected error without voice") @@ -664,6 +693,7 @@ mod tests { #[test] fn test_openai_requires_model() { + let _env = crate::test_support::env_guard(); let err = create_single_tts(&openai_config(None, Some("nova"))) .err() .expect("expected error without model") @@ -673,6 +703,7 @@ mod tests { #[test] fn test_required_fields_per_provider() { + let _env = crate::test_support::env_guard(); let dir = tempfile::tempdir().unwrap(); let sa_path = dir.path().join("sa.json"); std::fs::write(&sa_path, "{}").unwrap(); @@ -707,6 +738,7 @@ mod tests { #[test] fn test_openai_fully_specified_config() { + let _env = crate::test_support::env_guard(); let provider = create_single_tts(&openai_config(Some("gpt-4o-mini-tts"), Some("nova"))) .expect("fully specified openai entry should build"); assert_eq!(provider.name(), "openai"); @@ -716,6 +748,7 @@ mod tests { #[test] #[cfg(target_os = "macos")] fn test_create_tts_fallback() { + let _env = crate::test_support::env_guard(); // Google TTS without API key should fallback to macOS let providers = vec![ TtsProviderConfig { @@ -789,7 +822,7 @@ mod tests { &providers, &["macos", "say"], Some("Tingting"), - 250, + Some(250), Some(80), ); assert!(result.is_ok()); @@ -797,13 +830,13 @@ mod tests { // Other engines resolve from config the same way let espeak = vec![espeak_config("espeak", Some("cmn+f3"), Some(175))]; - let provider = resolve_tts_provider(&espeak, &["espeak"], None, 200, None) + let provider = resolve_tts_provider(&espeak, &["espeak"], None, Some(200), None) .expect("configured espeak entry should resolve"); assert_eq!(provider.name(), "espeak"); // The CLI `--voice` overlay reaches piper's model knob let piper = vec![piper_config(None, Some("/m/a.onnx"), None)]; - let provider = resolve_tts_provider(&piper, &["piper"], Some("/m/b.onnx"), 200, None) + let provider = resolve_tts_provider(&piper, &["piper"], Some("/m/b.onnx"), Some(200), None) .expect("configured piper entry should resolve"); assert_eq!(provider.name(), "piper"); } diff --git a/src/tts/xai.rs b/src/tts/xai.rs index a0f2654..782dc2a 100644 --- a/src/tts/xai.rs +++ b/src/tts/xai.rs @@ -90,10 +90,10 @@ impl TtsProvider for XaiTtsProvider { let text = if text.len() > MAX_TEXT_LENGTH { tracing::warn!( - "Text exceeds {} chars, truncating to limit", + "Text exceeds {} bytes, truncating to limit", MAX_TEXT_LENGTH ); - &text[..MAX_TEXT_LENGTH] + truncate_to_limit(text, MAX_TEXT_LENGTH) } else { text }; @@ -154,8 +154,20 @@ impl TtsProvider for XaiTtsProvider { } } +fn truncate_to_limit(text: &str, limit: usize) -> &str { + &text[..text.floor_char_boundary(limit)] +} + #[cfg(test)] mod tests { + #[test] + fn test_truncate_to_limit_respects_char_boundary() { + let text = format!("a{}", "你好".repeat(MAX_TEXT_LENGTH)); + let out = truncate_to_limit(&text, MAX_TEXT_LENGTH); + assert!(out.len() <= MAX_TEXT_LENGTH); + assert!(!out.is_empty()); + } + use super::*; #[test] diff --git a/tests/cli.rs b/tests/cli.rs index 2149320..7a688e5 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -142,28 +142,6 @@ fn notification_json_stop_active() -> String { .to_string() } -fn config_no_tts() -> String { - r#"[llm] -providers = [] -[llm.parameters] -max_tokens = 100 -temperature = 0.3 - -[tts] -providers = [] - -[summarization] -turns = 1 -system_message = "Test" -prompt_template = "Summarize: {context}" -fallback_message = "Test completed" - -[hooks.claude_code] -notification_filter = ["*"] -"# - .to_string() -} - fn config_with_audio_file(path: &str) -> String { format!( r#"[llm] @@ -336,15 +314,14 @@ fn test_sum_no_llm_config() { #[test] fn test_say_unknown_tts() { let env = TestEnv::new(); - // Use config with no TTS providers — "nonexistent" falls back to Auto, - // Auto with empty providers → error - env.setup_with_config(&config_no_tts()); + env.setup_with_config(&config_without_llm()); + env.mute(); env.cmd() - .args(["say", "hello", "--tts", "nonexistent"]) + .args(["say", "hello", "--tts", "nonsense"]) .assert() .failure() - .stderr(predicate::str::contains("No TTS provider")); + .stderr(predicate::str::contains("Unknown TTS engine 'nonsense'")); } #[test]