diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 1b266ffac..e98e16f5d 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -695,35 +695,34 @@ 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: &GitHubCredentials, + ctx: GitHubContext<'_>, owner: &str, repo: &str, branch: &str, - base_url: &str, ) -> Result { let client = http_client()?; - branch_exists_with_client(&client, creds, owner, repo, branch, base_url).await + branch_exists_with_client(&client, ctx, owner, repo, branch).await } async fn branch_exists_with_client( client: &impl HttpClient, - creds: &GitHubCredentials, + ctx: GitHubContext<'_>, owner: &str, repo: &str, branch: &str, - base_url: &str, ) -> Result { - let token = creds + let token = ctx + .creds .resolve_bearer_token( client, owner, repo, - base_url, + ctx.base_url, serde_json::json!({ "contents": "write" }), ) .await?; - let url = format!("{base_url}/repos/{owner}/{repo}/branches/{branch}"); + let url = format!("{}/repos/{owner}/{repo}/branches/{branch}", ctx.base_url); let auth = format!("Bearer {token}"); let resp = client .request(HttpMethod::Get, &url, &github_headers(&auth), None) @@ -881,21 +880,20 @@ 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: &GitHubCredentials, + ctx: GitHubContext<'_>, owner: &str, repo: &str, - base_url: &str, ) -> Result<(Option, Option), String> { - let token = match creds { + let token = match ctx.creds { GitHubCredentials::Token(token) => token.clone(), GitHubCredentials::App(_) => { let client = http_client()?; - creds + ctx.creds .resolve_bearer_token( &client, owner, repo, - base_url, + ctx.base_url, serde_json::json!({ "contents": "write" }), ) .await? @@ -917,12 +915,11 @@ 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: &GitHubCredentials, + ctx: GitHubContext<'_>, url: &str, - base_url: &str, ) -> Result { let (owner, repo) = parse_github_owner_repo(url)?; - let (_username, password) = resolve_clone_credentials(creds, &owner, &repo, base_url).await?; + let (_username, password) = resolve_clone_credentials(ctx, &owner, &repo).await?; match password { Some(token) => Ok(embed_token_in_url(url, &token)), None => Ok(url.to_string()), @@ -1601,8 +1598,14 @@ mod tests { app_id: "test".to_string(), private_key_pem: pem.to_string(), }); - let result = - branch_exists_with_client(&mock, &creds, "owner", "repo", "my-branch", "").await; + let result = branch_exists_with_client( + &mock, + GitHubContext::new(&creds, ""), + "owner", + "repo", + "my-branch", + ) + .await; assert!(result.unwrap()); } @@ -1633,8 +1636,14 @@ mod tests { 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; + let result = branch_exists_with_client( + &mock, + GitHubContext::new(&creds, ""), + "owner", + "repo", + "no-such-branch", + ) + .await; assert!(!result.unwrap()); } @@ -1665,7 +1674,14 @@ mod tests { app_id: "test".to_string(), private_key_pem: pem.to_string(), }); - let result = branch_exists_with_client(&mock, &creds, "owner", "repo", "broken", "").await; + let result = branch_exists_with_client( + &mock, + GitHubContext::new(&creds, ""), + "owner", + "repo", + "broken", + ) + .await; assert!(result.is_err()); } @@ -1681,8 +1697,14 @@ mod tests { .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; + let result = branch_exists_with_client( + &mock, + GitHubContext::new(&creds, ""), + "owner", + "repo", + "my-branch", + ) + .await; assert!(result.unwrap()); } @@ -1949,9 +1971,10 @@ mod tests { 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(); + let credentials = + resolve_clone_credentials(GitHubContext::new(&creds, ""), "owner", "repo") + .await + .unwrap(); assert_eq!( credentials, diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index e3d2a53b3..e32221b6c 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -187,9 +187,8 @@ async fn resolve_authenticated_url_embeds_token() { let creds = github_credentials(); let url = resolve_authenticated_url( - &creds, + GitHubContext::new(&creds, &twin.base_url), "https://github.com/acme/widgets.git", - &twin.base_url, ) .await .unwrap(); @@ -205,9 +204,12 @@ async fn resolve_authenticated_url_errors_on_non_github_url() { let twin = TwinGitHub::start(standard_app_state()).await; let creds = github_credentials(); - let error = resolve_authenticated_url(&creds, "https://gitlab.com/foo/bar", &twin.base_url) - .await - .unwrap_err(); + let error = resolve_authenticated_url( + GitHubContext::new(&creds, &twin.base_url), + "https://gitlab.com/foo/bar", + ) + .await + .unwrap_err(); assert!(error.contains("Not a GitHub HTTPS URL")); diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index b86094323..acab9a7ae 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -534,10 +534,12 @@ impl Sandbox for DaytonaSandbox { err })?; fabro_github::resolve_clone_credentials( - creds, + fabro_github::GitHubContext::new( + creds, + &fabro_github::github_api_base_url(), + ), &owner, &repo, - &fabro_github::github_api_base_url(), ) .await .map_err(|e| { @@ -814,9 +816,8 @@ impl Sandbox for DaytonaSandbox { }; let auth_url = fabro_github::resolve_authenticated_url( - creds, + fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), origin_url, - &fabro_github::github_api_base_url(), ) .await .map_err(|e| format!("Failed to refresh GitHub App token: {e}"))?; diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index b3ce4ec63..dd3769d78 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5381,32 +5381,29 @@ async fn create_run_pull_request( .clone() }; - let pull_request = - match pull_request::maybe_open_pull_request(pull_request::OpenPullRequestRequest { - github: fabro_github::GitHubContext::new(&creds, state.github_api_base_url.as_str()), - origin_url: &normalized_origin, - base_branch, - head_branch: run_branch, - goal: run_spec.graph.goal(), - diff, - model: &model, - draft: true, - auto_merge: None, - run_store: &run_store.clone().into(), - conclusion: Some(conclusion), - }) - .await - { - Ok(Some(record)) => record, - Ok(None) => { - return ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - "Pull request creation returned no record unexpectedly.", - ) - .into_response(); - } - Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(), - }; + let run_store_handle = run_store.clone().into(); + let request = pull_request::OpenPullRequestRequest::from_run_state( + fabro_github::GitHubContext::new(&creds, state.github_api_base_url.as_str()), + run_spec, + run_branch, + &normalized_origin, + base_branch, + diff, + &model, + &run_store_handle, + conclusion, + ); + let pull_request = match pull_request::maybe_open_pull_request(request).await { + Ok(Some(record)) => record, + Ok(None) => { + return ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "Pull request creation returned no record unexpectedly.", + ) + .into_response(); + } + Err(err) => return ApiError::new(StatusCode::BAD_GATEWAY, err).into_response(), + }; let event = workflow_event::Event::PullRequestCreated { pr_url: pull_request.html_url.clone(), diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 2c6e95968..b98b3ec76 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -409,6 +409,43 @@ pub struct OpenPullRequestRequest<'a> { pub conclusion: Option<&'a Conclusion>, } +impl<'a> OpenPullRequestRequest<'a> { + /// Build a draft PR request from validated run state. Defaults + /// `draft = true` and `auto_merge = None` — the shape the + /// `POST /runs/{id}/pull_request` server endpoint always uses. + /// The workflow pipeline path constructs the struct directly when + /// it needs non-default flags. + #[allow( + clippy::too_many_arguments, + reason = "fields are validated upstream and named at the call site for clarity" + )] + pub fn from_run_state( + github: github_app::GitHubContext<'a>, + run_spec: &'a RunSpec, + head_branch: &'a str, + normalized_origin: &'a str, + base_branch: &'a str, + diff: &'a str, + model: &'a str, + run_store: &'a RunStoreHandle, + conclusion: &'a Conclusion, + ) -> Self { + Self { + github, + origin_url: normalized_origin, + base_branch, + head_branch, + goal: run_spec.graph.goal(), + diff, + model, + draft: true, + auto_merge: None, + run_store, + conclusion: Some(conclusion), + } + } +} + /// Optionally open a pull request after a successful workflow run. /// /// Returns `Ok(Some(PullRequestRecord))` if a PR was created, `Ok(None)` if diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 453ed4883..765d5b294 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -151,9 +151,8 @@ pub async fn git_push_host( let https_url = fabro_github::ssh_url_to_https(&origin_url); let push_url = if let Some(creds) = github_app { match fabro_github::resolve_authenticated_url( - creds, + fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), &https_url, - &fabro_github::github_api_base_url(), ) .await { diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 66a4638e0..9c6cdfaa0 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -1493,10 +1493,9 @@ async fn daytona_clone_public_repo_gets_credentials() { // Directly test resolve_clone_credentials against a repo in an org where the // app is installed let (username, password) = fabro_github::resolve_clone_credentials( - &creds, + fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), "fabro-sh", "fabro", - &fabro_github::github_api_base_url(), ) .await .unwrap(); @@ -1519,10 +1518,9 @@ async fn daytona_iat_not_installed_gives_clear_error() { let creds = load_github_app_credentials(); let result = fabro_github::resolve_clone_credentials( - &creds, + fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), "torvalds", "linux", - &fabro_github::github_api_base_url(), ) .await;