From 178178879823ca5f44c234b7bdcee1ec5f4fafcd Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 08:39:58 -0400 Subject: [PATCH] refactor: extract RunPrInputs validation; drop dead is_app_public Two cleanups: 1. Extracted the 8 sequential let-Some-else-return validations from create_run_pull_request into a server-local RunPrInputs struct with an extract(&run_state, force) -> Result constructor. The handler shrinks from ~85 lines of validation + build to a single match RunPrInputs::extract(...) followed by creds + model + request build. All error codes/messages preserved. 2. Deleted is_app_public from fabro-github plus its 3 unit tests and the now-unused MockHeaderCheck::Missing / with_req_header_missing test-helper variants. No production caller remained after the server-side install flow stopped checking app visibility client-side. Verified: workspace fmt clean, clippy --all-targets -D warnings clean, cargo nextest run --workspace 4584 passed (down from 4587 by the 3 deleted is_app_public tests), 182 skipped. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-github/src/lib.rs | 107 ++------------- lib/crates/fabro-server/src/server.rs | 188 ++++++++++++++------------ 2 files changed, 106 insertions(+), 189 deletions(-) diff --git a/lib/crates/fabro-github/src/lib.rs b/lib/crates/fabro-github/src/lib.rs index e98e16f5d..9629e3b5d 100644 --- a/lib/crates/fabro-github/src/lib.rs +++ b/lib/crates/fabro-github/src/lib.rs @@ -842,38 +842,6 @@ pub async fn update_app_webhook_config( Ok(()) } -/// Check whether a GitHub App is publicly visible. -/// -/// Calls `GET /apps/{slug}` **without** authentication. Public apps return 200, -/// private apps return 404 to unauthenticated requests. -pub async fn is_app_public( - client: &impl HttpClient, - slug: &str, - base_url: &str, -) -> Result { - let url = format!("{base_url}/apps/{slug}"); - let resp = client - .request( - HttpMethod::Get, - &url, - &[ - ("Accept", "application/vnd.github+json"), - ("User-Agent", "fabro"), - ], - None, - ) - .await - .map_err(|e| format!("Failed to check GitHub App visibility: {e}"))?; - - match resp.status { - 200 => Ok(true), - 404 => Ok(false), - status => Err(format!( - "Unexpected status {status} checking GitHub App visibility" - )), - } -} - /// Resolve git clone credentials for a GitHub repository. /// /// Returns `(username, password)` for authenticated cloning. @@ -1354,7 +1322,6 @@ mod tests { enum MockHeaderCheck { Equals(String), - Missing, } struct MockHttpClient { @@ -1384,12 +1351,6 @@ mod tests { self } - fn with_req_header_missing(mut self, name: &str) -> Self { - self.routes.last_mut().unwrap().assert_header = - Some((name.to_string(), MockHeaderCheck::Missing)); - self - } - fn with_req_body(mut self, json_str: &str) -> Self { self.routes.last_mut().unwrap().assert_body_json = Some(serde_json::from_str(json_str).unwrap()); @@ -1407,26 +1368,14 @@ mod tests { ) -> Result { for route in &self.routes { if method == route.method && url.ends_with(&route.path) { - if let Some((name, check)) = &route.assert_header { - let found = headers.iter().find(|(k, _)| *k == name.as_str()); - match check { - MockHeaderCheck::Equals(expected) => { - let (_, v) = found.unwrap_or_else(|| { - panic!("Expected header '{name}' not found in request to {url}") - }); - assert_eq!( - *v, - expected.as_str(), - "Header '{name}' mismatch for {url}" - ); - } - MockHeaderCheck::Missing => { - assert!( - found.is_none(), - "Header '{name}' should be absent for {url}" - ); - } - } + if let Some((name, MockHeaderCheck::Equals(expected))) = &route.assert_header { + let (_, v) = headers + .iter() + .find(|(k, _)| *k == name.as_str()) + .unwrap_or_else(|| { + panic!("Expected header '{name}' not found in request to {url}") + }); + assert_eq!(*v, expected.as_str(), "Header '{name}' mismatch for {url}"); } if let Some(expected_body) = &route.assert_body_json { let actual = body.expect("Expected request body"); @@ -1782,46 +1731,6 @@ mod tests { ); } - // ----------------------------------------------------------------------- - // is_app_public - // ----------------------------------------------------------------------- - - #[tokio::test] - async fn is_app_public_returns_true_on_200() { - let mock = MockHttpClient::new().on( - HttpMethod::Get, - "/apps/my-fabro-app", - 200, - r#"{"slug": "my-fabro-app"}"#, - ); - - let result = is_app_public(&mock, "my-fabro-app", "").await; - assert!(result.unwrap()); - } - - #[tokio::test] - async fn is_app_public_returns_false_on_404() { - let mock = MockHttpClient::new().on(HttpMethod::Get, "/apps/my-private-app", 404, ""); - - let result = is_app_public(&mock, "my-private-app", "").await; - assert!(!result.unwrap()); - } - - #[tokio::test] - async fn is_app_public_no_auth_header() { - let mock = MockHttpClient::new() - .on( - HttpMethod::Get, - "/apps/my-app", - 200, - r#"{"slug": "my-app"}"#, - ) - .with_req_header_missing("Authorization"); - - let result = is_app_public(&mock, "my-app", "").await; - assert!(result.unwrap()); - } - // ----------------------------------------------------------------------- // get_pull_request // ----------------------------------------------------------------------- diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index dd3769d78..98544c1b9 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -5267,6 +5267,95 @@ async fn load_pull_request_github_context( }) } +struct RunPrInputs<'a> { + run_spec: &'a fabro_types::RunSpec, + base_branch: &'a str, + run_branch: &'a str, + diff: &'a str, + conclusion: &'a fabro_types::Conclusion, + normalized_origin: String, +} + +impl<'a> RunPrInputs<'a> { + fn extract(run_state: &'a fabro_store::RunProjection, force: bool) -> Result { + if let Some(record) = run_state.pull_request.as_ref() { + return Err(ApiError::with_code( + StatusCode::CONFLICT, + format!("Pull request already exists at {}", record.html_url), + "pull_request_exists", + )); + } + let run_spec = run_state.spec.as_ref().ok_or_else(|| { + ApiError::new( + StatusCode::INTERNAL_SERVER_ERROR, + "Run spec missing from store.", + ) + })?; + let origin_url = run_spec.repo_origin_url.as_deref().ok_or_else(|| { + ApiError::with_code( + StatusCode::BAD_REQUEST, + "Run has no repo origin URL — pull request creation requires git metadata.", + "missing_repo_origin", + ) + })?; + let base_branch = run_spec.base_branch.as_deref().ok_or_else(|| { + ApiError::with_code( + StatusCode::BAD_REQUEST, + "Run has no base branch — pull request creation requires git metadata.", + "missing_base_branch", + ) + })?; + let run_branch = run_state + .start + .as_ref() + .and_then(|start| start.run_branch.as_deref()) + .ok_or_else(|| { + ApiError::with_code( + StatusCode::BAD_REQUEST, + "Run has no run_branch — was it run with git push enabled?", + "missing_run_branch", + ) + })?; + let diff = run_state + .final_patch + .as_deref() + .filter(|d| !d.trim().is_empty()) + .ok_or_else(empty_pull_request_diff_error)?; + let conclusion = run_state.conclusion.as_ref().ok_or_else(|| { + ApiError::with_code( + StatusCode::BAD_REQUEST, + "Run is not finished yet.", + "run_not_finished", + ) + })?; + if !force + && !matches!( + conclusion.status, + fabro_types::StageStatus::Success | fabro_types::StageStatus::PartialSuccess + ) + { + return Err(ApiError::with_code( + StatusCode::BAD_REQUEST, + format!( + "Run status is '{}', expected success or partial_success", + conclusion.status + ), + "run_not_successful", + )); + } + let normalized_origin = fabro_github::ssh_url_to_https(origin_url); + parse_github_owner_repo_from_url(&normalized_origin, "repo origin URL")?; + Ok(Self { + run_spec, + base_branch, + run_branch, + diff, + conclusion, + normalized_origin, + }) + } +} + async fn create_run_pull_request( AuthorizeRunScoped(id): AuthorizeRunScoped, State(state): State>, @@ -5275,7 +5364,6 @@ async fn create_run_pull_request( let Ok(run_store) = state.store.open_run(&id).await else { return ApiError::not_found("Run not found.").into_response(); }; - let run_state = match run_store.state().await { Ok(run_state) => run_state, Err(err) => { @@ -5283,94 +5371,14 @@ async fn create_run_pull_request( .into_response(); } }; - - if let Some(record) = run_state.pull_request.as_ref() { - return ApiError::with_code( - StatusCode::CONFLICT, - format!("Pull request already exists at {}", record.html_url), - "pull_request_exists", - ) - .into_response(); - } - - let Some(run_spec) = run_state.spec.as_ref() else { - return ApiError::new( - StatusCode::INTERNAL_SERVER_ERROR, - "Run spec missing from store.", - ) - .into_response(); + let inputs = match RunPrInputs::extract(&run_state, body.force) { + Ok(inputs) => inputs, + Err(err) => return err.into_response(), }; - - let Some(origin_url) = run_spec.repo_origin_url.as_deref() else { - return ApiError::with_code( - StatusCode::BAD_REQUEST, - "Run has no repo origin URL — pull request creation requires git metadata.", - "missing_repo_origin", - ) - .into_response(); - }; - let Some(base_branch) = run_spec.base_branch.as_deref() else { - return ApiError::with_code( - StatusCode::BAD_REQUEST, - "Run has no base branch — pull request creation requires git metadata.", - "missing_base_branch", - ) - .into_response(); - }; - let Some(run_branch) = run_state - .start - .as_ref() - .and_then(|start| start.run_branch.as_deref()) - else { - return ApiError::with_code( - StatusCode::BAD_REQUEST, - "Run has no run_branch — was it run with git push enabled?", - "missing_run_branch", - ) - .into_response(); - }; - - let Some(diff) = run_state.final_patch.as_deref() else { - return empty_pull_request_diff_error().into_response(); - }; - if diff.trim().is_empty() { - return empty_pull_request_diff_error().into_response(); - } - - let Some(conclusion) = run_state.conclusion.as_ref() else { - return ApiError::with_code( - StatusCode::BAD_REQUEST, - "Run is not finished yet.", - "run_not_finished", - ) - .into_response(); - }; - if !body.force - && !matches!( - conclusion.status, - fabro_types::StageStatus::Success | fabro_types::StageStatus::PartialSuccess - ) - { - return ApiError::with_code( - StatusCode::BAD_REQUEST, - format!( - "Run status is '{}', expected success or partial_success", - conclusion.status - ), - "run_not_successful", - ) - .into_response(); - } - - let normalized_origin = fabro_github::ssh_url_to_https(origin_url); - if let Err(err) = parse_github_owner_repo_from_url(&normalized_origin, "repo origin URL") { - return err.into_response(); - } let creds = match load_server_github_credentials(state.as_ref()) { Ok(creds) => creds, Err(err) => return err.into_response(), }; - let model = if let Some(model) = body.model { model } else { @@ -5384,14 +5392,14 @@ async fn create_run_pull_request( 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, + inputs.run_spec, + inputs.run_branch, + &inputs.normalized_origin, + inputs.base_branch, + inputs.diff, &model, &run_store_handle, - conclusion, + inputs.conclusion, ); let pull_request = match pull_request::maybe_open_pull_request(request).await { Ok(Some(record)) => record,