Alex/refactor create pr (#746)

* Remvoe duplicate github url regex

* Better error prop

* Fix leaky auth

* Fix branch status not working or remote base branches

Make PR creation fire and forget

Fix url regex; fix error msg parsing

fmt, clippy

Revert "Make PR creation fire and forget"

This reverts commit 1a99ceb06b5534cc22fcb88c484b068292e90edb.

* Re-add open from backend

* Add creating indicator

* Remove duplication

* Add remote tests

* Fmt, clippy

* Fix https conversion edge case, fix PushRejected detection

* Add push rejected test

* Refactor githubservice

* add local fetch/push tests, ignore network test

* stop retry on reponotfound, add comment for url regex

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",

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

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)?;

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<octocrab::Error> for GitHubServiceError {
 }
 impl From<GitServiceError> 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<Self, sqlx::Error> {
-        let re = regex::Regex::new(r"github\.com/(?P<owner>[^/]+)/(?P<repo>[^/]+)").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<Self, GitHubServiceError> {
+        // Supports SSH, HTTPS and PR GitHub URLs. See tests for examples.
+        let re = Regex::new(r"github\.com[:/](?P<owner>[^/]+)/(?P<repo>[^/]+?)(?:\.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<PullRequestInfo, GitHubServiceError> {
-        (|| 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<PullRequestInfo, GitHubServiceError> {
-        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: {}",

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)

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();

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 <workspace>/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<String> {
+    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);
+}

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

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 ? (
+                <>
+                  <Loader2 className="mr-2 h-4 w-4 animate-spin" />
+                  Creating...
+                </>
+              ) : (
+                'Create PR'
+              )}
             </Button>
           </DialogFooter>
         </DialogContent>