Expand parser GitHub host unit coverage and harden host-repo assertions - #46926
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR Triage — Run §29791496271
Rationale: Pure test-only change (1 file). Adds coverage for GetGitHubHost env-priority/normalization. No production code changes. Auto-merge once undrafted and CI passes.
|
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (73 additions found, threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Expands parser unit coverage for GitHub host resolution and fallback behavior.
Changes:
- Adds host normalization, fallback, and partial precedence tests.
- Adds empty-enterprise-host coverage.
- Uses fail-fast assertions and removes the integration build tag.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/github_host_test.go |
Expands host-resolution tests and updates assertion behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/parser/github_host_test.go:39
- This case leaves
GH_HOSTempty, so theGITHUB_HOST > GH_HOSTpriority rule remains untested despite the PR claiming full env-priority coverage. SetGH_HOSTto a distinct value so reversing those two checks fails the test.
name: "trailing slash stripped",
serverURL: "",
enterpriseHost: "",
githubHost: "acme.ghe.com/",
ghHost: "",
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Medium
| name: "host without scheme gets https prefix", | ||
| serverURL: "", | ||
| enterpriseHost: "acme.ghe.com", | ||
| githubHost: "", | ||
| ghHost: "", |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 22.1 AIC · ⌖ 4.28 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report🚨 Test Quality Score: 88/100 — Excellent (but FAILED due to guideline violation)
📊 Metrics (10 scenarios in 2 test functions)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — two observations on test isolation and coverage completeness.
📋 Key Themes & Highlights
Issues
- Env isolation gap (line 120):
TestGetGitHubHostForRepo_PublicOrgFallbackclearsGITHUB_SERVER_URL,GITHUB_ENTERPRISE_HOST, andGH_HOSTbut never clearsGITHUB_HOST. A pre-existingGITHUB_HOSTin a CI environment could cause the new"empty gheHost falls back to public for non-fallback owner"case to pass for the wrong reason. - Missing normalisation edge case (line 23):
TestGetGitHubHosthas no case whereGITHUB_SERVER_URLalready includeshttps://, leaving a potential double-prefix regression undetected.
Positive Highlights
- ✅ Clean table-driven structure with good case names
- ✅
t.Setenvused throughout — safe parallel test execution - ✅
require.Equalupgrade is the right call for fail-fast loop semantics - ✅ Build-tag removal is correct: these are pure unit tests with no I/O
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 34.8 AIC · ⌖ 4.62 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/parser/github_host_test.go:120
[/tdd] GITHUB_HOST env var is never cleared in the subtest setup, so a pre-existing GITHUB_HOST in CI could silently override tt.gheHost="" and produce a false pass on the empty-gheHost fallback case.
<details>
<summary>💡 Suggested fix</summary>
Add a t.Setenv for GITHUB_HOST alongside the other resets:
t.Setenv("GITHUB_SERVER_URL", "")
t.Setenv("GITHUB_ENTERPRISE_HOST", tt.gheHost)
t.Setenv("GITHUB_HOST", "") // ← add this
t.Setenv("GH_HOST", "")Without it the n…
pkg/parser/github_host_test.go:23
[/tdd] No test case covers GITHUB_SERVER_URL already containing https:// — without it, a regression in NormalizeGitHubHostURL that double-prefixes would go undetected.
<details>
<summary>💡 Suggested additional test case</summary>
{
name: "GITHUB_SERVER_URL with existing https scheme is not double-prefixed",
serverURL: "(acme.ghe.com/redacted)",
expectedHost: "(acme.ghe.com/redacted)",
},This guards against a common URL normalisation regression.
<…
There was a problem hiding this comment.
Review: Expand parser GitHub host unit coverage
Non-blocking — the added tests are correct and well-structured; two gaps in coverage are worth addressing.
Findings
Missing priority-ordering tests (medium): Each of the new subtests sets only one env var at a time. No case exercises two env vars simultaneously to verify the documented precedence order (e.g. GITHUB_ENTERPRISE_HOST beats GITHUB_HOST). A reordering of the implementation's env-var slice would pass all tests silently.
No scheme-already-present test (low): In real CI, GITHUB_SERVER_URL is always a full URL ((redacted)). The NormalizeGitHubHostURL guard that prevents double-prefixing is untested in this file.
Both are straightforward additions to the existing table.
🔎 Code quality review by PR Code Quality Reviewer · 41.1 AIC · ⌖ 4.65 AIC · ⊞ 5.6K
Comment /review to run again
| githubHost: "github-host.ghe.com", | ||
| ghHost: "gh-host.ghe.com", | ||
| expectedHost: "https://acme.ghe.com/redacted", | ||
| }, |
There was a problem hiding this comment.
Missing priority-ordering assertions: only the top-priority env var is tested in competition; no case has two or more vars set simultaneously to verify that GITHUB_ENTERPRISE_HOST beats GITHUB_HOST and GITHUB_HOST beats GH_HOST. A silent reordering of the env-var slice in the implementation would pass all current tests.
💡 Suggested additions
{
name: "GITHUB_ENTERPRISE_HOST beats GITHUB_HOST",
serverURL: "",
enterpriseHost: "enterprise.ghe.com",
githubHost: "github-host.ghe.com",
ghHost: "gh-host.ghe.com",
expectedHost: "(enterprise.ghe.com/redacted)",
},
{
name: "GITHUB_HOST beats GH_HOST",
serverURL: "",
enterpriseHost: "",
githubHost: "github-host.ghe.com",
ghHost: "gh-host.ghe.com",
expectedHost: "(githubhost.ghe.com/redacted)",
},The implementation iterates ["GITHUB_SERVER_URL", "GITHUB_ENTERPRISE_HOST", "GITHUB_HOST", "GH_HOST"] in order — any reordering regression would be invisible with only single-variable cases.
| enterpriseHost: "acme.ghe.com", | ||
| githubHost: "", | ||
| ghHost: "", | ||
| expectedHost: "https://acme.ghe.com", |
There was a problem hiding this comment.
No test for scheme-already-present input: the GITHUB_SERVER_URL wins over others case uses a bare hostname (acme.ghe.com/redacted). In real CI, GITHUB_SERVER_URL is always set as a full URL like (acme.ghe.com/redacted). There is no test verifying that NormalizeGitHubHostURL does not double-prefix such a value to (https/redacted)://acme.ghe.com.
💡 Suggested addition
{
name: "GITHUB_SERVER_URL already has https scheme",
serverURL: "(acme.ghe.com/redacted)",
expectedHost: "(acme.ghe.com/redacted)",
},NormalizeGitHubHostURL has a HasPrefix("https://") guard, but this path has zero test coverage in this file.
|
@copilot please run the Unresolved review feedback:
Current blocking signal:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
- Add //go:build !integration to match package convention - Replace normalisation-only cases with real precedence tests: "GITHUB_ENTERPRISE_HOST wins over GITHUB_HOST and GH_HOST" populates all three lower-priority vars so a wrong ordering would be caught - "GITHUB_HOST wins over GH_HOST" likewise sets GH_HOST to a different value to verify the ordering holds Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in the latest commit. Both issues are addressed:
Local |
|
@copilot review all uses of GH_HOST, GH_ENTERPRISE_HOST, ... in other helpers and refactor to use same unified resolution |
- Add IsAnyGitHubHostEnvVarSet() to pkg/parser/github.go as the canonical
helper for checking if any host env var is explicitly set
- Add isAnyGitHubHostEnvVarSet() shim to pkg/cli/github.go delegating to
the parser function; remove the duplicate local version from engine_secrets.go
- Replace os.Getenv("GH_HOST") == "" bypass checks in compile_orchestrator.go,
add_interactive_orchestrator.go, and setup_repository.go with
isAnyGitHubHostEnvVarSet() so all four host env vars are respected
- Simplify importAuthGHHost() to use getGitHubHost() + isAnyGitHubHostEnvVarSet()
instead of reading only GH_HOST; update GHE auth tests to set/clear all four
env vars explicitly and add coverage for GITHUB_ENTERPRISE_HOST and
GITHUB_SERVER_URL
- Extend detectGHESDeployment() to check all four env vars
(GITHUB_SERVER_URL > GITHUB_ENTERPRISE_HOST > GITHUB_HOST > GH_HOST)
instead of only GITHUB_SERVER_URL and GH_HOST
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ffix - importAuthGHHost: add manual strip fallback for the defensive parse-failure branch; switch from u.Host to u.Hostname() so port numbers are excluded from the returned hostname - detectGHESDeployment: replace strings.TrimRight(host, "/") with strings.TrimSuffix(host, "/") to remove exactly one trailing slash rather than all trailing '/' characters Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…arse error, remove unreachable fallback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. All scattered direct env var reads (
New test coverage added for all three lower-priority env vars ( |
|
@copilot please run the Failed checks:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
The isAnyGitHubHostEnvVarSet() replacement broke three existing tests because GITHUB_SERVER_URL is always set in CI (to https://github.com), which caused the git-remote auto-detection block to be skipped entirely. The bypass condition should only apply to GH_HOST because: - GH_HOST is the env var the gh CLI uses to configure its target host - GITHUB_SERVER_URL/GITHUB_ENTERPRISE_HOST/GITHUB_HOST are informational automation vars that should not prevent local git remote detection Revert compile_orchestrator.go, setup_repository.go, and add_interactive_orchestrator.go to os.Getenv("GH_HOST") checks. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Head branch was pushed to by a user without write access
Fixed in the latest commit. Root cause: the The fix reverts |
|
🎉 This pull request is included in a new release. Release: |
pkg/parser/github_host_test.gocovered only part ofGetGitHubHostForRepobehavior and had no direct coverage forGetGitHubHostenv-priority and normalization rules. This update fills those gaps and aligns subtest assertions with fail-fast testify usage.GetGitHubHostbehavior coverageTestGetGitHubHostfor:GITHUB_SERVER_URL>GITHUB_ENTERPRISE_HOST>GITHUB_HOST>GH_HOST)https://normalizationGetGitHubHostForRepoedge-case coverageacme/repo->https://github.com)microsoft/*Assertion semantics
assert.Equalwithrequire.Equalinside the subtest loop to stop each failing subtest immediately.Build-tag alignment
//go:build !integrationfrom this file since tests are pure env/unit behavior.Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
pi.devSee Network Configuration for more information.