From 260ac55c60c6306d9beeac4d92a2d1ee6ffecad7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 11:33:43 +0200 Subject: [PATCH 01/16] Add flexible request surface to api.Client api.Client.REST and RESTWithNext only cover sending JSON and decoding JSON, so call sites needing anything else have stayed on the raw *http.Client. This adds the surface those sites need. Request and RequestWithContext return the response for the caller to consume rather than decoding into a receiver, which covers streaming bodies, non-JSON bodies, response headers and the status code of a successful response. Neither special-cases 204/205 nor decodes on success, so bodiless and non-JSON successes both work. Endpoint resolution is delegated to go-gh rather than reimplemented here, so that per-host API endpoint routing is inherited once it lands upstream rather than having to be rebuilt. This is why the surface does not hand back a caller-built *http.Request: building one requires resolving the URL first. RequestOption carries per-request headers, which are the single largest blocker across the remaining call sites, and go-gh has no equivalent since its headers are client-level only. api.Client already builds a fresh go-gh client per call, so client-level headers are effectively per-request. WithEndpointScopes preserves the EndpointNeedsScopes hook for call sites that can no longer reach the response before it becomes an error. DoRequest exists for the few sites that must set request fields which are neither method, path, body nor header, such as ContentLength and GetBody when uploading a release asset. Those sites all use absolute URLs supplied by the API, where there is no endpoint resolution to delegate, so it gives up nothing. Header precedence is verified against the transport built by NewHTTPClient rather than a test tripper, since the value of a per-request header depends entirely on it winning against the real transport defaults. Refs #13991 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- api/client.go | 124 +++++++++++++- api/request_test.go | 393 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 513 insertions(+), 4 deletions(-) create mode 100644 api/request_test.go diff --git a/api/client.go b/api/client.go index 27a747995c9..f11795edbe9 100644 --- a/api/client.go +++ b/api/client.go @@ -52,12 +52,50 @@ func (err HTTPError) ScopesSuggestion() string { return err.scopesSuggestion } +// RequestOption configures a single request made through Request, RequestWithContext or GraphQL. +type RequestOption func(*requestConfig) + +// requestConfig accumulates the effect of the RequestOptions applied to one request. +type requestConfig struct { + headers map[string]string + endpointScopes string +} + +// WithHeader sets a header on the request, taking precedence over any header of the same name +// that the underlying transport would otherwise supply. +func WithHeader(name, value string) RequestOption { + return func(cfg *requestConfig) { + cfg.headers[name] = value + } +} + +// WithEndpointScopes adds OAuth scopes to a 4xx error as if the server endpoint had returned them. +// This improves error messaging for endpoints that do not explicitly list the scopes they need, and +// is the Request equivalent of calling EndpointNeedsScopes on a raw response. +func WithEndpointScopes(scopes string) RequestOption { + return func(cfg *requestConfig) { + cfg.endpointScopes = scopes + } +} + +func newRequestConfig(opts []RequestOption) requestConfig { + cfg := requestConfig{headers: map[string]string{}} + for _, opt := range opts { + opt(&cfg) + } + return cfg +} + // GraphQL performs a GraphQL request using the query string and parses the response into data receiver. If there are errors in the response, // GraphQLError will be returned, but the receiver will also be partially populated. -func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}) error { - opts := clientOptions(hostname, c.http.Transport) - opts.Headers[graphqlFeatures] = features - gqlClient, err := ghAPI.NewGraphQLClient(opts) +func (c Client) GraphQL(hostname string, query string, variables map[string]interface{}, data interface{}, opts ...RequestOption) error { + cfg := newRequestConfig(opts) + clientOpts := clientOptions(hostname, c.http.Transport) + clientOpts.Headers[graphqlFeatures] = features + for name, value := range cfg.headers { + clientOpts.Headers[name] = value + } + gqlClient, err := ghAPI.NewGraphQLClient(clientOpts) if err != nil { return err } @@ -147,6 +185,69 @@ func (c Client) RESTWithNext(hostname string, method string, p string, body io.R return next, nil } +// Request performs a REST request and returns the response for the caller to consume. +// +// See RequestWithContext for the full semantics. +func (c Client) Request(hostname string, method string, p string, body io.Reader, opts ...RequestOption) (*http.Response, error) { + return c.RequestWithContext(context.Background(), hostname, method, p, body, opts...) +} + +// RequestWithContext performs a REST request and returns the response for the caller to consume, +// rather than decoding it into a receiver as REST does. It exists for call sites that need access +// to the response itself: streaming bodies, non-JSON bodies, response headers, or the status code +// of a successful response. +// +// p may be a path relative to the host's REST endpoint, or an absolute URL. Prefer a relative path. +// Absolute URLs are requested as given, which is correct for URLs supplied by the API itself, such +// as asset and upload URLs, but means they do not benefit from host-level endpoint resolution. +// +// Responses outside the 2xx range are returned as an HTTPError and their body is closed. On success +// the response is returned unread and the caller is responsible for closing its body. +func (c Client) RequestWithContext(ctx context.Context, hostname string, method string, p string, body io.Reader, opts ...RequestOption) (*http.Response, error) { + cfg := newRequestConfig(opts) + clientOpts := clientOptions(hostname, c.http.Transport) + for name, value := range cfg.headers { + clientOpts.Headers[name] = value + } + + restClient, err := ghAPI.NewRESTClient(clientOpts) + if err != nil { + return nil, err + } + + resp, err := restClient.RequestWithContext(ctx, method, p, body) + if err != nil { + return nil, handleRequestError(err, cfg.endpointScopes) + } + + return resp, nil +} + +// DoRequest sends a request that the caller has already built, and returns the response for the +// caller to consume. It applies the same error handling as Request. +// +// Prefer Request. DoRequest exists for the few call sites that must control fields of the request +// which are neither method, path, body nor header, such as ContentLength and GetBody when uploading +// a release asset. Because the caller supplies the URL, requests sent this way do not benefit from +// host-level endpoint resolution, so it is only appropriate for absolute URLs supplied by the API. +// +// Responses outside the 2xx range are returned as an HTTPError and their body is closed. On success +// the response is returned unread and the caller is responsible for closing its body. +func (c Client) DoRequest(req *http.Request) (*http.Response, error) { + resp, err := c.http.Do(req) + if err != nil { + return nil, err + } + + success := resp.StatusCode >= 200 && resp.StatusCode < 300 + if !success { + defer resp.Body.Close() + return nil, HandleHTTPError(resp) + } + + return resp, nil +} + // HandleHTTPError parses a http.Response into a HTTPError. // // The caller is responsible to close the response body stream. @@ -154,6 +255,21 @@ func HandleHTTPError(resp *http.Response) error { return handleResponse(ghAPI.HandleHTTPError(resp)) } +// handleRequestError converts a request error into an HTTPError or GraphQLError, first adding any +// endpoint scopes to the error's headers so that the scopes suggestion generated by handleResponse +// accounts for them. This is the error-path equivalent of calling EndpointNeedsScopes on a response +// before HandleHTTPError, which call sites can no longer do when the response never reaches them. +func handleRequestError(err error, endpointScopes string) error { + if endpointScopes != "" { + var restErr *ghAPI.HTTPError + if errors.As(err, &restErr) && restErr.Headers != nil && restErr.StatusCode >= 400 && restErr.StatusCode < 500 { + oldScopes := restErr.Headers.Get("X-Accepted-Oauth-Scopes") + restErr.Headers.Set("X-Accepted-Oauth-Scopes", fmt.Sprintf("%s, %s", oldScopes, endpointScopes)) + } + } + return handleResponse(err) +} + // handleResponse takes a ghAPI.HTTPError or ghAPI.GraphQLError and converts it into an // HTTPError or GraphQLError respectively. func handleResponse(err error) error { diff --git a/api/request_test.go b/api/request_test.go new file mode 100644 index 00000000000..ecf0cf53ef0 --- /dev/null +++ b/api/request_test.go @@ -0,0 +1,393 @@ +package api + +import ( + "bytes" + "context" + + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequestResolvesPathAgainstHost(t *testing.T) { + tests := []struct { + name string + hostname string + path string + wantHostname string + wantPath string + }{ + { + name: "relative path resolves against github.com", + hostname: "github.com", + path: "repos/OWNER/REPO", + wantHostname: "api.github.com", + wantPath: "/repos/OWNER/REPO", + }, + { + name: "relative path resolves against an enterprise host", + hostname: "example.com", + path: "repos/OWNER/REPO", + wantHostname: "example.com", + wantPath: "/api/v3/repos/OWNER/REPO", + }, + { + // Absolute URLs are how API-supplied locations such as asset and upload URLs + // reach this method, so they must be requested exactly as given. + name: "absolute URL is requested as given", + hostname: "github.com", + path: "https://uploads.github.com/assets/1234", + wantHostname: "uploads.github.com", + wantPath: "/assets/1234", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request(tt.hostname, http.MethodGet, tt.path, nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, tt.wantHostname, reg.Requests[0].URL.Hostname()) + assert.Equal(t, tt.wantPath, reg.Requests[0].URL.Path) + }) + } +} + +func TestRequestWithHeader(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil, + WithHeader("Accept", "application/vnd.github.raw")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/vnd.github.raw", reg.Requests[0].Header.Get("Accept")) +} + +// The transport supplies default headers of its own, so a per-request header is only useful if it +// takes precedence over them. +func TestRequestHeaderOverridesTransportDefault(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodPost, "repos/OWNER/REPO/releases", nil, + WithHeader("Content-Type", "application/zip")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/zip", reg.Requests[0].Header.Get("Content-Type")) +} + +// Pre-auth call sites supply their own token because it is not yet in config, so an explicit +// Authorization header must survive rather than be replaced by the transport. +func TestRequestExplicitAuthorizationHeaderIsPreserved(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + httpClient := &http.Client{} + httpmock.ReplaceTripper(httpClient, reg) + httpClient.Transport = AddAuthTokenHeader(httpClient.Transport, tinyConfig{"github.com:oauth_token": "config-token"}) + client := NewClientFromHTTP(httpClient) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "{}")) + + resp, err := client.Request("github.com", http.MethodGet, "user", nil, + WithHeader("Authorization", "token explicit-token")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "token explicit-token", reg.Requests[0].Header.Get("Authorization")) +} + +func TestRequestReturnsBodyUnread(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, "the raw body")) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO/contents/f", nil) + require.NoError(t, err) + defer resp.Body.Close() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, "the raw body", string(body)) +} + +// REST treats 204 and 205 specially and decodes every other 2xx as JSON. Request must do neither, +// so that bodiless successes and non-JSON successes both work. +func TestRequestDoesNotDecodeSuccessfulResponses(t *testing.T) { + tests := []struct { + name string + statusCode int + body string + }{ + {name: "bodiless 200, as returned by HEAD", statusCode: 200, body: ""}, + {name: "no content", statusCode: 204, body: ""}, + {name: "non-JSON body", statusCode: 200, body: "d3f9c4a"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(tt.statusCode, tt.body)) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, tt.statusCode, resp.StatusCode) + }) + } +} + +func TestRequestError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusJSONResponse(404, map[string]interface{}{ + "message": "Not Found", + })) + + resp, err := client.Request("github.com", http.MethodGet, "repos/OWNER/REPO", nil) + assert.Nil(t, resp) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 404, httpErr.StatusCode) +} + +func TestRequestErrorPreservesScopesSuggestion(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: 404, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Not Found"}`)), + Header: map[string][]string{ + "Content-Type": {"application/json; charset=utf-8"}, + "X-Accepted-Oauth-Scopes": {"delete_repo"}, + "X-Oauth-Scopes": {"repo"}, + }, + }, nil + }) + + _, err := client.Request("github.com", http.MethodDelete, "repos/OWNER/REPO", nil) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "delete_repo") +} + +// Call sites used to mutate the response with EndpointNeedsScopes before converting it to an error. +// Request converts internally, so WithEndpointScopes has to reproduce that suggestion. +func TestRequestWithEndpointScopes(t *testing.T) { + tests := []struct { + name string + statusCode int + acceptedScopes string + wantSuggestion string + }{ + { + name: "adds scopes the endpoint did not report", + statusCode: 403, + acceptedScopes: "", + wantSuggestion: "delete_repo", + }, + { + name: "appends to scopes the endpoint did report", + statusCode: 403, + acceptedScopes: "repo", + wantSuggestion: "delete_repo", + }, + { + name: "leaves 5xx errors alone", + statusCode: 500, + acceptedScopes: "", + wantSuggestion: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + header := map[string][]string{ + "Content-Type": {"application/json; charset=utf-8"}, + "X-Oauth-Scopes": {"repo"}, + } + if tt.acceptedScopes != "" { + header["X-Accepted-Oauth-Scopes"] = []string{tt.acceptedScopes} + } + return &http.Response{ + Request: req, + StatusCode: tt.statusCode, + Body: io.NopCloser(bytes.NewBufferString(`{"message": "Forbidden"}`)), + Header: header, + }, nil + }) + + _, err := client.Request("github.com", http.MethodDelete, "repos/OWNER/REPO", nil, + WithEndpointScopes("delete_repo")) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + if tt.wantSuggestion == "" { + assert.Empty(t, httpErr.ScopesSuggestion()) + } else { + assert.Contains(t, httpErr.ScopesSuggestion(), tt.wantSuggestion) + } + }) + } +} + +type ctxKey struct{} + +// Call sites such as release asset upload run inside a retry loop that owns a context, so the +// context has to reach the outgoing request. +func TestRequestWithContextPropagatesContext(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + var gotValue interface{} + reg.Register(httpmock.MatchAny, func(req *http.Request) (*http.Response, error) { + gotValue = req.Context().Value(ctxKey{}) + return httpmock.StatusStringResponse(200, "{}")(req) + }) + + ctx := context.WithValue(context.Background(), ctxKey{}, "carried") + + resp, err := client.RequestWithContext(ctx, "github.com", http.MethodGet, "repos/OWNER/REPO", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "carried", gotValue) +} + +func TestDoRequest(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(201, `{"id": 1}`)) + + // Fields such as ContentLength are the reason DoRequest exists, so assert one survives. + req, err := http.NewRequest(http.MethodPost, "https://uploads.github.com/assets", strings.NewReader("asset")) + require.NoError(t, err) + req.ContentLength = 5 + + resp, err := client.DoRequest(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, 201, resp.StatusCode) + assert.Equal(t, "uploads.github.com", reg.Requests[0].URL.Hostname()) + assert.Equal(t, int64(5), reg.Requests[0].ContentLength) + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Equal(t, `{"id": 1}`, string(body)) +} + +func TestDoRequestError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusJSONResponse(422, map[string]interface{}{ + "message": "Validation Failed", + })) + + req, err := http.NewRequest(http.MethodPost, "https://uploads.github.com/assets", nil) + require.NoError(t, err) + + resp, err := client.DoRequest(req) + assert.Nil(t, resp) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 422, httpErr.StatusCode) + assert.Equal(t, "Validation Failed", httpErr.Message) +} + +// The transport built by NewHTTPClient supplies default Accept, Content-Type and User-Agent +// headers. Per-request headers are only useful if they win against that real transport, so this +// exercises it rather than a bare test tripper. +func TestRequestHeadersAgainstRealTransport(t *testing.T) { + var gotReq *http.Request + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotReq = r + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + ios, _, _, _ := iostreams.Test() + httpClient, err := NewHTTPClient(HTTPClientOptions{ + AppVersion: "v1.2.3", + Config: tinyConfig{ts.URL[7:] + ":oauth_token": "MYTOKEN"}, + Log: ios.ErrOut, + }) + require.NoError(t, err) + client := NewClientFromHTTP(httpClient) + + resp, err := client.Request(ts.URL, http.MethodGet, ts.URL+"/user/repos", nil, + WithHeader("Accept", "application/vnd.github.raw"), + WithHeader("Content-Type", "application/zip")) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "application/vnd.github.raw", gotReq.Header.Get("Accept")) + assert.Equal(t, "application/zip", gotReq.Header.Get("Content-Type")) + + // Headers the caller did not override must still be supplied by the transport. + assert.Equal(t, "token MYTOKEN", gotReq.Header.Get("Authorization")) + assert.Equal(t, "GitHub CLI v1.2.3", gotReq.Header.Get("User-Agent")) +} + +func TestGraphQLWithHeader(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + client := newTestClient(reg) + + reg.Register(httpmock.MatchAny, httpmock.StatusStringResponse(200, `{"data": {}}`)) + + var response struct{} + err := client.GraphQL("github.com", "query{}", nil, &response, + WithHeader("Authorization", "token explicit-token")) + require.NoError(t, err) + + assert.Equal(t, "token explicit-token", reg.Requests[0].Header.Get("Authorization")) +} From acc084d6d419c1d4f12ad7a97744f1f89c1b2584 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:14:05 +0200 Subject: [PATCH 02/16] Route release fetch through api.Client The two release lookup sites in pkg/cmd/release/shared built an absolute URL with JoinPathWithHostPrefix and sent it through the raw http.Client. Both are plain GETs whose only reason for bypassing api.Client was that they need to treat a 404 as a not-found sentinel rather than an error. They now build a relative path and go through Request, which resolves the endpoint in go-gh. That is what lets these sites pick up api_host config later; lifting the absolute URL across would have moved the code without delivering the benefit. The 404 sentinel moves from a status check on the response to errors.As on the returned HTTPError. RequestWithContext is used rather than REST so that the cancellation FetchRelease relies on to drop the losing lookup is preserved. TestFetchRefSHA already pinned the sentinel for one of the two sites. Tests are added for the other, which was unpinned, covering the sentinel both directly through FetchLatestRelease and through the dual lookup in FetchRelease. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/release/shared/fetch.go | 67 ++++++++++----------- pkg/cmd/release/shared/fetch_test.go | 88 ++++++++++++++++++++++++++++ 2 files changed, 118 insertions(+), 37 deletions(-) diff --git a/pkg/cmd/release/shared/fetch.go b/pkg/cmd/release/shared/fetch.go index 74bf06657a1..bfc5a9fb667 100644 --- a/pkg/cmd/release/shared/fetch.go +++ b/pkg/cmd/release/shared/fetch.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "reflect" "strconv" @@ -14,7 +13,6 @@ import ( "time" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/httpmock" @@ -138,30 +136,23 @@ type fetchResult struct { } func FetchRefSHA(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "git", "ref", fmt.Sprintf("tags/%s", tagName)) - if err != nil { - return "", err - } - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "git", "ref", fmt.Sprintf("tags/%s", tagName)) if err != nil { return "", err } - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodGet, path.String(), nil) if err != nil { + if isNotFound(err) { + return "", ErrReleaseNotFound + } return "", err } defer resp.Body.Close() - if resp.StatusCode == http.StatusNotFound { - _, _ = io.Copy(io.Discard, resp.Body) - return "", ErrReleaseNotFound - } - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - var ref struct { Object struct { SHA string `json:"sha"` @@ -190,7 +181,7 @@ func DigestAlgForRef(digest string) string { // FetchRelease finds a published repository release by its tagName, or a draft release by its pending tag name. func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface, tagName string) (*Release, error) { - publishedURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) + publishedPath, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) if err != nil { return nil, err } @@ -200,7 +191,7 @@ func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Inte // published release lookup go func() { - release, err := fetchReleasePath(cc, httpClient, publishedURL) + release, err := fetchReleasePath(cc, httpClient, repo.RepoHost(), publishedPath) results <- fetchResult{release: release, error: err} }() @@ -235,11 +226,11 @@ func FetchRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Inte // FetchLatestRelease finds the latest published release for a repository. func FetchLatestRelease(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) (*Release, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "latest") + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "latest") if err != nil { return nil, err } - return fetchReleasePath(ctx, httpClient, url) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) } // fetchDraftRelease returns the first draft release that has tagName as its pending tag. @@ -271,32 +262,26 @@ func fetchDraftRelease(ctx context.Context, httpClient *http.Client, repo ghrepo // Then, use REST to get information about the draft release. In theory, we could have fetched // all the necessary information via GraphQL, but REST is safer for backwards compatibility. - path, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(query.Repository.Release.DatabaseID, 10)) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(query.Repository.Release.DatabaseID, 10)) if err != nil { return nil, err } - return fetchReleasePath(ctx, httpClient, path) + return fetchReleasePath(ctx, httpClient, repo.RepoHost(), path) } -func fetchReleasePath(ctx context.Context, httpClient *http.Client, url safeurl.SafeURL) (*Release, error) { - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) +func fetchReleasePath(ctx context.Context, httpClient *http.Client, host string, path safeurl.SafeURL) (*Release, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, host, http.MethodGet, path.String(), nil) if err != nil { + if isNotFound(err) { + return nil, ErrReleaseNotFound + } return nil, err } defer resp.Body.Close() - if resp.StatusCode == http.StatusNotFound { - _, _ = io.Copy(io.Discard, resp.Body) - return nil, ErrReleaseNotFound - } else if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - var release Release if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { return nil, err @@ -305,6 +290,14 @@ func fetchReleasePath(ctx context.Context, httpClient *http.Client, url safeurl. return &release, nil } +// isNotFound reports whether err is an API error carrying a 404 status. The release lookups +// treat a missing release as a sentinel rather than an error, and the api client reports the +// status through an HTTPError rather than through a response the call site can inspect. +func isNotFound(err error) bool { + var httpErr api.HTTPError + return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound +} + func StubFetchRelease(t *testing.T, reg *httpmock.Registry, owner, repoName, tagName, responseBody string) { path := "repos/OWNER/REPO/releases/tags/v1.2.3" if tagName == "" { diff --git a/pkg/cmd/release/shared/fetch_test.go b/pkg/cmd/release/shared/fetch_test.go index e68278f1ed1..97e0968ea34 100644 --- a/pkg/cmd/release/shared/fetch_test.go +++ b/pkg/cmd/release/shared/fetch_test.go @@ -93,6 +93,94 @@ func TestFetchRefSHA(t *testing.T) { } } +func TestFetchLatestRelease(t *testing.T) { + tests := []struct { + name string + responseStatus int + responseBody string + expectedTag string + expectedErr error + errorMessage string + }{ + { + name: "found (200)", + responseStatus: 200, + responseBody: `{"tag_name": "v1.2.3"}`, + expectedTag: "v1.2.3", + }, + { + name: "not found (404)", + responseStatus: 404, + expectedErr: ErrReleaseNotFound, + }, + { + name: "server error (500)", + responseStatus: 500, + errorMessage: "HTTP 500", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeHTTP := &httpmock.Registry{} + defer fakeHTTP.Verify(t) + + repo, err := ghrepo.FromFullName("owner/repo") + require.NoError(t, err) + + if tt.responseStatus > 299 { + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.JSONErrorResponse(tt.responseStatus, api.HTTPError{ + StatusCode: tt.responseStatus, + Message: "some error", + }), + ) + } else { + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody), + ) + } + + release, err := FetchLatestRelease(context.Background(), &http.Client{Transport: fakeHTTP}, repo) + + switch { + case tt.expectedErr != nil: + require.ErrorIs(t, err, tt.expectedErr) + case tt.errorMessage != "": + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errorMessage) + default: + require.NoError(t, err) + assert.Equal(t, tt.expectedTag, release.TagName) + } + }) + } +} + +// TestFetchReleaseNotFoundWhenBothLookupsMiss pins the sentinel that FetchRelease reports when +// neither the published nor the draft lookup finds a release, since the published lookup now +// recognises a miss from an HTTPError rather than from the response status directly. +func TestFetchReleaseNotFoundWhenBothLookupsMiss(t *testing.T) { + fakeHTTP := &httpmock.Registry{} + + repo, err := ghrepo.FromFullName("owner/repo") + require.NoError(t, err) + + fakeHTTP.Register( + httpmock.REST("GET", "repos/owner/repo/releases/tags/v9.9.9"), + httpmock.JSONErrorResponse(404, api.HTTPError{StatusCode: 404, Message: "Not Found"}), + ) + fakeHTTP.Register( + httpmock.GraphQL(`query RepositoryReleaseByTag\b`), + httpmock.StringResponse(`{"data": {"repository": {"release": null}}}`), + ) + + _, err = FetchRelease(context.Background(), &http.Client{Transport: fakeHTTP}, repo, "v9.9.9") + require.ErrorIs(t, err, ErrReleaseNotFound) +} + func TestDigestAlgForRef(t *testing.T) { tests := []struct { name string From eae8fd79e3761bd5faa81e0662710ce3057ae97c Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:15:16 +0200 Subject: [PATCH 03/16] Route repo read-file through api.Client Both Contents API sites bypassed api.Client only because they need a custom Accept media type: the object+json type that makes the API return a single object for files and directories alike, and the raw type used for files above the inline content limit. They now go through Request with WithHeader, and contentsAPIPath returns a relative path so the endpoint is resolved by go-gh rather than baked in here. The existing tests already match on the Accept header, so they confirm the per-request header survives the trip through the client and the transport. Test_contentsAPIPath is updated for the relative path it now builds. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/repo/read-file/http.go | 38 ++++++++---------------- pkg/cmd/repo/read-file/read_file_test.go | 10 +++---- 2 files changed, 18 insertions(+), 30 deletions(-) diff --git a/pkg/cmd/repo/read-file/http.go b/pkg/cmd/repo/read-file/http.go index 20c1ed0e9f1..5f4b8eb23b0 100644 --- a/pkg/cmd/repo/read-file/http.go +++ b/pkg/cmd/repo/read-file/http.go @@ -9,7 +9,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -88,25 +87,19 @@ func fetchContent(httpClient *http.Client, repo ghrepo.Interface, filePath, ref return nil, err } - req, err := http.NewRequest("GET", apiPath.String(), nil) - if err != nil { - return nil, err - } // We use the "application/vnd.github.object+json" media type to request a unified object // representation from the Contents API. Without this, the API returns a JSON array for // directories and a JSON object for files. - req.Header.Set("Accept", "application/vnd.github.object+json") - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, apiPath.String(), nil, + api.WithHeader("Accept", "application/vnd.github.object+json")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - body, err := io.ReadAll(resp.Body) if err != nil { return nil, err @@ -179,30 +172,25 @@ func fetchRawFile(httpClient *http.Client, repo ghrepo.Interface, filePath, ref return nil, err } - req, err := http.NewRequest("GET", apiPath.String(), nil) - if err != nil { - return nil, err - } - req.Header.Set("Accept", "application/vnd.github.raw") - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, apiPath.String(), nil, + api.WithHeader("Accept", "application/vnd.github.raw")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - return io.ReadAll(resp.Body) } -// contentsAPIPath builds the absolute Contents API URL for a path and optional ref. +// contentsAPIPath builds the Contents API path for a path and optional ref, relative to the +// host's REST endpoint. func contentsAPIPath(repo ghrepo.Interface, filePath, ref string) (safeurl.SafeURL, error) { // The Contents API accepts a fully percent-encoded path, including path separators // encoded as %2F, so spaces and other special characters are handled transparently. - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "contents", strings.TrimPrefix(filePath, "/")) + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "contents", strings.TrimPrefix(filePath, "/")) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/read-file/read_file_test.go b/pkg/cmd/repo/read-file/read_file_test.go index 59d3856e40b..422cb00c425 100644 --- a/pkg/cmd/repo/read-file/read_file_test.go +++ b/pkg/cmd/repo/read-file/read_file_test.go @@ -724,28 +724,28 @@ func Test_contentsAPIPath(t *testing.T) { { name: "simple path", filePath: "README.md", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md", + want: "repos/OWNER/REPO/contents/README.md", }, { name: "leading slash trimmed", filePath: "/README.md", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md", + want: "repos/OWNER/REPO/contents/README.md", }, { name: "nested path encodes separators", filePath: "pkg/cmd/create.go", - want: "https://api.github.com/repos/OWNER/REPO/contents/pkg%2Fcmd%2Fcreate.go", + want: "repos/OWNER/REPO/contents/pkg%2Fcmd%2Fcreate.go", }, { name: "spaces are encoded", filePath: "dir with spaces/file.txt", - want: "https://api.github.com/repos/OWNER/REPO/contents/dir%20with%20spaces%2Ffile.txt", + want: "repos/OWNER/REPO/contents/dir%20with%20spaces%2Ffile.txt", }, { name: "ref is appended", filePath: "README.md", ref: "feature/branch", - want: "https://api.github.com/repos/OWNER/REPO/contents/README.md?ref=feature%2Fbranch", + want: "repos/OWNER/REPO/contents/README.md?ref=feature%2Fbranch", }, } From 2a3a616aeaa9c337e8a0a2fe154f2a0f716e997f Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:18:23 +0200 Subject: [PATCH 04/16] Route remaining extension sites through api.Client Three sites remained on the raw client. All now go through Request. repoExists and fetchCommitSHA are body-blind or read a non-JSON body, so Request is the right fit rather than REST: go-gh's Do unmarshals the body even when the response argument is nil, which would reject the empty and non-JSON bodies that TestRepoExists already covers. repoExists keeps its exact-status contract. Only 200 counts as existence, and any other success status is still reported as an error, which the existing tests require. fetchCommitSHA and downloadAsset need custom Accept media types, and neither was covered by a test, so a missing header would have gone unnoticed. Tests are added that match on the media type, along with the 422 commit-not-found sentinel that also moves to errors.As. downloadAsset takes a hostname now because it is given an absolute asset URL by the API, which is requested as given rather than resolved against the host. Two asset fixtures in manager_test.go used a host-relative URL, which only worked because the mock transport never validated it. They are now absolute like every other asset fixture in that file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/extension/http.go | 80 +++++++++++++---------------- pkg/cmd/extension/http_test.go | 84 +++++++++++++++++++++++++++++++ pkg/cmd/extension/manager.go | 2 +- pkg/cmd/extension/manager_test.go | 4 +- 4 files changed, 122 insertions(+), 48 deletions(-) diff --git a/pkg/cmd/extension/http.go b/pkg/cmd/extension/http.go index 333564d29a4..740698871f5 100644 --- a/pkg/cmd/extension/http.go +++ b/pkg/cmd/extension/http.go @@ -8,38 +8,38 @@ import ( "os" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) func repoExists(httpClient *http.Client, repo ghrepo.Interface) (bool, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) - if err != nil { - return false, err - } - - req, err := http.NewRequest(http.MethodGet, url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return false, err } + // The response body is deliberately not read. Existence is decided by the status alone, + // so Request is used rather than REST, which would try to decode the body. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its exact-status contract and body-blind response handling. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodGet, path.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } defer resp.Body.Close() - switch resp.StatusCode { - case http.StatusOK: - return true, nil - case http.StatusNotFound: - return false, nil - default: + // Only 200 means the repository exists. Any other success status is unexpected here and is + // reported as an error rather than being taken as existence. + if resp.StatusCode != http.StatusOK { return false, api.HandleHTTPError(resp) } + + return true, nil } func hasScript(httpClient *http.Client, repo ghrepo.Interface) (bool, error) { @@ -76,27 +76,22 @@ type release struct { } // downloadAsset downloads a single asset to the given file path. -func downloadAsset(httpClient *http.Client, assetURL safeurl.SafeURL, destPath string) (downloadErr error) { - var req *http.Request - if req, downloadErr = http.NewRequest("GET", assetURL.String(), nil); downloadErr != nil { - return - } - - req.Header.Set("Accept", "application/octet-stream") - +// +// assetURL is an absolute URL supplied by the API, so it is requested as given rather than +// being resolved against the host's REST endpoint. hostname is still needed so the request +// is made through a client configured for the right host. +func downloadAsset(httpClient *http.Client, hostname string, assetURL safeurl.SafeURL, destPath string) (downloadErr error) { var resp *http.Response // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its custom Accept header and binary response streaming. - if resp, downloadErr = httpClient.Do(req); downloadErr != nil { + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, downloadErr = api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, assetURL.String(), nil, + api.WithHeader("Accept", "application/octet-stream")) + if downloadErr != nil { return } defer resp.Body.Close() - if resp.StatusCode > 299 { - downloadErr = api.HandleHTTPError(resp) - return - } - var f *os.File if f, downloadErr = os.OpenFile(destPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0755); downloadErr != nil { return @@ -173,30 +168,25 @@ func fetchReleaseFromTag(httpClient *http.Client, baseRepo ghrepo.Interface, tag // fetchCommitSHA finds full commit SHA from a target ref in a repo func fetchCommitSHA(httpClient *http.Client, baseRepo ghrepo.Interface, targetRef string) (string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "commits", targetRef) - if err != nil { - return "", err - } - req, err := http.NewRequest("GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "commits", targetRef) if err != nil { return "", err } - req.Header.Set("Accept", "application/vnd.github.v3.sha") + // The response body is a bare SHA rather than JSON, so Request is used instead of REST. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to its custom Accept header and bare SHA response body. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", "application/vnd.github.v3.sha")) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusUnprocessableEntity { + return "", commitNotFoundErr + } return "", err } - defer resp.Body.Close() - if resp.StatusCode == 422 { - return "", commitNotFoundErr - } - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } body, err := io.ReadAll(resp.Body) if err != nil { diff --git a/pkg/cmd/extension/http_test.go b/pkg/cmd/extension/http_test.go index 7a262f78026..e1e9ceed0e2 100644 --- a/pkg/cmd/extension/http_test.go +++ b/pkg/cmd/extension/http_test.go @@ -3,10 +3,13 @@ package extension import ( "fmt" "net/http" + "os" + "path/filepath" "testing" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -230,3 +233,84 @@ func TestFetchReleaseFromTag(t *testing.T) { }) } } + +// TestFetchCommitSHA covers the two things that kept this site on the raw client: the custom +// Accept media type that makes the API answer with a bare SHA, and the 422 sentinel. +func TestFetchCommitSHA(t *testing.T) { + repo := ghrepo.New("OWNER", "REPO") + + t.Run("sends the sha media type and returns the bare body", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + func(req *http.Request) bool { + return req.URL.Path == "/repos/OWNER/REPO/commits/main" && + req.Header.Get("Accept") == "application/vnd.github.v3.sha" + }, + httpmock.StatusStringResponse(http.StatusOK, "0123456789abcdef"), + ) + + sha, err := fetchCommitSHA(&http.Client{Transport: reg}, repo, "main") + + require.NoError(t, err) + assert.Equal(t, "0123456789abcdef", sha) + }) + + t.Run("unprocessable entity means the commit was not found", func(t *testing.T) { + client := extensionHTTPClient(t, "repos/OWNER/REPO/commits/nope", http.StatusUnprocessableEntity, `{"message":"No commit found"}`) + + _, err := fetchCommitSHA(client, repo, "nope") + + require.ErrorIs(t, err, commitNotFoundErr) + }) + + t.Run("other errors are reported", func(t *testing.T) { + client := extensionHTTPClient(t, "repos/OWNER/REPO/commits/main", http.StatusInternalServerError, `{"message":"Internal Server Error"}`) + + _, err := fetchCommitSHA(client, repo, "main") + + requireExtensionHTTPError(t, err, http.StatusInternalServerError) + }) +} + +// TestDownloadAsset pins the octet-stream media type, without which the API returns asset +// metadata as JSON rather than the binary itself. +func TestDownloadAsset(t *testing.T) { + t.Run("sends the octet-stream media type and writes the body", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + func(req *http.Request) bool { + return req.URL.String() == "https://example.com/release/cool" && + req.Header.Get("Accept") == "application/octet-stream" + }, + httpmock.StatusStringResponse(http.StatusOK, "BINARY"), + ) + + destPath := filepath.Join(t.TempDir(), "gh-cool") + err := downloadAsset(&http.Client{Transport: reg}, "github.com", safeurl.NewImmutableSafeURL("https://example.com/release/cool"), destPath) + + require.NoError(t, err) + contents, err := os.ReadFile(destPath) + require.NoError(t, err) + assert.Equal(t, "BINARY", string(contents)) + }) + + t.Run("errors are reported", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST(http.MethodGet, "release/cool"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + destPath := filepath.Join(t.TempDir(), "gh-cool") + err := downloadAsset(&http.Client{Transport: reg}, "github.com", safeurl.NewImmutableSafeURL("https://example.com/release/cool"), destPath) + + requireExtensionHTTPError(t, err, http.StatusNotFound) + assert.NoFileExists(t, destPath) + }) +} diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 82b7fbfe0f8..112efad876b 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -351,7 +351,7 @@ func (m *Manager) installBin(repo ghrepo.Interface, target string) error { binPath := filepath.Join(targetDir, name) binPath += ext - err = downloadAsset(m.client, safeurl.NewImmutableSafeURL(asset.APIURL), binPath) + err = downloadAsset(m.client, repo.RepoHost(), safeurl.NewImmutableSafeURL(asset.APIURL), binPath) if err != nil { return fmt.Errorf("failed to download asset %s: %w", asset.Name, err) } diff --git a/pkg/cmd/extension/manager_test.go b/pkg/cmd/extension/manager_test.go index 567f3dba3ba..d09585a85a4 100644 --- a/pkg/cmd/extension/manager_test.go +++ b/pkg/cmd/extension/manager_test.go @@ -540,7 +540,7 @@ func TestManager_MigrateToBinaryExtension(t *testing.T) { Assets: []releaseAsset{ { Name: "gh-remote-windows-amd64.exe", - APIURL: "/release/cool", + APIURL: "https://example.com/release/cool", }, }, })) @@ -552,7 +552,7 @@ func TestManager_MigrateToBinaryExtension(t *testing.T) { Assets: []releaseAsset{ { Name: "gh-remote-windows-amd64.exe", - APIURL: "/release/cool", + APIURL: "https://example.com/release/cool", }, }, })) From 83702111c12f46a86cc83901848b35e5b4f577e7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:21:29 +0200 Subject: [PATCH 05/16] Route pkg/cmd/run through api.Client Four sites, three of which stream a response body the client must not decode. ListArtifacts had its own apiGet helper that duplicated RESTWithNext, including a copy of the Link header regex. It is deleted in favour of RESTWithNext, which already does exactly this and keeps the absolute next-page URLs working. The two log fetchers and the artifact download hand their response body to the caller, so they use Request. Their not-found sentinels move to errors.As, and both log fetchers keep their existing contract of accepting only 200. The artifact download URL comes from the API and stays absolute, so that site takes a hostname to identify the client rather than to build the URL. Test_Download constructed an apiPlatform with no repo, which is not a state the command can produce; it now sets one like the other tests in the file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/run/download/http.go | 23 +++++++------- pkg/cmd/run/download/http_test.go | 1 + pkg/cmd/run/shared/artifacts.go | 50 +++++-------------------------- pkg/cmd/run/view/logs.go | 23 +++++++------- pkg/cmd/run/view/view.go | 29 +++++++++--------- 5 files changed, 46 insertions(+), 80 deletions(-) diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index a832f924b20..ad15984c67c 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -25,27 +25,24 @@ func (p *apiPlatform) List(runID string) ([]shared.Artifact, error) { } func (p *apiPlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) error { - return downloadArtifact(p.client, url, dir) + return downloadArtifact(p.client, p.repo.RepoHost(), url, dir) } -func downloadArtifact(httpClient *http.Client, url safeurl.SafeURL, destDir safepaths.Absolute) error { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return err - } +func downloadArtifact(httpClient *http.Client, hostname string, url safeurl.SafeURL, destDir safepaths.Absolute) error { // The server rejects this :( - //req.Header.Set("Accept", "application/zip") - - resp, err := httpClient.Do(req) + //api.WithHeader("Accept", "application/zip") + // + // The artifact download URL is supplied by the API, so it is absolute and is requested as + // given rather than being resolved against the host's REST endpoint. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, url.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - tmpfile, err := os.CreateTemp("", "gh-artifact.*.zip") if err != nil { return fmt.Errorf("error initializing temporary file: %w", err) diff --git a/pkg/cmd/run/download/http_test.go b/pkg/cmd/run/download/http_test.go index 5b68dff82d4..33874388621 100644 --- a/pkg/cmd/run/download/http_test.go +++ b/pkg/cmd/run/download/http_test.go @@ -72,6 +72,7 @@ func Test_Download(t *testing.T) { api := &apiPlatform{ client: &http.Client{Transport: reg}, + repo: ghrepo.New("OWNER", "REPO"), } require.NoError(t, api.Download(safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/actions/artifacts/12345/zip"), destDir)) diff --git a/pkg/cmd/run/shared/artifacts.go b/pkg/cmd/run/shared/artifacts.go index 36d0b39e73c..064b62ee8b1 100644 --- a/pkg/cmd/run/shared/artifacts.go +++ b/pkg/cmd/run/shared/artifacts.go @@ -1,13 +1,10 @@ package shared import ( - "encoding/json" "net/http" - "regexp" "strconv" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -26,14 +23,13 @@ type artifactsPayload struct { func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) ([]Artifact, error) { var results []Artifact - restPrefix := ghinstance.RESTPrefix(repo.RepoHost()) perPage := 100 - u, err := safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") if err != nil { return nil, err } if runID != "" { - u, err = safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") + u, err = safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") if err != nil { return nil, err } @@ -41,9 +37,14 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) u.SetQuery("per_page", strconv.Itoa(perPage)) var pageURL safeurl.SafeURL = u + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + client := api.NewClientFromHTTP(httpClient) + for { var payload artifactsPayload - nextURL, err := apiGet(httpClient, pageURL, &payload) + nextURL, err := client.RESTWithNext(repo.RepoHost(), http.MethodGet, pageURL.String(), nil, &payload) if err != nil { return nil, err } @@ -57,38 +58,3 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) return results, nil } - -func apiGet(httpClient *http.Client, url safeurl.SafeURL, data interface{}) (string, error) { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return "", err - } - - resp, err := httpClient.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(data); err != nil { - return "", err - } - - return findNextPage(resp), nil -} - -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(resp *http.Response) string { - for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { - if len(m) > 2 && m[2] == "next" { - return m[1] - } - } - return "" -} diff --git a/pkg/cmd/run/view/logs.go b/pkg/cmd/run/view/logs.go index ab1232837bb..113dc871526 100644 --- a/pkg/cmd/run/view/logs.go +++ b/pkg/cmd/run/view/logs.go @@ -14,7 +14,6 @@ import ( "unicode/utf16" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/run/shared" @@ -40,24 +39,26 @@ type apiLogFetcher struct { } func (f *apiLogFetcher) GetLog() (io.ReadCloser, error) { - logURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(f.repo.RepoHost()), "repos", f.repo.RepoOwner(), f.repo.RepoName(), "actions", "jobs", strconv.FormatInt(f.jobID, 10), "logs") + logPath, err := safeurl.JoinPath("repos", f.repo.RepoOwner(), f.repo.RepoName(), "actions", "jobs", strconv.FormatInt(f.jobID, 10), "logs") if err != nil { return nil, err } - req, err := http.NewRequest("GET", logURL.String(), nil) - if err != nil { - return nil, err - } - - resp, err := f.httpClient.Do(req) + // The response body is the log stream, which is handed to the caller rather than decoded. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(f.httpClient).Request(f.repo.RepoHost(), http.MethodGet, logPath.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("log not found: %v", f.jobID) + } return nil, err } - if resp.StatusCode == 404 { - return nil, fmt.Errorf("log not found: %v", f.jobID) - } else if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 95cdc891291..f913ce86015 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -16,7 +16,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -470,20 +469,22 @@ func shouldFetchJobs(opts *ViewOptions) bool { return false } -func getLog(httpClient *http.Client, logURL safeurl.SafeURL) (io.ReadCloser, error) { - req, err := http.NewRequest("GET", logURL.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) +func getLog(httpClient *http.Client, hostname string, logPath safeurl.SafeURL) (io.ReadCloser, error) { + // The response body is the log archive, which is handed to the caller rather than decoded. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, logPath.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return nil, errors.New("log not found") + } return nil, err } - if resp.StatusCode == 404 { - return nil, errors.New("log not found") - } else if resp.StatusCode != 200 { + if resp.StatusCode != http.StatusOK { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } @@ -499,19 +500,19 @@ func getRunLog(cache RunLogCache, httpClient *http.Client, repo ghrepo.Interface if !isCached { // Run log does not exist in cache so retrieve and store it - logURL, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "logs") + logPath, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "logs") if err != nil { return nil, err } if attempt > 0 { - logURL, err = safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "attempts", strconv.FormatUint(attempt, 10), "logs") + logPath, err = safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", strconv.FormatInt(run.ID, 10), "attempts", strconv.FormatUint(attempt, 10), "logs") if err != nil { return nil, err } } - resp, err := getLog(httpClient, logURL) + resp, err := getLog(httpClient, repo.RepoHost(), logPath) if err != nil { return nil, err } From 388a35c58739a73d42d3ac2fdd9822e2d4b80c06 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:25:41 +0200 Subject: [PATCH 06/16] Route remaining release sites through api.Client Eight sites across delete, delete-asset, edit, create, download and upload. The three deletes and the create existence check are body-blind or have no body at all, so they use Request rather than REST, which would try to decode one. editRelease also uses Request: REST returns early on a 204 without decoding, which would turn today's error into a zero valued release. uploadAsset is the one site that needs DoRequest. It sets ContentLength and GetBody on the request, neither of which is a header, and without them a file body would be sent chunked. Its retry behaviour is preserved by wrapping only transport failures as errNetwork and leaving a response-carrying failure to be judged by status, which Test_uploadWithDelete_retry covers. The archive download overrides CheckRedirect to avoid legacy Codeload resources. That is client level rather than request level, so it keeps copying the client and hands the copy to NewClientFromHTTP. No new API is needed. httpDoer is gone. It existed only because these sites took a narrow interface, and api.Client needs a concrete *http.Client. The upload retry test drives the client through a RoundTripper instead. Asset, release and upload URLs all come from the API and stay absolute. Sites that build their own URL now build a relative path instead. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/release/create/create.go | 2 +- pkg/cmd/release/create/http.go | 26 +++++----- pkg/cmd/release/delete-asset/delete_asset.go | 18 +++---- pkg/cmd/release/delete/delete.go | 33 +++++-------- pkg/cmd/release/download/download.go | 32 ++++++------ pkg/cmd/release/edit/http.go | 22 +++------ pkg/cmd/release/shared/upload.go | 51 +++++++++----------- pkg/cmd/release/shared/upload_test.go | 12 +++-- pkg/cmd/release/upload/upload.go | 2 +- 9 files changed, 88 insertions(+), 110 deletions(-) diff --git a/pkg/cmd/release/create/create.go b/pkg/cmd/release/create/create.go index 79d92bdf153..56e69a4158a 100644 --- a/pkg/cmd/release/create/create.go +++ b/pkg/cmd/release/create/create.go @@ -548,7 +548,7 @@ func createRun(opts *CreateOptions) error { } opts.IO.StartProgressIndicator() - err = shared.ConcurrentUpload(httpClient, safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) + err = shared.ConcurrentUpload(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { return cleanupDraftRelease(err) diff --git a/pkg/cmd/release/create/http.go b/pkg/cmd/release/create/http.go index a2dc4372cda..ff07a9b93e6 100644 --- a/pkg/cmd/release/create/http.go +++ b/pkg/cmd/release/create/http.go @@ -11,7 +11,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -110,32 +109,33 @@ func generateReleaseNotes(httpClient *http.Client, repo ghrepo.Interface, tagNam } func publishedReleaseExists(httpClient *http.Client, repo ghrepo.Interface, tagName string) (bool, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) - if err != nil { - return false, err - } - req, err := http.NewRequest("HEAD", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", "tags", tagName) if err != nil { return false, err } + // A HEAD response has no body, so Request is used rather than REST, which would try to + // decode one. // TODO(api-client-rollout) - // This has been deferred from moving to api.Client due to HEAD responses having no body while REST decodes non-204/205 2xx responses as JSON. - resp, err := httpClient.Do(req) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodHead, path.String(), nil) if err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } if resp.Body != nil { defer resp.Body.Close() } - if resp.StatusCode == 200 { - return true, nil - } else if resp.StatusCode == 404 { - return false, nil - } else { + if resp.StatusCode != http.StatusOK { return false, api.HandleHTTPError(resp) } + + return true, nil } func createRelease(httpClient *http.Client, repo ghrepo.Interface, params map[string]interface{}) (*shared.Release, error) { diff --git a/pkg/cmd/release/delete-asset/delete_asset.go b/pkg/cmd/release/delete-asset/delete_asset.go index 3aedc3e3a46..7528741c152 100644 --- a/pkg/cmd/release/delete-asset/delete_asset.go +++ b/pkg/cmd/release/delete-asset/delete_asset.go @@ -97,7 +97,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return fmt.Errorf("asset %s not found in release %s", opts.AssetName, release.TagName) } - err = deleteAsset(httpClient, safeurl.NewImmutableSafeURL(assetURL)) + err = deleteAsset(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(assetURL)) if err != nil { return err } @@ -112,20 +112,16 @@ func deleteAssetRun(opts *DeleteAssetOptions) error { return nil } -func deleteAsset(httpClient *http.Client, assetURL safeurl.SafeURL) error { - req, err := http.NewRequest("DELETE", assetURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteAsset(httpClient *http.Client, hostname string, assetURL safeurl.SafeURL) error { + // The asset URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodDelete, assetURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index 108ebae7ece..264d1d677bc 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -7,7 +7,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -93,7 +92,7 @@ func deleteRun(opts *DeleteOptions) error { } } - err = deleteRelease(httpClient, safeurl.NewImmutableSafeURL(release.APIURL)) + err = deleteRelease(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(release.APIURL)) if err != nil { return err } @@ -122,42 +121,34 @@ func deleteRun(opts *DeleteOptions) error { return nil } -func deleteRelease(httpClient *http.Client, releaseURL safeurl.SafeURL) error { - req, err := http.NewRequest("DELETE", releaseURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteRelease(httpClient *http.Client, hostname string, releaseURL safeurl.SafeURL) error { + // The release URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodDelete, releaseURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } func deleteTag(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) - if err != nil { - return err - } - req, err := http.NewRequest("DELETE", url.String(), nil) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) if err != nil { return err } - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodDelete, path.String(), nil) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } return nil } diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index 688d94c1472..66932cf46fe 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -242,7 +242,7 @@ func downloadRun(opts *DownloadOptions) error { } } - return downloadAssets(&dest, httpClient, targets, opts.Concurrency, isArchive, opts.IO) + return downloadAssets(&dest, httpClient, baseRepo.RepoHost(), targets, opts.Concurrency, isArchive, opts.IO) } func matchAny(patterns []string, name string) bool { @@ -259,7 +259,7 @@ type downloadTarget struct { name string } -func downloadAssets(dest *destinationWriter, httpClient *http.Client, toDownload []downloadTarget, numWorkers int, isArchive bool, io *iostreams.IOStreams) error { +func downloadAssets(dest *destinationWriter, httpClient *http.Client, hostname string, toDownload []downloadTarget, numWorkers int, isArchive bool, io *iostreams.IOStreams) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } @@ -275,7 +275,7 @@ func downloadAssets(dest *destinationWriter, httpClient *http.Client, toDownload go func() { for a := range jobs { io.StartProgressIndicatorWithLabel(fmt.Sprintf("Downloading %s", a.name)) - results <- downloadAsset(dest, httpClient, a.url, a.name, isArchive) + results <- downloadAsset(dest, httpClient, hostname, a.url, a.name, isArchive) } }() } @@ -297,22 +297,19 @@ func downloadAssets(dest *destinationWriter, httpClient *http.Client, toDownload return downloadError } -func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL safeurl.SafeURL, fileName string, isArchive bool) error { +func downloadAsset(dest *destinationWriter, httpClient *http.Client, hostname string, assetURL safeurl.SafeURL, fileName string, isArchive bool) error { if err := dest.Check(fileName); err != nil { return err } - req, err := http.NewRequest("GET", assetURL.String(), nil) - if err != nil { - return err - } - - req.Header.Set("Accept", "application/octet-stream") + accept := "application/octet-stream" if isArchive { // adding application/json to Accept header due to a bug in the zipball/tarball API endpoint that makes it mandatory - req.Header.Set("Accept", "application/octet-stream, application/json") + accept = "application/octet-stream, application/json" - // override HTTP redirect logic to avoid "legacy" Codeload resources + // override HTTP redirect logic to avoid "legacy" Codeload resources. This is client + // level rather than request level, so the client is copied before being handed over + // rather than mutating the one shared with every other caller. oldClient := *httpClient httpClient = &oldClient httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { @@ -323,16 +320,17 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL sa } } - resp, err := httpClient.Do(req) + // The asset URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, assetURL.String(), nil, + api.WithHeader("Accept", accept)) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - if len(fileName) == 0 { contentDisposition := resp.Header.Get("Content-Disposition") diff --git a/pkg/cmd/release/edit/http.go b/pkg/cmd/release/edit/http.go index 291123ad3ea..55f3100fbad 100644 --- a/pkg/cmd/release/edit/http.go +++ b/pkg/cmd/release/edit/http.go @@ -9,7 +9,6 @@ import ( "strconv" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -22,28 +21,23 @@ func editRelease(httpClient *http.Client, repo ghrepo.Interface, releaseID int64 return nil, err } - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(releaseID, 10)) - if err != nil { - return nil, err - } - req, err := http.NewRequest("PATCH", url.String(), bytes.NewBuffer(bodyBytes)) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "releases", strconv.FormatInt(releaseID, 10)) if err != nil { return nil, err } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - - resp, err := httpClient.Do(req) + // Request rather than REST because REST returns without decoding on a 204, which would + // yield a zero valued release instead of the error this site reports today. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(repo.RepoHost(), http.MethodPatch, path.String(), bytes.NewBuffer(bodyBytes), + api.WithHeader("Content-Type", "application/json; charset=utf-8")) if err != nil { return nil, err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, api.HandleHTTPError(resp) - } - b, err := io.ReadAll(resp.Body) if err != nil { return nil, err diff --git a/pkg/cmd/release/shared/upload.go b/pkg/cmd/release/shared/upload.go index 9307c8c01c2..1f2f0076580 100644 --- a/pkg/cmd/release/shared/upload.go +++ b/pkg/cmd/release/shared/upload.go @@ -20,10 +20,6 @@ import ( "golang.org/x/sync/errgroup" ) -type httpDoer interface { - Do(*http.Request) (*http.Response, error) -} - type errNetwork struct{ error } type AssetForUpload struct { @@ -112,7 +108,7 @@ func fileExt(fn string) string { return path.Ext(fn) } -func ConcurrentUpload(httpClient httpDoer, uploadURL safeurl.SafeURL, numWorkers int, assets []*AssetForUpload) error { +func ConcurrentUpload(httpClient *http.Client, hostname string, uploadURL safeurl.SafeURL, numWorkers int, assets []*AssetForUpload) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } @@ -124,7 +120,7 @@ func ConcurrentUpload(httpClient httpDoer, uploadURL safeurl.SafeURL, numWorkers for _, a := range assets { asset := *a g.Go(func() error { - return uploadWithDelete(gctx, httpClient, uploadURL, asset) + return uploadWithDelete(gctx, httpClient, hostname, uploadURL, asset) }) } @@ -143,9 +139,9 @@ func shouldRetry(err error) bool { // Allow injecting backoff interval in tests. var retryInterval = time.Millisecond * 200 -func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL safeurl.SafeURL, a AssetForUpload) error { +func uploadWithDelete(ctx context.Context, httpClient *http.Client, hostname string, uploadURL safeurl.SafeURL, a AssetForUpload) error { if a.ExistingURL != nil && a.ExistingURL.String() != "" { - if err := deleteAsset(ctx, httpClient, a.ExistingURL); err != nil { + if err := deleteAsset(ctx, httpClient, hostname, a.ExistingURL); err != nil { return err } } @@ -159,7 +155,7 @@ func uploadWithDelete(ctx context.Context, httpClient httpDoer, uploadURL safeur }, backoff.WithContext(backoff.WithMaxRetries(bo, 3), ctx)) } -func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.SafeURL, asset AssetForUpload) (*ReleaseAsset, error) { +func uploadAsset(ctx context.Context, httpClient *http.Client, uploadURL safeurl.SafeURL, asset AssetForUpload) (*ReleaseAsset, error) { u, err := url.Parse(uploadURL.String()) if err != nil { return nil, err @@ -186,17 +182,24 @@ func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.Saf req.Header.Set("Content-Type", asset.MIMEType) req.GetBody = asset.Open - resp, err := httpClient.Do(req) + // DoRequest rather than Request because ContentLength and GetBody are set on the request + // itself, and neither can be expressed as a header. The upload URL is supplied by the API + // and points at the uploads host, so there is no endpoint resolution to delegate here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).DoRequest(req) if err != nil { + // Only transport failures are retryable as network errors. A response that arrived and + // carried a failing status is reported as is, so shouldRetry can judge it by status. + var httpErr api.HTTPError + if errors.As(err, &httpErr) { + return nil, err + } return nil, errNetwork{err} } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, api.HandleHTTPError(resp) - } - var newAsset ReleaseAsset dec := json.NewDecoder(resp.Body) if err := dec.Decode(&newAsset); err != nil { @@ -206,22 +209,16 @@ func uploadAsset(ctx context.Context, httpClient httpDoer, uploadURL safeurl.Saf return &newAsset, nil } -func deleteAsset(ctx context.Context, httpClient httpDoer, assetURL safeurl.SafeURL) error { - req, err := http.NewRequestWithContext(ctx, "DELETE", assetURL.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) +func deleteAsset(ctx context.Context, httpClient *http.Client, hostname string, assetURL safeurl.SafeURL) error { + // The asset URL is supplied by the API, so it is absolute and requested as given. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, hostname, http.MethodDelete, assetURL.String(), nil) if err != nil { return err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return api.HandleHTTPError(resp) - } - return nil } diff --git a/pkg/cmd/release/shared/upload_test.go b/pkg/cmd/release/shared/upload_test.go index 8271fa6ae6e..9038abb21f9 100644 --- a/pkg/cmd/release/shared/upload_test.go +++ b/pkg/cmd/release/shared/upload_test.go @@ -82,7 +82,7 @@ func Test_uploadWithDelete_retry(t *testing.T) { ctx := context.Background() tries := 0 - client := funcClient(func(req *http.Request) (*http.Response, error) { + client := &http.Client{Transport: funcTripper(func(req *http.Request) (*http.Response, error) { tries++ if tries == 1 { return nil, errors.New("made up exception") @@ -98,8 +98,8 @@ func Test_uploadWithDelete_retry(t *testing.T) { StatusCode: 200, Body: io.NopCloser(bytes.NewBufferString(`{}`)), }, nil - }) - err := uploadWithDelete(ctx, client, safeurl.NewImmutableSafeURL("http://example.com/upload"), AssetForUpload{ + })} + err := uploadWithDelete(ctx, client, "github.com", safeurl.NewImmutableSafeURL("http://example.com/upload"), AssetForUpload{ Name: "asset", Label: "", Size: 8, @@ -116,8 +116,10 @@ func Test_uploadWithDelete_retry(t *testing.T) { } } -type funcClient func(*http.Request) (*http.Response, error) +// funcTripper drives uploadWithDelete from a plain function. It is a RoundTripper rather than +// a Do-er because the upload path takes a concrete *http.Client, which is what api.Client needs. +type funcTripper func(*http.Request) (*http.Response, error) -func (f funcClient) Do(req *http.Request) (*http.Response, error) { +func (f funcTripper) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } diff --git a/pkg/cmd/release/upload/upload.go b/pkg/cmd/release/upload/upload.go index 35b10860d75..84345adddb2 100644 --- a/pkg/cmd/release/upload/upload.go +++ b/pkg/cmd/release/upload/upload.go @@ -113,7 +113,7 @@ func uploadRun(opts *UploadOptions) error { } opts.IO.StartProgressIndicator() - err = shared.ConcurrentUpload(httpClient, safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) + err = shared.ConcurrentUpload(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(uploadURL), opts.Concurrency, opts.Assets) opts.IO.StopProgressIndicator() if err != nil { return err From acf78946ba516f8e560aae3478af0ed0f85b8bd7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:29:15 +0200 Subject: [PATCH 07/16] Route pkg/cmd/repo through api.Client Migrates the four remaining raw httpClient.Do call sites under pkg/cmd/repo: delete, garden, and the two topics calls in edit. Constructed URLs become relative paths so they will pick up api_host resolution from go-gh. Two behaviour notes: garden previously returned a bare errors.New("api call failed") on any non-2xx. It now returns api.HTTPError, so users see the API's own message. This is a user-visible message change and is pinned by a new test. Its explicit Content-Type: application/json; charset=utf-8 is dropped because the client already sends exactly that header; verified against the real transport rather than assumed. gh repo edit's Content-Type differs (no charset) so it stays. delete keeps its CheckRedirect client copy and declares the delete_repo scope through api.WithEndpointScopes. That suggestion had no coverage, so a test now pins it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/repo/delete/http.go | 19 ++++------- pkg/cmd/repo/delete/http_test.go | 51 +++++++++++++++++++++++++++++ pkg/cmd/repo/edit/edit.go | 29 ++++++++--------- pkg/cmd/repo/garden/http.go | 27 ++++++---------- pkg/cmd/repo/garden/http_test.go | 55 ++++++++++++++++++++++++++++++++ 5 files changed, 135 insertions(+), 46 deletions(-) create mode 100644 pkg/cmd/repo/delete/http_test.go create mode 100644 pkg/cmd/repo/garden/http_test.go diff --git a/pkg/cmd/repo/delete/http.go b/pkg/cmd/repo/delete/http.go index faffa006e3d..7a7c937db6b 100644 --- a/pkg/cmd/repo/delete/http.go +++ b/pkg/cmd/repo/delete/http.go @@ -4,7 +4,6 @@ import ( "net/http" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -16,26 +15,20 @@ func deleteRepo(client *http.Client, repo ghrepo.Interface) error { return http.ErrUseLastResponse } - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return err } - request, err := http.NewRequest("DELETE", url.String(), nil) - if err != nil { - return err - } - - resp, err := client.Do(request) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).Request(repo.RepoHost(), http.MethodDelete, path.String(), nil, + api.WithEndpointScopes("delete_repo")) if err != nil { return err } defer resp.Body.Close() - if resp.StatusCode > 299 { - api.EndpointNeedsScopes(resp, "delete_repo") - return api.HandleHTTPError(resp) - } - return nil } diff --git a/pkg/cmd/repo/delete/http_test.go b/pkg/cmd/repo/delete/http_test.go new file mode 100644 index 00000000000..5e3e07cd4eb --- /dev/null +++ b/pkg/cmd/repo/delete/http_test.go @@ -0,0 +1,51 @@ +package delete + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestDeleteRepo(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + httpmock.StatusStringResponse(204, ""), + ) + + require.NoError(t, deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"))) +} + +// TestDeleteRepoMissingScope pins the scopes suggestion, which is the reason this call site +// needs the delete_repo scope declared rather than relying on what the API reports. +func TestDeleteRepoMissingScope(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusJSONResponse(403, map[string]string{"message": "Must have admin rights to Repository."})(req) + if err != nil { + return nil, err + } + resp.Header.Set("X-Oauth-Scopes", "repo") + resp.Header.Set("X-Accepted-Oauth-Scopes", "") + return resp, nil + }, + ) + + err := deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO")) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "delete_repo") +} diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index c215f182ccc..ac9a443e6fd 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -14,7 +14,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" fd "github.com/cli/cli/v2/internal/featuredetection" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -567,18 +566,17 @@ func parseTopics(s string) []string { } func getTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interface) ([]string, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "topics") - if err != nil { - return nil, err - } - req, err := http.NewRequestWithContext(ctx, "GET", url.String(), nil) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "topics") if err != nil { return nil, err } // "mercy-preview" is still needed for some GitHub Enterprise versions - req.Header.Set("Accept", "application/vnd.github.mercy-preview+json") - res, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", "application/vnd.github.mercy-preview+json")) if err != nil { return nil, err } @@ -608,19 +606,18 @@ func setTopics(ctx context.Context, httpClient *http.Client, repo ghrepo.Interfa return err } - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "topics") - if err != nil { - return err - } - req, err := http.NewRequestWithContext(ctx, "PUT", url.String(), body) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "topics") if err != nil { return err } - req.Header.Set("Content-type", "application/json") // "mercy-preview" is still needed for some GitHub Enterprise versions - req.Header.Set("Accept", "application/vnd.github.mercy-preview+json") - res, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).RequestWithContext(ctx, repo.RepoHost(), http.MethodPut, path.String(), body, + api.WithHeader("Content-Type", "application/json"), + api.WithHeader("Accept", "application/vnd.github.mercy-preview+json")) if err != nil { return err } diff --git a/pkg/cmd/repo/garden/http.go b/pkg/cmd/repo/garden/http.go index 903de787632..9aeeb72c740 100644 --- a/pkg/cmd/repo/garden/http.go +++ b/pkg/cmd/repo/garden/http.go @@ -2,14 +2,13 @@ package garden import ( "encoding/json" - "errors" "io" "net/http" "strconv" "strings" "time" - "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -27,7 +26,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* commits := []*Commit{} pathF := func(page int) (*safeurl.MutableSafeURL, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "commits") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "commits") if err != nil { return nil, err } @@ -47,7 +46,7 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* if err != nil { return nil, err } - links, err := getResponse(client, path, &result) + links, err := getResponse(client, repo.RepoHost(), path, &result) if err != nil { return nil, err } @@ -80,24 +79,18 @@ func getCommits(client *http.Client, repo ghrepo.Interface, maxCommits int) ([]* // getResponse performs the API call and returns the response's link header values. // If the "Link" header is missing, the returned slice will be nil. -func getResponse(client *http.Client, url safeurl.SafeURL, data interface{}) ([]string, error) { - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return nil, err - } - - req.Header.Set("Content-Type", "application/json; charset=utf-8") - resp, err := client.Do(req) +func getResponse(client *http.Client, hostname string, path safeurl.SafeURL, data interface{}) ([]string, error) { + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).Request(hostname, http.MethodGet, path.String(), nil) if err != nil { return nil, err } defer resp.Body.Close() - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return nil, errors.New("api call failed") - } - links := resp.Header["Link"] if resp.StatusCode == http.StatusNoContent { diff --git a/pkg/cmd/repo/garden/http_test.go b/pkg/cmd/repo/garden/http_test.go new file mode 100644 index 00000000000..44b4e42d5cf --- /dev/null +++ b/pkg/cmd/repo/garden/http_test.go @@ -0,0 +1,55 @@ +package garden + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetCommits(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/commits"), + httpmock.JSONResponse([]map[string]interface{}{ + {"sha": "abc123def456", "author": map[string]string{"login": "monalisa"}}, + {"sha": "def456abc123", "author": map[string]string{"login": ""}}, + }), + ) + + commits, err := getCommits(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"), 10) + require.NoError(t, err) + + require.Len(t, commits, 2) + // commits are reversed so that older ones come first + assert.Equal(t, "def456abc123", commits[0].Sha) + assert.Equal(t, "a mysterious stranger", commits[0].Handle) + assert.Equal(t, "abc123def456", commits[1].Sha) + assert.Equal(t, "monalisa", commits[1].Handle) +} + +// TestGetCommitsHTTPError pins the error returned on a non-2xx response. This site used to +// return a bare "api call failed", and now surfaces the API's own message via api.HTTPError. +func TestGetCommitsHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/commits"), + httpmock.StatusJSONResponse(404, map[string]string{"message": "Not Found"}), + ) + + _, err := getCommits(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"), 10) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 404, httpErr.StatusCode) + assert.Equal(t, "Not Found", httpErr.Message) +} From 9ecdf860690211cacd54d429eef5c65fe421be2d Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:31:39 +0200 Subject: [PATCH 08/16] Route gist and pr diff through api.Client Migrates the three remaining raw httpClient.Do call sites in pkg/cmd/gist and pkg/cmd/pr/diff. gist create and pr diff construct their URLs, so those become relative paths and will pick up api_host resolution from go-gh. gist create declares its scope through api.WithEndpointScopes, and its explicit Content-Type is dropped because the client already sends exactly that header. The scope suggestion had no coverage, so a test now pins it. GetRawGistFile keeps its absolute URL: raw gists are served from gist.githubusercontent.com rather than the API host, so rewriting them to a gateway would be wrong. It gains a hostname parameter purely to configure the client, which both callers already have to hand. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/gist/create/create.go | 21 ++++++++------------ pkg/cmd/gist/create/create_test.go | 31 ++++++++++++++++++++++++++++++ pkg/cmd/gist/edit/edit.go | 2 +- pkg/cmd/gist/shared/shared.go | 14 +++++++------- pkg/cmd/gist/shared/shared_test.go | 2 +- pkg/cmd/gist/view/view.go | 2 +- pkg/cmd/pr/diff/diff.go | 17 +++++++--------- 7 files changed, 56 insertions(+), 33 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 6ed4f66263d..76f918bee93 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -17,7 +17,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmd/gist/shared" @@ -273,27 +272,23 @@ func createGist(client *http.Client, hostname, description string, public bool, return nil, err } - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "gists") + path, err := safeurl.JoinPath("gists") if err != nil { return nil, err } - req, err := http.NewRequest(http.MethodPost, u.String(), requestBody) - if err != nil { - return nil, err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - resp, err := client.Do(req) + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(client).Request(hostname, http.MethodPost, path.String(), requestBody, + api.WithEndpointScopes("gist")) if err != nil { return nil, err } defer resp.Body.Close() - if resp.StatusCode > 299 { - api.EndpointNeedsScopes(resp, "gist") - return nil, api.HandleHTTPError(resp) - } - result := &shared.Gist{} dec := json.NewDecoder(resp.Body) if err := dec.Decode(result); err != nil { diff --git a/pkg/cmd/gist/create/create_test.go b/pkg/cmd/gist/create/create_test.go index 44f0ba284ef..0885082926b 100644 --- a/pkg/cmd/gist/create/create_test.go +++ b/pkg/cmd/gist/create/create_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" @@ -21,6 +22,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_processFiles(t *testing.T) { @@ -415,3 +417,32 @@ func Test_detectEmptyFiles(t *testing.T) { assert.Equal(t, tt.isEmptyFile, isEmptyFile) } } + +// Test_createGist_missingScope pins the scopes suggestion, which is the reason this call site +// declares the gist scope rather than relying on what the API reports. +func Test_createGist_missingScope(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("POST", "gists"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusJSONResponse(404, map[string]string{"message": "Not Found"})(req) + if err != nil { + return nil, err + } + resp.Header.Set("X-Oauth-Scopes", "repo") + resp.Header.Set("X-Accepted-Oauth-Scopes", "") + return resp, nil + }, + ) + + _, err := createGist(&http.Client{Transport: reg}, "github.com", "", true, map[string]*shared.GistFile{ + "file.txt": {Filename: "file.txt", Content: "hello"}, + }) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Contains(t, httpErr.ScopesSuggestion(), "gist") +} diff --git a/pkg/cmd/gist/edit/edit.go b/pkg/cmd/gist/edit/edit.go index 8195c6aa0f9..36ea2b0e301 100644 --- a/pkg/cmd/gist/edit/edit.go +++ b/pkg/cmd/gist/edit/edit.go @@ -288,7 +288,7 @@ func editRun(opts *EditOptions) error { file := gist.Files[filename] if file.Truncated { if _, alreadyEdited := filesToUpdate[filename]; !alreadyEdited { - fullContent, err := shared.GetRawGistFile(client, safeurl.NewImmutableSafeURL(file.RawURL)) + fullContent, err := shared.GetRawGistFile(client, host, safeurl.NewImmutableSafeURL(file.RawURL)) if err != nil { return err } diff --git a/pkg/cmd/gist/shared/shared.go b/pkg/cmd/gist/shared/shared.go index 7c0a7c07565..ee51a902294 100644 --- a/pkg/cmd/gist/shared/shared.go +++ b/pkg/cmd/gist/shared/shared.go @@ -255,13 +255,13 @@ func PromptGists(prompter prompter.Prompter, client *http.Client, host string, c // GetRawGistFile fetches the full content of a gist file from its raw URL. The // bytes are external content, so they are returned as iostreams.Untrusted to // force callers to choose between sanitized display and raw round-tripping. -func GetRawGistFile(httpClient *http.Client, rawURL safeurl.SafeURL) (iostreams.Untrusted, error) { - req, err := http.NewRequest("GET", rawURL.String(), nil) - if err != nil { - return iostreams.Untrusted{}, err - } - - resp, err := httpClient.Do(req) +// The rawURL is supplied by the API and points at gist.githubusercontent.com rather than the +// API host, so it is requested as an absolute URL. The hostname only configures the client. +func GetRawGistFile(httpClient *http.Client, hostname string, rawURL safeurl.SafeURL) (iostreams.Untrusted, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, rawURL.String(), nil) if err != nil { return iostreams.Untrusted{}, err } diff --git a/pkg/cmd/gist/shared/shared_test.go b/pkg/cmd/gist/shared/shared_test.go index 8b891d8b9fe..1f24a9011e5 100644 --- a/pkg/cmd/gist/shared/shared_test.go +++ b/pkg/cmd/gist/shared/shared_test.go @@ -299,7 +299,7 @@ func TestGetRawGistFile(t *testing.T) { ) client := &http.Client{Transport: reg} - result, err := GetRawGistFile(client, safeurl.NewImmutableSafeURL("https://gist.githubusercontent.com/raw-url")) + result, err := GetRawGistFile(client, "github.com", safeurl.NewImmutableSafeURL("https://gist.githubusercontent.com/raw-url")) if tt.wantErr { assert.Error(t, err) diff --git a/pkg/cmd/gist/view/view.go b/pkg/cmd/gist/view/view.go index 51ec38d0443..36822e45985 100644 --- a/pkg/cmd/gist/view/view.go +++ b/pkg/cmd/gist/view/view.go @@ -150,7 +150,7 @@ func viewRun(opts *ViewOptions) error { // path fetches the full content from the raw URL. content := iostreams.NewUntrusted(gf.Content) if gf.Truncated { - fullContent, err := shared.GetRawGistFile(client, safeurl.NewImmutableSafeURL(gf.RawURL)) + fullContent, err := shared.GetRawGistFile(client, hostname, safeurl.NewImmutableSafeURL(gf.RawURL)) if err != nil { return err } diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go index 3e627b8b806..0a08b6cad53 100644 --- a/pkg/cmd/pr/diff/diff.go +++ b/pkg/cmd/pr/diff/diff.go @@ -15,7 +15,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/internal/text" @@ -211,7 +210,7 @@ func diffRun(opts *DiffOptions) error { } func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int, asPatch bool) (io.ReadCloser, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "pulls", strconv.Itoa(prNumber)) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "pulls", strconv.Itoa(prNumber)) if err != nil { return nil, err } @@ -220,18 +219,16 @@ func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int, acceptType = "application/vnd.github.v3.patch" } - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return nil, err - } - - req.Header.Set("Accept", acceptType) - - resp, err := httpClient.Do(req) + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(httpClient).Request(baseRepo.RepoHost(), http.MethodGet, path.String(), nil, + api.WithHeader("Accept", acceptType)) if err != nil { return nil, err } if resp.StatusCode != 200 { + defer resp.Body.Close() return nil, api.HandleHTTPError(resp) } From 469d4ad413c7fa9fad742e99bd7500aca2aa321e Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 13:35:35 +0200 Subject: [PATCH 09/16] Route auth, search and update through api.Client Migrates the last four in-scope raw httpClient.Do call sites. auth's GetScopes and GetCurrentLogin run before the token is stored in config, so they pass it explicitly. The transport only sets Authorization when it is absent, so the header set at the call site wins and no new concept is needed. GetCurrentLogin's hand-rolled GraphQL POST becomes an api.Client GraphQL call. Both functions took a narrow one-method httpClient interface, which is dropped in favour of *http.Client so the api client can be constructed from it; the existing tests already passed a real client, so they are unchanged. search keeps its own error type, because it formats invalid queries differently from the rest of the CLI. api.HTTPError carries the same fields, so the generic error is translated back rather than surfaced directly, including the status line fallback for non-JSON responses. update keeps its absolute URL deliberately: CLI releases live on github.com regardless of the host the user has configured, so it must not be rewritten to their API host. This completes the rollout. The census command from the design now returns only the ten genuinely out-of-scope sites plus DoRequest's own implementation, and no "deferred from moving to api.Client" comments remain. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- internal/update/update.go | 12 +++--- pkg/cmd/auth/shared/login_flow.go | 47 ++++++-------------- pkg/cmd/auth/shared/oauth_scopes.go | 32 +++++--------- pkg/search/searcher.go | 66 ++++++++++++++++------------- 4 files changed, 68 insertions(+), 89 deletions(-) diff --git a/internal/update/update.go b/internal/update/update.go index 27a7d8a248f..d0af16a632f 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -13,6 +13,7 @@ import ( "strings" "time" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ci" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/extensions" @@ -121,11 +122,12 @@ func getLatestReleaseInfo(ctx context.Context, client *http.Client, repo string) if err != nil { return nil, err } - req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) - if err != nil { - return nil, err - } - res, err := client.Do(req) + // The URL stays absolute on purpose: CLI releases live on github.com regardless of the + // host the user has configured, so this must not be rewritten to their API host. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(client).RequestWithContext(ctx, "github.com", http.MethodGet, u.String(), nil) if err != nil { return nil, err } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index c76dc5fb84c..a43f919b050 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -1,8 +1,6 @@ package shared import ( - "bytes" - "encoding/json" "fmt" "net/http" "os" @@ -13,8 +11,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/authflow" "github.com/cli/cli/v2/internal/browser" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/ssh-key/add" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/ssh" @@ -250,36 +246,21 @@ func sshKeyUpload(httpClient *http.Client, hostname, keyFile string, title strin return add.SSHKeyUpload(httpClient, hostname, f, title) } -func GetCurrentLogin(httpClient httpClient, hostname, authToken string) (string, error) { - query := `query UserCurrent{viewer{login}}` - reqBody, err := json.Marshal(map[string]interface{}{"query": query}) +// GetCurrentLogin returns the login of the user the token belongs to. +// +// The token is passed explicitly because this runs before the token is stored in config, so +// the transport has nothing to attach. The transport only sets Authorization when it is +// absent, so the header set here wins. +func GetCurrentLogin(httpClient *http.Client, hostname, authToken string) (string, error) { + var result struct{ Viewer struct{ Login string } } + + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + err := api.NewClientFromHTTP(httpClient).GraphQL(hostname, `query UserCurrent{viewer{login}}`, nil, &result, + api.WithHeader("Authorization", "token "+authToken)) if err != nil { return "", err } - result := struct { - Data struct{ Viewer struct{ Login string } } - }{} - apiEndpoint, err := safeurl.JoinPathWithHostPrefix(ghinstance.GraphQLEndpoint(hostname)) - if err != nil { - return "", err - } - req, err := http.NewRequest("POST", apiEndpoint.String(), bytes.NewBuffer(reqBody)) - if err != nil { - return "", err - } - req.Header.Set("Authorization", "token "+authToken) - res, err := httpClient.Do(req) - if err != nil { - return "", err - } - defer res.Body.Close() - if res.StatusCode > 299 { - return "", api.HandleHTTPError(res) - } - decoder := json.NewDecoder(res.Body) - err = decoder.Decode(&result) - if err != nil { - return "", err - } - return result.Data.Viewer.Login, nil + return result.Viewer.Login, nil } diff --git a/pkg/cmd/auth/shared/oauth_scopes.go b/pkg/cmd/auth/shared/oauth_scopes.go index bc5e611163a..67886b96b85 100644 --- a/pkg/cmd/auth/shared/oauth_scopes.go +++ b/pkg/cmd/auth/shared/oauth_scopes.go @@ -7,8 +7,6 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" - "github.com/cli/cli/v2/internal/safeurl" ) type MissingScopesError struct { @@ -28,25 +26,17 @@ func (e MissingScopesError) Error() string { return "missing required scopes " + scopes } -type httpClient interface { - Do(*http.Request) (*http.Response, error) -} - // GetScopes performs a GitHub API request and returns the value of the X-Oauth-Scopes header. -func GetScopes(httpClient httpClient, hostname, authToken string) (string, error) { - apiEndpoint, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname)) - if err != nil { - return "", err - } - - req, err := http.NewRequest("GET", apiEndpoint.String(), nil) - if err != nil { - return "", err - } - - req.Header.Set("Authorization", "token "+authToken) - - res, err := httpClient.Do(req) +// +// The token is passed explicitly because this runs before the token is stored in config, so +// the transport has nothing to attach. The transport only sets Authorization when it is +// absent, so the header set here wins. +func GetScopes(httpClient *http.Client, hostname, authToken string) (string, error) { + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + res, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, "", nil, + api.WithHeader("Authorization", "token "+authToken)) if err != nil { return "", err } @@ -67,7 +57,7 @@ func GetScopes(httpClient httpClient, hostname, authToken string) (string, error // HasMinimumScopes performs a GitHub API request and returns an error if the token used in the request // lacks the minimum required scopes for performing API operations with gh. -func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { +func HasMinimumScopes(httpClient *http.Client, hostname, authToken string) error { scopesHeader, err := GetScopes(httpClient, hostname, authToken) if err != nil { return err diff --git a/pkg/search/searcher.go b/pkg/search/searcher.go index dd8dd590cac..ab822517a48 100644 --- a/pkg/search/searcher.go +++ b/pkg/search/searcher.go @@ -2,16 +2,16 @@ package search import ( "encoding/json" + "errors" "fmt" - "io" "net/http" "net/url" "regexp" "strconv" "strings" + "github.com/cli/cli/v2/api" fd "github.com/cli/cli/v2/internal/featuredetection" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -24,7 +24,6 @@ const ( var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) var pageRE = regexp.MustCompile(`(\?|&)page=(\d*)`) -var jsonTypeRE = regexp.MustCompile(`[/+]json($|;)`) //go:generate moq -rm -out searcher_mock.go . Searcher type Searcher interface { @@ -198,7 +197,7 @@ func (s searcher) Issues(query Query) (IssuesResult, error) { // // For more information, see https://docs.github.com/en/rest/search/search?apiVersion=2022-11-28. func (s searcher) search(query Query, result interface{}) (string, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(s.host), "search", string(query.Kind)) + u, err := safeurl.JoinPath("search", string(query.Kind)) if err != nil { return "", err } @@ -236,28 +235,31 @@ func (s searcher) search(query Query, result interface{}) (string, error) { if query.Sort != "" { u.SetQuery(sortKey, query.Sort) } - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return "", err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - req.Header.Set("Accept", "application/vnd.github.v3+json") + accept := "application/vnd.github.v3+json" if query.Kind == KindCode { - req.Header.Set("Accept", "application/vnd.github.text-match+json") + accept = "application/vnd.github.text-match+json" } - resp, err := s.client.Do(req) + // The Content-Type this site used to set explicitly is the one the client already sends, + // so it is no longer stated here. + // TODO(api-client-rollout) + // This line of code is part of a mechanical roll out of the api client. + // As a follow up, consider whether the api client can be injected to this call site, rather than constructed + resp, err := api.NewClientFromHTTP(s.client).Request(s.host, http.MethodGet, u.String(), nil, + api.WithHeader("Accept", accept)) if err != nil { + // Search reports query errors in a shape of its own, so the generic API error is + // translated back rather than surfaced directly. + var apiErr api.HTTPError + if errors.As(err, &apiErr) { + return apiErr.Headers.Get("Link"), asSearchError(apiErr) + } return "", err } defer resp.Body.Close() link := resp.Header.Get("Link") - success := resp.StatusCode >= 200 && resp.StatusCode < 300 - if !success { - return link, handleHTTPError(resp) - } decoder := json.NewDecoder(resp.Body) err = decoder.Decode(result) if err != nil { @@ -297,23 +299,27 @@ func (err httpError) Error() string { return fmt.Sprintf("Invalid search query %q.\n%s", query, err.Errors[0].Message) } -func handleHTTPError(resp *http.Response) error { - httpError := httpError{ - RequestURL: resp.Request.URL, - StatusCode: resp.StatusCode, - } - if !jsonTypeRE.MatchString(resp.Header.Get("Content-Type")) { - httpError.Message = resp.Status - return httpError +// asSearchError converts an api.HTTPError into search's own error type, which formats +// invalid queries differently from the rest of the CLI. +func asSearchError(apiErr api.HTTPError) error { + searchErr := httpError{ + Message: apiErr.Message, + RequestURL: apiErr.RequestURL, + StatusCode: apiErr.StatusCode, } - body, err := io.ReadAll(resp.Body) - if err != nil { - return err + // A non-JSON response leaves Message empty, where this package used to report the status line. + if searchErr.Message == "" { + searchErr.Message = fmt.Sprintf("%d %s", apiErr.StatusCode, http.StatusText(apiErr.StatusCode)) } - if err := json.Unmarshal(body, &httpError); err != nil { - return err + for _, item := range apiErr.Errors { + searchErr.Errors = append(searchErr.Errors, httpErrorItem{ + Code: item.Code, + Field: item.Field, + Message: item.Message, + Resource: item.Resource, + }) } - return httpError + return searchErr } // nextPage extracts the next page number from an API response's link header. if From 03207f7ed55e911ac01b60f46be68646a1d9d9b8 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 14:03:32 +0200 Subject: [PATCH 10/16] Send redirect-sensitive requests through DoRequest Request delegates to go-gh, which builds an http.Client of its own from the transport alone. A CheckRedirect set on the client passed to NewClientFromHTTP is therefore silently discarded, which broke both call sites that rely on one. gh release download --archive stopped rewriting "legacy" Codeload paths, so the archive was saved under the wrong name. Acceptance caught this. gh repo delete stopped surfacing the 3xx for a renamed or transferred repo. Instead the redirect was followed, turning DELETE into GET against the new location, so the command reported success while deleting nothing. Nothing caught this: the existing unit test stubs a 307 with no Location header, and Go cannot follow a redirect without one, so the test passed whatever the redirect policy was. Both now send through DoRequest, which uses the client it was given. DoRequest gains RequestOption support so gh repo delete keeps declaring delete_repo. The cost is that gh repo delete keeps an absolute URL and so will not pick up api_host resolution until go-gh can carry a redirect policy. Release assets are unaffected, since their URLs come from the API and were always absolute. Tests are added at both sites and in the api package. They use real servers, because redirects are followed by the client, above the transport, so a stubbed RoundTripper cannot exercise this at all. Each was confirmed to fail against the broken version. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- api/client.go | 23 ++++++++--- api/request_test.go | 50 +++++++++++++++++++++++ pkg/cmd/release/download/download.go | 26 ++++++++---- pkg/cmd/release/download/download_test.go | 43 +++++++++++++++++++ pkg/cmd/repo/delete/http.go | 16 ++++++-- pkg/cmd/repo/delete/http_test.go | 30 ++++++++++++++ 6 files changed, 171 insertions(+), 17 deletions(-) diff --git a/api/client.go b/api/client.go index f11795edbe9..07027cabb94 100644 --- a/api/client.go +++ b/api/client.go @@ -226,14 +226,24 @@ func (c Client) RequestWithContext(ctx context.Context, hostname string, method // DoRequest sends a request that the caller has already built, and returns the response for the // caller to consume. It applies the same error handling as Request. // -// Prefer Request. DoRequest exists for the few call sites that must control fields of the request -// which are neither method, path, body nor header, such as ContentLength and GetBody when uploading -// a release asset. Because the caller supplies the URL, requests sent this way do not benefit from -// host-level endpoint resolution, so it is only appropriate for absolute URLs supplied by the API. +// Prefer Request. DoRequest exists for the few call sites that must control something Request +// cannot express. That is either a field of the request which is neither method, path, body nor +// header, such as ContentLength and GetBody when uploading a release asset, or a property of the +// client itself, such as CheckRedirect. Request delegates to go-gh, which builds a client of its +// own from the transport alone, so a redirect policy set on the client passed to +// NewClientFromHTTP only survives when the request is sent here. +// +// Because the caller supplies the URL, requests sent this way do not benefit from host-level +// endpoint resolution, so it is only appropriate for absolute URLs. +// +// Only the endpoint scopes of any RequestOption are applied; headers are the caller's own to set +// on the request they built. // // Responses outside the 2xx range are returned as an HTTPError and their body is closed. On success // the response is returned unread and the caller is responsible for closing its body. -func (c Client) DoRequest(req *http.Request) (*http.Response, error) { +func (c Client) DoRequest(req *http.Request, opts ...RequestOption) (*http.Response, error) { + cfg := newRequestConfig(opts) + resp, err := c.http.Do(req) if err != nil { return nil, err @@ -242,6 +252,9 @@ func (c Client) DoRequest(req *http.Request) (*http.Response, error) { success := resp.StatusCode >= 200 && resp.StatusCode < 300 if !success { defer resp.Body.Close() + if cfg.endpointScopes != "" { + EndpointNeedsScopes(resp, cfg.endpointScopes) + } return nil, HandleHTTPError(resp) } diff --git a/api/request_test.go b/api/request_test.go index ecf0cf53ef0..b0eea6de3b8 100644 --- a/api/request_test.go +++ b/api/request_test.go @@ -391,3 +391,53 @@ func TestGraphQLWithHeader(t *testing.T) { assert.Equal(t, "token explicit-token", reg.Requests[0].Header.Get("Authorization")) } + +// TestDoRequestPreservesCheckRedirect pins the difference between Request and DoRequest that +// motivates DoRequest existing for redirect-sensitive call sites. Request delegates to go-gh, +// which builds an http.Client of its own from the transport alone, so a redirect policy set on +// the client cannot survive. DoRequest sends through that client and so keeps it. +func TestDoRequestPreservesCheckRedirect(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/start" { + http.Redirect(w, r, "/final", http.StatusFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + newClient := func(called *bool) *http.Client { + return &http.Client{CheckRedirect: func(req *http.Request, via []*http.Request) error { + *called = true + return http.ErrUseLastResponse + }} + } + + t.Run("DoRequest honours it", func(t *testing.T) { + var called bool + req, err := http.NewRequest(http.MethodGet, ts.URL+"/start", nil) + require.NoError(t, err) + + // The redirect is not followed, so the 3xx is the final response and, being outside + // the 2xx range, is reported as an error. Call sites such as gh repo delete rely on + // seeing that status to explain that a repo was renamed or transferred. + _, err = NewClientFromHTTP(newClient(&called)).DoRequest(req) + require.Error(t, err) + + var httpErr HTTPError + require.ErrorAs(t, err, &httpErr) + assert.True(t, called, "CheckRedirect should have been consulted") + assert.Equal(t, http.StatusFound, httpErr.StatusCode) + }) + + t.Run("Request cannot honour it", func(t *testing.T) { + var called bool + + resp, err := NewClientFromHTTP(newClient(&called)).Request("github.com", http.MethodGet, ts.URL+"/start", nil) + require.NoError(t, err) + defer resp.Body.Close() + + assert.False(t, called, "go-gh builds its own client, so the policy cannot apply") + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) +} diff --git a/pkg/cmd/release/download/download.go b/pkg/cmd/release/download/download.go index 66932cf46fe..c3d29362f6c 100644 --- a/pkg/cmd/release/download/download.go +++ b/pkg/cmd/release/download/download.go @@ -242,7 +242,7 @@ func downloadRun(opts *DownloadOptions) error { } } - return downloadAssets(&dest, httpClient, baseRepo.RepoHost(), targets, opts.Concurrency, isArchive, opts.IO) + return downloadAssets(&dest, httpClient, targets, opts.Concurrency, isArchive, opts.IO) } func matchAny(patterns []string, name string) bool { @@ -259,7 +259,7 @@ type downloadTarget struct { name string } -func downloadAssets(dest *destinationWriter, httpClient *http.Client, hostname string, toDownload []downloadTarget, numWorkers int, isArchive bool, io *iostreams.IOStreams) error { +func downloadAssets(dest *destinationWriter, httpClient *http.Client, toDownload []downloadTarget, numWorkers int, isArchive bool, io *iostreams.IOStreams) error { if numWorkers == 0 { return errors.New("the number of concurrent workers needs to be greater than 0") } @@ -275,7 +275,7 @@ func downloadAssets(dest *destinationWriter, httpClient *http.Client, hostname s go func() { for a := range jobs { io.StartProgressIndicatorWithLabel(fmt.Sprintf("Downloading %s", a.name)) - results <- downloadAsset(dest, httpClient, hostname, a.url, a.name, isArchive) + results <- downloadAsset(dest, httpClient, a.url, a.name, isArchive) } }() } @@ -297,15 +297,20 @@ func downloadAssets(dest *destinationWriter, httpClient *http.Client, hostname s return downloadError } -func downloadAsset(dest *destinationWriter, httpClient *http.Client, hostname string, assetURL safeurl.SafeURL, fileName string, isArchive bool) error { +func downloadAsset(dest *destinationWriter, httpClient *http.Client, assetURL safeurl.SafeURL, fileName string, isArchive bool) error { if err := dest.Check(fileName); err != nil { return err } - accept := "application/octet-stream" + req, err := http.NewRequest(http.MethodGet, assetURL.String(), nil) + if err != nil { + return err + } + + req.Header.Set("Accept", "application/octet-stream") if isArchive { // adding application/json to Accept header due to a bug in the zipball/tarball API endpoint that makes it mandatory - accept = "application/octet-stream, application/json" + req.Header.Set("Accept", "application/octet-stream, application/json") // override HTTP redirect logic to avoid "legacy" Codeload resources. This is client // level rather than request level, so the client is copied before being handed over @@ -320,12 +325,15 @@ func downloadAsset(dest *destinationWriter, httpClient *http.Client, hostname st } } - // The asset URL is supplied by the API, so it is absolute and requested as given. + // The asset URL is supplied by the API, so it is absolute and requested as given. DoRequest + // rather than Request because the CheckRedirect above is a property of the client, and + // Request delegates to go-gh, which builds a client of its own from the transport alone and + // so silently drops it. Without it the "legacy" Codeload path is never rewritten and the + // archive is saved under the wrong name. // TODO(api-client-rollout) // This line of code is part of a mechanical roll out of the api client. // As a follow up, consider whether the api client can be injected to this call site, rather than constructed - resp, err := api.NewClientFromHTTP(httpClient).Request(hostname, http.MethodGet, assetURL.String(), nil, - api.WithHeader("Accept", accept)) + resp, err := api.NewClientFromHTTP(httpClient).DoRequest(req) if err != nil { return err } diff --git a/pkg/cmd/release/download/download_test.go b/pkg/cmd/release/download/download_test.go index 855380c3856..cc5ac482b76 100644 --- a/pkg/cmd/release/download/download_test.go +++ b/pkg/cmd/release/download/download_test.go @@ -4,18 +4,22 @@ import ( "bytes" "io" "net/http" + "net/http/httptest" "os" "path/filepath" "runtime" + "strings" "testing" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_NewCmdDownload(t *testing.T) { @@ -978,3 +982,42 @@ func listFiles(dir string) ([]string, error) { }) return files, err } + +// Test_downloadAsset_archiveAvoidsLegacyCodeload pins that the redirect policy which rewrites +// "legacy" Codeload paths is actually applied to the request. +// +// The policy lives on the http.Client rather than the request, so it only survives if the +// request is sent through the client we hand over. A real server is used because httpmock is a +// RoundTripper and redirects are followed by the client, above the transport, so a stubbed +// transport cannot exercise this at all. +func Test_downloadAsset_archiveAvoidsLegacyCodeload(t *testing.T) { + var requestedPaths []string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestedPaths = append(requestedPaths, r.URL.Path) + switch { + case r.URL.Path == "/asset": + http.Redirect(w, r, "/OWNER/REPO/legacy.zip/refs/tags/v1.2.3", http.StatusFound) + case strings.Contains(r.URL.Path, "legacy."): + t.Errorf("legacy Codeload path was requested: %s", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + default: + w.Header().Set("Content-Disposition", `attachment; filename=REPO-1.2.3.zip`) + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("archive contents")) + } + })) + defer ts.Close() + + tempDir := t.TempDir() + dest := destinationWriter{dir: tempDir} + + err := downloadAsset(&dest, ts.Client(), safeurl.NewImmutableSafeURL(ts.URL+"/asset"), "", true) + require.NoError(t, err) + + // The file name is derived from the response, so it also proves which resource was served. + body, err := os.ReadFile(filepath.Join(tempDir, "REPO-1.2.3.zip")) + require.NoError(t, err) + assert.Equal(t, "archive contents", string(body)) + + assert.Equal(t, []string{"/asset", "/OWNER/REPO/zip/refs/tags/v1.2.3"}, requestedPaths) +} diff --git a/pkg/cmd/repo/delete/http.go b/pkg/cmd/repo/delete/http.go index 7a7c937db6b..60d7a150798 100644 --- a/pkg/cmd/repo/delete/http.go +++ b/pkg/cmd/repo/delete/http.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -15,16 +16,25 @@ func deleteRepo(client *http.Client, repo ghrepo.Interface) error { return http.ErrUseLastResponse } - path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) + url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return err } + request, err := http.NewRequest(http.MethodDelete, url.String(), nil) + if err != nil { + return err + } + + // DoRequest rather than Request because the CheckRedirect set above is a property of the + // client, and Request delegates to go-gh, which builds a client of its own from the + // transport alone and so silently drops it. Following the redirect would delete nothing + // and report success. The cost is that this site keeps an absolute URL and so will not + // pick up api_host resolution until go-gh can carry a redirect policy. // TODO(api-client-rollout) // This line of code is part of a mechanical roll out of the api client. // As a follow up, consider whether the api client can be injected to this call site, rather than constructed - resp, err := api.NewClientFromHTTP(client).Request(repo.RepoHost(), http.MethodDelete, path.String(), nil, - api.WithEndpointScopes("delete_repo")) + resp, err := api.NewClientFromHTTP(client).DoRequest(request, api.WithEndpointScopes("delete_repo")) if err != nil { return err } diff --git a/pkg/cmd/repo/delete/http_test.go b/pkg/cmd/repo/delete/http_test.go index 5e3e07cd4eb..fde93823c05 100644 --- a/pkg/cmd/repo/delete/http_test.go +++ b/pkg/cmd/repo/delete/http_test.go @@ -49,3 +49,33 @@ func TestDeleteRepoMissingScope(t *testing.T) { require.ErrorAs(t, err, &httpErr) assert.Contains(t, httpErr.ScopesSuggestion(), "delete_repo") } + +// TestDeleteRepoDoesNotFollowRedirect pins that a renamed or transferred repo surfaces the 3xx +// to the caller rather than being followed. deleteRun relies on seeing that status to explain +// what happened, so following the redirect would report success while deleting nothing. +// +// The Location header matters: without it Go cannot follow a redirect at all, so a stub that +// omits it passes whatever the redirect policy is. +func TestDeleteRepoDoesNotFollowRedirect(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO"), + func(req *http.Request) (*http.Response, error) { + resp, err := httpmock.StatusStringResponse(301, "")(req) + if err != nil { + return nil, err + } + resp.Header.Set("Location", "https://api.github.com/repos/OWNER/RENAMED") + return resp, nil + }, + ) + + err := deleteRepo(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO")) + require.Error(t, err) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, 301, httpErr.StatusCode) +} From af4bf0c50f84a035d4c81014836266a632a65355 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 15:45:56 +0200 Subject: [PATCH 11/16] Make the transferred-ownership fixture actually redirect The "repo transferred ownership" case stubbed a 307 with no Location header. Go declines to follow a redirect without one, so the redirect policy under test was never consulted and the case passed whether or not deleteRepo suppressed redirects. Removing the CheckRedirect from deleteRepo left the whole Test_deleteRun suite green. Add the Location header so the stub is a redirect Go will actually follow, and assert wantStderr on the error path, which the early return had been skipping. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- pkg/cmd/repo/delete/delete_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/repo/delete/delete_test.go b/pkg/cmd/repo/delete/delete_test.go index 64a8ee30161..55a249d047a 100644 --- a/pkg/cmd/repo/delete/delete_test.go +++ b/pkg/cmd/repo/delete/delete_test.go @@ -194,9 +194,14 @@ func Test_deleteRun(t *testing.T) { errMsg: "SilentError", wantStderr: "X Failed to delete repository: OWNER/REPO has changed name or transferred ownership\n", httpStubs: func(reg *httpmock.Registry) { + // The Location header is what makes this a real redirect. Without it Go declines + // to follow the 307 regardless of the client's CheckRedirect policy, so the stub + // would pass even if deleteRepo stopped suppressing redirects. reg.Register( httpmock.REST("DELETE", "repos/OWNER/REPO"), - httpmock.StatusStringResponse(307, "{}")) + httpmock.WithHeader( + httpmock.StatusStringResponse(307, "{}"), + "Location", "https://api.github.com/repos/OWNER/RENAMED")) }, }, } @@ -230,6 +235,7 @@ func Test_deleteRun(t *testing.T) { if tt.wantErr { assert.Error(t, err) assert.Equal(t, tt.errMsg, err.Error()) + assert.Equal(t, tt.wantStderr, stderr.String()) return } assert.NoError(t, err) From 6b92c44a7d3bd71e173e1ceccdc417a23a7e8e72 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 16:02:24 +0200 Subject: [PATCH 12/16] Route remaining api package sites through relative paths RepoExists called client.HTTP().Head directly, bypassing api.Client entirely, and RenameRepo passed REST an absolute RESTPrefix URL, which go-gh requests as given rather than resolving against the host endpoint. Neither would pick up api_host. Both now build relative paths. RepoExists uses Request, since a HEAD response has no body to decode, and maps a 404 HTTPError back to a false result to preserve its existing contract. Add a case pinning that only 200 counts as existence, which the previous switch expressed through its default arm. This removes the last ghinstance.RESTPrefix use from the api package. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- api/queries_repo.go | 28 ++++++++++++++++------------ api/queries_repo_test.go | 14 ++++++++++++++ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 3e0b75648cb..6544b8ea548 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -13,7 +13,6 @@ import ( "time" "github.com/cli/cli/v2/internal/gh" - "github.com/cli/cli/v2/internal/ghinstance" "golang.org/x/sync/errgroup" "github.com/cli/cli/v2/internal/ghrepo" @@ -647,13 +646,13 @@ func RenameRepo(client *Client, repo ghrepo.Interface, newRepoName string) (*Rep return nil, err } - path, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return nil, err } result := repositoryV3{} - err = client.REST(repo.RepoHost(), "PATCH", path.String(), body, &result) + err = client.REST(repo.RepoHost(), http.MethodPatch, path.String(), body, &result) if err != nil { return nil, err } @@ -1671,26 +1670,31 @@ func GetRepoIDs(client *Client, host string, repositories []ghrepo.Interface) ([ } func RepoExists(client *Client, repo ghrepo.Interface) (bool, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName()) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName()) if err != nil { return false, err } - resp, err := client.HTTP().Head(u.String()) + // A HEAD request has no body to decode, so Request is used rather than REST. Existence is + // decided by the status alone. + resp, err := client.Request(repo.RepoHost(), http.MethodHead, path.String(), nil) if err != nil { + var httpErr HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return false, nil + } return false, err } defer resp.Body.Close() - switch resp.StatusCode { - case 200: - return true, nil - case 404: - return false, nil - default: - return false, ghAPI.HandleHTTPError(resp) + // Only 200 means the repository exists. Any other success status is unexpected here and is + // reported as an error rather than being taken as existence. + if resp.StatusCode != http.StatusOK { + return false, HandleHTTPError(resp) } + + return true, nil } // RepoLicenses fetches available repository licenses. diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 4fe7074f187..73edfbabd6d 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -798,6 +798,20 @@ func TestRepoExists(t *testing.T) { existCheck: false, wantErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO)", }, + { + // Only 200 counts as existence. A 2xx other than 200 is unexpected for this + // endpoint and must be reported rather than quietly taken as a yes. + name: "unexpected success status", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO"), + httpmock.StatusStringResponse(201, ""), + ) + }, + repo: ghrepo.New("OWNER", "REPO"), + existCheck: false, + wantErrMsg: "HTTP 201 (https://api.github.com/repos/OWNER/REPO)", + }, } for _, tt := range tests { reg := &httpmock.Registry{} From 02feb320d1b696381baabc0ecd6dc9ad660cd933 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 16:20:27 +0200 Subject: [PATCH 13/16] Add acceptance coverage for gist gh gist had no acceptance coverage, which left the gist call sites migrated to api.Client resting on unit tests alone. Two scripts cover the create, view, edit, rename, list and delete flows, and between them exercise three of the four migrated sites: the create POST, the single gist GET, and the GraphQL list. The fourth, fetching a truncated file's full content from a raw URL, needs a file over the API's 1MB inline limit to trigger. That is too large to carry in a txtar, so it stays with the unit tests and the gap is noted in the script. Deletion is tested without a deferred cleanup, following repo-delete and ssh-key, because a deferred delete would fail on the second attempt. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- acceptance/acceptance_test.go | 9 +++ .../gist/gist-create-view-delete.txtar | 40 +++++++++++++ .../testdata/gist/gist-edit-rename-list.txtar | 56 +++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 acceptance/testdata/gist/gist-create-view-delete.txtar create mode 100644 acceptance/testdata/gist/gist-edit-rename-list.txtar diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 9030e050611..c8ae750a047 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -85,6 +85,15 @@ func TestAuth(t *testing.T) { testscript.Run(t, testScriptParamsFor(tsEnv, "auth")) } +func TestGists(t *testing.T) { + var tsEnv testScriptEnv + if err := tsEnv.fromEnv(); err != nil { + t.Fatal(err) + } + + testscript.Run(t, testScriptParamsFor(tsEnv, "gist")) +} + func TestGPGKeys(t *testing.T) { var tsEnv testScriptEnv if err := tsEnv.fromEnv(); err != nil { diff --git a/acceptance/testdata/gist/gist-create-view-delete.txtar b/acceptance/testdata/gist/gist-create-view-delete.txtar new file mode 100644 index 00000000000..86ed08f53fb --- /dev/null +++ b/acceptance/testdata/gist/gist-create-view-delete.txtar @@ -0,0 +1,40 @@ +# Gists are owned by the authenticated user rather than an org, so unlike most +# other acceptance scripts there is no repository to create or clean up. +# +# Not covered here: gists whose files exceed the API's 1MB inline limit come +# back truncated, and the full content is then fetched from a raw URL on +# gist.githubusercontent.com rather than the API host. Triggering that needs a +# file too large to keep in a txtar, so it is left to unit tests. + +# Setup useful env vars +env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a gist from a file, capturing its URL +exec gh gist create gist-file.txt --desc ${GIST_DESC} +stdout 'https://gist.github.com/' +stdout2env GIST_URL + +# View the gist and check the description and content are both rendered +exec gh gist view ${GIST_URL} +stdout ${GIST_DESC} +stdout 'hello from the acceptance tests' + +# List the file names in the gist +exec gh gist view ${GIST_URL} --files +stdout 'gist-file.txt' + +# View a single file raw, which must be the content we uploaded and nothing else +exec gh gist view ${GIST_URL} --filename gist-file.txt --raw +stdout 'hello from the acceptance tests' +! stdout ${GIST_DESC} + +# Delete the gist. This is deliberately not deferred because deletion is what +# the script is testing, and a deferred delete would fail on the second attempt. +exec gh gist delete --yes ${GIST_URL} + +# Check the gist is gone +! exec gh gist view ${GIST_URL} +stderr 'not found' + +-- gist-file.txt -- +hello from the acceptance tests diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar new file mode 100644 index 00000000000..4fd27aeb62e --- /dev/null +++ b/acceptance/testdata/gist/gist-edit-rename-list.txtar @@ -0,0 +1,56 @@ +# Gists are owned by the authenticated user rather than an org, so unlike most +# other acceptance scripts there is no repository to create or clean up. +# +# Only --add and --remove are exercised here. Editing a file's content, and +# editing the description on its own, both fall through to an interactive +# editor, so they cannot be driven from a script. + +# Setup useful env vars +env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a gist from a file, capturing its URL +exec gh gist create first.txt --desc ${GIST_DESC} +stdout2env GIST_URL + +# Defer gist cleanup +defer gh gist delete --yes ${GIST_URL} + +# Add a second file to the gist +exec gh gist edit ${GIST_URL} --add second.txt + +# Check both files are now in the gist +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'second.txt' + +# Rename the second file +exec gh gist rename ${GIST_URL} second.txt renamed.txt + +# Check the rename took effect +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'renamed.txt' +! stdout 'second.txt' + +# Check the renamed file kept its content +exec gh gist view ${GIST_URL} --filename renamed.txt --raw +stdout 'the second file' + +# Find the gist by description, which lists secret gists by default +exec gh gist list --filter ${GIST_DESC} +stdout ${GIST_DESC} +stdout '2 files' +stdout 'secret' + +# Remove the renamed file +exec gh gist edit ${GIST_URL} --remove renamed.txt + +# Check only the first file remains +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +! stdout 'renamed.txt' + +-- first.txt -- +the first file +-- second.txt -- +the second file From 4fefe7763326a75bd4a51fd3b61a93407e1e32df Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 16:27:12 +0200 Subject: [PATCH 14/16] Remove racy assertions from the gist edit script The script asserted that a file was absent immediately after removing it, which reads the gist back before the write has necessarily settled and fails when it has not. Removing a file issues the same update request as adding one, so the step covered no further ground to pay for that. Drop the --remove step, and drop the assertion that the old filename is gone after a rename, which is redundant because the new name being present already proves the rename happened. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- .../testdata/gist/gist-edit-rename-list.txtar | 21 +++++++------------ 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar index 4fd27aeb62e..740323f3e15 100644 --- a/acceptance/testdata/gist/gist-edit-rename-list.txtar +++ b/acceptance/testdata/gist/gist-edit-rename-list.txtar @@ -1,9 +1,11 @@ # Gists are owned by the authenticated user rather than an org, so unlike most # other acceptance scripts there is no repository to create or clean up. # -# Only --add and --remove are exercised here. Editing a file's content, and -# editing the description on its own, both fall through to an interactive -# editor, so they cannot be driven from a script. +# Only --add is exercised here. Editing a file's content, and editing the +# description on its own, both fall through to an interactive editor, so they +# cannot be driven from a script. --remove is left out deliberately: it issues +# the same update request as --add, so it covers no further ground, and +# checking that a file has gone raced against the read after the write. # Setup useful env vars env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} @@ -23,14 +25,15 @@ exec gh gist view ${GIST_URL} --files stdout 'first.txt' stdout 'second.txt' -# Rename the second file +# Rename the second file. Checking that the new name is present is enough to +# prove the rename happened, and asserting the absence of the old name would +# only add another chance to read the gist before the write has settled. exec gh gist rename ${GIST_URL} second.txt renamed.txt # Check the rename took effect exec gh gist view ${GIST_URL} --files stdout 'first.txt' stdout 'renamed.txt' -! stdout 'second.txt' # Check the renamed file kept its content exec gh gist view ${GIST_URL} --filename renamed.txt --raw @@ -42,14 +45,6 @@ stdout ${GIST_DESC} stdout '2 files' stdout 'secret' -# Remove the renamed file -exec gh gist edit ${GIST_URL} --remove renamed.txt - -# Check only the first file remains -exec gh gist view ${GIST_URL} --files -stdout 'first.txt' -! stdout 'renamed.txt' - -- first.txt -- the first file -- second.txt -- From 6a0c261c303298620b16ad4a7b9bcc556e77b964 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 19:47:54 +0200 Subject: [PATCH 15/16] Drop the gist edit acceptance script Reads of a gist that has just been updated serve the previous state often enough to make the script unreliable, measured at 2 failures in 10 attempts against the real API. Creating and deleting a gist read back consistently over 15 attempts, so the remaining script stays. Removing rather than tolerating the race, because the script covered no code this rollout changed. Fetching, listing and updating a gist already went through the api client before this work; only creation and the raw file fetch moved. Creation is still covered, and the raw fetch needs a file larger than a txtar can hold. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- .../gist/gist-create-view-delete.txtar | 7 +++ .../testdata/gist/gist-edit-rename-list.txtar | 51 ------------------- 2 files changed, 7 insertions(+), 51 deletions(-) delete mode 100644 acceptance/testdata/gist/gist-edit-rename-list.txtar diff --git a/acceptance/testdata/gist/gist-create-view-delete.txtar b/acceptance/testdata/gist/gist-create-view-delete.txtar index 86ed08f53fb..404fda39405 100644 --- a/acceptance/testdata/gist/gist-create-view-delete.txtar +++ b/acceptance/testdata/gist/gist-create-view-delete.txtar @@ -5,6 +5,13 @@ # back truncated, and the full content is then fetched from a raw URL on # gist.githubusercontent.com rather than the API host. Triggering that needs a # file too large to keep in a txtar, so it is left to unit tests. +# +# Also not covered: editing an existing gist, whether by content, rename, add or +# remove. Reads of a gist that has just been updated serve the previous state +# often enough to make any such script unreliable, measured at 2 failures in 10 +# attempts, whereas creating and deleting read back consistently. Nothing is +# lost by the omission, because the edit path reaches the API through the same +# fetch and list calls this script already exercises. # Setup useful env vars env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar deleted file mode 100644 index 740323f3e15..00000000000 --- a/acceptance/testdata/gist/gist-edit-rename-list.txtar +++ /dev/null @@ -1,51 +0,0 @@ -# Gists are owned by the authenticated user rather than an org, so unlike most -# other acceptance scripts there is no repository to create or clean up. -# -# Only --add is exercised here. Editing a file's content, and editing the -# description on its own, both fall through to an interactive editor, so they -# cannot be driven from a script. --remove is left out deliberately: it issues -# the same update request as --add, so it covers no further ground, and -# checking that a file has gone raced against the read after the write. - -# Setup useful env vars -env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} - -# Create a gist from a file, capturing its URL -exec gh gist create first.txt --desc ${GIST_DESC} -stdout2env GIST_URL - -# Defer gist cleanup -defer gh gist delete --yes ${GIST_URL} - -# Add a second file to the gist -exec gh gist edit ${GIST_URL} --add second.txt - -# Check both files are now in the gist -exec gh gist view ${GIST_URL} --files -stdout 'first.txt' -stdout 'second.txt' - -# Rename the second file. Checking that the new name is present is enough to -# prove the rename happened, and asserting the absence of the old name would -# only add another chance to read the gist before the write has settled. -exec gh gist rename ${GIST_URL} second.txt renamed.txt - -# Check the rename took effect -exec gh gist view ${GIST_URL} --files -stdout 'first.txt' -stdout 'renamed.txt' - -# Check the renamed file kept its content -exec gh gist view ${GIST_URL} --filename renamed.txt --raw -stdout 'the second file' - -# Find the gist by description, which lists secret gists by default -exec gh gist list --filter ${GIST_DESC} -stdout ${GIST_DESC} -stdout '2 files' -stdout 'secret' - --- first.txt -- -the first file --- second.txt -- -the second file From ebc1a12a8edc1b0aaf43f849bab207eb2ac6e04c Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 6 Aug 2026 20:05:27 +0200 Subject: [PATCH 16/16] Restore the gist edit acceptance script with sleeps Updates to an existing gist are visible to a following read around eight times in ten, settling within about a second and a half, so a sleep after each update is enough. This matches how the pr and search scripts already wait on writes. The failures were not only late assertions. Rename reads the gist to build its update, so a stale read there renames nothing, exits 0, and leaves the gist permanently wrong. Two runs in fifteen never converged however long they were polled. Sleeping before the rename rather than before the assertion removed those, fifteen runs in fifteen. Creating a gist reads back consistently over fifteen attempts and needs no sleep. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144144f1-73c0-46dd-a4f2-5a1b3d0c2f83 --- .../gist/gist-create-view-delete.txtar | 7 --- .../testdata/gist/gist-edit-rename-list.txtar | 60 +++++++++++++++++++ 2 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 acceptance/testdata/gist/gist-edit-rename-list.txtar diff --git a/acceptance/testdata/gist/gist-create-view-delete.txtar b/acceptance/testdata/gist/gist-create-view-delete.txtar index 404fda39405..86ed08f53fb 100644 --- a/acceptance/testdata/gist/gist-create-view-delete.txtar +++ b/acceptance/testdata/gist/gist-create-view-delete.txtar @@ -5,13 +5,6 @@ # back truncated, and the full content is then fetched from a raw URL on # gist.githubusercontent.com rather than the API host. Triggering that needs a # file too large to keep in a txtar, so it is left to unit tests. -# -# Also not covered: editing an existing gist, whether by content, rename, add or -# remove. Reads of a gist that has just been updated serve the previous state -# often enough to make any such script unreliable, measured at 2 failures in 10 -# attempts, whereas creating and deleting read back consistently. Nothing is -# lost by the omission, because the edit path reaches the API through the same -# fetch and list calls this script already exercises. # Setup useful env vars env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar new file mode 100644 index 00000000000..537e63b9295 --- /dev/null +++ b/acceptance/testdata/gist/gist-edit-rename-list.txtar @@ -0,0 +1,60 @@ +# Gists are owned by the authenticated user rather than an org, so unlike most +# other acceptance scripts there is no repository to create or clean up. +# +# Only --add is exercised here. Editing a file's content, and editing the +# description on its own, both fall through to an interactive editor, so they +# cannot be driven from a script. --remove is left out deliberately, because it +# issues the same update request as --add and so covers no further ground. +# +# Every update to an existing gist is followed by a sleep. A read that comes +# straight after an update serves the previous state around twice in ten +# attempts, and it settles within about a second and a half. The sleeps matter +# most before the next gh command rather than before an assertion: rename reads +# the gist to build its update, so a stale read there renames nothing at all and +# still exits 0, leaving the gist permanently wrong rather than briefly behind. +# Creating a gist reads back consistently, so it needs no sleep. + +# Setup useful env vars +env GIST_DESC=${SCRIPT_NAME}-${RANDOM_STRING} + +# Create a gist from a file, capturing its URL +exec gh gist create first.txt --desc ${GIST_DESC} +stdout2env GIST_URL + +# Defer gist cleanup +defer gh gist delete --yes ${GIST_URL} + +# Add a second file to the gist +exec gh gist edit ${GIST_URL} --add second.txt +sleep 5 + +# Check both files are now in the gist +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'second.txt' + +# Rename the second file. Checking that the new name is present is enough to +# prove the rename happened, and asserting the absence of the old name would +# only add another chance to read the gist before the write has settled. +exec gh gist rename ${GIST_URL} second.txt renamed.txt +sleep 5 + +# Check the rename took effect +exec gh gist view ${GIST_URL} --files +stdout 'first.txt' +stdout 'renamed.txt' + +# Check the renamed file kept its content +exec gh gist view ${GIST_URL} --filename renamed.txt --raw +stdout 'the second file' + +# Find the gist by description, which lists secret gists by default +exec gh gist list --filter ${GIST_DESC} +stdout ${GIST_DESC} +stdout '2 files' +stdout 'secret' + +-- first.txt -- +the first file +-- second.txt -- +the second file