Skip to content

Honour api_host across the CLI, and give api.Client a flexible request surface - #14104

Merged
williammartin merged 38 commits into
trunkfrom
williammartin-api-host-commit-split
Sep 2, 2026
Merged

williammartin merged 38 commits into
trunkfrom
williammartin-api-host-commit-split

Conversation

@williammartin

@williammartin williammartin commented Aug 7, 2026 •

Copy link
Copy Markdown
Member

Relates to #13717

Fixes #13991

Description

There are users out there who would like to configure an api_host in their hosts.yml, such that any requests for the GitHub API are sent to a different host. There were a lot of call sites in gh that made direct httpClient.Do requests to provided URLs, avoiding any existing api client abstractions that existed in cli/cli or cli/go-gh. We've made a lot of progress in the PRs that landed under #13991 with fairly simple lift and shifts. This PR is the Giganotosaurus to cover (nearly) all the remaining places.

The only obvious missing piece to me are codespaces commands. I believe they historically actually already have a way to achieve this via setting GITHUB_API_URL as an env var, but I didn't feel like I was ready to tackle it quite yet.

Note this doesn't support setting api_host yet, it must be configured in the hosts.yml manually.

How did you test this change?

docs/api-host-test-harness.md introduced in a289d76 introduces a comprehensive test harness (that may never be merged) that runs a series of new tests and a representative subset of our acceptance tests in a container with iptables set to blackhole github.com, requiring that API requests go through a local proxy.

GH_APIHOST_ACCEPTANCE=yes GH_APIHOST_ORG=<org> script/api-host-gateway/run.sh

In each commit after the harness is added, there should be a document that outlines the state of the test harness and acceptance tests at that commit. I red/greened this entire feature by building out the harness first and then addressing each class of issues. It's probably not perfect (see below) but I think it's worked well.

Aside from the subset of tests, I have tested it against the full acceptance suite and found one meaningful bug around telemetry which is in d4f4995 but I didn't include it in this branch because it's not a functional thing to test and just adds noise.

Notes for reviewers

Read the commits one by one from top to bottom, seriously, I put a lot of effort into splitting them into meaningful chunks and I have reviewed most of the content of each to be sure that its not totally incompetent. However, with a chunk of work this size, I wouldn't be surprised if there's some nonsense in there.

One important note is that I tried my best here to just find a non-invasive seam so the interface now exposed by api.Client is a little strange (REST, RESTWithNext, Request, RequestWithContext, DoRequest). I do have another branch that goes ham on this problem and creates a whole new abstraction, introducing a totally new client. I think this might be the future, but the risk is high, and I wasn't confident getting it into anyone's hands before vacation.

This PR depends on cli/go-gh#275

Authorship and follow-up

Who wrote this:

  • A human wrote it.
  • An agent wrote it under close human direction.
  • An agent wrote it independently, and no human has guided the implementation beyond the initial prompt.

Who answers review comments:

  • @williammartin will read and reply directly.
  • An agent will draft replies and @username will read them before they are posted.
  • Nobody has explicitly committed to replying.

@williammartin
williammartin changed the base branch from williammartin-flexible-api-client-surface to trunk August 7, 2026 15:30
@williammartin
williammartin force-pushed the williammartin-api-host-commit-split branch from 7366aab to 9869df6 Compare August 7, 2026 15:45
@williammartin
williammartin requested a lite review from Copilot August 7, 2026 15:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This draft PR continues the api_host rollout by moving remaining direct http.Client API call sites onto a shared API request surface, so per-host API routing can be applied consistently across the CLI (including pagination, raw/binary bodies, scope hints, and redirect-sensitive calls). It also adds a purpose-built gateway-based black box harness and acceptance hooks to make api_host bypasses fail loudly.

Changes:

  • Introduces a flexible api.Client request surface (Request, RequestWithContext, DoRequest, per-request headers, endpoint scopes, and redirect control) and migrates multiple command implementations to use it.
  • Adds api_host config plumbing (forward lookup and reverse lookup for credential resolution) and updates auth token attachment logic to support gateway hosts.
  • Adds a containerized gateway + blackhole test harness, plus acceptance test support for gateway runs and additional/updated acceptance scripts.
