From f03ebacb0067a49fe0997a2998b4cacd8eeb9869 Mon Sep 17 00:00:00 2001 From: fku Date: Thu, 23 Apr 2026 14:42:42 +0200 Subject: [PATCH] fix(workflow): reuse resolved llm client for auto-pr Thread the workflow's resolved LLM client into native pull request generation so PR bodies use the same vault-backed provider resolution as normal runs. This fixes auto-PR failures when OpenAI is configured via credentials like openai_codex instead of process environment variables, and keeps the legacy fabro pr create call site compatible with the new signature. --- .../fabro-cli/src/commands/pr/create.rs | 1 + .../fabro-workflow/src/operations/start.rs | 1 + .../fabro-workflow/src/pipeline/finalize.rs | 2 + .../src/pipeline/pull_request.rs | 55 +++++++++++++++++-- .../fabro-workflow/src/pipeline/retro.rs | 3 +- .../fabro-workflow/src/pipeline/types.rs | 2 + 6 files changed, 58 insertions(+), 6 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index e42c633e2..589e1294d 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -129,6 +129,7 @@ pub(super) async fn create_command( None, &run_store.clone().into(), None, + None, ) .await .map_err(|err| anyhow::anyhow!("{err}"))?; diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index 7a087b34a..087765282 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -779,6 +779,7 @@ impl RunSession { pr_config: self.pr_config, github_app: self.pr_github_app, origin_url: self.pr_origin_url, + llm_client: retroed.llm_client.clone(), model: self.pr_model, }; diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 67dd9011c..543251733 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -234,6 +234,7 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result, + llm_client: Option, ) -> Result { debug!("Building PR body"); @@ -363,7 +367,10 @@ pub async fn build_pr_body( format!("Goal: {goal}\n\nDiff:\n```\n{truncated_diff}\n```") }; - let params = GenerateParams::new(model).system(system).prompt(prompt); + let mut params = GenerateParams::new(model).system(system).prompt(prompt); + if let Some(client) = llm_client { + params = params.client(Arc::new(client)); + } let result = generate(params) .await @@ -414,6 +421,7 @@ pub async fn maybe_open_pull_request( auto_merge: Option, run_store: &RunStoreHandle, conclusion: Option<&Conclusion>, + llm_client: Option, ) -> Result, String> { if diff.is_empty() { debug!("Empty diff, skipping pull request creation"); @@ -423,7 +431,7 @@ pub async fn maybe_open_pull_request( let https_url = ssh_url_to_https(origin_url); let (owner, repo) = github_app::parse_github_owner_repo(&https_url)?; - let body = build_pr_body(diff, goal, model, run_store, conclusion).await?; + let body = build_pr_body(diff, goal, model, run_store, conclusion, llm_client).await?; let body = truncate_pr_body(&body); let title = pr_title_from_goal(goal); @@ -538,6 +546,7 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> auto_merge, &options.run_store, Some(&conclusion), + options.llm_client.clone(), ) .await { @@ -606,12 +615,14 @@ mod tests { use crate::records::StageSummary; struct MockProvider { + name: String, response_text: String, } impl MockProvider { - fn new(text: &str) -> Self { + fn new(name: &str, text: &str) -> Self { Self { + name: name.to_string(), response_text: text.to_string(), } } @@ -620,7 +631,7 @@ mod tests { #[async_trait::async_trait] impl ProviderAdapter for MockProvider { fn name(&self) -> &str { - "mock" + &self.name } async fn complete(&self, _request: &Request) -> Result { @@ -689,12 +700,21 @@ mod tests { let mut providers: HashMap> = HashMap::new(); providers.insert( "mock".to_string(), - Arc::new(MockProvider::new("Narrative from mock.")), + Arc::new(MockProvider::new("mock", "Narrative from mock.")), ); set_default_client(Client::new(providers, Some("mock".to_string()), vec![])); }); } + fn explicit_client(provider_name: &str, text: &str) -> Client { + let mut providers: HashMap> = HashMap::new(); + providers.insert( + provider_name.to_string(), + Arc::new(MockProvider::new(provider_name, text)), + ); + Client::new(providers, Some(provider_name.to_string()), vec![]) + } + fn make_test_conclusion() -> Conclusion { Conclusion { timestamp: Utc::now(), @@ -1069,6 +1089,7 @@ mod tests { "mock-model", &run_store.clone().into(), Some(&conclusion), + None, ) .await .unwrap(); @@ -1134,6 +1155,7 @@ mod tests { "mock-model", &run_store.clone().into(), Some(&conclusion), + None, ) .await .unwrap(); @@ -1215,6 +1237,7 @@ mod tests { "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, ) .await .unwrap(); @@ -1223,6 +1246,27 @@ mod tests { assert!(body.contains("Plan from store")); } + #[tokio::test] + async fn build_pr_body_uses_explicit_llm_client() { + install_mock_llm(); + + let store = test_store(); + let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + let body = build_pr_body( + "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", + "Implement feature", + "gpt-5.4", + &run_store.clone().into(), + Some(&make_test_conclusion()), + Some(explicit_client("openai", "Narrative from explicit client.")), + ) + .await + .unwrap(); + + assert!(body.contains("Narrative from explicit client.")); + assert!(!body.contains("Narrative from mock.")); + } + // ── parse_dot_summary tests ───────────────────────────────────────── #[test] @@ -1358,6 +1402,7 @@ mod tests { None, &run_store.clone().into(), None, + None, ) .await; assert!(result.is_ok()); diff --git a/lib/crates/fabro-workflow/src/pipeline/retro.rs b/lib/crates/fabro-workflow/src/pipeline/retro.rs index 74dd181a2..4b1870151 100644 --- a/lib/crates/fabro-workflow/src/pipeline/retro.rs +++ b/lib/crates/fabro-workflow/src/pipeline/retro.rs @@ -150,7 +150,7 @@ pub async fn retro(executed: Executed, options: &RetroOptions) -> Retroed { sandbox, duration_ms, final_context: _, - llm_client: _, + llm_client, model: _, provider: _, } = executed; @@ -172,6 +172,7 @@ pub async fn retro(executed: Executed, options: &RetroOptions) -> Retroed { emitter, sandbox, duration_ms, + llm_client, retro, } } diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index 3f78fb826..a5d1e7c02 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -312,6 +312,7 @@ pub struct Retroed { pub emitter: Arc, pub sandbox: Arc, pub duration_ms: u64, + pub llm_client: Option, pub retro: Option, } @@ -380,5 +381,6 @@ pub struct PullRequestOptions { pub pr_config: Option, pub github_app: Option, pub origin_url: Option, + pub llm_client: Option, pub model: String, }