Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 30 additions & 22 deletions crates/goose/src/dictation/providers.rs
Original file line number Diff line number Diff line change
@@ -1,21 +1,18 @@
use crate::config::Config;
use crate::dictation::whisper::LOCAL_WHISPER_MODEL_CONFIG_KEY;
use crate::providers::api_client::{ApiClient, AuthMethod};
use anyhow::{Context, Result};
use anyhow::Result;
use serde::{Deserialize, Serialize};
use std::sync::Mutex;
use std::time::Duration;
use utoipa::ToSchema;

const REQUEST_TIMEOUT: Duration = Duration::from_secs(30);

// Global lazy-initialized transcriber to reuse the loaded model
// Stores (model_path, transcriber) to detect when model changes
static LOCAL_TRANSCRIBER: once_cell::sync::Lazy<
Mutex<Option<(String, super::whisper::WhisperTranscriber)>>,
> = once_cell::sync::Lazy::new(|| Mutex::new(None));

// Bundled tokenizer JSON (2.4MB)
const WHISPER_TOKENIZER_JSON: &str = include_str!("whisper_data/tokens.json");

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Deserialize, Serialize, ToSchema)]
Expand Down Expand Up @@ -85,7 +82,7 @@ pub fn get_provider_def(provider: DictationProvider) -> &'static DictationProvid
PROVIDERS
.iter()
.find(|def| def.provider == provider)
.unwrap() // Safe because all enum variants are in PROVIDERS
.unwrap()
}

pub fn is_configured(provider: DictationProvider) -> bool {
Expand All @@ -106,27 +103,22 @@ pub fn is_configured(provider: DictationProvider) -> bool {
}

pub async fn transcribe_local(audio_bytes: Vec<u8>) -> Result<String> {
// Run transcription in a blocking task to avoid blocking the async runtime
tokio::task::spawn_blocking(move || {
// Get model ID from config
let config = Config::global();
let model_id = config
.get(LOCAL_WHISPER_MODEL_CONFIG_KEY, false)
.ok()
.and_then(|v| v.as_str().map(|s| s.to_string()))
.ok_or_else(|| anyhow::anyhow!("Local Whisper model not configured"))?;

// Convert model ID to full path
let model = super::whisper::get_model(&model_id)
.ok_or_else(|| anyhow::anyhow!("Unknown model: {}", model_id))?;
let model_path = model.local_path();

// Get or initialize the transcriber
let mut transcriber_lock = LOCAL_TRANSCRIBER
.lock()
.map_err(|e| anyhow::anyhow!("Failed to lock transcriber: {}", e))?;

// Check if we need to load/reload the transcriber
let model_path_str = model_path.to_string_lossy().to_string();
let needs_reload = match transcriber_lock.as_ref() {
None => true,
Expand All @@ -145,25 +137,29 @@ pub async fn transcribe_local(audio_bytes: Vec<u8>) -> Result<String> {
*transcriber_lock = Some((model_path_str, transcriber));
}

// Transcribe the audio
let (_, transcriber) = transcriber_lock.as_mut().unwrap();
let text = transcriber
.transcribe(&audio_bytes)
.context("Transcription failed")?;
let text = transcriber.transcribe(&audio_bytes).map_err(|e| {
tracing::error!("Transcription failed: {}", e);
e
})?;
Comment on lines +141 to +144
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing anyhow::Context with map_err + logging drops useful error context for callers (and can lead to duplicate logging up the stack); prefer keeping .context("…") on these fallible calls and let the top-level handler decide if/where to log.

Copilot uses AI. Check for mistakes.

Ok(text)
})
.await
.context("Transcription task failed")?
.map_err(|e| {
tracing::error!("Transcription task failed: {}", e);
anyhow::anyhow!(e)
})?
}

fn build_api_client(provider: DictationProvider) -> Result<ApiClient> {
let config = Config::global();
let def = get_provider_def(provider);

let api_key = config
.get_secret(def.config_key)
.context(format!("{} not configured", def.config_key))?;
let api_key = config.get_secret(def.config_key).map_err(|e| {
tracing::error!("{} not configured: {}", def.config_key, e);
anyhow::anyhow!("{} not configured", def.config_key)
})?;
Comment on lines +159 to +162
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This map_err turns the config secret error into a new message and logs the underlying error, but it discards the original error as the source (making troubleshooting harder when only the returned error is surfaced); consider using .context/.with_context to preserve the cause instead of logging here.

Copilot uses AI. Check for mistakes.

let base_url = if let Some(host_key) = def.host_key {
config
Expand All @@ -185,7 +181,10 @@ fn build_api_client(provider: DictationProvider) -> Result<ApiClient> {
DictationProvider::Local => anyhow::bail!("Local provider should not use API client"),
};

ApiClient::with_timeout(base_url, auth, REQUEST_TIMEOUT).context("Failed to create API client")
ApiClient::with_timeout(base_url, auth, REQUEST_TIMEOUT).map_err(|e| {
tracing::error!("Failed to create API client: {}", e);
e
})
}

pub async fn transcribe_with_provider(
Expand All @@ -202,7 +201,10 @@ pub async fn transcribe_with_provider(
let part = reqwest::multipart::Part::bytes(audio_bytes)
.file_name(format!("audio.{}", extension))
.mime_str(mime_type)
.context("Failed to create multipart")?;
.map_err(|e| {
tracing::error!("Failed to create multipart: {}", e);
anyhow::anyhow!(e)
})?;
Comment on lines 201 to +207
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mime_str error handling replaces the previous .context("Failed to create multipart") with logging + anyhow!(e), which drops context from the returned error; consider restoring .context (and avoid logging here) so callers get an actionable error chain.

Copilot uses AI. Check for mistakes.

let form = reqwest::multipart::Form::new()
.part("file", part)
Expand All @@ -212,7 +214,10 @@ pub async fn transcribe_with_provider(
.request(None, def.endpoint_path)
.multipart_post(form)
.await
.context("Request failed")?;
.map_err(|e| {
tracing::error!("Request failed: {}", e);
e
})?;

if !response.status().is_success() {
let status = response.status();
Expand All @@ -229,7 +234,10 @@ pub async fn transcribe_with_provider(
}
}

let data: serde_json::Value = response.json().await.context("Failed to parse response")?;
let data: serde_json::Value = response.json().await.map_err(|e| {
tracing::error!("Failed to parse response: {}", e);
anyhow::anyhow!(e)
})?;
Comment on lines +237 to +240
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This JSON parse error handling drops the previous .context("Failed to parse response"), making the returned error less informative when logs aren’t available; consider restoring .context/.with_context here instead of logging inline.

Copilot uses AI. Check for mistakes.
Comment on lines 149 to +240
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These error handling changes replace concise .context() calls with verbose map_err closures that log and then recreate the error. This adds duplicate logging throughout. The pattern at lines 149-152, 159-162, 184-187, 204-207, 217-220, 237-240 all follow this same problematic pattern. Revert to using .context() which is more idiomatic and avoids the duplicate logging.

Copilot generated this review using guidance from repository custom instructions.

let text = data["text"]
.as_str()
Expand Down
Loading
Loading