Show a summary per file
File Description
script/api-host-gateway/test.sh Container-internal black box test script exercising routed vs control vs blackhole cases, plus optional acceptance subset runner.
script/api-host-gateway/run.sh Docker entrypoint for the gateway harness; enforces clean working tree and mounts local go.mod replace targets.
script/api-host-gateway/README.md Quickstart documentation for running the gateway harness.
script/api-host-gateway/gateway/main.go Recording TLS reverse proxy that rewrites upstream hostnames out of headers/bodies and logs request evidence.
script/api-host-gateway/gateway/main_test.go Unit tests verifying forwarding, rewriting, logging, and certificate behavior of the gateway proxy.
pkg/search/searcher.go Migrates search requests to api.Client.Request, preserving search-specific error formatting.
pkg/cmd/run/view/view.go Migrates workflow run log ZIP fetching to api.Client.Request while keeping streaming behavior.
pkg/cmd/run/view/logs.go Migrates job log streaming to api.Client.Request with 404-to-“log not found” translation.
pkg/cmd/run/download/http.go Migrates artifact download to api.Client.Request while still streaming ZIP data to disk.
pkg/cmd/run/download/http_test.go Updates tests to account for repo host usage in artifact download requests.
pkg/cmd/repo/read-file/read_file_test.go Updates expected “contents API path” values to be host-relative paths instead of absolute URLs.
pkg/cmd/repo/read-file/http.go Migrates contents/raw file fetches to api.Client.Request and makes contents path host-relative.
pkg/cmd/repo/garden/http.go Migrates commit listing to api.Client.Request and threads host into request helper.
pkg/cmd/repo/garden/http_test.go Adds coverage for success and non-2xx error behavior via api.HTTPError.
pkg/cmd/repo/edit/edit.go Migrates repo topics GET/PUT calls to api.Client.RequestWithContext with explicit headers.
pkg/cmd/repo/delete/http.go Migrates repo delete to api.Client.Request with endpoint scopes and redirect suppression.
pkg/cmd/repo/delete/http_test.go Adds tests pinning delete behavior, scope suggestions, and redirect policy behavior.
pkg/cmd/repo/delete/delete_test.go Strengthens redirect-related stub realism and pins stderr output on error path.
pkg/cmd/release/upload/upload.go Threads repo host into concurrent upload path to support host-aware clients.
pkg/cmd/release/shared/upload.go Refactors upload/delete flows to use api.Client request helpers (DoRequest/RequestWithContext).
pkg/cmd/release/shared/upload_test.go Updates upload retry test harness to use a RoundTripper-backed *http.Client.
pkg/cmd/release/shared/fetch.go Migrates release/ref fetches to api.Client.RequestWithContext and adds 404 sentinel detection.
pkg/cmd/release/shared/fetch_test.go Adds tests for latest release and not-found behavior under the new request/error model.
pkg/cmd/release/edit/http.go Migrates release edit to api.Client.Request while preserving status-dependent behavior.
pkg/cmd/release/download/download.go Switches asset download to api.Client.DoRequest to preserve client redirect policy behavior.
pkg/cmd/release/download/download_test.go Adds integration-style test with real server to pin redirect rewrite behavior.
pkg/cmd/release/create/http.go Migrates “published release exists” probe to api.Client.Request with 404 handling via api.HTTPError.
pkg/cmd/release/create/create.go Threads repo host into concurrent upload path during release creation.
pkg/cmd/pr/diff/diff.go Migrates PR diff fetch to api.Client.Request with proper Accept handling and unexpected-status errors.
pkg/cmd/gist/view/view.go Threads hostname into raw gist file fetch for host-configured clients.
pkg/cmd/gist/shared/shared.go Migrates raw gist file fetch to api.Client.Request (absolute raw URL, host-configured client).
pkg/cmd/gist/shared/shared_test.go Updates gist raw fetch test signature to include hostname.
pkg/cmd/gist/edit/edit.go Threads hostname into raw file fetch for truncated gist files.
pkg/cmd/gist/create/create.go Migrates gist creation to api.Client.Request with endpoint scopes.
pkg/cmd/gist/create/create_test.go Adds scope-suggestion test coverage for gist creation error path.
pkg/cmd/extension/manager.go Threads repo host into extension asset download to keep host-aware request behavior.
pkg/cmd/extension/manager_test.go Updates tests to use absolute API URLs for release assets.
pkg/cmd/extension/http.go Migrates repo existence checks, commit SHA fetch, and asset downloads to api.Client.Request.
pkg/cmd/extension/http_test.go Adds/updates tests for SHA media type, 422 sentinel, octet-stream asset downloads, and unexpected-status behavior.
pkg/cmd/auth/shared/oauth_scopes.go Migrates scope detection calls to api.Client.Request (explicit Authorization header pre-config).
pkg/cmd/auth/shared/login_flow.go Migrates “current viewer login” lookup to api.Client.GraphQL with explicit Authorization header.
pkg/cmd/api/http.go Adds api_host-aware request URL construction for gh api and introduces swapURLHost.
pkg/cmd/api/http_test.go Expands request-building tests for api_host, verbatim path behavior, and Content-Length handling.
pkg/cmd/api/api.go Plumbs per-host api_host into the gh api request path.
internal/update/update.go Migrates update check request to api.Client.RequestWithContext while keeping absolute GitHub API URL behavior.
internal/gh/gh.go Extends auth config interface to support api_host forward and reverse lookups.
internal/config/config.go Implements api_host config accessors and reverse lookup helper for credential resolution.
internal/config/auth_config_test.go Adds coverage for reverse api_host lookup behavior.
internal/authflow/flow.go Implements new auth config interface methods for login flow config shim (no api_host support there).
go.mod Bumps cli/go-gh and updates indirect dependencies (syntax highlighter, regexp, jq/timefmt, timezone, etc.).
go.sum Updates sums corresponding to dependency bumps.
docs/api-host.md Documents api_host behavior, credential resolution, scope, and known gaps.
docs/api-host-test-harness.md Detailed documentation for the gateway harness design, operation, and interpreting failures.
api/request_test.go Adds unit tests for the new api.Client request surface semantics and options.
api/queries_repo.go Migrates repo rename/existence probe to host-relative safeurl paths and Client.Request where appropriate.
api/queries_repo_test.go Adds test coverage for “unexpected 2xx” existence probe behavior.
api/http_client.go Adds reverse api_host mapping fallback when attaching auth tokens to gateway-hosted requests.
api/http_client_test.go Adds test cases ensuring gateway host requests receive the canonical host token, without hijacking real credentials.
api/client.go Introduces request options + new request methods (Request*, DoRequest) and UnexpectedStatusError.
api/client_test.go Adds tests covering redirect suppression behavior and UnexpectedStatusError.
acceptance/testdata/search/search-issues.txtar Increases indexing wait to reduce search flake during acceptance.
acceptance/testdata/gist/gist-edit-rename-list.txtar Adds new gist acceptance script covering edit/add/rename/list behavior with required sleeps.
acceptance/testdata/gist/gist-create-view-delete.txtar Adds gist acceptance script covering create/view/raw/delete flows.
acceptance/scriptfilter_unit_test.go Adds unit tests for new multi-script filtering support.
acceptance/scriptfilter_test.go Adds filter parsing + script selection helpers used by acceptance runner.
acceptance/acceptance_test.go Adds gist test group, supports multi-script filtering, supports gateway runs via hosts.yml + SSL_CERT_FILE propagation.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 65/66 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread pkg/cmd/api/http.go
Comment on lines +160 to +161
// swapURLHost replaces the host component of rawURL with newHost, preserving
// scheme, port (if already present in rawURL), path, and query unchanged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just flagging this, but I don't think we need to change anything here unless there's need for it.

