From f207d6e19ae9396cbad9df537d01c7c5f07b3512 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 11 Apr 2026 21:35:16 -0400 Subject: [PATCH] feat(github): add gh cli integration strategy Make gh_cli the default GitHub integration path across install, server, workflow, and CLI surfaces while keeping app-based setup available when explicitly selected. Also defer GitHub reqwest client initialization until an HTTP request is actually needed so missing-token and token-only paths do not trip workspace test slow timeouts. --- docs/administration/server-configuration.mdx | 18 +- docs/integrations/daytona.mdx | 9 +- docs/integrations/github.mdx | 32 +- docs/reference/cli.mdx | 4 +- lib/crates/fabro-cli/src/commands/install.rs | 231 ++++++++++--- lib/crates/fabro-cli/src/commands/pr/close.rs | 7 +- .../fabro-cli/src/commands/pr/create.rs | 5 +- lib/crates/fabro-cli/src/commands/pr/list.rs | 8 +- lib/crates/fabro-cli/src/commands/pr/merge.rs | 7 +- lib/crates/fabro-cli/src/commands/pr/mod.rs | 35 +- lib/crates/fabro-cli/src/commands/pr/view.rs | 7 +- .../fabro-cli/src/commands/repo/init.rs | 24 +- .../fabro-cli/src/commands/run/runner.rs | 52 +-- lib/crates/fabro-cli/src/shared/github.rs | 17 +- lib/crates/fabro-cli/tests/it/cmd/pr_list.rs | 6 +- lib/crates/fabro-cli/tests/it/cmd/pr_view.rs | 2 +- .../fabro-cli/tests/it/cmd/repo_init.rs | 4 +- lib/crates/fabro-config/src/resolve/server.rs | 1 + .../fabro-config/tests/resolve_server.rs | 44 ++- lib/crates/fabro-github/src/lib.rs | 271 ++++++++++++--- lib/crates/fabro-github/tests/integration.rs | 13 +- lib/crates/fabro-sandbox/src/daytona/mod.rs | 6 +- lib/crates/fabro-sandbox/src/sandbox_spec.rs | 4 +- lib/crates/fabro-server/src/diagnostics.rs | 96 ++++- lib/crates/fabro-server/src/run_manifest.rs | 258 ++++++++++---- lib/crates/fabro-server/src/serve.rs | 119 ++++--- lib/crates/fabro-server/src/server.rs | 327 ++++++++++++------ lib/crates/fabro-tracker/src/github.rs | 35 +- lib/crates/fabro-types/src/settings/server.rs | 11 + .../tests/server_settings_serde.rs | 23 ++ .../fabro-workflow/src/operations/start.rs | 6 +- .../fabro-workflow/src/pipeline/initialize.rs | 11 +- .../src/pipeline/pull_request.rs | 8 +- .../fabro-workflow/src/pipeline/types.rs | 2 +- lib/crates/fabro-workflow/src/run_options.rs | 4 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 8 +- .../tests/it/daytona_integration.rs | 8 +- 37 files changed, 1265 insertions(+), 458 deletions(-) create mode 100644 lib/crates/fabro-types/tests/server_settings_serde.rs diff --git a/docs/administration/server-configuration.mdx b/docs/administration/server-configuration.mdx index eebf47394..cd29ffefb 100644 --- a/docs/administration/server-configuration.mdx +++ b/docs/administration/server-configuration.mdx @@ -162,10 +162,18 @@ Customize the git author identity used for checkpoint commits. When not set, def ### `[server.integrations.github]` section -Configure a GitHub App integration. Required fields include `app_id`, `client_id`, and `slug`. Webhook delivery is configured under `[server.integrations.github.webhooks]`: +Configure GitHub integration auth. `strategy = "gh_cli"` is the default and uses a stored `GITHUB_CLI_TOKEN` from the server secret store. `strategy = "app"` enables the GitHub App flow, browser OAuth, and webhooks. ```toml title="settings.toml" [server.integrations.github] +strategy = "gh_cli" +``` + +For GitHub App mode, set `strategy = "app"` and include `app_id`, `client_id`, and `slug`. Webhook delivery is configured under `[server.integrations.github.webhooks]`: + +```toml title="settings.toml" +[server.integrations.github] +strategy = "app" app_id = "123456" client_id = "Iv1.abc123" slug = "fabro-app" @@ -227,7 +235,13 @@ Fabro stores server-managed credentials in `/secrets.json` and also ho | `FABRO_JWT_PUBLIC_KEY` | Ed25519 public key (base64-encoded PEM) for JWT verification | | `SESSION_SECRET` | Session encryption secret (64-character hex string) | -### GitHub App (optional) +### GitHub integration (optional) + +| Variable | Description | +|---|---| +| `GITHUB_CLI_TOKEN` | Token captured from `gh auth token` and stored by `fabro install` when `strategy = "gh_cli"` | + +### GitHub App extras (optional) | Variable | Description | |---|---| diff --git a/docs/integrations/daytona.mdx b/docs/integrations/daytona.mdx index 52a875b8a..51a4b8bad 100644 --- a/docs/integrations/daytona.mdx +++ b/docs/integrations/daytona.mdx @@ -18,7 +18,7 @@ description: "Run Fabro workflows in sandboxed Daytona cloud environments" ## Prerequisites - A `DAYTONA_API_KEY` environment variable (get one from [app.daytona.io](https://app.daytona.io)) -- A [GitHub App](/integrations/github) configured via the web UI (required for private repository cloning and checkpoint pushing) +- GitHub access configured via the default `gh_cli` strategy or a [GitHub App](/integrations/github) (required for private repository cloning and checkpoint pushing) ## Configuration @@ -99,14 +99,13 @@ If a snapshot is configured by name but doesn't exist and no `dockerfile` is pro ## Private repositories -Fabro automatically clones the current repository into the sandbox at `/home/daytona/workspace`. Public repositories work without extra configuration. Private repositories require a [GitHub App](/integrations/github) — Fabro uses short-lived Installation Access Tokens scoped to the specific repository. +Fabro automatically clones the current repository into the sandbox at `/home/daytona/workspace`. Public repositories work without extra configuration. Private repositories require GitHub access. In `gh_cli` mode, Fabro uses the stored CLI token directly. In `app` mode, Fabro uses short-lived Installation Access Tokens scoped to the specific repository. -If the clone fails without a GitHub App configured, Fabro suggests running the setup flow: +If the clone fails without GitHub access configured, Fabro suggests running the setup flow: ``` Git clone failed: ... If this is a private repository, -configure a GitHub App with `fabro install` and install it -for your organization. +run `gh auth login` or `fabro install` to configure GitHub access. ``` ## SSH access diff --git a/docs/integrations/github.mdx b/docs/integrations/github.mdx index 5ff271000..079ddb740 100644 --- a/docs/integrations/github.mdx +++ b/docs/integrations/github.mdx @@ -3,9 +3,27 @@ title: "GitHub" description: "Integrate Fabro with GitHub for repository access and OAuth login" --- -Fabro uses a [GitHub App](https://docs.github.com/en/apps/overview) to authenticate users in the web UI and to clone private repositories into remote sandboxes. The GitHub App is created automatically through a guided setup flow — no manual app configuration required. +Fabro supports two GitHub integration strategies: -## What the GitHub App enables +- `gh_cli` — the default for local and individual use. Fabro captures `gh auth token` during `fabro install`, stores it as `GITHUB_CLI_TOKEN`, and uses that token directly for repo access, pull requests, and sandbox `GITHUB_TOKEN` injection. +- `app` — the team-oriented option. Fabro registers a [GitHub App](https://docs.github.com/en/apps/overview), uses installation tokens for repo access, and enables browser OAuth and webhooks. + +`gh_cli` changes GitHub integration auth only. It does not provide browser sign-in, so the embedded web UI is disabled when `strategy = "gh_cli"`. + +## Strategy matrix + +| Capability | `gh_cli` | `app` | +|---|---|---| +| CLI pull requests | Yes | Yes | +| Private repo cloning | Yes | Yes | +| Sandbox `GITHUB_TOKEN` | Direct CLI token | Scoped installation token | +| Browser sign-in | No | Yes | +| Web UI routes | Disabled | Enabled | +| Webhooks | No | Yes | + +## GitHub App mode + +The rest of this page describes the `app` strategy, which is required for browser auth and webhooks. | Feature | How it's used | |---|---| @@ -98,6 +116,16 @@ Fabro stores the GitHub App secrets in the server secret store under these keys: The private key is stored as base64-encoded PEM. Fabro also accepts raw PEM format (starting with `-----BEGIN`). +## `gh_cli` mode + +Choose **GitHub CLI** in `fabro install` to use the default local-user flow. The installer: + +1. Runs `gh auth token` +2. Stores the token as `GITHUB_CLI_TOKEN` +3. Writes `strategy = "gh_cli"` under `[server.integrations.github]` + +In this mode, Fabro disables the embedded web UI and browser auth routes. Machine API routes and `/health` continue to work. + ## How it works ### OAuth login diff --git a/docs/reference/cli.mdx b/docs/reference/cli.mdx index ab523b131..60b5e3025 100644 --- a/docs/reference/cli.mdx +++ b/docs/reference/cli.mdx @@ -405,7 +405,7 @@ Run IDs support prefix matching — you can use the first few characters instead ## `fabro pr` -Manage GitHub pull requests created by workflow runs. Requires a [GitHub App](/integrations/github) to be configured. +Manage GitHub pull requests created by workflow runs. Requires GitHub access to be configured via the default `gh_cli` strategy or a [GitHub App](/integrations/github). ### `fabro pr create` @@ -659,7 +659,7 @@ The command must be run inside a git repository. It creates: - `.fabro/workflows/hello/workflow.fabro` — a simple greeting workflow - `.fabro/workflows/hello/workflow.toml` — run config for the hello workflow -After creating files, it checks whether the GitHub App is installed for the repository. If the app is not installed and the repository owner differs from the app owner, it warns that the app may need to be [made public](/integrations/github#github-app-is-private-but-this-repo-belongs-to-a-different-owner) first. +After creating files, it checks whether GitHub access is available for the repository. In GitHub App mode, if the app is not installed and the repository owner differs from the app owner, it warns that the app may need to be [made public](/integrations/github#github-app-is-private-but-this-repo-belongs-to-a-different-owner) first. ## `fabro repo deinit` diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index cdb8d4213..2ccbfcd78 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -3,7 +3,7 @@ use std::net::SocketAddr; use std::path::Path; use std::process::{Command, Stdio}; -use anyhow::{Context, Result, bail}; +use anyhow::{Context, Result, anyhow, bail}; use axum::extract::Query; use axum::response::Html; use axum::routing::get; @@ -254,6 +254,56 @@ fn merge_server_settings(doc: &mut toml::Value, username: &str) -> Result<()> { Ok(()) } +fn github_integration_table(doc: &mut toml::Value) -> Result<&mut toml::Table> { + let root = doc + .as_table_mut() + .context("settings.toml root is not a table")?; + let server = root + .entry("server") + .or_insert(toml::Value::Table(toml::Table::default())); + let server_table = server + .as_table_mut() + .context("settings.toml [server] is not a table")?; + let integrations = server_table + .entry("integrations") + .or_insert(toml::Value::Table(toml::Table::default())); + let integrations_table = integrations + .as_table_mut() + .context("settings.toml [server.integrations] is not a table")?; + let github = integrations_table + .entry("github") + .or_insert(toml::Value::Table(toml::Table::default())); + github + .as_table_mut() + .context("settings.toml [server.integrations.github] is not a table") +} + +fn write_github_cli_settings(doc: &mut toml::Value) -> Result<()> { + let github = github_integration_table(doc)?; + github.insert("strategy".into(), toml::Value::String("gh_cli".to_string())); + github.remove("app_id"); + github.remove("slug"); + github.remove("client_id"); + Ok(()) +} + +fn write_github_app_settings( + doc: &mut toml::Value, + app_id: &str, + slug: &str, + client_id: &str, +) -> Result<()> { + let github = github_integration_table(doc)?; + github.insert("strategy".into(), toml::Value::String("app".to_string())); + github.insert("app_id".into(), toml::Value::String(app_id.to_string())); + github.insert("slug".into(), toml::Value::String(slug.to_string())); + github.insert( + "client_id".into(), + toml::Value::String(client_id.to_string()), + ); + Ok(()) +} + #[cfg(test)] fn format_config_toml(username: &str) -> String { let mut doc = toml::Value::Table(toml::Table::default()); @@ -595,18 +645,7 @@ async fn setup_github_app( } else { toml::from_str(&existing).context("failed to parse existing settings.toml")? }; - let table = doc - .as_table_mut() - .context("settings.toml root is not a table")?; - let git = table - .entry("git") - .or_insert(toml::Value::Table(toml::Table::default())); - let git_table = git - .as_table_mut() - .context("settings.toml [git] is not a table")?; - git_table.insert("app_id".into(), toml::Value::String(app_id)); - git_table.insert("slug".into(), toml::Value::String(slug.clone())); - git_table.insert("client_id".into(), toml::Value::String(client_id)); + write_github_app_settings(&mut doc, &app_id, &slug, &client_id)?; std::fs::write(&user_toml_path, toml::to_string_pretty(&doc)?)?; fabro_util::printerr!( printer, @@ -691,7 +730,7 @@ pub(crate) async fn run_install( fabro_util::printerr!( printer, " {}", - s.dim.apply_to("LLM providers and GitHub App.") + s.dim.apply_to("LLM providers and GitHub access.") ); fabro_util::printerr!(printer, ""); @@ -840,46 +879,72 @@ pub(crate) async fn run_install( } fabro_util::printerr!(printer, ""); - // Step 2: GitHub App - fabro_util::printerr!(printer, " {}", s.bold.apply_to("Step 2 · GitHub App")); - fabro_util::printerr!(printer, " {}", s.dim.apply_to("───────────────────")); + // Step 2: GitHub + fabro_util::printerr!(printer, " {}", s.bold.apply_to("Step 2 · GitHub")); + fabro_util::printerr!(printer, " {}", s.dim.apply_to("───────────────")); fabro_util::printerr!(printer, ""); { - let setup_github = - spawn_blocking(|| prompt_confirm("Set up a GitHub App? (Recommended)", true)).await??; + let strategy_options = vec![ + "GitHub CLI — use your existing `gh` login".to_string(), + "GitHub App — recommended for teams".to_string(), + ]; + let strategy = spawn_blocking({ + let options = strategy_options.clone(); + move || prompt_select("How should Fabro authenticate with GitHub?", &options) + }) + .await??; - if setup_github { - let (owner, username) = prompt_github_app_owner(&s).await?; - let github_env_pairs = setup_github_app( - &fabro_dir, - &s, - web_url, - &owner, - username.as_deref(), - printer, - ) - .await?; - let slug = { + match strategy { + 0 => { + let token = fabro_github::gh_auth_token().map_err(|err| { + anyhow!("{err}. Run `gh auth login` and rerun `fabro install`.") + })?; let user_toml_path = fabro_dir.join(SETTINGS_CONFIG_FILENAME); - let toml_content = std::fs::read_to_string(&user_toml_path).unwrap_or_default(); - let doc: toml::Value = toml::from_str(&toml_content) - .unwrap_or(toml::Value::Table(toml::Table::default())); - doc.get("git") - .and_then(|g| g.get("slug")) - .and_then(|s| s.as_str()) - .unwrap_or("unknown") - .to_string() - }; - fabro_util::printerr!( - printer, - " {} GitHub App registered ({})", - s.green.apply_to("✔"), - slug - ); - secret_pairs.extend(github_env_pairs); - } else { - fabro_util::printerr!(printer, " Skipped"); + let existing = std::fs::read_to_string(&user_toml_path).unwrap_or_default(); + let mut doc: toml::Value = if existing.is_empty() { + toml::Value::Table(toml::Table::default()) + } else { + toml::from_str(&existing).context("failed to parse existing settings.toml")? + }; + write_github_cli_settings(&mut doc)?; + std::fs::write(&user_toml_path, toml::to_string_pretty(&doc)?)?; + fabro_util::printerr!(printer, " {} GitHub CLI configured", s.green.apply_to("✔")); + secret_pairs.push(("GITHUB_CLI_TOKEN".to_string(), token)); + } + 1 => { + let (owner, username) = prompt_github_app_owner(&s).await?; + let github_env_pairs = setup_github_app( + &fabro_dir, + &s, + web_url, + &owner, + username.as_deref(), + printer, + ) + .await?; + let slug = { + let user_toml_path = fabro_dir.join(SETTINGS_CONFIG_FILENAME); + let toml_content = std::fs::read_to_string(&user_toml_path).unwrap_or_default(); + let doc: toml::Value = toml::from_str(&toml_content) + .unwrap_or(toml::Value::Table(toml::Table::default())); + doc.get("server") + .and_then(|server| server.get("integrations")) + .and_then(|integrations| integrations.get("github")) + .and_then(|github| github.get("slug")) + .and_then(|slug| slug.as_str()) + .unwrap_or("unknown") + .to_string() + }; + fabro_util::printerr!( + printer, + " {} GitHub App registered ({})", + s.green.apply_to("✔"), + slug + ); + secret_pairs.extend(github_env_pairs); + } + _ => unreachable!("prompt_select returned an out-of-range index"), } } fabro_util::printerr!(printer, ""); @@ -1274,6 +1339,74 @@ name = "custom" ); } + #[test] + fn write_github_cli_settings_uses_server_integrations_github() { + let mut doc: toml::Value = toml::from_str( + r#" +_version = 1 + +[server.integrations.github] +strategy = "app" +app_id = "123" +slug = "fabro-app" +client_id = "client-id" +"#, + ) + .unwrap(); + + write_github_cli_settings(&mut doc).unwrap(); + + let github = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("integrations")) + .and_then(toml::Value::as_table) + .and_then(|integrations| integrations.get("github")) + .and_then(toml::Value::as_table) + .expect("server.integrations.github should exist"); + + assert_eq!( + github.get("strategy").and_then(toml::Value::as_str), + Some("gh_cli") + ); + assert!(!github.contains_key("app_id")); + assert!(!github.contains_key("slug")); + assert!(!github.contains_key("client_id")); + } + + #[test] + fn write_github_app_settings_uses_server_integrations_github() { + let mut doc = toml::Value::Table(toml::Table::default()); + + write_github_app_settings(&mut doc, "123", "fabro-app", "client-id").unwrap(); + + let github = doc + .get("server") + .and_then(toml::Value::as_table) + .and_then(|server| server.get("integrations")) + .and_then(toml::Value::as_table) + .and_then(|integrations| integrations.get("github")) + .and_then(toml::Value::as_table) + .expect("server.integrations.github should exist"); + + assert_eq!( + github.get("strategy").and_then(toml::Value::as_str), + Some("app") + ); + assert_eq!( + github.get("app_id").and_then(toml::Value::as_str), + Some("123") + ); + assert_eq!( + github.get("slug").and_then(toml::Value::as_str), + Some("fabro-app") + ); + assert_eq!( + github.get("client_id").and_then(toml::Value::as_str), + Some("client-id") + ); + } + // -- GitHub App owner -- #[test] diff --git a/lib/crates/fabro-cli/src/commands/pr/close.rs b/lib/crates/fabro-cli/src/commands/pr/close.rs index 1ceeb30da..a1e63aacf 100644 --- a/lib/crates/fabro-cli/src/commands/pr/close.rs +++ b/lib/crates/fabro-cli/src/commands/pr/close.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use fabro_util::printer::Printer; use tracing::info; @@ -7,15 +7,12 @@ use crate::shared::print_json_pretty; pub(super) async fn close_command( args: PrCloseArgs, - github_app: Option, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { let (record, _run_id) = super::load_pr_record(&args.server, &args.run_id, printer).await?; - let creds = github_app.context( - "GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id", - )?; + let creds = super::load_github_credentials_required(printer)?; fabro_github::close_pull_request( &creds, diff --git a/lib/crates/fabro-cli/src/commands/pr/create.rs b/lib/crates/fabro-cli/src/commands/pr/create.rs index 415b18abc..922ce69b9 100644 --- a/lib/crates/fabro-cli/src/commands/pr/create.rs +++ b/lib/crates/fabro-cli/src/commands/pr/create.rs @@ -15,7 +15,6 @@ use crate::shared::repo::ensure_matching_repo_origin; pub(super) async fn create_command( args: PrCreateArgs, - github_app: Option, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { @@ -72,9 +71,7 @@ pub(super) async fn create_command( let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url) .map_err(|err| anyhow::anyhow!("{err}"))?; - let creds = github_app.context( - "GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id", - )?; + let creds = super::load_github_credentials_required(printer)?; let branch_found = fabro_github::branch_exists( &creds, diff --git a/lib/crates/fabro-cli/src/commands/pr/list.rs b/lib/crates/fabro-cli/src/commands/pr/list.rs index a25165d66..051703887 100644 --- a/lib/crates/fabro-cli/src/commands/pr/list.rs +++ b/lib/crates/fabro-cli/src/commands/pr/list.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use fabro_util::printer::Printer; use futures::future::join_all; use serde::Serialize; @@ -20,13 +20,9 @@ struct PrRow { pub(super) async fn list_command( args: PrListArgs, - github_app: Option, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { - let creds = github_app.context( - "GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id", - )?; let ctx = CommandContext::for_target(&args.server, printer)?; let lookup = ServerSummaryLookup::from_client(ctx.server().await?).await?; @@ -48,6 +44,8 @@ pub(super) async fn list_command( return Ok(()); } + let creds = super::load_github_credentials_required(printer)?; + let futures: Vec<_> = entries .iter() .map(|(run_id, record)| { diff --git a/lib/crates/fabro-cli/src/commands/pr/merge.rs b/lib/crates/fabro-cli/src/commands/pr/merge.rs index 4f4e46fe3..34ac28d3b 100644 --- a/lib/crates/fabro-cli/src/commands/pr/merge.rs +++ b/lib/crates/fabro-cli/src/commands/pr/merge.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use fabro_util::printer::Printer; use tracing::info; @@ -7,15 +7,12 @@ use crate::shared::print_json_pretty; pub(super) async fn merge_command( args: PrMergeArgs, - github_app: Option, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { let (record, _run_id) = super::load_pr_record(&args.server, &args.run_id, printer).await?; - let creds = github_app.context( - "GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id", - )?; + let creds = super::load_github_credentials_required(printer)?; fabro_github::merge_pull_request( &creds, diff --git a/lib/crates/fabro-cli/src/commands/pr/mod.rs b/lib/crates/fabro-cli/src/commands/pr/mod.rs index 639e17c3f..6af52a6d8 100644 --- a/lib/crates/fabro-cli/src/commands/pr/mod.rs +++ b/lib/crates/fabro-cli/src/commands/pr/mod.rs @@ -4,7 +4,8 @@ mod list; mod merge; mod view; -use anyhow::{Context, Result}; +use anyhow::{Context, Result, anyhow}; +use fabro_github::GitHubCredentials; use fabro_types::PullRequestRecord; use fabro_types::settings::InterpString; use fabro_util::printer::Printer; @@ -12,17 +13,29 @@ use fabro_util::printer::Printer; use crate::args::{GlobalArgs, PrCommand, PrNamespace, ServerTargetArgs}; use crate::command_context::CommandContext; use crate::server_runs::ServerSummaryLookup; -use crate::shared::github::build_github_app_credentials; +use crate::shared::github::build_github_credentials; + +const GITHUB_CREDENTIALS_REQUIRED: &str = "GitHub credentials required — run `gh auth login` or configure a GitHub App with `fabro install`"; pub(crate) async fn dispatch( ns: PrNamespace, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { + match ns.command { + PrCommand::Create(args) => Box::pin(create::create_command(args, globals, printer)).await, + PrCommand::List(args) => list::list_command(args, globals, printer).await, + PrCommand::View(args) => view::view_command(args, globals, printer).await, + PrCommand::Merge(args) => merge::merge_command(args, globals, printer).await, + PrCommand::Close(args) => close::close_command(args, globals, printer).await, + } +} + +fn load_github_credentials_required(printer: Printer) -> Result { let ctx = CommandContext::base(printer)?; let server_settings = fabro_config::resolve_server_from_file(ctx.machine_settings()).map_err(|errors| { - anyhow::anyhow!( + anyhow!( "failed to resolve server settings:\n{}", errors .into_iter() @@ -31,7 +44,8 @@ pub(crate) async fn dispatch( .join("\n") ) })?; - let github_app = build_github_app_credentials( + let creds = build_github_credentials( + server_settings.integrations.github.strategy, server_settings .integrations .github @@ -39,16 +53,9 @@ pub(crate) async fn dispatch( .as_ref() .map(InterpString::as_source) .as_deref(), - )?; - match ns.command { - PrCommand::Create(args) => { - Box::pin(create::create_command(args, github_app, globals, printer)).await - } - PrCommand::List(args) => list::list_command(args, github_app, globals, printer).await, - PrCommand::View(args) => view::view_command(args, github_app, globals, printer).await, - PrCommand::Merge(args) => merge::merge_command(args, github_app, globals, printer).await, - PrCommand::Close(args) => close::close_command(args, github_app, globals, printer).await, - } + ) + .map_err(|_| anyhow!(GITHUB_CREDENTIALS_REQUIRED))?; + creds.context(GITHUB_CREDENTIALS_REQUIRED) } pub(crate) async fn load_pr_record( diff --git a/lib/crates/fabro-cli/src/commands/pr/view.rs b/lib/crates/fabro-cli/src/commands/pr/view.rs index 735a602db..ae0fee547 100644 --- a/lib/crates/fabro-cli/src/commands/pr/view.rs +++ b/lib/crates/fabro-cli/src/commands/pr/view.rs @@ -1,4 +1,4 @@ -use anyhow::{Context, Result}; +use anyhow::Result; use fabro_util::printer::Printer; use tracing::info; @@ -7,15 +7,12 @@ use crate::shared::print_json_pretty; pub(super) async fn view_command( args: PrViewArgs, - github_app: Option, globals: &GlobalArgs, printer: Printer, ) -> Result<()> { let (record, _run_id) = super::load_pr_record(&args.server, &args.run_id, printer).await?; - let creds = github_app.context( - "GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id", - )?; + let creds = super::load_github_credentials_required(printer)?; let detail = fabro_github::get_pull_request( &creds, diff --git a/lib/crates/fabro-cli/src/commands/repo/init.rs b/lib/crates/fabro-cli/src/commands/repo/init.rs index 4e908350d..085f58d69 100644 --- a/lib/crates/fabro-cli/src/commands/repo/init.rs +++ b/lib/crates/fabro-cli/src/commands/repo/init.rs @@ -154,14 +154,14 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print let dim = console::Style::new().dim(); fabro_util::printerr!( printer, - "\n {} No git remote found — skipping GitHub App check", + "\n {} No git remote found — skipping GitHub check", yellow.apply_to("!") ); fabro_util::printerr!( printer, " {}", dim.apply_to( - "Run `git remote add origin ` then `fabro install` to set up the GitHub App" + "Run `git remote add origin `, then `gh auth login` or `fabro install` to configure GitHub access" ) ); return; @@ -211,10 +211,7 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print { Ok(response) => response.into_inner(), Err(err) => { - fabro_util::printerr!( - printer, - "\n Warning: could not check GitHub App installation: {err}" - ); + fabro_util::printerr!(printer, "\n Warning: could not check GitHub access: {err}"); return; } }; @@ -223,7 +220,7 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print let green = console::Style::new().green(); fabro_util::printerr!( printer, - "\n {} GitHub App is installed for {owner}/{repo}", + "\n {} GitHub access is configured for {owner}/{repo}", green.apply_to("✔") ); return; @@ -232,11 +229,16 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print let yellow = console::Style::new().yellow(); fabro_util::printerr!( printer, - "\n {} GitHub App is not installed for {owner}/{repo}", + "\n {} GitHub access is not available for {owner}/{repo}", yellow.apply_to("!") ); if let Some(url) = &check.install_url { fabro_util::printerr!(printer, " Install at: {url}"); + } else { + fabro_util::printerr!( + printer, + " Run `gh auth login` or `fabro install`, then try again." + ); } if std::io::IsTerminal::is_terminal(&std::io::stdin()) { @@ -261,11 +263,11 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print let green = console::Style::new().green(); fabro_util::printerr!( printer, - " {} GitHub App is installed for {owner}/{repo}", + " {} GitHub access is configured for {owner}/{repo}", green.apply_to("✔") ); } else { - fabro_util::printerr!(printer, " GitHub App is still not installed."); + fabro_util::printerr!(printer, " GitHub access is still unavailable."); if let Some(url) = &check.install_url { fabro_util::printerr!(printer, " Install at: {url}"); } @@ -274,7 +276,7 @@ async fn check_github_app_installation(target: &ServerTargetArgs, printer: Print Err(err) => { fabro_util::printerr!( printer, - " Warning: could not re-check GitHub App installation: {err}" + " Warning: could not re-check GitHub access: {err}" ); } } diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 735c28737..9ffa0059d 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -8,6 +8,7 @@ use anyhow::{Context, Result, anyhow}; use async_trait::async_trait; use fabro_interview::{ControlInterviewer, WorkerControlEnvelope, WorkerControlMessage}; use fabro_store::{EventEnvelope, EventPayload, RunProjection}; +use fabro_types::settings::run::RunMode; use fabro_types::settings::{InterpString, SettingsLayer}; use fabro_types::{EventBody, RunBlobId, RunEvent, RunId, StatusReason}; use fabro_workflow::artifact_snapshot::CapturedArtifactInfo; @@ -23,7 +24,7 @@ use tokio::time::sleep; use crate::args::RunWorkerMode; use crate::server_client; -use crate::shared::github::build_github_app_credentials; +use crate::shared::github::build_github_credentials; const RUN_STORE_RETRY_DELAYS: [Duration; 3] = [ Duration::from_millis(50), @@ -74,7 +75,7 @@ pub(crate) async fn execute( spawn_worker_control_stream(Arc::clone(&interviewer), Arc::clone(&cancel_token))?; let run_control = RunControlState::new(); install_signal_handlers(Arc::clone(&run_control), Arc::clone(&cancel_token))?; - let github_app = maybe_build_github_app_credentials(&run_record.settings)?; + let github_app = maybe_build_github_credentials(&run_record.settings)?; let services = StartServices { run_id, cancel_token: Some(Arc::clone(&cancel_token)), @@ -465,32 +466,39 @@ fn update_worker_title_from_event(event: &RunEvent) { } } -fn maybe_build_github_app_credentials( +fn maybe_build_github_credentials( settings: &SettingsLayer, -) -> Result> { +) -> Result> { let resolved_run = fabro_config::resolve_run_from_file(settings).ok(); let resolved_server = fabro_config::resolve_server_from_file(settings).ok(); - let needs_github_app = resolved_run + let required_github_credentials = resolved_run.as_ref().is_some_and(|settings| { + settings.execution.mode != RunMode::DryRun && settings.sandbox.provider == "daytona" + }) || resolved_server .as_ref() - .is_some_and(|settings| settings.sandbox.provider == "daytona") - || resolved_run - .as_ref() - .is_some_and(|settings| settings.pull_request.is_some()) - || resolved_server - .as_ref() - .is_some_and(|settings| !settings.integrations.github.permissions.is_empty()); + .is_some_and(|settings| !settings.integrations.github.permissions.is_empty()); + let pull_request_enabled = resolved_run.as_ref().is_some_and(|settings| { + settings.execution.mode != RunMode::DryRun && settings.pull_request.is_some() + }); + let strategy = resolved_server + .as_ref() + .map(|settings| settings.integrations.github.strategy) + .unwrap_or_default(); + let app_id = resolved_server + .as_ref() + .and_then(|settings| settings.integrations.github.app_id.as_ref()) + .map(InterpString::as_source); - if needs_github_app { - build_github_app_credentials( - resolved_server - .as_ref() - .and_then(|settings| settings.integrations.github.app_id.as_ref()) - .map(InterpString::as_source) - .as_deref(), - ) - } else { - Ok(None) + if required_github_credentials { + return build_github_credentials(strategy, app_id.as_deref()); } + + if pull_request_enabled { + return Ok(build_github_credentials(strategy, app_id.as_deref()) + .ok() + .flatten()); + } + + Ok(None) } fn install_signal_handlers( diff --git a/lib/crates/fabro-cli/src/shared/github.rs b/lib/crates/fabro-cli/src/shared/github.rs index 894436a47..fc748bde6 100644 --- a/lib/crates/fabro-cli/src/shared/github.rs +++ b/lib/crates/fabro-cli/src/shared/github.rs @@ -1,8 +1,17 @@ use anyhow::anyhow; -use fabro_github::GitHubAppCredentials; +use fabro_github::GitHubCredentials; +use fabro_types::settings::server::GithubIntegrationStrategy; -pub(crate) fn build_github_app_credentials( +pub(crate) fn build_github_credentials( + strategy: GithubIntegrationStrategy, app_id: Option<&str>, -) -> anyhow::Result> { - GitHubAppCredentials::from_env(app_id).map_err(|err| anyhow!(err)) +) -> anyhow::Result> { + match strategy { + GithubIntegrationStrategy::App => { + GitHubCredentials::from_env(app_id).map_err(|err| anyhow!(err)) + } + GithubIntegrationStrategy::GhCli => fabro_github::gh_auth_token() + .map(|token| Some(GitHubCredentials::Token(token))) + .map_err(|err| anyhow!(err)), + } } diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs index 784976ce4..66cd60308 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs @@ -33,10 +33,10 @@ fn pr_list_missing_github_credentials_errors() { cmd.args(["pr", "list"]); fabro_snapshot!(context.filters(), cmd, @" - success: false - exit_code: 1 + success: true + exit_code: 0 ----- stdout ----- + No pull requests found. ----- stderr ----- - error: GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs index 6a6e56753..a4b6f295e 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/pr_view.rs @@ -126,6 +126,6 @@ fn pr_view_reads_pull_request_from_store_without_pull_request_json() { exit_code: 1 ----- stdout ----- ----- stderr ----- - error: GitHub App credentials required — set GITHUB_APP_PRIVATE_KEY and configure app_id + error: GitHub credentials required — run `gh auth login` or configure a GitHub App with `fabro install` "); } diff --git a/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs b/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs index 161aa530b..68c2ce931 100644 --- a/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs +++ b/lib/crates/fabro-cli/tests/it/cmd/repo_init.rs @@ -47,8 +47,8 @@ fn repo_init_creates_project_toml_and_hello_workflow() { fabro run hello - ! No git remote found — skipping GitHub App check - Run `git remote add origin ` then `fabro install` to set up the GitHub App + ! No git remote found — skipping GitHub check + Run `git remote add origin `, then `gh auth login` or `fabro install` to configure GitHub access "); assert_snapshot!( diff --git a/lib/crates/fabro-config/src/resolve/server.rs b/lib/crates/fabro-config/src/resolve/server.rs index 524f6182e..66a537798 100644 --- a/lib/crates/fabro-config/src/resolve/server.rs +++ b/lib/crates/fabro-config/src/resolve/server.rs @@ -259,6 +259,7 @@ fn resolve_integrations(layer: Option<&ServerIntegrationsLayer>) -> ServerIntegr .and_then(|integrations| integrations.github.as_ref()) .map(|github| GithubIntegrationSettings { enabled: github.enabled.unwrap_or(true), + strategy: github.strategy.unwrap_or_default(), app_id: github.app_id.clone(), client_id: github.client_id.clone(), slug: github.slug.clone(), diff --git a/lib/crates/fabro-config/tests/resolve_server.rs b/lib/crates/fabro-config/tests/resolve_server.rs index 8a16b349c..fa4efb220 100644 --- a/lib/crates/fabro-config/tests/resolve_server.rs +++ b/lib/crates/fabro-config/tests/resolve_server.rs @@ -1,5 +1,7 @@ use fabro_config::parse_settings_layer; -use fabro_types::settings::server::{ObjectStoreSettings, ServerListenSettings}; +use fabro_types::settings::server::{ + GithubIntegrationStrategy, ObjectStoreSettings, ServerListenSettings, +}; use fabro_types::settings::{InterpString, SettingsLayer}; use fabro_util::Home; @@ -139,3 +141,43 @@ slug = "fabro-app" Some(InterpString::parse("fabro-app")) ); } + +#[test] +fn resolves_github_integration_strategy_from_settings() { + let file = parse( + r#" +_version = 1 + +[server.integrations.github] +strategy = "app" +"#, + ); + + let settings = + fabro_config::resolve_server_from_file(&file).expect("server settings should resolve"); + + assert_eq!( + settings.integrations.github.strategy, + GithubIntegrationStrategy::App + ); +} + +#[test] +fn defaults_github_integration_strategy_to_gh_cli() { + let file = parse( + r#" +_version = 1 + +[server.integrations.github] +enabled = true +"#, + ); + + let settings = + fabro_config::resolve_server_from_file(&file).expect("server settings should resolve"); + + assert_eq!( + settings.integrations.github.strategy, + GithubIntegrationStrategy::GhCli + ); +} diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 9b47fbfe6..286b185db 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -83,6 +83,67 @@ impl GitHubAppCredentials { } } +#[derive(Clone, Debug)] +pub enum GitHubCredentials { + App(GitHubAppCredentials), + Token(String), +} + +impl GitHubCredentials { + pub fn from_env(app_id: Option<&str>) -> Result, String> { + Ok(GitHubAppCredentials::from_env(app_id)?.map(Self::App)) + } + + async fn resolve_bearer_token( + &self, + client: &impl HttpClient, + owner: &str, + repo: &str, + base_url: &str, + permissions: serde_json::Value, + ) -> Result { + match self { + Self::App(creds) => { + let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; + create_installation_access_token_with_permissions( + client, + &jwt, + owner, + repo, + base_url, + permissions, + ) + .await + } + Self::Token(token) => Ok(token.clone()), + } + } +} + +pub fn gh_auth_token() -> Result { + let output = std::process::Command::new("gh") + .args(["auth", "token"]) + .output() + .map_err(|err| format!("Failed to run `gh auth token`: {err}"))?; + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + let message = if stderr.is_empty() { + format!("`gh auth token` exited with status {}", output.status) + } else { + stderr + }; + return Err(format!("Failed to get GitHub CLI token: {message}")); + } + + let token = String::from_utf8(output.stdout) + .map_err(|err| format!("`gh auth token` returned invalid UTF-8: {err}"))?; + let token = token.trim().to_string(); + if token.is_empty() { + return Err("`gh auth token` returned an empty token".to_string()); + } + Ok(token) +} + fn decode_pem_env(name: &str, raw: &str) -> Result { if raw.starts_with("-----") { return Ok(raw.to_string()); @@ -396,7 +457,7 @@ pub struct CreatedPullRequest { /// GitHub pulls API. #[allow(clippy::too_many_arguments)] pub async fn create_pull_request( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, base: &str, @@ -413,11 +474,16 @@ pub async fn create_pull_request( node_id: String, } - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; let client = reqwest::Client::new(); - - let token = - create_installation_access_token_for_pr(&client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + &client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await?; tracing::debug!(title = %title, head = %head, base = %base, draft, "Creating pull request"); @@ -498,18 +564,23 @@ impl AutoMergeMethod { /// Requires the PR's `node_id` (from the REST API response) and a merge method. /// The repository must have auto-merge enabled in its settings. pub async fn enable_auto_merge( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, pr_node_id: &str, merge_method: AutoMergeMethod, base_url: &str, ) -> Result<(), String> { - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; let client = reqwest::Client::new(); - - let token = - create_installation_access_token_for_pr(&client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + &client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await?; let query = format!( r#"mutation {{ @@ -620,7 +691,7 @@ fn normalize_https_host_path(url: &str) -> String { /// Uses a GitHub App installation token to query the branches API. /// Returns `true` if the branch exists, `false` if it doesn't (404). pub async fn branch_exists( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, branch: &str, @@ -639,14 +710,21 @@ pub async fn branch_exists( async fn branch_exists_with_client( client: &impl HttpClient, - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, branch: &str, base_url: &str, ) -> Result { - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - let token = create_installation_access_token(client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write" }), + ) + .await?; let url = format!("{base_url}/repos/{owner}/{repo}/branches/{branch}"); let auth = format!("Bearer {token}"); @@ -768,15 +846,26 @@ pub async fn is_app_public( /// Always generates a token regardless of repo visibility, since the token /// is needed for pushing from the sandbox. pub async fn resolve_clone_credentials( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, base_url: &str, ) -> Result<(Option, Option), String> { - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - let client = reqwest::Client::new(); - - let token = create_installation_access_token(&client, &jwt, owner, repo, base_url).await?; + let token = match creds { + GitHubCredentials::Token(token) => token.clone(), + GitHubCredentials::App(_) => { + let client = reqwest::Client::new(); + creds + .resolve_bearer_token( + &client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write" }), + ) + .await? + } + }; Ok((Some("x-access-token".to_string()), Some(token))) } @@ -793,7 +882,7 @@ pub fn embed_token_in_url(url: &str, token: &str) -> String { /// Parses owner/repo from the URL, obtains a fresh installation access token, /// and returns the URL with embedded credentials. pub async fn resolve_authenticated_url( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, url: &str, base_url: &str, ) -> Result { @@ -807,7 +896,7 @@ pub async fn resolve_authenticated_url( /// Fetch detailed information about a pull request. pub async fn get_pull_request( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -826,7 +915,7 @@ pub async fn get_pull_request( async fn get_pull_request_with_client( client: &impl HttpClient, - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -834,9 +923,15 @@ async fn get_pull_request_with_client( ) -> Result { tracing::debug!(owner, repo, number, "Fetching pull request"); - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - let token = - create_installation_access_token_for_pr(client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await?; let url = format!("{base_url}/repos/{owner}/{repo}/pulls/{number}"); let auth = format!("Bearer {token}"); @@ -872,7 +967,7 @@ async fn get_pull_request_with_client( /// Merge a pull request. pub async fn merge_pull_request( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -894,7 +989,7 @@ pub async fn merge_pull_request( #[allow(clippy::too_many_arguments)] async fn merge_pull_request_with_client( client: &impl HttpClient, - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -903,9 +998,15 @@ async fn merge_pull_request_with_client( ) -> Result<(), String> { tracing::debug!(owner, repo, number, method, "Merging pull request"); - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - let token = - create_installation_access_token_for_pr(client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await?; let url = format!("{base_url}/repos/{owner}/{repo}/pulls/{number}/merge"); let body = serde_json::json!({ "merge_method": method }); @@ -938,7 +1039,7 @@ async fn merge_pull_request_with_client( /// Close a pull request. pub async fn close_pull_request( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -957,7 +1058,7 @@ pub async fn close_pull_request( async fn close_pull_request_with_client( client: &impl HttpClient, - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, owner: &str, repo: &str, number: u64, @@ -965,9 +1066,15 @@ async fn close_pull_request_with_client( ) -> Result<(), String> { tracing::debug!(owner, repo, number, "Closing pull request"); - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; - let token = - create_installation_access_token_for_pr(client, &jwt, owner, repo, base_url).await?; + let token = creds + .resolve_bearer_token( + client, + owner, + repo, + base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await?; let url = format!("{base_url}/repos/{owner}/{repo}/pulls/{number}"); let body = serde_json::json!({ "state": "closed" }); @@ -1469,10 +1576,10 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let result = branch_exists_with_client(&mock, &creds, "owner", "repo", "my-branch", "").await; assert!(result.unwrap()); @@ -1501,10 +1608,10 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let result = branch_exists_with_client(&mock, &creds, "owner", "repo", "no-such-branch", "").await; assert!(!result.unwrap()); @@ -1533,14 +1640,32 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let result = branch_exists_with_client(&mock, &creds, "owner", "repo", "broken", "").await; assert!(result.is_err()); } + #[tokio::test] + async fn branch_exists_with_token_uses_direct_bearer_token() { + let mock = MockHttpClient::new() + .on( + HttpMethod::Get, + "/repos/owner/repo/branches/my-branch", + 200, + r#"{"name": "my-branch"}"#, + ) + .with_req_header("Authorization", "Bearer ghu_test"); + + let creds = GitHubCredentials::Token("ghu_test".to_string()); + let result = + branch_exists_with_client(&mock, &creds, "owner", "repo", "my-branch", "").await; + + assert!(result.unwrap()); + } + // ----------------------------------------------------------------------- // check_app_installed // ----------------------------------------------------------------------- @@ -1701,10 +1826,10 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let detail = get_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "") .await .unwrap(); @@ -1738,10 +1863,10 @@ mod tests { .on(HttpMethod::Get, "/repos/owner/repo/pulls/999", 404, ""); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let err = get_pull_request_with_client(&mock, &creds, "owner", "repo", 999, "") .await .unwrap_err(); @@ -1749,6 +1874,42 @@ mod tests { assert!(err.contains("#999"), "got: {err}"); } + #[tokio::test] + async fn get_pr_with_token_uses_direct_bearer_token() { + let mock = MockHttpClient::new() + .on( + HttpMethod::Get, + "/repos/owner/repo/pulls/42", + 200, + mock_pr_json(), + ) + .with_req_header("Authorization", "Bearer ghu_test"); + + let creds = GitHubCredentials::Token("ghu_test".to_string()); + let detail = get_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "") + .await + .unwrap(); + + assert_eq!(detail.number, 42); + } + + #[tokio::test] + async fn resolve_clone_credentials_returns_token_for_token_credentials() { + let creds = GitHubCredentials::Token("ghu_test".to_string()); + + let credentials = resolve_clone_credentials(&creds, "owner", "repo", "") + .await + .unwrap(); + + assert_eq!( + credentials, + ( + Some("x-access-token".to_string()), + Some("ghu_test".to_string()) + ) + ); + } + // ----------------------------------------------------------------------- // merge_pull_request // ----------------------------------------------------------------------- @@ -1776,10 +1937,10 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); merge_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "squash", "") .await .unwrap(); @@ -1803,10 +1964,10 @@ mod tests { .on(HttpMethod::Put, "/repos/owner/repo/pulls/42/merge", 405, ""); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let err = merge_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "squash", "") .await .unwrap_err(); @@ -1831,10 +1992,10 @@ mod tests { .on(HttpMethod::Put, "/repos/owner/repo/pulls/42/merge", 409, ""); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let err = merge_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "squash", "") .await .unwrap_err(); @@ -1868,10 +2029,10 @@ mod tests { ); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); close_pull_request_with_client(&mock, &creds, "owner", "repo", 42, "") .await .unwrap(); @@ -1895,10 +2056,10 @@ mod tests { .on(HttpMethod::Patch, "/repos/owner/repo/pulls/999", 404, ""); let pem = test_rsa_key(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(GitHubAppCredentials { app_id: "test".to_string(), private_key_pem: pem.to_string(), - }; + }); let err = close_pull_request_with_client(&mock, &creds, "owner", "repo", 999, "") .await .unwrap_err(); diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index 47fa65e54..1943e4ce1 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -1,5 +1,5 @@ use fabro_github::{ - AutoMergeMethod, GitHubAppCredentials, close_pull_request, + AutoMergeMethod, GitHubAppCredentials, GitHubCredentials, close_pull_request, create_installation_access_token_for_pr, create_pull_request, enable_auto_merge, get_pull_request, merge_pull_request, resolve_authenticated_url, sign_app_jwt, }; @@ -7,11 +7,11 @@ use fabro_test::{GitHubAppOptions, GitHubAppState, TwinGitHub}; const TEST_RSA_KEY: &str = include_str!("../src/testdata/rsa_private.pem"); -fn github_credentials() -> GitHubAppCredentials { - GitHubAppCredentials { +fn github_credentials() -> GitHubCredentials { + GitHubCredentials::App(GitHubAppCredentials { app_id: "42".to_string(), private_key_pem: TEST_RSA_KEY.to_string(), - } + }) } fn standard_app_state() -> GitHubAppState { @@ -167,7 +167,10 @@ async fn enable_auto_merge_persists() { .await .unwrap(); - let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem).unwrap(); + let GitHubCredentials::App(app_creds) = &creds else { + panic!("expected app credentials"); + }; + let jwt = sign_app_jwt(&app_creds.app_id, &app_creds.private_key_pem).unwrap(); let client = fabro_test::test_http_client(); let token = create_installation_access_token_for_pr(&client, &jwt, "acme", "widgets", &twin.base_url) diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index 557cf8dc0..e1c03de3c 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -5,7 +5,7 @@ use std::time::Instant; use async_trait::async_trait; use daytona_sdk::api_types::SignedPortPreviewUrl; -use fabro_github::GitHubAppCredentials; +use fabro_github::GitHubCredentials; use fabro_types::RunId; use rand::Rng; use tokio::sync::OnceCell; @@ -30,7 +30,7 @@ pub use crate::config::{ pub struct DaytonaSandbox { config: DaytonaConfig, client: daytona_sdk::Client, - github_app: Option, + github_app: Option, sandbox: OnceCell, rg_available: OnceCell, event_callback: Option, @@ -48,7 +48,7 @@ impl DaytonaSandbox { /// Create a new `DaytonaSandbox`, creating the Daytona client internally. pub async fn new( config: DaytonaConfig, - github_app: Option, + github_app: Option, run_id: Option, clone_branch: Option, ) -> Result { diff --git a/lib/crates/fabro-sandbox/src/sandbox_spec.rs b/lib/crates/fabro-sandbox/src/sandbox_spec.rs index a2031dd81..c281a900e 100644 --- a/lib/crates/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/crates/fabro-sandbox/src/sandbox_spec.rs @@ -4,7 +4,7 @@ use std::sync::Arc; #[cfg(any(feature = "docker", feature = "daytona"))] use anyhow::anyhow; #[cfg(feature = "daytona")] -use fabro_github::GitHubAppCredentials; +use fabro_github::GitHubCredentials; use fabro_types::RunId; use crate::config::WorktreeMode; @@ -28,7 +28,7 @@ pub enum SandboxSpec { #[cfg(feature = "daytona")] Daytona { config: DaytonaConfig, - github_app: Option, + github_app: Option, run_id: Option, clone_branch: Option, }, diff --git a/lib/crates/fabro-server/src/diagnostics.rs b/lib/crates/fabro-server/src/diagnostics.rs index 56e10cb5c..89ea73d67 100644 --- a/lib/crates/fabro-server/src/diagnostics.rs +++ b/lib/crates/fabro-server/src/diagnostics.rs @@ -9,6 +9,7 @@ use fabro_llm::client::Client as LlmClient; use fabro_llm::types::{Message, Request}; use fabro_model::{Catalog, Provider}; use fabro_types::settings::InterpString; +use fabro_types::settings::server::GithubIntegrationStrategy; use fabro_util::check_report::{CheckDetail, CheckResult, CheckSection, CheckStatus}; use fabro_util::version::FABRO_VERSION; use regex::Regex; @@ -290,6 +291,97 @@ async fn probe_llm_provider(client: &LlmClient, provider: Provider) -> Result<() async fn check_github_app(state: &AppState) -> CheckResult { let settings = state.server_settings(); + if settings.integrations.github.strategy == GithubIntegrationStrategy::GhCli { + let token = match state + .github_credentials(&settings.integrations.github) + .await + { + Ok(Some(fabro_github::GitHubCredentials::Token(token))) => token, + Ok(Some(_)) => unreachable!("gh_cli strategy should not return app credentials"), + Ok(None) => { + return CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Warning, + summary: "not configured".to_string(), + details: Vec::new(), + remediation: Some( + "Run fabro install on the server host to store GITHUB_CLI_TOKEN" + .to_string(), + ), + }; + } + Err(err) => { + return CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "missing token".to_string(), + details: vec![CheckDetail::new(err.clone())], + remediation: Some(err), + }; + } + }; + + let http = reqwest::Client::new(); + let probe = timeout( + Duration::from_secs(15), + http.get(format!("{}/user", fabro_github::github_api_base_url())) + .header("Authorization", format!("Bearer {token}")) + .header("Accept", "application/vnd.github+json") + .header("User-Agent", "fabro-server") + .send(), + ) + .await; + + return match probe { + Ok(Ok(response)) if response.status().is_success() => CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Pass, + summary: "configured".to_string(), + details: Vec::new(), + remediation: None, + }, + Ok(Ok(response)) if response.status() == reqwest::StatusCode::UNAUTHORIZED => { + CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "token invalid".to_string(), + details: vec![CheckDetail::new(format!( + "GitHub returned {}", + response.status() + ))], + remediation: Some( + "Run fabro install on the server host to refresh GITHUB_CLI_TOKEN" + .to_string(), + ), + } + } + Ok(Ok(response)) => CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "connectivity error".to_string(), + details: vec![CheckDetail::new(format!( + "GitHub returned {}", + response.status() + ))], + remediation: Some("Check GitHub connectivity and the stored CLI token".to_string()), + }, + Ok(Err(err)) => CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "connectivity error".to_string(), + details: vec![CheckDetail::new(err.to_string())], + remediation: Some("Check GitHub connectivity and the stored CLI token".to_string()), + }, + Err(_) => CheckResult { + name: "GitHub CLI".to_string(), + status: CheckStatus::Error, + summary: "timeout".to_string(), + details: vec![CheckDetail::new("GitHub probe timed out".to_string())], + remediation: Some("Check GitHub connectivity and the stored CLI token".to_string()), + }, + }; + } + let app_id = settings .integrations .github @@ -328,7 +420,9 @@ async fn check_github_app(state: &AppState) -> CheckResult { status: CheckStatus::Error, summary: "missing app_id".to_string(), details: Vec::new(), - remediation: Some("Set git.app_id in settings.toml".to_string()), + remediation: Some( + "Set [server.integrations.github].app_id in settings.toml".to_string(), + ), }; }; let Some(private_key_raw) = private_key_raw else { diff --git a/lib/crates/fabro-server/src/run_manifest.rs b/lib/crates/fabro-server/src/run_manifest.rs index b5e7ab274..52c1b47fa 100644 --- a/lib/crates/fabro-server/src/run_manifest.rs +++ b/lib/crates/fabro-server/src/run_manifest.rs @@ -336,6 +336,20 @@ async fn build_preflight_report( validated: &Validated, ) -> Result<(CheckReport, bool)> { let graph = validated.graph(); + let mut checks = base_preflight_checks(prepared, graph); + if validated.has_errors() { + return Ok(( + CheckReport { + title: "Run Preflight".into(), + sections: vec![CheckSection { + title: String::new(), + checks, + }], + }, + true, + )); + } + let settings = &prepared.settings; let materialized = materialize_run(settings.clone(), graph, Catalog::builtin()); let resolved_run = fabro_config::resolve_run_from_file(&materialized) @@ -343,62 +357,22 @@ async fn build_preflight_report( let resolved_server = fabro_config::resolve_server_from_file(settings) .map_err(|errors| anyhow!(render_resolve_errors(&errors)))?; let sandbox_provider = resolve_sandbox_provider(&resolved_run)?; - let github_app = state - .github_app_credentials( - resolved_server - .integrations - .github - .app_id - .as_ref() - .map(InterpString::as_source) - .as_deref(), - ) - .await - .map_err(|err| anyhow!(err))?; - let mut checks = Vec::new(); - - let setup_command_count = resolved_run.prepare.commands.len(); - let repo_summary = prepared.git.as_ref().map_or_else( - || "unknown".to_string(), - |git| { - let https = fabro_github::ssh_url_to_https(&git.origin_url); - fabro_github::parse_github_owner_repo(&https).map_or_else( - |_| git.origin_url.clone(), - |(owner, repo)| format!("{owner}/{repo}"), - ) - }, - ); - checks.push(CheckResult { - name: "Repository".into(), - status: CheckStatus::Pass, - summary: repo_summary, - details: vec![ - CheckDetail::new(format!("Setup commands: {setup_command_count}")), - CheckDetail { - text: format!( - "Git: {}", - prepared.git.as_ref().map_or("unknown", |git| if git.clean { - "clean" - } else { - "dirty" - }) - ), - warn: prepared.git.as_ref().is_some_and(|git| !git.clean), - }, - ], - remediation: None, - }); - checks.push(CheckResult { - name: "Workflow".into(), - status: CheckStatus::Pass, - summary: graph.name.clone(), - details: vec![ - CheckDetail::new(format!("Nodes: {}", graph.nodes.len())), - CheckDetail::new(format!("Edges: {}", graph.edges.len())), - CheckDetail::new(format!("Goal: {}", graph.goal())), - ], - remediation: None, - }); + let sandbox_provider = + if resolved_run.execution.mode == RunMode::DryRun && !sandbox_provider.is_local() { + SandboxProvider::Local + } else { + sandbox_provider + }; + let needs_github_credentials = sandbox_provider == SandboxProvider::Daytona + || !resolved_server.integrations.github.permissions.is_empty(); + let github_app = if needs_github_credentials { + state + .github_credentials(&resolved_server.integrations.github) + .await + .unwrap_or_default() + } else { + None + }; let sandbox_ok = run_sandbox_check( &mut checks, @@ -425,6 +399,56 @@ async fn build_preflight_report( )) } +fn base_preflight_checks(prepared: &PreparedManifest, graph: &Graph) -> Vec { + let setup_command_count = fabro_config::resolve_run_from_file(&prepared.settings) + .map(|settings| settings.prepare.commands.len()) + .unwrap_or_default(); + let repo_summary = prepared.git.as_ref().map_or_else( + || "unknown".to_string(), + |git| { + let https = fabro_github::ssh_url_to_https(&git.origin_url); + fabro_github::parse_github_owner_repo(&https).map_or_else( + |_| git.origin_url.clone(), + |(owner, repo)| format!("{owner}/{repo}"), + ) + }, + ); + + vec![ + CheckResult { + name: "Repository".into(), + status: CheckStatus::Pass, + summary: repo_summary, + details: vec![ + CheckDetail::new(format!("Setup commands: {setup_command_count}")), + CheckDetail { + text: format!( + "Git: {}", + prepared.git.as_ref().map_or("unknown", |git| if git.clean { + "clean" + } else { + "dirty" + }) + ), + warn: prepared.git.as_ref().is_some_and(|git| !git.clean), + }, + ], + remediation: None, + }, + CheckResult { + name: "Workflow".into(), + status: CheckStatus::Pass, + summary: graph.name.clone(), + details: vec![ + CheckDetail::new(format!("Nodes: {}", graph.nodes.len())), + CheckDetail::new(format!("Edges: {}", graph.edges.len())), + CheckDetail::new(format!("Goal: {}", graph.goal())), + ], + remediation: None, + }, + ] +} + fn resolve_sandbox_provider(settings: &RunSettings) -> Result { Ok(Some(str::parse::( settings.sandbox.provider.as_str(), @@ -447,7 +471,7 @@ async fn run_sandbox_check( sandbox_provider: SandboxProvider, prepared: &PreparedManifest, resolved_run: &RunSettings, - github_app: Option, + github_app: Option, ) -> bool { let daytona_config = resolve_daytona_config(resolved_run); let sandbox_result: Result, String> = match sandbox_provider { @@ -679,7 +703,7 @@ async fn run_github_token_check( checks: &mut Vec, prepared: &PreparedManifest, settings: &ServerSettings, - github_app: Option, + github_app: Option, ) { if settings.integrations.github.permissions.is_empty() { return; @@ -723,19 +747,26 @@ async fn run_github_token_check( status: CheckStatus::Warning, summary: "skipped".into(), details: vec![], - remediation: Some("No GitHub App credentials or origin URL available".to_string()), + remediation: Some("No GitHub credentials or origin URL available".to_string()), }), } } async fn mint_github_token( - creds: &fabro_github::GitHubAppCredentials, + creds: &fabro_github::GitHubCredentials, origin_url: &str, permissions: &HashMap, ) -> Result { + if let fabro_github::GitHubCredentials::Token(token) = creds { + return Ok(token.clone()); + } + 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| anyhow!("{err}"))?; + let fabro_github::GitHubCredentials::App(creds) = creds else { + unreachable!("token credentials return early"); + }; let jwt = fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) .map_err(|err| anyhow!("{err}"))?; let client = reqwest::Client::new(); @@ -857,6 +888,18 @@ mod tests { } } + fn invalid_manifest() -> types::RunManifest { + types::RunManifest { + workflows: HashMap::from([("workflow.fabro".to_string(), types::ManifestWorkflow { + config: None, + files: HashMap::new(), + source: "digraph Invalid { exit [shape=Msquare] orphan exit -> orphan }" + .to_string(), + })]), + ..minimal_manifest() + } + } + fn server_settings_fixture(source: &str) -> SettingsLayer { fabro_config::parse_settings_layer(source).expect("v2 fixture should parse") } @@ -988,4 +1031,99 @@ app_id = "snapshotted-app-id" ); assert_eq!(resolved_server.storage.root.as_source(), "/srv/fabro"); } + + #[tokio::test] + async fn invalid_preflight_returns_diagnostics_without_runtime_checks() { + let state = crate::server::create_app_state(); + let prepared = + prepare_manifest_with_mode(&SettingsLayer::default(), &invalid_manifest(), false) + .unwrap(); + let validated = validate_prepared_manifest(&prepared).unwrap(); + + assert!(validated.has_errors()); + + let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); + + assert!(!ok); + assert_eq!(response.workflow.name, "Invalid"); + assert!(!response.workflow.diagnostics.is_empty()); + assert_eq!(response.checks.title, "Run Preflight"); + assert_eq!(response.checks.sections.len(), 1); + assert_eq!(response.checks.sections[0].checks.len(), 2); + } + + #[tokio::test] + async fn preflight_allows_pull_request_enabled_without_github_credentials() { + let state = crate::server::create_app_state(); + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some("/tmp/project/.fabro/project.toml".to_string()), + source: Some( + r#" +_version = 1 + +[run.pull_request] +enabled = true +"# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + + let prepared = + prepare_manifest_with_mode(&SettingsLayer::default(), &manifest, false).unwrap(); + let validated = validate_prepared_manifest(&prepared).unwrap(); + + assert!(!validated.has_errors()); + + let (response, ok) = run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); + + assert!(!ok); + assert!(response.workflow.diagnostics.is_empty()); + assert!( + response.checks.sections[0] + .checks + .iter() + .all(|check| check.name != "GitHub Token") + ); + } + + #[tokio::test] + async fn preflight_daytona_without_github_credentials_returns_report() { + let state = crate::server::create_app_state(); + let mut manifest = minimal_manifest(); + manifest.configs.push(types::ManifestConfig { + path: Some("/tmp/project/.fabro/project.toml".to_string()), + source: Some( + r#" +_version = 1 + +[run.sandbox] +provider = "daytona" +"# + .to_string(), + ), + type_: types::ManifestConfigType::Project, + }); + + let prepared = + prepare_manifest_with_mode(&SettingsLayer::default(), &manifest, false).unwrap(); + let validated = validate_prepared_manifest(&prepared).unwrap(); + + let (response, _ok) = run_preflight(state.as_ref(), &prepared, &validated) + .await + .unwrap(); + + assert!(response.workflow.diagnostics.is_empty()); + assert!( + response.checks.sections[0] + .checks + .iter() + .any(|check| check.name == "Sandbox") + ); + } } diff --git a/lib/crates/fabro-server/src/serve.rs b/lib/crates/fabro-server/src/serve.rs index 1244b759e..ec2a3c7cc 100644 --- a/lib/crates/fabro-server/src/serve.rs +++ b/lib/crates/fabro-server/src/serve.rs @@ -8,6 +8,7 @@ use fabro_config::user::{active_settings_path, load_settings_config}; use fabro_config::{Storage, resolve_server_from_file}; use fabro_llm::client::Client as LlmClient; use fabro_sandbox::SandboxProvider; +use fabro_types::settings::server::GithubIntegrationStrategy; use fabro_types::settings::{ InterpString, ObjectStoreSettings, ServerListenSettings, ServerSettings as ResolvedServerSettings, SettingsLayer, @@ -150,6 +151,11 @@ fn apply_runtime_settings( settings } +fn router_web_enabled(settings: &ResolvedServerSettings) -> bool { + settings.web.enabled + && settings.integrations.github.strategy != GithubIntegrationStrategy::GhCli +} + fn use_in_memory_store() -> bool { !matches!( std::env::var(TEST_IN_MEMORY_STORE_ENV).ok().as_deref(), @@ -315,7 +321,7 @@ where .unwrap_or(resolved_server_settings.scheduler.max_concurrent_runs); (auth_mode, client_auth, max_concurrent_runs) }; - let web_enabled = resolved_server_settings.web.enabled; + let web_enabled = router_web_enabled(&resolved_server_settings); let store_path = storage.store_dir(); let object_store = build_object_store(&store_path)?; @@ -357,52 +363,59 @@ where }; // Optionally start webhook listener - let webhook_app_id = resolved_server_settings - .integrations - .github - .webhooks - .as_ref() - .and(resolved_server_settings.integrations.github.app_id.as_ref()) - .map(resolve_interp) - .transpose()?; - let webhook_manager = match webhook_app_id { - Some(app_id) => { - let secret = secret_snapshot - .get("GITHUB_APP_WEBHOOK_SECRET") - .cloned() - .or_else(|| std::env::var("GITHUB_APP_WEBHOOK_SECRET").ok()); - let github_app = state - .github_app_credentials(Some(&app_id)) - .await - .unwrap_or_else(|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"); + let webhook_manager = match resolved_server_settings.integrations.github.strategy { + GithubIntegrationStrategy::GhCli => None, + GithubIntegrationStrategy::App => { + let webhook_app_id = resolved_server_settings + .integrations + .github + .webhooks + .as_ref() + .and(resolved_server_settings.integrations.github.app_id.as_ref()) + .map(resolve_interp) + .transpose()?; + match webhook_app_id { + Some(app_id) => { + let secret = secret_snapshot + .get("GITHUB_APP_WEBHOOK_SECRET") + .cloned() + .or_else(|| std::env::var("GITHUB_APP_WEBHOOK_SECRET").ok()); + let github_app = state + .github_credentials(&resolved_server_settings.integrations.github) + .await + .unwrap_or_else(|err| { + warn!( + error = %err, + "Webhook config present but GitHub credentials are invalid; skipping webhook listener" + ); + None + }); + if let (Some(secret), Some(fabro_github::GitHubCredentials::App(github_app))) = + (secret, github_app) + { + match WebhookManager::start( + secret.into_bytes(), + &app_id, + &github_app.private_key_pem, + ) + .await + { + Ok(manager) => Some(manager), + Err(err) => { + error!(error = %err, "Failed to start webhook listener"); + None + } + } + } else { + warn!( + "Webhook config present but GITHUB_APP_WEBHOOK_SECRET or GITHUB_APP_PRIVATE_KEY not set; skipping webhook listener" + ); None } } - } else { - warn!( - "Webhook config present but GITHUB_APP_WEBHOOK_SECRET or GITHUB_APP_PRIVATE_KEY not set; skipping webhook listener" - ); - None + None => None, } } - None => None, }; let (shutdown_tx, shutdown_rx) = watch::channel(false); @@ -695,7 +708,8 @@ mod tests { use super::{ ServeArgs, ServerTitlePhase, apply_runtime_settings, bind_tcp_host_with_fallback, - build_object_store_with_preference, server_bind_title, server_title, + build_object_store_with_preference, resolve_server_settings, router_web_enabled, + server_bind_title, server_title, }; use crate::bind::Bind; @@ -791,6 +805,25 @@ enabled = false ); } + #[test] + fn gh_cli_strategy_forces_web_disabled_in_router_options() { + let base = parse_settings( + r#" +_version = 1 + +[server.web] +enabled = true + +[server.integrations.github] +strategy = "gh_cli" +"#, + ); + + let resolved = resolve_server_settings(&base).expect("settings should resolve"); + + assert!(!router_web_enabled(&resolved)); + } + #[test] fn server_title_formats_boot_listening_and_stopping() { let bind = Bind::Tcp("127.0.0.1:3000".parse().unwrap()); diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 5e24cc52b..a1f32746d 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -62,6 +62,7 @@ use fabro_store::{ ArtifactStore, Database, EventEnvelope, EventPayload, PendingInterviewRecord, StageId, }; use fabro_types::settings::run::RunMode; +use fabro_types::settings::server::{GithubIntegrationSettings, GithubIntegrationStrategy}; use fabro_types::settings::{ InterpString, ServerSettings as ResolvedServerSettings, SettingsLayer, }; @@ -624,28 +625,51 @@ impl AppState { .map(|value| Key::derive_from(value.as_bytes())) } - pub(crate) async fn github_app_credentials( + pub(crate) async fn github_credentials( &self, - app_id: Option<&str>, - ) -> Result, String> { - let Some(app_id) = app_id else { - return Ok(None); - }; - let raw = self - .secret_store - .read() - .await - .get("GITHUB_APP_PRIVATE_KEY") - .map(str::to_string) - .or_else(|| std::env::var("GITHUB_APP_PRIVATE_KEY").ok()); - let Some(raw) = raw else { - return Ok(None); - }; - let private_key_pem = decode_secret_pem("GITHUB_APP_PRIVATE_KEY", &raw)?; - Ok(Some(fabro_github::GitHubAppCredentials { - app_id: app_id.to_string(), - private_key_pem, - })) + settings: &GithubIntegrationSettings, + ) -> Result, String> { + match settings.strategy { + GithubIntegrationStrategy::App => { + let Some(app_id) = settings.app_id.as_ref().map(InterpString::as_source) else { + return Ok(None); + }; + let raw = self + .secret_store + .read() + .await + .get("GITHUB_APP_PRIVATE_KEY") + .map(str::to_string) + .or_else(|| std::env::var("GITHUB_APP_PRIVATE_KEY").ok()); + let Some(raw) = raw else { + return Ok(None); + }; + let private_key_pem = decode_secret_pem("GITHUB_APP_PRIVATE_KEY", &raw)?; + Ok(Some(fabro_github::GitHubCredentials::App( + fabro_github::GitHubAppCredentials { + app_id, + private_key_pem, + }, + ))) + } + GithubIntegrationStrategy::GhCli => { + let token = self + .secret_store + .read() + .await + .get("GITHUB_CLI_TOKEN") + .map(str::trim) + .filter(|token| !token.is_empty()) + .map(str::to_string); + match token { + Some(token) => Ok(Some(fabro_github::GitHubCredentials::Token(token))), + None => Err( + "gh_cli strategy requires a stored token — run fabro install on the server host or manually provision GITHUB_CLI_TOKEN" + .to_string(), + ), + } + } + } } fn issue_artifact_upload_token(&self, run_id: &RunId) -> Result { @@ -1490,15 +1514,10 @@ fn resolved_storage_dir(settings: &SettingsLayer) -> Result { }) } -fn resolved_github_app_id(settings: &SettingsLayer) -> Result, String> { +fn resolved_github_settings(settings: &SettingsLayer) -> Result { let resolved = resolve_server_from_file(settings).map_err(|errors| render_resolve_errors(&errors))?; - Ok(resolved - .integrations - .github - .app_id - .as_ref() - .map(InterpString::as_source)) + Ok(resolved.integrations.github) } fn parse_system_duration(raw: &str) -> anyhow::Result { @@ -1687,92 +1706,117 @@ async fn get_github_repo( Path((owner, name)): Path<(String, String)>, ) -> Response { let settings = state.server_settings(); - let Some(app_id) = settings.integrations.github.app_id.as_ref() else { - return ApiError::new( - StatusCode::SERVICE_UNAVAILABLE, - "server.integrations.github.app_id is not configured", - ) - .into_response(); - }; - let app_id = match resolve_interp_string(app_id) { - Ok(app_id) => app_id, - Err(err) => { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()).into_response(); - } - }; - - let creds = match state.github_app_credentials(Some(&app_id)).await { - Ok(Some(creds)) => creds, - Ok(None) => { - return ApiError::new( - StatusCode::SERVICE_UNAVAILABLE, - "GITHUB_APP_PRIVATE_KEY is not configured", - ) - .into_response(); - } - Err(err) => { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); - } - }; - - let jwt = match fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) { - Ok(jwt) => jwt, - Err(err) => { - return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); - } - }; - + let github_settings = &settings.integrations.github; let base_url = fabro_github::github_api_base_url(); - let client = reqwest::Client::new(); - let install_url = match settings.integrations.github.slug.as_ref() { - Some(slug) => match resolve_interp_string(slug) { - Ok(slug) => format!("https://github.com/apps/{slug}/installations/new"), - Err(err) => { + let mut client = None; + let token = match github_settings.strategy { + GithubIntegrationStrategy::App => { + let Some(app_id) = github_settings.app_id.as_ref() else { + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "server.integrations.github.app_id is not configured", + ) + .into_response(); + }; + if let Err(err) = resolve_interp_string(app_id) { return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) .into_response(); } - }, - None => format!("https://github.com/organizations/{owner}/settings/installations"), - }; + let creds = match state.github_credentials(github_settings).await { + Ok(Some(fabro_github::GitHubCredentials::App(creds))) => creds, + Ok(Some(_)) => unreachable!("app strategy should not return token credentials"), + Ok(None) => { + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "GITHUB_APP_PRIVATE_KEY is not configured", + ) + .into_response(); + } + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + } + }; - let installed = - match fabro_github::check_app_installed(&client, &jwt, &owner, &name, &base_url).await { - Ok(installed) => installed, - Err(err) => { - return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); + let jwt = match fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) { + Ok(jwt) => jwt, + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + } + }; + let install_url = match github_settings.slug.as_ref() { + Some(slug) => match resolve_interp_string(slug) { + Ok(slug) => format!("https://github.com/apps/{slug}/installations/new"), + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.to_string()) + .into_response(); + } + }, + None => format!("https://github.com/organizations/{owner}/settings/installations"), + }; + + let client_ref = client.get_or_insert_with(reqwest::Client::new); + let installed = match fabro_github::check_app_installed( + &*client_ref, + &jwt, + &owner, + &name, + &base_url, + ) + .await + { + Ok(installed) => installed, + Err(err) => { + return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(); + } + }; + + if !installed { + return ( + StatusCode::OK, + Json(serde_json::json!({ + "owner": owner, + "name": name, + "accessible": false, + "default_branch": null, + "private": null, + "permissions": null, + "install_url": install_url, + })), + ) + .into_response(); } - }; - if !installed { - return ( - StatusCode::OK, - Json(serde_json::json!({ - "owner": owner, - "name": name, - "accessible": false, - "default_branch": null, - "private": null, - "permissions": null, - "install_url": install_url, - })), - ) - .into_response(); - } - - let token = match fabro_github::create_installation_access_token_with_permissions( - &client, - &jwt, - &owner, - &name, - &base_url, - serde_json::json!({ "contents": "write", "pull_requests": "write" }), - ) - .await - { - Ok(token) => token, - Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(), + match fabro_github::create_installation_access_token_with_permissions( + &*client_ref, + &jwt, + &owner, + &name, + &base_url, + serde_json::json!({ "contents": "write", "pull_requests": "write" }), + ) + .await + { + Ok(token) => token, + Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(), + } + } + GithubIntegrationStrategy::GhCli => match state.github_credentials(github_settings).await { + Ok(Some(fabro_github::GitHubCredentials::Token(token))) => token, + Ok(Some(_)) => unreachable!("gh_cli strategy should not return app credentials"), + Ok(None) => { + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "GITHUB_CLI_TOKEN is not configured", + ) + .into_response(); + } + Err(err) => { + return ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err).into_response(); + } + }, }; + let client = client.unwrap_or_else(reqwest::Client::new); let repo_response = match client .get(format!("{base_url}/repos/{owner}/{name}")) .header("Authorization", format!("Bearer {token}")) @@ -1782,6 +1826,37 @@ async fn get_github_repo( .await { Ok(response) if response.status().is_success() => response, + Ok(response) + if github_settings.strategy == GithubIntegrationStrategy::GhCli + && matches!( + response.status(), + reqwest::StatusCode::FORBIDDEN | reqwest::StatusCode::NOT_FOUND + ) => + { + return ( + StatusCode::OK, + Json(serde_json::json!({ + "owner": owner, + "name": name, + "accessible": false, + "default_branch": null, + "private": null, + "permissions": null, + "install_url": serde_json::Value::Null, + })), + ) + .into_response(); + } + Ok(response) + if github_settings.strategy == GithubIntegrationStrategy::GhCli + && response.status() == reqwest::StatusCode::UNAUTHORIZED => + { + return ApiError::new( + StatusCode::SERVICE_UNAVAILABLE, + "Stored GitHub CLI token is invalid — run fabro install or update GITHUB_CLI_TOKEN", + ) + .into_response(); + } Ok(response) => { return ApiError::new( StatusCode::BAD_GATEWAY, @@ -3692,28 +3767,56 @@ async fn execute_run_in_process(state: Arc, run_id: RunId) { return; } }; - let github_app_id = match resolved_github_app_id(&persisted.run_record().settings) { - Ok(app_id) => app_id, + let github_settings = match resolved_github_settings(&persisted.run_record().settings) { + Ok(settings) => settings, Err(err) => { - tracing::error!(run_id = %run_id, error = %err, "Invalid GitHub App config"); + tracing::error!(run_id = %run_id, error = %err, "Invalid GitHub integration config"); 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 config: {err}")); + managed_run.error = Some(format!("Invalid GitHub integration config: {err}")); clear_live_run_state(managed_run); } state.scheduler_notify.notify_one(); return; } }; - let github_app = match state.github_app_credentials(github_app_id.as_deref()).await { + let github_app_result = match fabro_config::resolve_run_from_file( + &persisted.run_record().settings, + ) { + Ok(settings) => { + let required_github_credentials = (settings.execution.mode != RunMode::DryRun + && settings.sandbox.provider == "daytona") + || !github_settings.permissions.is_empty(); + if required_github_credentials { + state.github_credentials(&github_settings).await + } else if settings.execution.mode != RunMode::DryRun && settings.pull_request.is_some() + { + match state.github_credentials(&github_settings).await { + Ok(github_app) => Ok(github_app), + Err(err) => { + tracing::warn!( + run_id = %run_id, + error = %err, + "GitHub credentials unavailable; pull request creation will be skipped" + ); + Ok(None) + } + } + } else { + Ok(None) + } + } + Err(_) => Ok(None), + }; + let github_app = match github_app_result { Ok(github_app) => github_app, Err(e) => { - tracing::error!(run_id = %run_id, error = %e, "Invalid GitHub App credentials"); + tracing::error!(run_id = %run_id, error = %e, "Invalid GitHub 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}")); + managed_run.error = Some(format!("Invalid GitHub credentials: {e}")); clear_live_run_state(managed_run); } state.scheduler_notify.notify_one(); diff --git a/lib/crates/fabro-tracker/src/github.rs b/lib/crates/fabro-tracker/src/github.rs index 6ac1aa627..adba5995f 100644 --- a/lib/crates/fabro-tracker/src/github.rs +++ b/lib/crates/fabro-tracker/src/github.rs @@ -1,6 +1,6 @@ use async_trait::async_trait; use fabro_github::{ - GitHubAppCredentials, create_installation_access_token_for_projects, sign_app_jwt, + GitHubCredentials, create_installation_access_token_for_projects, sign_app_jwt, }; use tokio::sync::OnceCell; @@ -29,7 +29,7 @@ async fn execute_github_graphql( /// /// Scoped to a single project board identified by `project_number`. pub struct GitHubTracker { - creds: GitHubAppCredentials, + creds: GitHubCredentials, client: reqwest::Client, owner: String, repo: String, @@ -40,7 +40,7 @@ pub struct GitHubTracker { impl GitHubTracker { pub fn new( - creds: GitHubAppCredentials, + creds: GitHubCredentials, client: reqwest::Client, owner: String, repo: String, @@ -63,15 +63,20 @@ impl GitHubTracker { } async fn fresh_token(&self) -> Result { - let jwt = sign_app_jwt(&self.creds.app_id, &self.creds.private_key_pem)?; - create_installation_access_token_for_projects( - &self.client, - &jwt, - &self.owner, - &self.repo, - &self.base_url, - ) - .await + match &self.creds { + GitHubCredentials::App(creds) => { + let jwt = sign_app_jwt(&creds.app_id, &creds.private_key_pem)?; + create_installation_access_token_for_projects( + &self.client, + &jwt, + &self.owner, + &self.repo, + &self.base_url, + ) + .await + } + GitHubCredentials::Token(token) => Ok(token.clone()), + } } async fn resolve_project_node_id(&self, token: &str) -> Result<&str, String> { @@ -493,7 +498,7 @@ impl Tracker for GitHubTracker { #[cfg(test)] mod tests { - use fabro_github::GitHubAppCredentials; + use fabro_github::{GitHubAppCredentials, GitHubCredentials}; use super::*; use crate::Issue; @@ -626,10 +631,10 @@ mod tests { fn mock_github_tracker(server_url: &str, pem: String) -> GitHubTracker { GitHubTracker::new( - GitHubAppCredentials { + GitHubCredentials::App(GitHubAppCredentials { app_id: "test-app".to_string(), private_key_pem: pem, - }, + }), test_http_client(), "owner".to_string(), "repo".to_string(), diff --git a/lib/crates/fabro-types/src/settings/server.rs b/lib/crates/fabro-types/src/settings/server.rs index 716e93403..44958565a 100644 --- a/lib/crates/fabro-types/src/settings/server.rs +++ b/lib/crates/fabro-types/src/settings/server.rs @@ -215,6 +215,7 @@ pub struct ServerIntegrationsSettings { #[derive(Debug, Clone, Default, PartialEq, Eq)] pub struct GithubIntegrationSettings { pub enabled: bool, + pub strategy: GithubIntegrationStrategy, pub app_id: Option, pub client_id: Option, pub slug: Option, @@ -506,6 +507,8 @@ pub struct GithubIntegrationLayer { #[serde(default, skip_serializing_if = "Option::is_none")] pub enabled: Option, #[serde(default, skip_serializing_if = "Option::is_none")] + pub strategy: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] pub app_id: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub client_id: Option, @@ -550,6 +553,14 @@ pub struct IntegrationWebhooksLayer { pub strategy: Option, } +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum GithubIntegrationStrategy { + #[default] + GhCli, + App, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum WebhookStrategy { diff --git a/lib/crates/fabro-types/tests/server_settings_serde.rs b/lib/crates/fabro-types/tests/server_settings_serde.rs new file mode 100644 index 000000000..dc40eb272 --- /dev/null +++ b/lib/crates/fabro-types/tests/server_settings_serde.rs @@ -0,0 +1,23 @@ +use fabro_types::settings::SettingsLayer; +use serde_json::json; + +#[test] +fn settings_layer_round_trips_github_integration_strategy() { + let source = json!({ + "_version": 1, + "server": { + "integrations": { + "github": { + "strategy": "gh_cli", + "app_id": "{{ env.GITHUB_APP_ID }}" + } + } + } + }); + + let settings: SettingsLayer = + serde_json::from_value(source.clone()).expect("settings should deserialize"); + let round_trip = serde_json::to_value(&settings).expect("settings should serialize"); + + assert_eq!(round_trip, source); +} diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index f251458e8..7dcb9f984 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -64,13 +64,13 @@ struct RunSession { event_sink: RunEventSink, artifact_sink: Option, git: Option, - github_app: Option, + github_app: Option, worktree_mode: Option, registry_override: Option>, retro_enabled: bool, preserve_sandbox: bool, pr_config: Option, - pr_github_app: Option, + pr_github_app: Option, pr_origin_url: Option, pr_model: String, workflow_path: Option, @@ -87,7 +87,7 @@ pub struct StartServices { pub event_sink: RunEventSink, pub artifact_sink: Option, pub run_control: Option>, - pub github_app: Option, + pub github_app: Option, pub on_node: crate::OnNodeCallback, pub registry_override: Option>, } diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 5c6844ddd..0562ecf94 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -202,13 +202,20 @@ async fn resolve_worktree_plan(options: &mut InitOptions) -> Result, ) -> Result { + if let fabro_github::GitHubCredentials::Token(token) = creds { + return Ok(token.clone()); + } + let https_url = fabro_github::ssh_url_to_https(origin_url); let (owner, repo) = fabro_github::parse_github_owner_repo(&https_url).map_err(|e| Error::engine(e.clone()))?; + let fabro_github::GitHubCredentials::App(creds) = creds else { + unreachable!("token credentials return early"); + }; let jwt = fabro_github::sign_app_jwt(&creds.app_id, &creds.private_key_pem) .map_err(|e| Error::engine(e.clone()))?; let client = reqwest::Client::new(); @@ -227,7 +234,7 @@ async fn mint_github_token( async fn build_sandbox_env( spec: &SandboxEnvSpec, - github_app: Option<&fabro_github::GitHubAppCredentials>, + github_app: Option<&fabro_github::GitHubCredentials>, emitter: &Emitter, ) -> Result, Error> { let mut env = spec.devcontainer_env.clone(); diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index b7521540a..4d46972c9 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1,4 +1,4 @@ -use fabro_github::{self as github_app, GitHubAppCredentials, ssh_url_to_https}; +use fabro_github::{self as github_app, GitHubCredentials, ssh_url_to_https}; use fabro_graphviz::parser; use fabro_llm::generate::{GenerateParams, generate}; use fabro_retro::retro::Retro; @@ -400,7 +400,7 @@ pub struct AutoMergeOptions { /// the diff was empty, or `Err` on failure. #[allow(clippy::too_many_arguments)] pub async fn maybe_open_pull_request( - creds: &GitHubAppCredentials, + creds: &GitHubCredentials, origin_url: &str, base_branch: &str, head_branch: &str, @@ -1338,10 +1338,10 @@ mod tests { async fn empty_diff_returns_none() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let creds = GitHubAppCredentials { + let creds = GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id: "123".to_string(), private_key_pem: "unused".to_string(), - }; + }); let result = maybe_open_pull_request( &creds, "https://github.com/owner/repo.git", diff --git a/lib/crates/fabro-workflow/src/pipeline/types.rs b/lib/crates/fabro-workflow/src/pipeline/types.rs index e75acd69d..ff87e0e20 100644 --- a/lib/crates/fabro-workflow/src/pipeline/types.rs +++ b/lib/crates/fabro-workflow/src/pipeline/types.rs @@ -375,7 +375,7 @@ pub struct PullRequestOptions { pub run_dir: PathBuf, pub run_store: RunStoreHandle, pub pr_config: Option, - pub github_app: Option, + pub github_app: Option, pub origin_url: Option, pub model: String, } diff --git a/lib/crates/fabro-workflow/src/run_options.rs b/lib/crates/fabro-workflow/src/run_options.rs index 8c9c14b6d..66beb3518 100644 --- a/lib/crates/fabro-workflow/src/run_options.rs +++ b/lib/crates/fabro-workflow/src/run_options.rs @@ -29,8 +29,8 @@ pub struct RunOptions { pub labels: HashMap, /// Workflow directory slug (e.g. "smoke" from `.fabro/workflows/smoke/`). pub workflow_slug: Option, - /// GitHub App credentials for pushing metadata branches to origin. - pub github_app: Option, + /// GitHub credentials for pushing metadata branches to origin. + pub github_app: Option, /// Host repo path for MetadataStore (shadow commits) and host-side pushes. pub host_repo_path: Option, /// Name of the branch the run was started from (for PR base). diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 468c839e0..31b1071a9 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -129,12 +129,12 @@ pub async fn git_checkpoint( /// Push a refspec from the host repo to origin (best-effort). /// -/// Authenticates via a GitHub App installation token so we don't depend -/// on the host's ambient git credentials. +/// Authenticates via resolved GitHub credentials so we don't depend on the +/// host's ambient git credentials. pub async fn git_push_host( repo_path: &Path, refspec: &str, - github_app: &Option, + github_app: &Option, label: &str, ) -> bool { let (origin_url, _) = match detect_repo_info(repo_path) { @@ -161,7 +161,7 @@ pub async fn git_push_host( } } } else { - tracing::warn!(label, "No GitHub App credentials for push"); + tracing::warn!(label, "No GitHub credentials for push"); return false; }; diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 07dadfa77..0efbd7910 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -156,14 +156,14 @@ fn test_artifact_store(run_dir: &Path) -> ArtifactStore { } async fn create_env_with_github_app( - github_app: Option, + github_app: Option, ) -> DaytonaSandbox { DaytonaSandbox::new(DaytonaConfig::default(), github_app, None, None) .await .expect("Failed to create Daytona client — is DAYTONA_API_KEY set?") } -fn load_github_app_credentials() -> fabro_github::GitHubAppCredentials { +fn load_github_app_credentials() -> fabro_github::GitHubCredentials { // Read app_id from ~/.fabro/settings.toml let home = dirs::home_dir().expect("No home directory"); let config_path = home.join(".fabro/settings.toml"); @@ -194,10 +194,10 @@ fn load_github_app_credentials() -> fabro_github::GitHubAppCredentials { .expect("GITHUB_APP_PRIVATE_KEY is not valid base64"); String::from_utf8(bytes).expect("GITHUB_APP_PRIVATE_KEY decoded to invalid UTF-8") }; - fabro_github::GitHubAppCredentials { + fabro_github::GitHubCredentials::App(fabro_github::GitHubAppCredentials { app_id, private_key_pem, - } + }) } #[fabro_macros::e2e_test(live("DAYTONA_API_KEY"), live("GITHUB_APP_PRIVATE_KEY"))]