Support GitHub Enterprise and SSH aliases via gh CLI (Vibe Kanban) (#1783)

* All changes have been implemented and tested. Here's a summary of what was done:

Updated the PR URL parsing logic to support GitHub Enterprise hostnames by using the `url` crate instead of searching for `"github.com"` as a string token.

1. **`crates/review/Cargo.toml`** - Added `url = "2.5"` dependency

2. **`crates/review/src/github.rs`** - Rewrote `parse_pr_url` function to use `Url::parse()` and `path_segments()` instead of string splitting. Added two new test cases:
   - `test_parse_pr_url_github_enterprise` - Tests GitHub Enterprise URLs
   - `test_parse_pr_url_any_host` - Tests that any host with the `/pull/` pattern works

3. **`crates/remote/src/routes/github_app.rs`** - Applied the same pattern to the duplicate `parse_pr_url` function

The new implementation:
- Parses the URL with `Url::parse()`
- Extracts path segments directly (e.g., `["owner", "repo", "pull", "123"]`)
- No longer requires `github.com` as a specific hostname
- Works with any hostname including `github.mycompany.com`, `github.enterprise.corp`, etc.

Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

All done! Here's a summary of what was implemented:

This PR adds support for GitHub Enterprise hostnames and SSH hostname aliases by delegating to the `gh` CLI instead of parsing URLs with hardcoded `github.com` patterns.

**1. `crates/services/src/services/github/cli.rs`**
- Added `run_in_dir()` helper method to run `gh` commands in a specific directory
- Added `get_repo_info(repo_path)` - uses `gh repo view --json owner,name` to get repo info from a directory
- Added `view_pr_by_url(pr_url)` - uses `gh pr view <url>` to get PR info directly from a URL

**2. `crates/services/src/services/github.rs`**
- Exported `GhCli` and `GhCliError` for use by other modules
- Added `update_pr_status_by_url(pr_url)` method to `GitHubService`
- Replaced regex-based `from_remote_url()` with hostname-agnostic URL parsing using the `url` crate (kept for backward compatibility)

**3. `crates/services/src/services/git.rs`**
- Updated `get_github_repo_info()` to use `GhCli::get_repo_info()` instead of parsing URLs

**4. `crates/services/src/services/pr_monitor.rs`**
- Updated `check_pr_status()` to use `update_pr_status_by_url()` instead of parsing the PR URL

**5. Tests** (`crates/services/tests/git_workflow.rs`)
- Updated existing tests for hostname-agnostic parsing
- Added new tests for GitHub Enterprise and SSH alias URLs

**6. `crates/review/src/github.rs`** (previously done)
- Already updated to use hostname-agnostic URL parsing

**7. `crates/remote/src/routes/github_app.rs`** (previously done)
- Already updated to use hostname-agnostic URL parsing

- **SSH hostname aliases** (e.g., `github-xxx` in `~/.ssh/config`) now work
- **GitHub Enterprise** instances (e.g., `github.mycompany.com`) now work
- **No regex** - removed regex dependency from `github.rs`, using simple URL parsing and `gh` CLI
- **Robust** - `gh` CLI handles all the complexity of authentication and hostname resolution

Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

Done! I've simplified the API by:

1. **Removed** the old `update_pr_status(&repo_info, pr_number)` method that took repo info and PR number separately
2. **Renamed** `update_pr_status_by_url(pr_url)` → `update_pr_status(pr_url)` for a cleaner API

Now there's just one method that takes the PR URL directly, and the `gh` CLI handles all the complexity of hostname resolution, SSH aliases, and GitHub Enterprise automatically.

All tests pass. The fix is complete - I renamed the test from `test_parse_pr_url_any_host` to `test_parse_pr_url_custom_github_host` and changed the URL from a misleading GitLab example to a realistic GitHub Enterprise URL (`github.acme.com`).

Done! I've removed `from_remote_url()` entirely:

1. **Removed** `GitHubRepoInfo::from_remote_url()` method from `crates/services/src/services/github.rs`
2. **Removed** the `use url::Url;` import from that file (since it's no longer needed there)
3. **Removed** both tests that exercised the now-deleted function from `crates/services/tests/git_workflow.rs`
4. **Updated** the import to remove `GitHubRepoInfo` and `GitHubServiceError` which are no longer used in tests

All tests pass and the workspace compiles cleanly. The `url` crate is still used elsewhere in the services crate, so I left it in Cargo.toml.

Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

Done! I've made both cleanups:

1. **Consolidated `run()` and `run_in_dir()`** - Now there's a single `run()` method with an optional `dir: Option<&Path>` parameter. All callers pass `None` for global commands and `Some(path)` when running in a specific directory.

2. **Renamed `view_pr_by_url()` to `view_pr()`** - Removed the old `view_pr(owner, repo, pr_number)` method and renamed `view_pr_by_url(pr_url)` to just `view_pr(pr_url)` for a cleaner API.

All tests pass and the workspace compiles cleanly.

Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

Done! I've reverted the test change in `crates/review/src/github.rs` back to the original `test_parse_pr_url_any_host` with the GitLab URL. The `crates/remote/src/routes/github_app.rs` file had no changes from my work to revert.

All tests pass and the workspace compiles. The changes in this PR are now scoped to just the `services` crate:
- `cli.rs` - consolidated `run()`, renamed `view_pr()`
- `github.rs` - removed `from_remote_url()` and `url` import
- `git.rs` - uses `GhCli::get_repo_info()`
- `git_workflow.rs` - removed tests for `from_remote_url()`

Done. I've reset the review and remote crate files to `origin/main`. The changes to those crates are now reverted and can be submitted in a separate PR. All tests pass and the workspace compiles.

This PR now only contains the services crate changes:
- `cli.rs` - consolidated `run()`, renamed `view_pr()`
- `github.rs` - removed `from_remote_url()` and `url` import
- `git.rs` - uses `GhCli::get_repo_info()`
- `git_workflow.rs` - removed tests for `from_remote_url()`

All done. All three review comments addressed:

1. Removed comment "Optionally runs in a specific directory..." from `cli.rs`
2. Removed doc comment from `update_pr_status` in `github.rs`
3. Replaced helper structs with `serde_json::Value` parsing (matches existing pattern in the file)

Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

Done. Reverted to the helper structs.

All done. Summary of changes:

1. **Removed 3 comments** as requested:
   - `git.rs:1599` - removed "Uses `gh repo view` which handles SSH aliases, GitHub Enterprise, etc."
   - `cli.rs:202` - removed "This handles GitHub Enterprise and custom hostnames automatically via the `gh` CLI."
   - `pr_monitor.rs:101` - removed "Use the URL directly - gh CLI handles all hostname/SSH alias complexity"

2. **Clone question in `github.rs:210`** - Added a comment explaining why the clone is needed (spawn_blocking requires owned values, each retry needs fresh copies)

3. **Fixed `--head {owner}:{branch}` bug** - Changed to `--head branch` only, since the `{owner}:{branch}` syntax is not supported per gh docs

4. **Added 2 unit tests** for `get_repo_info()` JSON parsing:
   - `test_parse_repo_info_valid` - tests successful parsing
   - `test_parse_repo_info_invalid` - tests error handling

* Done. Addressed the review comment by:

1. Removed `GitService.get_github_repo_info()` facade from `git.rs`
2. Updated 3 callers in `pr.rs` to use `GhCli::new().get_repo_info()` directly
3. Added `GhCliError` to `ApiError` in `error.rs` for proper error handling
4. Removed unused import in `git.rs`

* Done. Now the architecture is cleaner:

- `GhCli` is an internal implementation detail of `GitHubService`
- Callers use `GitHubService.get_repo_info(&path)` instead of touching `GhCli` directly
- Later, a `GitLabService` could have the same `get_repo_info` interface
- Removed `GhCliError` from `ApiError` since errors are now converted to `GitHubServiceError`

* Done.

* Done. `get_repo_info` is now async with `spawn_blocking`, matching the pattern used by other methods like `check_token`.

* Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338

* Done. `GhCli` and `GhCliError` are now private to the `github` module.

* Cleanup script changes for workspace e22082a4-ca31-4754-9c1c-2dc5b6991338
This commit is contained in:
Alex Netsch
2026-01-06 10:43:13 +00:00
committed by GitHub
parent cbfb637b73
commit 147c4fd53d
6 changed files with 106 additions and 152 deletions

View File

@@ -286,11 +286,8 @@ pub async fn create_github_pr(
base_branch: norm_target_branch_name.clone(), base_branch: norm_target_branch_name.clone(),
draft: request.draft, draft: request.draft,
}; };
// Use GitService to get the remote URL, then create GitHubRepoInfo
let repo_info = deployment.git().get_github_repo_info(&repo_path)?;
// Use GitHubService to create the PR
let github_service = GitHubService::new()?; let github_service = GitHubService::new()?;
let repo_info = github_service.get_repo_info(&repo_path).await?;
match github_service.create_pr(&repo_info, &pr_request).await { match github_service.create_pr(&repo_info, &pr_request).await {
Ok(pr_info) => { Ok(pr_info) => {
// Update the workspace with PR information // Update the workspace with PR information
@@ -391,7 +388,7 @@ pub async fn attach_existing_pr(
} }
let github_service = GitHubService::new()?; let github_service = GitHubService::new()?;
let repo_info = deployment.git().get_github_repo_info(&repo.path)?; let repo_info = github_service.get_repo_info(&repo.path).await?;
// List all PRs for branch (open, closed, and merged) // List all PRs for branch (open, closed, and merged)
let prs = github_service let prs = github_service
@@ -490,7 +487,7 @@ pub async fn get_pr_comments(
}; };
let github_service = GitHubService::new()?; let github_service = GitHubService::new()?;
let repo_info = deployment.git().get_github_repo_info(&repo.path)?; let repo_info = github_service.get_repo_info(&repo.path).await?;
// Fetch comments from GitHub // Fetch comments from GitHub
match github_service match github_service

View File

@@ -16,7 +16,6 @@ use cli::{ChangeType, StatusDiffEntry, StatusDiffOptions};
pub use cli::{GitCli, GitCliError}; pub use cli::{GitCli, GitCliError};
use super::file_ranker::FileStat; use super::file_ranker::FileStat;
use crate::services::github::GitHubRepoInfo;
#[derive(Debug, Error)] #[derive(Debug, Error)]
pub enum GitServiceError { pub enum GitServiceError {
@@ -1595,25 +1594,6 @@ impl GitService {
} }
} }
/// Extract GitHub owner and repo name from git repo path
pub fn get_github_repo_info(
&self,
repo_path: &Path,
) -> Result<GitHubRepoInfo, GitServiceError> {
let repo = self.open_repo(repo_path)?;
let remote_name = self.default_remote_name(&repo);
let remote = repo.find_remote(&remote_name).map_err(|_| {
GitServiceError::InvalidRepository(format!("No '{remote_name}' remote found"))
})?;
let url = remote
.url()
.ok_or_else(|| GitServiceError::InvalidRepository("Remote has no URL".to_string()))?;
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( pub fn get_remote_name_from_branch_name(
&self, &self,
repo_path: &Path, repo_path: &Path,

View File

@@ -1,9 +1,8 @@
use std::time::Duration; use std::{path::Path, time::Duration};
use backon::{ExponentialBuilder, Retryable}; use backon::{ExponentialBuilder, Retryable};
use chrono::{DateTime, Utc}; use chrono::{DateTime, Utc};
use db::models::merge::PullRequestInfo; use db::models::merge::PullRequestInfo;
use regex::Regex;
use serde::Serialize; use serde::Serialize;
use thiserror::Error; use thiserror::Error;
use tokio::task; use tokio::task;
@@ -107,41 +106,6 @@ pub struct GitHubRepoInfo {
pub owner: String, pub owner: String,
pub repo_name: String, pub repo_name: String,
} }
impl GitHubRepoInfo {
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 caps = re.captures(remote_url).ok_or_else(|| {
GitHubServiceError::Repository(format!("Invalid GitHub URL format: {remote_url}"))
})?;
let owner = caps
.name("owner")
.ok_or_else(|| {
GitHubServiceError::Repository(format!(
"Failed to extract owner from GitHub URL: {remote_url}"
))
})?
.as_str()
.to_string();
let repo_name = caps
.name("repo")
.ok_or_else(|| {
GitHubServiceError::Repository(format!(
"Failed to extract repo name from GitHub URL: {remote_url}"
))
})?
.as_str()
.to_string();
Ok(Self { owner, repo_name })
}
}
#[derive(Debug, Clone)] #[derive(Debug, Clone)]
pub struct CreatePrRequest { pub struct CreatePrRequest {
@@ -165,6 +129,20 @@ impl GitHubService {
}) })
} }
pub async fn get_repo_info(
&self,
repo_path: &Path,
) -> Result<GitHubRepoInfo, GitHubServiceError> {
let cli = self.gh_cli.clone();
let path = repo_path.to_path_buf();
task::spawn_blocking(move || cli.get_repo_info(&path))
.await
.map_err(|err| {
GitHubServiceError::Repository(format!("Failed to get repo info: {err}"))
})?
.map_err(Into::into)
}
pub async fn check_token(&self) -> Result<(), GitHubServiceError> { pub async fn check_token(&self) -> Result<(), GitHubServiceError> {
let cli = self.gh_cli.clone(); let cli = self.gh_cli.clone();
task::spawn_blocking(move || cli.check_auth()) task::spawn_blocking(move || cli.check_auth())
@@ -236,27 +214,20 @@ impl GitHubService {
Ok(cli_result) Ok(cli_result)
} }
/// Update and get the status of a pull request
pub async fn update_pr_status( pub async fn update_pr_status(
&self, &self,
repo_info: &GitHubRepoInfo, pr_url: &str,
pr_number: i64,
) -> Result<PullRequestInfo, GitHubServiceError> { ) -> Result<PullRequestInfo, GitHubServiceError> {
(|| async { (|| async {
let owner = repo_info.owner.clone();
let repo = repo_info.repo_name.clone();
let cli = self.gh_cli.clone(); let cli = self.gh_cli.clone();
let pr = task::spawn_blocking({ let url = pr_url.to_string();
let owner = owner.clone(); let pr = task::spawn_blocking(move || cli.view_pr(&url))
let repo = repo.clone(); .await
move || cli.view_pr(&owner, &repo, pr_number) .map_err(|err| {
}) GitHubServiceError::PullRequest(format!(
.await "Failed to execute GitHub CLI for viewing PR at {pr_url}: {err}"
.map_err(|err| { ))
GitHubServiceError::PullRequest(format!( })?;
"Failed to execute GitHub CLI for viewing PR #{pr_number}: {err}"
))
})?;
let pr = pr.map_err(GitHubServiceError::from)?; let pr = pr.map_err(GitHubServiceError::from)?;
Ok(pr) Ok(pr)
}) })

View File

@@ -6,6 +6,7 @@
use std::{ use std::{
ffi::{OsStr, OsString}, ffi::{OsStr, OsString},
path::Path,
process::Command, process::Command,
}; };
@@ -86,8 +87,7 @@ impl GhCli {
Ok(()) Ok(())
} }
/// Generic helper to execute `gh <args>` and return stdout on success. fn run<I, S>(&self, args: I, dir: Option<&Path>) -> Result<String, GhCliError>
fn run<I, S>(&self, args: I) -> Result<String, GhCliError>
where where
I: IntoIterator<Item = S>, I: IntoIterator<Item = S>,
S: AsRef<OsStr>, S: AsRef<OsStr>,
@@ -95,6 +95,9 @@ impl GhCli {
self.ensure_available()?; self.ensure_available()?;
let gh = resolve_executable_path_blocking("gh").ok_or(GhCliError::NotAvailable)?; let gh = resolve_executable_path_blocking("gh").ok_or(GhCliError::NotAvailable)?;
let mut cmd = Command::new(&gh); let mut cmd = Command::new(&gh);
if let Some(d) = dir {
cmd.current_dir(d);
}
for arg in args { for arg in args {
cmd.arg(arg); cmd.arg(arg);
} }
@@ -127,6 +130,29 @@ impl GhCli {
Err(GhCliError::CommandFailed(stderr)) Err(GhCliError::CommandFailed(stderr))
} }
pub fn get_repo_info(&self, repo_path: &Path) -> Result<GitHubRepoInfo, GhCliError> {
let raw = self.run(["repo", "view", "--json", "owner,name"], Some(repo_path))?;
#[derive(Deserialize)]
struct Response {
owner: Owner,
name: String,
}
#[derive(Deserialize)]
struct Owner {
login: String,
}
let resp: Response = serde_json::from_str(&raw).map_err(|e| {
GhCliError::UnexpectedOutput(format!("Failed to parse gh repo view response: {e}"))
})?;
Ok(GitHubRepoInfo {
owner: resp.owner.login,
repo_name: resp.name,
})
}
/// Run `gh pr create` and parse the response. /// Run `gh pr create` and parse the response.
/// ///
/// TODO: support writing the body to a temp file (`--body-file`) for large/multi-line /// TODO: support writing the body to a temp file (`--body-file`) for large/multi-line
@@ -159,35 +185,31 @@ impl GhCli {
args.push(OsString::from("--draft")); args.push(OsString::from("--draft"));
} }
let raw = self.run(args)?; let raw = self.run(args, None)?;
Self::parse_pr_create_text(&raw) Self::parse_pr_create_text(&raw)
} }
/// Ensure the GitHub CLI has valid auth. /// Ensure the GitHub CLI has valid auth.
pub fn check_auth(&self) -> Result<(), GhCliError> { pub fn check_auth(&self) -> Result<(), GhCliError> {
match self.run(["auth", "status"]) { match self.run(["auth", "status"], None) {
Ok(_) => Ok(()), Ok(_) => Ok(()),
Err(GhCliError::CommandFailed(msg)) => Err(GhCliError::AuthFailed(msg)), Err(GhCliError::CommandFailed(msg)) => Err(GhCliError::AuthFailed(msg)),
Err(err) => Err(err), Err(err) => Err(err),
} }
} }
/// Retrieve details for a single pull request. /// Retrieve details for a pull request by URL.
pub fn view_pr( pub fn view_pr(&self, pr_url: &str) -> Result<PullRequestInfo, GhCliError> {
&self, let raw = self.run(
owner: &str, [
repo: &str, "pr",
pr_number: i64, "view",
) -> Result<PullRequestInfo, GhCliError> { pr_url,
let raw = self.run([ "--json",
"pr", "number,url,state,mergedAt,mergeCommit",
"view", ],
&pr_number.to_string(), None,
"--repo", )?;
&format!("{owner}/{repo}"),
"--json",
"number,url,state,mergedAt,mergeCommit",
])?;
Self::parse_pr_view(&raw) Self::parse_pr_view(&raw)
} }
@@ -198,18 +220,21 @@ impl GhCli {
repo: &str, repo: &str,
branch: &str, branch: &str,
) -> Result<Vec<PullRequestInfo>, GhCliError> { ) -> Result<Vec<PullRequestInfo>, GhCliError> {
let raw = self.run([ let raw = self.run(
"pr", [
"list", "pr",
"--repo", "list",
&format!("{owner}/{repo}"), "--repo",
"--state", &format!("{owner}/{repo}"),
"all", "--state",
"--head", "all",
&format!("{owner}:{branch}"), "--head",
"--json", branch,
"number,url,state,mergedAt,mergeCommit", "--json",
])?; "number,url,state,mergedAt,mergeCommit",
],
None,
)?;
Self::parse_pr_list(&raw) Self::parse_pr_list(&raw)
} }
@@ -220,15 +245,18 @@ impl GhCli {
repo: &str, repo: &str,
pr_number: i64, pr_number: i64,
) -> Result<Vec<PrComment>, GhCliError> { ) -> Result<Vec<PrComment>, GhCliError> {
let raw = self.run([ let raw = self.run(
"pr", [
"view", "pr",
&pr_number.to_string(), "view",
"--repo", &pr_number.to_string(),
&format!("{owner}/{repo}"), "--repo",
"--json", &format!("{owner}/{repo}"),
"comments", "--json",
])?; "comments",
],
None,
)?;
Self::parse_pr_comments(&raw) Self::parse_pr_comments(&raw)
} }
@@ -239,10 +267,13 @@ impl GhCli {
repo: &str, repo: &str,
pr_number: i64, pr_number: i64,
) -> Result<Vec<PrReviewComment>, GhCliError> { ) -> Result<Vec<PrReviewComment>, GhCliError> {
let raw = self.run([ let raw = self.run(
"api", [
&format!("repos/{owner}/{repo}/pulls/{pr_number}/comments"), "api",
])?; &format!("repos/{owner}/{repo}/pulls/{pr_number}/comments"),
],
None,
)?;
Self::parse_pr_review_comments(&raw) Self::parse_pr_review_comments(&raw)
} }
} }