Agreed with :copilot:. Assigning u.Host clears any port numbers in the original URL. Copilot suggests we keep it as is and only update the godoc. The problem with that is we'll replace any original port number with the one in the newHost. I think it's almost okay because the original URL is meant to point at GitHub API hosts (e.g. api.github.com) and it's so rare that a production API exposes a non-default 443/80 port publicly.

The other approach, which is to preserve the port can also okay but there's a caveat, which is we should first store the original port number, then replace the host, and if there's no port number, then we replace it. This is because the newHost can be configured with a trailing port number.

Comment thread api/http_client.go

type tokenGetter interface {
ActiveToken(string) (string, string)
HostForAPIHost(string) (string, bool)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a later commit on a different branch this moves to a different interface, it shouldn't have been overloaded on tokenGetter.

Comment thread docs/api-host-test-harness.md Outdated
$ GH_APIHOST_ACCEPTANCE=yes GH_APIHOST_ORG=my-org script/api-host-gateway/run.sh
```

This creates real repositories and can take up to 45 minutes. It prompts for a

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, it takes just a few minutes to run, it's just the timeout is set high. A full acceptance suite may take 15-20 minutes though, but that's not what is running in this subset.

@babakks babakks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, @williammartin! 🙏

First off, real appreciation for the testing plan. The isolated network approach, blackholing github.com inside the container so a request that ignores api_host cannot connect at all, is a genuinely clever way to make routing falsifiable rather than something we hope is true. The per name, one script per invocation recording made it easy to reason about exactly which call site each change fixed.

I went through all of the changes, including the mechanical ones, and they read as reasonable and solid to me. There may be edge cases lurking, but if so they are the hard to find kind rather than anything obvious.

I have left a number of inline suggestions on the diff. Anything I considered a nitpick is marked as such.

A few overall points beyond the inline notes:

  • The RequestOption approach is lovely. It is both an idiomatic way to introduce optional arguments and mutations, and a nicely centralised place for call sites to shape their interaction with the API client. I expect this to age well.

  • api.UnexpectedStatusError changes the error text on unexpected 2xx responses. For example, at the body decoding call sites, a HTTP 204 (if ever) would previously have surfaced as a JSON unmarshal or EOF error, and at header reading sites like GetScopes a non 200 used to go through HandleHTTPError. Both now yield the uniform UnexpectedStatusError message. This is not quite real, but you get my point. Anyawy, users should not be relying on error text, so I think we are fine here. Just calling it out.

  • [Blocker] The new request methods need to be taught to the SafeURLPathConstruction.ql CodeQL query. Request and RequestWithContext accept a path but are not covered by isHttpUrlArgument, so a hand built URL passed to them would slip past the safeurl encoding check. The path argument index differs: Request can join the existing arg 2 branch alongside REST and RESTWithNext, while RequestWithContext needs its own branch at arg 3. I built a database locally and ran a modified query to confirm, which turned up two things worth handling as part of this: adding RequestWithContext also flags api/client.go itself, where Request delegates to it with the p parameter, so the query wants to exclude the api package; and the PR already introduces a real newly flagged site in pkg/cmd/auth/shared/oauth_scopes.go, which calls Request(hostname, GET, "", nil, ...) with an empty string path. That one is benign since there is no variable path component, but it still needs either a safeurl wrapping or an allowlist to keep the guard green. I would like to see the query updated and this site reconciled before we merge.

  • [Blocker?] Please document the new request surface in AGENTS.md. The "API Patterns" section currently only mentions GraphQL and REST. It would help future contributors to note when to reach for each of the new methods: Request and RequestWithContext for when you need the raw response, such as a streaming or non JSON body, a response header, or the status code of a success, and DoRequest for a caller built request that must control something the others cannot express, such as ContentLength and GetBody on an upload or a CheckRedirect on the client. Worth restating the guidance to prefer relative paths so host and api_host resolution applies, since absolute URLs bypass it.

Really nice piece of work overall. Enjoy the vacation! 🍻

Comment thread internal/config/config.go Outdated
Comment thread internal/config/auth_config_test.go
Comment thread acceptance/scriptfilter_unit_test.go
Comment thread internal/config/config.go
Comment thread pkg/cmd/api/http.go
Comment on lines +160 to +161
// swapURLHost replaces the host component of rawURL with newHost, preserving
// scheme, port (if already present in rawURL), path, and query unchanged.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just flagging this, but I don't think we need to change anything here unless there's need for it.

Agreed with :copilot:. Assigning u.Host clears any port numbers in the original URL. Copilot suggests we keep it as is and only update the godoc. The problem with that is we'll replace any original port number with the one in the newHost. I think it's almost okay because the original URL is meant to point at GitHub API hosts (e.g. api.github.com) and it's so rare that a production API exposes a non-default 443/80 port publicly.

The other approach, which is to preserve the port can also okay but there's a caveat, which is we should first store the original port number, then replace the host, and if there's no port number, then we replace it. This is because the newHost can be configured with a trailing port number.

Comment thread pkg/cmd/repo/delete/http.go
Comment thread api/client.go
Comment on lines +223 to +232
cfg := newRequestConfig(opts)
clientOpts := clientOptions(hostname, c.http.Transport)
for name, value := range cfg.headers {
clientOpts.Headers[name] = value
}
if cfg.noFollowRedirects {
clientOpts.CheckRedirect = func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this piece should be extracted as a function/method, and the reused here and in the GraphQL method.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on my own comment here: I dug into extracting this and I don't think it's worth it, @williammartin.

The two blocks only really overlap on the header loop. The parts that differ carry subtle contracts: GraphQL sets the feature header before applying caller headers so an explicit WithHeader wins, and the redirect policy is documented as REST only. Any shared helper either flips that header precedence or starts applying redirect handling on the GraphQL path, which is the kind of thing that quietly breaks behaviour when someone touches the shared code later.

Given how small each block is, I'd rather keep them separate than trade a tiny bit of deduplication for that risk. So I'm going to skip this one unless you feel otherwise.

Comment on lines -31 to -33
type httpClient interface {
Do(*http.Request) (*http.Response, error)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for removing this interface as it was masking HTTP client usages.

Comment thread pkg/cmd/auth/shared/oauth_scopes.go Outdated
Comment thread api/request_test.go Outdated

This comment was marked as spam.

@babakks
babakks force-pushed the williammartin-api-host-commit-split branch from 95aa24c to ecd6795 Compare August 28, 2026 12:43
@babakks

babakks commented Aug 28, 2026

Copy link
Copy Markdown
Member

Pushed a few changes (force-pushed after a rebase onto the latest trunk). Summary of what is new:

Align gh's auth-token origin check with go-gh (host only). The two token-attachment layers disagreed on port handling: go-gh's headerRoundTripper compares req.URL.Hostname() (no scheme, no port), while gh's redirect check in AddAuthTokenHeader compared URL.Host, which keeps an explicitly specified port. I converged gh onto go-gh's policy so both key the comparison on the hostname alone. getHost is now getHostname and strips any port before it is used for the redirect comparison, the token lookup key, and enterprise detection. This is consistent with how hosts are actually stored, since auth login rejects a hostname containing a port or scheme and only lowercases before writing, so config host keys are always bare hostnames. Updated the affected api tests to key their config by hostname rather than host:port.

Route attachment uploads through api.Client.DoRequest. This resolves the TODO on Uploader. It now holds an api.Client and posts assets with DoRequest, which is built for requests that must set fields Request cannot express (here ContentLength and GetBody) and already turns a non-2xx response into an HTTPError, so the hand-rolled status check and HandleHTTPError call are gone. NewUploader keeps its *http.Client parameter and wraps it, so callers are unchanged. Verified end to end against a live repo through both consumers: gh issue create --attach (append path) and gh issue comment --attach (in-body reference rewrite path).

Rebase housekeeping. Three small commits fall out of moving onto the newer trunk and its toolchain:

  • chore: apply go fix applies the Go modernizers (interface{} to any, maps.Copy, strings.SplitSeq) so the lint workflow's go fix -diff ./... check passes. All touched files were already part of this PR.
  • chore: tidy go.sum drops the stale go-gh sum lines left behind by the dependency bump.
  • A one-line test double gains the HostForAPIHost method now required by the tokenGetter interface.

go build ./..., the api and internal/attachments unit tests, golangci-lint, and both go fix -diff and go mod tidy -diff are all green.

@babakks babakks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

williammartin and others added 13 commits September 2, 2026 15:28
gist had no acceptance scripts, so its commands were never exercised
against a real host. Cover create, view and delete in one script, and
edit, rename and list in another.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
search-issues flakes because gh search reads a separate index that lags
issue creation, and five seconds was not reliably enough for it to catch
up.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The variable took a single script name, so running a chosen subset meant
one go test invocation per script. Accept a comma separated list and
select the ones belonging to the command directory under test.

A filter that matches nothing in a directory now skips that directory
rather than falling back to running all of it, so a mistyped script name
reports as a skip instead of silently passing a full run.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A call site that builds an absolute api.github.com URL and calls
httpClient.Do bypasses any central resolution, and no existing test
notices, because api.github.com is reachable from CI. That makes the
routing claim unfalsifiable.

This harness makes a bypass fail loudly. It runs the real gh binary
against a recording TLS reverse proxy, with api.github.com blackholed
inside the container, so a request that honours api_host reaches the
gateway and a request that ignores it cannot connect at all.

Results are recorded by name, and acceptance scripts run one per
invocation, so a change that fixes a single call site is visible as that
specific assertion turning green.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
go-gh gains per-host API endpoint overrides, so a host can route its API
traffic through a gateway. That immediately breaks authentication: gh
resolves tokens from the hostname in the request URL, and after the
override that hostname is the gateway, which gh has never logged in to
and holds no token for.

Map the gateway back to the host it stands in for and send that host's
token. The fallback only applies when the hostname has no token of its
own, so a host we do authenticate keeps resolving exactly as before and
an api_host mapping cannot hijack real credentials.

This is the credential half only. Requests still have to reach the
gateway to benefit, and most call sites do not yet.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
gh api builds its request URLs from the hostname directly rather than
going through the shared client, so a host's api_host had no effect on
it: gh api repos/cli/cli still went to api.github.com even when the host
was configured to route elsewhere.

Resolve api_host when building the URL for a relative path or graphql.
Absolute URLs are deliberately left alone, both because the user asked
for that exact URL and because paginating on a rewritten Link header
depends on following the gateway's own URLs unchanged.

This means api_host is now resolved in two places, which is a smell
worth being honest about rather than hiding: gh api takes its path
verbatim from the user, so it cannot use the shared client that resolves
api_host for everything else.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Being on api.Client is not the same as being routed. RenameRepo already
used client.REST, but handed it an absolute URL built from
ghinstance.RESTPrefix, so the host was decided before the client saw the
request and a configured api_host was ignored.

Pass a relative path and let the client resolve the host, as it does for
every other call. No new capability is needed here, only the removal of
a hardcoded prefix.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Many call sites could not move onto api.Client because REST decodes into
a receiver, and they need the response itself: a streaming body, a body
that is not JSON, a response header, or the status code of a success.
Having no way to express that, they built absolute api.github.com URLs
and called httpClient.Do, which decides the host before any central
resolution can apply.

Add Request and RequestWithContext, which return the response for the
caller to consume, and migrate those call sites. Non-2xx responses still
become an HTTPError, so callers only handle the success path.

Add UnexpectedStatusError for callers that require one specific status.
Since every non-2xx is already an error, such a caller can only be
surprised by a different 2xx, and handing a success to an error parser
would be wrong.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Some endpoints do not say which OAuth scopes they require, so gh supplies the
answer itself: it calls EndpointNeedsScopes on the response before turning it
into an error, which folds the scope into the suggestion the user is shown.

That only works while the call site holds the response. A call site that hands
request making to the shared client never sees a failed response, because the
client has already converted it into an error and closed the body. So the
scope has to travel with the request instead.

Add WithEndpointScopes, and apply it on the error path: the scope is added to
the error's headers before the suggestion is generated, which is the same
mechanism as before, moved to where the response still exists.

gh gist create is the first caller, and needed this to keep telling a user
with an under-scoped token that they are missing the gist scope.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
gh repo delete is the reason this option exists, and the reason it built its
own client rather than sharing one. Deleting a repository that has since been
renamed returns a 301, and Go's default redirect policy turns a DELETE into a
GET when it follows one. The user would be told the delete succeeded while
nothing had been deleted, so the command copied the http.Client, set
CheckRedirect on the copy, and made the request itself.

Making the request itself is also how it came to name api.github.com and
ignore a host's api_host. Add WithoutFollowingRedirects so the policy can be
stated per request, and the destination can go back to being the client's
business.

The option is deliberately REST-only. GraphQL does not meet redirects in
practice, and offering it there would suggest a guarantee that is not tested.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
The remaining call sites that build their own request do so for a header,
usually an Accept naming a preview media type or a raw representation. Building
the request also meant building the URL, which is how these came to name
api.github.com and ignore a host's api_host.

Add WithHeader, so a call site can say which header it needs without also
taking ownership of where the request goes. Headers set this way take
precedence over the transport's own, which is what a caller asking for a
specific representation means.

GraphQL takes options too, for one reason: gh auth login validates a token the
client has not been configured with yet, so it must pass an explicit
Authorization header.

This is the last of the three capabilities, and with it the gateway harness is
fully green.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
These two call sites are the last outside gh api that send a request through a
raw http.Client, and they resisted the shared request surface for a real
reason. Uploading an asset sets ContentLength and GetBody so a retry can rewind
the file, and neither is expressible as a method, path, body or header.

Add DoRequest, which takes a request the caller has built and applies the same
error handling as Request. It also sends the request through the client held by
api.Client, so a CheckRedirect set on that client survives, which Request
cannot promise because go-gh builds a client of its own from the transport.

This changes no behaviour. Both sites use absolute URLs the API returned, and
those URLs already point wherever the request that produced them went, so
routing was never wrong here. What changes is that api.Client is now the single
place a request leaves gh, so a later change to how a destination is resolved
reaches these two without anyone remembering they exist.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6dd1d13e-90bd-44b8-8bec-261795e282c3
Note that Go's default redirect policy rewrites any non-GET/HEAD method
(not just DELETE) to GET on 301/302/303, and document why repo delete
opts out of following redirects to avoid a phantom success.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
babakks and others added 17 commits September 2, 2026 15:28
Add Request and RequestWithContext to the SafeURL path construction query so
their URL arguments must flow through safeurl. Exclude the client's own
internal delegation between these methods, which forwards the caller's already
checked path and would otherwise be reported as a hand built URL.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
…oken

Converge cli/cli's origin-comparison policy onto go-gh's host-only policy so
both token layers ignore the port when deciding whether a request is same-host.
go-gh compares req.URL.Hostname() while cli/cli previously compared the full
host including port, making the two layers inconsistent.

Rename getHost to getHostname and strip any port from the host so redirect
comparison, token lookup, and enterprise detection all key on the hostname
alone, matching how gh stores config host keys (auth login rejects a port).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Signed-off-by: Babak K. Shandiz <babakks@github.com>
Hold an api.Client on Uploader and post assets with DoRequest instead of
calling http.Client.Do directly. DoRequest is built for requests that must
set fields Request cannot express, such as ContentLength and GetBody when
uploading an asset, and it already turns a non-2xx response into an HTTPError,
so the hand-rolled status check and HandleHTTPError call are no longer needed.

NewUploader keeps its *http.Client parameter and wraps it, so callers are
unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
The go-gh per-host api_host branch was rebased onto its latest trunk, so
update the pin to its new HEAD.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb7763a9-2086-461a-91a4-86ea4a2d078d
@williammartin
williammartin force-pushed the williammartin-api-host-commit-split branch from 2352e08 to 8103f34 Compare September 2, 2026 14:56
@williammartin
williammartin marked this pull request as ready for review September 2, 2026 14:56
@williammartin
williammartin requested review from a team as code owners September 2, 2026 14:56
@williammartin
williammartin force-pushed the williammartin-api-host-commit-split branch from 8103f34 to 6865464 Compare September 2, 2026 15:02

@BagToad BagToad left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this against a 150 test suite of various invocations of --attach across both github.com and GHEC DR

@williammartin
williammartin merged commit adda317 into trunk Sep 2, 2026
11 checks passed
@williammartin
williammartin deleted the williammartin-api-host-commit-split branch September 2, 2026 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consolidate API Client usage

5 participants