From f1c247bc0f52c01e63d608a1986032f8cf9eb4c3 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 6 May 2026 06:59:34 -0400 Subject: [PATCH] fix(github): refresh installation tokens during workflows Split PATs from installation access tokens so static configuration cannot accidentally store expiring ghs_* credentials. Workflow command and API agent stages now resolve GITHUB_TOKEN lazily from a refreshable source, while CLI agent stages surface their launch-time refresh limitation. --- Cargo.toml | 2 +- lib/crates/fabro-agent/src/lib.rs | 5 +- lib/crates/fabro-agent/src/mcp_integration.rs | 2 +- lib/crates/fabro-agent/src/session.rs | 93 +++++- lib/crates/fabro-agent/src/skills.rs | 6 +- lib/crates/fabro-agent/src/tool_execution.rs | 33 ++- lib/crates/fabro-agent/src/tool_registry.rs | 18 +- lib/crates/fabro-agent/src/tools.rs | 228 ++++++++++---- lib/crates/fabro-cli/src/commands/install.rs | 3 + lib/crates/fabro-cli/src/shared/github.rs | 5 +- lib/crates/fabro-github/src/lib.rs | 213 ++++++++++++-- lib/crates/fabro-hooks/src/executor.rs | 6 +- lib/crates/fabro-server/src/diagnostics.rs | 18 +- lib/crates/fabro-server/src/run_manifest.rs | 8 +- lib/crates/fabro-server/src/serve.rs | 19 +- lib/crates/fabro-server/src/server.rs | 6 +- .../fabro-server/src/server/handler/system.rs | 11 +- lib/crates/fabro-tracker/src/github.rs | 33 ++- lib/crates/fabro-types/src/run_event/infra.rs | 1 + .../fabro-workflow/src/github_token_source.rs | 277 ++++++++++++++++++ .../fabro-workflow/src/handler/command.rs | 82 +++++- .../fabro-workflow/src/handler/llm/api.rs | 31 +- .../fabro-workflow/src/handler/llm/cli.rs | 54 +++- .../src/handler/manager_loop.rs | 6 +- .../fabro-workflow/src/handler/parallel.rs | 6 +- lib/crates/fabro-workflow/src/lib.rs | 1 + .../fabro-workflow/src/pipeline/initialize.rs | 128 ++++---- .../src/pipeline/pull_request.rs | 2 +- lib/crates/fabro-workflow/src/run_metadata.rs | 7 +- lib/crates/fabro-workflow/src/services.rs | 98 ++++++- lib/crates/fabro-workflow/src/test_support.rs | 3 +- 31 files changed, 1167 insertions(+), 238 deletions(-) create mode 100644 lib/crates/fabro-workflow/src/github_token_source.rs diff --git a/Cargo.toml b/Cargo.toml index 4084e2403..726d8ad42 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,7 +32,7 @@ tokio-util = "0.7" clap = { version = "4", features = ["derive", "env"] } clap_complete = "4" jsonschema = { version = "0.42", default-features = false } -chrono = { version = "0.4", features = ["clock"] } +chrono = { version = "0.4", features = ["clock", "serde"] } dashmap = "6" bollard = "0.18" tar = "0.4" diff --git a/lib/crates/fabro-agent/src/lib.rs b/lib/crates/fabro-agent/src/lib.rs index 74e1d9e10..9f75a68e7 100644 --- a/lib/crates/fabro-agent/src/lib.rs +++ b/lib/crates/fabro-agent/src/lib.rs @@ -44,7 +44,10 @@ pub use sandbox::{ SandboxEvent, SandboxEventCallback, WorktreeEvent, WorktreeEventCallback, WorktreeOptions, WorktreeSandbox, format_lines_numbered, shell_quote, }; -pub use session::{CompletionCoordinator, Session, SessionControlHandle, SteeringItem}; +pub use session::{ + CompletionCoordinator, Session, SessionControlHandle, StaticEnvProvider, SteeringItem, + ToolEnvProvider, +}; pub use skills::Skill; pub use subagent::{ SubAgent, SubAgentEventCallback, SubAgentManager, SubAgentResult, SubAgentStatus, diff --git a/lib/crates/fabro-agent/src/mcp_integration.rs b/lib/crates/fabro-agent/src/mcp_integration.rs index da6154cbc..ed4ef4574 100644 --- a/lib/crates/fabro-agent/src/mcp_integration.rs +++ b/lib/crates/fabro-agent/src/mcp_integration.rs @@ -94,7 +94,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; diff --git a/lib/crates/fabro-agent/src/session.rs b/lib/crates/fabro-agent/src/session.rs index ef2e669d1..b6171c8a2 100644 --- a/lib/crates/fabro-agent/src/session.rs +++ b/lib/crates/fabro-agent/src/session.rs @@ -228,6 +228,20 @@ impl SessionControlHandle { } } +#[async_trait::async_trait] +pub trait ToolEnvProvider: Send + Sync { + async fn resolve(&self) -> anyhow::Result>; +} + +pub struct StaticEnvProvider(pub HashMap); + +#[async_trait::async_trait] +impl ToolEnvProvider for StaticEnvProvider { + async fn resolve(&self) -> anyhow::Result> { + Ok(self.0.clone()) + } +} + pub struct Session { id: String, config: SessionOptions, @@ -248,7 +262,7 @@ pub struct Session { skills: Vec, system_prompt: String, file_tracker: FileTracker, - tool_env: Option>, + tool_env_provider: Option>, subagent_manager: Option>>, completion_coordinator: Option>, } @@ -282,7 +296,7 @@ impl Session { skills: Vec::new(), system_prompt: String::new(), file_tracker: FileTracker::default(), - tool_env: None, + tool_env_provider: None, subagent_manager, completion_coordinator: None, } @@ -315,8 +329,12 @@ impl Session { )) } + pub fn set_tool_env_provider(&mut self, provider: Arc) { + self.tool_env_provider = Some(provider); + } + pub fn set_tool_env(&mut self, env: HashMap) { - self.tool_env = Some(env); + self.set_tool_env_provider(Arc::new(StaticEnvProvider(env))); } #[must_use] @@ -1356,7 +1374,7 @@ impl Session { &self.config, &self.event_emitter, &self.id, - self.tool_env.as_ref(), + self.tool_env_provider.as_ref(), ) .await; composite_watcher.abort(); @@ -1545,6 +1563,7 @@ mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; use std::time::Duration; + use anyhow::Context as _; use fabro_llm::error::{ProviderErrorDetail, ProviderErrorKind}; use fabro_llm::provider::{ProviderAdapter, StreamEventStream}; use fabro_llm::types::{ @@ -1708,6 +1727,72 @@ mod tests { } } + struct SequenceToolEnvProvider { + values: Mutex>>, + } + + #[async_trait::async_trait] + impl ToolEnvProvider for SequenceToolEnvProvider { + async fn resolve(&self) -> anyhow::Result> { + self.values + .lock() + .unwrap() + .pop_front() + .context("env script exhausted") + } + } + + #[tokio::test] + async fn session_passes_tool_env_provider_to_each_tool_round() { + let seen_tokens = Arc::new(Mutex::new(Vec::new())); + let seen_tokens_for_tool = Arc::clone(&seen_tokens); + let record_env_tool = RegisteredTool { + definition: ToolDefinition { + name: "record_env".into(), + description: "Records resolved env".into(), + parameters: serde_json::json!({"type": "object"}), + }, + executor: Arc::new(move |_args, ctx| { + let seen_tokens = Arc::clone(&seen_tokens_for_tool); + Box::pin(async move { + let env = ctx + .resolve_tool_env() + .await + .map_err(|err| format!("{err:#}"))? + .unwrap_or_default(); + seen_tokens.lock().unwrap().push( + env.get("GITHUB_TOKEN") + .cloned() + .unwrap_or_else(|| "".to_string()), + ); + Ok("recorded".to_string()) + }) + }), + }; + + let mut registry = ToolRegistry::new(); + registry.register(record_env_tool); + let responses = vec![ + tool_call_response("record_env", "call_1", serde_json::json!({})), + tool_call_response("record_env", "call_2", serde_json::json!({})), + text_response("Done!"), + ]; + let mut session = make_session_with_tools(responses, registry).await; + session.set_tool_env_provider(Arc::new(SequenceToolEnvProvider { + values: Mutex::new(VecDeque::from([ + HashMap::from([("GITHUB_TOKEN".to_string(), "t1".to_string())]), + HashMap::from([("GITHUB_TOKEN".to_string(), "t2".to_string())]), + ])), + })); + + session.process_input("Use tools").await.unwrap(); + + assert_eq!(seen_tokens.lock().unwrap().as_slice(), [ + "t1".to_string(), + "t2".to_string() + ]); + } + #[tokio::test] async fn max_tool_rounds_enforced() { let mut registry = ToolRegistry::new(); diff --git a/lib/crates/fabro-agent/src/skills.rs b/lib/crates/fabro-agent/src/skills.rs index fe5e3445a..2389a5082 100644 --- a/lib/crates/fabro-agent/src/skills.rs +++ b/lib/crates/fabro-agent/src/skills.rs @@ -581,7 +581,7 @@ name: trimmed let ctx = ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }; let result = (tool.executor)(args, ctx).await; assert_eq!( @@ -600,7 +600,7 @@ name: trimmed let ctx = ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }; let result = (tool.executor)(args, ctx).await; assert!(result.is_err()); @@ -617,7 +617,7 @@ name: trimmed let ctx = ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }; let result = (tool.executor)(args, ctx).await; assert!(result.is_err()); diff --git a/lib/crates/fabro-agent/src/tool_execution.rs b/lib/crates/fabro-agent/src/tool_execution.rs index d4ac99489..dd40c6c33 100644 --- a/lib/crates/fabro-agent/src/tool_execution.rs +++ b/lib/crates/fabro-agent/src/tool_execution.rs @@ -1,4 +1,3 @@ -use std::collections::HashMap; use std::sync::Arc; use fabro_llm::types::{ToolCall, ToolResult}; @@ -9,6 +8,7 @@ use tracing::debug; use crate::config::{SessionOptions, ToolHookCallback, ToolHookDecision}; use crate::event::Emitter; use crate::sandbox::Sandbox; +use crate::session::ToolEnvProvider; use crate::tool_registry::{RegisteredTool, ToolContext, ToolRegistry}; use crate::truncation::truncate_tool_output; use crate::types::AgentEvent; @@ -29,7 +29,7 @@ pub async fn execute_tool_calls( config: &SessionOptions, emitter: &Emitter, session_id: &str, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> Vec { if parallel && tool_calls.len() > 1 { execute_tool_calls_parallel( @@ -41,7 +41,7 @@ pub async fn execute_tool_calls( config, emitter, session_id, - tool_env, + tool_env_provider, ) .await } else { @@ -54,7 +54,7 @@ pub async fn execute_tool_calls( config, emitter, session_id, - tool_env, + tool_env_provider, ) .await } @@ -73,7 +73,7 @@ async fn execute_tool_calls_sequential( config: &SessionOptions, emitter: &Emitter, session_id: &str, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> Vec { let mut results = Vec::new(); for tc in tool_calls { @@ -91,7 +91,7 @@ async fn execute_tool_calls_sequential( config, emitter, session_id, - tool_env, + tool_env_provider, ) .await; results.push(result); @@ -112,9 +112,9 @@ async fn execute_tool_calls_parallel( config: &SessionOptions, emitter: &Emitter, session_id: &str, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> Vec { - let tool_env = tool_env.cloned(); + let tool_env_provider = tool_env_provider.cloned(); let futures: Vec<_> = tool_calls .iter() .map(|tc| { @@ -125,7 +125,7 @@ async fn execute_tool_calls_parallel( let tc = tc.clone(); let session_id = session_id.to_owned(); let tool_hooks = tool_hooks.cloned(); - let tool_env = tool_env.clone(); + let tool_env_provider = tool_env_provider.clone(); // Look up the tool before spawning since ToolRegistry is not Send. let registered_tool = registry.get(&tc.name).cloned(); async move { @@ -138,7 +138,7 @@ async fn execute_tool_calls_parallel( &config, &emitter, &session_id, - tool_env.as_ref(), + tool_env_provider.as_ref(), ) .await } @@ -162,7 +162,7 @@ pub async fn execute_and_emit_one_tool( config: &SessionOptions, emitter: &Emitter, session_id: &str, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> ToolResult { execute_and_emit_one_tool_with_lookup( tc, @@ -173,7 +173,7 @@ pub async fn execute_and_emit_one_tool( config, emitter, session_id, - tool_env, + tool_env_provider, ) .await } @@ -193,7 +193,7 @@ async fn execute_and_emit_one_tool_with_lookup( config: &SessionOptions, emitter: &Emitter, session_id: &str, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> ToolResult { emitter.emit(session_id.to_owned(), AgentEvent::ToolCallStarted { tool_name: tc.name.clone(), @@ -226,7 +226,7 @@ async fn execute_and_emit_one_tool_with_lookup( } } - let result = execute_one_tool(tc, registered_tool, env, cancel_token, tool_env).await; + let result = execute_one_tool(tc, registered_tool, env, cancel_token, tool_env_provider).await; emitter.emit(session_id.to_owned(), AgentEvent::ToolCallOutputDelta { delta: result.content.to_string(), @@ -270,7 +270,7 @@ async fn execute_one_tool( registered_tool: Option<&RegisteredTool>, env: Arc, cancel_token: CancellationToken, - tool_env: Option<&HashMap>, + tool_env_provider: Option<&Arc>, ) -> ToolResult { match registered_tool { Some(tool) => { @@ -283,7 +283,7 @@ async fn execute_one_tool( let ctx = ToolContext { env, cancel: cancel_token, - tool_env: tool_env.cloned(), + tool_env_provider: tool_env_provider.cloned(), }; match (tool.executor)(tc.arguments.clone(), ctx).await { Ok(output) => ToolResult::success(&tc.id, serde_json::json!(output)), @@ -347,6 +347,7 @@ pub fn validate_tool_args( #[cfg(test)] mod tests { + use std::collections::HashMap; use std::sync::Mutex; use fabro_llm::types::{ToolCall, ToolDefinition}; diff --git a/lib/crates/fabro-agent/src/tool_registry.rs b/lib/crates/fabro-agent/src/tool_registry.rs index cc0984249..59a527e2b 100644 --- a/lib/crates/fabro-agent/src/tool_registry.rs +++ b/lib/crates/fabro-agent/src/tool_registry.rs @@ -7,11 +7,21 @@ use fabro_llm::types::ToolDefinition; use tokio_util::sync::CancellationToken; use crate::sandbox::Sandbox; +use crate::session::ToolEnvProvider; pub struct ToolContext { - pub env: Arc, - pub cancel: CancellationToken, - pub tool_env: Option>, + pub env: Arc, + pub cancel: CancellationToken, + pub tool_env_provider: Option>, +} + +impl ToolContext { + pub async fn resolve_tool_env(&self) -> anyhow::Result>> { + match &self.tool_env_provider { + Some(provider) => Ok(Some(provider.resolve().await?)), + None => Ok(None), + } + } } pub type ToolExecutor = Arc< @@ -179,7 +189,7 @@ mod tests { let ctx = ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }; let result = (tool.executor)(serde_json::json!({}), ctx).await; assert_eq!(result.unwrap(), "ok"); diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 95eb55e34..351798693 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -240,11 +240,9 @@ pub fn make_shell_tool_with_config(config: &SessionOptions) -> RegisteredTool { .unwrap_or(default_timeout) .min(max_timeout); + let tool_env = ctx.resolve_tool_env().await.map_err(|e| format!("{e:#}"))?; tracing::debug!( - env_var_count = ctx - .tool_env - .as_ref() - .map_or(0, std::collections::HashMap::len), + env_var_count = tool_env.as_ref().map_or(0, std::collections::HashMap::len), "Injecting sandbox env vars into tool execution" ); let result = ctx @@ -253,7 +251,7 @@ pub fn make_shell_tool_with_config(config: &SessionOptions) -> RegisteredTool { command, timeout_ms, None, - ctx.tool_env.as_ref(), + tool_env.as_ref(), Some(ctx.cancel), ) .await @@ -609,13 +607,14 @@ pub(crate) fn make_web_fetch_tool(summarizer: Option) -> Reg "curl -sL --max-time {timeout_secs} -H 'User-Agent: fabro-agent/0.1' {escaped_url}" ); + let tool_env = ctx.resolve_tool_env().await.map_err(|e| format!("{e:#}"))?; let result = ctx .env .exec_command( &command, timeout_ms, None, - ctx.tool_env.as_ref(), + tool_env.as_ref(), Some(ctx.cancel), ) .await @@ -698,7 +697,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"file_path": "/test.txt"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; assert_eq!(result.unwrap(), " 1 | hello\n 2 | world"); @@ -722,7 +721,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -737,9 +736,9 @@ mod tests { let result = (tool.executor)( serde_json::json!({"file_path": "/out.txt", "content": "hello"}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -767,9 +766,9 @@ mod tests { "new_string": "goodbye" }), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -797,7 +796,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -822,7 +821,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -849,9 +848,9 @@ mod tests { "replace_all": true }), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -877,7 +876,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"command": "echo hello"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let output = result.unwrap(); @@ -893,9 +892,9 @@ mod tests { let _result = (tool.executor)( serde_json::json!({"command": "sleep 1", "timeout_ms": 5000}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -918,7 +917,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"command": "false"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let output = result.unwrap(); @@ -942,7 +941,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"command": "sleep 100"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let output = result.unwrap(); @@ -959,9 +958,9 @@ mod tests { let _result = (tool.executor)( serde_json::json!({"command": "echo $MY_KEY"}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: Some(tool_env.clone()), + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: Some(Arc::new(crate::StaticEnvProvider(tool_env.clone()))), }, ) .await; @@ -969,15 +968,124 @@ mod tests { assert_eq!(captured, Some(tool_env)); } + struct SequenceToolEnvProvider { + values: std::sync::Mutex>>, + } + + #[async_trait::async_trait] + impl crate::ToolEnvProvider for SequenceToolEnvProvider { + async fn resolve(&self) -> anyhow::Result> { + Ok(self.values.lock().unwrap().remove(0)) + } + } + + struct FailingToolEnvProvider; + + #[async_trait::async_trait] + impl crate::ToolEnvProvider for FailingToolEnvProvider { + async fn resolve(&self) -> anyhow::Result> { + Err(anyhow::anyhow!("GITHUB_TOKEN refresh failed")) + } + } + + #[tokio::test] + async fn shell_resolves_tool_env_for_each_call() { + let tool = make_shell_tool(); + let env = Arc::new(MockSandbox::default()); + let provider = Arc::new(SequenceToolEnvProvider { + values: std::sync::Mutex::new(vec![ + HashMap::from([("GITHUB_TOKEN".to_string(), "t1".to_string())]), + HashMap::from([("GITHUB_TOKEN".to_string(), "t2".to_string())]), + ]), + }); + + let _result = (tool.executor)( + serde_json::json!({"command": "echo $GITHUB_TOKEN"}), + ToolContext { + env: env.clone(), + cancel: CancellationToken::new(), + tool_env_provider: Some(provider.clone()), + }, + ) + .await; + assert_eq!( + env.captured_env_vars.lock().unwrap().clone(), + Some(HashMap::from([( + "GITHUB_TOKEN".to_string(), + "t1".to_string() + )])) + ); + + let _result = (tool.executor)( + serde_json::json!({"command": "echo $GITHUB_TOKEN"}), + ToolContext { + env: env.clone(), + cancel: CancellationToken::new(), + tool_env_provider: Some(provider), + }, + ) + .await; + assert_eq!( + env.captured_env_vars.lock().unwrap().clone(), + Some(HashMap::from([( + "GITHUB_TOKEN".to_string(), + "t2".to_string() + )])) + ); + } + + #[tokio::test] + async fn shell_returns_provider_error_for_env_resolution_failure() { + let tool = make_shell_tool(); + let env: Arc = Arc::new(MockSandbox::default()); + + let result = (tool.executor)( + serde_json::json!({"command": "echo $GITHUB_TOKEN"}), + ToolContext { + env, + cancel: CancellationToken::new(), + tool_env_provider: Some(Arc::new(FailingToolEnvProvider)), + }, + ) + .await + .unwrap_err(); + + assert!( + result.contains("GITHUB_TOKEN refresh failed"), + "got: {result}" + ); + } + + #[tokio::test] + async fn read_file_does_not_resolve_failing_tool_env_provider() { + let tool = make_read_file_tool(); + let mut files = HashMap::new(); + files.insert("/test.txt".into(), " 1 | hello".into()); + let env: Arc = Arc::new(MockSandbox { + files, + apply_read_offset_limit: true, + ..Default::default() + }); + + let result = (tool.executor)(serde_json::json!({"file_path": "/test.txt"}), ToolContext { + env, + cancel: CancellationToken::new(), + tool_env_provider: Some(Arc::new(FailingToolEnvProvider)), + }) + .await; + + assert_eq!(result.unwrap(), " 1 | hello"); + } + #[tokio::test] async fn shell_passes_none_env_when_tool_env_is_none() { let tool = make_shell_tool(); let env = Arc::new(MockSandbox::default()); let env_clone: Arc = env.clone(); let _result = (tool.executor)(serde_json::json!({"command": "echo hello"}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }) .await; let captured = env.captured_env_vars.lock().unwrap().clone(); @@ -1003,9 +1111,9 @@ mod tests { let _result = (tool.executor)( serde_json::json!({"url": "https://example.com"}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: Some(tool_env.clone()), + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: Some(Arc::new(crate::StaticEnvProvider(tool_env.clone()))), }, ) .await; @@ -1026,7 +1134,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"pattern": "fn"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let output = result.unwrap(); @@ -1044,7 +1152,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"pattern": "src/**/*.rs"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let output = result.unwrap(); @@ -1059,7 +1167,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({"query": "test"}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let err = result.unwrap_err(); @@ -1076,7 +1184,7 @@ mod tests { let result = (tool.executor)(serde_json::json!({}), ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }) .await; let err = result.unwrap_err(); @@ -1126,9 +1234,9 @@ mod tests { let result = (tool.executor)( serde_json::json!({"url": "https://example.com"}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -1165,7 +1273,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1184,9 +1292,9 @@ mod tests { let _result = (tool.executor)( serde_json::json!({"url": "https://example.com", "timeout_ms": 15000}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -1206,9 +1314,9 @@ mod tests { let _result = (tool.executor)( serde_json::json!({"url": "https://example.com", "timeout_ms": 120_000}), ToolContext { - env: env_clone, - cancel: CancellationToken::new(), - tool_env: None, + env: env_clone, + cancel: CancellationToken::new(), + tool_env_provider: None, }, ) .await; @@ -1239,7 +1347,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1266,7 +1374,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1314,7 +1422,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1345,7 +1453,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1414,7 +1522,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1468,7 +1576,7 @@ mod tests { ToolContext { env, cancel: CancellationToken::new(), - tool_env: None, + tool_env_provider: None, }, ) .await; @@ -1492,9 +1600,9 @@ mod tests { // read_file tool should mark the file as agent-read let tool = make_read_file_tool(); (tool.executor)(serde_json::json!({"file_path": "a.ts"}), ToolContext { - env: Arc::clone(&env), - cancel: CancellationToken::new(), - tool_env: None, + env: Arc::clone(&env), + cancel: CancellationToken::new(), + tool_env_provider: None, }) .await .unwrap(); @@ -1521,9 +1629,9 @@ mod tests { // grep tool should mark matched files as agent-read let tool = make_grep_tool(); (tool.executor)(serde_json::json!({"pattern": "content"}), ToolContext { - env: Arc::clone(&env), - cancel: CancellationToken::new(), - tool_env: None, + env: Arc::clone(&env), + cancel: CancellationToken::new(), + tool_env_provider: None, }) .await .unwrap(); diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index f6f2e0411..04a742a47 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -463,6 +463,7 @@ impl InstallInputSource for InteractiveInstallInputSource { } else { spawn_blocking(|| prompt_password("GitHub Personal Access Token")).await?? }; + fabro_github::validate_static_github_token(&token)?; Ok(GitHubInstallSelection::Token { token }) } 1 => { @@ -620,6 +621,7 @@ impl InstallInputSource for NonInteractiveInstallInputSource { let token = fabro_github::gh_auth_token() .await .context("Run `gh auth login` and rerun `fabro install`.")?; + fabro_github::validate_static_github_token(&token)?; Ok(GitHubInstallSelection::Token { token }) } Some(InstallGitHubStrategyArg::App) => Ok(GitHubInstallSelection::App { @@ -705,6 +707,7 @@ async fn choose_install_github_selection( let token = fabro_github::gh_auth_token() .await .context("Run `gh auth login` and rerun `fabro install github`.")?; + fabro_github::validate_static_github_token(&token)?; Ok(GitHubInstallSelection::Token { token }) } Some(InstallGitHubStrategyArg::App) => Ok(GitHubInstallSelection::App { diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index 5d6cbf51b..731467d1d 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -17,7 +17,10 @@ pub(crate) fn build_github_credentials( GithubIntegrationStrategy::Token => { let token = lookup_github_token(vault); match token { - Some(t) => Ok(Some(GitHubCredentials::Token(t))), + Some(t) => { + fabro_github::validate_static_github_token(&t)?; + Ok(Some(GitHubCredentials::Pat(t))) + } None => Err(anyhow!( "GITHUB_TOKEN not configured — run fabro install or set GITHUB_TOKEN" )), diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index bdc535b3b..bac1f5fbc 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -1,6 +1,7 @@ use anyhow::{Context as _, anyhow, bail}; use base64::Engine; use base64::engine::general_purpose::STANDARD; +use chrono::{DateTime, Utc}; use fabro_redact::DisplaySafeUrl; use fabro_static::EnvVars; use fabro_types::PullRequestGithubDetail; @@ -136,12 +137,60 @@ impl GitHubAppCredentials { format!("https://github.com/organizations/{owner}/settings/apps/{slug}/installations") }) } + + pub async fn mint_installation_token( + &self, + client: &impl HttpClient, + owner: &str, + repo: &str, + base_url: &str, + permissions: serde_json::Value, + install_url: Option<&str>, + ) -> anyhow::Result { + let jwt = sign_app_jwt(&self.app_id, &self.private_key_pem)?; + let default_install_url = self.installation_url(owner); + let install_url = install_url.or(default_install_url.as_deref()); + mint_installation_token_with_jwt( + client, + &jwt, + owner, + repo, + base_url, + permissions, + install_url, + ) + .await + } +} + +#[derive(Clone, Debug)] +pub struct InstallationToken { + pub token: String, + pub expires_at: DateTime, +} + +impl InstallationToken { + pub fn near_expiry(&self, threshold: std::time::Duration) -> bool { + let threshold = chrono::Duration::from_std(threshold).unwrap_or(chrono::Duration::MAX); + self.expires_at <= Utc::now() + threshold + } + + pub fn valid_token(&self) -> anyhow::Result<&str> { + if self.expires_at <= Utc::now() { + bail!( + "GitHub installation access token expired at {}", + self.expires_at + ); + } + Ok(&self.token) + } } #[derive(Clone, Debug)] pub enum GitHubCredentials { App(GitHubAppCredentials), - Token(String), + Pat(String), + Installation(InstallationToken), } impl GitHubCredentials { @@ -166,24 +215,35 @@ impl GitHubCredentials { ) -> anyhow::Result { match self { Self::App(creds) => { - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; let install_url = creds.installation_url(owner); - create_installation_access_token_with_permissions_and_install_url( - client, - &jwt, - owner, - repo, - base_url, - permissions, - install_url.as_deref(), - ) - .await + creds + .mint_installation_token( + client, + owner, + repo, + base_url, + permissions, + install_url.as_deref(), + ) + .await + .map(|token| token.token) } - Self::Token(token) => Ok(token.clone()), + Self::Pat(token) => Ok(token.clone()), + Self::Installation(token) => token.valid_token().map(str::to_owned), } } } +pub fn validate_static_github_token(token: &str) -> anyhow::Result<()> { + if token.starts_with("ghs_") { + bail!( + "GitHub installation access token (ghs_*) cannot be configured as a static token \ + because it expires quickly; use a PAT or GitHub App credentials instead" + ); + } + Ok(()) +} + pub async fn gh_auth_token() -> anyhow::Result { let output = Command::new("gh") .args(["auth", "token"]) @@ -401,6 +461,20 @@ pub async fn create_installation_access_token_with_permissions_and_install_url( permissions: serde_json::Value, install_url: Option<&str>, ) -> anyhow::Result { + mint_installation_token_with_jwt(client, jwt, owner, repo, base_url, permissions, install_url) + .await + .map(|token| token.token) +} + +async fn mint_installation_token_with_jwt( + client: &impl HttpClient, + jwt: &str, + owner: &str, + repo: &str, + base_url: &str, + permissions: serde_json::Value, + install_url: Option<&str>, +) -> anyhow::Result { #[derive(Deserialize)] struct Installation { id: u64, @@ -408,7 +482,8 @@ pub async fn create_installation_access_token_with_permissions_and_install_url( #[derive(Deserialize)] struct AccessToken { - token: String, + token: String, + expires_at: DateTime, } // Step 1: Find the installation for this repo @@ -506,7 +581,10 @@ pub async fn create_installation_access_token_with_permissions_and_install_url( .json() .context("Failed to parse access token response")?; - Ok(access_token.token) + Ok(InstallationToken { + token: access_token.token, + expires_at: access_token.expires_at, + }) } /// Request a scoped Installation Access Token with `contents: write`. @@ -963,7 +1041,8 @@ pub async fn resolve_clone_credentials( repo: &str, ) -> anyhow::Result<(Option, Option)> { let token = match ctx.creds { - GitHubCredentials::Token(token) => token.clone(), + GitHubCredentials::Pat(token) => token.clone(), + GitHubCredentials::Installation(token) => token.valid_token()?.to_string(), GitHubCredentials::App(_) => { let client = ctx.http_client()?; ctx.creds @@ -1635,6 +1714,50 @@ mod tests { // create_installation_access_token — success // ----------------------------------------------------------------------- + #[tokio::test] + async fn app_credentials_mint_installation_token_preserves_expiry() { + let mock = MockHttpClient::new() + .on( + HttpMethod::Get, + "/repos/owner/repo/installation", + 200, + r#"{"id": 123}"#, + ) + .on( + HttpMethod::Post, + "/app/installations/123/access_tokens", + 201, + r#"{"token": "ghs_xxx", "expires_at": "2026-01-01T12:00:00Z"}"#, + ) + .with_req_body(r#"{"permissions":{"contents":"write"},"repositories":["repo"]}"#); + + let creds = GitHubAppCredentials { + app_id: "test".to_string(), + private_key_pem: test_rsa_key().to_string(), + slug: None, + }; + + let token = creds + .mint_installation_token( + &mock, + "owner", + "repo", + "", + serde_json::json!({ "contents": "write" }), + None, + ) + .await + .unwrap(); + + assert_eq!(token.token, "ghs_xxx"); + assert_eq!( + token.expires_at, + "2026-01-01T12:00:00Z" + .parse::>() + .unwrap() + ); + } + #[tokio::test] async fn create_iat_success() { let mock = MockHttpClient::new() @@ -1649,7 +1772,7 @@ mod tests { HttpMethod::Post, "/app/installations/123/access_tokens", 201, - r#"{"token": "ghs_xxx"}"#, + r#"{"token": "ghs_xxx", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .with_req_header("Authorization", "Bearer test-jwt") .with_req_body(r#"{"permissions":{"contents":"write"},"repositories":["repo"]}"#); @@ -1771,7 +1894,7 @@ mod tests { HttpMethod::Post, "/app/installations/456/access_tokens", 201, - r#"{"token": "ghs_pr_token"}"#, + r#"{"token": "ghs_pr_token", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .with_req_header("Authorization", "Bearer test-jwt") .with_req_body( @@ -1801,7 +1924,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Get, @@ -1840,7 +1963,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Get, @@ -1879,7 +2002,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Get, @@ -1916,7 +2039,7 @@ mod tests { ) .with_req_header("Authorization", "Bearer ghu_test"); - let creds = GitHubCredentials::Token("ghu_test".to_string()); + let creds = GitHubCredentials::Pat("ghu_test".to_string()); let result = branch_exists_with_client( &mock, &GitHubContext::new(&creds, ""), @@ -2047,7 +2170,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Get, @@ -2098,7 +2221,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on(HttpMethod::Get, "/repos/owner/repo/pulls/999", 404, ""); @@ -2141,7 +2264,7 @@ mod tests { ) .with_req_header("Authorization", "Bearer ghu_test"); - let creds = GitHubCredentials::Token("ghu_test".to_string()); + let creds = GitHubCredentials::Pat("ghu_test".to_string()); let detail = get_pull_request_with_client( &mock, &GitHubContext::new(&creds, ""), @@ -2157,7 +2280,7 @@ mod tests { #[tokio::test] async fn resolve_clone_credentials_returns_token_for_token_credentials() { - let creds = GitHubCredentials::Token("ghu_test".to_string()); + let creds = GitHubCredentials::Pat("ghu_test".to_string()); let credentials = resolve_clone_credentials(&GitHubContext::new(&creds, ""), "owner", "repo") @@ -2173,6 +2296,34 @@ mod tests { ); } + #[test] + fn installation_token_valid_token_rejects_expired_tokens() { + let expired = InstallationToken { + token: "ghs_expired".to_string(), + expires_at: chrono::Utc::now() - chrono::Duration::seconds(1), + }; + assert!(expired.valid_token().is_err()); + + let fresh = InstallationToken { + token: "ghs_fresh".to_string(), + expires_at: chrono::Utc::now() + chrono::Duration::minutes(30), + }; + assert_eq!(fresh.valid_token().unwrap(), "ghs_fresh"); + assert!(!fresh.near_expiry(std::time::Duration::from_mins(15))); + } + + #[test] + fn validate_static_github_token_rejects_installation_tokens() { + validate_static_github_token("ghp_personal").unwrap(); + validate_static_github_token("gho_oauth").unwrap(); + validate_static_github_token("ghu_user").unwrap(); + + let err = validate_static_github_token("ghs_installation") + .unwrap_err() + .to_string(); + assert!(err.contains("installation access token"), "got: {err}"); + } + // ----------------------------------------------------------------------- // merge_pull_request // ----------------------------------------------------------------------- @@ -2190,7 +2341,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Put, @@ -2230,7 +2381,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on(HttpMethod::Put, "/repos/owner/repo/pulls/42/merge", 405, ""); @@ -2266,7 +2417,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on(HttpMethod::Put, "/repos/owner/repo/pulls/42/merge", 409, ""); @@ -2306,7 +2457,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on( HttpMethod::Patch, @@ -2339,7 +2490,7 @@ mod tests { HttpMethod::Post, "/app/installations/1/access_tokens", 201, - r#"{"token": "ghs_test"}"#, + r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#, ) .on(HttpMethod::Patch, "/repos/owner/repo/pulls/999", 404, ""); diff --git a/lib/crates/fabro-hooks/src/executor.rs b/lib/crates/fabro-hooks/src/executor.rs index 22dbcdd4b..acb877254 100644 --- a/lib/crates/fabro-hooks/src/executor.rs +++ b/lib/crates/fabro-hooks/src/executor.rs @@ -424,9 +424,9 @@ impl HookExecutorImpl { for tc in &tool_calls { let tool = registry.get(&tc.name).cloned(); let ctx = ToolContext { - env: sandbox.clone(), - cancel: cancel.child_token(), - tool_env: None, + env: sandbox.clone(), + cancel: cancel.child_token(), + tool_env_provider: None, }; let result = match tool { Some(t) => match (t.executor)(tc.arguments.clone(), ctx).await { diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 7a06e3e34..414c87852 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -230,7 +230,23 @@ async fn check_github_app(state: &AppState) -> CheckResult { let settings = state.server_settings(); if settings.server.integrations.github.strategy == GithubIntegrationStrategy::Token { let token = match state.github_credentials(&settings.server.integrations.github) { - Ok(Some(fabro_github::GitHubCredentials::Token(token))) => token, + Ok(Some(fabro_github::GitHubCredentials::Pat(token))) => token, + Ok(Some(fabro_github::GitHubCredentials::Installation(token))) => { + match token.valid_token() { + Ok(token) => token.to_string(), + Err(err) => { + return CheckResult { + name: "GitHub Token".to_string(), + status: CheckStatus::Error, + summary: "token expired".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some( + "Run fabro install or update GITHUB_TOKEN".to_string(), + ), + }; + } + } + } Ok(Some(_)) => unreachable!("token strategy should not return app credentials"), Ok(None) => { return CheckResult { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index cd1b766f3..4c7281d00 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -1234,8 +1234,12 @@ async fn mint_github_token( origin_url: &str, permissions: &HashMap, ) -> Result { - if let fabro_github::GitHubCredentials::Token(token) = creds { - return Ok(token.clone()); + match creds { + fabro_github::GitHubCredentials::Pat(token) => return Ok(token.clone()), + fabro_github::GitHubCredentials::Installation(token) => { + return token.valid_token().map(str::to_owned); + } + fabro_github::GitHubCredentials::App(_) => {} } let https_url = fabro_github::ssh_url_to_https(origin_url); diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index e1da8587c..0021dc80b 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -256,10 +256,21 @@ fn resolve_webhook_preconditions( ))); } }; - let Some(fabro_github::GitHubCredentials::App(github_app)) = github_app else { - return Ok(WebhookPreconditions::Skip( - "GITHUB_APP_PRIVATE_KEY is not available".to_string(), - )); + let github_app = match github_app { + Some(fabro_github::GitHubCredentials::App(github_app)) => github_app, + Some( + fabro_github::GitHubCredentials::Pat(_) + | fabro_github::GitHubCredentials::Installation(_), + ) => { + return Ok(WebhookPreconditions::Skip( + "GitHub webhooks require GitHub App credentials".to_string(), + )); + } + None => { + return Ok(WebhookPreconditions::Skip( + "GITHUB_APP_PRIVATE_KEY is not available".to_string(), + )); + } }; Ok(WebhookPreconditions::Ready { app_id, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 0520c0424..c4405daca 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -798,7 +798,11 @@ impl AppState { .filter(|token| !token.is_empty()) .map(str::to_string); match token { - Some(token) => Ok(Some(fabro_github::GitHubCredentials::Token(token))), + Some(token) => { + fabro_github::validate_static_github_token(&token) + .map_err(|err| err.to_string())?; + Ok(Some(fabro_github::GitHubCredentials::Pat(token))) + } None => Err( "GITHUB_TOKEN not configured — run fabro install or set GITHUB_TOKEN" .to_string(), diff --git a/lib/crates/fabro-server/src/server/handler/system.rs b/lib/crates/fabro-server/src/server/handler/system.rs index 63dd0f28d..55809a6d4 100644 --- a/lib/crates/fabro-server/src/server/handler/system.rs +++ b/lib/crates/fabro-server/src/server/handler/system.rs @@ -366,7 +366,16 @@ async fn get_github_repo( } GithubIntegrationStrategy::Token => { let token = match state.github_credentials(github_settings) { - Ok(Some(fabro_github::GitHubCredentials::Token(token))) => token, + Ok(Some(fabro_github::GitHubCredentials::Pat(token))) => token, + Ok(Some(fabro_github::GitHubCredentials::Installation(token))) => { + match token.valid_token() { + Ok(token) => token.to_string(), + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) + .into_response(); + } + } + } Ok(Some(_)) => unreachable!("token strategy should not return app credentials"), Ok(None) => { return ApiError::new( diff --git a/lib/crates/fabro-tracker/src/github.rs b/lib/crates/fabro-tracker/src/github.rs index b0cd5ef08..c0ced8015 100644 --- a/lib/crates/fabro-tracker/src/github.rs +++ b/lib/crates/fabro-tracker/src/github.rs @@ -78,7 +78,8 @@ impl GitHubTracker { .await .context("creating GitHub App installation token") } - GitHubCredentials::Token(token) => Ok(token.clone()), + GitHubCredentials::Pat(token) => Ok(token.clone()), + GitHubCredentials::Installation(token) => token.valid_token().map(str::to_owned), } } @@ -725,7 +726,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -765,7 +767,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -818,7 +821,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -853,7 +857,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -888,7 +893,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -929,7 +935,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -975,7 +982,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -1026,7 +1034,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -1099,7 +1108,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server @@ -1182,7 +1192,8 @@ mod tests { .mock_async(|when, then| { when.method("POST") .path("/app/installations/1/access_tokens"); - then.status(201).body(r#"{"token": "ghs_test"}"#); + then.status(201) + .body(r#"{"token": "ghs_test", "expires_at": "2099-01-01T00:00:00Z"}"#); }) .await; server diff --git a/lib/crates/fabro-types/src/run_event/infra.rs b/lib/crates/fabro-types/src/run_event/infra.rs index 77689087b..77319920f 100644 --- a/lib/crates/fabro-types/src/run_event/infra.rs +++ b/lib/crates/fabro-types/src/run_event/infra.rs @@ -27,6 +27,7 @@ pub enum RunNoticeCode { GitDiffFailed, GitPushFailed, GithubTokenFailed, + GithubTokenRefreshLimited, ParallelBaseCheckpointFailed, PullRequestFailed, SandboxCleanupFailed, diff --git a/lib/crates/fabro-workflow/src/github_token_source.rs b/lib/crates/fabro-workflow/src/github_token_source.rs new file mode 100644 index 000000000..7a7815f67 --- /dev/null +++ b/lib/crates/fabro-workflow/src/github_token_source.rs @@ -0,0 +1,277 @@ +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 should_refresh = cache + .as_ref() + .is_none_or(|token| token.near_expiry(REFRESH_THRESHOLD)); + + if should_refresh { + match minter.mint().await { + Ok(token) => { + *cache = Some(token); + } + Err(err) => { + if let Some(token) = cache.as_ref() { + if token.valid_token().is_ok() { + warn!( + error = %err, + "GitHub installation token refresh failed; using cached token" + ); + return token.valid_token().map(str::to_owned); + } + } + return Err(err) + .context("failed to mint GitHub installation access token"); + } + } + } + + cache + .as_ref() + .expect("mintable token source should have a token after refresh") + .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/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index dbb8fa933..ea525b30d 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -104,11 +104,11 @@ impl Handler for CommandHandler { ); let timeout_ms = node.timeout().map_or(600_000, crate::millis_u64); - let env_vars = if services.env.is_empty() { - None - } else { - Some(&services.env) - }; + let env = services + .env_for_stage() + .await + .map_err(|err| Error::handler_with_anyhow("Failed to resolve stage env", &err))?; + let env_vars = if env.is_empty() { None } else { Some(&env) }; let cancel_token = services.run.cancel_token().child_token(); let stage_id = stage_scope.stage_id(); let recorder = CommandLogRecorder::create(run_dir, &stage_id).await?; @@ -984,6 +984,21 @@ mod tests { services } + struct RefreshingMinter { + calls: std::sync::atomic::AtomicUsize, + } + + #[async_trait::async_trait] + impl crate::github_token_source::IatMinter 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 { + token: format!("ghs_{call}"), + expires_at: chrono::Utc::now() + chrono::Duration::minutes(10), + }) + } + } + #[tokio::test] async fn executes_script_via_sandbox() { let spy = std::sync::Arc::new(SpySandbox::new(fabro_agent::sandbox::ExecResult { @@ -1087,7 +1102,7 @@ mod tests { let mut services = make_spy_services(spy.clone()); services - .env + .base_env .insert("MY_VAR".to_string(), "my_value".to_string()); handler @@ -1102,6 +1117,61 @@ mod tests { ); } + #[tokio::test] + async fn refreshes_github_token_for_each_command_stage_when_near_expiry() { + let spy = std::sync::Arc::new(SpySandbox::new(fabro_agent::sandbox::ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: Some(0), + termination: CommandTermination::Exited, + duration_ms: 5, + })); + let minter = std::sync::Arc::new(RefreshingMinter { + calls: std::sync::atomic::AtomicUsize::new(0), + }); + let mut services = make_spy_services(spy.clone()); + services.github_token = Some(std::sync::Arc::new( + crate::github_token_source::GitHubTokenSource::mintable(minter.clone()), + )); + + let handler = CommandHandler; + let mut node = Node::new("script_node"); + node.attrs + .insert("script".to_string(), AttrValue::String("true".to_string())); + let context = Context::new(); + let graph = Graph::new("test"); + let run_dir = tempfile::tempdir().unwrap(); + + handler + .execute(&node, &context, &graph, run_dir.path(), &services) + .await + .unwrap(); + assert_eq!( + spy.captured_env_vars + .lock() + .unwrap() + .as_ref() + .and_then(|env| env.get("GITHUB_TOKEN")) + .map(String::as_str), + Some("ghs_1") + ); + + handler + .execute(&node, &context, &graph, run_dir.path(), &services) + .await + .unwrap(); + assert_eq!( + spy.captured_env_vars + .lock() + .unwrap() + .as_ref() + .and_then(|env| env.get("GITHUB_TOKEN")) + .map(String::as_str), + Some("ghs_2") + ); + assert_eq!(minter.calls.load(std::sync::atomic::Ordering::SeqCst), 2); + } + #[tokio::test] async fn passes_run_cancellation_to_sandbox() { let spy = std::sync::Arc::new(SpySandbox::new(fabro_agent::sandbox::ExecResult { diff --git a/lib/crates/fabro-workflow/src/handler/llm/api.rs b/lib/crates/fabro-workflow/src/handler/llm/api.rs index b69913f9f..a65093d85 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/api.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/api.rs @@ -5,7 +5,8 @@ use async_trait::async_trait; use fabro_agent::subagent::{SessionFactory, SubAgentManager}; use fabro_agent::{ AgentEvent, AgentProfile, AnthropicProfile, CompletionCoordinator, GeminiProfile, - OpenAiProfile, Sandbox, Session, SessionControlHandle, SessionOptions, Turn, + OpenAiProfile, Sandbox, Session, SessionControlHandle, SessionOptions, StaticEnvProvider, + ToolEnvProvider, Turn, }; use fabro_auth::{CredentialSource, EnvCredentialSource}; use fabro_graphviz::graph::Node; @@ -250,7 +251,7 @@ pub struct AgentApiBackend { provider: Provider, fallback_chain: Vec, sessions: Mutex>, - env: HashMap, + tool_env: Option>, mcp_servers: Vec, source: Arc, steering_hub: Arc, @@ -270,7 +271,7 @@ impl AgentApiBackend { provider, fallback_chain, sessions: Mutex::new(HashMap::new()), - env: HashMap::new(), + tool_env: None, mcp_servers: Vec::new(), source, steering_hub, @@ -295,7 +296,13 @@ impl AgentApiBackend { #[must_use] pub fn with_env(mut self, env: HashMap) -> Self { - self.env = env; + self.tool_env = Some(Arc::new(StaticEnvProvider(env))); + self + } + + #[must_use] + pub fn with_tool_env_provider(mut self, provider: Arc) -> Self { + self.tool_env = Some(provider); self } @@ -322,7 +329,7 @@ impl AgentApiBackend { node, sandbox, self.source.as_ref(), - &self.env, + self.tool_env.as_ref(), tool_hooks, self.mcp_servers.clone(), ) @@ -335,7 +342,7 @@ impl AgentApiBackend { node: &Node, sandbox: &Arc, source: &dyn CredentialSource, - env: &HashMap, + tool_env: Option<&Arc>, tool_hooks: Option>, mcp_servers: Vec, ) -> Result { @@ -363,7 +370,7 @@ impl AgentApiBackend { let factory_client = client.clone(); let factory_model = model.to_string(); let factory_env = Arc::clone(sandbox); - let factory_tool_env = env.clone(); + let factory_tool_env = tool_env.cloned(); let factory: SessionFactory = Arc::new(move || { let child_profile: Arc = match provider { Provider::OpenAi => Arc::new(OpenAiProfile::new(&factory_model)), @@ -384,8 +391,8 @@ impl AgentApiBackend { SessionOptions::default(), None, ); - if !factory_tool_env.is_empty() { - session.set_tool_env(factory_tool_env.clone()); + if let Some(provider) = &factory_tool_env { + session.set_tool_env_provider(Arc::clone(provider)); } session }); @@ -400,8 +407,8 @@ impl AgentApiBackend { config, Some(manager_for_callback.clone()), ); - if !env.is_empty() { - session.set_tool_env(env.clone()); + if let Some(provider) = tool_env { + session.set_tool_env_provider(Arc::clone(provider)); } // Wire subagent event callback to parent session's emitter @@ -793,7 +800,7 @@ impl CodergenBackend for AgentApiBackend { node, sandbox, self.source.as_ref(), - &self.env, + self.tool_env.as_ref(), tool_hooks.clone(), self.mcp_servers.clone(), ) diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index 58fdb3633..c69f8d99c 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -1,8 +1,13 @@ +//! CLI agent stages resolve workflow tool env once when launching the external +//! CLI process. Long-running CLI stages do not observe later GitHub +//! installation token refreshes until a future credential-helper integration +//! moves token lookup inside the child process. + use std::collections::HashMap; use std::sync::{Arc, Mutex}; use async_trait::async_trait; -use fabro_agent::{Sandbox, shell_quote}; +use fabro_agent::{Sandbox, StaticEnvProvider, ToolEnvProvider, shell_quote}; use fabro_auth::{CliAgentKind, CredentialResolver, CredentialUsage, ResolvedCredential}; use fabro_graphviz::graph::Node; use fabro_llm::types::TokenCounts; @@ -36,7 +41,7 @@ fn cli_failure_detail(stdout: &str, stderr: &str, command: &str) -> String { use super::super::agent::{CodergenBackend, CodergenResult}; use crate::context::Context; use crate::error::Error; -use crate::event::{Emitter, Event, StageScope}; +use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel, StageScope}; use crate::outcome::billed_model_usage_from_llm; /// Maps a provider to its corresponding CLI tool metadata. @@ -395,11 +400,12 @@ pub fn parse_cli_response(provider: Provider, output: &str) -> Option, + model: String, + provider: Provider, + tool_env: Option>, + github_token_refresh_managed: bool, poll_interval: std::time::Duration, - resolver: Option, + resolver: Option, } impl AgentCliBackend { @@ -408,7 +414,8 @@ impl AgentCliBackend { Self { model, provider, - env: HashMap::new(), + tool_env: None, + github_token_refresh_managed: false, poll_interval: std::time::Duration::from_secs(5), resolver: Some(resolver), } @@ -419,7 +426,8 @@ impl AgentCliBackend { Self { model, provider, - env: HashMap::new(), + tool_env: None, + github_token_refresh_managed: false, poll_interval: std::time::Duration::from_secs(5), resolver: None, } @@ -427,7 +435,18 @@ impl AgentCliBackend { #[must_use] pub fn with_env(mut self, env: HashMap) -> Self { - self.env = env; + self.tool_env = Some(Arc::new(StaticEnvProvider(env))); + self + } + + #[must_use] + pub fn with_tool_env_provider( + mut self, + provider: Arc, + github_token_refresh_managed: bool, + ) -> Self { + self.tool_env = Some(provider); + self.github_token_refresh_managed = github_token_refresh_managed; self } @@ -585,8 +604,21 @@ impl CodergenBackend for AgentCliBackend { } env }; - for (name, val) in &self.env { - launch_env.insert(name.clone(), val.clone()); + if let Some(provider) = &self.tool_env { + if self.github_token_refresh_managed { + emitter.notice( + RunNoticeLevel::Info, + RunNoticeCode::GithubTokenRefreshLimited, + "CLI agent stages receive GitHub tokens at process launch; stages running \ + beyond token expiry may need to be retried.", + ); + } + let tool_env = provider.resolve().await.map_err(|err| { + Error::handler_with_anyhow("Failed to resolve CLI agent env", &err) + })?; + for (name, val) in tool_env { + launch_env.insert(name, val); + } } // Write env file so the inner shell that runs the CLI command picks up diff --git a/lib/crates/fabro-workflow/src/handler/manager_loop.rs b/lib/crates/fabro-workflow/src/handler/manager_loop.rs index 8cc366796..6a24e465b 100644 --- a/lib/crates/fabro-workflow/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflow/src/handler/manager_loop.rs @@ -227,7 +227,8 @@ impl Handler for SubWorkflowHandler { let parent_run = Arc::clone(&services.run); let registry = Arc::clone(&services.registry); - let env = services.env.clone(); + let base_env = services.base_env.clone(); + let github_token = services.github_token.clone(); let inputs = services.inputs.clone(); let dry_run = services.dry_run; let workflow_bundle = services.workflow_bundle.clone(); @@ -265,7 +266,8 @@ impl Handler for SubWorkflowHandler { run: child_run, registry, git_state: std::sync::RwLock::new(None), - env, + base_env, + github_token, inputs, dry_run, workflow_path: child_workflow_path, diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index dcf6de747..d7eb9a179 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -303,7 +303,8 @@ impl Handler for ParallelHandler { for setup in branch_setups { let parent_run = Arc::clone(&services.run); let registry = Arc::clone(&services.registry); - let env = services.env.clone(); + let base_env = services.base_env.clone(); + let github_token = services.github_token.clone(); let inputs = services.inputs.clone(); let dry_run = services.dry_run; let workflow_path = services.workflow_path.clone(); @@ -371,7 +372,8 @@ impl Handler for ParallelHandler { run: parent_run.with_sandbox(Arc::clone(&setup.sandbox)), registry: Arc::clone(®istry), git_state: std::sync::RwLock::new(None), - env: env.clone(), + base_env: base_env.clone(), + github_token: github_token.clone(), inputs: inputs.clone(), dry_run, workflow_path, diff --git a/lib/crates/fabro-workflow/src/lib.rs b/lib/crates/fabro-workflow/src/lib.rs index 1ff880ddb..0cdef9f8a 100644 --- a/lib/crates/fabro-workflow/src/lib.rs +++ b/lib/crates/fabro-workflow/src/lib.rs @@ -279,6 +279,7 @@ 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/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index df688aab5..c60628df4 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -27,17 +27,16 @@ use tokio::time::timeout as tokio_timeout; use super::types::{InitOptions, Initialized, LlmSpec, Persisted, SandboxEnvSpec}; use crate::devcontainer_bridge::{devcontainer_to_snapshot_config, run_devcontainer_lifecycle}; use crate::error::Error; -use crate::event::{Emitter, Event, RunNoticeCode, RunNoticeLevel}; +use crate::event::{Event, RunNoticeCode, RunNoticeLevel}; use crate::git::RUN_BRANCH_PREFIX; +use crate::github_token_source::{AppIatMinter, GitHubTokenSource}; use crate::handler::llm::{AgentApiBackend, AgentCliBackend, BackendRouter}; use crate::handler::{HandlerRegistry, default_registry}; -use crate::run_metadata::{ - RunMetadataRuntime, build_metadata_writer, metadata_branch_name, mint_token, -}; +use crate::run_metadata::{RunMetadataRuntime, build_metadata_writer, metadata_branch_name}; use crate::run_options::{GitCheckpointOptions, RunOptions}; use crate::sandbox_git::GIT_REMOTE; use crate::sandbox_git_runtime::SandboxGitRuntime; -use crate::services::{EngineServices, RunServices}; +use crate::services::{EngineServices, RunServices, WorkflowToolEnvProvider}; use crate::steering_hub::SteeringHub; struct WorktreePlan { @@ -47,6 +46,8 @@ struct WorktreePlan { skip_branch_creation: bool, } +type BuiltSandboxEnv = (HashMap, Option>); + async fn resolve_worktree_base_sha( sandbox: &dyn Sandbox, plan: &WorktreePlan, @@ -223,52 +224,62 @@ fn git_setup_intent(run_options: &RunOptions) -> GitSetupIntent { } } -async fn mint_github_token( - creds: &fabro_github::GitHubCredentials, - origin_url: &str, - permissions: &HashMap, -) -> Result { - mint_token(creds, origin_url, permissions) - .await - .map_err(|err| Error::engine_with_anyhow("Failed to mint GitHub token", &err)) -} - -async fn build_sandbox_env( +fn build_sandbox_env( spec: &SandboxEnvSpec, github_app: Option<&fabro_github::GitHubCredentials>, - emitter: &Emitter, -) -> Result, Error> { +) -> Result { let mut env = spec.devcontainer_env.clone(); env.extend(spec.toml_env.clone()); - if let Some(permissions) = spec.github_permissions.as_ref() { - if !permissions.is_empty() { - if let (Some(creds), Some(origin_url)) = (github_app, spec.origin_url.as_deref()) { - match mint_github_token(creds, origin_url, permissions).await { - Ok(token) => { - env.insert("GITHUB_TOKEN".to_string(), token); - } - Err(e) => { - tracing::warn!(error = %e, "Failed to mint GitHub token"); - emitter.notice( - RunNoticeLevel::Warn, - RunNoticeCode::GithubTokenFailed, - format!("Failed to mint GitHub token: {e}"), - ); - } - } - } - } - } + let Some(permissions) = spec.github_permissions.as_ref().filter(|p| !p.is_empty()) else { + return Ok((env, None)); + }; + let Some(creds) = github_app else { + return Ok((env, None)); + }; - Ok(env) + let source = match creds { + fabro_github::GitHubCredentials::Pat(token) => { + Some(Arc::new(GitHubTokenSource::pat(token.clone()))) + } + fabro_github::GitHubCredentials::Installation(token) => { + Some(Arc::new(GitHubTokenSource::static_iat(token.clone()))) + } + fabro_github::GitHubCredentials::App(app) => { + let Some(origin_url) = spec.origin_url.as_deref() else { + return Ok((env, None)); + }; + let https_url = fabro_github::ssh_url_to_https(origin_url); + let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url) + .map_err(|err| Error::engine_with_anyhow("Failed to parse GitHub origin", &err))?; + 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)))) + } + }; + + Ok((env, source)) } async fn build_registry( spec: &LlmSpec, interviewer: Arc, steering_hub: Arc, - sandbox_env: &HashMap, + tool_env_provider: Arc, + github_token_refresh_managed: bool, graph: &graph::Graph, llm_source: Arc, cli_resolver: Option, @@ -306,14 +317,15 @@ async fn build_registry( Ok((build_no_backend(), false)) } Ok(_result) => { - let env = sandbox_env.clone(); let model = spec.model.clone(); let provider = spec.provider; let fallback_chain = spec.fallback_chain.clone(); let mcp_servers = spec.mcp_servers.clone(); let llm_source_for_api = Arc::clone(&llm_source); let steering_hub_for_api = Arc::clone(&steering_hub); + let tool_env_provider_for_backend = Arc::clone(&tool_env_provider); let registry = Arc::new(default_registry(interviewer, move || { + let tool_env_provider = Arc::clone(&tool_env_provider_for_backend); let api = AgentApiBackend::new( model.clone(), provider, @@ -321,7 +333,7 @@ async fn build_registry( Arc::clone(&llm_source_for_api), Arc::clone(&steering_hub_for_api), ) - .with_env(env.clone()) + .with_tool_env_provider(tool_env_provider.clone()) .with_mcp_servers(mcp_servers.clone()); let cli = cli_resolver .clone() @@ -329,7 +341,7 @@ async fn build_registry( || AgentCliBackend::new_from_env(model.clone(), provider), |resolver| AgentCliBackend::new(model.clone(), provider, resolver), ) - .with_env(env.clone()); + .with_tool_env_provider(tool_env_provider, github_token_refresh_managed); Some(Box::new(BackendRouter::new(Box::new(api), cli))) })); Ok((registry, false)) @@ -580,12 +592,17 @@ pub async fn initialize( clone_branch: sandbox_record.clone_branch.clone(), }); - let env = build_sandbox_env( + let (base_env, github_token) = build_sandbox_env( &options.sandbox_env, options.run_options.github_app.as_ref(), - &options.emitter, - ) - .await?; + )?; + let tool_env_provider = Arc::new(WorkflowToolEnvProvider { + base_env: base_env.clone(), + github_token: github_token.clone(), + }); + let github_token_refresh_managed = github_token + .as_deref() + .is_some_and(GitHubTokenSource::is_refreshable); 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) @@ -594,7 +611,8 @@ pub async fn initialize( &options.llm, Arc::clone(&options.interviewer), Arc::clone(&options.steering_hub), - &env, + Arc::clone(&tool_env_provider), + github_token_refresh_managed, &graph, Arc::clone(&llm_source), cli_resolver, @@ -757,7 +775,8 @@ pub async fn initialize( run: Arc::clone(&run_services), registry, git_state: std::sync::RwLock::new(None), - env, + base_env, + github_token, inputs: options.run_options.settings.run.inputs.clone(), dry_run: options.dry_run, workflow_path: options.workflow_path.clone(), @@ -1117,7 +1136,11 @@ mod tests { assert_eq!(initialized.source, source); assert!(initialized.engine.run.hook_runner.is_none()); assert_eq!( - initialized.engine.env.get("TEST_KEY").map(String::as_str), + initialized + .engine + .base_env + .get("TEST_KEY") + .map(String::as_str), Some("value") ); assert!(initialized.engine.dry_run); @@ -1161,6 +1184,10 @@ mod tests { let vault = Arc::new(AsyncRwLock::new(vault)); let test_emitter = Arc::new(crate::event::Emitter::new(test_run_id())); + let tool_env_provider = Arc::new(WorkflowToolEnvProvider { + base_env: HashMap::new(), + github_token: None, + }); let (_registry, effective_dry_run) = build_registry( &LlmSpec { model: "claude-opus-4-6".to_string(), @@ -1171,7 +1198,8 @@ mod tests { }, Arc::new(AutoApproveInterviewer::engine()), Arc::new(crate::steering_hub::SteeringHub::new(test_emitter)), - &HashMap::new(), + tool_env_provider, + false, &graph, Arc::new(VaultCredentialSource::new(Arc::clone(&vault))), Some(CredentialResolver::new(vault)), diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 37aec323e..03edcbc0e 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1947,7 +1947,7 @@ mod tests { }, )); - let creds = fabro_github::GitHubCredentials::Token("test-token".to_string()); + let creds = fabro_github::GitHubCredentials::Pat("test-token".to_string()); let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); diff --git a/lib/crates/fabro-workflow/src/run_metadata.rs b/lib/crates/fabro-workflow/src/run_metadata.rs index de27f743e..c508a892c 100644 --- a/lib/crates/fabro-workflow/src/run_metadata.rs +++ b/lib/crates/fabro-workflow/src/run_metadata.rs @@ -646,8 +646,11 @@ mod tests { run_id: fabro_types::fixtures::RUN_1, labels: HashMap::new(), workflow_slug: Some("metadata".to_string()), - github_app: Some(fabro_github::GitHubCredentials::Token( - "ghs_token".to_string(), + github_app: Some(fabro_github::GitHubCredentials::Installation( + fabro_github::InstallationToken { + token: "ghs_token".to_string(), + expires_at: chrono::Utc::now() + chrono::Duration::hours(1), + }, )), pre_run_git: Some(GitContext { origin_url: origin_url.to_string(), diff --git a/lib/crates/fabro-workflow/src/services.rs b/lib/crates/fabro-workflow/src/services.rs index 5ad0b983b..a878758aa 100644 --- a/lib/crates/fabro-workflow/src/services.rs +++ b/lib/crates/fabro-workflow/src/services.rs @@ -5,7 +5,7 @@ use std::sync::Arc; #[cfg(test)] use std::time::Duration; -use fabro_agent::Sandbox; +use fabro_agent::{Sandbox, ToolEnvProvider}; use fabro_auth::CredentialSource; #[cfg(test)] use fabro_auth::ResolvedCredentials; @@ -15,6 +15,7 @@ use tokio_util::sync::CancellationToken; use crate::ManifestPath; use crate::event::Emitter; +use crate::github_token_source::GitHubTokenSource; use crate::handler::HandlerRegistry; use crate::run_metadata::{RunMetadataRuntime, RunMetadataWriterHandle}; use crate::runtime_store::RunStoreHandle; @@ -135,9 +136,10 @@ pub struct EngineServices { /// Git state for the current run. Set via `set_git_state` at the start of /// `execute` and read by parallel/fan-in handlers. pub(crate) git_state: std::sync::RwLock>>, - /// Environment variables from `[sandbox.env]` config, injected into command - /// nodes. - pub env: HashMap, + /// Environment variables from devcontainer and `[sandbox.env]` config. + pub base_env: HashMap, + /// GitHub token source used to inject `GITHUB_TOKEN` at the point of use. + 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 @@ -150,6 +152,10 @@ pub struct EngineServices { } impl EngineServices { + pub async fn env_for_stage(&self) -> anyhow::Result> { + resolve_workflow_env(&self.base_env, self.github_token.as_ref()).await + } + /// Read the current git state (if any). pub fn git_state(&self) -> Option> { self.git_state.read().unwrap().clone() @@ -227,7 +233,8 @@ impl EngineServices { ), registry: Arc::new(HandlerRegistry::new(Box::new(start::StartHandler))), git_state: std::sync::RwLock::new(None), - env: HashMap::new(), + base_env: HashMap::new(), + github_token: None, inputs: HashMap::new(), dry_run: false, workflow_path: None, @@ -236,9 +243,40 @@ impl EngineServices { } } +pub struct WorkflowToolEnvProvider { + pub base_env: HashMap, + pub github_token: Option>, +} + +#[async_trait::async_trait] +impl ToolEnvProvider for WorkflowToolEnvProvider { + async fn resolve(&self) -> anyhow::Result> { + resolve_workflow_env(&self.base_env, self.github_token.as_ref()).await + } +} + +async fn resolve_workflow_env( + base_env: &HashMap, + 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?); + } + Ok(env) +} + #[cfg(test)] mod tests { - use super::EngineServices; + use std::collections::HashMap; + use std::sync::Arc; + + use anyhow::anyhow; + use fabro_agent::ToolEnvProvider as _; + use fabro_github::InstallationToken; + + use super::{EngineServices, WorkflowToolEnvProvider}; + use crate::github_token_source::{GitHubTokenSource, IatMinter}; #[tokio::test] async fn test_default_uses_stub_credential_source() { @@ -253,4 +291,52 @@ mod tests { .is_empty() ); } + + #[tokio::test] + async fn workflow_tool_env_provider_returns_base_env_without_github_token() { + let provider = WorkflowToolEnvProvider { + base_env: HashMap::from([("FOO".to_string(), "bar".to_string())]), + github_token: None, + }; + + let env = provider.resolve().await.unwrap(); + + assert_eq!(env.get("FOO").map(String::as_str), Some("bar")); + assert!(!env.contains_key("GITHUB_TOKEN")); + } + + #[tokio::test] + 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()))), + }; + + let env = provider.resolve().await.unwrap(); + + assert_eq!(env.get("FOO").map(String::as_str), Some("bar")); + assert_eq!(env.get("GITHUB_TOKEN").map(String::as_str), Some("ghp_pat")); + } + + struct FailingMinter; + + #[async_trait::async_trait] + impl IatMinter for FailingMinter { + async fn mint(&self) -> anyhow::Result { + Err(anyhow!("GITHUB_TOKEN refresh failed")) + } + } + + #[tokio::test] + 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, + )))), + }; + + let err = format!("{:#}", provider.resolve().await.unwrap_err()); + assert!(err.contains("GITHUB_TOKEN refresh failed"), "got: {err}"); + } } diff --git a/lib/crates/fabro-workflow/src/test_support.rs b/lib/crates/fabro-workflow/src/test_support.rs index 0bf6377a6..c3e1fe517 100644 --- a/lib/crates/fabro-workflow/src/test_support.rs +++ b/lib/crates/fabro-workflow/src/test_support.rs @@ -166,7 +166,8 @@ async fn initialized( ), registry: Arc::new(registry), git_state: std::sync::RwLock::new(None), - env: options.env, + base_env: options.env, + github_token: None, inputs: run_options.settings.run.inputs.clone(), dry_run: run_options.dry_run_enabled(), workflow_path: None,