From eb7bcee62ef46ed42f8c2491e195d3aa83d73138 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:16:35 -0400 Subject: [PATCH 01/16] feat: add open pull request workflow --- Cargo.lock | 1 + crates/bitbygit-core/src/lib.rs | 6 + crates/bitbygit-core/src/prompt_parser.rs | 50 ++- crates/bitbygit-tui/Cargo.toml | 1 + crates/bitbygit-tui/src/lib.rs | 506 ++++++++++++++++++++++ 5 files changed, 562 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7118566..61733df 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -45,6 +45,7 @@ name = "bitbygit-tui" version = "0.1.0" dependencies = [ "bitbygit-core", + "bitbygit-gh", "bitbygit-git", "bitbygit-store", "crossterm", diff --git a/crates/bitbygit-core/src/lib.rs b/crates/bitbygit-core/src/lib.rs index 01c4241..a698ca7 100644 --- a/crates/bitbygit-core/src/lib.rs +++ b/crates/bitbygit-core/src/lib.rs @@ -39,6 +39,9 @@ pub enum OperationRequest { Rebase { base: String, }, + OpenPullRequest { + base: Option, + }, PromptSequence { requests: Vec, }, @@ -63,6 +66,7 @@ pub enum OperationKind { CreateBranch, MergeFastForward, Rebase, + OpenPullRequest, } impl OperationKind { @@ -84,6 +88,7 @@ impl OperationKind { Self::CreateBranch => "create branch", Self::MergeFastForward => "merge", Self::Rebase => "rebase", + Self::OpenPullRequest => "open pull request", } } @@ -106,6 +111,7 @@ impl OperationKind { Self::CreateBranch => "create_branch", Self::MergeFastForward => "merge_ff_only", Self::Rebase => "rebase", + Self::OpenPullRequest => "open_pull_request", } } } diff --git a/crates/bitbygit-core/src/prompt_parser.rs b/crates/bitbygit-core/src/prompt_parser.rs index 162ebc5..5a17a43 100644 --- a/crates/bitbygit-core/src/prompt_parser.rs +++ b/crates/bitbygit-core/src/prompt_parser.rs @@ -3,8 +3,8 @@ use std::fmt; use crate::OperationRequest; -const PROMPT_EXAMPLES: &str = "branches, checkout , branch , branch from , merge , rebase , commit -m \"message\", fetch, push, pull, or pull --rebase"; -pub const UNSUPPORTED_PROMPT_MESSAGE: &str = "Unsupported prompt. Try: branches, checkout , branch , branch from , merge , rebase , commit -m \"message\", fetch, push, pull, or pull --rebase"; +const PROMPT_EXAMPLES: &str = "branches, checkout , branch , branch from , merge , rebase , open pr, commit -m \"message\", fetch, push, pull, or pull --rebase"; +pub const UNSUPPORTED_PROMPT_MESSAGE: &str = "Unsupported prompt. Try: branches, checkout , branch , branch from , merge , rebase , open pr, commit -m \"message\", fetch, push, pull, or pull --rebase"; const SHELL_SYNTAX_MESSAGE: &str = "Shell-style prompt syntax is not supported. Use one guarded prompt at a time."; const RAW_GIT_MESSAGE: &str = @@ -168,6 +168,7 @@ fn parse_single_prompt(input: &str) -> Result Ok(OperationRequest::Branches), "fetch" => Ok(OperationRequest::Fetch), "push" => Ok(OperationRequest::Push), + "open pr" | "open pull request" => Ok(OperationRequest::OpenPullRequest { base: None }), "pull" => Ok(OperationRequest::Pull { rebase: false }), "pull --rebase" | "pull rebase" => Ok(OperationRequest::Pull { rebase: true }), _ if lower.starts_with("checkout ") => parse_one_arg_prompt(trimmed, "checkout") @@ -178,6 +179,10 @@ fn parse_single_prompt(input: &str) -> Result { parse_one_arg_prompt(trimmed, "rebase").map(|base| OperationRequest::Rebase { base }) } + _ if lower.starts_with("open pr to ") => parse_open_pull_request_prompt(trimmed, "open pr"), + _ if lower.starts_with("open pull request to ") => { + parse_open_pull_request_prompt(trimmed, "open pull request") + } _ if lower.starts_with("branch ") => parse_branch_prompt(trimmed), _ if lower == "commit" || lower.starts_with("commit ") => { parse_commit_prompt(trimmed).map(|message| OperationRequest::Commit { message }) @@ -186,6 +191,30 @@ fn parse_single_prompt(input: &str) -> Result Result { + let Some(rest) = input.get(command.len()..) else { + return Err(parse_error("Expected: open pr or open pr to ")); + }; + let rest = rest.trim_start(); + let Some(base) = rest + .get(..2) + .filter(|prefix| prefix.eq_ignore_ascii_case("to")) + .and_then(|_prefix| rest.get(2..)) + .filter(|base| base.starts_with(char::is_whitespace)) + else { + return Err(parse_error("Expected: open pr or open pr to ")); + }; + Ok(OperationRequest::OpenPullRequest { + base: Some(parse_branch_arg( + base.trim(), + "Expected: open pr or open pr to ", + )?), + }) +} + fn parse_one_arg_prompt(input: &str, command: &str) -> Result { let Some(rest) = input.get(command.len()..) else { return Err(parse_error(format!("Expected: {command} "))); @@ -304,6 +333,23 @@ mod tests { ("branches", OperationRequest::Branches), ("fetch", OperationRequest::Fetch), ("push", OperationRequest::Push), + ("open pr", OperationRequest::OpenPullRequest { base: None }), + ( + "open pull request", + OperationRequest::OpenPullRequest { base: None }, + ), + ( + "open pull request to develop", + OperationRequest::OpenPullRequest { + base: Some("develop".to_owned()), + }, + ), + ( + "OPEN PR TO release", + OperationRequest::OpenPullRequest { + base: Some("release".to_owned()), + }, + ), ("pull", OperationRequest::Pull { rebase: false }), ("pull --rebase", OperationRequest::Pull { rebase: true }), ("pull rebase", OperationRequest::Pull { rebase: true }), diff --git a/crates/bitbygit-tui/Cargo.toml b/crates/bitbygit-tui/Cargo.toml index 117de06..6d15710 100644 --- a/crates/bitbygit-tui/Cargo.toml +++ b/crates/bitbygit-tui/Cargo.toml @@ -8,6 +8,7 @@ rust-version.workspace = true [dependencies] bitbygit-core = { path = "../bitbygit-core" } +bitbygit-gh = { path = "../bitbygit-gh" } bitbygit-git = { path = "../bitbygit-git" } bitbygit-store = { path = "../bitbygit-store" } crossterm = "0.28" diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 9270d51..61fa762 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -21,6 +21,7 @@ use bitbygit_core::{ RiskLevel, prompt_parser::{ParsedPrompt, parse_prompt}, }; +use bitbygit_gh::{CreatePullRequest, GhError, GitHub}; use bitbygit_git::{ BranchInfo, BranchKind, BranchState, BranchTarget, ChangeKind, Git, GitError, GitOutput, Head, HeadTarget, StatusEntry, StatusEntryType, @@ -675,6 +676,17 @@ enum PendingPayload { base: BranchTarget, target: HeadTarget, }, + OpenPullRequest { + branch: String, + upstream: String, + remote: String, + remote_urls: Vec, + target: HeadTarget, + base: String, + title: String, + repository: String, + github_executable: Option, + }, } fn prompt_accepts_modifiers(modifiers: KeyModifiers) -> bool { @@ -1187,6 +1199,42 @@ fn rebase_plan(current: &str, base: &BranchTarget) -> OperationPlan { ) } +fn open_pull_request_plan( + request: OperationRequest, + remote: &str, + head: &str, + base: &str, + title: &str, + repository: &str, + existing_url: Option<&str>, +) -> OperationPlan { + let target = existing_url.map(ToOwned::to_owned).unwrap_or_else(|| { + format!("https://github.com/{repository}/compare/{base}...{head}?expand=1") + }); + let summary = if existing_url.is_some() { + format!("surface existing pull request for {head}") + } else { + format!("open pull request from {head} to {base}") + }; + OperationPlan::new( + request, + "Open pull request plan", + vec![ + OperationStep::new(OperationKind::OpenPullRequest, RiskLevel::Medium, summary) + .with_detail("provider: GitHub") + .with_detail(format!("remote: {remote}")) + .with_detail(format!("head: {head}")) + .with_detail(format!("base: {base}")) + .with_detail(format!("title: {title}")) + .with_detail(format!("target: {target}")) + .with_detail( + "revalidate branch, upstream, remote, and pull request state before execution", + ), + ], + "Press y to open or surface the pull request or n to cancel.", + ) +} + fn prompt_sequence_plan( requests: &[OperationRequest], first: &PreparedOperation, @@ -1344,6 +1392,9 @@ fn prompt_sequence_request_preview( RiskLevel::High, format!("rebase current branch onto {base}"), )), + OperationRequest::OpenPullRequest { .. } => { + Err("open pull request is only available as a single prompt".to_owned()) + } OperationRequest::RefreshStatus | OperationRequest::ViewDiff { .. } | OperationRequest::StagePaths { .. } @@ -1435,12 +1486,14 @@ fn short_oid(oid: &str) -> String { #[derive(Debug, Clone)] struct OperationPlanner { repo_root: PathBuf, + github_executable: Option, } impl OperationPlanner { fn current() -> Self { Self { repo_root: current_dir(), + github_executable: None, } } @@ -1448,6 +1501,7 @@ impl OperationPlanner { fn new(repo_root: impl Into) -> Self { Self { repo_root: repo_root.into(), + github_executable: None, } } @@ -1490,6 +1544,7 @@ impl OperationPlanner { } OperationRequest::Merge { branch } => self.plan_merge(branch), OperationRequest::Rebase { base } => self.plan_rebase(base), + OperationRequest::OpenPullRequest { base } => self.plan_open_pull_request(base), OperationRequest::PromptSequence { .. } => { Err("Prompt sequences are handled by prompt submission.".to_owned()) } @@ -1818,6 +1873,93 @@ impl OperationPlanner { )) } + fn plan_open_pull_request( + &self, + requested_base: Option, + ) -> Result { + let git = self.git(); + let status = git + .status() + .map_err(|error| format!("Unable to prepare pull request plan: {error}"))?; + let branch = branch_name(&status.branch)?; + let Some(upstream) = status.branch.upstream.clone() else { + return Err("Open pull request blocked: current branch has no upstream. Push it with `push` first.".to_owned()); + }; + if status.branch.ahead > 0 || status.branch.behind > 0 { + return Err("Open pull request blocked: current branch is not fully pushed and up to date. Push or pull it first.".to_owned()); + } + let (remote, upstream_branch) = git + .upstream_push_target(&branch) + .map_err(|error| format!("Unable to prepare pull request target: {error}"))? + .ok_or_else(|| { + format!("Open pull request blocked: unable to resolve upstream {upstream}.") + })?; + if upstream != format!("{remote}/{upstream_branch}") || upstream_branch != branch { + return Err( + "Open pull request blocked: current branch must track a same-named remote branch." + .to_owned(), + ); + } + let remote_urls = git + .remote_push_urls(&remote) + .map_err(|error| format!("Unable to prepare pull request remote: {error}"))?; + let push_url = single_pull_request_push_url(&remote, &remote_urls)?; + let target = git + .head_target() + .map_err(|error| format!("Unable to snapshot pull request branch: {error}"))?; + let local_oid = target.oid.as_deref().ok_or_else(|| { + "Open pull request blocked: current branch needs a commit before opening a pull request." + .to_owned() + })?; + let remote_oid = git + .remote_url_head_oid(push_url, &branch) + .map_err(|error| format!("Unable to verify pushed branch: {error}"))?; + if remote_oid.as_deref() != Some(local_oid) { + return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); + } + let github = self.github(); + let repository = github.repository().map_err(open_pull_request_gh_error)?; + let base = requested_base.unwrap_or(repository.default_branch); + if base == branch { + return Err( + "Open pull request blocked: base branch must differ from the current branch." + .to_owned(), + ); + } + let existing = github + .existing_pull_requests(&branch) + .map_err(open_pull_request_gh_error)?; + let existing_url = existing + .first() + .map(|pull_request| pull_request.url.as_str()); + let title = branch.clone(); + let request = OperationRequest::OpenPullRequest { + base: Some(base.clone()), + }; + Ok(PreparedOperation::new( + open_pull_request_plan( + request, + &remote, + &branch, + &base, + &title, + &repository.name_with_owner, + existing_url, + ), + ExecutionContext::from_payload(PendingPayload::OpenPullRequest { + branch, + upstream, + remote, + remote_urls, + target, + base, + title, + repository: repository.name_with_owner, + github_executable: self.github_executable.clone(), + }), + )) + } + fn ensure_clean_branch_worktree(&self, action: &str) -> Result<(), String> { self.git() .ensure_clean_worktree(&action.to_ascii_lowercase()) @@ -1844,6 +1986,17 @@ impl OperationPlanner { fn git(&self) -> Git { Git::new(self.repo_root.clone()) } + + fn github(&self) -> GitHub { + match &self.github_executable { + Some(executable) => GitHub::with_executable(&self.repo_root, executable), + None => GitHub::new(&self.repo_root), + } + } +} + +fn open_pull_request_gh_error(error: GhError) -> String { + format!("Open pull request blocked: {error}") } fn single_push_url<'a>(remote: &str, urls: &'a [String]) -> Result<&'a str, String> { @@ -1858,6 +2011,18 @@ fn single_push_url<'a>(remote: &str, urls: &'a [String]) -> Result<&'a str, Stri } } +fn single_pull_request_push_url<'a>(remote: &str, urls: &'a [String]) -> Result<&'a str, String> { + match urls { + [url] => Ok(url), + [] => Err(format!( + "Open pull request blocked: remote {remote} has no push URL." + )), + _ => Err(format!( + "Open pull request blocked: remote {remote} has multiple push URLs." + )), + } +} + fn validate_push_plan( git: &Git, branch: &str, @@ -1932,6 +2097,81 @@ fn status_visible_len(area: Rect) -> usize { area.height.saturating_sub(2).max(1) as usize } +fn validate_open_pull_request_plan( + git: &Git, + branch: &str, + expected_upstream: &str, + remote: &str, + remote_urls: &[String], + target: &HeadTarget, +) -> Result<(), String> { + if git + .head_target() + .map_err(|error| format!("Unable to revalidate pull request target: {error}"))? + != *target + { + return Err( + "Open pull request blocked: branch target changed since the plan was shown.".to_owned(), + ); + } + let status = git + .status() + .map_err(|error| format!("Unable to revalidate pull request plan: {error}"))?; + if branch_name(&status.branch)? != branch { + return Err( + "Open pull request blocked: current branch changed since the plan was shown." + .to_owned(), + ); + } + if status.branch.upstream.as_deref() != Some(expected_upstream) { + return Err( + "Open pull request blocked: upstream changed since the plan was shown.".to_owned(), + ); + } + if status.branch.ahead > 0 || status.branch.behind > 0 { + return Err( + "Open pull request blocked: current branch is no longer fully pushed and up to date." + .to_owned(), + ); + } + let current_target = git + .upstream_push_target(branch) + .map_err(|error| format!("Unable to revalidate pull request upstream: {error}"))?; + if current_target + .as_ref() + .map(|(name, branch)| format!("{name}/{branch}")) + != Some(expected_upstream.to_owned()) + { + return Err( + "Open pull request blocked: upstream target changed since the plan was shown." + .to_owned(), + ); + } + let current_remote_urls = git + .remote_push_urls(remote) + .map_err(|error| format!("Unable to revalidate pull request remote: {error}"))?; + if current_remote_urls != remote_urls { + return Err( + "Open pull request blocked: remote URLs changed since the plan was shown.".to_owned(), + ); + } + let push_url = single_pull_request_push_url(remote, ¤t_remote_urls)?; + let local_oid = target + .oid + .as_deref() + .ok_or_else(|| "Open pull request blocked: planned branch has no commit.".to_owned())?; + let remote_oid = git + .remote_url_head_oid(push_url, branch) + .map_err(|error| format!("Unable to revalidate pushed branch: {error}"))?; + if remote_oid.as_deref() != Some(local_oid) { + return Err( + "Open pull request blocked: current branch is no longer pushed to its upstream." + .to_owned(), + ); + } + Ok(()) +} + fn status_file_visible_len(area: Rect) -> usize { status_visible_len(area).saturating_sub(1) } @@ -2193,6 +2433,7 @@ impl PlanExecutor { OperationKind::CreateBranch => self.run_create_branch_step(plan, context, &git), OperationKind::MergeFastForward => self.run_merge_step(plan, context, &git), OperationKind::Rebase => self.run_rebase_step(plan, context, &git), + OperationKind::OpenPullRequest => self.run_open_pull_request_step(plan, context, &git), OperationKind::RefreshStatus | OperationKind::ViewDiff => { Err(StepExecutionError::Unsupported(format!( "{} step is not executable by the typed operation executor", @@ -2468,6 +2709,87 @@ impl PlanExecutor { } git_output(git.rebase_onto(target, head)) } + + fn run_open_pull_request_step( + &self, + plan: &OperationPlan, + context: &ExecutionContext, + git: &Git, + ) -> StepRunResult { + let OperationRequest::OpenPullRequest { + base: Some(requested_base), + } = &plan.request + else { + return Err(StepExecutionError::Unsupported( + "open pull request step requires a typed pull request request".to_owned(), + )); + }; + let PendingPayload::OpenPullRequest { + branch, + upstream, + remote, + remote_urls, + target, + base, + title, + repository, + github_executable, + } = typed_payload(context, OperationKind::OpenPullRequest)? + else { + return Err(mismatched_context(OperationKind::OpenPullRequest)); + }; + if base != requested_base { + return Err(StepExecutionError::Unsupported( + "open pull request request does not match its typed execution context".to_owned(), + )); + } + validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target) + .map_err(StepExecutionError::Blocked)?; + let github = match github_executable { + Some(executable) => GitHub::with_executable(&self.repo_root, executable), + None => GitHub::new(&self.repo_root), + }; + let current_repository = github.repository().map_err(StepExecutionError::GitHub)?; + if current_repository.name_with_owner != *repository { + return Err(StepExecutionError::Blocked( + "Open pull request blocked: GitHub repository changed since the plan was shown." + .to_owned(), + )); + } + if let Some(existing) = github + .existing_pull_requests(branch) + .map_err(StepExecutionError::GitHub)? + .into_iter() + .next() + { + return Ok(ExecutionOutput::PullRequest { + url: existing.url, + existing: true, + }); + } + match github.create_pull_request(&CreatePullRequest { + title: title.clone(), + body: String::new(), + base: base.clone(), + head: branch.clone(), + }) { + Ok(created) => Ok(ExecutionOutput::PullRequest { + url: created.url, + existing: false, + }), + Err(error) => match github.existing_pull_requests(branch) { + Ok(existing) => existing + .into_iter() + .next() + .map(|pull_request| ExecutionOutput::PullRequest { + url: pull_request.url, + existing: true, + }) + .ok_or(StepExecutionError::GitHub(error)), + Err(_) => Err(StepExecutionError::GitHub(error)), + }, + } + } } #[derive(Debug, Clone)] @@ -2496,6 +2818,7 @@ impl PromptSequenceExecutor { let total_steps = sequence.total_steps(); let planner = OperationPlanner { repo_root: self.repo_root.clone(), + github_executable: None, }; let executor = PlanExecutor { repo_root: self.repo_root.clone(), @@ -2787,11 +3110,13 @@ type StepRunResult = Result; enum ExecutionOutput { Git(GitOutput), Branches(Vec), + PullRequest { url: String, existing: bool }, } #[derive(Debug)] enum StepExecutionError { Git(GitError), + GitHub(GhError), Blocked(String), Unsupported(String), } @@ -2800,6 +3125,7 @@ impl StepExecutionError { fn audit_message(&self) -> String { match self { Self::Git(error) => sanitized_git_error(error), + Self::GitHub(error) => sanitized_github_error(error), Self::Blocked(message) => format!("operation blocked: {message}"), Self::Unsupported(message) => message.clone(), } @@ -2810,6 +3136,7 @@ impl std::fmt::Display for StepExecutionError { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { Self::Git(error) => write!(formatter, "{error}"), + Self::GitHub(error) => write!(formatter, "{error}"), Self::Blocked(message) | Self::Unsupported(message) => write!(formatter, "{message}"), } } @@ -2891,6 +3218,13 @@ fn mismatched_context(kind: OperationKind) -> StepExecutionError { fn success_message(kind: OperationKind, action: &str, output: &ExecutionOutput) -> String { match output { ExecutionOutput::Branches(branches) => branch_list_message(branches), + ExecutionOutput::PullRequest { url, existing } => { + if *existing { + format!("Pull request already open: {url}") + } else { + format!("Pull request created: {url}") + } + } ExecutionOutput::Git(output) => { let output = git_output_text(output); if should_show_git_output(kind) && !output.is_empty() { @@ -2913,6 +3247,9 @@ fn success_with_audit_error_message( ExecutionOutput::Branches(_) => { format!("{action} succeeded, but audit finalization failed: {audit_error}\n{success}") } + ExecutionOutput::PullRequest { .. } => { + format!("{success}, but audit finalization failed: {audit_error}") + } ExecutionOutput::Git(output) => { let output = git_output_text(output); if should_show_git_output(kind) && !output.is_empty() { @@ -3124,6 +3461,19 @@ fn sanitized_git_error(error: &GitError) -> String { } } +fn sanitized_github_error(error: &GhError) -> String { + match error { + GhError::MissingCli | GhError::NotAuthenticated | GhError::InvalidInput { .. } => { + error.to_string() + } + GhError::CommandFailed { status } => format!("GitHub CLI failed with status {status}"), + GhError::Io { .. } => "GitHub CLI failed before execution".to_owned(), + GhError::InvalidOutput { operation } => { + format!("GitHub CLI returned invalid output for {operation}") + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -3557,6 +3907,119 @@ mod tests { Ok(()) } + #[test] + fn open_pull_request_plan_previews_target_and_creates_pull_request() + -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-create")?; + let fake_gh = fake_gh("open-pr-create", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + let preview = operation.plan.preview_text(); + assert!(preview.contains("provider: GitHub")); + assert!(preview.contains("remote: origin")); + assert!(preview.contains("head: feature/open-pr")); + assert!(preview.contains("base: main")); + assert!(preview.contains("title: feature/open-pr")); + assert!(preview.contains("https://github.com/octo/repo/compare/main...feature/open-pr")); + assert_eq!( + operation.plan.confirmation.requirement, + ConfirmationRequirement::VisiblePlan + ); + let paths = isolated_store_paths("open-pr-create-audit")?; + let execution = PlanExecutor::with_audit_paths(&repo, paths.clone()) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + assert_eq!( + execution.message(), + "Pull request created: https://github.com/octo/repo/pull/43" + ); + let entries = LocalStore::open(paths)?.list_audit_entries()?; + assert_eq!(entries[0].operation, "open_pull_request"); + assert!( + entries + .iter() + .all(|entry| !entry.message.contains("pull/43")) + ); + Ok(()) + } + + #[test] + fn existing_pull_request_is_surfaced_without_creation() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-existing")?; + let fake_gh = fake_gh("open-pr-existing", true)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + }; + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!( + operation + .plan + .preview_text() + .contains("target: https://github.com/octo/repo/pull/42") + ); + let execution = + PlanExecutor::with_audit_paths(&repo, isolated_store_paths("open-pr-existing-audit")?) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + assert_eq!( + execution.message(), + "Pull request already open: https://github.com/octo/repo/pull/42" + ); + assert!( + !std::fs::read_to_string(fake_gh.with_file_name("invocations"))?.contains("pr:create") + ); + Ok(()) + } + + #[test] + fn open_pull_request_blocks_missing_upstream_and_matching_base() -> Result<(), Box> { + let repo = isolated_git_repo("open-pr-invalid-state")?; + configure_git_identity(&repo)?; + std::fs::write(repo.join("file.txt"), "base\n")?; + git_stdout(&repo, &["add", "file.txt"])?; + git_stdout(&repo, &["commit", "-m", "initial"])?; + let planner = OperationPlanner::new(&repo); + + let error = match planner.plan_request(OperationRequest::OpenPullRequest { base: None }) { + Ok(_) => return Err(std::io::Error::other("missing upstream must fail closed").into()), + Err(error) => error, + }; + assert!(error.contains("has no upstream")); + + let remote = isolated_bare_git_repo("open-pr-invalid-state-remote")?; + let branch = git_stdout(&repo, &["branch", "--show-current"])? + .trim() + .to_owned(); + let remote_arg = remote.to_string_lossy().to_string(); + git_stdout(&repo, &["remote", "add", "origin", remote_arg.as_str()])?; + git_stdout(&repo, &["push", "-u", "origin", branch.as_str()])?; + let fake_gh = fake_gh("open-pr-invalid-state", false)?; + let planner = OperationPlanner { + repo_root: repo, + github_executable: Some(fake_gh), + }; + let error = + match planner.plan_request(OperationRequest::OpenPullRequest { base: Some(branch) }) { + Ok(_) => return Err(std::io::Error::other("base=head must fail closed").into()), + Err(error) => error, + }; + assert!(error.contains("base branch must differ")); + Ok(()) + } + #[test] fn operation_plans_use_guardrail_risk_levels() { let branch = BranchTarget { @@ -4327,6 +4790,49 @@ mod tests { Ok(root) } + fn pushed_branch_repo(name: &str) -> Result> { + let repo = isolated_git_repo(name)?; + let remote = isolated_bare_git_repo(&format!("{name}-remote"))?; + configure_git_identity(&repo)?; + std::fs::write(repo.join("file.txt"), "base\n")?; + git_stdout(&repo, &["add", "file.txt"])?; + git_stdout(&repo, &["commit", "-m", "initial"])?; + git_stdout(&repo, &["checkout", "-b", "feature/open-pr"])?; + std::fs::write(repo.join("file.txt"), "feature\n")?; + git_stdout(&repo, &["add", "file.txt"])?; + git_stdout(&repo, &["commit", "-m", "feature"])?; + let remote_arg = remote.to_string_lossy().to_string(); + git_stdout(&repo, &["remote", "add", "origin", remote_arg.as_str()])?; + git_stdout(&repo, &["push", "-u", "origin", "feature/open-pr"])?; + Ok(repo) + } + + fn fake_gh(name: &str, existing: bool) -> Result> { + use std::os::unix::fs::PermissionsExt; + + let root = isolated_temp_root(&format!("fake-gh-{name}"))?; + std::fs::create_dir_all(&root)?; + let executable = root.join("gh"); + let invocations = root.join("invocations"); + let pull_requests = if existing { + "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]" + } else { + "[]" + }; + std::fs::write( + &executable, + format!( + "#!/bin/sh\nprintf '%s:%s\\n' \"$1\" \"$2\" >> '{}'\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}' ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) printf '%s\\n' 'https://github.com/octo/repo/pull/43' ;;\n*) exit 1 ;;\nesac\n", + invocations.display(), + pull_requests + ), + )?; + let mut permissions = std::fs::metadata(&executable)?.permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&executable, permissions)?; + Ok(executable) + } + fn git_stdout(repo: &std::path::Path, args: &[&str]) -> Result> { let output = std::process::Command::new("git") .arg("-C") From c2a71d00c93ea7f4fb7ea0a25a769efc3fec3722 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:22:28 -0400 Subject: [PATCH 02/16] fix: honor pull request base target --- crates/bitbygit-tui/src/lib.rs | 115 ++++++++++++++++++++++++++++++--- 1 file changed, 106 insertions(+), 9 deletions(-) diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 61fa762..9b5a12e 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1209,7 +1209,11 @@ fn open_pull_request_plan( existing_url: Option<&str>, ) -> OperationPlan { let target = existing_url.map(ToOwned::to_owned).unwrap_or_else(|| { - format!("https://github.com/{repository}/compare/{base}...{head}?expand=1") + format!( + "https://github.com/{repository}/compare/{}...{}?expand=1", + compare_url_ref(base), + compare_url_ref(head) + ) }); let summary = if existing_url.is_some() { format!("surface existing pull request for {head}") @@ -1235,6 +1239,25 @@ fn open_pull_request_plan( ) } +fn compare_url_ref(reference: &str) -> String { + const HEX: &[u8; 16] = b"0123456789ABCDEF"; + + let mut encoded = String::with_capacity(reference.len()); + for byte in reference.bytes() { + match byte { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' | b'/' => { + encoded.push(char::from(byte)); + } + _ => { + encoded.push('%'); + encoded.push(char::from(HEX[usize::from(byte >> 4)])); + encoded.push(char::from(HEX[usize::from(byte & 0x0f)])); + } + } + } + encoded +} + fn prompt_sequence_plan( requests: &[OperationRequest], first: &PreparedOperation, @@ -1930,7 +1953,8 @@ impl OperationPlanner { .existing_pull_requests(&branch) .map_err(open_pull_request_gh_error)?; let existing_url = existing - .first() + .iter() + .find(|pull_request| pull_request.base_ref_name == base) .map(|pull_request| pull_request.url.as_str()); let title = branch.clone(); let request = OperationRequest::OpenPullRequest { @@ -2760,7 +2784,7 @@ impl PlanExecutor { .existing_pull_requests(branch) .map_err(StepExecutionError::GitHub)? .into_iter() - .next() + .find(|pull_request| pull_request.base_ref_name == *base) { return Ok(ExecutionOutput::PullRequest { url: existing.url, @@ -2780,7 +2804,7 @@ impl PlanExecutor { Err(error) => match github.existing_pull_requests(branch) { Ok(existing) => existing .into_iter() - .next() + .find(|pull_request| pull_request.base_ref_name == *base) .map(|pull_request| ExecutionOutput::PullRequest { url: pull_request.url, existing: true, @@ -3984,6 +4008,65 @@ mod tests { Ok(()) } + #[test] + fn pull_request_for_other_base_is_not_surfaced_after_create_failure() + -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-other-base")?; + let fake_gh = fake_gh_with_pull_requests( + "open-pr-other-base", + "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]", + false, + )?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + }; + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { + base: Some("release".to_owned()), + }) + .map_err(std::io::Error::other)?; + + assert!( + operation + .plan + .preview_text() + .contains("https://github.com/octo/repo/compare/release...feature/open-pr") + ); + let execution = PlanExecutor::with_audit_paths( + &repo, + isolated_store_paths("open-pr-other-base-audit")?, + ) + .execute(&operation.plan, operation.context); + + assert!(!execution.succeeded()); + assert!(!execution.message().contains("pull/42")); + assert!( + std::fs::read_to_string(fake_gh.with_file_name("invocations"))? + .contains("--base release") + ); + Ok(()) + } + + #[test] + fn open_pull_request_plan_encodes_compare_ref_names() { + let plan = open_pull_request_plan( + OperationRequest::OpenPullRequest { + base: Some("release#candidate".to_owned()), + }, + "origin", + "feature%ready", + "release#candidate", + "feature%ready", + "octo/repo", + None, + ); + + assert!(plan.preview_text().contains( + "https://github.com/octo/repo/compare/release%23candidate...feature%25ready?expand=1" + )); + } + #[test] fn open_pull_request_blocks_missing_upstream_and_matching_base() -> Result<(), Box> { let repo = isolated_git_repo("open-pr-invalid-state")?; @@ -4808,23 +4891,37 @@ mod tests { } fn fake_gh(name: &str, existing: bool) -> Result> { + let pull_requests = if existing { + "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]" + } else { + "[]" + }; + fake_gh_with_pull_requests(name, pull_requests, true) + } + + fn fake_gh_with_pull_requests( + name: &str, + pull_requests: &str, + create_succeeds: bool, + ) -> Result> { use std::os::unix::fs::PermissionsExt; let root = isolated_temp_root(&format!("fake-gh-{name}"))?; std::fs::create_dir_all(&root)?; let executable = root.join("gh"); let invocations = root.join("invocations"); - let pull_requests = if existing { - "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]" + let create_response = if create_succeeds { + "printf '%s\\n' 'https://github.com/octo/repo/pull/43' ;;" } else { - "[]" + "exit 1 ;;" }; std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s\\n' \"$1\" \"$2\" >> '{}'\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}' ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) printf '%s\\n' 'https://github.com/octo/repo/pull/43' ;;\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}' ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), - pull_requests + pull_requests, + create_response ), )?; let mut permissions = std::fs::metadata(&executable)?.permissions(); From e471fe5b022b1d74445eb932806e4d81f7ce5038 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:28:45 -0400 Subject: [PATCH 03/16] fix: qualify existing pull request heads --- crates/bitbygit-gh/src/lib.rs | 21 +++++++++-- crates/bitbygit-tui/src/lib.rs | 69 ++++++++++++++++++++++++++++++++-- 2 files changed, 83 insertions(+), 7 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 49d636c..039a157 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -82,7 +82,7 @@ impl GitHub { "--limit".to_owned(), "0".to_owned(), "--json".to_owned(), - "number,url,title,baseRefName,headRefName".to_owned(), + "number,url,title,baseRefName,headRefName,headRepository".to_owned(), ])?; parse_json(&output, "pull request query") } @@ -202,6 +202,14 @@ pub struct PullRequest { pub base_ref_name: String, #[serde(rename = "headRefName")] pub head_ref_name: String, + #[serde(rename = "headRepository")] + pub head_repository: Option, +} + +#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +pub struct PullRequestRepository { + #[serde(rename = "nameWithOwner")] + pub name_with_owner: String, } #[derive(Debug, Clone, PartialEq, Eq)] @@ -383,7 +391,7 @@ mod tests { #[test] fn queries_repository_and_all_existing_pull_requests() -> Result<(), Box> { let fake = FakeGh::new( - "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\npr:list) printf '%s\\n' '[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature\"}]' ;;\nesac", + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\npr:list) printf '%s\\n' '[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]' ;;\nesac", )?; let github = fake.github(); @@ -395,8 +403,15 @@ mod tests { assert_eq!(pull_requests.len(), 1); assert_eq!(pull_requests[0].number, 42); assert_eq!(pull_requests[0].url, "https://github.com/octo/repo/pull/42"); + assert_eq!( + pull_requests[0] + .head_repository + .as_ref() + .map(|repository| repository.name_with_owner.as_str()), + Some("octo/repo") + ); assert!(fake.invocations()?.contains( - "pr\u{1f}list\u{1f}--head\u{1f}feature\u{1f}--state\u{1f}open\u{1f}--limit\u{1f}0\u{1f}--json\u{1f}number,url,title,baseRefName,headRefName\u{1f}\n" + "pr\u{1f}list\u{1f}--head\u{1f}feature\u{1f}--state\u{1f}open\u{1f}--limit\u{1f}0\u{1f}--json\u{1f}number,url,title,baseRefName,headRefName,headRepository\u{1f}\n" )); assert_eq!(fake.prompt_values()?, "1\n1\n1\n1\n1\n1\n"); Ok(()) diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 9b5a12e..43c8e43 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1954,7 +1954,16 @@ impl OperationPlanner { .map_err(open_pull_request_gh_error)?; let existing_url = existing .iter() - .find(|pull_request| pull_request.base_ref_name == base) + .find(|pull_request| { + pull_request.base_ref_name == base + && pull_request.head_ref_name == branch + && pull_request + .head_repository + .as_ref() + .is_some_and(|head_repository| { + head_repository.name_with_owner == repository.name_with_owner + }) + }) .map(|pull_request| pull_request.url.as_str()); let title = branch.clone(); let request = OperationRequest::OpenPullRequest { @@ -2784,7 +2793,16 @@ impl PlanExecutor { .existing_pull_requests(branch) .map_err(StepExecutionError::GitHub)? .into_iter() - .find(|pull_request| pull_request.base_ref_name == *base) + .find(|pull_request| { + pull_request.base_ref_name == *base + && pull_request.head_ref_name == *branch + && pull_request + .head_repository + .as_ref() + .is_some_and(|head_repository| { + head_repository.name_with_owner == current_repository.name_with_owner + }) + }) { return Ok(ExecutionOutput::PullRequest { url: existing.url, @@ -2804,7 +2822,16 @@ impl PlanExecutor { Err(error) => match github.existing_pull_requests(branch) { Ok(existing) => existing .into_iter() - .find(|pull_request| pull_request.base_ref_name == *base) + .find(|pull_request| { + pull_request.base_ref_name == *base + && pull_request.head_ref_name == *branch + && pull_request.head_repository.as_ref().is_some_and( + |head_repository| { + head_repository.name_with_owner + == current_repository.name_with_owner + }, + ) + }) .map(|pull_request| ExecutionOutput::PullRequest { url: pull_request.url, existing: true, @@ -4048,6 +4075,40 @@ mod tests { Ok(()) } + #[test] + fn pull_request_from_another_fork_is_not_surfaced() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-fork-collision")?; + let fake_gh = fake_gh_with_pull_requests( + "open-pr-fork-collision", + "[{\"number\":42,\"url\":\"https://github.com/bob/repo/pull/42\",\"title\":\"Other fork PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"bob/repo\"}}]", + true, + )?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + }; + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!(!operation.plan.preview_text().contains("pull/42")); + let execution = PlanExecutor::with_audit_paths( + &repo, + isolated_store_paths("open-pr-fork-collision-audit")?, + ) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + assert_eq!( + execution.message(), + "Pull request created: https://github.com/octo/repo/pull/43" + ); + assert!( + std::fs::read_to_string(fake_gh.with_file_name("invocations"))?.contains("pr:create") + ); + Ok(()) + } + #[test] fn open_pull_request_plan_encodes_compare_ref_names() { let plan = open_pull_request_plan( @@ -4892,7 +4953,7 @@ mod tests { fn fake_gh(name: &str, existing: bool) -> Result> { let pull_requests = if existing { - "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]" + "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]" } else { "[]" }; From aeeb8edc4b13d0e63bbda5bd9a1048d0f2c94ce0 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:36:58 -0400 Subject: [PATCH 04/16] fix: bind pull requests to push remote --- crates/bitbygit-gh/src/lib.rs | 34 +++++++-- crates/bitbygit-git/src/lib.rs | 20 ++++-- crates/bitbygit-tui/src/lib.rs | 124 ++++++++++++++++++++++++++++++--- 3 files changed, 155 insertions(+), 23 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 039a157..02ba321 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -14,6 +14,7 @@ pub const AUTHENTICATE_GH_GUIDANCE: &str = pub struct GitHub { cwd: PathBuf, executable: PathBuf, + repository: Option, } impl GitHub { @@ -25,6 +26,19 @@ impl GitHub { Self { cwd: cwd.into(), executable: executable.into(), + repository: None, + } + } + + pub fn with_executable_and_repository( + cwd: impl Into, + executable: impl Into, + repository: impl Into, + ) -> Self { + Self { + cwd: cwd.into(), + executable: executable.into(), + repository: Some(repository.into()), } } @@ -50,12 +64,12 @@ impl GitHub { pub fn repository(&self) -> Result { self.ensure_ready()?; - let output = self.run_output(vec![ + let output = self.run_output(self.with_repository(vec![ "repo".to_owned(), "view".to_owned(), "--json".to_owned(), "nameWithOwner,defaultBranchRef".to_owned(), - ])?; + ]))?; let repository: RepositoryResponse = parse_json(&output, "repository")?; let default_branch = repository .default_branch_ref @@ -72,7 +86,7 @@ impl GitHub { pub fn existing_pull_requests(&self, head: &str) -> Result, GhError> { self.ensure_ready()?; - let output = self.run_output(vec![ + let output = self.run_output(self.with_repository(vec![ "pr".to_owned(), "list".to_owned(), "--head".to_owned(), @@ -83,7 +97,7 @@ impl GitHub { "0".to_owned(), "--json".to_owned(), "number,url,title,baseRefName,headRefName,headRepository".to_owned(), - ])?; + ]))?; parse_json(&output, "pull request query") } @@ -93,7 +107,7 @@ impl GitHub { ) -> Result { self.ensure_ready()?; request.validate()?; - let output = self.run_output(vec![ + let output = self.run_output(self.with_repository(vec![ "pr".to_owned(), "create".to_owned(), "--title".to_owned(), @@ -104,7 +118,7 @@ impl GitHub { request.base.clone(), "--head".to_owned(), request.head.clone(), - ])?; + ]))?; let url = output .lines() .map(str::trim) @@ -125,6 +139,14 @@ impl GitHub { } } + fn with_repository(&self, mut args: Vec) -> Vec { + if let Some(repository) = &self.repository { + args.push("--repo".to_owned()); + args.push(repository.clone()); + } + args + } + fn run_status(&self, args: Vec) -> Result<(), GhError> { let output = self.command(&args).output().map_err(|source| { if source.kind() == io::ErrorKind::NotFound { diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 53635b3..d8f3b67 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -279,13 +279,19 @@ impl Git { } pub fn remote_push_urls(&self, remote: &str) -> Result, GitError> { + let push_urls = self.remote_config_urls(remote, "pushurl")?; + if push_urls.is_empty() { + self.remote_config_urls(remote, "url") + } else { + Ok(push_urls) + } + } + + fn remote_config_urls(&self, remote: &str, name: &str) -> Result, GitError> { match self.run_args(vec![ - "remote".to_owned(), - "get-url".to_owned(), - "--push".to_owned(), - "--all".to_owned(), - "--".to_owned(), - remote.to_owned(), + "config".to_owned(), + "--get-all".to_owned(), + format!("remote.{remote}.{name}"), ]) { Ok(output) => Ok(output .stdout @@ -294,7 +300,7 @@ impl Git { .filter(|line| !line.is_empty()) .map(ToOwned::to_owned) .collect()), - Err(GitError::GitFailed { status, .. }) if status.code() == Some(2) => Ok(Vec::new()), + Err(GitError::GitFailed { status, .. }) if status.code() == Some(1) => Ok(Vec::new()), Err(error) => Err(error), } } diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 43c8e43..58eb0b2 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -685,6 +685,7 @@ enum PendingPayload { base: String, title: String, repository: String, + github_repository: String, github_executable: Option, }, } @@ -1940,7 +1941,12 @@ impl OperationPlanner { if remote_oid.as_deref() != Some(local_oid) { return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); } - let github = self.github(); + let github_repository = github_repository_from_push_url(push_url).ok_or_else(|| { + format!( + "Open pull request blocked: remote {remote} does not identify a GitHub repository." + ) + })?; + let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; let base = requested_base.unwrap_or(repository.default_branch); if base == branch { @@ -1988,6 +1994,7 @@ impl OperationPlanner { base, title, repository: repository.name_with_owner, + github_repository, github_executable: self.github_executable.clone(), }), )) @@ -2020,10 +2027,12 @@ impl OperationPlanner { Git::new(self.repo_root.clone()) } - fn github(&self) -> GitHub { + fn github(&self, repository: &str) -> GitHub { match &self.github_executable { - Some(executable) => GitHub::with_executable(&self.repo_root, executable), - None => GitHub::new(&self.repo_root), + Some(executable) => { + GitHub::with_executable_and_repository(&self.repo_root, executable, repository) + } + None => GitHub::with_executable_and_repository(&self.repo_root, "gh", repository), } } } @@ -2056,6 +2065,42 @@ fn single_pull_request_push_url<'a>(remote: &str, urls: &'a [String]) -> Result< } } +fn github_repository_from_push_url(url: &str) -> Option { + let (host, path) = if let Some((_, rest)) = url.split_once("://") { + let (authority, path) = rest.split_once('/')?; + ( + authority + .rsplit_once('@') + .map_or(authority, |(_, host)| host), + path, + ) + } else { + let (authority, path) = url.split_once(':')?; + if authority.contains('/') { + return None; + } + ( + authority + .rsplit_once('@') + .map_or(authority, |(_, host)| host), + path, + ) + }; + let mut components = path.trim_end_matches('/').split('/'); + let owner = components.next()?; + let repository = components.next()?.strip_suffix(".git").unwrap_or_default(); + if host.is_empty() + || owner.is_empty() + || repository.is_empty() + || components.next().is_some() + || owner.contains(['?', '#']) + || repository.contains(['?', '#']) + { + return None; + } + Some(format!("{host}/{owner}/{repository}")) +} + fn validate_push_plan( git: &Git, branch: &str, @@ -2766,6 +2811,7 @@ impl PlanExecutor { base, title, repository, + github_repository, github_executable, } = typed_payload(context, OperationKind::OpenPullRequest)? else { @@ -2779,8 +2825,14 @@ impl PlanExecutor { validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target) .map_err(StepExecutionError::Blocked)?; let github = match github_executable { - Some(executable) => GitHub::with_executable(&self.repo_root, executable), - None => GitHub::new(&self.repo_root), + Some(executable) => GitHub::with_executable_and_repository( + &self.repo_root, + executable, + github_repository, + ), + None => { + GitHub::with_executable_and_repository(&self.repo_root, "gh", github_repository) + } }; let current_repository = github.repository().map_err(StepExecutionError::GitHub)?; if current_repository.name_with_owner != *repository { @@ -4109,6 +4161,48 @@ mod tests { Ok(()) } + #[test] + fn pull_request_gh_commands_follow_the_tracked_fork_remote() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-tracked-fork")?; + git_stdout(&repo, &["remote", "rename", "origin", "fork"])?; + git_stdout( + &repo, + &[ + "remote", + "add", + "origin", + "https://github.com/upstream/repo.git", + ], + )?; + let fake_gh = fake_gh("open-pr-tracked-fork", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + assert!(operation.plan.preview_text().contains("remote: fork")); + let execution = PlanExecutor::with_audit_paths( + &repo, + isolated_store_paths("open-pr-tracked-fork-audit")?, + ) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; + for invocation in invocations.lines().filter(|line| { + line.starts_with("repo:view") + || line.starts_with("pr:list") + || line.starts_with("pr:create") + }) { + assert!(invocation.contains("--repo github.com/octo/repo")); + assert!(!invocation.contains("upstream/repo")); + } + Ok(()) + } + #[test] fn open_pull_request_plan_encodes_compare_ref_names() { let plan = open_pull_request_plan( @@ -4147,8 +4241,7 @@ mod tests { let branch = git_stdout(&repo, &["branch", "--show-current"])? .trim() .to_owned(); - let remote_arg = remote.to_string_lossy().to_string(); - git_stdout(&repo, &["remote", "add", "origin", remote_arg.as_str()])?; + add_github_remote(&repo, "origin", &remote)?; git_stdout(&repo, &["push", "-u", "origin", branch.as_str()])?; let fake_gh = fake_gh("open-pr-invalid-state", false)?; let planner = OperationPlanner { @@ -4945,12 +5038,23 @@ mod tests { std::fs::write(repo.join("file.txt"), "feature\n")?; git_stdout(&repo, &["add", "file.txt"])?; git_stdout(&repo, &["commit", "-m", "feature"])?; - let remote_arg = remote.to_string_lossy().to_string(); - git_stdout(&repo, &["remote", "add", "origin", remote_arg.as_str()])?; + add_github_remote(&repo, "origin", &remote)?; git_stdout(&repo, &["push", "-u", "origin", "feature/open-pr"])?; Ok(repo) } + fn add_github_remote( + repo: &std::path::Path, + name: &str, + bare_remote: &std::path::Path, + ) -> Result<(), Box> { + let remote_url = "https://github.com/octo/repo.git"; + let rewrite_key = format!("url.{}.insteadOf", bare_remote.display()); + git_stdout(repo, &["config", "--add", rewrite_key.as_str(), remote_url])?; + git_stdout(repo, &["remote", "add", name, remote_url])?; + Ok(()) + } + fn fake_gh(name: &str, existing: bool) -> Result> { let pull_requests = if existing { "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]" From e9e48db138115487df06c5d94414a88b2a478fab Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:48:44 -0400 Subject: [PATCH 05/16] fix: resolve effective pull request push URLs --- crates/bitbygit-git/src/lib.rs | 80 +++++++++++++++++++++++----------- crates/bitbygit-tui/src/lib.rs | 47 ++++++++++++++++++-- 2 files changed, 99 insertions(+), 28 deletions(-) diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index d8f3b67..458b4a9 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -279,30 +279,20 @@ impl Git { } pub fn remote_push_urls(&self, remote: &str) -> Result, GitError> { - let push_urls = self.remote_config_urls(remote, "pushurl")?; - if push_urls.is_empty() { - self.remote_config_urls(remote, "url") - } else { - Ok(push_urls) - } - } - - fn remote_config_urls(&self, remote: &str, name: &str) -> Result, GitError> { - match self.run_args(vec![ - "config".to_owned(), - "--get-all".to_owned(), - format!("remote.{remote}.{name}"), - ]) { - Ok(output) => Ok(output - .stdout - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - .map(ToOwned::to_owned) - .collect()), - Err(GitError::GitFailed { status, .. }) if status.code() == Some(1) => Ok(Vec::new()), - Err(error) => Err(error), - } + let output = self.run_args(vec![ + "remote".to_owned(), + "get-url".to_owned(), + "--push".to_owned(), + "--all".to_owned(), + remote.to_owned(), + ])?; + Ok(output + .stdout + .lines() + .map(str::trim) + .filter(|line| !line.is_empty()) + .map(ToOwned::to_owned) + .collect()) } pub fn pull(&self) -> Result { @@ -835,7 +825,10 @@ impl Git { if env::var_os("GIT_SSH_COMMAND").is_none() { command.env( "GIT_SSH_COMMAND", - "ssh -oBatchMode=yes -oNumberOfPasswordPrompts=0 -oKbdInteractiveAuthentication=no -oStrictHostKeyChecking=yes", + format!( + "{} -oBatchMode=yes -oNumberOfPasswordPrompts=0 -oKbdInteractiveAuthentication=no -oStrictHostKeyChecking=yes", + self.configured_ssh_command().unwrap_or_else(|| "ssh".to_owned()) + ), ); } let output = command @@ -873,6 +866,20 @@ impl Git { }) } + fn configured_ssh_command(&self) -> Option { + let output = Command::new("git") + .current_dir(&self.cwd) + .args(["config", "--get", "core.sshCommand"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + let command = String::from_utf8(output.stdout).ok()?; + let command = command.trim(); + (!command.is_empty()).then(|| command.to_owned()) + } + fn run_path_args( &self, args: [&str; N], @@ -2552,6 +2559,29 @@ mod tests { Ok(()) } + #[test] + fn remote_push_urls_applies_push_instead_of_rewrites() -> Result<(), Box> { + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + repo.run([ + "remote", + "add", + "origin", + "https://github.com/upstream/repo.git", + ])?; + repo.run([ + "config", + "--add", + "url.https://github.com/fork/repo.git.pushInsteadOf", + "https://github.com/upstream/repo.git", + ])?; + assert_eq!( + Git::new(repo.path()).remote_push_urls("origin")?, + vec!["https://github.com/fork/repo.git"] + ); + Ok(()) + } + #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 58eb0b2..0af51d7 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -5048,13 +5048,54 @@ mod tests { name: &str, bare_remote: &std::path::Path, ) -> Result<(), Box> { - let remote_url = "https://github.com/octo/repo.git"; - let rewrite_key = format!("url.{}.insteadOf", bare_remote.display()); - git_stdout(repo, &["config", "--add", rewrite_key.as_str(), remote_url])?; + let remote_url = "ssh://git@github.com/octo/repo.git"; + let test_bare_key = format!("remote.{name}.testbare"); git_stdout(repo, &["remote", "add", name, remote_url])?; + git_stdout( + repo, + &[ + "config", + test_bare_key.as_str(), + &bare_remote.display().to_string(), + ], + )?; + git_stdout( + repo, + &[ + "config", + "core.sshCommand", + &test_ssh_command()?.display().to_string(), + ], + )?; Ok(()) } + fn test_ssh_command() -> Result> { + use std::os::unix::fs::PermissionsExt; + use std::sync::OnceLock; + + static COMMAND: OnceLock> = OnceLock::new(); + match COMMAND.get_or_init(|| { + (|| -> Result> { + let root = isolated_temp_root("fake-ssh")?; + std::fs::create_dir_all(&root)?; + let executable = root.join("ssh"); + std::fs::write( + &executable, + "#!/bin/sh\ntarget=$(git config --get-regexp '^remote\\..*\\.testbare$' | cut -d' ' -f2-)\ncase \"$*\" in\n*git-receive-pack*) exec git-receive-pack \"$target\" ;;\n*) exec git-upload-pack \"$target\" ;;\nesac\n", + )?; + let mut permissions = std::fs::metadata(&executable)?.permissions(); + permissions.set_mode(0o755); + std::fs::set_permissions(&executable, permissions)?; + Ok(executable) + })() + .map_err(|error| error.to_string()) + }) { + Ok(command) => Ok(command.clone()), + Err(error) => Err(std::io::Error::other(error.clone()).into()), + } + } + fn fake_gh(name: &str, existing: bool) -> Result> { let pull_requests = if existing { "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]" From 63ccb09bbae7d89e5bf45faae6ebe4dc67464683 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 17:59:54 -0400 Subject: [PATCH 06/16] fix: target upstream for fork pull requests --- crates/bitbygit-tui/src/lib.rs | 145 +++++++++++++++++++++++++++------ 1 file changed, 119 insertions(+), 26 deletions(-) diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 0af51d7..778434f 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -686,6 +686,9 @@ enum PendingPayload { title: String, repository: String, github_repository: String, + head_github_repository: String, + head_repository: String, + head: String, github_executable: Option, }, } @@ -1941,13 +1944,21 @@ impl OperationPlanner { if remote_oid.as_deref() != Some(local_oid) { return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); } - let github_repository = github_repository_from_push_url(push_url).ok_or_else(|| { + let head_github_repository = github_repository_from_push_url(push_url).ok_or_else(|| { format!( "Open pull request blocked: remote {remote} does not identify a GitHub repository." ) })?; + let github_repository = github_base_repository(&git, &head_github_repository)?; let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; + let head_repository = github_repository_name(&head_github_repository) + .ok_or_else(|| { + "Open pull request blocked: source remote does not identify a GitHub repository." + .to_owned() + })? + .to_owned(); + let head = pull_request_head(&head_github_repository, &github_repository, &branch)?; let base = requested_base.unwrap_or(repository.default_branch); if base == branch { return Err( @@ -1956,19 +1967,18 @@ impl OperationPlanner { ); } let existing = github - .existing_pull_requests(&branch) + .existing_pull_requests(&head) .map_err(open_pull_request_gh_error)?; let existing_url = existing .iter() .find(|pull_request| { pull_request.base_ref_name == base && pull_request.head_ref_name == branch - && pull_request - .head_repository - .as_ref() - .is_some_and(|head_repository| { - head_repository.name_with_owner == repository.name_with_owner - }) + && pull_request.head_repository.as_ref().is_some_and( + |pull_request_head_repository| { + pull_request_head_repository.name_with_owner == head_repository + }, + ) }) .map(|pull_request| pull_request.url.as_str()); let title = branch.clone(); @@ -1979,7 +1989,7 @@ impl OperationPlanner { open_pull_request_plan( request, &remote, - &branch, + &head, &base, &title, &repository.name_with_owner, @@ -1995,6 +2005,9 @@ impl OperationPlanner { title, repository: repository.name_with_owner, github_repository, + head_github_repository, + head_repository, + head, github_executable: self.github_executable.clone(), }), )) @@ -2101,6 +2114,65 @@ fn github_repository_from_push_url(url: &str) -> Option { Some(format!("{host}/{owner}/{repository}")) } +fn github_base_repository(git: &Git, head_repository: &str) -> Result { + let upstream = git + .remotes() + .map_err(|error| format!("Unable to prepare pull request base remote: {error}"))? + .into_iter() + .find(|remote| remote.name == "upstream"); + let Some(upstream) = upstream else { + return Ok(head_repository.to_owned()); + }; + let url = upstream + .fetch_url + .as_deref() + .or(upstream.push_url.as_deref()) + .ok_or_else(|| { + "Open pull request blocked: upstream remote has no URL; configure the base repository or remove the remote." + .to_owned() + })?; + github_repository_from_push_url(url).ok_or_else(|| { + "Open pull request blocked: upstream remote does not identify a GitHub repository." + .to_owned() + }) +} + +fn github_repository_name(repository: &str) -> Option<&str> { + let (_, name) = repository.split_once('/')?; + if name.split('/').count() == 2 { + Some(name) + } else { + None + } +} + +fn pull_request_head( + head_repository: &str, + base_repository: &str, + branch: &str, +) -> Result { + if head_repository == base_repository { + return Ok(branch.to_owned()); + } + let (head_host, head_name) = head_repository.split_once('/').ok_or_else(|| { + "Open pull request blocked: source remote does not identify a GitHub repository.".to_owned() + })?; + let (base_host, _) = base_repository.split_once('/').ok_or_else(|| { + "Open pull request blocked: upstream remote does not identify a GitHub repository." + .to_owned() + })?; + if head_host != base_host { + return Err( + "Open pull request blocked: source and upstream remotes use different GitHub hosts." + .to_owned(), + ); + } + let (owner, _) = head_name.split_once('/').ok_or_else(|| { + "Open pull request blocked: source remote does not identify a GitHub repository.".to_owned() + })?; + Ok(format!("{owner}:{branch}")) +} + fn validate_push_plan( git: &Git, branch: &str, @@ -2812,6 +2884,9 @@ impl PlanExecutor { title, repository, github_repository, + head_github_repository, + head_repository, + head, github_executable, } = typed_payload(context, OperationKind::OpenPullRequest)? else { @@ -2824,6 +2899,15 @@ impl PlanExecutor { } validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target) .map_err(StepExecutionError::Blocked)?; + if github_base_repository(git, head_github_repository) + .map_err(StepExecutionError::Blocked)? + != *github_repository + { + return Err(StepExecutionError::Blocked( + "Open pull request blocked: GitHub base repository changed since the plan was shown." + .to_owned(), + )); + } let github = match github_executable { Some(executable) => GitHub::with_executable_and_repository( &self.repo_root, @@ -2842,18 +2926,17 @@ impl PlanExecutor { )); } if let Some(existing) = github - .existing_pull_requests(branch) + .existing_pull_requests(head) .map_err(StepExecutionError::GitHub)? .into_iter() .find(|pull_request| { pull_request.base_ref_name == *base && pull_request.head_ref_name == *branch - && pull_request - .head_repository - .as_ref() - .is_some_and(|head_repository| { - head_repository.name_with_owner == current_repository.name_with_owner - }) + && pull_request.head_repository.as_ref().is_some_and( + |pull_request_head_repository| { + pull_request_head_repository.name_with_owner == *head_repository + }, + ) }) { return Ok(ExecutionOutput::PullRequest { @@ -2865,22 +2948,21 @@ impl PlanExecutor { title: title.clone(), body: String::new(), base: base.clone(), - head: branch.clone(), + head: head.clone(), }) { Ok(created) => Ok(ExecutionOutput::PullRequest { url: created.url, existing: false, }), - Err(error) => match github.existing_pull_requests(branch) { + Err(error) => match github.existing_pull_requests(head) { Ok(existing) => existing .into_iter() .find(|pull_request| { pull_request.base_ref_name == *base && pull_request.head_ref_name == *branch && pull_request.head_repository.as_ref().is_some_and( - |head_repository| { - head_repository.name_with_owner - == current_repository.name_with_owner + |pull_request_head_repository| { + pull_request_head_repository.name_with_owner == *head_repository }, ) }) @@ -4162,7 +4244,7 @@ mod tests { } #[test] - fn pull_request_gh_commands_follow_the_tracked_fork_remote() -> Result<(), Box> { + fn pull_request_gh_commands_target_upstream_for_a_tracked_fork() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-tracked-fork")?; git_stdout(&repo, &["remote", "rename", "origin", "fork"])?; git_stdout( @@ -4170,7 +4252,7 @@ mod tests { &[ "remote", "add", - "origin", + "upstream", "https://github.com/upstream/repo.git", ], )?; @@ -4184,6 +4266,14 @@ mod tests { .plan_request(OperationRequest::OpenPullRequest { base: None }) .map_err(std::io::Error::other)?; assert!(operation.plan.preview_text().contains("remote: fork")); + assert!( + operation + .plan + .preview_text() + .contains("https://github.com/upstream/repo/compare/main...octo%3Afeature/open-pr"), + "{}", + operation.plan.preview_text() + ); let execution = PlanExecutor::with_audit_paths( &repo, isolated_store_paths("open-pr-tracked-fork-audit")?, @@ -4197,8 +4287,11 @@ mod tests { || line.starts_with("pr:list") || line.starts_with("pr:create") }) { - assert!(invocation.contains("--repo github.com/octo/repo")); - assert!(!invocation.contains("upstream/repo")); + assert!(invocation.contains("--repo github.com/upstream/repo")); + assert!(!invocation.contains("--repo github.com/octo/repo")); + if invocation.starts_with("pr:list") || invocation.starts_with("pr:create") { + assert!(invocation.contains("octo:feature/open-pr")); + } } Ok(()) } @@ -5124,7 +5217,7 @@ mod tests { std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}' ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), pull_requests, create_response From 3e8b8e24882878b76b874eeb77b8e405976fe13e Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 18:11:21 -0400 Subject: [PATCH 07/16] fix: use valid gh pull request queries --- crates/bitbygit-gh/src/lib.rs | 83 ++++++++++++++++++++++++---------- crates/bitbygit-tui/src/lib.rs | 30 ++++++++---- 2 files changed, 78 insertions(+), 35 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 02ba321..2b03f10 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -64,12 +64,55 @@ impl GitHub { pub fn repository(&self) -> Result { self.ensure_ready()?; - let output = self.run_output(self.with_repository(vec![ - "repo".to_owned(), - "view".to_owned(), + self.repository_after_ready() + } + + pub fn existing_pull_requests(&self, head: &str) -> Result, GhError> { + self.ensure_ready()?; + let repository = self.repository_after_ready()?; + let (owner, branch) = match head.split_once(':') { + Some((owner, branch)) => (owner, branch), + None => ( + repository + .name_with_owner + .split_once('/') + .map_or("", |(owner, _)| owner), + head, + ), + }; + if owner.is_empty() || branch.is_empty() { + return Err(GhError::InvalidInput { + name: "pull request head", + }); + } + let output = self.run_output(vec![ + "api".to_owned(), + "--method".to_owned(), + "GET".to_owned(), + "--paginate".to_owned(), + "--slurp".to_owned(), + format!("repos/{}/pulls", repository.name_with_owner), + "-f".to_owned(), + "state=open".to_owned(), + "-f".to_owned(), + format!("head={owner}:{branch}"), + "-f".to_owned(), + "per_page=100".to_owned(), + ])?; + let pages: Vec> = parse_json(&output, "pull request query")?; + Ok(pages.into_iter().flatten().collect()) + } + + fn repository_after_ready(&self) -> Result { + let mut args = vec!["repo".to_owned(), "view".to_owned()]; + if let Some(repository) = &self.repository { + args.push(repository.clone()); + } + args.extend([ "--json".to_owned(), "nameWithOwner,defaultBranchRef".to_owned(), - ]))?; + ]); + let output = self.run_output(args)?; let repository: RepositoryResponse = parse_json(&output, "repository")?; let default_branch = repository .default_branch_ref @@ -84,23 +127,6 @@ impl GitHub { }) } - pub fn existing_pull_requests(&self, head: &str) -> Result, GhError> { - self.ensure_ready()?; - let output = self.run_output(self.with_repository(vec![ - "pr".to_owned(), - "list".to_owned(), - "--head".to_owned(), - head.to_owned(), - "--state".to_owned(), - "open".to_owned(), - "--limit".to_owned(), - "0".to_owned(), - "--json".to_owned(), - "number,url,title,baseRefName,headRefName,headRepository".to_owned(), - ]))?; - parse_json(&output, "pull request query") - } - pub fn create_pull_request( &self, request: &CreatePullRequest, @@ -413,9 +439,13 @@ mod tests { #[test] fn queries_repository_and_all_existing_pull_requests() -> Result<(), Box> { let fake = FakeGh::new( - "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\npr:list) printf '%s\\n' '[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]' ;;\nesac", + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]]' ;;\n*) exit 1 ;;\nesac", )?; - let github = fake.github(); + let github = GitHub::with_executable_and_repository( + &fake.path, + &fake.executable, + "github.com/octo/repo", + ); let repository = github.repository()?; let pull_requests = github.existing_pull_requests("feature")?; @@ -433,9 +463,12 @@ mod tests { Some("octo/repo") ); assert!(fake.invocations()?.contains( - "pr\u{1f}list\u{1f}--head\u{1f}feature\u{1f}--state\u{1f}open\u{1f}--limit\u{1f}0\u{1f}--json\u{1f}number,url,title,baseRefName,headRefName,headRepository\u{1f}\n" + "repo\u{1f}view\u{1f}github.com/octo/repo\u{1f}--json\u{1f}nameWithOwner,defaultBranchRef\u{1f}\n" + )); + assert!(fake.invocations()?.contains( + "api\u{1f}--method\u{1f}GET\u{1f}--paginate\u{1f}--slurp\u{1f}repos/octo/repo/pulls\u{1f}-f\u{1f}state=open\u{1f}-f\u{1f}head=octo:feature\u{1f}-f\u{1f}per_page=100\u{1f}\n" )); - assert_eq!(fake.prompt_values()?, "1\n1\n1\n1\n1\n1\n"); + assert_eq!(fake.prompt_values()?, "1\n1\n1\n1\n1\n1\n1\n"); Ok(()) } diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 778434f..77860c6 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -4282,16 +4282,26 @@ mod tests { assert!(execution.succeeded(), "{}", execution.message()); let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; - for invocation in invocations.lines().filter(|line| { - line.starts_with("repo:view") - || line.starts_with("pr:list") - || line.starts_with("pr:create") - }) { + for invocation in invocations + .lines() + .filter(|line| line.starts_with("repo:view")) + { + assert!(invocation.contains("view github.com/upstream/repo")); + assert!(!invocation.contains("--repo")); + } + for invocation in invocations + .lines() + .filter(|line| line.starts_with("api:--method")) + { + assert!(invocation.contains("repos/upstream/repo/pulls")); + assert!(invocation.contains("head=octo:feature/open-pr")); + } + for invocation in invocations + .lines() + .filter(|line| line.starts_with("pr:create")) + { assert!(invocation.contains("--repo github.com/upstream/repo")); - assert!(!invocation.contains("--repo github.com/octo/repo")); - if invocation.starts_with("pr:list") || invocation.starts_with("pr:create") { - assert!(invocation.contains("octo:feature/open-pr")); - } + assert!(invocation.contains("octo:feature/open-pr")); } Ok(()) } @@ -5217,7 +5227,7 @@ mod tests { std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\npr:list) printf '%s\\n' '{}' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), pull_requests, create_response From f6ffac495db92c5c64d5af553258145891ce679d Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Thu, 9 Jul 2026 18:18:24 -0400 Subject: [PATCH 08/16] fix: decode REST pull request responses --- crates/bitbygit-gh/src/lib.rs | 57 ++++++++++++++++++++++++++++------ crates/bitbygit-tui/src/lib.rs | 6 ++-- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 2b03f10..98faaf8 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -99,8 +99,8 @@ impl GitHub { "-f".to_owned(), "per_page=100".to_owned(), ])?; - let pages: Vec> = parse_json(&output, "pull request query")?; - Ok(pages.into_iter().flatten().collect()) + let pages: Vec> = parse_json(&output, "pull request query")?; + Ok(pages.into_iter().flatten().map(PullRequest::from).collect()) } fn repository_after_ready(&self) -> Result { @@ -241,25 +241,60 @@ pub struct Repository { pub default_branch: String, } -#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct PullRequest { pub number: u64, pub url: String, pub title: String, - #[serde(rename = "baseRefName")] pub base_ref_name: String, - #[serde(rename = "headRefName")] pub head_ref_name: String, - #[serde(rename = "headRepository")] pub head_repository: Option, } -#[derive(Debug, Clone, PartialEq, Eq, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq)] pub struct PullRequestRepository { - #[serde(rename = "nameWithOwner")] pub name_with_owner: String, } +#[derive(Deserialize)] +struct RestPullRequest { + number: u64, + html_url: String, + title: String, + base: RestPullRequestBranch, + head: RestPullRequestBranch, +} + +#[derive(Deserialize)] +struct RestPullRequestBranch { + #[serde(rename = "ref")] + reference: String, + repo: Option, +} + +#[derive(Deserialize)] +struct RestPullRequestRepository { + full_name: String, +} + +impl From for PullRequest { + fn from(pull_request: RestPullRequest) -> Self { + Self { + number: pull_request.number, + url: pull_request.html_url, + title: pull_request.title, + base_ref_name: pull_request.base.reference, + head_ref_name: pull_request.head.reference, + head_repository: pull_request + .head + .repo + .map(|repository| PullRequestRepository { + name_with_owner: repository.full_name, + }), + } + } +} + #[derive(Debug, Clone, PartialEq, Eq)] pub struct CreatePullRequest { pub title: String, @@ -437,9 +472,9 @@ mod tests { } #[test] - fn queries_repository_and_all_existing_pull_requests() -> Result<(), Box> { + fn decodes_rest_existing_pull_request_responses() -> Result<(), Box> { let fake = FakeGh::new( - "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]]' ;;\n*) exit 1 ;;\nesac", + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature\",\"repo\":{\"full_name\":\"octo/repo\"}}}]]' ;;\n*) exit 1 ;;\nesac", )?; let github = GitHub::with_executable_and_repository( &fake.path, @@ -455,6 +490,8 @@ mod tests { assert_eq!(pull_requests.len(), 1); assert_eq!(pull_requests[0].number, 42); assert_eq!(pull_requests[0].url, "https://github.com/octo/repo/pull/42"); + assert_eq!(pull_requests[0].base_ref_name, "main"); + assert_eq!(pull_requests[0].head_ref_name, "feature"); assert_eq!( pull_requests[0] .head_repository diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 77860c6..2dc9c1d 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -4175,7 +4175,7 @@ mod tests { let repo = pushed_branch_repo("open-pr-other-base")?; let fake_gh = fake_gh_with_pull_requests( "open-pr-other-base", - "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\"}]", + "[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature/open-pr\",\"repo\":{\"full_name\":\"octo/repo\"}}}]", false, )?; let planner = OperationPlanner { @@ -4214,7 +4214,7 @@ mod tests { let repo = pushed_branch_repo("open-pr-fork-collision")?; let fake_gh = fake_gh_with_pull_requests( "open-pr-fork-collision", - "[{\"number\":42,\"url\":\"https://github.com/bob/repo/pull/42\",\"title\":\"Other fork PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"bob/repo\"}}]", + "[{\"number\":42,\"html_url\":\"https://github.com/bob/repo/pull/42\",\"title\":\"Other fork PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature/open-pr\",\"repo\":{\"full_name\":\"bob/repo\"}}}]", true, )?; let planner = OperationPlanner { @@ -5201,7 +5201,7 @@ mod tests { fn fake_gh(name: &str, existing: bool) -> Result> { let pull_requests = if existing { - "[{\"number\":42,\"url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"baseRefName\":\"main\",\"headRefName\":\"feature/open-pr\",\"headRepository\":{\"nameWithOwner\":\"octo/repo\"}}]" + "[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature/open-pr\",\"repo\":{\"full_name\":\"octo/repo\"}}}]" } else { "[]" }; From d93cd2b119be2a4f335c4c555526ce046df39e39 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 11:30:43 -0400 Subject: [PATCH 09/16] fix: ignore repository SSH commands --- crates/bitbygit-git/src/lib.rs | 81 +++++++++++++++++-------- crates/bitbygit-tui/src/lib.rs | 104 +++++++++++++++++++++++++++------ 2 files changed, 142 insertions(+), 43 deletions(-) diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 458b4a9..5e46d70 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -1,5 +1,4 @@ use std::collections::BTreeMap; -use std::env; use std::error::Error; use std::ffi::OsString; use std::fmt::{self, Display, Formatter}; @@ -13,6 +12,7 @@ use std::os::unix::ffi::OsStringExt; const ZERO_OID: &str = "0000000000000000000000000000000000000000"; const EMPTY_TREE_OID: &str = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"; +const SSH_OPTIONS: &str = "-oBatchMode=yes -oNumberOfPasswordPrompts=0 -oKbdInteractiveAuthentication=no -oStrictHostKeyChecking=yes"; const COMMIT_HOOKS: &[&str] = &[ "pre-commit", "prepare-commit-msg", @@ -23,11 +23,25 @@ const COMMIT_HOOKS: &[&str] = &[ #[derive(Debug, Clone)] pub struct Git { cwd: PathBuf, + ssh_executable: Option, } impl Git { pub fn new(cwd: impl Into) -> Self { - Self { cwd: cwd.into() } + Self { + cwd: cwd.into(), + ssh_executable: None, + } + } + + pub fn with_ssh_executable( + cwd: impl Into, + ssh_executable: impl Into, + ) -> Self { + Self { + cwd: cwd.into(), + ssh_executable: Some(ssh_executable.into()), + } } pub fn repository(&self) -> Result { @@ -822,15 +836,12 @@ impl Git { .env("GIT_ASKPASS", "") .env("SSH_ASKPASS", "") .env("SSH_ASKPASS_REQUIRE", "never"); - if env::var_os("GIT_SSH_COMMAND").is_none() { - command.env( - "GIT_SSH_COMMAND", - format!( - "{} -oBatchMode=yes -oNumberOfPasswordPrompts=0 -oKbdInteractiveAuthentication=no -oStrictHostKeyChecking=yes", - self.configured_ssh_command().unwrap_or_else(|| "ssh".to_owned()) - ), - ); - } + let ssh_executable = self + .ssh_executable + .as_ref() + .map(|path| shell_quote(&path.to_string_lossy())) + .unwrap_or_else(|| "ssh".to_owned()); + command.env("GIT_SSH_COMMAND", format!("{ssh_executable} {SSH_OPTIONS}")); let output = command .args(&args) .output() @@ -866,20 +877,6 @@ impl Git { }) } - fn configured_ssh_command(&self) -> Option { - let output = Command::new("git") - .current_dir(&self.cwd) - .args(["config", "--get", "core.sshCommand"]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - let command = String::from_utf8(output.stdout).ok()?; - let command = command.trim(); - (!command.is_empty()).then(|| command.to_owned()) - } - fn run_path_args( &self, args: [&str; N], @@ -1610,6 +1607,10 @@ fn split_once_byte(value: &[u8], delimiter: u8) -> Option<(&[u8], &[u8])> { Some((&value[..index], &value[index + 1..])) } +fn shell_quote(value: &str) -> String { + format!("'{}'", value.replace('\'', "'\\''")) +} + #[cfg(unix)] fn path_from_bytes(value: &[u8]) -> PathBuf { PathBuf::from(OsString::from_vec(value.to_vec())) @@ -2582,6 +2583,36 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn remote_url_head_oid_ignores_configured_ssh_command() -> Result<(), Box> { + use std::os::unix::fs::PermissionsExt; + + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + let marker = repo.path().join("configured-ssh-ran"); + let configured_ssh = repo.path().join("configured-ssh"); + fs::write( + &configured_ssh, + format!("#!/bin/sh\ntouch '{}'\nexit 1\n", marker.display()), + )?; + let mut permissions = fs::metadata(&configured_ssh)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&configured_ssh, permissions)?; + repo.run_args(&[ + "config", + "core.sshCommand", + &configured_ssh.display().to_string(), + ])?; + + let result = + Git::new(repo.path()).remote_url_head_oid("ssh://git@127.0.0.1:1/repo.git", "main"); + + assert!(result.is_err()); + assert!(!marker.exists()); + Ok(()) + } + #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 2dc9c1d..7547956 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1514,6 +1514,8 @@ fn short_oid(oid: &str) -> String { struct OperationPlanner { repo_root: PathBuf, github_executable: Option, + #[cfg(test)] + ssh_executable: Option, } impl OperationPlanner { @@ -1521,6 +1523,8 @@ impl OperationPlanner { Self { repo_root: current_dir(), github_executable: None, + #[cfg(test)] + ssh_executable: None, } } @@ -1529,6 +1533,7 @@ impl OperationPlanner { Self { repo_root: repo_root.into(), github_executable: None, + ssh_executable: None, } } @@ -2037,6 +2042,10 @@ impl OperationPlanner { } fn git(&self) -> Git { + #[cfg(test)] + if let Some(executable) = &self.ssh_executable { + return Git::with_ssh_executable(self.repo_root.clone(), executable); + } Git::new(self.repo_root.clone()) } @@ -2477,6 +2486,8 @@ impl ExecutionContext { struct PlanExecutor { repo_root: PathBuf, audit: AuditDestination, + #[cfg(test)] + ssh_executable: Option, } impl PlanExecutor { @@ -2484,6 +2495,8 @@ impl PlanExecutor { Self { repo_root: current_dir(), audit: AuditDestination::Environment, + #[cfg(test)] + ssh_executable: None, } } @@ -2492,6 +2505,20 @@ impl PlanExecutor { Self { repo_root: repo_root.into(), audit: AuditDestination::Paths(paths), + ssh_executable: None, + } + } + + #[cfg(test)] + fn with_audit_paths_and_ssh( + repo_root: impl Into, + paths: StorePaths, + ssh_executable: impl Into, + ) -> Self { + Self { + repo_root: repo_root.into(), + audit: AuditDestination::Paths(paths), + ssh_executable: Some(ssh_executable.into()), } } @@ -2594,6 +2621,10 @@ impl PlanExecutor { } fn git(&self) -> Git { + #[cfg(test)] + if let Some(executable) = &self.ssh_executable { + return Git::with_ssh_executable(self.repo_root.clone(), executable); + } Git::new(self.repo_root.clone()) } @@ -3004,10 +3035,14 @@ impl PromptSequenceExecutor { let planner = OperationPlanner { repo_root: self.repo_root.clone(), github_executable: None, + #[cfg(test)] + ssh_executable: None, }; let executor = PlanExecutor { repo_root: self.repo_root.clone(), audit: self.audit.clone(), + #[cfg(test)] + ssh_executable: None, }; let mut step_results = Vec::with_capacity(total_steps); @@ -4100,6 +4135,7 @@ mod tests { let planner = OperationPlanner { repo_root: repo.clone(), github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), }; let operation = planner @@ -4118,8 +4154,9 @@ mod tests { ConfirmationRequirement::VisiblePlan ); let paths = isolated_store_paths("open-pr-create-audit")?; - let execution = PlanExecutor::with_audit_paths(&repo, paths.clone()) - .execute(&operation.plan, operation.context); + let execution = + PlanExecutor::with_audit_paths_and_ssh(&repo, paths.clone(), test_ssh_command()?) + .execute(&operation.plan, operation.context); assert!(execution.succeeded(), "{}", execution.message()); assert_eq!( @@ -4143,6 +4180,7 @@ mod tests { let planner = OperationPlanner { repo_root: repo.clone(), github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), }; let operation = planner .plan_request(OperationRequest::OpenPullRequest { base: None }) @@ -4154,9 +4192,12 @@ mod tests { .preview_text() .contains("target: https://github.com/octo/repo/pull/42") ); - let execution = - PlanExecutor::with_audit_paths(&repo, isolated_store_paths("open-pr-existing-audit")?) - .execute(&operation.plan, operation.context); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-existing-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); assert!(execution.succeeded(), "{}", execution.message()); assert_eq!( @@ -4181,6 +4222,7 @@ mod tests { let planner = OperationPlanner { repo_root: repo.clone(), github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), }; let operation = planner .plan_request(OperationRequest::OpenPullRequest { @@ -4194,9 +4236,10 @@ mod tests { .preview_text() .contains("https://github.com/octo/repo/compare/release...feature/open-pr") ); - let execution = PlanExecutor::with_audit_paths( + let execution = PlanExecutor::with_audit_paths_and_ssh( &repo, isolated_store_paths("open-pr-other-base-audit")?, + test_ssh_command()?, ) .execute(&operation.plan, operation.context); @@ -4220,15 +4263,17 @@ mod tests { let planner = OperationPlanner { repo_root: repo.clone(), github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), }; let operation = planner .plan_request(OperationRequest::OpenPullRequest { base: None }) .map_err(std::io::Error::other)?; assert!(!operation.plan.preview_text().contains("pull/42")); - let execution = PlanExecutor::with_audit_paths( + let execution = PlanExecutor::with_audit_paths_and_ssh( &repo, isolated_store_paths("open-pr-fork-collision-audit")?, + test_ssh_command()?, ) .execute(&operation.plan, operation.context); @@ -4260,6 +4305,7 @@ mod tests { let planner = OperationPlanner { repo_root: repo.clone(), github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), }; let operation = planner @@ -4274,9 +4320,10 @@ mod tests { "{}", operation.plan.preview_text() ); - let execution = PlanExecutor::with_audit_paths( + let execution = PlanExecutor::with_audit_paths_and_ssh( &repo, isolated_store_paths("open-pr-tracked-fork-audit")?, + test_ssh_command()?, ) .execute(&operation.plan, operation.context); @@ -4345,11 +4392,16 @@ mod tests { .trim() .to_owned(); add_github_remote(&repo, "origin", &remote)?; - git_stdout(&repo, &["push", "-u", "origin", branch.as_str()])?; + git_stdout_with_ssh( + &repo, + &["push", "-u", "origin", branch.as_str()], + &test_ssh_command()?, + )?; let fake_gh = fake_gh("open-pr-invalid-state", false)?; let planner = OperationPlanner { repo_root: repo, github_executable: Some(fake_gh), + ssh_executable: Some(test_ssh_command()?), }; let error = match planner.plan_request(OperationRequest::OpenPullRequest { base: Some(branch) }) { @@ -5142,7 +5194,11 @@ mod tests { git_stdout(&repo, &["add", "file.txt"])?; git_stdout(&repo, &["commit", "-m", "feature"])?; add_github_remote(&repo, "origin", &remote)?; - git_stdout(&repo, &["push", "-u", "origin", "feature/open-pr"])?; + git_stdout_with_ssh( + &repo, + &["push", "-u", "origin", "feature/open-pr"], + &test_ssh_command()?, + )?; Ok(repo) } @@ -5162,17 +5218,29 @@ mod tests { &bare_remote.display().to_string(), ], )?; - git_stdout( - repo, - &[ - "config", - "core.sshCommand", - &test_ssh_command()?.display().to_string(), - ], - )?; Ok(()) } + fn git_stdout_with_ssh( + repo: &std::path::Path, + args: &[&str], + ssh_executable: &std::path::Path, + ) -> Result> { + let output = std::process::Command::new("git") + .current_dir(repo) + .env("GIT_SSH_COMMAND", ssh_executable) + .args(args) + .output()?; + if !output.status.success() { + return Err(std::io::Error::other(format!( + "git command failed: {}", + String::from_utf8_lossy(&output.stderr) + )) + .into()); + } + Ok(String::from_utf8(output.stdout)?) + } + fn test_ssh_command() -> Result> { use std::os::unix::fs::PermissionsExt; use std::sync::OnceLock; From bace2b98247f3dfb04b59fc2d5e839ec8f3dc721 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 11:44:45 -0400 Subject: [PATCH 10/16] fix: harden pull request planning --- crates/bitbygit-gh/src/lib.rs | 63 +++++++++ crates/bitbygit-git/src/lib.rs | 64 +++++++-- crates/bitbygit-tui/src/lib.rs | 237 +++++++++++++++++++++++++-------- 3 files changed, 301 insertions(+), 63 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 98faaf8..09b1aa9 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -103,6 +103,34 @@ impl GitHub { Ok(pages.into_iter().flatten().map(PullRequest::from).collect()) } + pub fn branch_exists(&self, branch: &str) -> Result { + self.ensure_ready()?; + if branch.trim().is_empty() { + return Err(GhError::InvalidInput { + name: "base branch", + }); + } + let repository = self.repository_after_ready()?; + let output = self.run_output(vec![ + "api".to_owned(), + "--method".to_owned(), + "GET".to_owned(), + "--paginate".to_owned(), + "--slurp".to_owned(), + format!( + "repos/{}/git/matching-refs/heads/{}", + repository.name_with_owner, + url_path_component(branch) + ), + ])?; + let pages: Vec> = parse_json(&output, "branch query")?; + let expected = format!("refs/heads/{branch}"); + Ok(pages + .into_iter() + .flatten() + .any(|reference| reference.name == expected)) + } + fn repository_after_ready(&self) -> Result { let mut args = vec!["repo".to_owned(), "view".to_owned()]; if let Some(repository) = &self.repository { @@ -277,6 +305,12 @@ struct RestPullRequestRepository { full_name: String, } +#[derive(Deserialize)] +struct RestReference { + #[serde(rename = "ref")] + name: String, +} + impl From for PullRequest { fn from(pull_request: RestPullRequest) -> Self { Self { @@ -388,6 +422,25 @@ fn parse_json Deserialize<'de>>( serde_json::from_str(output).map_err(|_| GhError::InvalidOutput { operation }) } +fn url_path_component(value: &str) -> String { + const HEX: &[u8; 16] = b"0123456789ABCDEF"; + + let mut encoded = String::with_capacity(value.len()); + for byte in value.bytes() { + match byte { + b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' => { + encoded.push(char::from(byte)); + } + _ => { + encoded.push('%'); + encoded.push(char::from(HEX[usize::from(byte >> 4)])); + encoded.push(char::from(HEX[usize::from(byte & 0x0f)])); + } + } + } + encoded +} + #[cfg(all(test, unix))] mod tests { use super::*; @@ -509,6 +562,16 @@ mod tests { Ok(()) } + #[test] + fn checks_encoded_base_branch_reference() -> Result<(), Box> { + let fake = FakeGh::new( + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$6\" = repos/octo/repo/git/matching-refs/heads/release%2Fnext ] || exit 1; printf '%s\\n' '[[{\"ref\":\"refs/heads/release/next\"}]]' ;;\n*) exit 1 ;;\nesac", + )?; + + assert!(fake.github().branch_exists("release/next")?); + Ok(()) + } + #[test] fn creates_pull_request_with_literal_shell_metacharacters_in_title() -> Result<(), Box> { diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 5e46d70..600c9d0 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -269,21 +269,37 @@ impl Git { } pub fn remote_head_oid(&self, remote: &str, branch: &str) -> Result, GitError> { - self.head_oid_at(remote, branch) + self.head_oid_at(remote, branch, None) } pub fn remote_url_head_oid(&self, url: &str, branch: &str) -> Result, GitError> { - self.head_oid_at(url, branch) + self.head_oid_at(url, branch, None) } - fn head_oid_at(&self, target: &str, branch: &str) -> Result, GitError> { - let output = self.run_args(vec![ - "ls-remote".to_owned(), - "--heads".to_owned(), - "--".to_owned(), - target.to_owned(), - format!("refs/heads/{branch}"), - ])?; + pub fn github_remote_url_head_oid( + &self, + url: &str, + branch: &str, + ) -> Result, GitError> { + self.head_oid_at(url, branch, Some("https:ssh")) + } + + fn head_oid_at( + &self, + target: &str, + branch: &str, + allowed_protocols: Option<&str>, + ) -> Result, GitError> { + let output = self.run_args_with_allowed_protocols( + vec![ + "ls-remote".to_owned(), + "--heads".to_owned(), + "--".to_owned(), + target.to_owned(), + format!("refs/heads/{branch}"), + ], + allowed_protocols, + )?; Ok(output .stdout .lines() @@ -829,6 +845,14 @@ impl Git { } fn run_args(&self, args: Vec) -> Result { + self.run_args_with_allowed_protocols(args, None) + } + + fn run_args_with_allowed_protocols( + &self, + args: Vec, + allowed_protocols: Option<&str>, + ) -> Result { let mut command = Command::new("git"); command .current_dir(&self.cwd) @@ -842,6 +866,9 @@ impl Git { .map(|path| shell_quote(&path.to_string_lossy())) .unwrap_or_else(|| "ssh".to_owned()); command.env("GIT_SSH_COMMAND", format!("{ssh_executable} {SSH_OPTIONS}")); + if let Some(protocols) = allowed_protocols { + command.env("GIT_ALLOW_PROTOCOL", protocols); + } let output = command .args(&args) .output() @@ -2613,6 +2640,23 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn github_remote_url_head_oid_rejects_ext_transport_even_when_enabled() + -> Result<(), Box> { + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + repo.run(["config", "protocol.ext.allow", "always"])?; + let marker = repo.path().join("ext-ran"); + + let result = Git::new(repo.path()) + .github_remote_url_head_oid(&format!("ext::touch {}", marker.display()), "main"); + + assert!(result.is_err()); + assert!(!marker.exists()); + Ok(()) + } + #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 7547956..f809f24 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1936,6 +1936,11 @@ impl OperationPlanner { .remote_push_urls(&remote) .map_err(|error| format!("Unable to prepare pull request remote: {error}"))?; let push_url = single_pull_request_push_url(&remote, &remote_urls)?; + let head_github_repository = github_repository_from_push_url(push_url).ok_or_else(|| { + format!( + "Open pull request blocked: remote {remote} does not identify a GitHub repository." + ) + })?; let target = git .head_target() .map_err(|error| format!("Unable to snapshot pull request branch: {error}"))?; @@ -1944,16 +1949,11 @@ impl OperationPlanner { .to_owned() })?; let remote_oid = git - .remote_url_head_oid(push_url, &branch) + .github_remote_url_head_oid(push_url, &branch) .map_err(|error| format!("Unable to verify pushed branch: {error}"))?; if remote_oid.as_deref() != Some(local_oid) { return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); } - let head_github_repository = github_repository_from_push_url(push_url).ok_or_else(|| { - format!( - "Open pull request blocked: remote {remote} does not identify a GitHub repository." - ) - })?; let github_repository = github_base_repository(&git, &head_github_repository)?; let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; @@ -1971,20 +1971,19 @@ impl OperationPlanner { .to_owned(), ); } + if !github + .branch_exists(&base) + .map_err(open_pull_request_gh_error)? + { + return Err(format!( + "Open pull request blocked: base branch {base} was not found in {}.", + repository.name_with_owner + )); + } let existing = github .existing_pull_requests(&head) .map_err(open_pull_request_gh_error)?; - let existing_url = existing - .iter() - .find(|pull_request| { - pull_request.base_ref_name == base - && pull_request.head_ref_name == branch - && pull_request.head_repository.as_ref().is_some_and( - |pull_request_head_repository| { - pull_request_head_repository.name_with_owner == head_repository - }, - ) - }) + let existing_url = matching_pull_request(&existing, &base, &branch, &head_repository) .map(|pull_request| pull_request.url.as_str()); let title = branch.clone(); let request = OperationRequest::OpenPullRequest { @@ -2088,7 +2087,10 @@ fn single_pull_request_push_url<'a>(remote: &str, urls: &'a [String]) -> Result< } fn github_repository_from_push_url(url: &str) -> Option { - let (host, path) = if let Some((_, rest)) = url.split_once("://") { + let (host, path) = if let Some((scheme, rest)) = url.split_once("://") { + if !scheme.eq_ignore_ascii_case("https") && !scheme.eq_ignore_ascii_case("ssh") { + return None; + } let (authority, path) = rest.split_once('/')?; ( authority @@ -2108,6 +2110,9 @@ fn github_repository_from_push_url(url: &str) -> Option { path, ) }; + if !host.eq_ignore_ascii_case("github.com") { + return None; + } let mut components = path.trim_end_matches('/').split('/'); let owner = components.next()?; let repository = components.next()?.strip_suffix(".git").unwrap_or_default(); @@ -2120,7 +2125,7 @@ fn github_repository_from_push_url(url: &str) -> Option { { return None; } - Some(format!("{host}/{owner}/{repository}")) + Some(format!("github.com/{owner}/{repository}")) } fn github_base_repository(git: &Git, head_repository: &str) -> Result { @@ -2160,7 +2165,7 @@ fn pull_request_head( base_repository: &str, branch: &str, ) -> Result { - if head_repository == base_repository { + if head_repository.eq_ignore_ascii_case(base_repository) { return Ok(branch.to_owned()); } let (head_host, head_name) = head_repository.split_once('/').ok_or_else(|| { @@ -2170,7 +2175,7 @@ fn pull_request_head( "Open pull request blocked: upstream remote does not identify a GitHub repository." .to_owned() })?; - if head_host != base_host { + if !head_host.eq_ignore_ascii_case(base_host) { return Err( "Open pull request blocked: source and upstream remotes use different GitHub hosts." .to_owned(), @@ -2182,6 +2187,26 @@ fn pull_request_head( Ok(format!("{owner}:{branch}")) } +fn matching_pull_request<'a>( + pull_requests: &'a [bitbygit_gh::PullRequest], + base: &str, + branch: &str, + head_repository: &str, +) -> Option<&'a bitbygit_gh::PullRequest> { + pull_requests.iter().find(|pull_request| { + pull_request.base_ref_name == base + && pull_request.head_ref_name == branch + && pull_request + .head_repository + .as_ref() + .is_some_and(|pull_request_head_repository| { + pull_request_head_repository + .name_with_owner + .eq_ignore_ascii_case(head_repository) + }) + }) +} + fn validate_push_plan( git: &Git, branch: &str, @@ -2320,7 +2345,7 @@ fn validate_open_pull_request_plan( .as_deref() .ok_or_else(|| "Open pull request blocked: planned branch has no commit.".to_owned())?; let remote_oid = git - .remote_url_head_oid(push_url, branch) + .github_remote_url_head_oid(push_url, branch) .map_err(|error| format!("Unable to revalidate pushed branch: {error}"))?; if remote_oid.as_deref() != Some(local_oid) { return Err( @@ -2930,9 +2955,9 @@ impl PlanExecutor { } validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target) .map_err(StepExecutionError::Blocked)?; - if github_base_repository(git, head_github_repository) + if !github_base_repository(git, head_github_repository) .map_err(StepExecutionError::Blocked)? - != *github_repository + .eq_ignore_ascii_case(github_repository) { return Err(StepExecutionError::Blocked( "Open pull request blocked: GitHub base repository changed since the plan was shown." @@ -2950,31 +2975,32 @@ impl PlanExecutor { } }; let current_repository = github.repository().map_err(StepExecutionError::GitHub)?; - if current_repository.name_with_owner != *repository { + if !current_repository + .name_with_owner + .eq_ignore_ascii_case(repository) + { return Err(StepExecutionError::Blocked( "Open pull request blocked: GitHub repository changed since the plan was shown." .to_owned(), )); } - if let Some(existing) = github + let existing = github .existing_pull_requests(head) - .map_err(StepExecutionError::GitHub)? - .into_iter() - .find(|pull_request| { - pull_request.base_ref_name == *base - && pull_request.head_ref_name == *branch - && pull_request.head_repository.as_ref().is_some_and( - |pull_request_head_repository| { - pull_request_head_repository.name_with_owner == *head_repository - }, - ) - }) - { + .map_err(StepExecutionError::GitHub)?; + if let Some(existing) = matching_pull_request(&existing, base, branch, head_repository) { return Ok(ExecutionOutput::PullRequest { - url: existing.url, + url: existing.url.clone(), existing: true, }); } + if !github + .branch_exists(base) + .map_err(StepExecutionError::GitHub)? + { + return Err(StepExecutionError::Blocked(format!( + "Open pull request blocked: base branch {base} no longer exists in {repository}." + ))); + } match github.create_pull_request(&CreatePullRequest { title: title.clone(), body: String::new(), @@ -2986,19 +3012,9 @@ impl PlanExecutor { existing: false, }), Err(error) => match github.existing_pull_requests(head) { - Ok(existing) => existing - .into_iter() - .find(|pull_request| { - pull_request.base_ref_name == *base - && pull_request.head_ref_name == *branch - && pull_request.head_repository.as_ref().is_some_and( - |pull_request_head_repository| { - pull_request_head_repository.name_with_owner == *head_repository - }, - ) - }) + Ok(existing) => matching_pull_request(&existing, base, branch, head_repository) .map(|pull_request| ExecutionOutput::PullRequest { - url: pull_request.url, + url: pull_request.url.clone(), existing: true, }) .ok_or(StepExecutionError::GitHub(error)), @@ -4210,6 +4226,119 @@ mod tests { Ok(()) } + #[test] + fn existing_pull_request_repository_match_is_case_insensitive() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-existing-mixed-case")?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "origin", + "ssh://git@github.com/OCTO/REPO.git", + ], + )?; + let fake_gh = fake_gh("open-pr-existing-mixed-case", true)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!(operation.plan.preview_text().contains("pull/42")); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-existing-mixed-case-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + assert!(execution.succeeded(), "{}", execution.message()); + assert!(execution.message().contains("already open")); + assert!( + !std::fs::read_to_string(fake_gh.with_file_name("invocations"))?.contains("pr:create") + ); + Ok(()) + } + + #[test] + fn open_pull_request_plan_rejects_missing_base_branch() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-missing-base")?; + let fake_gh = fake_gh("open-pr-missing-base", false)?; + let planner = OperationPlanner { + repo_root: repo, + github_executable: Some(fake_gh), + ssh_executable: Some(test_ssh_command()?), + }; + + let error = match planner.plan_request(OperationRequest::OpenPullRequest { + base: Some("missing".to_owned()), + }) { + Ok(_) => return Err(std::io::Error::other("missing base must fail closed").into()), + Err(error) => error, + }; + + assert!(error.contains("base branch missing was not found")); + Ok(()) + } + + #[test] + fn open_pull_request_execution_rejects_deleted_base_branch() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-deleted-base")?; + let fake_gh = fake_gh("open-pr-deleted-base", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + std::fs::write(fake_gh.with_file_name("base-missing"), "")?; + + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-deleted-base-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + + assert!(!execution.succeeded()); + assert!( + execution + .message() + .contains("base branch main no longer exists") + ); + assert!( + !std::fs::read_to_string(fake_gh.with_file_name("invocations"))?.contains("pr:create") + ); + Ok(()) + } + + #[cfg(unix)] + #[test] + fn open_pull_request_plan_never_launches_ext_transport() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-ext-transport")?; + let marker = repo.join("ext-ran"); + let push_url = format!("ext::touch {}", marker.display()); + git_stdout(&repo, &["remote", "set-url", "--push", "origin", &push_url])?; + git_stdout(&repo, &["config", "protocol.ext.allow", "always"])?; + + let error = match OperationPlanner::new(&repo) + .plan_request(OperationRequest::OpenPullRequest { base: None }) + { + Ok(_) => return Err(std::io::Error::other("ext transport must fail closed").into()), + Err(error) => error, + }; + + assert!(error.contains("does not identify a GitHub repository")); + assert!(!marker.exists()); + Ok(()) + } + #[test] fn pull_request_for_other_base_is_not_surfaced_after_create_failure() -> Result<(), Box> { @@ -4338,7 +4467,7 @@ mod tests { } for invocation in invocations .lines() - .filter(|line| line.starts_with("api:--method")) + .filter(|line| line.starts_with("api:--method") && line.contains("/pulls")) { assert!(invocation.contains("repos/upstream/repo/pulls")); assert!(invocation.contains("head=octo:feature/open-pr")); @@ -5287,6 +5416,7 @@ mod tests { std::fs::create_dir_all(&root)?; let executable = root.join("gh"); let invocations = root.join("invocations"); + let base_missing = root.join("base-missing"); let create_response = if create_succeeds { "printf '%s\\n' 'https://github.com/octo/repo/pull/43' ;;" } else { @@ -5295,8 +5425,9 @@ mod tests { std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method)\n case \"$6\" in\n */git/matching-refs/heads/*)\n if [ -f '{}' ]; then printf '%s\\n' '[[]]'; else printf '%s\\n' '[[{{\"ref\":\"refs/heads/main\"}},{{\"ref\":\"refs/heads/release\"}}]]'; fi ;;\n */pulls) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\n *) exit 1 ;;\n esac ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), + base_missing.display(), pull_requests, create_response ), From c0bfed731b49cfa892d5feab7660fb7934614259 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 11:54:48 -0400 Subject: [PATCH 11/16] fix: address pull request review concerns --- crates/bitbygit-tui/src/lib.rs | 188 ++++++++++++++++++++++++++------- 1 file changed, 152 insertions(+), 36 deletions(-) diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index f809f24..9522ce6 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1965,7 +1965,7 @@ impl OperationPlanner { .to_owned(); let head = pull_request_head(&head_github_repository, &github_repository, &branch)?; let base = requested_base.unwrap_or(repository.default_branch); - if base == branch { + if head_github_repository.eq_ignore_ascii_case(&github_repository) && base == branch { return Err( "Open pull request blocked: base branch must differ from the current branch." .to_owned(), @@ -2115,7 +2115,8 @@ fn github_repository_from_push_url(url: &str) -> Option { } let mut components = path.trim_end_matches('/').split('/'); let owner = components.next()?; - let repository = components.next()?.strip_suffix(".git").unwrap_or_default(); + let repository = components.next()?; + let repository = repository.strip_suffix(".git").unwrap_or(repository); if host.is_empty() || owner.is_empty() || repository.is_empty() @@ -2288,70 +2289,66 @@ fn validate_open_pull_request_plan( remote: &str, remote_urls: &[String], target: &HeadTarget, -) -> Result<(), String> { - if git - .head_target() - .map_err(|error| format!("Unable to revalidate pull request target: {error}"))? - != *target - { - return Err( +) -> Result<(), StepExecutionError> { + if git.head_target().map_err(StepExecutionError::Git)? != *target { + return Err(StepExecutionError::Blocked( "Open pull request blocked: branch target changed since the plan was shown.".to_owned(), - ); + )); } - let status = git - .status() - .map_err(|error| format!("Unable to revalidate pull request plan: {error}"))?; - if branch_name(&status.branch)? != branch { - return Err( + let status = git.status().map_err(StepExecutionError::Git)?; + if branch_name(&status.branch).map_err(StepExecutionError::Blocked)? != branch { + return Err(StepExecutionError::Blocked( "Open pull request blocked: current branch changed since the plan was shown." .to_owned(), - ); + )); } if status.branch.upstream.as_deref() != Some(expected_upstream) { - return Err( + return Err(StepExecutionError::Blocked( "Open pull request blocked: upstream changed since the plan was shown.".to_owned(), - ); + )); } if status.branch.ahead > 0 || status.branch.behind > 0 { - return Err( + return Err(StepExecutionError::Blocked( "Open pull request blocked: current branch is no longer fully pushed and up to date." .to_owned(), - ); + )); } let current_target = git .upstream_push_target(branch) - .map_err(|error| format!("Unable to revalidate pull request upstream: {error}"))?; + .map_err(StepExecutionError::Git)?; if current_target .as_ref() .map(|(name, branch)| format!("{name}/{branch}")) != Some(expected_upstream.to_owned()) { - return Err( + return Err(StepExecutionError::Blocked( "Open pull request blocked: upstream target changed since the plan was shown." .to_owned(), - ); + )); } let current_remote_urls = git .remote_push_urls(remote) - .map_err(|error| format!("Unable to revalidate pull request remote: {error}"))?; + .map_err(StepExecutionError::Git)?; if current_remote_urls != remote_urls { - return Err( + return Err(StepExecutionError::Blocked( "Open pull request blocked: remote URLs changed since the plan was shown.".to_owned(), - ); + )); } - let push_url = single_pull_request_push_url(remote, ¤t_remote_urls)?; - let local_oid = target - .oid - .as_deref() - .ok_or_else(|| "Open pull request blocked: planned branch has no commit.".to_owned())?; + let push_url = single_pull_request_push_url(remote, ¤t_remote_urls) + .map_err(StepExecutionError::Blocked)?; + let local_oid = target.oid.as_deref().ok_or_else(|| { + StepExecutionError::Blocked( + "Open pull request blocked: planned branch has no commit.".to_owned(), + ) + })?; let remote_oid = git .github_remote_url_head_oid(push_url, branch) - .map_err(|error| format!("Unable to revalidate pushed branch: {error}"))?; + .map_err(StepExecutionError::Git)?; if remote_oid.as_deref() != Some(local_oid) { - return Err( + return Err(StepExecutionError::Blocked( "Open pull request blocked: current branch is no longer pushed to its upstream." .to_owned(), - ); + )); } Ok(()) } @@ -2953,8 +2950,7 @@ impl PlanExecutor { "open pull request request does not match its typed execution context".to_owned(), )); } - validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target) - .map_err(StepExecutionError::Blocked)?; + validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target)?; if !github_base_repository(git, head_github_repository) .map_err(StepExecutionError::Blocked)? .eq_ignore_ascii_case(github_repository) @@ -4189,6 +4185,50 @@ mod tests { Ok(()) } + #[test] + fn pull_request_transport_failure_does_not_persist_remote_credentials() + -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-credential-audit")?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "origin", + "ssh://abc123@github.com/octo/repo.git", + ], + )?; + let fake_gh = fake_gh("open-pr-credential-audit", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh), + ssh_executable: Some(test_ssh_command()?), + }; + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + let remote = PathBuf::from( + git_stdout(&repo, &["config", "remote.origin.testbare"])? + .trim() + .to_owned(), + ); + std::fs::remove_dir_all(remote)?; + let paths = isolated_store_paths("open-pr-credential-audit")?; + + let execution = + PlanExecutor::with_audit_paths_and_ssh(&repo, paths.clone(), test_ssh_command()?) + .execute(&operation.plan, operation.context); + + assert!(!execution.succeeded()); + let entries = LocalStore::open(paths)?.list_audit_entries()?; + assert_eq!(entries.len(), 2); + assert_eq!(entries[1].result, "error"); + assert!(entries[1].message.contains("git failed with status")); + assert!(!entries[1].message.contains("abc123")); + assert!(!entries[1].message.contains("github.com/octo/repo")); + Ok(()) + } + #[test] fn existing_pull_request_is_surfaced_without_creation() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-existing")?; @@ -4482,6 +4522,82 @@ mod tests { Ok(()) } + #[test] + fn pull_request_accepts_suffixless_head_and_upstream_remotes() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-suffixless-remotes")?; + git_stdout(&repo, &["remote", "rename", "origin", "fork"])?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "fork", + "ssh://git@github.com/octo/repo", + ], + )?; + git_stdout( + &repo, + &["remote", "add", "upstream", "git@github.com:upstream/repo"], + )?; + let fake_gh = fake_gh("open-pr-suffixless-remotes", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!( + operation + .plan + .preview_text() + .contains("https://github.com/upstream/repo/compare/main...octo%3Afeature/open-pr") + ); + Ok(()) + } + + #[test] + fn pull_request_allows_matching_branch_names_across_fork_and_upstream() + -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-matching-fork-branch")?; + git_stdout(&repo, &["branch", "-m", "main"])?; + git_stdout_with_ssh( + &repo, + &["push", "-u", "origin", "main"], + &test_ssh_command()?, + )?; + git_stdout( + &repo, + &[ + "remote", + "add", + "upstream", + "https://github.com/upstream/repo.git", + ], + )?; + let fake_gh = fake_gh("open-pr-matching-fork-branch", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!( + operation + .plan + .preview_text() + .contains("https://github.com/upstream/repo/compare/main...octo%3Amain") + ); + Ok(()) + } + #[test] fn open_pull_request_plan_encodes_compare_ref_names() { let plan = open_pull_request_plan( From fcc27afe2657c5136929ef3ea8948cca6ee2a4bf Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 12:11:38 -0400 Subject: [PATCH 12/16] fix: honor tracked pull request heads --- crates/bitbygit-git/src/lib.rs | 20 ++++++ crates/bitbygit-tui/src/lib.rs | 117 +++++++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 19 deletions(-) diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 600c9d0..3ea37d4 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -314,6 +314,7 @@ impl Git { "get-url".to_owned(), "--push".to_owned(), "--all".to_owned(), + "--".to_owned(), remote.to_owned(), ])?; Ok(output @@ -2610,6 +2611,25 @@ mod tests { Ok(()) } + #[test] + fn remote_push_urls_accepts_leading_hyphen_remote() -> Result<(), Box> { + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + repo.run([ + "remote", + "add", + "--", + "-fork", + "https://github.com/fork/repo.git", + ])?; + + assert_eq!( + Git::new(repo.path()).remote_push_urls("-fork")?, + vec!["https://github.com/fork/repo.git"] + ); + Ok(()) + } + #[cfg(unix)] #[test] fn remote_url_head_oid_ignores_configured_ssh_command() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 9522ce6..197e2ad 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -680,6 +680,7 @@ enum PendingPayload { branch: String, upstream: String, remote: String, + upstream_branch: String, remote_urls: Vec, target: HeadTarget, base: String, @@ -1926,9 +1927,9 @@ impl OperationPlanner { .ok_or_else(|| { format!("Open pull request blocked: unable to resolve upstream {upstream}.") })?; - if upstream != format!("{remote}/{upstream_branch}") || upstream_branch != branch { + if upstream != format!("{remote}/{upstream_branch}") { return Err( - "Open pull request blocked: current branch must track a same-named remote branch." + "Open pull request blocked: current branch upstream does not match its configured push target." .to_owned(), ); } @@ -1949,7 +1950,7 @@ impl OperationPlanner { .to_owned() })?; let remote_oid = git - .github_remote_url_head_oid(push_url, &branch) + .github_remote_url_head_oid(push_url, &upstream_branch) .map_err(|error| format!("Unable to verify pushed branch: {error}"))?; if remote_oid.as_deref() != Some(local_oid) { return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); @@ -1963,11 +1964,17 @@ impl OperationPlanner { .to_owned() })? .to_owned(); - let head = pull_request_head(&head_github_repository, &github_repository, &branch)?; + let head = pull_request_head( + &head_github_repository, + &github_repository, + &upstream_branch, + )?; let base = requested_base.unwrap_or(repository.default_branch); - if head_github_repository.eq_ignore_ascii_case(&github_repository) && base == branch { + if head_github_repository.eq_ignore_ascii_case(&github_repository) + && base == upstream_branch + { return Err( - "Open pull request blocked: base branch must differ from the current branch." + "Open pull request blocked: base branch must differ from the pull request head." .to_owned(), ); } @@ -1983,8 +1990,9 @@ impl OperationPlanner { let existing = github .existing_pull_requests(&head) .map_err(open_pull_request_gh_error)?; - let existing_url = matching_pull_request(&existing, &base, &branch, &head_repository) - .map(|pull_request| pull_request.url.as_str()); + let existing_url = + matching_pull_request(&existing, &base, &upstream_branch, &head_repository) + .map(|pull_request| pull_request.url.as_str()); let title = branch.clone(); let request = OperationRequest::OpenPullRequest { base: Some(base.clone()), @@ -2003,6 +2011,7 @@ impl OperationPlanner { branch, upstream, remote, + upstream_branch, remote_urls, target, base, @@ -2287,6 +2296,7 @@ fn validate_open_pull_request_plan( branch: &str, expected_upstream: &str, remote: &str, + upstream_branch: &str, remote_urls: &[String], target: &HeadTarget, ) -> Result<(), StepExecutionError> { @@ -2318,8 +2328,8 @@ fn validate_open_pull_request_plan( .map_err(StepExecutionError::Git)?; if current_target .as_ref() - .map(|(name, branch)| format!("{name}/{branch}")) - != Some(expected_upstream.to_owned()) + .map(|(name, branch)| (name.as_str(), branch.as_str())) + != Some((remote, upstream_branch)) { return Err(StepExecutionError::Blocked( "Open pull request blocked: upstream target changed since the plan was shown." @@ -2342,7 +2352,7 @@ fn validate_open_pull_request_plan( ) })?; let remote_oid = git - .github_remote_url_head_oid(push_url, branch) + .github_remote_url_head_oid(push_url, upstream_branch) .map_err(StepExecutionError::Git)?; if remote_oid.as_deref() != Some(local_oid) { return Err(StepExecutionError::Blocked( @@ -2931,6 +2941,7 @@ impl PlanExecutor { branch, upstream, remote, + upstream_branch, remote_urls, target, base, @@ -2950,7 +2961,15 @@ impl PlanExecutor { "open pull request request does not match its typed execution context".to_owned(), )); } - validate_open_pull_request_plan(git, branch, upstream, remote, remote_urls, target)?; + validate_open_pull_request_plan( + git, + branch, + upstream, + remote, + upstream_branch, + remote_urls, + target, + )?; if !github_base_repository(git, head_github_repository) .map_err(StepExecutionError::Blocked)? .eq_ignore_ascii_case(github_repository) @@ -2983,7 +3002,9 @@ impl PlanExecutor { let existing = github .existing_pull_requests(head) .map_err(StepExecutionError::GitHub)?; - if let Some(existing) = matching_pull_request(&existing, base, branch, head_repository) { + if let Some(existing) = + matching_pull_request(&existing, base, upstream_branch, head_repository) + { return Ok(ExecutionOutput::PullRequest { url: existing.url.clone(), existing: true, @@ -3008,12 +3029,14 @@ impl PlanExecutor { existing: false, }), Err(error) => match github.existing_pull_requests(head) { - Ok(existing) => matching_pull_request(&existing, base, branch, head_repository) - .map(|pull_request| ExecutionOutput::PullRequest { - url: pull_request.url.clone(), - existing: true, - }) - .ok_or(StepExecutionError::GitHub(error)), + Ok(existing) => { + matching_pull_request(&existing, base, upstream_branch, head_repository) + .map(|pull_request| ExecutionOutput::PullRequest { + url: pull_request.url.clone(), + existing: true, + }) + .ok_or(StepExecutionError::GitHub(error)) + } Err(_) => Err(StepExecutionError::GitHub(error)), }, } @@ -4185,6 +4208,62 @@ mod tests { Ok(()) } + #[test] + fn pull_request_uses_differently_named_tracked_remote_branch() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-different-upstream-branch")?; + git_stdout_with_ssh( + &repo, + &["push", "origin", "HEAD:review-feature"], + &test_ssh_command()?, + )?; + git_stdout_with_ssh( + &repo, + &["push", "origin", "--delete", "feature/open-pr"], + &test_ssh_command()?, + )?; + git_stdout( + &repo, + &[ + "branch", + "--set-upstream-to=origin/review-feature", + "feature/open-pr", + ], + )?; + let fake_gh = fake_gh_with_pull_requests( + "open-pr-different-upstream-branch", + "[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"review-feature\",\"repo\":{\"full_name\":\"octo/repo\"}}}]", + true, + )?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + let preview = operation.plan.preview_text(); + assert!(preview.contains("head: review-feature")); + assert!(preview.contains("title: feature/open-pr")); + assert!(preview.contains("target: https://github.com/octo/repo/pull/42")); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-different-upstream-branch-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + assert!(execution.message().contains("Pull request already open")); + let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; + assert!(invocations.contains("head=octo:review-feature")); + assert!(!invocations.contains("head=octo:feature/open-pr")); + assert!(!invocations.contains("pr:create")); + Ok(()) + } + #[test] fn pull_request_transport_failure_does_not_persist_remote_credentials() -> Result<(), Box> { From 69369d6d776ba5304b6593dab4a106f37e5de9e6 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 12:33:26 -0400 Subject: [PATCH 13/16] fix: address pull request review findings --- crates/bitbygit-gh/src/lib.rs | 28 ++++- crates/bitbygit-git/src/lib.rs | 101 +++++++++++++-- crates/bitbygit-tui/src/lib.rs | 220 ++++++++++++++++++++++++++------- 3 files changed, 289 insertions(+), 60 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 09b1aa9..4e60d96 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -98,6 +98,8 @@ impl GitHub { format!("head={owner}:{branch}"), "-f".to_owned(), "per_page=100".to_owned(), + "--hostname".to_owned(), + "github.com".to_owned(), ])?; let pages: Vec> = parse_json(&output, "pull request query")?; Ok(pages.into_iter().flatten().map(PullRequest::from).collect()) @@ -122,6 +124,8 @@ impl GitHub { repository.name_with_owner, url_path_component(branch) ), + "--hostname".to_owned(), + "github.com".to_owned(), ])?; let pages: Vec> = parse_json(&output, "branch query")?; let expected = format!("refs/heads/{branch}"); @@ -527,7 +531,7 @@ mod tests { #[test] fn decodes_rest_existing_pull_request_responses() -> Result<(), Box> { let fake = FakeGh::new( - "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature\",\"repo\":{\"full_name\":\"octo/repo\"}}}]]' ;;\n*) exit 1 ;;\nesac", + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] && [ \"${13}\" = --hostname ] && [ \"${14}\" = github.com ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature\",\"repo\":{\"full_name\":\"octo/repo\"}}}]]' ;;\n*) exit 1 ;;\nesac", )?; let github = GitHub::with_executable_and_repository( &fake.path, @@ -556,7 +560,7 @@ mod tests { "repo\u{1f}view\u{1f}github.com/octo/repo\u{1f}--json\u{1f}nameWithOwner,defaultBranchRef\u{1f}\n" )); assert!(fake.invocations()?.contains( - "api\u{1f}--method\u{1f}GET\u{1f}--paginate\u{1f}--slurp\u{1f}repos/octo/repo/pulls\u{1f}-f\u{1f}state=open\u{1f}-f\u{1f}head=octo:feature\u{1f}-f\u{1f}per_page=100\u{1f}\n" + "api\u{1f}--method\u{1f}GET\u{1f}--paginate\u{1f}--slurp\u{1f}repos/octo/repo/pulls\u{1f}-f\u{1f}state=open\u{1f}-f\u{1f}head=octo:feature\u{1f}-f\u{1f}per_page=100\u{1f}--hostname\u{1f}github.com\u{1f}\n" )); assert_eq!(fake.prompt_values()?, "1\n1\n1\n1\n1\n1\n1\n"); Ok(()) @@ -572,6 +576,17 @@ mod tests { Ok(()) } + #[test] + fn rest_queries_pin_github_com_when_gh_host_conflicts() -> Result<(), Box> { + let fake = FakeGh::new( + "GH_HOST=example.com\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) case \"$6\" in\n*/pulls) [ \"${13}\" = --hostname ] && [ \"${14}\" = github.com ] || exit 1; printf '%s\\n' '[[]]' ;;\n*/git/matching-refs/heads/*) [ \"$7\" = --hostname ] && [ \"$8\" = github.com ] || exit 1; printf '%s\\n' '[[]]' ;;\n*) exit 1 ;;\nesac ;;\n*) exit 1 ;;\nesac", + )?; + + assert!(fake.github().existing_pull_requests("feature")?.is_empty()); + assert!(!fake.github().branch_exists("main")?); + Ok(()) + } + #[test] fn creates_pull_request_with_literal_shell_metacharacters_in_title() -> Result<(), Box> { @@ -608,8 +623,13 @@ mod tests { impl FakeGh { fn new(body: &str) -> Result> { let id = NEXT_FAKE_ID.fetch_add(1, Ordering::Relaxed); - let path = - std::env::temp_dir().join(format!("bitbygit-fake-gh-{}-{id}", std::process::id())); + let nonce = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH)? + .as_nanos(); + let path = std::env::temp_dir().join(format!( + "bitbygit-fake-gh-{}-{id}-{nonce}", + std::process::id() + )); if path.exists() { fs::remove_dir_all(&path)?; } diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 3ea37d4..e09094b 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -290,16 +290,18 @@ impl Git { branch: &str, allowed_protocols: Option<&str>, ) -> Result, GitError> { - let output = self.run_args_with_allowed_protocols( - vec![ - "ls-remote".to_owned(), - "--heads".to_owned(), - "--".to_owned(), - target.to_owned(), - format!("refs/heads/{branch}"), - ], - allowed_protocols, - )?; + let mut args = Vec::new(); + if allowed_protocols.is_some() { + args.extend(["-c".to_owned(), "credential.helper=".to_owned()]); + } + args.extend([ + "ls-remote".to_owned(), + "--heads".to_owned(), + "--".to_owned(), + target.to_owned(), + format!("refs/heads/{branch}"), + ]); + let output = self.run_args_with_allowed_protocols(args, allowed_protocols)?; Ok(output .stdout .lines() @@ -386,6 +388,26 @@ impl Git { Ok(Some((remote, branch))) } + pub fn push_target(&self, branch: &str) -> Result, GitError> { + let output = self.run_args(vec![ + "for-each-ref".to_owned(), + "--format=%(push:remotename)%00%(push:short)".to_owned(), + "--count=1".to_owned(), + "--".to_owned(), + format!("refs/heads/{branch}"), + ])?; + let Some((remote, target)) = output.stdout.trim().split_once('\0') else { + return self.upstream_push_target(branch); + }; + let Some(push_branch) = target.strip_prefix(&format!("{remote}/")) else { + return self.upstream_push_target(branch); + }; + if remote.is_empty() || push_branch.is_empty() { + return self.upstream_push_target(branch); + } + Ok(Some((remote.to_owned(), push_branch.to_owned()))) + } + pub fn remote_tracking_oid( &self, remote: &str, @@ -2630,6 +2652,34 @@ mod tests { Ok(()) } + #[test] + fn push_target_uses_branch_push_remote_in_triangular_workflow() -> Result<(), Box> { + let repo = TempRepo::new()?; + repo.run(["init", "-b", "feature"])?; + repo.run(["config", "user.email", "bitbygit@example.invalid"])?; + repo.run(["config", "user.name", "bitbygit test"])?; + repo.run(["commit", "--allow-empty", "-m", "initial"])?; + repo.run([ + "remote", + "add", + "upstream", + "https://github.com/upstream/repo.git", + ])?; + repo.run(["remote", "add", "fork", "https://github.com/fork/repo.git"])?; + repo.run(["update-ref", "refs/remotes/upstream/main", "HEAD"])?; + repo.run(["update-ref", "refs/remotes/fork/feature", "HEAD"])?; + repo.run(["config", "branch.feature.remote", "upstream"])?; + repo.run(["config", "branch.feature.merge", "refs/heads/main"])?; + repo.run(["config", "branch.feature.pushRemote", "fork"])?; + repo.run(["config", "push.default", "current"])?; + + assert_eq!( + Git::new(repo.path()).push_target("feature")?, + Some(("fork".to_owned(), "feature".to_owned())) + ); + Ok(()) + } + #[cfg(unix)] #[test] fn remote_url_head_oid_ignores_configured_ssh_command() -> Result<(), Box> { @@ -2677,6 +2727,37 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn github_remote_url_head_oid_ignores_configured_credential_helper() + -> Result<(), Box> { + use std::os::unix::fs::PermissionsExt; + + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + let marker = repo.path().join("credential-helper-ran"); + let helper = repo.path().join("credential-helper"); + fs::write( + &helper, + format!("#!/bin/sh\ntouch '{}'\nexit 1\n", marker.display()), + )?; + let mut permissions = fs::metadata(&helper)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&helper, permissions)?; + repo.run_args(&[ + "config", + "credential.helper", + &format!("!{}", helper.display()), + ])?; + + let result = Git::new(repo.path()) + .github_remote_url_head_oid("https://bitbygit@127.0.0.1:1/octo/repo.git", "main"); + + assert!(result.is_err()); + assert!(!marker.exists()); + Ok(()) + } + #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 197e2ad..6ba1834 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1918,21 +1918,13 @@ impl OperationPlanner { let Some(upstream) = status.branch.upstream.clone() else { return Err("Open pull request blocked: current branch has no upstream. Push it with `push` first.".to_owned()); }; - if status.branch.ahead > 0 || status.branch.behind > 0 { - return Err("Open pull request blocked: current branch is not fully pushed and up to date. Push or pull it first.".to_owned()); - } let (remote, upstream_branch) = git - .upstream_push_target(&branch) + .push_target(&branch) .map_err(|error| format!("Unable to prepare pull request target: {error}"))? .ok_or_else(|| { - format!("Open pull request blocked: unable to resolve upstream {upstream}.") + "Open pull request blocked: unable to resolve the current branch push target." + .to_owned() })?; - if upstream != format!("{remote}/{upstream_branch}") { - return Err( - "Open pull request blocked: current branch upstream does not match its configured push target." - .to_owned(), - ); - } let remote_urls = git .remote_push_urls(&remote) .map_err(|error| format!("Unable to prepare pull request remote: {error}"))?; @@ -1958,19 +1950,24 @@ impl OperationPlanner { let github_repository = github_base_repository(&git, &head_github_repository)?; let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; - let head_repository = github_repository_name(&head_github_repository) - .ok_or_else(|| { - "Open pull request blocked: source remote does not identify a GitHub repository." - .to_owned() - })? - .to_owned(); + let canonical_head_repository = + if head_github_repository.eq_ignore_ascii_case(&github_repository) { + repository.clone() + } else { + self.github(&head_github_repository) + .repository() + .map_err(open_pull_request_gh_error)? + }; + let head_repository = canonical_head_repository.name_with_owner; + let canonical_head_github_repository = format!("github.com/{head_repository}"); + let canonical_github_repository = format!("github.com/{}", repository.name_with_owner); let head = pull_request_head( - &head_github_repository, - &github_repository, + &canonical_head_github_repository, + &canonical_github_repository, &upstream_branch, )?; let base = requested_base.unwrap_or(repository.default_branch); - if head_github_repository.eq_ignore_ascii_case(&github_repository) + if canonical_head_github_repository.eq_ignore_ascii_case(&canonical_github_repository) && base == upstream_branch { return Err( @@ -2161,15 +2158,6 @@ fn github_base_repository(git: &Git, head_repository: &str) -> Result Option<&str> { - let (_, name) = repository.split_once('/')?; - if name.split('/').count() == 2 { - Some(name) - } else { - None - } -} - fn pull_request_head( head_repository: &str, base_repository: &str, @@ -2317,23 +2305,14 @@ fn validate_open_pull_request_plan( "Open pull request blocked: upstream changed since the plan was shown.".to_owned(), )); } - if status.branch.ahead > 0 || status.branch.behind > 0 { - return Err(StepExecutionError::Blocked( - "Open pull request blocked: current branch is no longer fully pushed and up to date." - .to_owned(), - )); - } - let current_target = git - .upstream_push_target(branch) - .map_err(StepExecutionError::Git)?; + let current_target = git.push_target(branch).map_err(StepExecutionError::Git)?; if current_target .as_ref() .map(|(name, branch)| (name.as_str(), branch.as_str())) != Some((remote, upstream_branch)) { return Err(StepExecutionError::Blocked( - "Open pull request blocked: upstream target changed since the plan was shown." - .to_owned(), + "Open pull request blocked: push target changed since the plan was shown.".to_owned(), )); } let current_remote_urls = git @@ -2999,6 +2978,47 @@ impl PlanExecutor { .to_owned(), )); } + let current_head_repository = + if head_github_repository.eq_ignore_ascii_case(github_repository) { + current_repository.clone() + } else { + let head_github = match github_executable { + Some(executable) => GitHub::with_executable_and_repository( + &self.repo_root, + executable, + head_github_repository, + ), + None => GitHub::with_executable_and_repository( + &self.repo_root, + "gh", + head_github_repository, + ), + }; + head_github + .repository() + .map_err(StepExecutionError::GitHub)? + }; + if !current_head_repository + .name_with_owner + .eq_ignore_ascii_case(head_repository) + { + return Err(StepExecutionError::Blocked( + "Open pull request blocked: GitHub head repository changed since the plan was shown." + .to_owned(), + )); + } + let current_head = pull_request_head( + &format!("github.com/{}", current_head_repository.name_with_owner), + &format!("github.com/{}", current_repository.name_with_owner), + upstream_branch, + ) + .map_err(StepExecutionError::Blocked)?; + if current_head != *head { + return Err(StepExecutionError::Blocked( + "Open pull request blocked: GitHub head changed since the plan was shown." + .to_owned(), + )); + } let existing = github .existing_pull_requests(head) .map_err(StepExecutionError::GitHub)?; @@ -4383,6 +4403,44 @@ mod tests { Ok(()) } + #[test] + fn renamed_head_repository_surfaces_existing_pull_request() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-renamed-head")?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "origin", + "ssh://git@github.com/octo/old-repo.git", + ], + )?; + let fake_gh = fake_gh("open-pr-renamed-head", true)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!(operation.plan.preview_text().contains("pull/42")); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-renamed-head-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + assert!(execution.succeeded(), "{}", execution.message()); + assert!(execution.message().contains("already open")); + assert!( + !std::fs::read_to_string(fake_gh.with_file_name("invocations"))?.contains("pr:create") + ); + Ok(()) + } + #[test] fn open_pull_request_plan_rejects_missing_base_branch() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-missing-base")?; @@ -4577,13 +4635,25 @@ mod tests { assert!(execution.succeeded(), "{}", execution.message()); let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; - for invocation in invocations + let repo_views = invocations .lines() .filter(|line| line.starts_with("repo:view")) - { - assert!(invocation.contains("view github.com/upstream/repo")); - assert!(!invocation.contains("--repo")); - } + .collect::>(); + assert!( + repo_views + .iter() + .any(|invocation| invocation.contains("view github.com/upstream/repo")) + ); + assert!( + repo_views + .iter() + .any(|invocation| invocation.contains("view github.com/octo/repo")) + ); + assert!( + repo_views + .iter() + .all(|invocation| !invocation.contains("--repo")) + ); for invocation in invocations .lines() .filter(|line| line.starts_with("api:--method") && line.contains("/pulls")) @@ -4601,6 +4671,64 @@ mod tests { Ok(()) } + #[test] + fn pull_request_uses_push_remote_in_triangular_workflow() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-triangular-fork")?; + git_stdout(&repo, &["remote", "rename", "origin", "fork"])?; + git_stdout( + &repo, + &[ + "remote", + "add", + "upstream", + "https://github.com/upstream/repo.git", + ], + )?; + git_stdout( + &repo, + &["update-ref", "refs/remotes/upstream/main", "HEAD^"], + )?; + git_stdout( + &repo, + &["config", "branch.feature/open-pr.remote", "upstream"], + )?; + git_stdout( + &repo, + &["config", "branch.feature/open-pr.merge", "refs/heads/main"], + )?; + git_stdout( + &repo, + &["config", "branch.feature/open-pr.pushRemote", "fork"], + )?; + git_stdout(&repo, &["config", "push.default", "current"])?; + let fake_gh = fake_gh("open-pr-triangular-fork", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + let preview = operation.plan.preview_text(); + assert!(preview.contains("remote: fork")); + assert!(preview.contains("head: octo:feature/open-pr")); + assert!(preview.contains("https://github.com/upstream/repo/compare/main")); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-triangular-fork-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + assert!(execution.succeeded(), "{}", execution.message()); + let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; + assert!(invocations.contains("head=octo:feature/open-pr")); + assert!(invocations.contains("pr:create")); + Ok(()) + } + #[test] fn pull_request_accepts_suffixless_head_and_upstream_remotes() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-suffixless-remotes")?; @@ -5620,7 +5748,7 @@ mod tests { std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method)\n case \"$6\" in\n */git/matching-refs/heads/*)\n if [ -f '{}' ]; then printf '%s\\n' '[[]]'; else printf '%s\\n' '[[{{\"ref\":\"refs/heads/main\"}},{{\"ref\":\"refs/heads/release\"}}]]'; fi ;;\n */pulls) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\n *) exit 1 ;;\n esac ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\nif [ \"$repository\" = octo/old-repo ]; then repository=octo/repo; fi\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method)\n case \"$6\" in\n */git/matching-refs/heads/*)\n if [ -f '{}' ]; then printf '%s\\n' '[[]]'; else printf '%s\\n' '[[{{\"ref\":\"refs/heads/main\"}},{{\"ref\":\"refs/heads/release\"}}]]'; fi ;;\n */pulls) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\n *) exit 1 ;;\n esac ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), base_missing.display(), pull_requests, From 94676308d6336c0699b147fc9b7708bef19e1f12 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 12:42:53 -0400 Subject: [PATCH 14/16] fix: preserve triangular push target --- crates/bitbygit-git/src/lib.rs | 18 ++++++++++++++++-- crates/bitbygit-tui/src/lib.rs | 3 +-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index e09094b..83a25cb 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -399,6 +399,21 @@ impl Git { let Some((remote, target)) = output.stdout.trim().split_once('\0') else { return self.upstream_push_target(branch); }; + if !remote.is_empty() && target.is_empty() { + let push_default = self.config_value(["config", "--get", "push.default"])?; + let upstream = self.upstream_push_target(branch)?; + if push_default.as_deref().unwrap_or("simple") == "simple" { + return Ok(upstream.map(|(upstream_remote, upstream_branch)| { + let push_branch = if upstream_remote == remote { + upstream_branch + } else { + branch.to_owned() + }; + (remote.to_owned(), push_branch) + })); + } + return Ok(None); + } let Some(push_branch) = target.strip_prefix(&format!("{remote}/")) else { return self.upstream_push_target(branch); }; @@ -2653,7 +2668,7 @@ mod tests { } #[test] - fn push_target_uses_branch_push_remote_in_triangular_workflow() -> Result<(), Box> { + fn push_target_uses_default_simple_in_triangular_workflow() -> Result<(), Box> { let repo = TempRepo::new()?; repo.run(["init", "-b", "feature"])?; repo.run(["config", "user.email", "bitbygit@example.invalid"])?; @@ -2671,7 +2686,6 @@ mod tests { repo.run(["config", "branch.feature.remote", "upstream"])?; repo.run(["config", "branch.feature.merge", "refs/heads/main"])?; repo.run(["config", "branch.feature.pushRemote", "fork"])?; - repo.run(["config", "push.default", "current"])?; assert_eq!( Git::new(repo.path()).push_target("feature")?, diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 6ba1834..89db117 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -4672,7 +4672,7 @@ mod tests { } #[test] - fn pull_request_uses_push_remote_in_triangular_workflow() -> Result<(), Box> { + fn pull_request_uses_default_simple_in_triangular_workflow() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-triangular-fork")?; git_stdout(&repo, &["remote", "rename", "origin", "fork"])?; git_stdout( @@ -4700,7 +4700,6 @@ mod tests { &repo, &["config", "branch.feature/open-pr.pushRemote", "fork"], )?; - git_stdout(&repo, &["config", "push.default", "current"])?; let fake_gh = fake_gh("open-pr-triangular-fork", false)?; let planner = OperationPlanner { repo_root: repo.clone(), From 3ff4451103dace74c4dc7f595e6813cdeb431444 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 12:58:31 -0400 Subject: [PATCH 15/16] fix: authenticate pull request head checks --- crates/bitbygit-gh/src/lib.rs | 89 ++++++++++++++++++----- crates/bitbygit-git/src/lib.rs | 87 ++--------------------- crates/bitbygit-tui/src/lib.rs | 125 +++++++++++++++++++++------------ 3 files changed, 158 insertions(+), 143 deletions(-) diff --git a/crates/bitbygit-gh/src/lib.rs b/crates/bitbygit-gh/src/lib.rs index 4e60d96..32d7973 100644 --- a/crates/bitbygit-gh/src/lib.rs +++ b/crates/bitbygit-gh/src/lib.rs @@ -14,6 +14,8 @@ pub const AUTHENTICATE_GH_GUIDANCE: &str = pub struct GitHub { cwd: PathBuf, executable: PathBuf, + #[cfg(test)] + executable_args: Vec, repository: Option, } @@ -26,6 +28,8 @@ impl GitHub { Self { cwd: cwd.into(), executable: executable.into(), + #[cfg(test)] + executable_args: Vec::new(), repository: None, } } @@ -38,10 +42,18 @@ impl GitHub { Self { cwd: cwd.into(), executable: executable.into(), + #[cfg(test)] + executable_args: Vec::new(), repository: Some(repository.into()), } } + #[cfg(test)] + fn with_executable_args(mut self, executable_args: Vec) -> Self { + self.executable_args = executable_args; + self + } + pub fn setup_status(&self) -> Result { match self.run_status(vec!["--version".to_owned()]) { Ok(()) => {} @@ -106,11 +118,19 @@ impl GitHub { } pub fn branch_exists(&self, branch: &str) -> Result { + Ok(self.branch_reference(branch)?.is_some()) + } + + pub fn branch_oid(&self, branch: &str) -> Result, GhError> { + Ok(self + .branch_reference(branch)? + .map(|reference| reference.object.sha)) + } + + fn branch_reference(&self, branch: &str) -> Result, GhError> { self.ensure_ready()?; if branch.trim().is_empty() { - return Err(GhError::InvalidInput { - name: "base branch", - }); + return Err(GhError::InvalidInput { name: "branch" }); } let repository = self.repository_after_ready()?; let output = self.run_output(vec![ @@ -132,7 +152,7 @@ impl GitHub { Ok(pages .into_iter() .flatten() - .any(|reference| reference.name == expected)) + .find(|reference| reference.name == expected)) } fn repository_after_ready(&self) -> Result { @@ -244,8 +264,10 @@ impl GitHub { let mut command = Command::new(&self.executable); command .current_dir(&self.cwd) - .env("GH_PROMPT_DISABLED", "1") - .args(args); + .env("GH_PROMPT_DISABLED", "1"); + #[cfg(test)] + command.args(&self.executable_args); + command.args(args); command } } @@ -313,6 +335,12 @@ struct RestPullRequestRepository { struct RestReference { #[serde(rename = "ref")] name: String, + object: RestReferenceObject, +} + +#[derive(Deserialize)] +struct RestReferenceObject { + sha: String, } impl From for PullRequest { @@ -449,7 +477,6 @@ fn url_path_component(value: &str) -> String { mod tests { use super::*; use std::fs; - use std::os::unix::fs::PermissionsExt; use std::sync::atomic::{AtomicUsize, Ordering}; static NEXT_FAKE_ID: AtomicUsize = AtomicUsize::new(0); @@ -533,11 +560,9 @@ mod tests { let fake = FakeGh::new( "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) [ \"$3\" = github.com/octo/repo ] && [ \"$4\" = --json ] && [ \"$5\" = nameWithOwner,defaultBranchRef ] || exit 1; printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$3\" = GET ] && [ \"$4\" = --paginate ] && [ \"$5\" = --slurp ] && [ \"$6\" = repos/octo/repo/pulls ] && [ \"$7\" = -f ] && [ \"$8\" = state=open ] && [ \"$9\" = -f ] && [ \"${10}\" = head=octo:feature ] && [ \"${11}\" = -f ] && [ \"${12}\" = per_page=100 ] && [ \"${13}\" = --hostname ] && [ \"${14}\" = github.com ] || exit 1; printf '%s\\n' '[[{\"number\":42,\"html_url\":\"https://github.com/octo/repo/pull/42\",\"title\":\"Existing PR\",\"base\":{\"ref\":\"main\"},\"head\":{\"ref\":\"feature\",\"repo\":{\"full_name\":\"octo/repo\"}}}]]' ;;\n*) exit 1 ;;\nesac", )?; - let github = GitHub::with_executable_and_repository( - &fake.path, - &fake.executable, - "github.com/octo/repo", - ); + let github = + GitHub::with_executable_and_repository(&fake.path, "/bin/sh", "github.com/octo/repo") + .with_executable_args(vec![fake.executable.display().to_string()]); let repository = github.repository()?; let pull_requests = github.existing_pull_requests("feature")?; @@ -569,10 +594,14 @@ mod tests { #[test] fn checks_encoded_base_branch_reference() -> Result<(), Box> { let fake = FakeGh::new( - "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$6\" = repos/octo/repo/git/matching-refs/heads/release%2Fnext ] || exit 1; printf '%s\\n' '[[{\"ref\":\"refs/heads/release/next\"}]]' ;;\n*) exit 1 ;;\nesac", + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) printf '%s\\n' '{\"nameWithOwner\":\"octo/repo\",\"defaultBranchRef\":{\"name\":\"main\"}}' ;;\napi:--method) [ \"$6\" = repos/octo/repo/git/matching-refs/heads/release%2Fnext ] || exit 1; printf '%s\\n' '[[{\"ref\":\"refs/heads/release/next\",\"object\":{\"sha\":\"abc123\"}}]]' ;;\n*) exit 1 ;;\nesac", )?; assert!(fake.github().branch_exists("release/next")?); + assert_eq!( + fake.github().branch_oid("release/next")?.as_deref(), + Some("abc123") + ); Ok(()) } @@ -613,6 +642,34 @@ mod tests { Ok(()) } + #[test] + fn fake_gh_executes_reliably_in_parallel() -> Result<(), Box> { + let threads = (0..32) + .map(|_| { + std::thread::spawn(|| -> Result<(), String> { + let fake = FakeGh::new( + "case \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nesac\nexit 1", + ) + .map_err(|error| error.to_string())?; + if fake.github().setup_status().map_err(|error| error.to_string())? + != GhSetupStatus::Ready + { + return Err("fake GitHub CLI was not ready".to_owned()); + } + Ok(()) + }) + }) + .collect::>(); + + for thread in threads { + thread + .join() + .map_err(|_| io::Error::other("fake GitHub CLI thread panicked"))? + .map_err(io::Error::other)?; + } + Ok(()) + } + struct FakeGh { path: PathBuf, executable: PathBuf, @@ -646,9 +703,6 @@ mod tests { prompt_values.display(), ), )?; - let mut permissions = fs::metadata(&executable)?.permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&executable, permissions)?; Ok(Self { path, executable, @@ -658,7 +712,8 @@ mod tests { } fn github(&self) -> GitHub { - GitHub::with_executable(&self.path, &self.executable) + GitHub::with_executable(&self.path, "/bin/sh") + .with_executable_args(vec![self.executable.display().to_string()]) } fn invocations(&self) -> Result { diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index 83a25cb..bdaa2c5 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -269,39 +269,21 @@ impl Git { } pub fn remote_head_oid(&self, remote: &str, branch: &str) -> Result, GitError> { - self.head_oid_at(remote, branch, None) + self.head_oid_at(remote, branch) } pub fn remote_url_head_oid(&self, url: &str, branch: &str) -> Result, GitError> { - self.head_oid_at(url, branch, None) + self.head_oid_at(url, branch) } - pub fn github_remote_url_head_oid( - &self, - url: &str, - branch: &str, - ) -> Result, GitError> { - self.head_oid_at(url, branch, Some("https:ssh")) - } - - fn head_oid_at( - &self, - target: &str, - branch: &str, - allowed_protocols: Option<&str>, - ) -> Result, GitError> { - let mut args = Vec::new(); - if allowed_protocols.is_some() { - args.extend(["-c".to_owned(), "credential.helper=".to_owned()]); - } - args.extend([ + fn head_oid_at(&self, target: &str, branch: &str) -> Result, GitError> { + let output = self.run_args(vec![ "ls-remote".to_owned(), "--heads".to_owned(), "--".to_owned(), target.to_owned(), format!("refs/heads/{branch}"), - ]); - let output = self.run_args_with_allowed_protocols(args, allowed_protocols)?; + ])?; Ok(output .stdout .lines() @@ -883,14 +865,6 @@ impl Git { } fn run_args(&self, args: Vec) -> Result { - self.run_args_with_allowed_protocols(args, None) - } - - fn run_args_with_allowed_protocols( - &self, - args: Vec, - allowed_protocols: Option<&str>, - ) -> Result { let mut command = Command::new("git"); command .current_dir(&self.cwd) @@ -904,9 +878,6 @@ impl Git { .map(|path| shell_quote(&path.to_string_lossy())) .unwrap_or_else(|| "ssh".to_owned()); command.env("GIT_SSH_COMMAND", format!("{ssh_executable} {SSH_OPTIONS}")); - if let Some(protocols) = allowed_protocols { - command.env("GIT_ALLOW_PROTOCOL", protocols); - } let output = command .args(&args) .output() @@ -2724,54 +2695,6 @@ mod tests { Ok(()) } - #[cfg(unix)] - #[test] - fn github_remote_url_head_oid_rejects_ext_transport_even_when_enabled() - -> Result<(), Box> { - let repo = TempRepo::new()?; - repo.run(["init", "-b", "main"])?; - repo.run(["config", "protocol.ext.allow", "always"])?; - let marker = repo.path().join("ext-ran"); - - let result = Git::new(repo.path()) - .github_remote_url_head_oid(&format!("ext::touch {}", marker.display()), "main"); - - assert!(result.is_err()); - assert!(!marker.exists()); - Ok(()) - } - - #[cfg(unix)] - #[test] - fn github_remote_url_head_oid_ignores_configured_credential_helper() - -> Result<(), Box> { - use std::os::unix::fs::PermissionsExt; - - let repo = TempRepo::new()?; - repo.run(["init", "-b", "main"])?; - let marker = repo.path().join("credential-helper-ran"); - let helper = repo.path().join("credential-helper"); - fs::write( - &helper, - format!("#!/bin/sh\ntouch '{}'\nexit 1\n", marker.display()), - )?; - let mut permissions = fs::metadata(&helper)?.permissions(); - permissions.set_mode(0o755); - fs::set_permissions(&helper, permissions)?; - repo.run_args(&[ - "config", - "credential.helper", - &format!("!{}", helper.display()), - ])?; - - let result = Git::new(repo.path()) - .github_remote_url_head_oid("https://bitbygit@127.0.0.1:1/octo/repo.git", "main"); - - assert!(result.is_err()); - assert!(!marker.exists()); - Ok(()) - } - #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 89db117..8f81e4b 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1941,23 +1941,24 @@ impl OperationPlanner { "Open pull request blocked: current branch needs a commit before opening a pull request." .to_owned() })?; - let remote_oid = git - .github_remote_url_head_oid(push_url, &upstream_branch) - .map_err(|error| format!("Unable to verify pushed branch: {error}"))?; - if remote_oid.as_deref() != Some(local_oid) { - return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); - } let github_repository = github_base_repository(&git, &head_github_repository)?; let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; + let head_github = self.github(&head_github_repository); let canonical_head_repository = if head_github_repository.eq_ignore_ascii_case(&github_repository) { repository.clone() } else { - self.github(&head_github_repository) + head_github .repository() .map_err(open_pull_request_gh_error)? }; + let remote_oid = head_github + .branch_oid(&upstream_branch) + .map_err(open_pull_request_gh_error)?; + if remote_oid.as_deref() != Some(local_oid) { + return Err("Open pull request blocked: current branch is not pushed to its upstream. Push it with `push` first.".to_owned()); + } let head_repository = canonical_head_repository.name_with_owner; let canonical_head_github_repository = format!("github.com/{head_repository}"); let canonical_github_repository = format!("github.com/{}", repository.name_with_owner); @@ -2323,22 +2324,8 @@ fn validate_open_pull_request_plan( "Open pull request blocked: remote URLs changed since the plan was shown.".to_owned(), )); } - let push_url = single_pull_request_push_url(remote, ¤t_remote_urls) + single_pull_request_push_url(remote, ¤t_remote_urls) .map_err(StepExecutionError::Blocked)?; - let local_oid = target.oid.as_deref().ok_or_else(|| { - StepExecutionError::Blocked( - "Open pull request blocked: planned branch has no commit.".to_owned(), - ) - })?; - let remote_oid = git - .github_remote_url_head_oid(push_url, upstream_branch) - .map_err(StepExecutionError::Git)?; - if remote_oid.as_deref() != Some(local_oid) { - return Err(StepExecutionError::Blocked( - "Open pull request blocked: current branch is no longer pushed to its upstream." - .to_owned(), - )); - } Ok(()) } @@ -2978,22 +2965,22 @@ impl PlanExecutor { .to_owned(), )); } + let head_github = match github_executable { + Some(executable) => GitHub::with_executable_and_repository( + &self.repo_root, + executable, + head_github_repository, + ), + None => GitHub::with_executable_and_repository( + &self.repo_root, + "gh", + head_github_repository, + ), + }; let current_head_repository = if head_github_repository.eq_ignore_ascii_case(github_repository) { current_repository.clone() } else { - let head_github = match github_executable { - Some(executable) => GitHub::with_executable_and_repository( - &self.repo_root, - executable, - head_github_repository, - ), - None => GitHub::with_executable_and_repository( - &self.repo_root, - "gh", - head_github_repository, - ), - }; head_github .repository() .map_err(StepExecutionError::GitHub)? @@ -3007,6 +2994,20 @@ impl PlanExecutor { .to_owned(), )); } + let local_oid = target.oid.as_deref().ok_or_else(|| { + StepExecutionError::Blocked( + "Open pull request blocked: planned branch has no commit.".to_owned(), + ) + })?; + let remote_oid = head_github + .branch_oid(upstream_branch) + .map_err(StepExecutionError::GitHub)?; + if remote_oid.as_deref() != Some(local_oid) { + return Err(StepExecutionError::Blocked( + "Open pull request blocked: current branch is no longer pushed to its upstream." + .to_owned(), + )); + } let current_head = pull_request_head( &format!("github.com/{}", current_head_repository.name_with_owner), &format!("github.com/{}", current_repository.name_with_owner), @@ -4228,6 +4229,44 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn private_https_pull_request_uses_authenticated_api_not_repository_credential_helpers() + -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-private-https")?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "origin", + "https://github.com/octo/repo.git", + ], + )?; + let marker = repo.join("credential-helper-ran"); + git_stdout( + &repo, + &[ + "config", + "credential.helper", + &format!("!touch '{}'", marker.display()), + ], + )?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh("open-pr-private-https", false)?), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + assert!(operation.plan.preview_text().contains("base: main")); + assert!(!marker.exists()); + Ok(()) + } + #[test] fn pull_request_uses_differently_named_tracked_remote_branch() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-different-upstream-branch")?; @@ -4285,7 +4324,7 @@ mod tests { } #[test] - fn pull_request_transport_failure_does_not_persist_remote_credentials() + fn pull_request_head_revalidation_failure_does_not_persist_remote_credentials() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-credential-audit")?; git_stdout( @@ -4300,18 +4339,13 @@ mod tests { let fake_gh = fake_gh("open-pr-credential-audit", false)?; let planner = OperationPlanner { repo_root: repo.clone(), - github_executable: Some(fake_gh), + github_executable: Some(fake_gh.clone()), ssh_executable: Some(test_ssh_command()?), }; let operation = planner .plan_request(OperationRequest::OpenPullRequest { base: None }) .map_err(std::io::Error::other)?; - let remote = PathBuf::from( - git_stdout(&repo, &["config", "remote.origin.testbare"])? - .trim() - .to_owned(), - ); - std::fs::remove_dir_all(remote)?; + std::fs::write(fake_gh.with_file_name("head-missing"), "")?; let paths = isolated_store_paths("open-pr-credential-audit")?; let execution = @@ -4319,10 +4353,11 @@ mod tests { .execute(&operation.plan, operation.context); assert!(!execution.succeeded()); + assert!(execution.message().contains("no longer pushed")); let entries = LocalStore::open(paths)?.list_audit_entries()?; assert_eq!(entries.len(), 2); assert_eq!(entries[1].result, "error"); - assert!(entries[1].message.contains("git failed with status")); + assert!(entries[1].message.contains("no longer pushed")); assert!(!entries[1].message.contains("abc123")); assert!(!entries[1].message.contains("github.com/octo/repo")); Ok(()) @@ -5739,6 +5774,7 @@ mod tests { let executable = root.join("gh"); let invocations = root.join("invocations"); let base_missing = root.join("base-missing"); + let head_missing = root.join("head-missing"); let create_response = if create_succeeds { "printf '%s\\n' 'https://github.com/octo/repo/pull/43' ;;" } else { @@ -5747,9 +5783,10 @@ mod tests { std::fs::write( &executable, format!( - "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\nif [ \"$repository\" = octo/old-repo ]; then repository=octo/repo; fi\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method)\n case \"$6\" in\n */git/matching-refs/heads/*)\n if [ -f '{}' ]; then printf '%s\\n' '[[]]'; else printf '%s\\n' '[[{{\"ref\":\"refs/heads/main\"}},{{\"ref\":\"refs/heads/release\"}}]]'; fi ;;\n */pulls) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\n *) exit 1 ;;\n esac ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", + "#!/bin/sh\nprintf '%s:%s %s\\n' \"$1\" \"$2\" \"$*\" >> '{}'\nrepository=octo/repo\nfor arg in \"$@\"; do\n case \"$arg\" in\n github.com/*/*) repository=${{arg#github.com/}} ;;\n esac\ndone\nif [ \"$repository\" = octo/old-repo ]; then repository=octo/repo; fi\ncase \"$1:$2\" in\n--version:*) exit 0 ;;\nauth:status) exit 0 ;;\nrepo:view) case \"$*\" in *--repo*) exit 1 ;; esac; printf '{{\"nameWithOwner\":\"%s\",\"defaultBranchRef\":{{\"name\":\"main\"}}}}\\n' \"$repository\" ;;\napi:--method)\n case \"$6\" in\n */git/matching-refs/heads/*)\n ref=${{6##*/heads/}}\n ref=$(printf '%s' \"$ref\" | sed 's/%2F/\\//g')\n if [ \"$ref\" = missing ] || {{ [ \"$ref\" = main ] && [ -f '{}' ]; }} || {{ [ \"$ref\" != main ] && [ \"$ref\" != release ] && [ -f '{}' ]; }}; then\n printf '%s\\n' '[[]]'\n else\n oid=$(git rev-parse HEAD)\n printf '[[{{\"ref\":\"refs/heads/%s\",\"object\":{{\"sha\":\"%s\"}}}}]]\\n' \"$ref\" \"$oid\"\n fi ;;\n */pulls) case \"$*\" in *--head*|*'--limit 0'*) exit 1 ;; esac; printf '%s\\n' '[{}]' ;;\n *) exit 1 ;;\n esac ;;\npr:create) {}\n*) exit 1 ;;\nesac\n", invocations.display(), base_missing.display(), + head_missing.display(), pull_requests, create_response ), From 1787103131d4d61dd15d0bc48199fed4fc06fd70 Mon Sep 17 00:00:00 2001 From: Adrian Cosentino Date: Tue, 14 Jul 2026 13:08:11 -0400 Subject: [PATCH 16/16] fix: preserve fork and ssh remote semantics --- crates/bitbygit-git/src/lib.rs | 92 +++++++++++++++++++++++++++++++--- crates/bitbygit-tui/src/lib.rs | 77 +++++++++++++++++++++++++--- 2 files changed, 156 insertions(+), 13 deletions(-) diff --git a/crates/bitbygit-git/src/lib.rs b/crates/bitbygit-git/src/lib.rs index bdaa2c5..657dd20 100644 --- a/crates/bitbygit-git/src/lib.rs +++ b/crates/bitbygit-git/src/lib.rs @@ -1,4 +1,5 @@ use std::collections::BTreeMap; +use std::env; use std::error::Error; use std::ffi::OsString; use std::fmt::{self, Display, Formatter}; @@ -872,12 +873,14 @@ impl Git { .env("GIT_ASKPASS", "") .env("SSH_ASKPASS", "") .env("SSH_ASKPASS_REQUIRE", "never"); - let ssh_executable = self - .ssh_executable - .as_ref() - .map(|path| shell_quote(&path.to_string_lossy())) - .unwrap_or_else(|| "ssh".to_owned()); - command.env("GIT_SSH_COMMAND", format!("{ssh_executable} {SSH_OPTIONS}")); + if self.ssh_executable.is_some() || env::var_os("GIT_SSH_COMMAND").is_none() { + let ssh_executable = self + .ssh_executable + .as_ref() + .map(|path| shell_quote(&path.to_string_lossy())) + .unwrap_or_else(|| "ssh".to_owned()); + command.env("GIT_SSH_COMMAND", format!("{ssh_executable} {SSH_OPTIONS}")); + } let output = command .args(&args) .output() @@ -2695,6 +2698,83 @@ mod tests { Ok(()) } + #[cfg(unix)] + #[test] + fn fetch_preserves_inherited_ssh_command() -> Result<(), Box> { + use std::os::unix::fs::PermissionsExt; + + if let Some(repo) = env::var_os("BITBYGIT_TEST_INHERITED_SSH_REPO") { + Git::new(repo).fetch_remote_branch("origin", "main")?; + return Ok(()); + } + + let remote = TempRepo::new()?; + remote.run(["init", "--bare"])?; + let repo = TempRepo::new()?; + repo.run(["init", "-b", "main"])?; + repo.run(["config", "user.email", "bitbygit@example.invalid"])?; + repo.run(["config", "user.name", "bitbygit test"])?; + repo.run(["commit", "--allow-empty", "-m", "initial"])?; + let remote_path = remote.path().to_string_lossy().into_owned(); + repo.run_args(&["remote", "add", "origin", &remote_path])?; + repo.run(["push", "origin", "main"])?; + repo.run([ + "remote", + "set-url", + "origin", + "ssh://git@127.0.0.1/repo.git", + ])?; + + let inherited_marker = repo.path().join("inherited-ssh-ran"); + let inherited_ssh = repo.path().join("inherited-ssh"); + fs::write( + &inherited_ssh, + "#!/bin/sh\ntouch \"$BITBYGIT_TEST_INHERITED_SSH_MARKER\"\nexec git-upload-pack \"$BITBYGIT_TEST_INHERITED_SSH_REMOTE\"\n", + )?; + let mut permissions = fs::metadata(&inherited_ssh)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&inherited_ssh, permissions)?; + + let configured_marker = repo.path().join("configured-ssh-ran"); + let configured_ssh = repo.path().join("configured-ssh"); + fs::write( + &configured_ssh, + format!( + "#!/bin/sh\ntouch '{}'\nexit 1\n", + configured_marker.display() + ), + )?; + let mut permissions = fs::metadata(&configured_ssh)?.permissions(); + permissions.set_mode(0o755); + fs::set_permissions(&configured_ssh, permissions)?; + repo.run_args(&[ + "config", + "core.sshCommand", + &configured_ssh.display().to_string(), + ])?; + + let output = Command::new(env::current_exe()?) + .args([ + "--exact", + "tests::fetch_preserves_inherited_ssh_command", + "--nocapture", + ]) + .env("BITBYGIT_TEST_INHERITED_SSH_REPO", repo.path()) + .env("BITBYGIT_TEST_INHERITED_SSH_REMOTE", remote.path()) + .env("BITBYGIT_TEST_INHERITED_SSH_MARKER", &inherited_marker) + .env("GIT_SSH_COMMAND", &inherited_ssh) + .output()?; + + assert!( + output.status.success(), + "child test failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + assert!(inherited_marker.exists()); + assert!(!configured_marker.exists()); + Ok(()) + } + #[cfg(unix)] #[test] fn reads_non_utf8_repository_root() -> Result<(), Box> { diff --git a/crates/bitbygit-tui/src/lib.rs b/crates/bitbygit-tui/src/lib.rs index 8f81e4b..c076f9d 100644 --- a/crates/bitbygit-tui/src/lib.rs +++ b/crates/bitbygit-tui/src/lib.rs @@ -1941,7 +1941,7 @@ impl OperationPlanner { "Open pull request blocked: current branch needs a commit before opening a pull request." .to_owned() })?; - let github_repository = github_base_repository(&git, &head_github_repository)?; + let github_repository = github_base_repository(&git, &remote, &head_github_repository)?; let github = self.github(&github_repository); let repository = github.repository().map_err(open_pull_request_gh_error)?; let head_github = self.github(&head_github_repository); @@ -2136,12 +2136,24 @@ fn github_repository_from_push_url(url: &str) -> Option { Some(format!("github.com/{owner}/{repository}")) } -fn github_base_repository(git: &Git, head_repository: &str) -> Result { - let upstream = git +fn github_base_repository( + git: &Git, + push_remote: &str, + head_repository: &str, +) -> Result { + let remotes = git .remotes() - .map_err(|error| format!("Unable to prepare pull request base remote: {error}"))? - .into_iter() - .find(|remote| remote.name == "upstream"); + .map_err(|error| format!("Unable to prepare pull request base remote: {error}"))?; + if let Some(fetch_repository) = remotes + .iter() + .find(|remote| remote.name == push_remote) + .and_then(|remote| remote.fetch_url.as_deref()) + .and_then(github_repository_from_push_url) + .filter(|repository| !repository.eq_ignore_ascii_case(head_repository)) + { + return Ok(fetch_repository); + } + let upstream = remotes.into_iter().find(|remote| remote.name == "upstream"); let Some(upstream) = upstream else { return Ok(head_repository.to_owned()); }; @@ -2936,7 +2948,7 @@ impl PlanExecutor { remote_urls, target, )?; - if !github_base_repository(git, head_github_repository) + if !github_base_repository(git, remote, head_github_repository) .map_err(StepExecutionError::Blocked)? .eq_ignore_ascii_case(github_repository) { @@ -4706,6 +4718,57 @@ mod tests { Ok(()) } + #[test] + fn pull_request_targets_fetch_repository_for_a_split_remote() -> Result<(), Box> { + let repo = pushed_branch_repo("open-pr-split-remote")?; + git_stdout( + &repo, + &[ + "remote", + "set-url", + "origin", + "https://github.com/upstream/repo.git", + ], + )?; + git_stdout( + &repo, + &[ + "config", + "--add", + "url.https://github.com/octo/repo.git.pushInsteadOf", + "https://github.com/upstream/repo.git", + ], + )?; + let fake_gh = fake_gh("open-pr-split-remote", false)?; + let planner = OperationPlanner { + repo_root: repo.clone(), + github_executable: Some(fake_gh.clone()), + ssh_executable: Some(test_ssh_command()?), + }; + + let operation = planner + .plan_request(OperationRequest::OpenPullRequest { base: None }) + .map_err(std::io::Error::other)?; + + let preview = operation.plan.preview_text(); + assert!(preview.contains("remote: origin")); + assert!(preview.contains("head: octo:feature/open-pr")); + assert!(preview.contains("https://github.com/upstream/repo/compare/main")); + let execution = PlanExecutor::with_audit_paths_and_ssh( + &repo, + isolated_store_paths("open-pr-split-remote-audit")?, + test_ssh_command()?, + ) + .execute(&operation.plan, operation.context); + + assert!(execution.succeeded(), "{}", execution.message()); + let invocations = std::fs::read_to_string(fake_gh.with_file_name("invocations"))?; + assert!(invocations.contains("repos/upstream/repo/pulls")); + assert!(invocations.contains("head=octo:feature/open-pr")); + assert!(invocations.contains("--repo github.com/upstream/repo")); + Ok(()) + } + #[test] fn pull_request_uses_default_simple_in_triangular_workflow() -> Result<(), Box> { let repo = pushed_branch_repo("open-pr-triangular-fork")?;