scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-10-08 03:10:26 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 5

refactor: extend GitHubContext to remaining callers; add OpenPullRequestRequest::from_run_state

Browse source 
Two cleanups:

1. Threaded GitHubContext through the remaining fabro-github functions
   that pair credentials with the API base URL: branch_exists,
   resolve_clone_credentials, resolve_authenticated_url. Each loses its
   trailing `base_url: &str` and replaces `creds: &GitHubCredentials`
   with `ctx: GitHubContext<'_>`. is_app_public was skipped — it doesn't
   take credentials. Updated production callers in fabro-sandbox/daytona
   and fabro-workflow/sandbox_git, plus integration and unit tests.

2. Added OpenPullRequestRequest::from_run_state on the workflow struct.
   Bundles the validated unpacked-from-RunState pieces into a draft PR
   request with the server's defaults (`draft = true`, `auto_merge =
   None`). Server's create_run_pull_request handler now calls the
   constructor instead of inlining a 12-field struct literal — the
   handler reads as a sequence of validations followed by one named
   request build, not as plumbing.

Verified: workspace fmt clean, clippy --all-targets -D warnings clean,
cargo nextest run --workspace 4587 passed, 182 skipped.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-24 08:31:14 -04:00
parent 176c0c0159
commit 60e2027ac3
No known key found for this signature in database
7 changed files with 124 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

75
lib/crates/fabro-github/src/lib.rs
View file

@ -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<bool, String> {
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<bool, String> {
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<String>, Option<String>), 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<String, String> {
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,

12
lib/crates/fabro-github/tests/integration.rs
View file

@ -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"));

9
lib/crates/fabro-sandbox/src/daytona/mod.rs
View file

@ -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}"))?;

49
lib/crates/fabro-server/src/server.rs
View file

@ -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(),

37
lib/crates/fabro-workflow/src/pipeline/pull_request.rs
View file

@ -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

3
lib/crates/fabro-workflow/src/sandbox_git.rs
View file

@ -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
{

6
lib/crates/fabro-workflow/tests/it/daytona_integration.rs
View file

@ -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;