mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-10 22:43:37 +00:00
fix: harden GitHub token refresh handling
This commit is contained in:
parent
579f3db26f
commit
f8a82d6865
20 changed files with 365 additions and 512 deletions
|
|
@ -9,6 +9,9 @@ description = "GitHub App authentication and API helpers for Fabro"
|
|||
[lib]
|
||||
doctest = false
|
||||
|
||||
[features]
|
||||
test-support = []
|
||||
|
||||
[lints]
|
||||
workspace = true
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
26
lib/components/fabro-github/src/test_support.rs
Normal file
26
lib/components/fabro-github/src/test_support.rs
Normal file
|
|
@ -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<InstallationToken>;
|
||||
}
|
||||
|
||||
struct TestMinterAdapter(Arc<dyn InstallationTokenMinter>);
|
||||
|
||||
#[async_trait::async_trait]
|
||||
impl InnerMinter for TestMinterAdapter {
|
||||
async fn mint(&self) -> anyhow::Result<InstallationToken> {
|
||||
self.0.mint().await
|
||||
}
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn installation_token_source(
|
||||
repo: impl Into<String>,
|
||||
minter: Arc<dyn InstallationTokenMinter>,
|
||||
) -> Arc<InstallationTokenSource> {
|
||||
InstallationTokenSource::with_minter(repo.into(), Box::new(TestMinterAdapter(minter)))
|
||||
}
|
||||
|
|
@ -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<InstallationToken>;
|
||||
}
|
||||
|
||||
|
|
@ -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<Arc<Self>> {
|
||||
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<dyn InstallationTokenMinter>) -> Arc<Self> {
|
||||
pub fn pat(token: String) -> Arc<Self> {
|
||||
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<Self> {
|
||||
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<dyn InstallationTokenMinter>) -> Arc<Self> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -336,7 +336,6 @@ pub struct DaytonaSandbox {
|
|||
config: DaytonaConfig,
|
||||
client: daytona_sdk::Client,
|
||||
api_key: Option<String>,
|
||||
github_app: Option<GitHubCredentials>,
|
||||
push_credentials: PushCredentialState,
|
||||
sandbox: OnceCell<daytona_sdk::Sandbox>,
|
||||
snapshot_name: OnceCell<String>,
|
||||
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -133,7 +133,6 @@ impl Default for DockerSandboxOptions {
|
|||
pub struct DockerSandbox {
|
||||
docker: Docker,
|
||||
config: DockerSandboxOptions,
|
||||
github_app: Option<GitHubCredentials>,
|
||||
push_credentials: PushCredentialState,
|
||||
run_id: Option<RunId>,
|
||||
clone_origin_url: Option<String>,
|
||||
|
|
@ -164,7 +163,7 @@ enum ContainerStartAction {
|
|||
impl DockerSandbox {
|
||||
pub fn new(
|
||||
config: DockerSandboxOptions,
|
||||
github_app: Option<GitHubCredentials>,
|
||||
github_app: Option<&GitHubCredentials>,
|
||||
run_id: Option<RunId>,
|
||||
clone_origin_url: Option<String>,
|
||||
clone_branch: Option<String>,
|
||||
|
|
@ -183,19 +182,18 @@ impl DockerSandbox {
|
|||
fn with_docker_client(
|
||||
docker: Docker,
|
||||
config: DockerSandboxOptions,
|
||||
github_app: Option<GitHubCredentials>,
|
||||
github_app: Option<&GitHubCredentials>,
|
||||
run_id: Option<RunId>,
|
||||
clone_origin_url: Option<String>,
|
||||
clone_branch: Option<String>,
|
||||
) -> crate::Result<Self> {
|
||||
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,
|
||||
)
|
||||
},
|
||||
)?,
|
||||
),
|
||||
|
|
|
|||
|
|
@ -18,6 +18,13 @@ pub enum Error {
|
|||
source: Box<dyn std::error::Error + Send + Sync + 'static>,
|
||||
},
|
||||
|
||||
#[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<String>, source: anyhow::Error) -> Self {
|
||||
Self::AnyhowContext {
|
||||
message: message.into(),
|
||||
source,
|
||||
}
|
||||
}
|
||||
|
||||
pub fn exec(label: impl Into<String>, result: ExecResult) -> Self {
|
||||
Self::Exec {
|
||||
label: label.into(),
|
||||
|
|
|
|||
|
|
@ -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(|| {
|
||||
|
|
|
|||
|
|
@ -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]
|
||||
|
|
|
|||
|
|
@ -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<TokenSnapshot>,
|
||||
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<TokenSnapshot> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -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"] }
|
||||
|
|
|
|||
|
|
@ -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<InstallationToken>;
|
||||
}
|
||||
|
||||
pub struct AppIatMinter {
|
||||
creds: GitHubAppCredentials,
|
||||
http: fabro_http::HttpClient,
|
||||
owner: String,
|
||||
repo: String,
|
||||
api_base: String,
|
||||
install_url: Option<String>,
|
||||
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<String>,
|
||||
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<InstallationToken> {
|
||||
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<dyn IatMinter>,
|
||||
cache: Mutex<Option<InstallationToken>>,
|
||||
},
|
||||
}
|
||||
|
||||
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<dyn IatMinter>) -> 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<String> {
|
||||
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<chrono::Utc>),
|
||||
Error(&'static str),
|
||||
}
|
||||
|
||||
struct MockMinter {
|
||||
calls: AtomicUsize,
|
||||
script: Mutex<VecDeque<MintAction>>,
|
||||
}
|
||||
|
||||
impl MockMinter {
|
||||
fn new(script: Vec<MintAction>) -> 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<InstallationToken> {
|
||||
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}");
|
||||
}
|
||||
}
|
||||
|
|
@ -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<fabro_github::InstallationToken> {
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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<Duration> {
|
|||
/// 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<Duration> {
|
||||
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<Duration> {
|
||||
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<dyn Sandbox>,
|
||||
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<dyn Fn(String, Option<Principal>) + 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<AbortOnDrop> = refresh_enabled
|
||||
.then(push_cred_refresh_interval)
|
||||
.flatten()
|
||||
.map(|interval| {
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let _refresh_ahead_guard: Option<AbortOnDrop> =
|
||||
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<dyn Sandbox>,
|
||||
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<dyn Sandbox>,
|
||||
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]
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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<String, String>, Option<Arc<GitHubTokenSource>>);
|
||||
type BuiltSandboxEnv = (
|
||||
HashMap<String, String>,
|
||||
Option<Arc<InstallationTokenSource>>,
|
||||
);
|
||||
|
||||
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)
|
||||
|
|
|
|||
|
|
@ -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)?,
|
||||
|
|
|
|||
|
|
@ -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<String, String>,
|
||||
/// GitHub token source used to inject `GITHUB_TOKEN` at the point of use.
|
||||
pub github_token: Option<Arc<GitHubTokenSource>>,
|
||||
pub github_token: Option<Arc<InstallationTokenSource>>,
|
||||
/// Typed values from `[run.inputs]`, available to prompt templates.
|
||||
pub inputs: HashMap<String, toml::Value>,
|
||||
/// When true, handlers should skip real execution and return simulated
|
||||
|
|
@ -342,7 +342,7 @@ impl EngineServices {
|
|||
|
||||
pub struct WorkflowToolEnvProvider {
|
||||
pub base_env: HashMap<String, String>,
|
||||
pub github_token: Option<Arc<GitHubTokenSource>>,
|
||||
pub github_token: Option<Arc<InstallationTokenSource>>,
|
||||
}
|
||||
|
||||
#[async_trait::async_trait]
|
||||
|
|
@ -354,11 +354,15 @@ impl ToolEnvProvider for WorkflowToolEnvProvider {
|
|||
|
||||
async fn resolve_workflow_env(
|
||||
base_env: &HashMap<String, String>,
|
||||
github_token: Option<&Arc<GitHubTokenSource>>,
|
||||
github_token: Option<&Arc<InstallationTokenSource>>,
|
||||
) -> anyhow::Result<HashMap<String, String>> {
|
||||
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<InstallationToken> {
|
||||
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());
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue