From ca56c15f2fc2af70ef23be8e818fe80218957a1c Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Apr 2026 21:03:21 -0400 Subject: [PATCH] refactor(llm): return Self from Client::from_source Let callers decide whether to wrap in Arc. Also consolidates the two state() fetches in build_pr_body into one. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-agent/src/cli.rs | 1 - .../fabro-agent/tests/it/parity_matrix.rs | 5 ++- lib/crates/fabro-hooks/src/executor.rs | 2 +- lib/crates/fabro-llm/src/client.rs | 5 ++- lib/crates/fabro-server/src/server.rs | 1 - .../fabro-workflow/src/handler/llm/api.rs | 2 -- .../src/pipeline/pull_request.rs | 32 +++++++++++-------- .../fabro-workflow/src/pipeline/retro.rs | 2 +- .../fabro-workflow/tests/it/integration.rs | 8 ++--- 9 files changed, 27 insertions(+), 31 deletions(-) diff --git a/lib/crates/fabro-agent/src/cli.rs b/lib/crates/fabro-agent/src/cli.rs index 8a957159a..c50deca89 100644 --- a/lib/crates/fabro-agent/src/cli.rs +++ b/lib/crates/fabro-agent/src/cli.rs @@ -459,7 +459,6 @@ pub async fn run_with_args_and_source( let provider = parse_provider(&args)?; let client = Client::from_source(llm_source.as_ref()) .await - .map(|client| (*client).clone()) .map_err(|e| anyhow::anyhow!("Failed to create LLM client: {e}"))?; ensure_provider_registered(&client, provider)?; run_with_args_and_client(args, client, mcp_servers).await diff --git a/lib/crates/fabro-agent/tests/it/parity_matrix.rs b/lib/crates/fabro-agent/tests/it/parity_matrix.rs index a775af9fc..8b8630e4d 100644 --- a/lib/crates/fabro-agent/tests/it/parity_matrix.rs +++ b/lib/crates/fabro-agent/tests/it/parity_matrix.rs @@ -150,10 +150,9 @@ async fn make_client(provider: Provider, twin: Option<&OpenAiTwinOptions>) -> Cl } let source = EnvCredentialSource::new(); - (*Client::from_source(&source) + Client::from_source(&source) .await - .expect("Client::from_source failed")) - .clone() + .expect("Client::from_source failed") } fn make_twin_client(twin: &OpenAiTwinOptions) -> Client { diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 996f4a84f..dbdc86021 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -305,7 +305,7 @@ impl HookExecutorImpl { Self::execute_llm_with_timeout(definition.timeout(), "prompt", || async move { let client = match LlmClient::from_source(llm_source).await { - Ok(client) => client, + Ok(client) => Arc::new(client), Err(e) => { tracing::warn!(error = %e, "prompt hook client creation failed, proceeding"); return HookDecision::Proceed; diff --git a/lib/crates/fabro-llm/src/client.rs b/lib/crates/fabro-llm/src/client.rs index e6cb001c9..865cff207 100644 --- a/lib/crates/fabro-llm/src/client.rs +++ b/lib/crates/fabro-llm/src/client.rs @@ -44,13 +44,12 @@ impl Client { /// /// Returns `Error` if the source cannot resolve credentials or any provider /// adapter fails to initialize. - pub async fn from_source(source: &dyn CredentialSource) -> Result, Error> { + pub async fn from_source(source: &dyn CredentialSource) -> Result { let resolved = source.resolve().await.map_err(|err| Error::Configuration { message: format!("Failed to resolve LLM credentials: {err}"), source: None, })?; - let client = Self::from_credentials(resolved.credentials).await?; - Ok(Arc::new(client)) + Self::from_credentials(resolved.credentials).await } /// Create a Client from typed provider credentials. diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 95fff2cb6..bb77f5213 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -6833,7 +6833,6 @@ async fn create_completion( // Force non-streaming for structured output let use_stream = req.stream && req.schema.is_none(); - // Resolve an LLM client from the current credential source. let llm_result = match state.resolve_llm_client().await { Ok(result) => result, Err(err) => { diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index dcdca8760..36c93f5c3 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -205,7 +205,6 @@ impl AgentApiBackend { ) -> Result { let client = Client::from_source(source) .await - .map(|client| (*client).clone()) .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; let mut profile = build_profile(model, provider); @@ -289,7 +288,6 @@ impl CodergenBackend for AgentApiBackend { ) -> Result { let client = Client::from_source(self.source.as_ref()) .await - .map(|client| (*client).clone()) .map_err(|e| Error::handler(format!("Failed to create LLM client: {e}")))?; let model = node.model().unwrap_or(&self.model); diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 39f02b75f..fe6f3225e 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -299,7 +299,15 @@ pub async fn build_pr_body( .await .map_err(|e| format!("Failed to create LLM client: {e}"))?; - build_pr_body_with_client(diff, goal, model, &services.run_store, conclusion, client).await + build_pr_body_with_client( + diff, + goal, + model, + &services.run_store, + conclusion, + Arc::new(client), + ) + .await } async fn build_pr_body_with_client( @@ -312,19 +320,6 @@ async fn build_pr_body_with_client( ) -> Result { debug!("Building PR body"); - let loaded_conclusion = if conclusion.is_none() { - run_store - .state() - .await - .inspect_err(|err| { - tracing::warn!(error = %err, "Failed to load conclusion from store for PR body"); - }) - .ok() - .and_then(|state| state.conclusion) - } else { - None - }; - let conclusion = conclusion.or(loaded_conclusion.as_ref()); let run_state = run_store .state() .await @@ -332,6 +327,15 @@ async fn build_pr_body_with_client( tracing::warn!(error = %err, "Failed to load run state from store for PR body"); }) .ok(); + let loaded_conclusion = conclusion + .is_none() + .then(|| { + run_state + .as_ref() + .and_then(|state| state.conclusion.clone()) + }) + .flatten(); + let conclusion = conclusion.or(loaded_conclusion.as_ref()); let plan_text = run_state.as_ref().and_then(read_plan_text); let retro = run_state.as_ref().and_then(|state| state.retro.clone()); let run_spec = run_state.as_ref().and_then(|state| state.spec.clone()); diff --git a/lib/crates/fabro-workflow/src/pipeline/retro.rs b/lib/crates/fabro-workflow/src/pipeline/retro.rs index 95afcc6bd..a0dff7b0f 100644 --- a/lib/crates/fabro-workflow/src/pipeline/retro.rs +++ b/lib/crates/fabro-workflow/src/pipeline/retro.rs @@ -95,7 +95,7 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { &state, &events, &options.run_dir, - client.as_ref(), + &client, services.provider, &options.model, Some(event_callback), diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 781e1e70d..4692b2ece 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6144,11 +6144,9 @@ mod real_llm { fabro_test::require_env("ANTHROPIC_API_KEY")?; let source = fabro_auth::EnvCredentialSource::new(); - Some( - Client::from_source(&source) - .await - .expect("unified-llm client should initialize from env source"), - ) + Some(Arc::new(Client::from_source(&source).await.expect( + "unified-llm client should initialize from env source", + ))) } fn make_llm_backend(client: Arc) -> Box {