From f8a82d6865eea4b5d9e7c91e87f10043857352ac Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 20 Aug 2026 19:56:06 -0400 Subject: [PATCH] fix: harden GitHub token refresh handling --- lib/components/fabro-github/Cargo.toml | 3 + lib/components/fabro-github/src/lib.rs | 3 + .../fabro-github/src/test_support.rs | 26 ++ .../fabro-github/src/token_source.rs | 89 +++++- lib/components/fabro-sandbox/Cargo.toml | 1 + .../fabro-sandbox/src/daytona/mod.rs | 24 +- lib/components/fabro-sandbox/src/docker.rs | 25 +- lib/components/fabro-sandbox/src/error.rs | 14 + .../fabro-sandbox/src/provider/docker.rs | 9 +- .../fabro-sandbox/src/push_credentials.rs | 81 +++--- lib/components/fabro-sandbox/src/sandbox.rs | 52 +++- .../fabro-sandbox/src/sandbox_spec.rs | 2 +- lib/components/fabro-workflow/Cargo.toml | 1 + .../fabro-workflow/src/github_token_source.rs | 274 ------------------ .../fabro-workflow/src/handler/command.rs | 7 +- .../fabro-workflow/src/handler/llm/acp.rs | 191 ++++++------ lib/components/fabro-workflow/src/lib.rs | 1 - .../fabro-workflow/src/pipeline/initialize.rs | 33 +-- .../fabro-workflow/src/run_metadata.rs | 13 +- lib/components/fabro-workflow/src/services.rs | 28 +- 20 files changed, 365 insertions(+), 512 deletions(-) create mode 100644 lib/components/fabro-github/src/test_support.rs delete mode 100644 lib/components/fabro-workflow/src/github_token_source.rs diff --git a/lib/components/fabro-github/Cargo.toml b/lib/components/fabro-github/Cargo.toml index 788c6efe1..8f755dd79 100644 --- a/lib/components/fabro-github/Cargo.toml +++ b/lib/components/fabro-github/Cargo.toml @@ -9,6 +9,9 @@ description = "GitHub App authentication and API helpers for Fabro" [lib] doctest = false +[features] +test-support = [] + [lints] workspace = true diff --git a/lib/components/fabro-github/src/lib.rs b/lib/components/fabro-github/src/lib.rs index 9973c4377..a38c6c8ce 100644 --- a/lib/components/fabro-github/src/lib.rs +++ b/lib/components/fabro-github/src/lib.rs @@ -11,6 +11,9 @@ use tokio::process::Command; pub mod token_source; +#[cfg(any(test, feature = "test-support"))] +pub mod test_support; + pub const GITHUB_API_BASE_URL: &str = "https://api.github.com"; /// Returns the GitHub API base URL, allowing override via `GITHUB_BASE_URL` env diff --git a/lib/components/fabro-github/src/test_support.rs b/lib/components/fabro-github/src/test_support.rs new file mode 100644 index 000000000..09cca93ce --- /dev/null +++ b/lib/components/fabro-github/src/test_support.rs @@ -0,0 +1,26 @@ +use std::sync::Arc; + +use crate::InstallationToken; +use crate::token_source::{InstallationTokenMinter as InnerMinter, InstallationTokenSource}; + +#[async_trait::async_trait] +pub trait InstallationTokenMinter: Send + Sync { + async fn mint(&self) -> anyhow::Result; +} + +struct TestMinterAdapter(Arc); + +#[async_trait::async_trait] +impl InnerMinter for TestMinterAdapter { + async fn mint(&self) -> anyhow::Result { + self.0.mint().await + } +} + +#[must_use] +pub fn installation_token_source( + repo: impl Into, + minter: Arc, +) -> Arc { + InstallationTokenSource::with_minter(repo.into(), Box::new(TestMinterAdapter(minter))) +} diff --git a/lib/components/fabro-github/src/token_source.rs b/lib/components/fabro-github/src/token_source.rs index 11743e077..5af872af6 100644 --- a/lib/components/fabro-github/src/token_source.rs +++ b/lib/components/fabro-github/src/token_source.rs @@ -1,12 +1,10 @@ //! Cached GitHub installation-token source. //! -//! One [`InstallationTokenSource`] serves every GitHub-token consumer for an -//! origin repository — the clone-based sandbox providers and the run-metadata -//! writer share a single source, so "reuse a token until near expiry" is the -//! default behavior instead of a per-call-site special case. Reusing mature -//! tokens keeps consumers out of GitHub's token-replication lag window, where -//! a token minted milliseconds earlier is rejected with 404 "Repository not -//! found" or an authentication failure. +//! One [`InstallationTokenSource`] can serve GitHub-token consumers that share +//! a repository and permission scope. Reusing mature tokens keeps consumers +//! out of GitHub's token-replication lag window, where a token minted +//! milliseconds earlier is rejected with 404 "Repository not found" or an +//! authentication failure. //! //! The source also reports *provenance*: when it minted the token it returned, //! and which mint generation it belongs to. Retry classification, logging, and @@ -138,7 +136,7 @@ pub struct ResolvedToken { /// Mints installation tokens for [`InstallationTokenSource`]. Abstracted so /// tests can script mint results without HTTP. #[async_trait::async_trait] -pub trait InstallationTokenMinter: Send + Sync { +pub(crate) trait InstallationTokenMinter: Send + Sync { async fn mint(&self) -> anyhow::Result; } @@ -228,6 +226,16 @@ impl InstallationTokenSource { let normalized = crate::normalize_repo_origin_url(origin_url); let (owner, repo) = crate::parse_github_owner_repo(&normalized) .context("parsing GitHub origin for token source")?; + Self::for_repository(creds, owner, repo, permissions) + } + + /// Build a source for an already parsed GitHub repository. + pub fn for_repository( + creds: &GitHubCredentials, + owner: String, + repo: String, + permissions: serde_json::Value, + ) -> anyhow::Result> { let repo_display = format!("{owner}/{repo}"); let state = match creds { GitHubCredentials::Pat(token) => SourceState::Pat(SecretString::new(token.clone())), @@ -255,9 +263,28 @@ impl InstallationTokenSource { })) } - /// Build a minting source over a custom minter. For tests. + /// Build a source for a personal access token. #[must_use] - pub fn with_minter(repo: String, minter: Box) -> Arc { + pub fn pat(token: String) -> Arc { + Arc::new(Self { + repo: String::new(), + state: SourceState::Pat(SecretString::new(token)), + }) + } + + /// Build a source for a pre-minted installation token. + #[must_use] + pub fn installation(token: InstallationToken) -> Arc { + Arc::new(Self { + repo: String::new(), + state: SourceState::Installation(token), + }) + } + + /// Build a minting source over a custom minter. + #[cfg(any(test, feature = "test-support"))] + #[must_use] + pub(crate) fn with_minter(repo: String, minter: Box) -> Arc { Arc::new(Self { repo, state: SourceState::App { @@ -296,7 +323,27 @@ impl InstallationTokenSource { return Ok(resolved); } } - self.mint_locked(minter.as_ref(), &mut cache).await + match self.mint_locked(minter.as_ref(), &mut cache).await { + Ok(resolved) => Ok(resolved), + Err(err) => { + if let Some(cached) = cache.as_ref() { + if cached.token.valid_token().is_ok() { + tracing::warn!( + error = %format!("{err:#}"), + repo = %self.repo, + generation = cached.generation, + expires_at = %cached.token.expires_at, + "GitHub installation token refresh failed; using cached token" + ); + return Ok(cached.resolved(TokenProvenance::Reused { + minted_at: cached.minted_at, + expires_at: cached.token.expires_at, + })); + } + } + Err(err) + } + } } } } @@ -517,6 +564,26 @@ mod tests { assert_eq!(second.token.expose(), "ghs_gen2"); } + #[tokio::test] + async fn resolve_uses_a_valid_cached_token_when_refresh_fails() { + let (source, minter) = mintable(vec![ + MintAction::Token("ghs_gen1", Utc::now() + chrono::Duration::minutes(5)), + MintAction::Error("mint failed"), + ]); + + let first = source.resolve().await.unwrap(); + let second = source.resolve().await.unwrap(); + + assert_eq!(minter.calls(), 2); + assert_eq!(first.snapshot.generation, 1); + assert_eq!(second.snapshot.generation, 1); + assert!(matches!( + second.snapshot.provenance, + TokenProvenance::Reused { .. } + )); + assert_eq!(second.token.expose(), "ghs_gen1"); + } + #[tokio::test] async fn concurrent_resolves_share_one_generation() { // Single mint in the script: a second mint would panic on an empty diff --git a/lib/components/fabro-sandbox/Cargo.toml b/lib/components/fabro-sandbox/Cargo.toml index 7153e3ea1..251e6bd77 100644 --- a/lib/components/fabro-sandbox/Cargo.toml +++ b/lib/components/fabro-sandbox/Cargo.toml @@ -64,6 +64,7 @@ futures-util = { workspace = true, optional = true } rustls = { version = "0.23", default-features = false, features = ["std", "ring"], optional = true } [dev-dependencies] +fabro-github = { path = "../fabro-github", features = ["test-support"] } tokio = { workspace = true, features = ["test-util", "macros"] } tempfile = "3" serde_json.workspace = true diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 79658120e..e78877ef7 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -336,7 +336,6 @@ pub struct DaytonaSandbox { config: DaytonaConfig, client: daytona_sdk::Client, api_key: Option, - github_app: Option, push_credentials: PushCredentialState, sandbox: OnceCell, snapshot_name: OnceCell, @@ -379,7 +378,6 @@ impl DaytonaSandbox { config, client, api_key, - github_app, push_credentials, sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), @@ -432,7 +430,6 @@ impl DaytonaSandbox { config: DaytonaConfig::default(), client, api_key, - github_app: None, push_credentials: PushCredentialState::new(None), sandbox: sandbox_cell, snapshot_name: OnceCell::new(), @@ -1072,10 +1069,11 @@ impl Sandbox for DaytonaSandbox { // against the clone token instead of believing nothing was // ever embedded. let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { - let err = crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )); + Some(source) => Some(source.mint_for_clone().await.map_err(|source| { + let err = crate::Error::context_anyhow( + "Failed to get GitHub App credentials for clone", + source, + ); self.emit(SandboxEvent::GitCloneFailed { url: origin_url.clone(), error: err.to_string(), @@ -1295,7 +1293,7 @@ impl Sandbox for DaytonaSandbox { } Err(e) => { tracing::warn!( - origin = %origin_url, + origin = %fabro_redact::redacted_url_for_log(&origin_url), error = %e, "Failed to build authenticated origin URL — \ subsequent git push from this sandbox will fail" @@ -1304,7 +1302,7 @@ impl Sandbox for DaytonaSandbox { } } } - Err(e) if self.github_app.is_none() => { + Err(e) if self.push_credentials.source().is_none() => { let err = crate::Error::context( "Git clone failed. If this is a private repository, \ configure a GitHub App with `fabro install` and install it \ @@ -1554,9 +1552,10 @@ impl Sandbox for DaytonaSandbox { let result = self .exec_command(&cmd, 10_000, None, None, None) .await - .map_err(|_| { - crate::Error::message( - "Failed to refresh push credentials: set_url_exec_failed", + .map_err(|err| { + crate::Error::context( + "Failed to refresh push credentials: set origin URL", + err, ) })?; if !result.is_success() { @@ -2762,7 +2761,6 @@ mod tests { config, client, api_key: Some(api_key.to_string()), - github_app: None, push_credentials: PushCredentialState::new(None), sandbox: OnceCell::new(), snapshot_name: OnceCell::new(), diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 532e7cbb2..70c4da230 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -133,7 +133,6 @@ impl Default for DockerSandboxOptions { pub struct DockerSandbox { docker: Docker, config: DockerSandboxOptions, - github_app: Option, push_credentials: PushCredentialState, run_id: Option, clone_origin_url: Option, @@ -164,7 +163,7 @@ enum ContainerStartAction { impl DockerSandbox { pub fn new( config: DockerSandboxOptions, - github_app: Option, + github_app: Option<&GitHubCredentials>, run_id: Option, clone_origin_url: Option, clone_branch: Option, @@ -183,19 +182,18 @@ impl DockerSandbox { fn with_docker_client( docker: Docker, config: DockerSandboxOptions, - github_app: Option, + github_app: Option<&GitHubCredentials>, run_id: Option, clone_origin_url: Option, clone_branch: Option, ) -> crate::Result { let push_credentials = PushCredentialState::new(push_credentials::build_token_source( - github_app.as_ref(), + github_app, clone_origin_url.as_deref(), )?); Ok(Self { docker, config, - github_app, push_credentials, run_id, clone_origin_url, @@ -724,7 +722,7 @@ impl DockerSandbox { ) -> crate::Error { let error = result .into_exec_error_with_redactor("git clone", |output| redact_auth_url(output, auth_url)); - let message = if self.github_app.is_none() { + let message = if self.push_credentials.source().is_none() { "Git clone failed. If this is a private repository, configure a GitHub App with \ `fabro install` and install it for your organization." } else { @@ -753,10 +751,8 @@ impl DockerSandbox { // the shared source, so the first refresh compares against the clone // token instead of believing nothing was ever embedded. let resolved_token = match self.push_credentials.source() { - Some(source) => Some(source.mint_for_clone().await.map_err(|e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) + Some(source) => Some(source.mint_for_clone().await.map_err(|err| { + crate::Error::context_anyhow("Failed to get GitHub App credentials for clone", err) })?), None => None, }; @@ -767,10 +763,11 @@ impl DockerSandbox { let auth_url = match &resolved_token { Some(token) => Some( fabro_github::embed_token_in_url(&origin_url, token.token.expose()).map_err( - |e| { - crate::Error::message(format!( - "Failed to get GitHub App credentials for clone: {e}" - )) + |err| { + crate::Error::context_anyhow( + "Failed to build authenticated GitHub clone URL", + err, + ) }, )?, ), diff --git a/lib/components/fabro-sandbox/src/error.rs b/lib/components/fabro-sandbox/src/error.rs index 8d6bf0d73..76f096d4b 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -18,6 +18,13 @@ pub enum Error { source: Box, }, + #[error("{message}")] + AnyhowContext { + message: String, + #[source] + source: anyhow::Error, + }, + #[cfg(feature = "docker")] #[error("Failed to connect to Docker daemon")] DockerConnect { @@ -68,6 +75,13 @@ impl Error { } } + pub fn context_anyhow(message: impl Into, source: anyhow::Error) -> Self { + Self::AnyhowContext { + message: message.into(), + source, + } + } + pub fn exec(label: impl Into, result: ExecResult) -> Self { Self::Exec { label: label.into(), diff --git a/lib/components/fabro-sandbox/src/provider/docker.rs b/lib/components/fabro-sandbox/src/provider/docker.rs index f1f8bb725..865e361be 100644 --- a/lib/components/fabro-sandbox/src/provider/docker.rs +++ b/lib/components/fabro-sandbox/src/provider/docker.rs @@ -98,8 +98,13 @@ impl SandboxProvider for DockerSandboxProvider { )); }; - let sandbox = - DockerSandbox::new(config, github_app, run_id, clone_origin_url, clone_branch)?; + let sandbox = DockerSandbox::new( + config, + github_app.as_ref(), + run_id, + clone_origin_url, + clone_branch, + )?; sandbox.initialize().await?; let container_id = sandbox.container_identifier()?.to_string(); self.get(&container_id).await?.ok_or_else(|| { diff --git a/lib/components/fabro-sandbox/src/push_credentials.rs b/lib/components/fabro-sandbox/src/push_credentials.rs index 90116317f..2c0496544 100644 --- a/lib/components/fabro-sandbox/src/push_credentials.rs +++ b/lib/components/fabro-sandbox/src/push_credentials.rs @@ -15,7 +15,7 @@ use fabro_github::token_source::{InstallationTokenSource, ResolvedToken}; use fabro_redact::DisplaySafeUrl; use tokio::sync::Mutex; -use crate::sandbox::{RefreshOutcome, RemoteCredentialAction}; +use crate::sandbox::RefreshOutcome; /// Build the shared installation-token source for a clone-based sandbox. /// @@ -33,18 +33,19 @@ pub(crate) fn build_token_source( return Ok(None); }; let normalized = fabro_github::normalize_repo_origin_url(origin_url); - if fabro_github::parse_github_owner_repo(&normalized).is_err() { + let Ok((owner, repo)) = fabro_github::parse_github_owner_repo(&normalized) else { // Non-GitHub origins never clone in these providers, so there is no // remote to keep credentials fresh for. return Ok(None); - } - InstallationTokenSource::for_origin( + }; + InstallationTokenSource::for_repository( creds, - &normalized, + owner, + repo, serde_json::json!({ "contents": "write" }), ) .map(Some) - .map_err(|err| crate::Error::message(format!("Failed to build GitHub token source: {err:#}"))) + .map_err(|err| crate::Error::context_anyhow("Failed to build GitHub token source", err)) } /// Push-credential state one provider instance tracks for its `origin` @@ -122,8 +123,9 @@ impl PushCredentialState { "GitHub token refresh failed and no credentials were ever embedded" ); } - return Err(crate::Error::message( - "Failed to refresh push credentials: token_mint_failed", + return Err(crate::Error::context_anyhow( + "Failed to refresh push credentials", + err, )); } }; @@ -131,22 +133,16 @@ impl PushCredentialState { .as_ref() .is_some_and(|prev| prev.snapshot.generation == resolved.snapshot.generation) { - return Ok(RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(resolved.snapshot), - }); + return Ok(RefreshOutcome::unchanged(resolved.snapshot)); } let auth_url = fabro_github::embed_token_in_url(origin_url, resolved.token.expose()) .map_err(|err| { - crate::Error::message(format!("Failed to build authenticated origin URL: {err:#}")) + crate::Error::context_anyhow("Failed to build authenticated origin URL", err) })?; set_url(auth_url).await?; let snapshot = resolved.snapshot; *embedded = Some(resolved); - Ok(RefreshOutcome { - action: RemoteCredentialAction::Embedded, - token: Some(snapshot), - }) + Ok(RefreshOutcome::embedded(snapshot)) } } @@ -156,9 +152,10 @@ mod tests { use chrono::Utc; use fabro_github::InstallationToken; - use fabro_github::token_source::InstallationTokenMinter; + use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; use super::*; + use crate::sandbox::RemoteCredentialAction; struct FixedMinter { calls: AtomicUsize, @@ -186,9 +183,9 @@ mod tests { } fn minting_state(ttl: chrono::Duration) -> PushCredentialState { - PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FixedMinter { + PushCredentialState::new(Some(installation_token_source( + "owner/repo", + Arc::new(FixedMinter { calls: AtomicUsize::new(0), ttl, }), @@ -204,8 +201,7 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::None); - assert_eq!(outcome.token, None); + assert_eq!(outcome, RefreshOutcome::none()); } #[tokio::test] @@ -221,8 +217,8 @@ mod tests { }) .await .unwrap(); - assert_eq!(first.action, RemoteCredentialAction::Embedded); - assert_eq!(first.token.unwrap().generation, 1); + assert_eq!(first.action(), RemoteCredentialAction::Embedded); + assert_eq!(first.token().unwrap().generation, 1); // The cached token is fresh, so the second refresh must skip set-url. let second = state @@ -232,8 +228,8 @@ mod tests { }) .await .unwrap(); - assert_eq!(second.action, RemoteCredentialAction::Unchanged); - assert_eq!(second.token.unwrap().generation, 1); + assert_eq!(second.action(), RemoteCredentialAction::Unchanged); + assert_eq!(second.token().unwrap().generation, 1); assert_eq!(set_url_calls.load(Ordering::SeqCst), 1); } @@ -258,9 +254,9 @@ mod tests { .await .unwrap(); - assert_eq!(first.token.unwrap().generation, 1); - assert_eq!(second.action, RemoteCredentialAction::Embedded); - assert_eq!(second.token.unwrap().generation, 2); + assert_eq!(first.token().unwrap().generation, 1); + assert_eq!(second.action(), RemoteCredentialAction::Embedded); + assert_eq!(second.token().unwrap().generation, 2); assert_eq!(set_url_calls.load(Ordering::SeqCst), 2); } @@ -274,8 +270,8 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert_eq!(outcome.token.unwrap().generation, 1); + assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); + assert_eq!(outcome.token().unwrap().generation, 1); } #[tokio::test] @@ -293,8 +289,8 @@ mod tests { // The generation was not recorded, so the retry embeds again instead // of wrongly skipping. let retried = state.refresh(ORIGIN, |_| async { Ok(()) }).await.unwrap(); - assert_eq!(retried.action, RemoteCredentialAction::Embedded); - assert_eq!(retried.token.unwrap().generation, 1); + assert_eq!(retried.action(), RemoteCredentialAction::Embedded); + assert_eq!(retried.token().unwrap().generation, 1); } #[tokio::test] @@ -313,22 +309,25 @@ mod tests { .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap(); - assert_eq!(outcome.action, RemoteCredentialAction::Unchanged); - assert!(outcome.token.unwrap().is_static()); + assert_eq!(outcome.action(), RemoteCredentialAction::Unchanged); + assert!(outcome.token().unwrap().is_static()); } #[tokio::test] - async fn mint_failure_maps_to_the_token_mint_failed_error() { - let state = PushCredentialState::new(Some(InstallationTokenSource::with_minter( - "owner/repo".to_string(), - Box::new(FailingMinter), + async fn mint_failure_preserves_the_mint_error_chain() { + let state = PushCredentialState::new(Some(installation_token_source( + "owner/repo", + Arc::new(FailingMinter), ))); let err = state .refresh(ORIGIN, |_| async { panic!("set-url must not run") }) .await .unwrap_err(); - assert!(err.to_string().contains("token_mint_failed"), "{err}"); + assert_eq!(err.causes(), vec![ + "minting GitHub installation access token", + "mint failed" + ]); } #[test] diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index fb8cb0102..94def6760 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1038,22 +1038,48 @@ pub enum RemoteCredentialAction { None, } -/// Outcome of [`Sandbox::refresh_push_credentials`]: what this call did to the -/// remote, and the non-secret description of the token embedded in it. -/// `token` is `None` only when `action` is [`RemoteCredentialAction::None`]. +/// Outcome of [`Sandbox::refresh_push_credentials`]. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct RefreshOutcome { - pub action: RemoteCredentialAction, - pub token: Option, +pub enum RefreshOutcome { + /// No managed credentials exist for this sandbox. + None, + /// The remote already carried this token generation. + Unchanged(TokenSnapshot), + /// The remote was updated to carry this token generation. + Embedded(TokenSnapshot), } impl RefreshOutcome { /// No managed credentials to refresh. #[must_use] - pub fn none() -> Self { - Self { - action: RemoteCredentialAction::None, - token: None, + pub const fn none() -> Self { + Self::None + } + + #[must_use] + pub const fn unchanged(token: TokenSnapshot) -> Self { + Self::Unchanged(token) + } + + #[must_use] + pub const fn embedded(token: TokenSnapshot) -> Self { + Self::Embedded(token) + } + + #[must_use] + pub const fn action(self) -> RemoteCredentialAction { + match self { + Self::None => RemoteCredentialAction::None, + Self::Unchanged(_) => RemoteCredentialAction::Unchanged, + Self::Embedded(_) => RemoteCredentialAction::Embedded, + } + } + + #[must_use] + pub const fn token(self) -> Option { + match self { + Self::None => None, + Self::Unchanged(token) | Self::Embedded(token) => Some(token), } } } @@ -1550,17 +1576,17 @@ pub(crate) async fn fetch_source_run_ref( pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::Result<()> { let token = match sandbox.refresh_push_credentials().await { Ok(outcome) => { - if let Some(token) = outcome.token { + if let Some(token) = outcome.token() { tracing::debug!( refspec = %refspec, - action = %outcome.action, + action = %outcome.action(), generation = token.generation, provenance = %token.provenance, token_age_ms = token.age_ms(), "Resolved push credentials before git push" ); } - outcome.token + outcome.token() } Err(e) => { // The provider logged which token stays embedded; the push diff --git a/lib/components/fabro-sandbox/src/sandbox_spec.rs b/lib/components/fabro-sandbox/src/sandbox_spec.rs index b6a56bd40..7fcc7232a 100644 --- a/lib/components/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/components/fabro-sandbox/src/sandbox_spec.rs @@ -205,7 +205,7 @@ impl SandboxSpec { } => { let mut sandbox = DockerSandbox::new( config.clone(), - github_app.clone(), + github_app.as_ref(), *run_id, clone_origin_url.clone(), clone_branch.clone(), diff --git a/lib/components/fabro-workflow/Cargo.toml b/lib/components/fabro-workflow/Cargo.toml index b3ab70260..0024c8b79 100644 --- a/lib/components/fabro-workflow/Cargo.toml +++ b/lib/components/fabro-workflow/Cargo.toml @@ -76,6 +76,7 @@ toml.workspace = true fabro-vault = { path = "../../foundation/fabro-vault" } [dev-dependencies] fabro-auth = { path = "../../foundation/fabro-auth", features = ["test-support"] } +fabro-github = { path = "../fabro-github", features = ["test-support"] } base64.workspace = true fabro-acp = { path = "../fabro-acp", features = ["test-support"] } fabro-workflow = { path = ".", features = ["test-support"] } diff --git a/lib/components/fabro-workflow/src/github_token_source.rs b/lib/components/fabro-workflow/src/github_token_source.rs deleted file mode 100644 index 2abf95bd3..000000000 --- a/lib/components/fabro-workflow/src/github_token_source.rs +++ /dev/null @@ -1,274 +0,0 @@ -use std::sync::Arc; -use std::time::Duration; - -use anyhow::Context as _; -use fabro_github::{GitHubAppCredentials, InstallationToken}; -use tokio::sync::Mutex; -use tracing::warn; - -const REFRESH_THRESHOLD: Duration = Duration::from_mins(15); - -#[async_trait::async_trait] -pub trait IatMinter: Send + Sync { - async fn mint(&self) -> anyhow::Result; -} - -pub struct AppIatMinter { - creds: GitHubAppCredentials, - http: fabro_http::HttpClient, - owner: String, - repo: String, - api_base: String, - install_url: Option, - permissions: serde_json::Value, -} - -impl AppIatMinter { - #[must_use] - pub fn new( - creds: GitHubAppCredentials, - http: fabro_http::HttpClient, - owner: String, - repo: String, - api_base: String, - install_url: Option, - permissions: serde_json::Value, - ) -> Self { - Self { - creds, - http, - owner, - repo, - api_base, - install_url, - permissions, - } - } -} - -#[async_trait::async_trait] -impl IatMinter for AppIatMinter { - async fn mint(&self) -> anyhow::Result { - self.creds - .mint_installation_token( - &self.http, - &self.owner, - &self.repo, - &self.api_base, - self.permissions.clone(), - self.install_url.as_deref(), - ) - .await - } -} - -pub struct GitHubTokenSource { - state: SourceState, -} - -enum SourceState { - Pat(String), - StaticIat(InstallationToken), - Mintable { - minter: Arc, - cache: Mutex>, - }, -} - -impl GitHubTokenSource { - #[must_use] - pub fn pat(token: String) -> Self { - Self { - state: SourceState::Pat(token), - } - } - - #[must_use] - pub fn static_iat(token: InstallationToken) -> Self { - Self { - state: SourceState::StaticIat(token), - } - } - - #[must_use] - pub fn mintable(minter: Arc) -> Self { - Self { - state: SourceState::Mintable { - minter, - cache: Mutex::new(None), - }, - } - } - - #[must_use] - pub fn is_refreshable(&self) -> bool { - matches!(self.state, SourceState::Mintable { .. }) - } - - pub async fn current_token(&self) -> anyhow::Result { - match &self.state { - SourceState::Pat(token) => Ok(token.clone()), - SourceState::StaticIat(token) => token.valid_token().map(str::to_owned), - SourceState::Mintable { minter, cache } => { - let mut cache = cache.lock().await; - let cached_is_fresh = cache - .as_ref() - .is_some_and(|token| !token.near_expiry(REFRESH_THRESHOLD)); - - if !cached_is_fresh { - match minter.mint().await { - Ok(token) => *cache = Some(token), - Err(err) => { - if let Some(token) = cache.as_ref() { - if let Ok(value) = token.valid_token() { - warn!( - error = %err, - "GitHub installation token refresh failed; using cached token" - ); - return Ok(value.to_owned()); - } - } - return Err(err) - .context("failed to mint GitHub installation access token"); - } - } - } - - let token = cache - .as_ref() - .ok_or_else(|| anyhow::anyhow!("mintable token source has no cached token"))?; - token.valid_token().map(str::to_owned) - } - } - } -} - -#[cfg(test)] -mod tests { - use std::collections::VecDeque; - use std::sync::atomic::{AtomicUsize, Ordering}; - - use anyhow::anyhow; - - use super::*; - - enum MintAction { - Token(&'static str, chrono::DateTime), - Error(&'static str), - } - - struct MockMinter { - calls: AtomicUsize, - script: Mutex>, - } - - impl MockMinter { - fn new(script: Vec) -> Self { - Self { - calls: AtomicUsize::new(0), - script: Mutex::new(script.into()), - } - } - - fn calls(&self) -> usize { - self.calls.load(Ordering::SeqCst) - } - } - - #[async_trait::async_trait] - impl IatMinter for MockMinter { - async fn mint(&self) -> anyhow::Result { - self.calls.fetch_add(1, Ordering::SeqCst); - match self.script.lock().await.pop_front().expect("mint script") { - MintAction::Token(token, expires_at) => Ok(InstallationToken { - token: token.to_string(), - expires_at, - }), - MintAction::Error(message) => Err(anyhow!(message)), - } - } - } - - #[tokio::test] - async fn pat_returns_same_token_without_minting() { - let source = GitHubTokenSource::pat("ghp_pat".to_string()); - - assert_eq!(source.current_token().await.unwrap(), "ghp_pat"); - assert_eq!(source.current_token().await.unwrap(), "ghp_pat"); - assert!(!source.is_refreshable()); - } - - #[tokio::test] - async fn static_iat_returns_valid_token_and_rejects_expired_token() { - let valid = GitHubTokenSource::static_iat(InstallationToken { - token: "ghs_valid".to_string(), - expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), - }); - assert_eq!(valid.current_token().await.unwrap(), "ghs_valid"); - assert!(!valid.is_refreshable()); - - let expired = GitHubTokenSource::static_iat(InstallationToken { - token: "ghs_expired".to_string(), - expires_at: chrono::Utc::now() - chrono::Duration::seconds(1), - }); - assert!(expired.current_token().await.is_err()); - } - - #[tokio::test] - async fn mintable_reuses_cached_token_until_refresh_threshold() { - let minter = Arc::new(MockMinter::new(vec![MintAction::Token( - "ghs_cached", - chrono::Utc::now() + chrono::Duration::minutes(30), - )])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert!(source.is_refreshable()); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(minter.calls(), 1); - } - - #[tokio::test] - async fn mintable_refreshes_cached_token_near_expiry() { - let minter = Arc::new(MockMinter::new(vec![ - MintAction::Token( - "ghs_first", - chrono::Utc::now() + chrono::Duration::minutes(10), - ), - MintAction::Token( - "ghs_second", - chrono::Utc::now() + chrono::Duration::minutes(30), - ), - ])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert_eq!(source.current_token().await.unwrap(), "ghs_first"); - assert_eq!(source.current_token().await.unwrap(), "ghs_second"); - assert_eq!(minter.calls(), 2); - } - - #[tokio::test] - async fn mintable_uses_valid_cached_token_when_refresh_fails() { - let minter = Arc::new(MockMinter::new(vec![ - MintAction::Token( - "ghs_cached", - chrono::Utc::now() + chrono::Duration::minutes(10), - ), - MintAction::Error("mint failed"), - ])); - let source = GitHubTokenSource::mintable(minter.clone()); - - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(source.current_token().await.unwrap(), "ghs_cached"); - assert_eq!(minter.calls(), 2); - } - - #[tokio::test] - async fn mintable_errors_when_no_cached_token_can_cover_mint_failure() { - let minter = Arc::new(MockMinter::new(vec![MintAction::Error("mint failed")])); - let source = GitHubTokenSource::mintable(minter); - - let err = format!("{:#}", source.current_token().await.unwrap_err()); - assert!(err.contains("mint failed"), "got: {err}"); - } -} diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index d36ef8be8..82593c59b 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -1693,7 +1693,7 @@ mod tests { } #[async_trait::async_trait] - impl crate::github_token_source::IatMinter for RefreshingMinter { + impl fabro_github::test_support::InstallationTokenMinter for RefreshingMinter { async fn mint(&self) -> anyhow::Result { let call = self.calls.fetch_add(1, std::sync::atomic::Ordering::SeqCst) + 1; Ok(fabro_github::InstallationToken { @@ -1834,8 +1834,9 @@ mod tests { calls: std::sync::atomic::AtomicUsize::new(0), }); let mut services = make_sandbox_services(spy.clone()); - services.github_token = Some(std::sync::Arc::new( - crate::github_token_source::GitHubTokenSource::mintable(minter.clone()), + services.github_token = Some(fabro_github::test_support::installation_token_source( + "owner/repo", + minter.clone(), )); let handler = CommandHandler; diff --git a/lib/components/fabro-workflow/src/handler/llm/acp.rs b/lib/components/fabro-workflow/src/handler/llm/acp.rs index 4edcf3e81..cfd498beb 100644 --- a/lib/components/fabro-workflow/src/handler/llm/acp.rs +++ b/lib/components/fabro-workflow/src/handler/llm/acp.rs @@ -11,8 +11,7 @@ use fabro_acp::{ render_stop_reason, }; use fabro_agent::{ - AgentEvent, RefreshOutcome, RemoteCredentialAction, Sandbox, StaticEnvProvider, SteeringItem, - ToolEnvProvider, + AgentEvent, RefreshOutcome, Sandbox, StaticEnvProvider, SteeringItem, ToolEnvProvider, }; use fabro_github::token_source::REFRESH_MARGIN; use fabro_graphviz::graph::Node; @@ -115,12 +114,8 @@ fn push_cred_refresh_interval() -> Option { /// tick. Schedule from the token's own `expires_at` instead: wake when the /// cache margin opens, so that tick re-mints. `None` disables the loop — /// static credentials cannot be re-minted by waiting. -fn next_refresh_delay(outcome: &RefreshOutcome, fallback: Duration) -> Option { - let Some(token) = outcome.token else { - // No managed credentials to watch; keep the configured cadence in - // case a later tick sees them (e.g. after a reconnect). - return Some(fallback); - }; +fn next_refresh_delay(outcome: &RefreshOutcome) -> Option { + let token = outcome.token()?; let expires_at = token.expires_at()?; let margin = chrono::Duration::from_std(REFRESH_MARGIN).unwrap_or(chrono::Duration::MAX); let until_margin = ((expires_at - margin) - chrono::Utc::now()) @@ -140,9 +135,10 @@ async fn refresh_ahead_loop( sandbox: Arc, cancel: CancellationToken, interval: Duration, + initial_delay: Duration, ) { let retry_delay = interval.min(Duration::from_mins(1)); - let mut delay = interval; + let mut delay = initial_delay; loop { tokio::select! { () = cancel.cancelled() => break, @@ -151,26 +147,26 @@ async fn refresh_ahead_loop( .await { Ok(Ok(outcome)) => { - match outcome.action { - RemoteCredentialAction::Embedded => { + match outcome { + RefreshOutcome::Embedded(token) => { tracing::info!( - generation = outcome.token.map(|token| token.generation), + generation = token.generation, "refresh-ahead re-embedded push credentials mid-turn" ); } - RemoteCredentialAction::Unchanged => { + RefreshOutcome::Unchanged(token) => { tracing::debug!( - generation = outcome.token.map(|token| token.generation), + generation = token.generation, "refresh-ahead tick: embedded push credentials still fresh" ); } - RemoteCredentialAction::None => { + RefreshOutcome::None => { tracing::debug!( "refresh-ahead tick: no managed push credentials to refresh" ); } } - if let Some(next) = next_refresh_delay(&outcome, interval) { + if let Some(next) = next_refresh_delay(&outcome) { delay = next; } else { tracing::debug!( @@ -312,77 +308,57 @@ impl AgentAcpBackend { }) as Arc) + Send + Sync> }); - // Keep the sandbox's push credentials fresh for the duration of this ACP - // turn so the agent's own `git push` uses a live token instead of the one - // baked into the clone at run start. - // - // Part 2 (turn-entry): resolve through the cached token source and - // rewrite the origin URL before the ACP process spawns, covering a push - // early in the turn. A fresh cached token makes this a no-op exec-wise. - // Non-fatal and timeout-bounded — a stalled mint must neither fail nor - // hang node entry. Part 3 (loop): a background task keeps the embedded - // token fresh so a single turn that outlives the ~60-min - // installation-token TTL still pushes with a fresh token; ticks - // reschedule from the embedded token's expiry, so a normal short turn - // never ticks (the drop-guard aborts the task at turn end). - // - // FABRO_PUSH_CRED_REFRESH_AHEAD=0 (or false/off/no/empty, case- - // insensitive) disables the WHOLE feature — turn-entry refresh AND loop — - // so an operator who manages `origin` themselves can opt out of all - // fabro-side origin rewriting. FABRO_PUSH_CRED_REFRESH_INTERVAL_SECONDS - // overrides the loop cadence for ticks without token expiry info; 0 - // disables just the loop. - // - // Known limitations tracked as follow-ups (not addressed here): (a) - // resumed/parked runs reconnect the sandbox with no GitHub App creds, so - // refresh no-ops until those creds are threaded through the reconnect - // path; (b) the background `git remote set-url` can contend with the - // agent's own git on `.git/config.lock` (skipped entirely while the - // cached generation is already embedded); (c) parallel ACP branches each - // run their own loop; (d) this refresh lives in the ACP handler only, - // though the stale-origin problem is stage-type-agnostic (native/command - // stages that push are not covered); (e) refresh failures are logged via - // tracing but not surfaced as a RunNotice event on the run stream. + // Refresh before launch for early pushes. Schedule later refreshes from + // token expiry so the loop cannot sleep past the cache margin. let refresh_enabled = push_cred_refresh_enabled(); - if refresh_enabled { + let refresh_interval = refresh_enabled.then(push_cred_refresh_interval).flatten(); + let refresh_schedule = if refresh_enabled { match timeout(REFRESH_MINT_TIMEOUT, sandbox.refresh_push_credentials()).await { - Ok(Ok(outcome)) => match outcome.action { - RemoteCredentialAction::Embedded => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "refreshed sandbox push credentials at ACP turn entry" - ); + Ok(Ok(outcome)) => { + match outcome { + RefreshOutcome::Embedded(token) => { + tracing::debug!( + generation = token.generation, + "refreshed sandbox push credentials at ACP turn entry" + ); + } + RefreshOutcome::Unchanged(token) => { + tracing::debug!( + generation = token.generation, + "sandbox push credentials already fresh at ACP turn entry" + ); + } + RefreshOutcome::None => {} } - RemoteCredentialAction::Unchanged => { - tracing::debug!( - generation = outcome.token.map(|token| token.generation), - "sandbox push credentials already fresh at ACP turn entry" - ); - } - RemoteCredentialAction::None => {} - }, + refresh_interval.zip(next_refresh_delay(&outcome)) + } Ok(Err(e)) => { tracing::warn!( error = %fabro_sandbox::display_for_log(&e), "node-entry push-credential refresh failed (non-fatal)" ); + refresh_interval + .map(|interval| (interval, interval.min(Duration::from_mins(1)))) } Err(_elapsed) => { tracing::warn!( timeout_secs = REFRESH_MINT_TIMEOUT.as_secs(), "node-entry push-credential refresh timed out (non-fatal)" ); + refresh_interval + .map(|interval| (interval, interval.min(Duration::from_mins(1)))) } } - } - let _refresh_ahead_guard: Option = refresh_enabled - .then(push_cred_refresh_interval) - .flatten() - .map(|interval| { + } else { + None + }; + let _refresh_ahead_guard: Option = + refresh_schedule.map(|(interval, initial_delay)| { AbortOnDrop(tokio::spawn(refresh_ahead_loop( Arc::clone(sandbox), cancel_token.child_token(), interval, + initial_delay, ))) }); @@ -766,23 +742,22 @@ mod tests { expires_at, } }; - RefreshOutcome { - action, - token: Some(TokenSnapshot { - generation, - provenance, - }), + let token = TokenSnapshot { + generation, + provenance, + }; + match action { + RemoteCredentialAction::Embedded => RefreshOutcome::embedded(token), + RemoteCredentialAction::Unchanged => RefreshOutcome::unchanged(token), + RemoteCredentialAction::None => RefreshOutcome::none(), } } fn static_outcome() -> RefreshOutcome { - RefreshOutcome { - action: RemoteCredentialAction::Unchanged, - token: Some(TokenSnapshot { - generation: 0, - provenance: TokenProvenance::Static, - }), - } + RefreshOutcome::unchanged(TokenSnapshot { + generation: 0, + provenance: TokenProvenance::Static, + }) } #[test] @@ -794,7 +769,7 @@ mod tests { chrono::Duration::minutes(60), false, ); - let delay = next_refresh_delay(&outcome, Duration::from_mins(45)).unwrap(); + let delay = next_refresh_delay(&outcome).unwrap(); // Expiry minus the 10-minute refresh margin: ~50 minutes out. assert!(delay > Duration::from_mins(49), "{delay:?}"); assert!(delay <= Duration::from_mins(50), "{delay:?}"); @@ -809,26 +784,17 @@ mod tests { chrono::Duration::minutes(5), true, ); - assert_eq!( - next_refresh_delay(&outcome, Duration::from_mins(45)), - Some(REFRESH_RESCHEDULE_FLOOR) - ); + assert_eq!(next_refresh_delay(&outcome), Some(REFRESH_RESCHEDULE_FLOOR)); } #[test] fn next_refresh_delay_disables_the_loop_for_static_credentials() { - assert_eq!( - next_refresh_delay(&static_outcome(), Duration::from_mins(45)), - None - ); + assert_eq!(next_refresh_delay(&static_outcome()), None); } #[test] - fn next_refresh_delay_keeps_the_cadence_without_managed_credentials() { - assert_eq!( - next_refresh_delay(&RefreshOutcome::none(), Duration::from_mins(45)), - Some(Duration::from_mins(45)) - ); + fn next_refresh_delay_disables_the_loop_without_managed_credentials() { + assert_eq!(next_refresh_delay(&RefreshOutcome::none()), None); } /// Sandbox stub whose refresh outcomes are scripted, recording when each @@ -989,6 +955,7 @@ mod tests { Arc::clone(&sandbox) as Arc, cancel.clone(), interval, + interval, )); while sandbox.ticks().len() < 3 { @@ -1011,19 +978,41 @@ mod tests { } #[tokio::test(start_paused = true)] - async fn refresh_ahead_stops_by_itself_for_static_credentials() { - let sandbox = ScriptedRefreshSandbox::new(vec![static_outcome()]); + async fn refresh_ahead_honors_the_expiry_based_initial_delay() { + let interval = Duration::from_mins(45); + let entry_outcome = minted_outcome( + RemoteCredentialAction::Unchanged, + 1, + chrono::Duration::minutes(45), + chrono::Duration::minutes(15), + true, + ); + let initial_delay = next_refresh_delay(&entry_outcome).unwrap(); + let sandbox = ScriptedRefreshSandbox::new(vec![minted_outcome( + RemoteCredentialAction::Embedded, + 2, + chrono::Duration::zero(), + chrono::Duration::minutes(60), + false, + )]); let cancel = CancellationToken::new(); + let start = tokio::time::Instant::now(); let loop_task = tokio::spawn(refresh_ahead_loop( Arc::clone(&sandbox) as Arc, cancel.clone(), - Duration::from_mins(45), + interval, + initial_delay, )); - // The loop exits after the first tick without being cancelled: static - // credentials cannot be re-minted, so there is nothing to keep fresh. - loop_task.await.expect("refresh loop should stop by itself"); - assert_eq!(sandbox.ticks().len(), 1); + while sandbox.ticks().is_empty() { + tokio::time::sleep(Duration::from_secs(1)).await; + } + cancel.cancel(); + loop_task.await.expect("refresh loop should exit cleanly"); + + let first_tick = sandbox.ticks()[0] - start; + assert!(first_tick <= Duration::from_mins(5), "{first_tick:?}"); + assert!(first_tick > Duration::from_mins(4), "{first_tick:?}"); } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/lib.rs b/lib/components/fabro-workflow/src/lib.rs index c34bec62c..3178542d7 100644 --- a/lib/components/fabro-workflow/src/lib.rs +++ b/lib/components/fabro-workflow/src/lib.rs @@ -293,7 +293,6 @@ pub mod error; pub mod event; pub mod file_resolver; pub mod git; -pub mod github_token_source; pub(crate) mod graph; pub mod handler; mod hook_context; diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 2ccb3b6f2..4d4d529c4 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -7,6 +7,7 @@ use fabro_agent::{Sandbox, ToolSecrets}; use fabro_auth::{ CredentialSource, ExtraHeadersCredentialSource, VaultCredentialSource, auth_issue_message, }; +use fabro_github::token_source::InstallationTokenSource; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, HookRunner}; use fabro_model::Catalog; @@ -23,7 +24,6 @@ use super::types::{InitOptions, Initialized, LlmSpec, Persisted, SandboxEnvSpec} use crate::error::Error; use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::git::GitAuthor; -use crate::github_token_source::{AppIatMinter, GitHubTokenSource}; use crate::handler::llm::{AgentAcpBackend, AgentApiBackend, BackendRouter, routing}; use crate::handler::{HandlerRegistry, default_registry}; #[cfg(test)] @@ -37,7 +37,10 @@ use crate::services::{ use crate::stage_execution::{StageExecutionSeed, StageExecutionTracker}; use crate::steering_hub::SteeringHub; -type BuiltSandboxEnv = (HashMap, Option>); +type BuiltSandboxEnv = ( + HashMap, + Option>, +); async fn run_hooks( hook_runner: Option<&HookRunner>, @@ -99,12 +102,12 @@ fn build_sandbox_env( let source = match creds { fabro_github::GitHubCredentials::Pat(token) => { - Some(Arc::new(GitHubTokenSource::pat(token.clone()))) + Some(InstallationTokenSource::pat(token.clone())) } fabro_github::GitHubCredentials::Installation(token) => { - Some(Arc::new(GitHubTokenSource::static_iat(token.clone()))) + Some(InstallationTokenSource::installation(token.clone())) } - fabro_github::GitHubCredentials::App(app) => { + fabro_github::GitHubCredentials::App(_) => { let Some(origin_url) = spec.origin_url.as_deref() else { return Ok((env, None)); }; @@ -114,19 +117,11 @@ fn build_sandbox_env( let permissions = serde_json::to_value(permissions).map_err(|err| { Error::engine_with_source("Failed to serialize GitHub permissions", err) })?; - let http = fabro_http::http_client() - .map_err(|err| Error::engine_with_source("Failed to build HTTP client", err))?; - let install_url = app.installation_url(&owner); - let minter = AppIatMinter::new( - app.clone(), - http, - owner, - repo, - fabro_github::github_api_base_url(), - install_url, - permissions, - ); - Some(Arc::new(GitHubTokenSource::mintable(Arc::new(minter)))) + Some( + InstallationTokenSource::for_repository(creds, owner, repo, permissions).map_err( + |err| Error::engine_with_anyhow("Failed to build GitHub token source", err), + )?, + ) } }; @@ -458,7 +453,7 @@ pub async fn initialize( }); let github_token_refresh_managed = github_token .as_deref() - .is_some_and(GitHubTokenSource::is_refreshable); + .is_some_and(InstallationTokenSource::mints_installation_tokens); let (registry, effective_dry_run) = if let Some(registry) = options.registry_override.clone() { // A caller-supplied registry owns execution behavior for its handlers. (registry, options.dry_run) diff --git a/lib/components/fabro-workflow/src/run_metadata.rs b/lib/components/fabro-workflow/src/run_metadata.rs index 9fa002b57..76b9fa23d 100644 --- a/lib/components/fabro-workflow/src/run_metadata.rs +++ b/lib/components/fabro-workflow/src/run_metadata.rs @@ -222,19 +222,16 @@ pub(crate) fn build_metadata_writer( if !normalized_url.starts_with("https://") { return Ok(None); } - if fabro_github::parse_github_owner_repo(&normalized_url).is_err() { + let Ok((owner, repo)) = fabro_github::parse_github_owner_repo(&normalized_url) else { return Ok(None); - } + }; - // Share the sandbox's token source so the metadata writer reuses the - // same cached token as every other consumer for this origin. Resumed - // runs reconnect the sandbox without one; they build their own cached - // source from the run's credentials. let source = match token_source { Some(source) => source, - None => InstallationTokenSource::for_origin( + None => InstallationTokenSource::for_repository( creds, - &normalized_url, + owner, + repo, serde_json::json!({ "contents": "write" }), ) .map_err(RunMetadataError::TokenMint)?, diff --git a/lib/components/fabro-workflow/src/services.rs b/lib/components/fabro-workflow/src/services.rs index 2af76a9c7..64facc23e 100644 --- a/lib/components/fabro-workflow/src/services.rs +++ b/lib/components/fabro-workflow/src/services.rs @@ -8,6 +8,7 @@ use fabro_agent::{Sandbox, ToolEnvProvider}; use fabro_auth::CredentialSource; #[cfg(test)] use fabro_auth::ResolvedCredentials; +use fabro_github::token_source::InstallationTokenSource; use fabro_hooks::{HookContext, HookDecision, HookExecutionContext, HookRunner}; use fabro_interview::Interviewer; use fabro_model::{Catalog, ProviderId}; @@ -15,7 +16,6 @@ use fabro_types::{ManifestPath, RunId}; use tokio_util::sync::CancellationToken; use crate::event::Emitter; -use crate::github_token_source::GitHubTokenSource; use crate::handler::HandlerRegistry; use crate::interview_runtime::RunInterviewBlocker; use crate::run_metadata::{RunMetadataRuntime, RunMetadataWriterHandle}; @@ -238,7 +238,7 @@ pub struct EngineServices { /// Environment variables from `[sandbox.env]` config. pub base_env: HashMap, /// GitHub token source used to inject `GITHUB_TOKEN` at the point of use. - pub github_token: Option>, + pub github_token: Option>, /// Typed values from `[run.inputs]`, available to prompt templates. pub inputs: HashMap, /// When true, handlers should skip real execution and return simulated @@ -342,7 +342,7 @@ impl EngineServices { pub struct WorkflowToolEnvProvider { pub base_env: HashMap, - pub github_token: Option>, + pub github_token: Option>, } #[async_trait::async_trait] @@ -354,11 +354,15 @@ impl ToolEnvProvider for WorkflowToolEnvProvider { async fn resolve_workflow_env( base_env: &HashMap, - github_token: Option<&Arc>, + github_token: Option<&Arc>, ) -> anyhow::Result> { let mut env = base_env.clone(); if let Some(source) = github_token { - env.insert("GITHUB_TOKEN".to_string(), source.current_token().await?); + let resolved = source.resolve().await?; + env.insert( + "GITHUB_TOKEN".to_string(), + resolved.token.expose().to_owned(), + ); } Ok(env) } @@ -371,9 +375,10 @@ mod tests { use anyhow::anyhow; use fabro_agent::ToolEnvProvider as _; use fabro_github::InstallationToken; + use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; + use fabro_github::token_source::InstallationTokenSource; use super::{EngineServices, WorkflowToolEnvProvider}; - use crate::github_token_source::{GitHubTokenSource, IatMinter}; #[tokio::test] async fn test_default_uses_stub_credential_source() { @@ -406,7 +411,7 @@ mod tests { async fn workflow_tool_env_provider_merges_current_github_token() { let provider = WorkflowToolEnvProvider { base_env: HashMap::from([("FOO".to_string(), "bar".to_string())]), - github_token: Some(Arc::new(GitHubTokenSource::pat("ghp_pat".to_string()))), + github_token: Some(InstallationTokenSource::pat("ghp_pat".to_string())), }; let env = provider.resolve().await.unwrap(); @@ -418,7 +423,7 @@ mod tests { struct FailingMinter; #[async_trait::async_trait] - impl IatMinter for FailingMinter { + impl InstallationTokenMinter for FailingMinter { async fn mint(&self) -> anyhow::Result { Err(anyhow!("GITHUB_TOKEN refresh failed")) } @@ -428,9 +433,10 @@ mod tests { async fn workflow_tool_env_provider_propagates_token_refresh_errors() { let provider = WorkflowToolEnvProvider { base_env: HashMap::new(), - github_token: Some(Arc::new(GitHubTokenSource::mintable(Arc::new( - FailingMinter, - )))), + github_token: Some(installation_token_source( + "owner/repo", + Arc::new(FailingMinter), + )), }; let err = format!("{:#}", provider.resolve().await.unwrap_err());