Honor GH_HOST in gh aw trial --clone-repo repository URLs#41159
Honor GH_HOST in gh aw trial --clone-repo repository URLs#41159Copilot wants to merge 6 commits into
gh aw trial --clone-repo repository URLs#41159Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw trial --clone-repo repository URLs
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Updates gh aw trial --clone-repo to construct all displayed and git remote/clone URLs using the active GitHub host (via getGitHubHost()), fixing GitHub Enterprise Server (GHES) scenarios where URLs were previously hard-coded to github.com.
Changes:
- Centralized trial-mode URL construction into helper functions (
trialRepositoryURL,trialRepositoryGitURL,trialRepositoryActionsSettingsURL) and applied them across trial host creation messaging and clone/push operations. - Updated clone-mode (
--clone-repo) to build clone and host-remote URLs against the configured GitHub host. - Added focused unit coverage for host-aware trial URL helpers and GHES URL parsing in clone-mode tests.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/trial_repository.go | Introduces host-aware URL helpers and replaces hard-coded github.com URLs for trial display, clone, and push remotes. |
| pkg/cli/trial_repository_test.go | Adds unit tests verifying URL helper behavior across default host, GH_HOST, and GITHUB_SERVER_URL precedence. |
| pkg/cli/trial_command_test.go | Extends repo-spec parsing coverage to include GHES URL forms when GH_HOST is configured. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnose, and /zoom-out — no blocking issues; 4 targeted observations.
📋 Key Themes & Highlights
Key Themes
- Test struct dead fields (
trial_repository_test.go):enterpriseHostandgithubHostare declared but never populated, soGITHUB_ENTERPRISE_HOST(priority 2) andGITHUB_HOST(priority 3) remain untested. Either add cases or remove the fields. - Missing negative test (
trial_command_test.go): The new GHES happy-path case has no companion "GHES URL without GH_HOST set → error" test. - Regression coverage gap (
trial_repository.go:564): The actual bug call sites are validated only transitively through the helper tests; a lightweight test stubbingRunGitCombinedwould fully close the loop. - Intentional behavior change (
trialRepositoryURL):getGitHubHost()rather thangetGitHubHostForRepo()is used for the clone-source URL — this silently changes behavior forgithub/,githubnext/,microsoft/repos on GHES. Likely correct for trial mode, but worth a brief comment.
Positive Highlights
- ✅ Clean centralization of URL construction into three well-named helpers — easy to audit and change in one place
- ✅ Diff is tightly scoped; no unrelated changes
- ✅
GITHUB_SERVER_URLvsGH_HOSTprecedence is tested, covering the GitHub Actions runtime case - ✅ PR description is thorough with a concrete before/after example
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 80.5 AIC · ⌖ 8.2 AIC · ⊞ 6.5K
| expectedGitURL: "https://server.ghe.com/owner/repo.git", | ||
| expectedActionsURL: "https://server.ghe.com/owner/repo/settings/actions", | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] enterpriseHost and githubHost fields are declared but never populated in any test case — GITHUB_ENTERPRISE_HOST (priority 2) and GITHUB_HOST (priority 3) are both silently untested.
Since getGitHubHost() checks env vars in order (GITHUB_SERVER_URL → GITHUB_ENTERPRISE_HOST → GITHUB_HOST → GH_HOST), the precedence of the two middle vars isn't verified.
💡 Suggested additions or cleanup
Either add test cases:
{
name: "GITHUB_ENTERPRISE_HOST used when no GITHUB_SERVER_URL",
enterpriseHost: "enterprise.ghe.com",
repoSlug: "owner/repo",
expectedRepoURL: "(enterprise.ghe.com/redacted),
expectedGitURL: "(enterprise.ghe.com/redacted),
expectedActionsURL: "(enterprise.ghe.com/redacted),
},Or remove the unused enterpriseHost and githubHost struct fields to avoid misleading future contributors.
| expectedVersion: "v2.1.0", | ||
| shouldError: false, | ||
| description: "Should parse GHES URL with tag when GH_HOST is configured", | ||
| }, |
There was a problem hiding this comment.
[/tdd] The new case covers the happy path (GHES URL + GH_HOST set), but there's no companion test for the error path: a GHES URL passed when GH_HOST is not configured should fail gracefully.
Without GH_HOST, getGitHubHost() returns https://github.com, so parseRepoSpec("(example.ghe.com/redacted) will hit the else` branch and return an error about unexpected URL format. A test documenting this behaviour makes the contract explicit.
💡 Suggested error-path test case
{
name: "GHES URL without GH_HOST set returns error",
cloneRepoSpec: "(example.ghe.com/redacted),
ghHost: "",
shouldError: true,
description: "GHES URL is rejected when GH_HOST is not configured",
},| var checkoutActionPattern = regexp.MustCompile(`^(\s*)(uses: actions/checkout@[^\s]*)(.*)$`) | ||
|
|
||
| func trialRepositoryURL(repoSlug string) string { | ||
| return fmt.Sprintf("%s/%s", getGitHubHost(), repoSlug) |
There was a problem hiding this comment.
[/zoom-out] trialRepositoryURL calls getGitHubHost() unconditionally, which is correct for the host repository. It's also used in cloneRepoContentsIntoHost for the source clone URL (line ~564).
Before this PR, all URLs were hardcoded to github.com. After this PR, --clone-repo github/some-public-repo on a GHES instance will attempt to clone from https://<ghes-host>/github/some-public-repo.git — whereas getGitHubHostForRepo() would fall back to https://github.com for the github, githubnext, and microsoft orgs.
This is likely intentional for trial mode (where both repos are on the same host), but worth a brief comment so future readers understand the choice isn't accidental.
|
|
||
| // Clone the source repository | ||
| cloneURL := fmt.Sprintf("https://github.com/%s.git", cloneRepoSlug) | ||
| cloneURL := trialRepositoryGitURL(cloneRepoSlug) |
There was a problem hiding this comment.
[/diagnose] This is one of the two call sites where the bug actually manifested (the other is line ~585), but there's no test that exercises cloneRepoContentsIntoHost with a GHES host and verifies the clone URL used. The new tests in trial_repository_test.go validate the URL helpers in isolation — a missing integration-style test here means a future regression at this call site would not be caught until runtime.
💡 Approach
Even a lightweight table-driven test that stubs workflow.RunGitCombined and asserts the URL argument contains the expected GHES host would close this gap. Alternatively, document explicitly in a comment that the helper tests are considered sufficient coverage.
There was a problem hiding this comment.
Non-blocking observation
The core fix is correct: // properly centralise host resolution through getGitHubHost(), and all hard-coded https://github.com strings in the trial flow are replaced consistently. Using getGitHubHost() (not getGitHubHostForRepo) is intentional and right here — trial repos always live on the configured host, unlike canonical workflow-source repos.
One testing gap noted inline: the new TestTrialRepositoryURLHelpers file declares struct fields for GITHUB_ENTERPRISE_HOST and GITHUB_HOST but has no test cases that actually set them, leaving two of the four priority tiers untested.
🔎 Code quality review by PR Code Quality Reviewer · 90.4 AIC · ⌖ 7.09 AIC · ⊞ 5.2K
| enterpriseHost string | ||
| githubHost string |
There was a problem hiding this comment.
Dead test fields: enterpriseHost and githubHost are declared in the struct but never populated in any test case, so GITHUB_ENTERPRISE_HOST and GITHUB_HOST are never exercised — the two middle tiers of GetGitHubHost()'s priority chain are untested.
💡 Suggested additions
GetGitHubHost() resolves four env vars in priority order:
GITHUB_SERVER_URL → GITHUB_ENTERPRISE_HOST → GITHUB_HOST → GH_HOST
This test file covers the highest-priority (GITHUB_SERVER_URL) and lowest-priority (GH_HOST) vars only. A regression — accidentally swapping GITHUB_ENTERPRISE_HOST and GITHUB_HOST in the priority slice, or dropping one — would go completely undetected.
Add at least two more cases:
{
name: "uses GITHUB_ENTERPRISE_HOST",
enterpriseHost: "enterprise.ghe.com",
repoSlug: "owner/repo",
expectedRepoURL: "(enterprise.ghe.com/redacted),
expectedGitURL: "(enterprise.ghe.com/redacted),
expectedActionsURL: "(enterprise.ghe.com/redacted),
},
{
name: "GITHUB_ENTERPRISE_HOST beats GH_HOST",
enterpriseHost: "enterprise.ghe.com",
ghHost: "other.ghe.com",
repoSlug: "owner/repo",
expectedRepoURL: "(enterprise.ghe.com/redacted),
expectedGitURL: "(enterprise.ghe.com/redacted),
expectedActionsURL: "(enterprise.ghe.com/redacted),
},
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 2 ( i️ Flagged: Test Inflation (1 notice)
Context: This is a justified, benign inflation. The 21 production lines introduce 3 new helper functions ( Verdict
References: §28078386190
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both tests verify observable behavioral contracts for GH_HOST / GITHUB_SERVER_URL URL-construction. No coding-guideline violations detected.
|
@copilot please add the remaining GH_HOST and GITHUB_ENTERPRISE_HOST test coverage, then refresh checks.
|
|
@copilot please add the remaining GH_HOST and GITHUB_ENTERPRISE_HOST test coverage, then refresh checks.
|
|
pr-sous-chef @copilot review all comments and address unresolved review feedback. Please summarize the remaining blockers and refresh checks after that.
|
|
@copilot please refresh checks after addressing review feedback and summarize any remaining blockers.
|
gh aw trial --clone-repowas deriving clone and display URLs fromgithub.comunconditionally, which broke GHES trials even whenghwas authenticated to the enterprise host. Fully-qualified GHES clone URLs were also normalized back toowner/repoand then rebuilt against the wrong host.Use the active GitHub host for all trial repository URLs
getGitHubHost()--clone-repoKeep clone-mode behavior consistent on GHES
--clone-repo owner/reponow resolves against the configured GitHub host instead of hard-codinggithub.comowner/repo, but trial clone/push operations now rebuild URLs against the active host instead of public GitHubAdd focused GHES coverage
Example of the resolved behavior:
Trial clone/push URLs now resolve as:
pr-sous-chef: requested branch update from run https://github.com/github/gh-aw/actions/runs/28079516670
pr-sous-chef: update branch requested from workflow run 28095175203