From 5a9f568e4630751d4ebfa06595bced73f07eb24e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 6 May 2026 07:13:18 -0400 Subject: [PATCH] refactor: simplify token plumbing and parallelize read_many_files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace mint_github_token's hand-rolled Pat/Installation/App match with GitHubCredentials::resolve_bearer_token, removing a near-duplicate of the same logic already in run_metadata::mint_token. - Parallelize read_many_files via futures::future::join_all so the tool actually reads concurrently — previously serial despite the name. - Replace .expect() on the post-refresh GitHubTokenSource cache with a proper anyhow error so a refresh edge case can't panic. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-agent/src/tools.rs | 24 ++++++++++---- lib/crates/fabro-server/src/run_manifest.rs | 32 ++++++------------- .../fabro-workflow/src/github_token_source.rs | 21 ++++++------ 3 files changed, 35 insertions(+), 42 deletions(-) diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 351798693..e3b255c4d 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -6,6 +6,7 @@ use fabro_llm::client::Client; use fabro_llm::types::{Message, Request, ToolDefinition}; use fabro_model::ModelHandle; use fabro_static::EnvVars; +use futures::future::join_all; use crate::config::SessionOptions; use crate::sandbox::GrepOptions; @@ -393,16 +394,25 @@ pub(crate) fn make_read_many_files_tool() -> RegisteredTool { }, executor: Arc::new(|args, ctx| { Box::pin(async move { - let paths = args["paths"] + let paths: Vec<&str> = args["paths"] .as_array() - .ok_or_else(|| "paths must be an array".to_string())?; + .ok_or_else(|| "paths must be an array".to_string())? + .iter() + .map(|p| { + p.as_str() + .ok_or_else(|| "each path must be a string".to_string()) + }) + .collect::>()?; + + let reads = paths.iter().map(|path| { + let env = Arc::clone(&ctx.env); + async move { (*path, env.read_file(path, None, None).await) } + }); + let results = join_all(reads).await; let mut output = String::new(); - for path_val in paths { - let path = path_val - .as_str() - .ok_or_else(|| "each path must be a string".to_string())?; - match ctx.env.read_file(path, None, None).await { + for (path, result) in results { + match result { Ok(content) => { ctx.env.mark_agent_read(path); let _ = write!(output, "=== {path} ===\n{content}\n\n"); diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index 4c7281d00..5e2a20608 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -1234,33 +1234,19 @@ async fn mint_github_token( origin_url: &str, permissions: &HashMap, ) -> Result { - match creds { - fabro_github::GitHubCredentials::Pat(token) => return Ok(token.clone()), - fabro_github::GitHubCredentials::Installation(token) => { - return token.valid_token().map(str::to_owned); - } - fabro_github::GitHubCredentials::App(_) => {} - } - let https_url = fabro_github::ssh_url_to_https(origin_url); let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url)?; - let fabro_github::GitHubCredentials::App(creds) = creds else { - unreachable!("token credentials return early"); - }; - let jwt = fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; let client = fabro_http::http_client()?; let perms_json = serde_json::to_value(permissions)?; - let install_url = creds.installation_url(&owner); - fabro_github::create_installation_access_token_with_permissions_and_install_url( - &client, - &jwt, - &owner, - &repo, - &fabro_github::github_api_base_url(), - perms_json, - install_url.as_deref(), - ) - .await + creds + .resolve_bearer_token( + &client, + &owner, + &repo, + &fabro_github::github_api_base_url(), + perms_json, + ) + .await } fn preflight_response( diff --git a/lib/crates/fabro-workflow/src/github_token_source.rs b/lib/crates/fabro-workflow/src/github_token_source.rs index 7a7815f67..2abf95bd3 100644 --- a/lib/crates/fabro-workflow/src/github_token_source.rs +++ b/lib/crates/fabro-workflow/src/github_token_source.rs @@ -111,23 +111,21 @@ impl GitHubTokenSource { SourceState::StaticIat(token) => token.valid_token().map(str::to_owned), SourceState::Mintable { minter, cache } => { let mut cache = cache.lock().await; - let should_refresh = cache + let cached_is_fresh = cache .as_ref() - .is_none_or(|token| token.near_expiry(REFRESH_THRESHOLD)); + .is_some_and(|token| !token.near_expiry(REFRESH_THRESHOLD)); - if should_refresh { + if !cached_is_fresh { match minter.mint().await { - Ok(token) => { - *cache = Some(token); - } + Ok(token) => *cache = Some(token), Err(err) => { if let Some(token) = cache.as_ref() { - if token.valid_token().is_ok() { + if let Ok(value) = token.valid_token() { warn!( error = %err, "GitHub installation token refresh failed; using cached token" ); - return token.valid_token().map(str::to_owned); + return Ok(value.to_owned()); } } return Err(err) @@ -136,11 +134,10 @@ impl GitHubTokenSource { } } - cache + let token = cache .as_ref() - .expect("mintable token source should have a token after refresh") - .valid_token() - .map(str::to_owned) + .ok_or_else(|| anyhow::anyhow!("mintable token source has no cached token"))?; + token.valid_token().map(str::to_owned) } } }