Enhance GitHub host detection for PAT creation and setup commands#46128
Conversation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ 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
Enhances GHES host detection for diagnostics and PAT creation.
Changes:
- Detects GHES hosts from Git remotes for setup checks.
- Generates host-specific PAT creation URLs.
- Adds tests and enterprise-host guidance.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/setup_repository.go |
Configures detected hosts for setup checks. |
pkg/cli/engine_secrets.go |
Builds host-specific PAT URLs. |
pkg/cli/engine_secrets_test.go |
Tests PAT URL host selection. |
pkg/cli/doctor_command.go |
Adds enterprise-host help. |
pkg/cli/doctor_command_test.go |
Tests detection and help text. |
pkg/cli/cli_consistency_help_test.go |
Validates generated documentation. |
docs/src/content/docs/setup/cli.md |
Documents doctor host detection. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 7
- Review effort level: Medium
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design, /diagnosing-bugs, and /tdd — requesting changes on test reliability and a design duplication issue.
📋 Key Themes & Highlights
Key Themes
- Duplicated host detection —
buildPATCreationURLre-implements the same origin-remote fallback thatconfigureDefaultGHHostFromOriginRemoteIfUnsetalready provides, creating a divergence risk. - Fragile test assertions — The
checkAuthstubs derive the host from a liveghsubprocess rather than from the in-process state, making assertions environment-sensitive and potentially silently vacuous in CI. - Test-helper inconsistency — The new
engine_secrets_test.gosub-test uses rawexec.Command + os.Chdir + deferwhile the newdoctor_command_test.gotests correctly usetestutil.TempDir+t.Chdir+initTestGitRepo.
Positive Highlights
- ✅ Good layering:
buildPATCreationURLis correctly extracted as a shared helper. - ✅
configureDefaultGHHostFromOriginRemoteIfUnsetis well-named and fits the existing pattern. - ✅ Solid documentation update — the doc prose and help text are in sync.
- ✅ The new sub-table test structure for
TestBuildCopilotPATCreationURLis much more readable.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 36.2 AIC · ⌖ 7.27 AIC · ⊞ 6.7K
Comment /matt to run again
🧪 Test Quality Sentinel Report
📊 Metrics (7 tests)
📋 Test Classification Detailsdoctor_command_test.go (3 new tests):
engine_secrets_test.go (3 new table-driven subtests):
cli_consistency_help_test.go (1 assertion addition):
📈 Test Inflation Analysis
Justification: Integration-style tests with substantial infrastructure setup (git operations, filesystem manipulation, environment variables) naturally have higher line counts than production code changes. Verdict
Key strengths:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 75/100. All 7 new/modified tests verify behavioral contracts for GitHub host detection. 0% implementation tests (threshold: 30%). No coding guideline violations. Well-designed integration tests with proper infrastructure setup and cleanup.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (180 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
|
There was a problem hiding this comment.
Review: Enhance GitHub host detection for PAT creation and setup commands
Good direction — auto-detecting the GHES host from the git remote and plumbing it through to PAT URLs is the right behaviour. Two blocking correctness issues found.
Blocking issues
-
Incomplete guard in
configureDefaultGHHostFromOriginRemoteIfUnset(inline comment onsetup_repository.go:21): the early-return only checksGH_HOST, butparser.GetGitHubHost()honours four env vars (GITHUB_SERVER_URL,GITHUB_ENTERPRISE_HOST,GITHUB_HOST,GH_HOST). In a standard GitHub Actions run against GHES,GITHUB_SERVER_URLis set butGH_HOSTis empty, so the guard does not fire — origin-detection runs andSetDefaultGHHostpotentially overwrites an already-correct host. -
Duplicate host-detection in
buildPATCreationURL(inline comment onengine_secrets.go:413): this function independently re-implements the same guard and fallback logic with the sameGH_HOST-only blind spot. The two paths will drift; fixing #1 will not automatically fix this one.
Non-blocking
buildGenericPATCreationURLis a one-liner wrapper with no dedicated tests; consider inlining it to reduce indirection.- Tests in
engine_secrets_test.gomanually callexec.Command("git", ...)instead of using theinitTestGitRepo/addOriginRemoteToTestRepohelpers already used elsewhere in the package — worth consolidating for consistency.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 48.4 AIC · ⌖ 4.72 AIC · ⊞ 5K
There was a problem hiding this comment.
The GHES host-detection feature is a useful improvement, but has several blocking issues around test correctness and global state management that need to be addressed before merging.
Blocking themes
1. os.Chdir in test — process-wide race
The new TestBuildCopilotPATCreationURL subtest uses os.Chdir/defer os.Chdir instead of t.Chdir(). os.Chdir affects the entire process, not just the current goroutine. Other tests in the same binary that call getHostFromOriginRemote() (which shells out to git in the cwd) will silently pick up the wrong directory. The other two new tests correctly use t.Chdir — fix the subtest to match.
2. exec.Command without context — unbounded hang risk
The git init and git remote add calls in the same subtest have no timeout. Any CI environment with credential helpers or hooks will hang indefinitely.
3. configureDefaultGHHostFromOriginRemoteIfUnset mutates global state as a side effect
workflow.SetDefaultGHHost is a process-wide global. Both runSetupAuthWithRuntime and runSetupRepositoryCheckWithRuntime now unconditionally mutate it as a side effect. The explicit else if detectedHost == "github.com" { SetDefaultGHHost("") } branch also silently resets the global, which can clobber a previously-set value with no explanation. These concerns are detailed in inline comments.
4. Duplicate git subprocess in buildPATCreationURL
buildPATCreationURL independently calls getHostFromOriginRemote() instead of reading the global that configureDefaultGHHostFromOriginRemoteIfUnset already set. This doubles the subprocess cost and introduces a potential TOCTOU inconsistency if the cwd changes between calls.
🔎 Code quality review by PR Code Quality Reviewer · 81.1 AIC · ⌖ 5.12 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
| }() | ||
|
|
||
| require.NoError(t, os.Chdir(tmpDir)) |
There was a problem hiding this comment.
os.Chdir in a subtest mutates the entire process working directory and will race with other test goroutines.
💡 Details and suggested fix
os.Chdir changes the working directory for the whole process, not just the current goroutine. Go runs subtests in the same binary and can schedule goroutines concurrently; any code that implicitly uses the cwd (e.g. getHostFromOriginRemote() which shells out to git remote) will see the wrong directory while this subtest is running.
The defer os.Chdir(originalDir) is also silently logged rather than t.Fatal-ed on failure.
t.Chdir() (Go 1.24+) handles this correctly and is already used in TestRunSetupAuthAutoDetectsDefaultGHHost in the same file:
// Replace this block:
originalDir, err := os.Getwd()
require.NoError(t, err)
defer func() {
if err := os.Chdir(originalDir); err != nil {
t.Logf("Warning: failed to restore directory: %v", err)
}
}()
require.NoError(t, os.Chdir(tmpDir))
// With:
t.Chdir(tmpDir)|
@copilot Address all open review comments. |
Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
- Only use git remote fallback in buildPATCreationURL when no host env vars (GITHUB_SERVER_URL, GITHUB_ENTERPRISE_HOST, GITHUB_HOST, GH_HOST) are explicitly set, preventing a silent override of an explicit GH_HOST=github.com selection - Call configureDefaultGHHostFromOriginRemoteIfUnset() at the start of runTokensBootstrap so that secret discovery and upload both target the correct GHES host when running in a GHES checkout - Fix 'defaults to public github' test to clear all host env vars and run outside a git checkout to isolate from process environment - Fix 'uses GH_HOST for enterprise' test to clear higher-priority env vars (GITHUB_SERVER_URL etc.) so GH_HOST is actually honoured - Update doctor command guidance and docs to require both authentication (gh auth login --hostname) and GH_HOST for repository diagnostics outside a checkout - Update cli_consistency_help_test.go assertion to match corrected guidance text Co-authored-by: mnkiefer <8320933+mnkiefer@users.noreply.github.com>
All review comments have been addressed in commit
|
This comment has been minimized.
This comment has been minimized.
|
Hey @mnkiefer 👋 — thanks for this improvement to GHES host detection and PAT creation guidance! The change is well-scoped, includes an ADR, tests across
|
|
@copilot please run the
|
- Replace manual os.Getwd + defer os.Chdir + os.Chdir(tmpDir) pattern
with t.Chdir(tmpDir) in both subtests to avoid mutating the process
working directory for the duration of the whole process and racing
with other test goroutines
- Replace exec.Command("git", "init") + exec.Command("git", "remote",
"add", ...) with initTestGitRepo + addOriginRemoteToTestRepo helpers
which create the .git structure directly without spawning git
subprocesses (no timeout risk, no git hook interference)
- Remove os/exec import that is no longer needed
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the remaining open review thread in commit
CI is stale after the agent push and will need to be re-triggered by a maintainer. |
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds implicit GitHub Enterprise Server (GHES) host detection to
setup auth,setup repository check,tokens bootstrap, and PAT URL construction. WhenGH_HOSTis unset, the implementation parses theoriginremote URL at command entry to determine the target host, eliminating hardcodedgithub.comreferences that caused GHES users to receive incorrect PAT creation URLs and auth guidance.Changes
Core implementation (
pkg/cli/)setup_repository.go: AddedconfigureDefaultGHHostFromOriginRemoteIfUnset(). WhenGH_HOSTis unset, callsgetHostFromOriginRemote()and invokesworkflow.SetDefaultGHHost(detectedHost)for non-github.comhosts. Called at the entry ofrunSetupAuthWithRuntimeandrunSetupRepositoryCheckWithRuntime.tokens_bootstrap.go: CallsconfigureDefaultGHHostFromOriginRemoteIfUnset()at the start ofrunTokensBootstrapso secret discovery and upload target the correct host.engine_secrets.go:buildCopilotPATCreationURLto delegate to a new sharedbuildPATCreationURL(values url.Values)helper.buildGenericPATCreationURL()for system token flows.isAnyGitHubHostEnvVarSet()— checksGITHUB_SERVER_URL,GITHUB_ENTERPRISE_HOST,GITHUB_HOST,GH_HOST.buildPATCreationURLuses the git remote host when no host env var is set and the detected host is notgithub.com.promptForSystemTokenUnified: replaced hardcodedhttps://github.com/settings/personal-access-tokens/newwithbuildGenericPATCreationURL().doctor_command.go: ExtendedLongdescription to document GHES auto-detection and manual fallback (gh auth login --hostname <host>+GH_HOST=<host>).Tests (
pkg/cli/)engine_secrets_test.go: ExpandedTestBuildCopilotPATCreationURLinto 3 subtests: defaults togithub.comoutside a git repo with no env vars; usesGH_HOSTwhen set; falls back to origin remote host when all host env vars are unset.doctor_command_test.go: AddedTestDoctorCommandLongMentionsEnterpriseHostFallback; added integration testsTestRunSetupAuthAutoDetectsDefaultGHHostandTestRunSetupRepositoryCheckAutoDetectsDefaultGHHostusing real temporary git repos.cli_consistency_help_test.go: Added assertion that doctor docs mention the enterprise host fallback.Documentation
docs/adr/46128-auto-detect-ghes-host-from-git-remote.md(new): Draft ADR recording context, decision, alternatives considered, and consequences.docs/src/content/docs/setup/cli.md: Added paragraph todoctorsection documenting auto-detection behavior and fallback.Behaviour
GH_HOST,GITHUB_SERVER_URL, etc.)ghauth callsgithub.comor absentgithub.comLimitations (documented in ADR)
github.com.