diff --git a/crates/server/src/routes/task_attempts.rs b/crates/server/src/routes/task_attempts.rs index bdb278d9..4b1fe668 100644 --- a/crates/server/src/routes/task_attempts.rs +++ b/crates/server/src/routes/task_attempts.rs @@ -1117,13 +1117,6 @@ pub async fn create_github_pr( }; // Create GitHub service instance let github_service = GitHubService::new(&github_token)?; - if let Err(e) = github_service.check_token().await { - if e.is_api_data() { - return Ok(ResponseJson(ApiResponse::error_with_data(e))); - } else { - return Err(ApiError::GitHubService(e)); - } - } // Get the task attempt to access the stored base branch let base_branch = request.base_branch.unwrap_or_else(|| { // Use the stored base branch from the task attempt as the default @@ -1147,11 +1140,6 @@ pub async fn create_github_pr( .await? .ok_or(ApiError::Project(ProjectError::ProjectNotFound))?; - // Use GitService to get the remote URL, then create GitHubRepoInfo - let repo_info = deployment - .git() - .get_github_repo_info(&project.git_repo_path)?; - // Get branch name from task attempt let branch_name = task_attempt.branch.as_ref().ok_or_else(|| { ApiError::TaskAttempt(TaskAttemptError::ValidationError( @@ -1207,6 +1195,10 @@ pub async fn create_github_pr( head_branch: branch_name.clone(), base_branch: norm_base_branch_name.clone(), }; + // Use GitService to get the remote URL, then create GitHubRepoInfo + let repo_info = deployment + .git() + .get_github_repo_info(&project.git_repo_path)?; match github_service.create_pr(&repo_info, &pr_request).await { Ok(pr_info) => { @@ -1223,6 +1215,10 @@ pub async fn create_github_pr( tracing::error!("Failed to update task attempt PR status: {}", e); } + // Auto-open PR in browser + if let Err(e) = utils::browser::open_browser(&pr_info.url).await { + tracing::warn!("Failed to open PR in browser: {}", e); + } deployment .track_if_analytics_allowed( "github_pr_created", diff --git a/crates/services/src/services/git.rs b/crates/services/src/services/git.rs index 713123d5..42967dae 100644 --- a/crates/services/src/services/git.rs +++ b/crates/services/src/services/git.rs @@ -2,10 +2,9 @@ use std::{collections::HashMap, path::Path}; use chrono::{DateTime, Utc}; use git2::{ - BranchType, Delta, DiffFindOptions, DiffOptions, Error as GitError, FetchOptions, Reference, - Remote, Repository, Sort, build::CheckoutBuilder, + BranchType, Delta, DiffFindOptions, DiffOptions, Error as GitError, Reference, Remote, + Repository, Sort, build::CheckoutBuilder, }; -use regex; use serde::{Deserialize, Serialize}; use thiserror::Error; use ts_rs::TS; @@ -21,6 +20,8 @@ pub enum GitServiceError { #[error(transparent)] Git(#[from] GitError), #[error(transparent)] + GitCLI(#[from] GitCliError), + #[error(transparent)] IoError(#[from] std::io::Error), #[error("Invalid repository: {0}")] InvalidRepository(String), @@ -30,18 +31,13 @@ pub enum GitServiceError { MergeConflicts(String), #[error("Branches diverged: {0}")] BranchesDiverged(String), - #[error("Invalid path: {0}")] - InvalidPath(String), #[error("{0} has uncommitted changes: {1}")] WorktreeDirty(String, String), - #[error("Invalid file paths: {0}")] - InvalidFilePaths(String), #[error("No GitHub token available.")] TokenUnavailable, #[error("Rebase in progress; resolve or abort it before retrying")] RebaseInProgress, } - /// Service for managing Git operations in task execution workflows #[derive(Clone)] pub struct GitService {} @@ -763,7 +759,7 @@ impl GitService { } .into_reference(); let remote = self.get_remote_from_branch_ref(&repo, &base_branch_ref)?; - self.fetch_from_remote(&repo, &github_token, &remote)?; + self.fetch_all_from_remote(&repo, &github_token, &remote)?; self.get_branch_status_inner(&repo, &branch_ref, &base_branch_ref) } @@ -1206,7 +1202,12 @@ impl GitService { let github_token = github_token.ok_or(GitServiceError::TokenUnavailable)?; let remote = self.get_remote_from_branch_ref(&main_repo, &nbr)?; // First, fetch the latest changes from remote - self.fetch_from_remote(&main_repo, &github_token, &remote)?; + self.fetch_branch_from_remote( + &main_repo, + &github_token, + &remote, + &new_base_branch_name, + )?; } // Ensure identity for any commits produced by rebase @@ -1471,20 +1472,9 @@ impl GitService { let url = remote .url() .ok_or_else(|| GitServiceError::InvalidRepository("Remote has no URL".to_string()))?; - - // Parse GitHub URL (supports both HTTPS and SSH formats) - let github_regex = regex::Regex::new(r"github\.com[:/]([^/]+)/(.+?)(?:\.git)?/?$") - .map_err(|e| GitServiceError::InvalidRepository(format!("Regex error: {e}")))?; - - if let Some(captures) = github_regex.captures(url) { - let owner = captures.get(1).unwrap().as_str().to_string(); - let repo_name = captures.get(2).unwrap().as_str().to_string(); - Ok(GitHubRepoInfo { owner, repo_name }) - } else { - Err(GitServiceError::InvalidRepository(format!( - "Not a GitHub repository: {url}" - ))) - } + GitHubRepoInfo::from_remote_url(url).map_err(|e| { + GitServiceError::InvalidRepository(format!("Failed to parse remote URL: {e}")) + }) } pub fn get_remote_name_from_branch_name( @@ -1541,56 +1531,34 @@ impl GitService { .url() .ok_or_else(|| GitServiceError::InvalidRepository("Remote has no URL".to_string()))?; let https_url = self.convert_to_https_url(remote_url); + let git_cli = GitCli::new(); + if let Err(e) = + git_cli.push_with_token(worktree_path, &https_url, branch_name, github_token) + { + tracing::error!("Push to GitHub failed: {}", e); + return Err(e.into()); + } - // Create a temporary remote with HTTPS URL for pushing - let temp_remote_name = "temp_https_origin"; - - // Remove any existing temp remote - let _ = repo.remote_delete(temp_remote_name); - - // Create temporary HTTPS remote - let mut temp_remote = repo.remote(temp_remote_name, &https_url)?; - - // Create refspec for pushing the branch - let refspec = format!("refs/heads/{branch_name}:refs/heads/{branch_name}"); - - // Set up authentication callback using the GitHub token - let mut callbacks = git2::RemoteCallbacks::new(); - callbacks.credentials(|_url, username_from_url, _allowed_types| { - git2::Cred::userpass_plaintext(username_from_url.unwrap_or("git"), github_token) - }); - - // Configure push options - let mut push_options = git2::PushOptions::new(); - push_options.remote_callbacks(callbacks); - - // Push the branch - let push_result = temp_remote.push(&[&refspec], Some(&mut push_options)); - - // Clean up the temporary remote - let _ = repo.remote_delete(temp_remote_name); - - // Check push result - push_result.map_err(|e| match e.code() { - git2::ErrorCode::NotFastForward => { - GitServiceError::BranchesDiverged(format!( - "Push failed: branch '{branch_name}' has diverged and cannot be fast-forwarded. Either merge the changes or force push." - )) - } - _ => e.into(), - })?; - self.fetch_from_remote(&repo, github_token, &remote)?; let mut branch = Self::find_branch(&repo, branch_name)?; if !branch.get().is_remote() { + if let Some(branch_target) = branch.get().target() { + let remote_ref = format!("refs/remotes/{remote_name}/{branch_name}"); + repo.reference( + &remote_ref, + branch_target, + true, + "update remote tracking branch", + )?; + } branch.set_upstream(Some(&format!("{remote_name}/{branch_name}")))?; } Ok(()) } - fn convert_to_https_url(&self, url: &str) -> String { + pub fn convert_to_https_url(&self, url: &str) -> String { // Convert SSH URL to HTTPS URL if necessary - if url.starts_with("git@github.com:") { + let new_url = if url.starts_with("git@github.com:") { // Convert git@github.com:owner/repo.git to https://github.com/owner/repo.git url.replace("git@github.com:", "https://github.com/") } else if url.starts_with("ssh://git@github.com/") { @@ -1598,7 +1566,13 @@ impl GitService { url.replace("ssh://git@github.com/", "https://github.com/") } else { url.to_string() + }; + let mut normalized = new_url.trim_end_matches('/').to_string(); + if !normalized.ends_with(".git") { + normalized.push_str(".git"); } + + normalized } /// Fetch from remote repository using GitHub token authentication @@ -1607,44 +1581,50 @@ impl GitService { repo: &Repository, github_token: &str, remote: &Remote, + refspec: &str, ) -> Result<(), GitServiceError> { // Get the remote let remote_url = remote .url() .ok_or_else(|| GitServiceError::InvalidRepository("Remote has no URL".to_string()))?; - // Create a temporary remote with HTTPS URL for fetching - let temp_remote_name = "temp_https_origin"; - - // Remove any existing temp remote - let _ = repo.remote_delete(temp_remote_name); - let https_url = self.convert_to_https_url(remote_url); // Create temporary HTTPS remote - let mut temp_remote = repo.remote(temp_remote_name, &https_url)?; + let git_cli = GitCli::new(); + if let Err(e) = + git_cli.fetch_with_token_and_refspec(repo.path(), &https_url, refspec, github_token) + { + tracing::error!("Fetch from GitHub failed: {}", e); + return Err(e.into()); + } + Ok(()) + } - // Set up authentication callback using the GitHub token - let mut callbacks = git2::RemoteCallbacks::new(); - callbacks.credentials(|_url, username_from_url, _allowed_types| { - git2::Cred::userpass_plaintext(username_from_url.unwrap_or("git"), github_token) - }); - - // Configure fetch options - let mut fetch_opts = FetchOptions::new(); - fetch_opts.remote_callbacks(callbacks); + /// Fetch from remote repository using GitHub token authentication + fn fetch_branch_from_remote( + &self, + repo: &Repository, + github_token: &str, + remote: &Remote, + branch_name: &str, + ) -> Result<(), GitServiceError> { let default_remote_name = self.default_remote_name(repo); let remote_name = remote.name().unwrap_or(&default_remote_name); + let refspec = format!("+refs/heads/{branch_name}:refs/remotes/{remote_name}/{branch_name}"); + self.fetch_from_remote(repo, github_token, remote, &refspec) + } + /// Fetch from remote repository using GitHub token authentication + fn fetch_all_from_remote( + &self, + repo: &Repository, + github_token: &str, + remote: &Remote, + ) -> Result<(), GitServiceError> { + let default_remote_name = self.default_remote_name(repo); + let remote_name = remote.name().unwrap_or(&default_remote_name); let refspec = format!("+refs/heads/*:refs/remotes/{remote_name}/*"); - - let fetch_result = temp_remote.fetch(&[&refspec], Some(&mut fetch_opts), None); - // Clean up the temporary remote - let _ = repo.remote_delete(temp_remote_name); - - // Check fetch result - fetch_result.map_err(GitServiceError::Git)?; - - Ok(()) + self.fetch_from_remote(repo, github_token, remote, &refspec) } /// Clone a repository to the specified directory diff --git a/crates/services/src/services/git_cli.rs b/crates/services/src/services/git_cli.rs index d19653ac..2498a2df 100644 --- a/crates/services/src/services/git_cli.rs +++ b/crates/services/src/services/git_cli.rs @@ -21,6 +21,7 @@ use std::{ process::Command, }; +use base64::{Engine, engine::general_purpose::STANDARD as BASE64_STANDARD}; use thiserror::Error; use utils::shell::resolve_executable_path; @@ -30,6 +31,10 @@ pub enum GitCliError { NotAvailable, #[error("git command failed: {0}")] CommandFailed(String), + #[error("authentication failed: {0}")] + AuthFailed(String), + #[error("push rejected: {0}")] + PushRejected(String), #[error("rebase in progress in this worktree")] RebaseInProgress, } @@ -291,6 +296,62 @@ impl GitCli { self.git(worktree_path, ["commit", "-m", message])?; Ok(()) } + /// Fetch a branch to the given remote using an HTTPS token for authentication. + pub fn fetch_with_token_and_refspec( + &self, + repo_path: &Path, + remote_url: &str, + refspec: &str, + token: &str, + ) -> Result<(), GitCliError> { + let auth_header = self.build_auth_header(token); + let envs = self.build_token_env(&auth_header); + + let args = [ + OsString::from("-c"), + OsString::from("credential.helper="), + OsString::from("--config-env"), + OsString::from("http.extraHeader=GIT_HTTP_EXTRAHEADER"), + OsString::from("fetch"), + OsString::from(remote_url), + OsString::from(refspec), + ]; + + match self.git_with_env(repo_path, args, &envs) { + Ok(_) => Ok(()), + Err(GitCliError::CommandFailed(msg)) => Err(self.classify_cli_error(msg)), + Err(err) => Err(err), + } + } + + /// Push a branch to the given remote using an HTTPS token for authentication. + pub fn push_with_token( + &self, + repo_path: &Path, + remote_url: &str, + branch: &str, + token: &str, + ) -> Result<(), GitCliError> { + let refspec = format!("refs/heads/{branch}:refs/heads/{branch}"); + let auth_header = self.build_auth_header(token); + let envs = self.build_token_env(&auth_header); + + let args = [ + OsString::from("-c"), + OsString::from("credential.helper="), + OsString::from("--config-env"), + OsString::from("http.extraHeader=GIT_HTTP_EXTRAHEADER"), + OsString::from("push"), + OsString::from(remote_url), + OsString::from(refspec), + ]; + + match self.git_with_env(repo_path, args, &envs) { + Ok(_) => Ok(()), + Err(GitCliError::CommandFailed(msg)) => Err(self.classify_cli_error(msg)), + Err(err) => Err(err), + } + } // Parse `git diff --name-status` output into structured entries. // Handles rename/copy scores like `R100` by matching the first letter. @@ -507,6 +568,41 @@ impl GitCli { // Private methods impl GitCli { + fn classify_cli_error(&self, msg: String) -> GitCliError { + let lower = msg.to_ascii_lowercase(); + if lower.contains("authentication failed") + || lower.contains("could not read username") + || lower.contains("invalid username or password") + { + GitCliError::AuthFailed(msg) + } else if lower.contains("non-fast-forward") + || lower.contains("failed to push some refs") + || lower.contains("fetch first") + || lower.contains("updates were rejected because the tip") + { + GitCliError::PushRejected(msg) + } else { + GitCliError::CommandFailed(msg) + } + } + + fn build_auth_header(&self, token: &str) -> String { + let auth_value = BASE64_STANDARD.encode(format!("x-access-token:{token}")); + format!("Authorization: Basic {auth_value}") + } + + fn build_token_env(&self, auth_header: &str) -> Vec<(OsString, OsString)> { + vec![ + (OsString::from("GIT_TERMINAL_PROMPT"), OsString::from("0")), + (OsString::from("GIT_ASKPASS"), OsString::from("")), + (OsString::from("SSH_ASKPASS"), OsString::from("")), + ( + OsString::from("GIT_HTTP_EXTRAHEADER"), + OsString::from(auth_header), + ), + ] + } + /// Ensure `git` is available on PATH fn ensure_available(&self) -> Result<(), GitCliError> { let git = resolve_executable_path("git").ok_or(GitCliError::NotAvailable)?; diff --git a/crates/services/src/services/github_service.rs b/crates/services/src/services/github_service.rs index f9406be1..8054f63a 100644 --- a/crates/services/src/services/github_service.rs +++ b/crates/services/src/services/github_service.rs @@ -2,13 +2,14 @@ use std::time::Duration; use backon::{ExponentialBuilder, Retryable}; use db::models::merge::{MergeStatus, PullRequestInfo}; -use octocrab::{Octocrab, OctocrabBuilder}; +use octocrab::{Octocrab, OctocrabBuilder, models::IssueState}; +use regex::Regex; use serde::{Deserialize, Serialize}; use thiserror::Error; use tracing::info; use ts_rs::TS; -use crate::services::git::GitServiceError; +use crate::services::{git::GitServiceError, git_cli::GitCliError}; #[derive(Debug, Error, Serialize, Deserialize, TS)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] @@ -19,9 +20,6 @@ pub enum GitHubServiceError { #[error(transparent)] Client(octocrab::Error), #[ts(skip)] - #[error("Authentication error: {0}")] - Auth(String), - #[ts(skip)] #[error("Repository error: {0}")] Repository(String), #[ts(skip)] @@ -63,21 +61,19 @@ impl From for GitHubServiceError { } impl From for GitHubServiceError { fn from(error: GitServiceError) -> Self { - if let GitServiceError::Git(err) = error { - if err - .message() - .contains("too many redirects or authentication replays") - { - Self::TokenInvalid - } else if err.message().contains("status code: 403") { - Self::InsufficientPermissions - } else if err.message().contains("status code: 404") { - Self::RepoNotFoundOrNoAccess - } else { - Self::GitService(GitServiceError::Git(err)) + match error { + GitServiceError::GitCLI(GitCliError::AuthFailed(_)) => Self::TokenInvalid, + GitServiceError::GitCLI(GitCliError::CommandFailed(msg)) => { + let lower = msg.to_ascii_lowercase(); + if lower.contains("the requested url returned error: 403") { + Self::InsufficientPermissions + } else if lower.contains("the requested url returned error: 404") { + Self::RepoNotFoundOrNoAccess + } else { + Self::GitService(GitServiceError::GitCLI(GitCliError::CommandFailed(msg))) + } } - } else { - Self::GitService(error) + other => Self::GitService(other), } } } @@ -91,6 +87,10 @@ impl GitHubServiceError { | GitHubServiceError::RepoNotFoundOrNoAccess ) } + + pub fn should_retry(&self) -> bool { + !self.is_api_data() + } } #[derive(Debug, Clone)] @@ -99,16 +99,21 @@ pub struct GitHubRepoInfo { pub repo_name: String, } impl GitHubRepoInfo { - pub fn from_pr_url(pr_url: &str) -> Result { - let re = regex::Regex::new(r"github\.com/(?P[^/]+)/(?P[^/]+)").unwrap(); - let caps = re - .captures(pr_url) - .ok_or_else(|| sqlx::Error::ColumnNotFound("Invalid URL format".into()))?; + pub fn from_remote_url(remote_url: &str) -> Result { + // Supports SSH, HTTPS and PR GitHub URLs. See tests for examples. + let re = Regex::new(r"github\.com[:/](?P[^/]+)/(?P[^/]+?)(?:\.git)?(?:/|$)") + .map_err(|e| { + GitHubServiceError::Repository(format!("Failed to compile regex: {e}")) + })?; - let owner = caps.name("owner").unwrap().as_str().to_string(); - let repo_name = caps.name("repo").unwrap().as_str().to_string(); + let caps = re.captures(remote_url).ok_or_else(|| { + GitHubServiceError::Repository(format!("Invalid GitHub URL format: {remote_url}")) + })?; - Ok(Self { owner, repo_name }) + Ok(Self { + owner: caps.name("owner").unwrap().as_str().to_string(), + repo_name: caps.name("repo").unwrap().as_str().to_string(), + }) } } @@ -167,10 +172,7 @@ impl GitHubService { .with_max_times(3) .with_jitter(), ) - .when(|e| { - !matches!(e, GitHubServiceError::TokenInvalid) - && !matches!(e, GitHubServiceError::Branch(_)) - }) + .when(|e| e.should_retry()) .notify(|err: &GitHubServiceError, dur: Duration| { tracing::warn!( "GitHub API call failed, retrying after {:.2}s: {}", @@ -191,82 +193,61 @@ impl GitHubService { .repos(&repo_info.owner, &repo_info.repo_name) .get() .await - .map_err(|e| { - GitHubServiceError::Repository(format!( + .map_err(|error| match GitHubServiceError::from(error) { + GitHubServiceError::Client(source) => GitHubServiceError::Repository(format!( "Cannot access repository {}/{}: {}", - repo_info.owner, repo_info.repo_name, e - )) + repo_info.owner, repo_info.repo_name, source + )), + other => other, })?; // Check if the base branch exists self.client .repos(&repo_info.owner, &repo_info.repo_name) .get_ref(&octocrab::params::repos::Reference::Branch( - request.base_branch.clone(), + request.base_branch.to_string(), )) .await - .map_err(|e| { - GitHubServiceError::Branch(format!( + .map_err(|err| match GitHubServiceError::from(err) { + GitHubServiceError::Client(source) => GitHubServiceError::Branch(format!( "Base branch '{}' does not exist: {}", - request.base_branch, e - )) + request.base_branch, source + )), + other => other, })?; // Check if the head branch exists self.client .repos(&repo_info.owner, &repo_info.repo_name) .get_ref(&octocrab::params::repos::Reference::Branch( - request.head_branch.clone(), + request.head_branch.to_string(), )) .await - .map_err(|e| { - GitHubServiceError::Branch(format!( - "Head branch '{}' does not exist. Make sure the branch was pushed successfully: {}", - request.head_branch, e - )) + .map_err(|err| match GitHubServiceError::from(err) { + GitHubServiceError::Client(source) => GitHubServiceError::Branch(format!( + "Head branch '{}' does not exist: {}", + request.head_branch, source + )), + other => other, })?; // Create the pull request - let pr = self + let pr_info = self .client .pulls(&repo_info.owner, &repo_info.repo_name) .create(&request.title, &request.head_branch, &request.base_branch) .body(request.body.as_deref().unwrap_or("")) .send() .await - .map_err(|e| match e { - octocrab::Error::GitHub { source, .. } => { - if source.status_code.as_u16() == 401 - || source.status_code.as_u16() == 403 - || source - .message - .to_ascii_lowercase() - .contains("bad credentials") - || source - .message - .to_ascii_lowercase() - .contains("token expired") - { - GitHubServiceError::TokenInvalid - } else { - GitHubServiceError::PullRequest(format!( - "GitHub API error: {} (status: {})", - source.message, - source.status_code.as_u16() - )) - } - } - _ => GitHubServiceError::PullRequest(format!("Failed to create PR: {e}")), + .map(Self::map_pull_request) + .map_err(|err| match GitHubServiceError::from(err) { + GitHubServiceError::Client(source) => GitHubServiceError::PullRequest(format!( + "Failed to create PR for '{} -> {}': {}", + request.head_branch, request.base_branch, source + )), + other => other, })?; - let pr_info = PullRequestInfo { - number: pr.number as i64, - url: pr.html_url.map(|url| url.to_string()).unwrap_or_default(), - status: MergeStatus::Open, - merged_at: None, - merge_commit_sha: None, - }; - info!( "Created GitHub PR #{} for branch {} in {}/{}", pr_info.number, request.head_branch, repo_info.owner, repo_info.repo_name @@ -281,42 +262,41 @@ impl GitHubService { repo_info: &GitHubRepoInfo, pr_number: i64, ) -> Result { - (|| async { self.update_pr_status_internal(repo_info, pr_number).await }) - .retry( - &ExponentialBuilder::default() - .with_min_delay(Duration::from_secs(1)) - .with_max_delay(Duration::from_secs(30)) - .with_max_times(3) - .with_jitter(), - ) - .when(|e| !matches!(e, GitHubServiceError::TokenInvalid)) - .notify(|err: &GitHubServiceError, dur: Duration| { - tracing::warn!( - "GitHub API call failed, retrying after {:.2}s: {}", - dur.as_secs_f64(), - err - ); - }) - .await + (|| async { + self.client + .pulls(&repo_info.owner, &repo_info.repo_name) + .get(pr_number as u64) + .await + .map(Self::map_pull_request) + .map_err(|err| match GitHubServiceError::from(err) { + GitHubServiceError::Client(source) => GitHubServiceError::PullRequest(format!( + "Failed to get PR #{pr_number}: {source}", + )), + other => other, + }) + }) + .retry( + &ExponentialBuilder::default() + .with_min_delay(Duration::from_secs(1)) + .with_max_delay(Duration::from_secs(30)) + .with_max_times(3) + .with_jitter(), + ) + .when(|err| err.should_retry()) + .notify(|err: &GitHubServiceError, dur: Duration| { + tracing::warn!( + "GitHub API call failed, retrying after {:.2}s: {}", + dur.as_secs_f64(), + err + ); + }) + .await } - async fn update_pr_status_internal( - &self, - repo_info: &GitHubRepoInfo, - pr_number: i64, - ) -> Result { - let pr = self - .client - .pulls(&repo_info.owner, &repo_info.repo_name) - .get(pr_number as u64) - .await - .map_err(|e| { - GitHubServiceError::PullRequest(format!("Failed to get PR #{pr_number}: {e}")) - })?; - - let status = match pr.state { - Some(octocrab::models::IssueState::Open) => MergeStatus::Open, - Some(octocrab::models::IssueState::Closed) => { + fn map_pull_request(pr: octocrab::models::pulls::PullRequest) -> PullRequestInfo { + let state = match pr.state { + Some(IssueState::Open) => MergeStatus::Open, + Some(IssueState::Closed) => { if pr.merged_at.is_some() { MergeStatus::Merged } else { @@ -327,15 +307,13 @@ impl GitHubService { Some(_) => MergeStatus::Unknown, }; - let pr_info = PullRequestInfo { + PullRequestInfo { number: pr.number as i64, url: pr.html_url.map(|url| url.to_string()).unwrap_or_default(), - status, + status: state, merged_at: pr.merged_at.map(|dt| dt.naive_utc().and_utc()), - merge_commit_sha: pr.merge_commit_sha.clone(), - }; - - Ok(pr_info) + merge_commit_sha: pr.merge_commit_sha, + } } /// List repositories for the authenticated user with pagination @@ -352,7 +330,7 @@ impl GitHubService { .with_max_times(3) .with_jitter(), ) - .when(|e| !matches!(e, GitHubServiceError::TokenInvalid)) + .when(|err| err.should_retry()) .notify(|err: &GitHubServiceError, dur: Duration| { tracing::warn!( "GitHub API call failed, retrying after {:.2}s: {}", diff --git a/crates/services/src/services/pr_monitor.rs b/crates/services/src/services/pr_monitor.rs index ac10353d..1ec2c7e8 100644 --- a/crates/services/src/services/pr_monitor.rs +++ b/crates/services/src/services/pr_monitor.rs @@ -94,7 +94,7 @@ impl PrMonitorService { let github_service = GitHubService::new(&github_token)?; - let repo_info = GitHubRepoInfo::from_pr_url(&pr_merge.pr_info.url)?; + let repo_info = GitHubRepoInfo::from_remote_url(&pr_merge.pr_info.url)?; let pr_status = github_service .update_pr_status(&repo_info, pr_merge.pr_info.number) diff --git a/crates/services/tests/git_ops_safety.rs b/crates/services/tests/git_ops_safety.rs index 525d4cfe..93e7220c 100644 --- a/crates/services/tests/git_ops_safety.rs +++ b/crates/services/tests/git_ops_safety.rs @@ -4,9 +4,11 @@ use std::{ path::{Path, PathBuf}, }; -use git2::{Repository, build::CheckoutBuilder}; -use services::services::git::GitService; -use services::services::git_cli::GitCli; // used only to set up sparse-checkout +use git2::{PushOptions, Repository, build::CheckoutBuilder}; +use services::services::{ + git::GitService, + git_cli::{GitCli, GitCliError}, +}; use tempfile::TempDir; // Avoid direct git CLI usage in tests; exercise GitService instead. @@ -61,6 +63,15 @@ fn configure_user(repo: &Repository) { cfg.set_str("user.email", "test@example.com").unwrap(); } +fn push_ref(repo: &Repository, local: &str, remote: &str) { + let mut remote_handle = repo.find_remote("origin").unwrap(); + let mut opts = PushOptions::new(); + let spec = format!("+{local}:{remote}"); + remote_handle + .push(&[spec.as_str()], Some(&mut opts)) + .unwrap(); +} + use services::services::git::DiffTarget; // Non-conflicting setup used by several tests @@ -219,6 +230,182 @@ fn setup_direct_conflict_repo(root: &TempDir) -> (PathBuf, PathBuf) { (repo_path, worktree_path) } +#[test] +fn push_with_token_reports_non_fast_forward() { + let temp_dir = TempDir::new().unwrap(); + let remote_path = temp_dir.path().join("remote.git"); + Repository::init_bare(&remote_path).expect("init bare remote"); + let remote_url = remote_path.to_str().expect("remote path str"); + + // Seed the bare repo with an initial main branch commit + let seed_path = temp_dir.path().join("seed"); + let service = GitService::new(); + service + .initialize_repo_with_main_branch(&seed_path) + .expect("init seed repo"); + let seed_repo = Repository::open(&seed_path).expect("open seed repo"); + configure_user(&seed_repo); + seed_repo.remote("origin", remote_url).expect("add remote"); + push_ref(&seed_repo, "refs/heads/main", "refs/heads/main"); + Repository::open_bare(&remote_path) + .expect("open bare remote") + .set_head("refs/heads/main") + .expect("set remote HEAD"); + + // Local clone that will attempt the push later + let local_path = temp_dir.path().join("local"); + let local_repo = Repository::clone(remote_url, &local_path).expect("clone local"); + configure_user(&local_repo); + checkout_branch(&local_repo, "main"); + write_file(&local_path, "file.txt", "initial local\n"); + commit_all(&local_repo, "initial local commit"); + push_ref(&local_repo, "refs/heads/main", "refs/heads/main"); + + // Separate clone simulates someone else pushing first + let updater_path = temp_dir.path().join("updater"); + let updater_repo = Repository::clone(remote_url, &updater_path).expect("clone updater"); + configure_user(&updater_repo); + checkout_branch(&updater_repo, "main"); + write_file(&updater_path, "file.txt", "upstream change\n"); + commit_all(&updater_repo, "upstream commit"); + push_ref(&updater_repo, "refs/heads/main", "refs/heads/main"); + + // Local branch diverges but has not fetched the updater's commit + write_file(&local_path, "file.txt", "local change\n"); + commit_all(&local_repo, "local commit"); + let remote = local_repo.find_remote("origin").expect("origin remote"); + let remote_url_string = remote.url().expect("origin url").to_string(); + + let git_cli = GitCli::new(); + let result = git_cli.push_with_token(&local_path, &remote_url_string, "main", "dummy-token"); + match result { + Err(GitCliError::PushRejected(msg)) => { + let lower = msg.to_ascii_lowercase(); + assert!( + lower.contains("failed to push some refs") || lower.contains("fetch first"), + "unexpected stderr: {msg}" + ); + } + Err(other) => panic!("expected push rejected, got {other:?}"), + Ok(_) => panic!("push unexpectedly succeeded"), + } +} + +#[test] +fn fetch_with_token_missing_ref_returns_error() { + let temp_dir = TempDir::new().unwrap(); + let remote_path = temp_dir.path().join("remote.git"); + Repository::init_bare(&remote_path).expect("init bare remote"); + let remote_url = remote_path.to_str().expect("remote path str"); + + let seed_path = temp_dir.path().join("seed"); + let service = GitService::new(); + service + .initialize_repo_with_main_branch(&seed_path) + .expect("init seed repo"); + let seed_repo = Repository::open(&seed_path).expect("open seed repo"); + configure_user(&seed_repo); + seed_repo.remote("origin", remote_url).expect("add remote"); + push_ref(&seed_repo, "refs/heads/main", "refs/heads/main"); + Repository::open_bare(&remote_path) + .expect("open bare remote") + .set_head("refs/heads/main") + .expect("set remote HEAD"); + + let local_path = temp_dir.path().join("local"); + Repository::clone(remote_url, &local_path).expect("clone local"); + + let git_cli = GitCli::new(); + let refspec = "+refs/heads/missing:refs/remotes/origin/missing"; + let result = + git_cli.fetch_with_token_and_refspec(&local_path, remote_url, refspec, "dummy-token"); + match result { + Err(GitCliError::CommandFailed(msg)) => { + assert!( + msg.to_ascii_lowercase() + .contains("couldn't find remote ref"), + "unexpected stderr: {msg}" + ); + } + Err(other) => panic!("expected command failed, got {other:?}"), + Ok(_) => panic!("fetch unexpectedly succeeded"), + } +} + +#[test] +fn push_and_fetch_roundtrip_updates_tracking_branch() { + let temp_dir = TempDir::new().unwrap(); + let remote_path = temp_dir.path().join("remote.git"); + Repository::init_bare(&remote_path).expect("init bare remote"); + let remote_url = remote_path.to_str().expect("remote path str"); + + let seed_path = temp_dir.path().join("seed"); + let service = GitService::new(); + service + .initialize_repo_with_main_branch(&seed_path) + .expect("init seed repo"); + let seed_repo = Repository::open(&seed_path).expect("open seed repo"); + configure_user(&seed_repo); + seed_repo.remote("origin", remote_url).expect("add remote"); + push_ref(&seed_repo, "refs/heads/main", "refs/heads/main"); + Repository::open_bare(&remote_path) + .expect("open bare remote") + .set_head("refs/heads/main") + .expect("set remote HEAD"); + + let producer_path = temp_dir.path().join("producer"); + let producer_repo = Repository::clone(remote_url, &producer_path).expect("clone producer"); + configure_user(&producer_repo); + checkout_branch(&producer_repo, "main"); + + let consumer_path = temp_dir.path().join("consumer"); + let consumer_repo = Repository::clone(remote_url, &consumer_path).expect("clone consumer"); + configure_user(&consumer_repo); + checkout_branch(&consumer_repo, "main"); + let old_oid = consumer_repo + .find_reference("refs/remotes/origin/main") + .expect("consumer tracking ref") + .target() + .expect("consumer tracking ref"); + + write_file(&producer_path, "file.txt", "new work\n"); + commit_all(&producer_repo, "producer commit"); + + let remote = producer_repo.find_remote("origin").expect("origin remote"); + let remote_url_string = remote.url().expect("origin url").to_string(); + + let git_cli = GitCli::new(); + git_cli + .push_with_token(&producer_path, &remote_url_string, "main", "dummy-token") + .expect("push succeeded"); + + let new_oid = producer_repo + .head() + .expect("producer head") + .target() + .expect("producer head oid"); + assert_ne!(old_oid, new_oid, "producer created new commit"); + + git_cli + .fetch_with_token_and_refspec( + &consumer_path, + &remote_url_string, + "+refs/heads/main:refs/remotes/origin/main", + "dummy-token", + ) + .expect("fetch succeeded"); + + let updated_oid = consumer_repo + .find_reference("refs/remotes/origin/main") + .expect("updated tracking ref") + .target() + .expect("updated tracking ref"); + assert_eq!( + updated_oid, new_oid, + "tracking branch advanced to remote head" + ); +} + #[test] fn rebase_preserves_untracked_files() { let td = TempDir::new().unwrap(); diff --git a/crates/services/tests/git_remote_ops.rs b/crates/services/tests/git_remote_ops.rs new file mode 100644 index 00000000..f30710fe --- /dev/null +++ b/crates/services/tests/git_remote_ops.rs @@ -0,0 +1,88 @@ +use std::{ + net::{TcpStream, ToSocketAddrs}, + path::{Path, PathBuf}, + time::Duration, +}; + +use git2::Repository; +use services::services::{ + git::GitService, + git_cli::{GitCli, GitCliError}, +}; + +fn workspace_root() -> PathBuf { + // CARGO_MANIFEST_DIR for this crate is /crates/services + let manifest_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + manifest_dir + .parent() + .and_then(Path::parent) + .expect("workspace root") + .to_path_buf() +} + +fn repo_https_remote(repo_path: &Path) -> Option { + let repo = Repository::open(repo_path).ok()?; + let remote = repo.find_remote("origin").ok()?; + let url = remote.url()?; + Some(GitService::new().convert_to_https_url(url)) +} + +fn assert_auth_failed(result: Result<(), GitCliError>) { + match result { + Err(GitCliError::AuthFailed(_)) => {} + Err(other) => panic!("expected auth failure, got {other:?}"), + Ok(_) => panic!("operation unexpectedly succeeded"), + } +} + +fn can_reach_github() -> bool { + let addr = match ("github.com", 443).to_socket_addrs() { + Ok(mut addrs) => addrs.next(), + Err(_) => return false, + }; + if let Some(addr) = addr { + TcpStream::connect_timeout(&addr, Duration::from_secs(2)).is_ok() + } else { + false + } +} + +#[ignore] +#[test] +fn fetch_with_invalid_token_returns_auth_error() { + let repo_path = workspace_root(); + let Some(remote_url) = repo_https_remote(&repo_path) else { + eprintln!("Skipping fetch test: origin remote not configured"); + return; + }; + + if !can_reach_github() { + eprintln!("Skipping fetch test: cannot reach github.com"); + return; + } + + let cli = GitCli::new(); + let refspec = "+refs/heads/main:refs/remotes/origin/main"; + let result = + cli.fetch_with_token_and_refspec(&repo_path, &remote_url, refspec, "invalid-token"); + assert_auth_failed(result); +} + +#[ignore] +#[test] +fn push_with_invalid_token_returns_auth_error() { + let repo_path = workspace_root(); + let Some(remote_url) = repo_https_remote(&repo_path) else { + eprintln!("Skipping push test: origin remote not configured"); + return; + }; + + if !can_reach_github() { + eprintln!("Skipping push test: cannot reach github.com"); + return; + } + + let cli = GitCli::new(); + let result = cli.push_with_token(&repo_path, &remote_url, "main", "invalid-token"); + assert_auth_failed(result); +} diff --git a/crates/services/tests/git_workflow.rs b/crates/services/tests/git_workflow.rs index bb3a00ab..5af0ef26 100644 --- a/crates/services/tests/git_workflow.rs +++ b/crates/services/tests/git_workflow.rs @@ -4,7 +4,10 @@ use std::{ path::{Path, PathBuf}, }; -use services::services::git::{DiffTarget, GitService}; +use services::services::{ + git::{DiffTarget, GitService}, + github_service::{GitHubRepoInfo, GitHubServiceError}, +}; use tempfile::TempDir; use utils::diff::DiffChangeKind; @@ -537,6 +540,53 @@ fn delete_file_commit_has_author_without_user() { } } +#[test] +fn convert_to_https_url_handles_common_git_forms() { + let svc = GitService::new(); + + let ssh_url = "git@github.com:owner/repo.git"; + assert_eq!( + svc.convert_to_https_url(ssh_url), + "https://github.com/owner/repo.git" + ); + + let ssh_scheme_url = "ssh://git@github.com/owner/repo"; + assert_eq!( + svc.convert_to_https_url(ssh_scheme_url), + "https://github.com/owner/repo.git" + ); + + let https_without_suffix = "https://github.com/owner/repo"; + assert_eq!( + svc.convert_to_https_url(https_without_suffix), + "https://github.com/owner/repo.git" + ); + + let converted = svc.convert_to_https_url("https://github.com/owner/repo/"); + assert_eq!(converted, "https://github.com/owner/repo.git"); +} + +#[test] +fn github_repo_info_parses_https_and_ssh_urls() { + let info = GitHubRepoInfo::from_remote_url("https://github.com/owner/repo.git").unwrap(); + assert_eq!(info.owner, "owner"); + assert_eq!(info.repo_name, "repo"); + + let info = GitHubRepoInfo::from_remote_url("git@github.com:owner/repo.git").unwrap(); + assert_eq!(info.owner, "owner"); + assert_eq!(info.repo_name, "repo"); + + let info = GitHubRepoInfo::from_remote_url("https://github.com/owner/repo/pull/123").unwrap(); + assert_eq!(info.owner, "owner"); + assert_eq!(info.repo_name, "repo"); + + let err = GitHubRepoInfo::from_remote_url("https://example.com/not/github").unwrap_err(); + match err { + GitHubServiceError::Repository(msg) => assert!(msg.contains("Invalid GitHub URL")), + other => panic!("unexpected error variant: {other:?}"), + } +} + #[test] fn squash_merge_libgit2_sets_author_without_user() { // Verify merge_changes (libgit2 path) uses fallback author when no config exists diff --git a/frontend/src/components/dialogs/tasks/CreatePRDialog.tsx b/frontend/src/components/dialogs/tasks/CreatePRDialog.tsx index 0af72bfe..30da7fea 100644 --- a/frontend/src/components/dialogs/tasks/CreatePRDialog.tsx +++ b/frontend/src/components/dialogs/tasks/CreatePRDialog.tsx @@ -21,6 +21,7 @@ import { TaskWithAttemptStatus, } from 'shared/types'; import { projectsApi } from '@/lib/api.ts'; +import { Loader2 } from 'lucide-react'; import NiceModal, { useModal } from '@ebay/nice-modal-react'; const CreatePrDialog = NiceModal.create(() => { const modal = useModal(); @@ -79,7 +80,6 @@ const CreatePrDialog = NiceModal.create(() => { if (result.success) { setError(null); // Clear any previous errors on success - window.open(result.data, '_blank'); // Reset form and close dialog setPrTitle(''); setPrBody(''); @@ -181,7 +181,14 @@ const CreatePrDialog = NiceModal.create(() => { disabled={creatingPR || !prTitle.trim()} className="bg-blue-600 hover:bg-blue-700" > - {creatingPR ? 'Creating...' : 'Create PR'} + {creatingPR ? ( + <> + + Creating... + + ) : ( + 'Create PR' + )}