diff --git a/docs/adr/41159-honor-gh-host-in-trial-repository-urls.md b/docs/adr/41159-honor-gh-host-in-trial-repository-urls.md index d45586c4dcc..a1c02abab94 100644 --- a/docs/adr/41159-honor-gh-host-in-trial-repository-urls.md +++ b/docs/adr/41159-honor-gh-host-in-trial-repository-urls.md @@ -1,18 +1,20 @@ # ADR-41159: Honor the active GitHub host for trial repository URLs **Date**: 2026-06-24 -**Status**: Draft +**Status**: Accepted **Deciders**: pelikhan (PR author), gh-aw maintainers --- ### Context -`gh aw trial` (including `--clone-repo`) constructs several URLs for the host and source repositories: clone URLs, force-push remote URLs, displayed repository links, and Actions settings links. These were all hard-coded against `https://github.com/`, so trials run against a GitHub Enterprise Server (GHES) host failed even when `gh` was authenticated to the enterprise host. Worse, a fully-qualified GHES repo spec (`https://example.ghe.com/owner/repo`) was normalized to `owner/repo` and then rebuilt against public GitHub, silently targeting the wrong host. The codebase already exposes `getGitHubHost()`, which resolves the active host from `GITHUB_SERVER_URL` / `GITHUB_ENTERPRISE_HOST` / `GITHUB_HOST` / `GH_HOST`. +`gh aw trial` (including `--clone-repo`) constructs several URLs for the host and source repositories: clone URLs, force-push remote URLs, displayed repository links, and Actions settings links. These were all hard-coded against `https://github.com/`, so trials run against a GitHub Enterprise Server (GHES) host failed even when `gh` was authenticated to the enterprise host. Worse, a fully-qualified GHES repo spec (`https://example.ghe.com/owner/repo`) was normalized to `owner/repo` and then rebuilt against public GitHub, silently targeting the wrong host. Additionally, `parseIssueSpec` used a regex that hard-coded `https://github.com/` for matching `--trigger-context` issue URLs, so GHES issue URLs (e.g. `https://example.ghe.com/owner/repo/issues/123`) were silently ignored. The codebase already exposes `getGitHubHost()`, which resolves the active host from `GITHUB_SERVER_URL` / `GITHUB_ENTERPRISE_HOST` / `GITHUB_HOST` / `GH_HOST`. ### Decision -We will centralize all trial-mode repository URL construction on the active GitHub host. We introduce three small helpers in `pkg/cli/trial_repository.go` — `trialRepositoryURL`, `trialRepositoryGitURL`, and `trialRepositoryActionsSettingsURL` — that build URLs from `getGitHubHost()` plus the repo slug, and we route every previously hard-coded `https://github.com/...` string in the trial flow through them. Clone-mode repo specs continue to parse down to `owner/repo`, but clone/push/display operations now rebuild URLs against the resolved host. +We centralize all trial-mode repository URL construction on the active GitHub host. Three small helpers in `pkg/cli/trial_repository.go` — `trialRepositoryURL`, `trialRepositoryGitURL`, and `trialRepositoryActionsSettingsURL` — build URLs from `getGitHubHost()` plus the repo slug, and every previously hard-coded `https://github.com/...` string in the trial flow is routed through them. Clone-mode repo specs continue to parse down to `owner/repo`, but clone/push/display operations now rebuild URLs against the resolved host. + +For `--trigger-context` issue URL parsing, we replace the hard-coded `issueURLPattern` regex with URL-based matching in `parseIssueSpec`: the URL host is compared against the normalized host from `getGitHubHost()`, so only issue URLs on the configured GitHub host (public or GHES) are accepted. ### Alternatives Considered @@ -24,10 +26,15 @@ Replace each hard-coded literal with an inline `fmt.Sprintf("%s/%s.git", getGitH Pass the resolved host (or a host-aware URL builder) down through `ensureTrialRepository`, `cloneTrialHostRepository`, and `cloneRepoContentsIntoHost` instead of reading it from ambient environment inside the helpers. This is more testable and explicit, but it widens several function signatures and is a larger change than the bug fix warrants. Rejected for scope; the helpers keep the existing `getGitHubHost()` resolution that the rest of the CLI already relies on. +#### Alternative 3: Build a dynamic `issueURLPattern` from `getGitHubHost()` + +Compile the issue URL regex dynamically from the configured host on each call. Rejected because compiling a regex on each `parseIssueSpec` invocation is wasteful; URL parsing with `net/url` is both idiomatic and efficient. + ### Consequences #### Positive - `gh aw trial --clone-repo` now works correctly against GHES hosts. +- `gh aw trial --trigger-context` now accepts issue URLs on the configured GHES host. - A single source of truth for trial URL construction reduces the chance of a future hard-coded `github.com` regression. #### Negative @@ -37,7 +44,3 @@ Pass the resolved host (or a host-aware URL builder) down through `ensureTrialRe #### Neutral - Behavior on public GitHub is unchanged: with no host env set, `getGitHubHost()` resolves to `github.com`. - The precedence order among `GITHUB_SERVER_URL`, `GITHUB_ENTERPRISE_HOST`, `GITHUB_HOST`, and `GH_HOST` is inherited unchanged from `getGitHubHost()`. - ---- - -*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/trial_helpers.go b/pkg/cli/trial_helpers.go index fb213cb9a83..3e3cfa3730b 100644 --- a/pkg/cli/trial_helpers.go +++ b/pkg/cli/trial_helpers.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "net/url" "os" "os/exec" "path/filepath" @@ -22,8 +23,8 @@ import ( "github.com/github/gh-aw/pkg/workflow" ) -// issueURLPattern matches GitHub issue URLs like https://github.com/owner/repo/issues/123 -var issueURLPattern = regexp.MustCompile(`https://github\.com/[^/]+/[^/]+/issues/(\d+)`) +// issuePathPattern matches the path portion of a GitHub issue URL: /owner/repo/issues/NUMBER +var issuePathPattern = regexp.MustCompile(`^/[^/]+/[^/]+/issues/(\d+)`) // issueRefPattern matches issue references like #123 var issueRefPattern = regexp.MustCompile(`^#(\d+)$`) @@ -236,15 +237,23 @@ func triggerWorkflowRun(repoSlug, workflowName string, triggerContext string, ve // parseIssueSpec extracts the issue number from various formats // Supports: -// - GitHub issue URLs: https://github.com/owner/repo/issues/123 +// - GitHub issue URLs: https://github.com/owner/repo/issues/123 (public GitHub) +// - GitHub Enterprise issue URLs: https://example.ghe.com/owner/repo/issues/123 (GHES, respects GH_HOST) // - Issue references: #123 // - Plain numbers: 123 func parseIssueSpec(input string) string { input = strings.TrimSpace(input) - // First try to match GitHub issue URLs - if matches := issueURLPattern.FindStringSubmatch(input); len(matches) >= 2 { - return matches[1] + // First try to match GitHub issue URLs (supports public GitHub and GHES via GH_HOST). + // Both the scheme and host are compared against the configured GitHub host so that + // HTTP-only GHES instances and unrelated hosts are handled correctly. + if u, err := url.Parse(input); err == nil && u.Host != "" { + configuredHostURL, err := url.Parse(getGitHubHost()) + if err == nil && u.Scheme == configuredHostURL.Scheme && u.Host == configuredHostURL.Host { + if matches := issuePathPattern.FindStringSubmatch(u.Path); len(matches) >= 2 { + return matches[1] + } + } } // Try to match issue references like #123 diff --git a/pkg/cli/trial_issue_mode_test.go b/pkg/cli/trial_issue_mode_test.go index 4020832dd8c..f8a2948f172 100644 --- a/pkg/cli/trial_issue_mode_test.go +++ b/pkg/cli/trial_issue_mode_test.go @@ -7,6 +7,8 @@ import ( ) func TestExtractIssueNumberFromURL(t *testing.T) { + // All of these cases run with the default host (no GH_HOST set), so + // getGitHubHost() returns https://github.com. testCases := []struct { name string url string @@ -64,6 +66,67 @@ func TestExtractIssueNumberFromURL(t *testing.T) { }, } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Ensure no GH_HOST is set so getGitHubHost() returns the public github.com default. + t.Setenv("GITHUB_SERVER_URL", "") + t.Setenv("GITHUB_ENTERPRISE_HOST", "") + t.Setenv("GITHUB_HOST", "") + t.Setenv("GH_HOST", "") + result := parseIssueSpec(tc.url) + if result != tc.expected { + t.Errorf("parseIssueSpec(%q) = %q, expected %q", tc.url, result, tc.expected) + } + }) + } +} + +func TestExtractIssueNumberFromURL_GHES(t *testing.T) { + // When GH_HOST points to a GitHub Enterprise Server, parseIssueSpec must accept + // issue URLs on that host and must reject github.com URLs (which belong to a + // different host in this configuration). + t.Setenv("GITHUB_SERVER_URL", "") + t.Setenv("GITHUB_ENTERPRISE_HOST", "") + t.Setenv("GITHUB_HOST", "") + t.Setenv("GH_HOST", "example.ghe.com") + + testCases := []struct { + name string + url string + expected string + }{ + { + name: "GHES issue URL", + url: "https://example.ghe.com/owner/repo/issues/42", + expected: "42", + }, + { + name: "GHES issue URL with query parameters", + url: "https://example.ghe.com/owner/repo/issues/99?tab=comments", + expected: "99", + }, + { + name: "GHES issue URL with fragment", + url: "https://example.ghe.com/owner/repo/issues/7#issuecomment-1", + expected: "7", + }, + { + name: "github.com issue URL rejected when GH_HOST is GHES", + url: "https://github.com/owner/repo/issues/123", + expected: "", + }, + { + name: "other host rejected", + url: "https://gitlab.com/owner/repo/issues/123", + expected: "", + }, + { + name: "GHES non-issue URL rejected", + url: "https://example.ghe.com/owner/repo/pulls/5", + expected: "", + }, + } + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { result := parseIssueSpec(tc.url)