fix: force GH_HOST=github.com for action SHA resolution in resolveFromGitHub#40031
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
When GH_HOST is set to a GHE instance, action SHA resolution via `gh api` was targeting the wrong host. Add ForceGHHostEnv helper that strips any existing GH_HOST from the command environment and sets GH_HOST=github.com, then call it in resolveFromGitHub (both the initial tag lookup and the annotated-tag peel loop) so action SHAs are always resolved against github.com. ResolveGhAwRef is left unchanged since it resolves refs in the github/gh-aw repo, which may live on the user's configured host. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40031 does not have the 'implementation' label and has 91 new lines of code in business logic directories, which is below the 100-line threshold. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR aims to make ActionResolver.resolveFromGitHub reliably resolve action tag SHAs from github.com even when the caller has GH_HOST set to a GitHub Enterprise host, avoiding silent fallbacks to bundled pins caused by querying the wrong API endpoint.
Changes:
- Adds
ForceGHHostEnvto unconditionally setGH_HOSTonghsubprocesses (overriding inherited env). - Uses
ForceGHHostEnv(cmd, "github.com")for thegh apicalls insideresolveFromGitHub(initial tag lookup + peel loop). - Updates several generated workflow
.lock.ymlfiles to includeawkin the allowed-tools list (not described in the PR summary).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/github_cli.go | Adds ForceGHHostEnv to override inherited GH_HOST for subprocesses. |
| pkg/workflow/github_cli_wasm.go | Adds a wasm-safe no-op stub for ForceGHHostEnv. |
| pkg/workflow/action_resolver.go | Forces GH_HOST=github.com on gh api commands used for action SHA resolution. |
| pkg/workflow/action_resolver_test.go | Adds a test for host-forcing behavior (but currently doesn’t exercise resolveFromGitHub itself). |
| actions/setup-cli/install.sh | Updates script sync comment. |
| .github/workflows/spec-librarian.lock.yml | Adds shell(awk) to allowed tools list. |
| .github/workflows/go-fan.lock.yml | Adds Bash(awk) to allowed tools list. |
| .github/workflows/daily-testify-uber-super-expert.lock.yml | Adds shell(awk) to allowed tools list. |
| .github/workflows/daily-file-diet.lock.yml | Adds shell(awk) to allowed tools list. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 7
| if err := os.Unsetenv("GH_HOST"); err != nil { | ||
| t.Fatalf("failed to unset GH_HOST: %v", err) | ||
| } | ||
| t.Cleanup(func() { os.Unsetenv("GH_HOST") }) //nolint:errcheck | ||
| } |
There was a problem hiding this comment.
Fixed: the "GH_HOST unset" branch now uses os.LookupEnv to save the original value and t.Cleanup to restore it (or re-unset if it was never set), matching the pattern used in github_cli_test.go.
| // TestResolveFromGitHubForcesGitHubComHost verifies that the commands created by | ||
| // resolveFromGitHub always have GH_HOST=github.com in their environment, regardless | ||
| // of the process-level GH_HOST setting. This ensures that action SHA lookups always | ||
| // target github.com even when the user has GH_HOST set to a GHE instance. | ||
| func TestResolveFromGitHubForcesGitHubComHost(t *testing.T) { |
There was a problem hiding this comment.
Fixed: renamed to TestForceGHHostEnvSetsGitHubCom with an updated comment accurately describing what is tested (direct ForceGHHostEnv coverage, not resolveFromGitHub integration, since the latter requires a live gh api endpoint).
| # --allow-tool github | ||
| # --allow-tool safeoutputs | ||
| # --allow-tool serena | ||
| # --allow-tool shell(awk) | ||
| # --allow-tool shell(cat pkg/**/*.go) |
There was a problem hiding this comment.
Reverted: the shell(awk) addition was unrelated to this fix and is not present in the markdown source; the lock file has been restored to its original state.
| # --allow-tool github | ||
| # --allow-tool safeoutputs | ||
| # --allow-tool serena | ||
| # --allow-tool shell(awk) | ||
| # --allow-tool shell(cat pkg/**/*.go) |
There was a problem hiding this comment.
Reverted: the shell(awk) addition was unrelated to this fix and is not present in the markdown source; the lock file has been restored to its original state.
| # --allow-tool github | ||
| # --allow-tool safeoutputs | ||
| # --allow-tool serena | ||
| # --allow-tool shell(awk) | ||
| # --allow-tool shell(cat **/*_test.go) |
There was a problem hiding this comment.
Reverted: the shell(awk) addition was unrelated to this fix and is not present in the markdown source; the lock file has been restored to its original state.
| - name: Execute Claude Code CLI | ||
| id: agentic_execution | ||
| # Allowed tools (sorted): | ||
| # - Bash(awk) | ||
| # - Bash(cat /tmp/gh-aw/cache-memory/) |
There was a problem hiding this comment.
Reverted: the Bash(awk) addition was unrelated to this fix and is not present in the markdown source; the lock file has been restored to its original state.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Go: 3 ( Scoring breakdown:
Verdict
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — approving with non-blocking suggestions on test quality.
📋 Key Themes & Highlights
Key Themes
- Test scope mismatch:
TestResolveFromGitHubForcesGitHubComHosttests theForceGHHostEnvhelper in isolation rather than exercisingresolveFromGitHubdirectly. The test name overpromises — if the two call sites inaction_resolver.gowere removed, the test would still pass. - Test isolation gap: The
"GH_HOST unset"subtest doesn't restore a pre-existingGH_HOSTvalue on cleanup; usingos.LookupEnvto save/restore would fully mirrort.Setenvsemantics. - Untested branch: The
cmd.Env != nilbranch ofForceGHHostEnv(the dedup loop) has no test coverage, sinceExecGHContextalways yields anil-Env cmd at the call sites.
Positive Highlights
- ✅ Root cause correctly addressed: both call sites in
resolveFromGitHub(initial tag lookup + annotated-tag peel loop) now forceGH_HOST=github.com - ✅
ResolveGhAwRefintentionally left unchanged — correctly preserving the user-configured host for self-referential lookups againstgithub/gh-aw - ✅
ForceGHHostEnvcorrectly strips all existingGH_HOST=entries before appending — avoids the duplicate-entry risk present in the olderSetGHHostEnv - ✅ WASM stub is the right call: maintains build-constraint parity with zero runtime impact
- ✅ Three-boundary parametric test (unset / GHE / github.com) is a solid smoke test for the helper function
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
| // resolveFromGitHub always have GH_HOST=github.com in their environment, regardless | ||
| // of the process-level GH_HOST setting. This ensures that action SHA lookups always | ||
| // target github.com even when the user has GH_HOST set to a GHE instance. | ||
| func TestResolveFromGitHubForcesGitHubComHost(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The test name and comment say it verifies resolveFromGitHub commands have GH_HOST=github.com, but the body only exercises ForceGHHostEnv directly — never calling resolveFromGitHub. If the two ForceGHHostEnv calls were removed from action_resolver.go, this test would still pass.
💡 Suggested improvement
Consider renaming to TestForceGHHostEnvStripsProcessGHHost to accurately reflect what is tested, and add a comment explaining why resolveFromGitHub isn't exercised directly (live gh api endpoint required).
// TestForceGHHostEnvStripsProcessGHHost verifies that ForceGHHostEnv overrides
// any process-level GH_HOST, including GHE hosts.
// Note: resolveFromGitHub is not called directly here because it requires a live
// gh api endpoint. The integration contract is enforced by the ForceGHHostEnv call
// sites in action_resolver.go.
func TestForceGHHostEnvStripsProcessGHHost(t *testing.T) {There was a problem hiding this comment.
Fixed: renamed to TestForceGHHostEnvSetsGitHubCom with a comment explaining that resolveFromGitHub isn't called directly because it requires a live gh api endpoint.
| } else { | ||
| // Truly unset GH_HOST (t.Setenv("GH_HOST", "") sets to empty string, | ||
| // which is not the same as unset; use os.Unsetenv with cleanup instead). | ||
| if err := os.Unsetenv("GH_HOST"); err != nil { |
There was a problem hiding this comment.
[/tdd] The cleanup for the "GH_HOST unset" case doesn't restore the original GH_HOST value if it was set in the environment before this test ran — the t.Cleanup just calls os.Unsetenv again instead of restoring what was there.
💡 Suggested fix using os.LookupEnv
} else {
original, hadVal := os.LookupEnv("GH_HOST")
if err := os.Unsetenv("GH_HOST"); err != nil {
t.Fatalf("failed to unset GH_HOST: %v", err)
}
t.Cleanup(func() {
if hadVal {
os.Setenv("GH_HOST", original) (nolint/redacted):errcheck
} else {
os.Unsetenv("GH_HOST") (nolint/redacted):errcheck
}
})
}This mirrors what t.Setenv does internally, ensuring the test doesn't permanently unset GH_HOST for other tests running in the same process (e.g. in CI where GH_HOST may be set globally).
There was a problem hiding this comment.
Fixed: the "GH_HOST unset" branch now saves the original value with os.LookupEnv and restores it (or re-unsets) in t.Cleanup, exactly as suggested.
| return | ||
| } | ||
| base := cmd.Env | ||
| if base == nil { |
There was a problem hiding this comment.
[/tdd] ForceGHHostEnv has two branches: cmd.Env == nil (hydrates from os.Environ()) and cmd.Env != nil (uses the existing slice). The new tests only exercise the nil-Env branch because ExecGHContext always returns a cmd with nil Env. The non-nil branch — including its dedup logic — has no test coverage.
💡 Suggested test
Add a case to the existing table (or a separate TestForceGHHostEnvWithPresetCmdEnv) that pre-populates cmd.Env with a stale GH_HOST entry:
cmd := exec.Command("gh", "api", "/test")
cmd.Env = []string{"GH_HOST=stale.ghe.com", "OTHER=value"}
ForceGHHostEnv(cmd, "github.com")
// assert: exactly one GH_HOST=github.com, no stale entry, OTHER preservedThis would guard the dedup loop and ensure SetGHHostEnv + ForceGHHostEnv chaining (if it ever occurs) doesn't produce duplicate entries.
There was a problem hiding this comment.
Fixed: added TestForceGHHostEnvWithPresetCmdEnv which pre-populates cmd.Env with ["GH_HOST=stale.ghe.com", "OTHER=value"], calls ForceGHHostEnv, then asserts exactly one GH_HOST=github.com entry and that OTHER=value is preserved. This covers the non-nil branch and the dedup loop.
There was a problem hiding this comment.
REQUEST_CHANGES — two correctness gaps and two test quality issues
The fix logic in ForceGHHostEnv is correct in isolation, but there are two blocking issues before merging.
Blocking issues
1. The test doesn't exercise resolveFromGitHub (see inline comment on line 298 of the test file). TestResolveFromGitHubForcesGitHubComHost directly calls the helper primitives and would pass even if both ForceGHHostEnv call sites were removed from resolveFromGitHub. The regression this PR is meant to prevent has no automated guard.
2. ResolveGhAwRef is left with the same ambient-host problem (see inline comment on line 262 of action_resolver.go). apiPath is hardcoded to /repos/github/gh-aw/commits/..., so a GHE GH_HOST will silently route it to the wrong server. The PR description calls this intentional, but the stated reason (github/gh-aw may live on the user's host) is not convincing — this is the extension's own canonical repo on github.com. At minimum a code comment justifying the asymmetry is needed; more likely ForceGHHostEnv should be applied here too.
Non-blocking but should be addressed
- Test cleanup for the "unset" case uses bare
os.Unsetenvinstead of the save-and-restore idiom, which breaks isolation whenGH_HOSTwas set before the test ran. - The
defaultGHHostpath throughsetupGHCommandis not exercised by any subtest.
🔎 Code quality review by PR Code Quality Reviewer
Comments that could not be inline-anchored
pkg/workflow/action_resolver_test.go:298
Test never exercises resolveFromGitHub, so removing the fix passes silently: this test constructs a command and calls ForceGHHostEnv directly — it does not go through resolveFromGitHub. If either of the two ForceGHHostEnv(cmd...) lines in action_resolver.go are removed in a future refactor, this test still passes.
<details>
<summary>💡 Suggested approach</summary>
Replace the direct-primitive test with a spy/intercept pattern that actually invokes resolveFromGitHub. One lightw…
pkg/workflow/action_resolver.go:262
ResolveGhAwRef silently misroutes under a GHE GH_HOST: the PR applies ForceGHHostEnv to resolveFromGitHub but leaves ResolveGhAwRef inheriting the process GH_HOST. Since apiPath is hardcoded to /repos/github/gh-aw/commits/..., a GHE host in the environment will cause this call to hit the wrong endpoint and fail with a confusing error.
<details>
<summary>💡 Detail and suggested fix</summary>
The PR description says _"ResolveGhAwRef is intentionally unchanged — it targets gi…
pkg/workflow/action_resolver_test.go:295
Cleanup for the "unset" case doesn't restore GH_HOST to its pre-test value: if the test environment has GH_HOST set (e.g., github.com in CI), os.Unsetenv at cleanup permanently drops it for the rest of the test binary run instead of restoring the original.
<details>
<summary>💡 Suggested fix</summary>
Use os.LookupEnv to save-and-restore, the same way t.Setenv works internally:
// Before clearing:
if orig, existed := os.LookupEnv("GH_HOST"); existed {
t.Cleanup(func…
</details>
<details><summary>pkg/workflow/action_resolver_test.go:286</summary>
**Missing coverage for the `defaultGHHost` path**: `setupGHCommand` (called by `ExecGHContext`) has a second way a GHE host can land in `cmd.Env` — via `SetGHHostEnv(cmd, getDefaultGHHost())` when `GH_HOST` is absent from the process env but `SetDefaultGHHost("myorg.ghe.com")` was called earlier. No subtest exercises this path.
<details>
<summary>💡 Why it matters</summary>
This is a plausible real-world config: a user has set a default GHE host programmatically but has no process-level `GH_H…
</details>|
@copilot run pr-finisher skill |
… cleanup in tests, rename test, revert unrelated lock file changes Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All review threads addressed. Summary of changes made:
All unit tests pass locally. CI needs a human re-trigger after the agent's push. |
When
GH_HOSTis set to a GHE instance,resolveFromGitHubinherits that host in itsgh apisubprocesses, causing action SHA lookups (e.g.actions/checkout@v4) to query the wrong endpoint and silently fall back to bundled hardcoded pins.Changes
pkg/workflow/github_cli.go— addsForceGHHostEnv(cmd, host): strips any existingGH_HOSTfrom the command env (including inherited process env) and sets the specified value. UnlikeSetGHHostEnv, this always applies — including for"github.com"— to override a GHE host set by the caller.pkg/workflow/github_cli_wasm.go— adds a no-op stub forForceGHHostEnvsoaction_resolver.go(no build constraint) compiles cleanly for WASM targets.pkg/workflow/action_resolver.go— callsForceGHHostEnv(cmd, "github.com")immediately after eachExecGHContextinresolveFromGitHub: once for the initial tag ref lookup and once per annotated-tag peel iteration.ResolveGhAwRefis intentionally unchanged — it targetsgithub/gh-aw, which may live on the user's configured host.pkg/workflow/action_resolver_test.go— addsTestResolveFromGitHubForcesGitHubComHostcovering:GH_HOSTtruly unset, set to a GHE host, and already set togithub.com.