Auto-detect GHES host in compile for correct repo validation#40030
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ 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. |
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 🔒 This PR cannot merge until an ADR is linked in the PR body.
|
There was a problem hiding this comment.
Pull request overview
This pull request aligns gh aw compile with other commands by auto-detecting the GitHub host from the current repo’s git remote when GH_HOST is unset, ensuring repo validation and other gh-backed checks target the correct host (e.g., GHES) instead of defaulting to github.com.
Changes:
- Add startup GH host auto-detection to
CompileWorkflowsand setworkflow’s default GH host when a non-github.comremote is detected andGH_HOSTis not set. - Add unit tests covering GHES remote detection, explicit
GH_HOSTnon-overwrite behavior, andgithub.comno-op behavior. - Regenerate/update several workflow
.lock.ymlfiles to includeawkin the allowed tool lists (consistent with shared Go source analysis tooling), and clarify the install script sync note.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/compile_orchestrator.go | Adds git-remote-based host detection and sets the default GH host before validation. |
| pkg/cli/compile_orchestrator_host_test.go | Adds regression coverage for compile host auto-detection and non-overwrite behavior. |
| actions/setup-cli/install.sh | Updates the header note to clarify the canonical script source. |
| .github/workflows/spec-librarian.lock.yml | Updates the compiled workflow lock to allow awk in the tool allowlist. |
| .github/workflows/go-fan.lock.yml | Updates the compiled workflow lock to allow awk in the tool allowlist. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Updates the compiled workflow lock to allow awk in the tool allowlist. |
| .github/workflows/daily-file-diet.lock.yml | Updates the compiled workflow lock to allow awk in the tool allowlist. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 0
![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 /diagnose and /tdd — 4 observations, none blocking. The fix itself is correct.
📋 Key Themes & Highlights
Key Themes
- Condition inconsistency (
compile_orchestrator.go):&& detectedHost != ""is dead code that diverges from the identical pattern inadd_interactive_orchestrator.go— safe to remove. - Test coverage gap: All three subtests use HTTPS remote URLs; SSH remotes (
git@host:owner/repo.git) are a common GHES format and would complete the integration picture. - Cleanup pattern:
require.NoErrorinsidet.Cleanupshould beassert.NoErrorto avoid early goroutine exit skipping other cleanup callbacks. - Mid-test env manipulation: The
os.Unsetenv("GH_HOST")before the assertion in subtest 2 is correct but unexplained — a one-line comment would prevent future confusion.
Positive Highlights
- ✅ Minimal, surgical fix — 7 lines that port a proven pattern already used in
add_interactive_orchestrator.goandaudit.go. - ✅ Three focused subtests cover all behavioural branches: GHES+unset, explicit env override, and public github.com no-op.
- ✅
t.Cleanupused correctly for both global state (SetDefaultGHHost) and working directory restoration. - ✅
NoEmit: truekeeps the test self-contained without requiring real GitHub auth.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| } | ||
|
|
||
| if os.Getenv("GH_HOST") == "" { | ||
| if detectedHost := getHostFromOriginRemote(); detectedHost != "github.com" && detectedHost != "" { |
There was a problem hiding this comment.
[/diagnose] The && detectedHost != "" guard is dead code — getHostFromOriginRemote() always returns "github.com" on error (never an empty string). This creates a silent divergence from the sibling pattern in add_interactive_orchestrator.go which omits the check.
💡 Suggested fix
Remove the redundant guard to stay in sync with the existing pattern:
if detectedHost := getHostFromOriginRemote(); detectedHost != "github.com" {
compileOrchestratorLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost)
workflow.SetDefaultGHHost(detectedHost)
}Having the extra != "" check implies an edge case that does not exist and could mislead future readers.
There was a problem hiding this comment.
Addressed in the latest commit. The condition now uses else if detectedHost == "github.com" to reset defaultGHHost for watch-mode safety, while retaining && detectedHost != "" in the if branch as a defensive guard (in case a future code path in extractHostFromRemoteURL returns empty). This keeps the intent explicit without the silent inheritance concern.
| require.NoError(t, err) | ||
| require.NoError(t, os.Chdir(tempDir)) | ||
| t.Cleanup(func() { | ||
| require.NoError(t, os.Chdir(originalDir)) |
There was a problem hiding this comment.
[/tdd] Using require.NoError inside t.Cleanup calls t.FailNow() → runtime.Goexit() on the cleanup goroutine. If os.Chdir fails here, subsequent cleanup functions registered before this one (LIFO order) are skipped — including any cleanup that depends on the working directory being restored. assert.NoError is the safer idiomatic choice in cleanup callbacks.
💡 Suggested fix
t.Cleanup(func() {
assert.NoError(t, os.Chdir(originalDir))
})This ensures all cleanup functions run even if the Chdir fails, and the test is still marked as failed via assert.
There was a problem hiding this comment.
Fixed — replaced the manual os.Getwd() + t.Cleanup(func() { require.NoError(...) }) + os.Chdir sequence with a single t.Chdir(tempDir) call (Go 1.24+). This registers cleanup before the mutation and uses t.Fatal on failure rather than require.NoError inside a cleanup callback.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 1 file (
|
There was a problem hiding this comment.
REQUEST_CHANGES — the 7-line production change is correct and consistent with the existing pattern; the issues are in the test file and a latent global-state concern in the production code.
Blocking themes
Test isolation: os.Chdir and os.Unsetenv leave global state dirty
os.Chdir(tempDir)inrunCompileWorkflowsHostDetectionCheckis process-global. The cleanup is registered after the Chdir call; if the Chdir succeeds but a laterrequire.NoErroraborts the test, the cleanup still runs — but the bigger problem is that no parallel test can safely share this process while the directory is changed. Uset.Chdir(Go 1.24+) or register the restore before the mutation.- Sub-tests 1 and 3 call
os.Unsetenv("GH_HOST")without saving/restoring the previous value. IfGH_HOSTwas set before the test (e.g., developer machine, CI environment), it will be absent for all subsequent code in the binary. - Sub-test 2 mixes
t.Setenvand bareos.Unsetenvon the same variable. The intent is correct (unset to force ExecGH to readdefaultGHHost) but the pattern is fragile and misleading; a comment explaining the intent is the minimum required.
Production: SetDefaultGHHost is never reset after a compile
Consistent with the existing add and init paths, but CompileWorkflows is also called in watch mode. Once the GHES host is pinned on the first call, subsequent calls (potentially from a different repo) will silently inherit it. Consider resetting the global when no GHES host is detected, or documenting the single-call-per-process assumption.
🔎 Code quality review by PR Code Quality Reviewer
|
|
||
| originalDir, err := os.Getwd() | ||
| require.NoError(t, err) | ||
| require.NoError(t, os.Chdir(tempDir)) |
There was a problem hiding this comment.
os.Chdir is a process-global side effect — cleanup is registered after the mutation, and concurrent tests will observe the temporary directory.
💡 Suggested fix
Register cleanup before calling os.Chdir, or use t.Chdir (Go 1.24+):
originalDir, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() { require.NoError(t, os.Chdir(originalDir)) })
require.NoError(t, os.Chdir(tempDir))As written, if os.Chdir(tempDir) fails, require.NoError calls t.FailNow/runtime.Goexit before the t.Cleanup line is ever reached — so the working directory is permanently corrupted for the test binary. Additionally, os.Chdir is shared across all goroutines in the process, meaning any concurrently scheduled work (including sub-tests that might gain t.Parallel() later) will observe the temporary directory. Use t.Chdir if available, or at minimum register the restore before the mutation.
There was a problem hiding this comment.
Fixed — replaced the manual os.Getwd() / os.Chdir / t.Cleanup sequence with t.Chdir(tempDir) (Go 1.24+), which registers cleanup before the mutation and eliminates the risk of a corrupted working directory if the initial Chdir fails.
| default: | ||
| } | ||
|
|
||
| if os.Getenv("GH_HOST") == "" { |
There was a problem hiding this comment.
SetDefaultGHHost is never reset — repeated CompileWorkflows calls in the same process will silently inherit the first detected GHES host even when switching repos.
💡 Detail
This matches the existing pattern in add_interactive_orchestrator.go, so it is consistent. But CompileWorkflows is also invoked in watch mode (repeated recompilations) and in tests that call it multiple times. Once the global is set on the first call, subsequent calls targeting a github.com remote (or a repo with no remote) will still have the GHES host pinned — the guard if os.Getenv("GH_HOST") == "" only prevents overwriting an explicit env var; it does not clear the global when the repo has changed.
Consider resetting the default when no GHES host is detected:
if os.Getenv("GH_HOST") == "" {
if detectedHost := getHostFromOriginRemote(); detectedHost != "github.com" && detectedHost != "" {
compileOrchestratorLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost)
workflow.SetDefaultGHHost(detectedHost)
} else {
workflow.SetDefaultGHHost("") // reset when not on GHES
}
}Or document clearly that CompileWorkflows must not be called repeatedly in the same process when the remote may change between calls.
There was a problem hiding this comment.
Addressed — the block now uses else if detectedHost == "github.com" { workflow.SetDefaultGHHost("") } to reset the global for watch-mode calls targeting a github.com remote, while leaving it unchanged when detectedHost is empty (unresolvable remote). A new subtest "resets default host to empty for github.com remotes" verifies this behaviour.
|
@copilot run pr-finisher skill |
…st, reset host on github.com Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…om reset Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Addressed all 8 in-scope
All unit tests and lint pass locally. CI re-trigger needed before merge. |
gh aw compiledid not inherit the same GHES host auto-detection used byaddandaudit. As a result,--validatecould send repo feature checks to the wrong GitHub host whenGH_HOSTwas unset, and downstream host-pinning logic would not engage.Compile startup host detection
CompileWorkflows.GH_HOSTis unset and the origin remote resolves to a non-github.comhost, compile now callsworkflow.SetDefaultGHHost(...)before validation and othergh-backed checks run.Behavior preserved for explicit/public host cases
GH_HOSTfrom the environment.github.comremotes unchanged.Regression coverage
GH_HOST→ default host is setGH_HOST→ compile does not override host selectiongithub.comremote → no-op behavior