Honor default GH host in PinContext hardcoded-pin fallback guard#40028
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PinContext hardcoded-pin fallback guard
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40028 does not have the 'implementation' label and has 33 new lines (≤100 threshold) in business logic directories. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Updates WorkflowData.PinContext() host detection so the hardcoded-pin fallback is disabled not only for GH_HOST-driven GHES/GHEC compilation, but also when a non-github.qkg1.top default host is set programmatically (e.g. via SetDefaultGHHost), aligning behavior with the effective host selection used by gh helpers.
Changes:
- Extend
PinContext()to considergetDefaultGHHost()when determiningSkipHardcodedFallback. - Add unit tests covering
SetDefaultGHHostbehavior whenGH_HOSTis unset. - Add test scaffolding to restore the original default host after the test.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Adds default-host-based logic for disabling bundled hardcoded-pin fallback. |
| pkg/workflow/compiler_types_test.go | Adds focused tests for SkipHardcodedFallback behavior with SetDefaultGHHost and GH_HOST unset. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
| // Also disable fallback when a non-github.qkg1.top default host has been set | ||
| // programmatically (for example from auto-detected git remotes). | ||
| if defaultHost := getDefaultGHHost(); defaultHost != "" && defaultHost != "github.com" { | ||
| pinCtx.SkipHardcodedFallback = true | ||
| } |
| if defaultHost := getDefaultGHHost(); defaultHost != "" && defaultHost != "github.com" { | ||
| pinCtx.SkipHardcodedFallback = true |
| t.Run("does not set SkipHardcodedFallback when GH_HOST is not set", func(t *testing.T) { | ||
| require.NoError(t, os.Unsetenv("GH_HOST")) | ||
| SetDefaultGHHost("") | ||
|
|
Test Quality Sentinel Report Score: 90/100 Excellent
|
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness bug + one test gap that hides it.
### Blocking issues
1. Priority inversion in PinContext() (high)
The two if blocks evaluate GH_HOST and getDefaultGHHost() independently. SetDefaultGHHost is documented as the fallback when GH_HOST is not set, but the new code can still set SkipHardcodedFallback=true even when GH_HOST=github.com is explicitly present. This is both a contract violation and a correctness bug — a user who sets GH_HOST=github.com to override an auto-detected GHE remote will have pin resolution silently fail.
The fix is to consult defaultGHHost only when GH_HOST is absent, matching setupGHCommand's identical precedence logic.
2. Missing conflict-scenario test (medium)
The GH_HOST=github.com subtest does not set defaultGHHost to a conflicting value, so the test suite passes green despite the bug above. A test covering GH_HOST=github.com + defaultGHHost=myorg.ghe.com → SkipHardcodedFallback=false is required; it will red-bar today and go green after the fix.
🔎 Code quality review by PR Code Quality Reviewer
Comments that could not be inline-anchored
pkg/workflow/compiler_types.go:649
Priority inversion: defaultGHHost is evaluated independently of GH_HOST, so an explicit GH_HOST=github.com cannot prevent SkipHardcodedFallback from being set when a non-github.qkg1.top default host is active.
<details>
<summary>💡 Details and suggested fix</summary>
SetDefaultGHHost is documented as "the default host used by gh CLI helper commands when GH_HOST is not set in the process environment", and setupGHCommand enforces this contract:
if os.Getenv("GH_HOST") …
</details>
<details><summary>pkg/workflow/compiler_types_test.go:30</summary>
**This test cannot catch the priority-inversion regression**: `defaultGHHost` is not set to a conflicting non-`github.com` value here, so the test passes even with the buggy independent-`if` logic.
<details>
<summary>💡 Required test case</summary>
The critical path that needs to be covered is: `GH_HOST=github.com` **and** `defaultGHHost=myorg.ghe.com`. In that configuration the existing code sets `SkipHardcodedFallback=true` (wrong), while the corrected single-priority logic would leave it `…
</details>There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — one test-isolation suggestion and one pre-existing parallel code path to track.
📋 Key Themes & Highlights
Pre-existing parallel code path (/diagnose)
getCachedActionPinFromResolver in action_pins.go:121 constructs a PinContext{} directly and never sets SkipHardcodedFallback:
func getCachedActionPinFromResolver(repo string, resolver SHAResolver) string {
ctx := &actionpins.PinContext{} // SkipHardcodedFallback stays false
...
}It is called from maintenance_workflow_yaml.go in ~20 places. GHE users on the maintenance-workflow generation path will still silently receive hardcoded-pin fallback — the same class of bug fixed here. The fix is out of scope for this PR, but worth a follow-up issue so the whole surface is covered consistently.
Test isolation gap (/tdd)
See inline comment on line 41. The SetDefaultGHHost("") pattern added to sub-test 3 should also appear in the sibling sub-test 2 for symmetry and order-independence.
Positive Highlights
- ✅ Fix is minimal and correct — OR-logic handles both env-var and programmatic host sources; once
true,SkipHardcodedFallbackis never reset tofalse - ✅ Two new test cases neatly mirror the existing
GH_HOSTtests for thedefaultGHHostpath - ✅ Top-level
t.Cleanupis the right pattern for restoring shared global state across sub-tests - ✅ Clear PR description identifying root cause and the specific gap being closed
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer
|
|
||
| t.Run("does not set SkipHardcodedFallback when GH_HOST is not set", func(t *testing.T) { | ||
| require.NoError(t, os.Unsetenv("GH_HOST")) | ||
| SetDefaultGHHost("") |
There was a problem hiding this comment.
[/tdd] The sibling sub-test "does not set SkipHardcodedFallback when GH_HOST is github.com" also asserts SkipHardcodedFallback = false but doesn't call SetDefaultGHHost(""). Now that sub-tests 4 and 5 mutate the shared defaultGHHost, sub-test 2 relies on execution order for isolation.
💡 Suggested fix for sub-test 2
t.Run("does not set SkipHardcodedFallback when GH_HOST is github.com", func(t *testing.T) {
t.Setenv("GH_HOST", "github.com")
SetDefaultGHHost("") // add this guard
d := &WorkflowData{}
ctx := d.PinContext()
...
})Without this, if a future sub-test is inserted before it that sets defaultGHHost to a GHE host, the assert.False assertion would spuriously fail.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ence test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Three issues from the review are now addressed (commit
Local |
|
@copilot merge main and recompile |
…rdcodedfallback-detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
SkipHardcodedFallbackonly consideredGH_HOST, so compile flows targeting GHE viaSetDefaultGHHostcould still silently fall back to bundled pins after dynamic resolution failures. This change makes host detection consistent with effective runtime host selection (setupGHCommandingithub_cli.go) and preservesgithub.qkg1.topbehavior.Pin context host evaluation
WorkflowData.PinContext()to setSkipHardcodedFallbackwhen either:GH_HOSTis a non-github.qkg1.tophost, orGH_HOSTis unset andgetDefaultGHHost()is a non-github.qkg1.tophost.GH_HOSTtakes unconditional precedence over the programmatic default host, mirroringsetupGHCommand's behavior.Wasm build compatibility
getDefaultGHHost()andSetDefaultGHHost()togithub_cli_wasm.gosocompiler_types.gocompiles underjs/wasmtargets.Coverage
GH_HOST=github.com+ non-github.qkg1.topdefault host → fallback staysfalse(precedence regression test).SetDefaultGHHostscenarios withGH_HOSTunset:github.qkg1.topdefault host ⇒ fallback disabledgithub.qkg1.topdefault host ⇒ fallback unchangedGH_HOST-based assertions intact.