From 7e83cd38e5d6a06adcb90aa4d8e30e65da336b23 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 09:02:13 -0400 Subject: [PATCH] refactor(pipeline): split build_pr_body signature; drop RunServices::for_cli MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit build_pr_body and maybe_open_pull_request now take the two things they actually need — run_store: &RunStoreHandle and llm_source: &dyn CredentialSource — instead of services: &RunServices. The workflow PULL_REQUEST phase decomposes services at the callsite; the standalone fabro pr create command passes its own directly. This removes RunServices::for_cli, a stub constructor that fabricated an emitter, sandbox, and provider just to satisfy the RunServices type for two fields it cared about. The "leaky fake" is gone. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-cli/src/commands/pr/create.rs | 7 ++-- .../src/pipeline/pull_request.rs | 35 +++++++++---------- lib/crates/fabro-workflow/src/services.rs | 17 --------- .../fabro-workflow/tests/it/integration.rs | 5 +-- 4 files changed, 22 insertions(+), 42 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 55884d49c..6cdadbf1e 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -3,7 +3,6 @@ use fabro_model::Catalog; use fabro_sandbox::daytona::detect_repo_info; use fabro_workflow::outcome::StageStatus; use fabro_workflow::pull_request::maybe_open_pull_request; -use fabro_workflow::services::RunServices; use tracing::info; use crate::args::PrCreateArgs; @@ -100,8 +99,7 @@ pub(super) async fn create_command(args: PrCreateArgs, base_ctx: &CommandContext .id .clone() }); - let pr_services = RunServices::for_cli(run_store.clone().into(), llm_source); - + let run_store_handle = run_store.clone().into(); let pull_request = maybe_open_pull_request( &creds, &origin_url, @@ -112,7 +110,8 @@ pub(super) async fn create_command(args: PrCreateArgs, base_ctx: &CommandContext &model, true, None, - pr_services.as_ref(), + &run_store_handle, + llm_source.as_ref(), None, ) .await diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 62ea8a29d..29923abfc 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use fabro_auth::CredentialSource; use fabro_github::{self as github_app, GitHubCredentials, ssh_url_to_https}; use fabro_graphviz::parser; use fabro_llm::client::Client; @@ -16,7 +17,6 @@ use crate::event::{Event, RunNoticeLevel}; use crate::outcome::{StageStatus, format_cost as outcome_format_cost}; use crate::records::{Conclusion, RunSpec}; use crate::runtime_store::RunStoreHandle; -use crate::services::RunServices; /// Derive a PR title from the workflow goal. /// @@ -292,22 +292,15 @@ pub async fn build_pr_body( diff: &str, goal: &str, model: &str, - services: &RunServices, + run_store: &RunStoreHandle, + llm_source: &dyn CredentialSource, conclusion: Option<&Conclusion>, ) -> Result { - let client = Client::from_source(services.llm_source.as_ref()) + let client = Client::from_source(llm_source) .await .map_err(|e| format!("Failed to create LLM client: {e}"))?; - build_pr_body_with_client( - diff, - goal, - model, - &services.run_store, - conclusion, - Arc::new(client), - ) - .await + build_pr_body_with_client(diff, goal, model, run_store, conclusion, Arc::new(client)).await } async fn build_pr_body_with_client( @@ -424,7 +417,8 @@ pub async fn maybe_open_pull_request( model: &str, draft: bool, auto_merge: Option, - services: &RunServices, + run_store: &RunStoreHandle, + llm_source: &dyn CredentialSource, conclusion: Option<&Conclusion>, ) -> Result, String> { if diff.is_empty() { @@ -435,7 +429,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, services, conclusion).await?; + let body = build_pr_body(diff, goal, model, run_store, llm_source, conclusion).await?; let body = truncate_pr_body(&body); let title = pr_title_from_goal(goal); @@ -546,7 +540,8 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> &options.model, pr_cfg.draft, auto_merge, - &services, + &services.run_store, + services.llm_source.as_ref(), Some(&conclusion), ) .await @@ -1329,13 +1324,14 @@ mod tests { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let services = RunServices::for_cli(run_store.into(), llm_source); + let run_store_handle: RunStoreHandle = run_store.into(); 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", - services.as_ref(), + &run_store_handle, + llm_source.as_ref(), Some(&make_test_conclusion()), ) .await @@ -1464,8 +1460,8 @@ mod tests { async fn empty_diff_returns_none() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + let run_store_handle: RunStoreHandle = run_store.into(); let llm_source = test_llm_source(); - let services = RunServices::for_cli(run_store.clone().into(), llm_source); let creds = GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id: "123".to_string(), private_key_pem: "unused".to_string(), @@ -1480,7 +1476,8 @@ mod tests { "claude-sonnet-4-20250514", false, None, - services.as_ref(), + &run_store_handle, + llm_source.as_ref(), None, ) .await; diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index c6519b88d..90c968935 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -70,23 +70,6 @@ impl RunServices { .await } - /// CLI helper: minimal cross-phase services for PR generation and similar - /// source-backed operations outside the workflow executor. - #[must_use] - pub fn for_cli(run_store: RunStoreHandle, llm_source: Arc) -> Arc { - Self::new( - run_store, - Arc::new(Emitter::default()), - Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")), - )), - None, - None, - Provider::Anthropic, - llm_source, - ) - } - #[must_use] pub fn with_run_store(self: &Arc, run_store: RunStoreHandle) -> Arc { Arc::new(Self { diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 47646d482..39acd777f 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6734,13 +6734,14 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { None, )); let run_store = store.open_run_reader(&run_options.run_id).await.unwrap(); - let services = fabro_workflow::services::RunServices::for_cli(run_store.into(), llm_source); + let run_store_handle: fabro_workflow::runtime_store::RunStoreHandle = run_store.into(); let body = fabro_workflow::pull_request::build_pr_body( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "gpt-5.4", - services.as_ref(), + &run_store_handle, + llm_source.as_ref(), Some(&Conclusion { timestamp: Utc::now(), status: StageStatus::Success,