View File

@@ -16,7 +16,7 @@ use tracing::{debug, error, info};
use crate::services::{ use crate::services::{
analytics::AnalyticsContext, analytics::AnalyticsContext,
github::{GitHubRepoInfo, GitHubService, GitHubServiceError}, github::{GitHubService, GitHubServiceError},
share::SharePublisher, share::SharePublisher,
}; };
@@ -97,10 +97,9 @@ impl PrMonitorService {
async fn check_pr_status(&self, pr_merge: &PrMerge) -> Result<(), PrMonitorError> { async fn check_pr_status(&self, pr_merge: &PrMerge) -> Result<(), PrMonitorError> {
// GitHubService now uses gh CLI, no token needed // GitHubService now uses gh CLI, no token needed
let github_service = GitHubService::new()?; let github_service = GitHubService::new()?;
let repo_info = GitHubRepoInfo::from_remote_url(&pr_merge.pr_info.url)?;
let pr_status = github_service let pr_status = github_service
.update_pr_status(&repo_info, pr_merge.pr_info.number) .update_pr_status(&pr_merge.pr_info.url)
.await?; .await?;
debug!( debug!(

View File

@@ -5,10 +5,7 @@ use std::{
}; };
use git2::{Repository, build::CheckoutBuilder}; use git2::{Repository, build::CheckoutBuilder};
use services::services::{ use services::services::git::{DiffTarget, GitCli, GitService};
git::{DiffTarget, GitCli, GitService},
github::{GitHubRepoInfo, GitHubServiceError},
};
use tempfile::TempDir; use tempfile::TempDir;
use utils::diff::DiffChangeKind; use utils::diff::DiffChangeKind;
@@ -466,27 +463,6 @@ fn worktree_diff_permission_only_change() {
assert_eq!(d.old_content, d.new_content); assert_eq!(d.old_content, d.new_content);
} }
#[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] #[test]
fn squash_merge_libgit2_sets_author_without_user() { fn squash_merge_libgit2_sets_author_without_user() {
// Verify merge_changes (libgit2 path) uses fallback author when no config exists // Verify merge_changes (libgit2 path) uses fallback author when no config exists