diff --git a/lib/crates/fabro-cli/src/commands/pr/mod.rs b/lib/crates/fabro-cli/src/commands/pr/mod.rs index bceb19684..5dd28b494 100644 --- a/lib/crates/fabro-cli/src/commands/pr/mod.rs +++ b/lib/crates/fabro-cli/src/commands/pr/mod.rs @@ -18,7 +18,7 @@ use crate::user_config::load_user_settings_with_globals; pub(crate) async fn dispatch(ns: PrNamespace, globals: &GlobalArgs) -> Result<()> { let cli_settings = load_user_settings_with_globals(globals)?; - let github_app = build_github_app_credentials(cli_settings.app_id()); + let github_app = build_github_app_credentials(cli_settings.app_id())?; match ns.command { PrCommand::Create(args) => create::create_command(args, github_app, globals).await, diff --git a/lib/crates/fabro-cli/src/commands/preflight.rs b/lib/crates/fabro-cli/src/commands/preflight.rs index 8b980e8c3..c1b89a532 100644 --- a/lib/crates/fabro-cli/src/commands/preflight.rs +++ b/lib/crates/fabro-cli/src/commands/preflight.rs @@ -26,7 +26,7 @@ pub(crate) async fn execute(mut args: PreflightArgs, globals: &GlobalArgs) -> an let cli_settings: FabroSettings = load_user_settings_with_globals(globals)?; args.verbose = args.verbose || cli_settings.verbose_enabled(); - let github_app = build_github_app_credentials(cli_settings.app_id()); + let github_app = build_github_app_credentials(cli_settings.app_id())?; let cli_args_config = ConfigLayer::try_from(&args)?; let cwd = std::env::current_dir()?; let settings = cli_args_config diff --git a/lib/crates/fabro-cli/src/commands/repo/init.rs b/lib/crates/fabro-cli/src/commands/repo/init.rs index a24aca886..d1b39b7b3 100644 --- a/lib/crates/fabro-cli/src/commands/repo/init.rs +++ b/lib/crates/fabro-cli/src/commands/repo/init.rs @@ -173,15 +173,22 @@ async fn check_github_app_installation() { let slug = cli_settings.slug().map(String::from); // Build GitHub App credentials - let Some(creds) = build_github_app_credentials(Some(&app_id)) else { - eprintln!( - "\n Set {} to enable GitHub App integration", - console::Style::new() - .cyan() - .bold() - .apply_to("GITHUB_APP_PRIVATE_KEY") - ); - return; + let creds = match build_github_app_credentials(Some(&app_id)) { + Ok(Some(creds)) => creds, + Ok(None) => { + eprintln!( + "\n Set {} to enable GitHub App integration", + console::Style::new() + .cyan() + .bold() + .apply_to("GITHUB_APP_PRIVATE_KEY") + ); + return; + } + Err(err) => { + eprintln!("\n Warning: invalid GITHUB_APP_PRIVATE_KEY: {err}"); + return; + } }; let jwt = match fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) { diff --git a/lib/crates/fabro-cli/src/commands/run/detached.rs b/lib/crates/fabro-cli/src/commands/run/detached.rs index 8af85814c..902e52ace 100644 --- a/lib/crates/fabro-cli/src/commands/run/detached.rs +++ b/lib/crates/fabro-cli/src/commands/run/detached.rs @@ -31,7 +31,7 @@ pub(crate) async fn execute(run_dir: PathBuf, launcher_path: PathBuf, resume: bo let store = store::build_store(&run_record.settings.storage_dir())?; let run_store = open_or_hydrate_run(store.as_ref(), &run_dir).await?; - let github_app = shared::github::build_github_app_credentials(run_record.settings.app_id()); + let github_app = shared::github::build_github_app_credentials(run_record.settings.app_id())?; let runtime_state = RuntimeState::new(&run_dir); let services = StartServices { diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index d7ca8fb39..894436a47 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -1,5 +1,8 @@ +use anyhow::anyhow; use fabro_github::GitHubAppCredentials; -pub(crate) fn build_github_app_credentials(app_id: Option<&str>) -> Option { - GitHubAppCredentials::from_env(app_id) +pub(crate) fn build_github_app_credentials( + app_id: Option<&str>, +) -> anyhow::Result> { + GitHubAppCredentials::from_env(app_id).map_err(|err| anyhow!(err)) } diff --git a/lib/crates/fabro-cli/tests/it/cli.rs b/lib/crates/fabro-cli/tests/it/cli.rs index 7f6fa8130..6208e18d4 100644 --- a/lib/crates/fabro-cli/tests/it/cli.rs +++ b/lib/crates/fabro-cli/tests/it/cli.rs @@ -1587,6 +1587,75 @@ digraph Test { ); } +#[test] +fn bug5_detached_uses_snapshotted_app_id_for_github_credentials() { + let storage_dir = tempfile::tempdir().unwrap(); + let home = init_cli_home(storage_dir.path()); + let run_dir = storage_dir.path().join("runs").join("20260101-test-bug5"); + std::fs::create_dir_all(&run_dir).unwrap(); + + let dot = "\ +digraph G { + start [shape=Mdiamond, label=\"Start\"] + exit [shape=Msquare, label=\"Exit\"] + start -> exit +}"; + + let run_record = serde_json::json!({ + "run_id": "test-bug5", + "created_at": "2026-01-01T00:00:00Z", + "settings": { + "dry_run": true, + "auto_approve": true, + "no_retro": true, + "llm": { + "model": "test-model" + }, + "sandbox": { + "provider": "local" + }, + "git": { + "app_id": "snapshotted-app-id" + } + }, + "graph": { + "name": "G", + "nodes": {}, + "edges": [], + "attrs": {} + }, + "working_directory": run_dir.to_str().unwrap(), + }); + std::fs::write( + run_dir.join("run.json"), + serde_json::to_string(&run_record).unwrap(), + ) + .unwrap(); + std::fs::write(run_dir.join("graph.fabro"), dot).unwrap(); + + arc() + .env("HOME", home.path()) + .env("GITHUB_APP_PRIVATE_KEY", "%%%not-base64%%%") + .args([ + "__detached", + "--run-dir", + run_dir.to_str().unwrap(), + "--launcher-path", + storage_dir + .path() + .join("launchers") + .join("test-bug5.json") + .to_str() + .unwrap(), + ]) + .timeout(std::time::Duration::from_secs(10)) + .assert() + .failure() + .stderr(predicate::str::contains( + "GITHUB_APP_PRIVATE_KEY is not valid PEM or base64", + )); +} + // Bug 3: attach loop must leave interview_request.json in place until the // engine consumes interview_response.json, so reattach remains safe. #[test] diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 49cb6ac35..bd2672ff3 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -54,23 +54,37 @@ pub struct GitHubAppCredentials { } impl GitHubAppCredentials { - pub fn from_env(app_id: Option<&str>) -> Option { - let app_id = app_id?; - let raw = std::env::var("GITHUB_APP_PRIVATE_KEY").ok()?; - let private_key_pem = if raw.starts_with("-----") { - raw - } else { - let pem_bytes = - base64::Engine::decode(&base64::engine::general_purpose::STANDARD, &raw).ok()?; - String::from_utf8(pem_bytes).ok()? + pub fn private_key_from_env() -> Result, String> { + let Ok(raw) = std::env::var("GITHUB_APP_PRIVATE_KEY") else { + return Ok(None); }; - Some(Self { + decode_pem_env("GITHUB_APP_PRIVATE_KEY", &raw).map(Some) + } + + pub fn from_env(app_id: Option<&str>) -> Result, String> { + let Some(app_id) = app_id else { + return Ok(None); + }; + let Some(private_key_pem) = Self::private_key_from_env()? else { + return Ok(None); + }; + Ok(Some(Self { app_id: app_id.to_string(), private_key_pem, - }) + })) } } +fn decode_pem_env(name: &str, raw: &str) -> Result { + if raw.starts_with("-----") { + return Ok(raw.to_string()); + } + let pem_bytes = base64::Engine::decode(&base64::engine::general_purpose::STANDARD, raw) + .map_err(|err| format!("{name} is not valid PEM or base64: {err}"))?; + String::from_utf8(pem_bytes) + .map_err(|err| format!("{name} base64 decoded to invalid UTF-8: {err}")) +} + /// HTTP method used in GitHub API calls. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum HttpMethod { @@ -960,6 +974,28 @@ pub async fn create_installation_access_token_for_projects( mod tests { use super::*; + #[test] + fn decode_pem_env_accepts_raw_pem() { + let pem = "-----BEGIN TEST KEY-----\nabc\n-----END TEST KEY-----"; + assert_eq!(decode_pem_env("GITHUB_APP_PRIVATE_KEY", pem).unwrap(), pem); + } + + #[test] + fn decode_pem_env_accepts_base64_pem() { + let pem = "-----BEGIN TEST KEY-----\nabc\n-----END TEST KEY-----"; + let encoded = base64::Engine::encode(&base64::engine::general_purpose::STANDARD, pem); + assert_eq!( + decode_pem_env("GITHUB_APP_PRIVATE_KEY", &encoded).unwrap(), + pem + ); + } + + #[test] + fn decode_pem_env_rejects_invalid_base64() { + let err = decode_pem_env("GITHUB_APP_PRIVATE_KEY", "%%%not-base64%%%").unwrap_err(); + assert!(err.contains("GITHUB_APP_PRIVATE_KEY is not valid PEM or base64")); + } + // ----------------------------------------------------------------------- // parse_github_owner_repo // ----------------------------------------------------------------------- diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 9acaa3b5e..8901234d3 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -14,7 +14,7 @@ use clap::Args; use fabro_config::FabroSettings; use crate::github_webhooks::WebhookManager; -use crate::jwt_auth::{AuthMode, AuthStrategy, decode_pem_env, resolve_auth_mode}; +use crate::jwt_auth::{AuthMode, AuthStrategy, resolve_auth_mode}; use crate::server::{build_router, create_app_state_with_store, spawn_scheduler}; use crate::tls::{ClientAuth, build_rustls_config, serve_tls}; use fabro_llm::client::Client as LlmClient; @@ -182,9 +182,24 @@ pub async fn serve_command( let webhook_manager = match webhook_app_id { Some(app_id) => { let secret = std::env::var("GITHUB_APP_WEBHOOK_SECRET").ok(); - let private_key_pem = read_github_private_key(); - if let (Some(secret), Some(pem)) = (secret, private_key_pem) { - match WebhookManager::start(secret.into_bytes(), &app_id, &pem).await { + let github_app = match fabro_github::GitHubAppCredentials::from_env(Some(&app_id)) { + Ok(github_app) => github_app, + Err(err) => { + warn!( + error = %err, + "Webhook config present but GITHUB_APP_PRIVATE_KEY is invalid; skipping webhook listener" + ); + None + } + }; + if let (Some(secret), Some(github_app)) = (secret, github_app) { + match WebhookManager::start( + secret.into_bytes(), + &github_app.app_id, + &github_app.private_key_pem, + ) + .await + { Ok(manager) => Some(manager), Err(err) => { error!(error = %err, "Failed to start webhook listener"); @@ -268,12 +283,6 @@ pub async fn serve_command( Ok(()) } -/// Read the GitHub App private key from the environment, decoding base64 if needed. -fn read_github_private_key() -> Option { - let raw = std::env::var("GITHUB_APP_PRIVATE_KEY").ok()?; - Some(decode_pem_env("GITHUB_APP_PRIVATE_KEY", &raw)) -} - /// Derive client certificate verification mode from the resolved auth strategies. fn client_auth_from_mode(auth_mode: &AuthMode) -> ClientAuth { let strategies = match auth_mode { diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 849f7c7fd..076520c29 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -490,6 +490,13 @@ fn compute_queue_positions(runs: &HashMap) -> HashMap>, @@ -625,6 +632,7 @@ async fn execute_run(state: Arc, run_id: String) { if let Some(managed_run) = runs.get_mut(&run_id) { if managed_run.status != RunStatus::Starting { // Was cancelled during setup + clear_live_run_state(managed_run); state.scheduler_notify.notify_one(); return; } @@ -642,7 +650,7 @@ async fn execute_run(state: Arc, run_id: String) { if let Some(managed_run) = runs.get_mut(&run_id) { managed_run.status = RunStatus::Failed; managed_run.error = Some(format!("Failed to open or hydrate run store: {e}")); - managed_run.event_tx = None; + clear_live_run_state(managed_run); } state.scheduler_notify.notify_one(); return; @@ -656,14 +664,28 @@ async fn execute_run(state: Arc, run_id: String) { if let Some(managed_run) = runs.get_mut(&run_id) { managed_run.status = RunStatus::Failed; managed_run.error = Some(format!("Failed to load persisted run: {e}")); - managed_run.event_tx = None; + clear_live_run_state(managed_run); + } + state.scheduler_notify.notify_one(); + return; + } + }; + let github_app = match fabro_github::GitHubAppCredentials::from_env( + persisted.run_record().settings.app_id(), + ) { + Ok(github_app) => github_app, + Err(e) => { + tracing::error!(run_id = %run_id, error = %e, "Invalid GitHub App credentials"); + let mut runs = state.runs.lock().expect("runs lock poisoned"); + if let Some(managed_run) = runs.get_mut(&run_id) { + managed_run.status = RunStatus::Failed; + managed_run.error = Some(format!("Invalid GitHub App credentials: {e}")); + clear_live_run_state(managed_run); } state.scheduler_notify.notify_one(); return; } }; - let github_app = - fabro_github::GitHubAppCredentials::from_env(persisted.run_record().settings.app_id()); let services = operations::StartServices { cancel_token: Some(Arc::clone(&cancel_token)), emitter: Arc::clone(&emitter), @@ -757,9 +779,7 @@ async fn execute_run(state: Arc, run_id: String) { } } managed_run.run_dir = Some(run_dir); - managed_run.event_tx = None; - managed_run.cancel_tx = None; - managed_run.cancel_token = None; + clear_live_run_state(managed_run); } drop(runs); state.scheduler_notify.notify_one(); @@ -2459,6 +2479,49 @@ mod tests { ); } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn cancel_before_run_transitions_to_running_closes_event_stream() { + let state = create_app_state_with_registry_factory(test_db().await, |interviewer| { + std::thread::sleep(std::time::Duration::from_millis(200)); + fabro_workflows::handler::default_registry(interviewer, || None) + }); + let app = build_router(Arc::clone(&state), AuthMode::Disabled); + + let req = Request::builder() + .method("POST") + .uri("/runs") + .header("content-type", "application/json") + .body(Body::from( + serde_json::to_string(&serde_json::json!({"dot_source": MINIMAL_DOT})).unwrap(), + )) + .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::CREATED); + let body = body_json(response.into_body()).await; + let run_id = body["id"].as_str().unwrap().to_string(); + + let runner = tokio::spawn(execute_run(Arc::clone(&state), run_id.clone())); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + + let req = Request::builder() + .method("POST") + .uri(format!("/runs/{run_id}/cancel")) + .body(Body::empty()) + .unwrap(); + let response = app.clone().oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::OK); + + runner.await.unwrap(); + + let req = Request::builder() + .method("GET") + .uri(format!("/runs/{run_id}/events")) + .body(Body::empty()) + .unwrap(); + let response = app.oneshot(req).await.unwrap(); + assert_eq!(response.status(), StatusCode::GONE); + } + #[tokio::test] async fn queue_position_reported_for_queued_runs() { let state = create_app_state(test_db().await);