refactor(server): narrow RunPrInputs to goal; drop PullRequestGithubContext.{owner,repo}

Two code-reuse findings from the simplify review:

1. PullRequestGithubContext carried owner/repo String fields obtained by
   re-parsing record.html_url, even though PullRequestRecord already
   carries typed non-optional owner/repo fields. Dropped the redundant
   fields; the 3 PR handlers read via &ctx.record.owner /
   &ctx.record.repo instead. The incidental non-github.com URL
   rejection is preserved as an explicit one-line host-validation
   call (documented by the rejects_non_github_record_url tests).

2. RunPrInputs held run_spec: &RunSpec purely to read goal()
   downstream. Narrowed to goal: &str stored directly; the server
   handler passes inputs.goal to OpenPullRequestRequest::from_run_state,
   which no longer needs the full RunSpec. Fewer fields, clearer
   dependency at the call site.

Also tightened the from_run_state doc comment (was narrating peer
callers' behavior rather than the method's contract).

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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-24 10:38:55 -04:00
parent d822af3d32
commit 9ede01698c
No known key found for this signature in database
2 changed files with 16 additions and 25 deletions

View file

@ -5224,8 +5224,6 @@ fn github_pull_request_not_found_error(record: &PullRequestRecord) -> ApiError {
struct PullRequestGithubContext {
record: PullRequestRecord,
owner: String,
repo: String,
creds: fabro_github::GitHubCredentials,
}
@ -5249,18 +5247,13 @@ async fn load_pull_request_github_context(
"no_stored_record",
)
})?;
let (owner, repo) = parse_github_owner_repo_from_url(&record.html_url, "pull request URL")?;
parse_github_owner_repo_from_url(&record.html_url, "pull request URL")?;
let creds = load_server_github_credentials(state.as_ref())?;
Ok(PullRequestGithubContext {
record,
owner,
repo,
creds,
})
Ok(PullRequestGithubContext { record, creds })
}
struct RunPrInputs<'a> {
run_spec: &'a fabro_types::RunSpec,
goal: &'a str,
base_branch: &'a str,
run_branch: &'a str,
diff: &'a str,
@ -5344,7 +5337,7 @@ impl<'a> RunPrInputs<'a> {
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,
goal: run_spec.graph.goal(),
base_branch,
run_branch,
diff,
@ -5390,7 +5383,7 @@ 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()),
inputs.run_spec,
inputs.goal,
inputs.run_branch,
&inputs.normalized_origin,
inputs.base_branch,
@ -5439,8 +5432,8 @@ 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()),
&ctx.owner,
&ctx.repo,
&ctx.record.owner,
&ctx.record.repo,
ctx.record.number,
)
.await
@ -5469,8 +5462,8 @@ 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()),
&ctx.owner,
&ctx.repo,
&ctx.record.owner,
&ctx.record.repo,
ctx.record.number,
body.method,
)
@ -5501,8 +5494,8 @@ 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()),
&ctx.owner,
&ctx.repo,
&ctx.record.owner,
&ctx.record.repo,
ctx.record.number,
)
.await

View file

@ -410,18 +410,16 @@ pub struct OpenPullRequestRequest<'a> {
}
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.
/// Build a draft PR request from already-validated inputs. Defaults
/// `draft = true` and `auto_merge = None` — the shape every
/// `POST /runs/{id}/pull_request` request uses.
#[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,
goal: &'a str,
head_branch: &'a str,
normalized_origin: &'a str,
base_branch: &'a str,
@ -435,7 +433,7 @@ impl<'a> OpenPullRequestRequest<'a> {
origin_url: normalized_origin,
base_branch,
head_branch,
goal: run_spec.graph.goal(),
goal,
diff,
model,
draft: true,