From d0de0edf7380d08a63452ab1e237b706cf7628aa Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 4 May 2026 11:20:15 -0700 Subject: [PATCH 1/2] browse: disambiguate decimal-only short SHAs from issue numbers When a selector argument passed to gh browse consists entirely of decimal digits and is at least 7 characters long, it satisfies both isNumber and isCommit. Previously isNumber was always checked first, so gh browse 309628980 would open an issues URL even when that string referred to an actual commit SHA. Only in the ambiguous case (decimal-only, no # prefix, passes both checks), call the commits API to determine whether the ref resolves to a real commit. If it does, open the commit URL; otherwise fall back to the issues URL. No API call is made for unambiguous inputs. Fixes #12357 --- api/queries_repo.go | 20 ++++++++++++++++++ pkg/cmd/browse/browse.go | 16 ++++++++++++++ pkg/cmd/browse/browse_test.go | 39 +++++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/api/queries_repo.go b/api/queries_repo.go index d1bc2df1e29..14b1ffc2a2e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -1685,6 +1685,26 @@ func RepoExists(client *Client, repo ghrepo.Interface) (bool, error) { } } +func CommitExists(client *Client, repo ghrepo.Interface, ref string) (bool, error) { + path := fmt.Sprintf("repos/%s/%s/commits/%s", repo.RepoOwner(), repo.RepoName(), ref) + + resp, err := client.HTTP().Head(ghinstance.RESTPrefix(repo.RepoHost()) + path) + if err != nil { + return false, err + } + + defer resp.Body.Close() + + switch resp.StatusCode { + case 200: + return true, nil + case 404, 422: + return false, nil + default: + return false, ghAPI.HandleHTTPError(resp) + } +} + // RepoLicenses fetches available repository licenses. // It uses API v3 because licenses are not supported by GraphQL. func RepoLicenses(httpClient *http.Client, hostname string) ([]License, error) { diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index d057139565a..a6ea66408fe 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -250,6 +250,22 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error return "", nil } if isNumber(opts.SelectorArg) { + // A selector without a "#" prefix that satisfies both isNumber and + // isCommit is ambiguous (e.g. "309628980"). Disambiguate via API. + if !strings.HasPrefix(opts.SelectorArg, "#") && isCommit(opts.SelectorArg) { + httpClient, err := opts.HttpClient() + if err != nil { + return "", err + } + apiClient := api.NewClientFromHTTP(httpClient) + exists, err := api.CommitExists(apiClient, baseRepo, opts.SelectorArg) + if err != nil { + return "", err + } + if exists { + return fmt.Sprintf("commit/%s", opts.SelectorArg), nil + } + } return fmt.Sprintf("issues/%s", strings.TrimPrefix(opts.SelectorArg, "#")), nil } if isCommit(opts.SelectorArg) { diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index c321fbdbb57..233b45f6a40 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -674,6 +674,45 @@ func Test_runBrowse(t *testing.T) { expectedURL: "https://github.com/owner/repo/blame/abc123/src/app.js#L50", wantsErr: false, }, + { + name: "decimal-only short SHA that is a valid commit", + opts: BrowseOptions{ + SelectorArg: "309628980", + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/cli/cli/commits/309628980"), + httpmock.StatusStringResponse(200, ""), + ) + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/commit/309628980", + wantsErr: false, + }, + { + name: "decimal-only string that is not a valid commit", + opts: BrowseOptions{ + SelectorArg: "1234567", + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/cli/cli/commits/1234567"), + httpmock.StatusStringResponse(404, ""), + ) + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/issues/1234567", + wantsErr: false, + }, + { + name: "decimal-only short SHA with # prefix forces issue", + opts: BrowseOptions{ + SelectorArg: "#309628980", + }, + baseRepo: ghrepo.New("cli", "cli"), + expectedURL: "https://github.com/cli/cli/issues/309628980", + wantsErr: false, + }, } for _, tt := range tests { From 7af3d158eb6ff0e0239f83eed521643812201751 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Mon, 4 May 2026 11:25:53 -0700 Subject: [PATCH 2/2] Add unit tests for CommitExists API helper Cover all status code paths: 200 (exists), 404 (not found), 422 (unprocessable ref), and 500 (server error). --- api/queries_repo_test.go | 73 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 4fe7074f187..123e39ef8f2 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -823,6 +823,79 @@ func TestRepoExists(t *testing.T) { } } +func TestCommitExists(t *testing.T) { + tests := []struct { + name string + httpStub func(*httpmock.Registry) + existCheck bool + wantErrMsg string + }{ + { + name: "commit exists", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/309628980"), + httpmock.StringResponse("{}"), + ) + }, + existCheck: true, + }, + { + name: "commit not found", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(404, "Not Found"), + ) + }, + existCheck: false, + }, + { + name: "unprocessable ref", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(422, "Unprocessable Entity"), + ) + }, + existCheck: false, + }, + { + name: "http error", + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.REST("HEAD", "repos/OWNER/REPO/commits/1234567"), + httpmock.StatusStringResponse(500, "Internal Server Error"), + ) + }, + existCheck: false, + wantErrMsg: "HTTP 500 (https://api.github.com/repos/OWNER/REPO/commits/1234567)", + }, + } + for _, tt := range tests { + reg := &httpmock.Registry{} + if tt.httpStub != nil { + tt.httpStub(reg) + } + + client := newTestClient(reg) + ref := "309628980" + if !tt.existCheck || tt.wantErrMsg != "" { + ref = "1234567" + } + + t.Run(tt.name, func(t *testing.T) { + exist, err := CommitExists(client, ghrepo.New("OWNER", "REPO"), ref) + if tt.wantErrMsg != "" { + assert.Equal(t, tt.wantErrMsg, err.Error()) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.existCheck, exist) + }) + } +} + func TestForkRepoReturnsErrorWhenForkIsNotPossible(t *testing.T) { // Given our API returns 202 with a Fork that is the same as // the repo we provided