From 12ca595f5486b31786d668ab5830b01e637f233f Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 09:15:25 -0400 Subject: [PATCH] refactor(github): take GitHubContext by reference in public API Public functions now take ctx: &GitHubContext<'_> instead of by-value GitHubContext<'_>. Matches the surrounding &str / &GitHubCredentials convention. The type stays Copy so internal call sites that pass `ctx` through still work without explicit reborrows. Touched: 8 fabro-github functions + matching _with_client variants, plus call sites in fabro-server, fabro-workflow, fabro-sandbox, and fabro-github's integration + unit tests. Pure mechanical change. Verified: workspace fmt clean, clippy --all-targets -D warnings clean, cargo nextest run --workspace 4581 passed, 182 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-github/src/lib.rs | 50 +++++++++---------- lib/crates/fabro-github/tests/integration.rs | 12 ++--- lib/crates/fabro-sandbox/src/daytona/mod.rs | 4 +- lib/crates/fabro-server/src/server.rs | 6 +-- .../src/pipeline/pull_request.rs | 4 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 2 +- .../tests/it/daytona_integration.rs | 4 +- 7 files changed, 41 insertions(+), 41 deletions(-) diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index 9629e3b5d..bc9f8f182 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -471,7 +471,7 @@ pub struct CreatedPullRequest { reason = "Creating a pull request needs explicit repo, branch, and body fields." )] pub async fn create_pull_request( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, base: &str, @@ -568,7 +568,7 @@ fn merge_method_as_graphql_value(method: fabro_types::MergeMethod) -> &'static s /// 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( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, pr_node_id: &str, @@ -695,7 +695,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( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, branch: &str, @@ -706,7 +706,7 @@ pub async fn branch_exists( async fn branch_exists_with_client( client: &impl HttpClient, - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, branch: &str, @@ -848,7 +848,7 @@ pub async fn update_app_webhook_config( /// Always generates a token regardless of repo visibility, since the token /// is needed for pushing from the sandbox. pub async fn resolve_clone_credentials( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, ) -> Result<(Option, Option), String> { @@ -883,7 +883,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( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, url: &str, ) -> Result { let (owner, repo) = parse_github_owner_repo(url)?; @@ -896,7 +896,7 @@ pub async fn resolve_authenticated_url( /// Fetch detailed information about a pull request. pub async fn get_pull_request( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -907,7 +907,7 @@ pub async fn get_pull_request( async fn get_pull_request_with_client( client: &impl HttpClient, - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -963,7 +963,7 @@ async fn get_pull_request_with_client( /// Merge a pull request. pub async fn merge_pull_request( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -975,7 +975,7 @@ pub async fn merge_pull_request( async fn merge_pull_request_with_client( client: &impl HttpClient, - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -1029,7 +1029,7 @@ async fn merge_pull_request_with_client( /// Close a pull request. pub async fn close_pull_request( - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -1040,7 +1040,7 @@ pub async fn close_pull_request( async fn close_pull_request_with_client( client: &impl HttpClient, - ctx: GitHubContext<'_>, + ctx: &GitHubContext<'_>, owner: &str, repo: &str, number: u64, @@ -1549,7 +1549,7 @@ mod tests { }); let result = branch_exists_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", "my-branch", @@ -1587,7 +1587,7 @@ mod tests { }); let result = branch_exists_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", "no-such-branch", @@ -1625,7 +1625,7 @@ mod tests { }); let result = branch_exists_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", "broken", @@ -1648,7 +1648,7 @@ mod tests { let creds = GitHubCredentials::Token("ghu_test".to_string()); let result = branch_exists_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", "my-branch", @@ -1786,7 +1786,7 @@ mod tests { }); let detail = get_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 42, @@ -1831,7 +1831,7 @@ mod tests { }); let err = get_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 999, @@ -1865,7 +1865,7 @@ mod tests { let creds = GitHubCredentials::Token("ghu_test".to_string()); let detail = get_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 42, @@ -1881,7 +1881,7 @@ mod tests { let creds = GitHubCredentials::Token("ghu_test".to_string()); let credentials = - resolve_clone_credentials(GitHubContext::new(&creds, ""), "owner", "repo") + resolve_clone_credentials(&GitHubContext::new(&creds, ""), "owner", "repo") .await .unwrap(); @@ -1927,7 +1927,7 @@ mod tests { }); merge_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 42, @@ -1961,7 +1961,7 @@ mod tests { }); let err = merge_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 42, @@ -1996,7 +1996,7 @@ mod tests { }); let err = merge_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 42, @@ -2038,7 +2038,7 @@ mod tests { app_id: "test".to_string(), private_key_pem: pem.to_string(), }); - close_pull_request_with_client(&mock, GitHubContext::new(&creds, ""), "owner", "repo", 42) + close_pull_request_with_client(&mock, &GitHubContext::new(&creds, ""), "owner", "repo", 42) .await .unwrap(); } @@ -2067,7 +2067,7 @@ mod tests { }); let err = close_pull_request_with_client( &mock, - GitHubContext::new(&creds, ""), + &GitHubContext::new(&creds, ""), "owner", "repo", 999, diff --git a/lib/crates/fabro-github/tests/integration.rs b/lib/crates/fabro-github/tests/integration.rs index e32221b6c..ea9ebcb5c 100644 --- a/lib/crates/fabro-github/tests/integration.rs +++ b/lib/crates/fabro-github/tests/integration.rs @@ -39,7 +39,7 @@ fn standard_app_state() -> GitHubAppState { async fn create_and_get_pull_request() { let twin = TwinGitHub::start(standard_app_state()).await; let creds = github_credentials(); - let ctx = GitHubContext::new(&creds, &twin.base_url); + let ctx = &GitHubContext::new(&creds, &twin.base_url); let created = create_pull_request( ctx, @@ -70,7 +70,7 @@ async fn create_and_get_pull_request() { async fn create_merge_and_verify_state() { let twin = TwinGitHub::start(standard_app_state()).await; let creds = github_credentials(); - let ctx = GitHubContext::new(&creds, &twin.base_url); + let ctx = &GitHubContext::new(&creds, &twin.base_url); let created = create_pull_request( ctx, "acme", "widgets", "main", "feature", "Merge me", "PR body", false, @@ -97,7 +97,7 @@ async fn create_close_and_verify_state() { let twin = TwinGitHub::start(standard_app_state()).await; let creds = github_credentials(); - let ctx = GitHubContext::new(&creds, &twin.base_url); + let ctx = &GitHubContext::new(&creds, &twin.base_url); let created = create_pull_request( ctx, "acme", "widgets", "main", "feature", "Close me", "PR body", false, @@ -123,7 +123,7 @@ async fn enable_auto_merge_persists() { let twin = TwinGitHub::start(standard_app_state()).await; let creds = github_credentials(); - let ctx = GitHubContext::new(&creds, &twin.base_url); + let ctx = &GitHubContext::new(&creds, &twin.base_url); let created = create_pull_request( ctx, @@ -187,7 +187,7 @@ async fn resolve_authenticated_url_embeds_token() { let creds = github_credentials(); let url = resolve_authenticated_url( - GitHubContext::new(&creds, &twin.base_url), + &GitHubContext::new(&creds, &twin.base_url), "https://github.com/acme/widgets.git", ) .await @@ -205,7 +205,7 @@ async fn resolve_authenticated_url_errors_on_non_github_url() { let creds = github_credentials(); let error = resolve_authenticated_url( - GitHubContext::new(&creds, &twin.base_url), + &GitHubContext::new(&creds, &twin.base_url), "https://gitlab.com/foo/bar", ) .await diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index acab9a7ae..7ad99a52a 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -534,7 +534,7 @@ impl Sandbox for DaytonaSandbox { err })?; fabro_github::resolve_clone_credentials( - fabro_github::GitHubContext::new( + &fabro_github::GitHubContext::new( creds, &fabro_github::github_api_base_url(), ), @@ -816,7 +816,7 @@ impl Sandbox for DaytonaSandbox { }; let auth_url = fabro_github::resolve_authenticated_url( - fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), + &fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), origin_url, ) .await diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index 605278930..73ee641a1 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5438,7 +5438,7 @@ async fn get_run_pull_request( }; match fabro_github::get_pull_request( - fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), + &fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), &ctx.owner, &ctx.repo, ctx.record.number, @@ -5468,7 +5468,7 @@ async fn merge_run_pull_request( }; match fabro_github::merge_pull_request( - fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), + &fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), &ctx.owner, &ctx.repo, ctx.record.number, @@ -5500,7 +5500,7 @@ async fn close_run_pull_request( }; match fabro_github::close_pull_request( - fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), + &fabro_github::GitHubContext::new(&ctx.creds, state.github_api_base_url.as_str()), &ctx.owner, &ctx.repo, ctx.record.number, diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index b98b3ec76..36d35a93d 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -467,7 +467,7 @@ pub async fn maybe_open_pull_request( let title = pr_title_from_goal(req.goal); let created = github_app::create_pull_request( - req.github, + &req.github, &owner, &repo, req.base_branch, @@ -487,7 +487,7 @@ pub async fn maybe_open_pull_request( MergeStrategy::Rebase => fabro_types::MergeMethod::Rebase, }; match github_app::enable_auto_merge( - req.github, + &req.github, &owner, &repo, &created.node_id, diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 765d5b294..3a952a7bb 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -151,7 +151,7 @@ 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( - fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), + &fabro_github::GitHubContext::new(creds, &fabro_github::github_api_base_url()), &https_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 9c6cdfaa0..55c796d7a 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -1493,7 +1493,7 @@ 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( - fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), + &fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), "fabro-sh", "fabro", ) @@ -1518,7 +1518,7 @@ async fn daytona_iat_not_installed_gives_clear_error() { let creds = load_github_app_credentials(); let result = fabro_github::resolve_clone_credentials( - fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), + &fabro_github::GitHubContext::new(&creds, &fabro_github::github_api_base_url()), "torvalds", "linux", )