From 6cb185b858432aa263f265248f70027aacecf9b1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Apr 2026 10:43:00 -0400 Subject: [PATCH] feat(github): point install errors at the configured app and require creds for docker GitHubAppCredentials now carries the configured app slug, so the "not installed" error from the installation lookup links to the specific app's install page (https://github.com/organizations/{owner}/settings/apps/{slug}/installations) when known, instead of the generic org installations page. Threaded through the server, workflow pipeline, and CLI runner. Also treat docker like daytona for GitHub credential gating: both are clone-based providers that need an installation token to fetch the repo, so a docker run now requires credentials when daytona would. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../fabro-cli/src/commands/run/runner.rs | 30 +++++- lib/crates/fabro-cli/src/shared/github.rs | 3 +- lib/crates/fabro-github/src/lib.rs | 101 +++++++++++++++++- lib/crates/fabro-github/tests/integration.rs | 1 + lib/crates/fabro-server/src/run_manifest.rs | 4 +- lib/crates/fabro-server/src/server.rs | 17 ++- lib/crates/fabro-tracker/src/github.rs | 1 + .../fabro-workflow/src/pipeline/initialize.rs | 4 +- .../src/pipeline/pull_request.rs | 1 + .../tests/it/daytona_integration.rs | 1 + 10 files changed, 149 insertions(+), 14 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 3596dfa24..24c423f12 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -501,7 +501,7 @@ fn maybe_build_github_credentials( let resolved_run = &settings.run; let resolved_server = ServerSettingsBuilder::load_default().ok(); let required_github_credentials = (resolved_run.execution.mode != RunMode::DryRun - && resolved_run.sandbox.provider == "daytona") + && clone_sandbox_requires_github_credentials(&resolved_run.sandbox.provider)) || resolved_server .as_ref() .is_some_and(|settings| !settings.server.integrations.github.permissions.is_empty()); @@ -515,20 +515,33 @@ fn maybe_build_github_credentials( .as_ref() .and_then(|settings| settings.server.integrations.github.app_id.as_ref()) .map(InterpString::as_source); + let app_slug = resolved_server + .as_ref() + .and_then(|settings| settings.server.integrations.github.slug.as_ref()) + .map(InterpString::as_source); if required_github_credentials { - return build_github_credentials(strategy, app_id.as_deref(), vault); + return build_github_credentials(strategy, app_id.as_deref(), app_slug.as_deref(), vault); } if pull_request_enabled { - return Ok(build_github_credentials(strategy, app_id.as_deref(), vault) - .ok() - .flatten()); + return Ok(build_github_credentials( + strategy, + app_id.as_deref(), + app_slug.as_deref(), + vault, + ) + .ok() + .flatten()); } Ok(None) } +fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { + matches!(provider, "docker" | "daytona") +} + fn install_signal_handlers( run_control: Arc, cancel_token: Arc, @@ -599,6 +612,13 @@ mod tests { }; use crate::args::RunWorkerMode; + #[test] + fn clone_sandbox_credentials_are_required_for_clone_based_providers() { + assert!(super::clone_sandbox_requires_github_credentials("docker")); + assert!(super::clone_sandbox_requires_github_credentials("daytona")); + assert!(!super::clone_sandbox_requires_github_credentials("local")); + } + fn running_event(actor: Option) -> fabro_types::RunEvent { fabro_types::RunEvent { id: "evt_1".to_string(), diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index 7eea66df3..5d6cbf51b 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -7,11 +7,12 @@ use fabro_vault::Vault; pub(crate) fn build_github_credentials( strategy: GithubIntegrationStrategy, app_id: Option<&str>, + app_slug: Option<&str>, vault: Option<&Vault>, ) -> anyhow::Result> { match strategy { GithubIntegrationStrategy::App => { - GitHubCredentials::from_env(app_id).map_err(|err| anyhow!(err)) + GitHubCredentials::from_env_with_slug(app_id, app_slug).map_err(|err| anyhow!(err)) } GithubIntegrationStrategy::Token => { let token = lookup_github_token(vault); diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 2c339e7db..3f82f1009 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -97,6 +97,7 @@ pub struct AppInfo { pub struct GitHubAppCredentials { pub app_id: String, pub private_key_pem: String, + pub slug: Option, } impl GitHubAppCredentials { @@ -112,6 +113,13 @@ impl GitHubAppCredentials { } pub fn from_env(app_id: Option<&str>) -> Result, String> { + Self::from_env_with_slug(app_id, None) + } + + pub fn from_env_with_slug( + app_id: Option<&str>, + slug: Option<&str>, + ) -> Result, String> { let Some(app_id) = app_id else { return Ok(None); }; @@ -121,8 +129,18 @@ impl GitHubAppCredentials { Ok(Some(Self { app_id: app_id.to_string(), private_key_pem, + slug: slug + .map(str::trim) + .filter(|slug| !slug.is_empty()) + .map(str::to_string), })) } + + pub fn installation_url(&self, owner: &str) -> Option { + self.slug.as_ref().map(|slug| { + format!("https://github.com/organizations/{owner}/settings/apps/{slug}/installations") + }) + } } #[derive(Clone, Debug)] @@ -136,6 +154,13 @@ impl GitHubCredentials { Ok(GitHubAppCredentials::from_env(app_id)?.map(Self::App)) } + pub fn from_env_with_slug( + app_id: Option<&str>, + slug: Option<&str>, + ) -> Result, String> { + Ok(GitHubAppCredentials::from_env_with_slug(app_id, slug)?.map(Self::App)) + } + async fn resolve_bearer_token( &self, client: &impl HttpClient, @@ -147,13 +172,15 @@ impl GitHubCredentials { match self { Self::App(creds) => { let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - create_installation_access_token_with_permissions( + 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 } @@ -357,6 +384,27 @@ pub async fn create_installation_access_token_with_permissions( repo: &str, base_url: &str, permissions: serde_json::Value, +) -> Result { + create_installation_access_token_with_permissions_and_install_url( + client, + jwt, + owner, + repo, + base_url, + permissions, + None, + ) + .await +} + +pub async fn create_installation_access_token_with_permissions_and_install_url( + client: &impl HttpClient, + jwt: &str, + owner: &str, + repo: &str, + base_url: &str, + permissions: serde_json::Value, + install_url: Option<&str>, ) -> Result { #[derive(Deserialize)] struct Installation { @@ -369,19 +417,27 @@ pub async fn create_installation_access_token_with_permissions( } // Step 1: Find the installation for this repo - let install_url = format!("{base_url}/repos/{owner}/{repo}/installation"); + let installation_endpoint = format!("{base_url}/repos/{owner}/{repo}/installation"); let auth = format!("Bearer {jwt}"); let resp = client - .request(HttpMethod::Get, &install_url, &github_headers(&auth), None) + .request( + HttpMethod::Get, + &installation_endpoint, + &github_headers(&auth), + None, + ) .await .map_err(|e| format!("Failed to look up GitHub App installation: {e}"))?; match resp.status { 200 => {} 404 => { + let install_url = install_url.map(str::to_string).unwrap_or_else(|| { + format!("https://github.com/organizations/{owner}/settings/installations") + }); return Err(format!( "GitHub App is not installed for {owner}. \ - Install it at https://github.com/organizations/{owner}/settings/installations" + Install it at {install_url}" )); } 403 => { @@ -1600,6 +1656,33 @@ mod tests { assert!(err.contains("owner"), "got: {err}"); } + #[tokio::test] + async fn create_iat_not_installed_uses_app_specific_install_url() { + let mock = + MockHttpClient::new().on(HttpMethod::Get, "/repos/owner/repo/installation", 404, ""); + let install_url = + "https://github.com/organizations/owner/settings/apps/fabro-test/installations"; + + let err = create_installation_access_token_with_permissions_and_install_url( + &mock, + "jwt", + "owner", + "repo", + "", + serde_json::json!({ "contents": "write" }), + Some(install_url), + ) + .await + .unwrap_err(); + + assert!(err.contains("not installed"), "got: {err}"); + assert!(err.contains(install_url), "got: {err}"); + assert!( + !err.contains("https://github.com/organizations/owner/settings/installations"), + "got: {err}" + ); + } + #[tokio::test] async fn create_iat_suspended() { let mock = @@ -1706,6 +1789,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1744,6 +1828,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1782,6 +1867,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let result = branch_exists_with_client( &mock, @@ -1943,6 +2029,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let detail = get_pull_request_with_client( &mock, @@ -1988,6 +2075,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = get_pull_request_with_client( &mock, @@ -2084,6 +2172,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); merge_pull_request_with_client( &mock, @@ -2118,6 +2207,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = merge_pull_request_with_client( &mock, @@ -2153,6 +2243,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = merge_pull_request_with_client( &mock, @@ -2197,6 +2288,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); close_pull_request_with_client(&mock, &GitHubContext::new(&creds, ""), "owner", "repo", 42) .await @@ -2224,6 +2316,7 @@ mod tests { let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), + slug: None, }); let err = close_pull_request_with_client( &mock, diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index 53a3eae2c..de46ea983 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -12,6 +12,7 @@ fn github_credentials() -> GitHubCredentials { GitHubCredentials::App(GitHubAppCredentials { app_id: "42".to_string(), private_key_pem: TEST_RSA_KEY.to_string(), + slug: Some("test-app".to_string()), }) } diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index d18bf7ab1..3668c9a40 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -969,13 +969,15 @@ async fn mint_github_token( .map_err(|err| anyhow!("{err}"))?; let client = fabro_http::http_client()?; let perms_json = serde_json::to_value(permissions)?; - fabro_github::create_installation_access_token_with_permissions( + let install_url = creds.installation_url(&owner); + fabro_github::create_installation_access_token_with_permissions_and_install_url( &client, &jwt, &owner, &repo, &fabro_github::github_api_base_url(), perms_json, + install_url.as_deref(), ) .await .map_err(|err| anyhow!("{err}")) diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 4c8eb55a3..2ccae6603 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -801,6 +801,7 @@ impl AppState { fabro_github::GitHubAppCredentials { app_id, private_key_pem, + slug: settings.slug.as_ref().map(InterpString::as_source), }, ))) } @@ -1743,6 +1744,10 @@ fn system_sandbox_provider( ) } +fn clone_sandbox_requires_github_credentials(provider: &str) -> bool { + matches!(provider, "docker" | "daytona") +} + fn parse_system_duration(raw: &str) -> anyhow::Result { let raw = raw.trim(); anyhow::ensure!(!raw.is_empty(), "empty duration string"); @@ -2033,13 +2038,14 @@ async fn get_github_repo( .into_response(); } - match fabro_github::create_installation_access_token_with_permissions( + match fabro_github::create_installation_access_token_with_permissions_and_install_url( client_ref, &jwt, &owner, &name, &base_url, serde_json::json!({ "contents": "write", "pull_requests": "write" }), + Some(&install_url), ) .await { @@ -4632,7 +4638,7 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { let github_app_result = { let settings = &persisted.run_spec().settings.run; let required_github_credentials = (settings.execution.mode != RunMode::DryRun - && settings.sandbox.provider == "daytona") + && clone_sandbox_requires_github_credentials(&settings.sandbox.provider)) || !github_settings.permissions.is_empty(); if required_github_credentials { state.github_credentials(github_settings) @@ -8667,6 +8673,13 @@ provider = "invalid-provider" ); } + #[test] + fn clone_sandbox_credentials_are_required_for_clone_based_providers() { + assert!(clone_sandbox_requires_github_credentials("docker")); + assert!(clone_sandbox_requires_github_credentials("daytona")); + assert!(!clone_sandbox_requires_github_credentials("local")); + } + #[tokio::test] async fn create_secret_stores_file_secret_and_excludes_it_from_snapshot() { let state = create_app_state(); diff --git a/lib/crates/fabro-tracker/src/github.rs b/lib/crates/fabro-tracker/src/github.rs index 10aa4de9e..92485ed73 100644 --- a/lib/crates/fabro-tracker/src/github.rs +++ b/lib/crates/fabro-tracker/src/github.rs @@ -634,6 +634,7 @@ mod tests { GitHubCredentials::App(GitHubAppCredentials { app_id: "test-app".to_string(), private_key_pem: pem, + slug: None, }), test_http_client(), "owner".to_string(), diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index b1f3b3f27..69bead9e8 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -229,13 +229,15 @@ async fn mint_github_token( .map_err(|e| Error::engine(e.clone()))?; let client = fabro_http::http_client().map_err(|e| Error::engine(e.to_string()))?; let perms_json = serde_json::to_value(permissions).map_err(|e| Error::engine(e.to_string()))?; - fabro_github::create_installation_access_token_with_permissions( + let install_url = creds.installation_url(&owner); + fabro_github::create_installation_access_token_with_permissions_and_install_url( &client, &jwt, &owner, &repo, &fabro_github::github_api_base_url(), perms_json, + install_url.as_deref(), ) .await .map_err(|e| Error::engine(e.clone())) diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 48572a331..81f308d7b 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1510,6 +1510,7 @@ mod tests { let creds = fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id: "123".to_string(), private_key_pem: "unused".to_string(), + slug: None, }); let base_url = github_app::github_api_base_url(); let result = maybe_open_pull_request(OpenPullRequestRequest { diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 5c4874406..5b33f0789 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -207,6 +207,7 @@ fn load_github_app_credentials() -> fabro_github::GitHubCredentials { fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id, private_key_pem, + slug: None, }) }