From 0222d061a93fb5e56ec8b0d782ecd6dd277a5f74 Mon Sep 17 00:00:00 2001 From: Christopher Langton Date: Thu, 12 Nov 2026 21:24:07 +1100 Subject: [PATCH 1/3] test(forge): cover Check, repoAccessError, comma and CheckResult.String Check runs against an httptest TLS server posing as a GitHub Enterprise host, so every branch of the two-check flow is exercised without network: user token with and without repository access, a repository-scoped token, a rejected token with no repository, and a rate-limit read that fails. repoAccessError, comma, CheckResult.String and NewClient's enterprise URL handling get direct cases. Package coverage 19.3% -> 37.6%. Closes #292 --- internal/analyze/forge/auth_check_test.go | 185 ++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 internal/analyze/forge/auth_check_test.go diff --git a/internal/analyze/forge/auth_check_test.go b/internal/analyze/forge/auth_check_test.go new file mode 100644 index 0000000000..b9b352081c --- /dev/null +++ b/internal/analyze/forge/auth_check_test.go @@ -0,0 +1,185 @@ +package forge + +import ( + "context" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + "github.com/google/go-github/v66/github" + "github.com/stretchr/testify/require" +) + +// fakeGitHub serves the three endpoints Check calls, as a GitHub Enterprise +// host would (under /api/v3/), each answering with the status it is given. +type fakeGitHub struct { + userStatus, repoStatus, rateStatus int + calls []string +} + +func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { + f.calls = append(f.calls, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + reply := func(status int, body string) { + w.WriteHeader(status) + if status == http.StatusOK { + _, _ = fmt.Fprint(w, body) + return + } + _, _ = fmt.Fprintf(w, `{"message":"%s"}`, http.StatusText(status)) + } + switch r.URL.Path { + case "/api/v3/user": + reply(f.userStatus, `{"login":"octocat"}`) + case "/api/v3/repos/acme/widgets": + reply(f.repoStatus, `{"name":"widgets","owner":{"login":"acme"}}`) + case "/api/v3/rate_limit": + reply(f.rateStatus, `{"resources":{"core":{"limit":5000,"remaining":4321,"reset":1767225600}}}`) + default: + http.NotFound(w, r) + } +} + +// startFakeGitHub runs f over TLS and returns the host to pass to Check. The +// client Check builds uses http.DefaultTransport, which is pointed at the test +// server's trusted transport for the duration of the test. +func startFakeGitHub(t *testing.T, f *fakeGitHub) string { + t.Helper() + srv := httptest.NewTLSServer(f) + t.Cleanup(srv.Close) + prev := http.DefaultTransport + http.DefaultTransport = srv.Client().Transport + t.Cleanup(func() { http.DefaultTransport = prev }) + u, err := url.Parse(srv.URL) + require.NoError(t, err) + return u.Host +} + +var testCreds = &Credentials{Token: "t0ken", Source: "GH_TOKEN"} +var testRepo = Repo{Owner: "acme", Name: "widgets"} + +func TestCheck_UserTokenWithRepoAccess(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 200, repoStatus: 200, rateStatus: 200}) + + res, err := Check(context.Background(), testCreds, host, testRepo) + require.NoError(t, err) + require.Equal(t, "octocat", res.Login) + require.Equal(t, "GH_TOKEN", res.Source) + require.Equal(t, 4321, res.RateLimitRemaining) + require.Equal(t, 5000, res.RateLimitLimit) + require.Equal(t, time.Unix(1767225600, 0).UTC(), res.RateLimitResetsAt.UTC()) +} + +func TestCheck_UserRejectedWithoutRepoIsATokenError(t *testing.T) { + f := &fakeGitHub{userStatus: 401, repoStatus: 200, rateStatus: 200} + host := startFakeGitHub(t, f) + + _, err := Check(context.Background(), testCreds, host, Repo{}) + require.Error(t, err) + require.Contains(t, err.Error(), "GitHub rejected the token from GH_TOKEN") + require.NotContains(t, f.calls, "/api/v3/repos/acme/widgets", "with no repository there is nothing else to try") +} + +// GitHub Actions' GITHUB_TOKEN cannot read /user; reading the repository is +// the validity check, and the result has no login. +func TestCheck_RepositoryScopedToken(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 403, repoStatus: 200, rateStatus: 200}) + + res, err := Check(context.Background(), testCreds, host, testRepo) + require.NoError(t, err) + require.Empty(t, res.Login) + require.Equal(t, 4321, res.RateLimitRemaining) + require.Contains(t, res.String(), "repository-scoped token via GH_TOKEN") +} + +func TestCheck_RepositoryScopedTokenWithoutRateLimit(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 403, repoStatus: 200, rateStatus: 500}) + + res, err := Check(context.Background(), testCreds, host, testRepo) + require.NoError(t, err) + require.Equal(t, CheckResult{Source: "GH_TOKEN"}, *res) +} + +func TestCheck_RepositoryScopedTokenThatCannotSeeTheRepo(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 403, repoStatus: 404, rateStatus: 200}) + + _, err := Check(context.Background(), testCreds, host, testRepo) + require.Error(t, err) + require.Contains(t, err.Error(), "authenticated as repository-scoped token") +} + +// A valid user token that cannot see a private repository gets 404, not 403; +// the error must say it is an access problem and name the token and login. +func TestCheck_UserTokenCannotSeeTheRepo(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 200, repoStatus: 404, rateStatus: 200}) + + _, err := Check(context.Background(), testCreds, host, testRepo) + require.Error(t, err) + msg := err.Error() + require.Contains(t, msg, "the GitHub token from GH_TOKEN is valid (authenticated as octocat) but cannot read acme/widgets") + require.Contains(t, msg, "almost certainly\n an access problem") +} + +// The quota is informational: failing to read it does not fail the check. +func TestCheck_RateLimitFailureIsNotFatal(t *testing.T) { + host := startFakeGitHub(t, &fakeGitHub{userStatus: 200, repoStatus: 200, rateStatus: 500}) + + res, err := Check(context.Background(), testCreds, host, testRepo) + require.NoError(t, err) + require.Equal(t, CheckResult{Login: "octocat", Source: "GH_TOKEN"}, *res) +} + +func TestRepoAccessError(t *testing.T) { + raw := errors.New("boom") + resp := func(code int) *github.Response { + return &github.Response{Response: &http.Response{StatusCode: code}} + } + + for _, code := range []int{http.StatusNotFound, http.StatusForbidden} { + err := repoAccessError(testCreds, "octocat", testRepo, resp(code), raw) + require.Contains(t, err.Error(), "the GitHub token from GH_TOKEN is valid (authenticated as octocat) but cannot read acme/widgets", "status %d", code) + require.NotErrorIs(t, err, raw, "the access message replaces the bare status") + } + + for _, r := range []*github.Response{resp(http.StatusInternalServerError), nil} { + err := repoAccessError(testCreds, "octocat", testRepo, r, raw) + require.Equal(t, "GitHub could not read acme/widgets: boom", err.Error()) + require.ErrorIs(t, err, raw, "any other failure keeps the raw error") + } +} + +func TestComma(t *testing.T) { + for n, want := range map[int]string{ + 0: "0", 7: "7", 999: "999", 1000: "1,000", 12345: "12,345", + 123456: "123,456", 1234567: "1,234,567", + } { + require.Equal(t, want, comma(n), "comma(%d)", n) + } +} + +func TestCheckResultString(t *testing.T) { + user := CheckResult{Login: "octocat", Source: "gh CLI", RateLimitRemaining: 4321, RateLimitLimit: 5000} + require.Equal(t, "GitHub authentication: ok (octocat via gh CLI, 4,321/5,000 requests remaining)", user.String()) + + scoped := CheckResult{Source: "GITHUB_TOKEN", RateLimitRemaining: 999, RateLimitLimit: 1000} + require.Equal(t, "GitHub authentication: ok (repository-scoped token via GITHUB_TOKEN, 999/1,000 requests remaining)", scoped.String()) +} + +func TestNewClientEnterpriseHost(t *testing.T) { + c, err := NewClient(testCreds, "ghe.example.com") + require.NoError(t, err) + require.Equal(t, "https://ghe.example.com/api/v3/", c.GitHub().BaseURL.String()) + + c, err = NewClient(testCreds, "github.com") + require.NoError(t, err) + require.Equal(t, "https://api.github.com/", c.GitHub().BaseURL.String(), "the public host keeps the default API URL") + + _, err = NewClient(testCreds, "bad host%zz") + require.Error(t, err) + require.True(t, strings.HasPrefix(err.Error(), "configure GitHub Enterprise host bad host%zz:"), err.Error()) +} From e611440b628095df0c4d9c1d30255a73e39d4f90 Mon Sep 17 00:00:00 2001 From: Christopher Langton Date: Thu, 12 Nov 2026 21:46:03 +1100 Subject: [PATCH 2/3] test(forge): cover the PR, issue and commit-review fetchers FetchPullRequests against a fake GitHub Enterprise API: merged versus rejected state, the window ending the walk, review state casing, the author's own and bot reviews and comments excluded from first review and first response, draft intervals summed across cycles and a response before ready-for-review discarded. FetchIssues drops the pull requests the issues endpoint returns. FetchCommitReviews: approved on a PR into the default branch, self-approval as review_required, PRs into other branches and failed lookups as unknown. Coverage 37.6% -> 85.5%. Closes #218 --- internal/analyze/forge/auth_check_test.go | 4 +- internal/analyze/forge/fetch_test.go | 152 ++++++++++++++++++++++ 2 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 internal/analyze/forge/fetch_test.go diff --git a/internal/analyze/forge/auth_check_test.go b/internal/analyze/forge/auth_check_test.go index b9b352081c..95f58eb6a9 100644 --- a/internal/analyze/forge/auth_check_test.go +++ b/internal/analyze/forge/auth_check_test.go @@ -45,10 +45,10 @@ func (f *fakeGitHub) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } -// startFakeGitHub runs f over TLS and returns the host to pass to Check. The +// startFakeGitHub serves f over TLS and returns the host to pass to Check or NewClient. The // client Check builds uses http.DefaultTransport, which is pointed at the test // server's trusted transport for the duration of the test. -func startFakeGitHub(t *testing.T, f *fakeGitHub) string { +func startFakeGitHub(t *testing.T, f http.Handler) string { t.Helper() srv := httptest.NewTLSServer(f) t.Cleanup(srv.Close) diff --git a/internal/analyze/forge/fetch_test.go b/internal/analyze/forge/fetch_test.go new file mode 100644 index 0000000000..7a47526aad --- /dev/null +++ b/internal/analyze/forge/fetch_test.go @@ -0,0 +1,152 @@ +package forge + +import ( + "context" + "fmt" + "net/http" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// ghMux is a GitHub Enterprise API (under /api/v3/) answering fixed JSON per +// path. A path with no route is a 404. +func ghMux(routes map[string]string) http.Handler { + mux := http.NewServeMux() + for path, body := range routes { + body := body + mux.HandleFunc("/api/v3"+path, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = fmt.Fprint(w, body) + }) + } + return mux +} + +func fakeClient(t *testing.T, routes map[string]string) *Client { + t.Helper() + host := startFakeGitHub(t, ghMux(routes)) + c, err := NewClient(testCreds, host) + require.NoError(t, err) + return c +} + +var ( + t0 = time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC) + at = func(d time.Duration) string { return t0.Add(d).Format(time.RFC3339) } + fetchRp = Repo{Owner: "acme", Name: "widgets", DefaultBranch: "main"} +) + +func TestFetchPullRequestsHydratesStateReviewsAndResponses(t *testing.T) { + c := fakeClient(t, map[string]string{ + "/repos/acme/widgets/pulls": `[ + {"number":3,"title":"merged","html_url":"u3","user":{"login":"ann"},"created_at":"` + at(0) + `","updated_at":"` + at(72*time.Hour) + `","merged_at":"` + at(5*time.Hour) + `","closed_at":"` + at(5*time.Hour) + `","merged_by":{"login":"bob"},"additions":10,"deletions":2,"changed_files":1,"comments":2}, + {"number":2,"title":"rejected","user":{"login":"ann"},"created_at":"` + at(0) + `","updated_at":"` + at(48*time.Hour) + `","closed_at":"` + at(6*time.Hour) + `"}, + {"number":1,"title":"too old","user":{"login":"ann"},"created_at":"` + at(-200*time.Hour) + `","updated_at":"` + at(-100*time.Hour) + `"} + ]`, + "/repos/acme/widgets/issues/3/timeline": `[]`, + "/repos/acme/widgets/issues/2/timeline": `[]`, + "/repos/acme/widgets/pulls/3/reviews": `[ + {"user":{"login":"ann"},"state":"COMMENTED","submitted_at":"` + at(30*time.Minute) + `"}, + {"user":{"login":"ci","type":"Bot"},"state":"COMMENTED","submitted_at":"` + at(40*time.Minute) + `"}, + {"user":{"login":"bob"},"state":"APPROVED","submitted_at":"` + at(3*time.Hour) + `"} + ]`, + "/repos/acme/widgets/pulls/2/reviews": `[]`, + "/repos/acme/widgets/issues/3/comments": `[ + {"user":{"login":"ann"},"created_at":"` + at(10*time.Minute) + `"}, + {"user":{"login":"dependabot[bot]","type":"Bot"},"created_at":"` + at(20*time.Minute) + `"}, + {"user":{"login":"cid"},"created_at":"` + at(2*time.Hour) + `"} + ]`, + "/repos/acme/widgets/issues/2/comments": `[]`, + }) + + prs, err := c.FetchPullRequests(context.Background(), fetchRp, t0) + require.NoError(t, err) + require.Len(t, prs, 2, "the PR updated before the window ends the walk") + + merged := prs[0] + require.Equal(t, 3, merged.Number) + require.Equal(t, "merged", merged.State) + require.Equal(t, "bob", merged.MergedBy.Login) + require.Equal(t, 10, merged.Additions) + require.Len(t, merged.Reviews, 3) + require.Equal(t, "approved", merged.Reviews[2].State, "review states are lower-cased") + require.Equal(t, t0.Add(3*time.Hour), merged.FirstReviewAt.UTC(), "the author's own and a bot's review do not count") + require.Equal(t, t0.Add(2*time.Hour), merged.FirstResponseAt.UTC(), "the first non-author, non-bot comment came before the review") + require.Equal(t, t0, merged.ReadyForReviewAt.UTC(), "never a draft: ready when opened") + + rejected := prs[1] + require.Equal(t, "closed", rejected.State, "closed without merging is a rejection, not a merge") + require.True(t, rejected.MergedAt.IsZero()) + require.True(t, rejected.FirstResponseAt.IsZero()) +} + +func TestDraftHistorySumsEveryDraftInterval(t *testing.T) { + c := fakeClient(t, map[string]string{ + "/repos/acme/widgets/pulls": `[{"number":7,"draft":false,"user":{"login":"ann"},"created_at":"` + at(0) + `","updated_at":"` + at(10*time.Hour) + `"}]`, + "/repos/acme/widgets/issues/7/timeline": `[ + {"event":"convert_to_draft","created_at":"` + at(1*time.Hour) + `"}, + {"event":"ready_for_review","created_at":"` + at(2*time.Hour) + `"}, + {"event":"convert_to_draft","created_at":"` + at(3*time.Hour) + `"}, + {"event":"ready_for_review","created_at":"` + at(5*time.Hour) + `"} + ]`, + "/repos/acme/widgets/pulls/7/reviews": `[]`, + "/repos/acme/widgets/issues/7/comments": `[ + {"user":{"login":"bob"},"created_at":"` + at(90*time.Minute) + `"} + ]`, + }) + + prs, err := c.FetchPullRequests(context.Background(), fetchRp, t0) + require.NoError(t, err) + require.Len(t, prs, 1) + p := prs[0] + require.Equal(t, t0.Add(2*time.Hour), p.ReadyForReviewAt.UTC(), "ready at the first ready_for_review") + require.Equal(t, int((3 * time.Hour).Seconds()), p.TimeInDraftSeconds, "1h + 2h across both draft cycles") + require.True(t, p.FirstResponseAt.IsZero(), "a comment before the PR was ready is not a response to it") +} + +func TestFetchIssuesSkipsPullRequests(t *testing.T) { + c := fakeClient(t, map[string]string{ + "/repos/acme/widgets/issues": `[ + {"number":10,"title":"bug","state":"closed","state_reason":"completed","user":{"login":"ann"},"labels":[{"name":"bug"},{"name":"p1"}],"created_at":"` + at(0) + `","closed_at":"` + at(time.Hour) + `"}, + {"number":11,"title":"a PR","user":{"login":"ann"},"pull_request":{"url":"x"},"created_at":"` + at(0) + `"} + ]`, + "/repos/acme/widgets/issues/10/comments": `[{"user":{"login":"bob"},"created_at":"` + at(30*time.Minute) + `"}]`, + }) + + issues, err := c.FetchIssues(context.Background(), fetchRp, t0) + require.NoError(t, err) + require.Len(t, issues, 1, "the PR the issues endpoint returns is not an issue") + is := issues[0] + require.Equal(t, 10, is.Number) + require.Equal(t, "completed", is.StateReason) + require.Equal(t, []string{"bug", "p1"}, is.Labels) + require.Equal(t, t0.Add(30*time.Minute), is.FirstResponseAt.UTC()) +} + +func TestFetchCommitReviews(t *testing.T) { + c := fakeClient(t, map[string]string{ + // approved on a PR into main + "/repos/acme/widgets/commits/aaa/pulls": `[ + {"number":20,"html_url":"u20","user":{"login":"ann"},"base":{"ref":"release"}}, + {"number":21,"html_url":"u21","user":{"login":"ann"},"base":{"ref":"main"}} + ]`, + "/repos/acme/widgets/pulls/21/reviews": `[{"user":{"login":"bob"},"state":"APPROVED","submitted_at":"` + at(0) + `"}]`, + // a PR into main nobody but the author approved + "/repos/acme/widgets/commits/bbb/pulls": `[{"number":22,"user":{"login":"ann"},"base":{"ref":"main"}}]`, + "/repos/acme/widgets/pulls/22/reviews": `[{"user":{"login":"ann"},"state":"APPROVED","submitted_at":"` + at(0) + `"}]`, + // no PR into main at all + "/repos/acme/widgets/commits/ccc/pulls": `[{"number":23,"user":{"login":"ann"},"base":{"ref":"release"}}]`, + // "ddd" has no route: the lookup fails and the commit is unknown + }) + + got, err := c.FetchCommitReviews(context.Background(), fetchRp, []string{"aaa", "bbb", "ccc", "ddd"}) + require.NoError(t, err) + require.Equal(t, []CommitReview{ + {SHA: "aaa", PRNumber: 21, PRURL: "u21", Status: "approved"}, + {SHA: "bbb", PRNumber: 22, Status: "review_required"}, + {SHA: "ccc", Status: "unknown"}, + {SHA: "ddd", Status: "unknown"}, + }, got) +} From 45f24c77cfc761fc01692bb5bdb544e90f68799b Mon Sep 17 00:00:00 2001 From: Christopher Langton Date: Thu, 12 Nov 2026 21:46:38 +1100 Subject: [PATCH 3/3] test(forge): cover the gh CLI credential fallback A stub gh on PATH: its trimmed token is used with source "gh CLI"; a failing gh, a blank answer and no gh at all give ErrNoCredentials. --- internal/analyze/forge/fetch_test.go | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/internal/analyze/forge/fetch_test.go b/internal/analyze/forge/fetch_test.go index 7a47526aad..d69b7c0191 100644 --- a/internal/analyze/forge/fetch_test.go +++ b/internal/analyze/forge/fetch_test.go @@ -4,6 +4,9 @@ import ( "context" "fmt" "net/http" + "os" + "path/filepath" + "runtime" "testing" "time" @@ -150,3 +153,38 @@ func TestFetchCommitReviews(t *testing.T) { {SHA: "ddd", Status: "unknown"}, }, got) } + +// fakeGh puts a gh on PATH that runs script. +func fakeGh(t *testing.T, script string) { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "gh"), []byte("#!/bin/sh\n"+script+"\n"), 0o755)) + t.Setenv("PATH", dir) +} + +func TestResolveCredentials_GhCLI(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses a shell script as gh") + } + t.Setenv("GITHUB_TOKEN", "") + t.Setenv("GH_TOKEN", "") + + fakeGh(t, `[ "$1 $2" = "auth token" ] && echo " from-gh-cli "`) + creds, err := ResolveCredentials(context.Background()) + require.NoError(t, err) + require.Equal(t, "from-gh-cli", creds.Token, "gh's output is trimmed") + require.Equal(t, "gh CLI", creds.Source) + + for name, script := range map[string]string{ + "gh fails": "exit 1", + "gh prints blank": "echo", + } { + fakeGh(t, script) + _, err := ResolveCredentials(context.Background()) + require.ErrorIs(t, err, ErrNoCredentials, name) + } + + t.Setenv("PATH", t.TempDir()) // no gh at all + _, err = ResolveCredentials(context.Background()) + require.ErrorIs(t, err, ErrNoCredentials, "gh absent") +}