more code reorganization

Author: Thomas Stromberg

cmd/prx/main.go

@@ -115,7 +115,8 @@ func githubToken() (string, error) {
 	return token, nil
 }
 
-func parsePRURL(prURL string) (owner, repo string, prNumber int, err error) { //nolint:revive // Function needs all 4 return values
+//nolint:revive // function-result-limit: function needs all 4 return values
+func parsePRURL(prURL string) (owner, repo string, prNumber int, err error) {
 	u, err := url.Parse(prURL)
 	if err != nil {
 		return "", "", 0, err

pkg/prx/check_run_history_test.go

@@ -164,11 +164,7 @@ func TestCheckRunHistory_MultipleCommits(t *testing.T) {
 
 	httpClient := &http.Client{Transport: http.DefaultTransport}
 	client := NewClient("test-token", WithHTTPClient(httpClient))
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	prData, err := client.PullRequest(ctx, "codeGROOVE-dev", "slacker", 66)
@@ -319,11 +315,7 @@ func TestCheckRunHistory_CommitSHAPreservation(t *testing.T) {
 
 	httpClient := &http.Client{Transport: http.DefaultTransport}
 	client := NewClient("test-token", WithHTTPClient(httpClient))
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	prData, err := client.PullRequest(ctx, "testowner", "testrepo", 100)

pkg/prx/checks_test.go

@@ -127,11 +127,7 @@ func TestClient_PullRequestWithCheckRuns(t *testing.T) {
 
 	httpClient := &http.Client{Transport: http.DefaultTransport}
 	client := NewClient("test-token", WithHTTPClient(httpClient))
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	prData, err := client.PullRequest(ctx, "testowner", "testrepo", 555)
@@ -243,11 +239,7 @@ func TestClient_PullRequestWithBranchProtection(t *testing.T) {
 
 	httpClient := &http.Client{Transport: http.DefaultTransport}
 	client := NewClient("test-token", WithHTTPClient(httpClient))
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	prData, err := client.PullRequest(ctx, "testowner", "testrepo", 666)

pkg/prx/client.go

@@ -7,7 +7,6 @@ import (
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"log/slog"
@@ -18,6 +17,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/codeGROOVE-dev/prx/pkg/prx/github"
 	"github.com/codeGROOVE-dev/sfcache"
 	"github.com/codeGROOVE-dev/sfcache/pkg/store/localfs"
 )
@@ -39,11 +39,7 @@ type PRStore = sfcache.Store[string, PullRequestData]
 
 // Client provides methods to fetch GitHub pull request events.
 type Client struct {
-	github interface {
-		get(ctx context.Context, path string, v any) (*githubResponse, error)
-		raw(ctx context.Context, path string) (json.RawMessage, *githubResponse, error)
-		collaborators(ctx context.Context, owner, repo string) (map[string]string, error)
-	}
+	github             *github.Client
 	logger             *slog.Logger
 	collaboratorsCache *sfcache.MemoryCache[string, map[string]string]
 	prCache            *sfcache.TieredCache[string, PullRequestData]
@@ -65,11 +61,11 @@ func WithHTTPClient(httpClient *http.Client) Option {
 	return func(c *Client) {
 		// Wrap the transport with retry logic if not already wrapped
 		if httpClient.Transport == nil {
-			httpClient.Transport = &RetryTransport{Base: http.DefaultTransport}
-		} else if _, ok := httpClient.Transport.(*RetryTransport); !ok {
-			httpClient.Transport = &RetryTransport{Base: httpClient.Transport}
+			httpClient.Transport = &github.Transport{Base: http.DefaultTransport}
+		} else if _, ok := httpClient.Transport.(*github.Transport); !ok {
+			httpClient.Transport = &github.Transport{Base: httpClient.Transport}
 		}
-		c.github = &githubClient{client: httpClient, token: c.token, api: githubAPI}
+		c.github = newGitHubClient(httpClient, c.token, github.API)
 	}
 }
 
@@ -102,14 +98,14 @@ func NewClient(token string, opts ...Option) *Client {
 		logger:             slog.Default(),
 		token:              token,
 		collaboratorsCache: sfcache.New[string, map[string]string](sfcache.TTL(collaboratorsCacheTTL)),
-		github: &githubClient{
-			client: &http.Client{
-				Transport: &RetryTransport{Base: transport},
+		github: newGitHubClient(
+			&http.Client{
+				Transport: &github.Transport{Base: transport},
 				Timeout:   30 * time.Second,
 			},
-			token: token,
-			api:   githubAPI,
-		},
+			token,
+			github.API,
+		),
 	}
 
 	for _, opt := range opts {

pkg/prx/client_cache_test.go

@@ -96,9 +96,7 @@ func TestCacheClient(t *testing.T) {
 	}()
 
 	// Override the GitHub client to use test server
-	if gc, ok := client.github.(*githubClient); ok {
-		gc.api = server.URL
-	}
+	client.github = newTestGitHubClient(&http.Client{Transport: &http.Transport{}}, "test-token", server.URL)
 
 	ctx := context.Background()
 

pkg/prx/client_graphql.go

@@ -7,6 +7,8 @@ import (
 	"sort"
 	"strings"
 	"time"
+
+	"github.com/codeGROOVE-dev/prx/pkg/prx/github"
 )
 
 // pullRequestViaGraphQL fetches pull request data using GraphQL with minimal REST fallbacks.
@@ -79,31 +81,31 @@ func (c *Client) pullRequestViaGraphQL(ctx context.Context, owner, repo string,
 
 // fetchRulesetsREST fetches repository rulesets via REST API (not available in GraphQL).
 func (c *Client) fetchRulesetsREST(ctx context.Context, owner, repo string) ([]string, error) {
-	rulesetPath := fmt.Sprintf("/repos/%s/%s/rulesets", owner, repo)
-	var rulesets []githubRuleset
+	path := fmt.Sprintf("/repos/%s/%s/rulesets", owner, repo)
+	var rulesets []github.Ruleset
 
-	_, err := c.github.get(ctx, rulesetPath, &rulesets)
-	if err != nil {
+	if _, err := c.github.Get(ctx, path, &rulesets); err != nil {
 		return nil, err
 	}
 
-	var requiredChecks []string
-	for _, ruleset := range rulesets {
-		if ruleset.Target == "branch" {
-			for _, rule := range ruleset.Rules {
-				if rule.Type == "required_status_checks" && rule.Parameters.RequiredStatusChecks != nil {
-					for _, check := range rule.Parameters.RequiredStatusChecks {
-						requiredChecks = append(requiredChecks, check.Context)
-					}
+	var required []string
+	for _, rs := range rulesets {
+		if rs.Target != "branch" {
+			continue
+		}
+		for _, rule := range rs.Rules {
+			if rule.Type == "required_status_checks" && rule.Parameters.RequiredStatusChecks != nil {
+				for _, chk := range rule.Parameters.RequiredStatusChecks {
+					required = append(required, chk.Context)
 				}
 			}
 		}
 	}
 
 	c.logger.InfoContext(ctx, "fetched required checks from rulesets",
-		"count", len(requiredChecks), "checks", requiredChecks)
+		"count", len(required), "checks", required)
 
-	return requiredChecks, nil
+	return required, nil
 }
 
 // fetchCheckRunsREST fetches check runs via REST API for a specific commit.
@@ -113,8 +115,8 @@ func (c *Client) fetchCheckRunsREST(ctx context.Context, owner, repo, sha string
 	}
 
 	path := fmt.Sprintf("/repos/%s/%s/commits/%s/check-runs?per_page=100", owner, repo, sha)
-	var checkRuns githubCheckRuns
-	if _, err := c.github.get(ctx, path, &checkRuns); err != nil {
+	var checkRuns github.CheckRuns
+	if _, err := c.github.Get(ctx, path, &checkRuns); err != nil {
 		return nil, fmt.Errorf("fetching check runs: %w", err)
 	}
 
@@ -172,33 +174,26 @@ func (c *Client) fetchCheckRunsREST(ctx context.Context, owner, repo, sha string
 // Errors fetching individual commits are logged but don't stop the overall process.
 func (c *Client) fetchAllCheckRunsREST(ctx context.Context, owner, repo string, prData *PullRequestData) []Event {
 	// Collect all unique commit SHAs from the PR
-	commitSHAs := make([]string, 0)
+	shas := make(map[string]bool)
 
 	// Add HEAD SHA (most important)
 	if prData.PullRequest.HeadSHA != "" {
-		commitSHAs = append(commitSHAs, prData.PullRequest.HeadSHA)
+		shas[prData.PullRequest.HeadSHA] = true
 	}
 
 	// Add all other commit SHAs from commit events
 	for i := range prData.Events {
 		e := &prData.Events[i]
 		if e.Kind == "commit" && e.Body != "" {
-			// Body contains the commit SHA for commit events
-			commitSHAs = append(commitSHAs, e.Body)
+			shas[e.Body] = true
 		}
 	}
 
-	// Deduplicate SHAs (in case HEAD is also in commit events)
-	uniqueSHAs := make(map[string]bool)
-	for _, sha := range commitSHAs {
-		uniqueSHAs[sha] = true
-	}
-
 	// Fetch check runs for each unique commit
-	var allEvents []Event
-	seenCheckRuns := make(map[string]bool) // Track unique check runs by "name:timestamp"
+	var all []Event
+	seen := make(map[string]bool) // Track unique check runs by "name:timestamp"
 
-	for sha := range uniqueSHAs {
+	for sha := range shas {
 		events, err := c.fetchCheckRunsREST(ctx, owner, repo, sha)
 		if err != nil {
 			c.logger.WarnContext(ctx, "failed to fetch check runs for commit", "sha", sha, "error", err)
@@ -207,19 +202,17 @@ func (c *Client) fetchAllCheckRunsREST(ctx context.Context, owner, repo string,
 
 		// Add only unique check runs (same check can run on multiple commits)
 		for i := range events {
-			event := &events[i]
-			// Create a unique key based on check name and timestamp
-			key := fmt.Sprintf("%s:%s", event.Body, event.Timestamp.Format(time.RFC3339Nano))
-			if !seenCheckRuns[key] {
-				seenCheckRuns[key] = true
-				// Add the commit SHA to the Target field
-				event.Target = sha
-				allEvents = append(allEvents, *event)
+			ev := &events[i]
+			key := fmt.Sprintf("%s:%s", ev.Body, ev.Timestamp.Format(time.RFC3339Nano))
+			if !seen[key] {
+				seen[key] = true
+				ev.Target = sha
+				all = append(all, *ev)
 			}
 		}
 	}
 
-	return allEvents
+	return all
 }
 
 // existingRequiredChecks extracts required checks that were already identified.
@@ -228,19 +221,17 @@ func (*Client) existingRequiredChecks(prData *PullRequestData) []string {
 
 	// Extract from existing events that are marked as required
 	for i := range prData.Events {
-		event := &prData.Events[i]
-		if event.Required && (event.Kind == "check_run" || event.Kind == "status_check") {
-			required = append(required, event.Body)
+		e := &prData.Events[i]
+		if e.Required && (e.Kind == "check_run" || e.Kind == "status_check") {
+			required = append(required, e.Body)
 		}
 	}
 
 	// Also extract from pending checks in check summary (these are required but haven't run)
 	if prData.PullRequest.CheckSummary != nil {
-		for check := range prData.PullRequest.CheckSummary.Pending {
-			// Check if it's not already in the list
-			found := slices.Contains(required, check)
-			if !found {
-				required = append(required, check)
+		for chk := range prData.PullRequest.CheckSummary.Pending {
+			if !slices.Contains(required, chk) {
+				required = append(required, chk)
 			}
 		}
 	}
@@ -252,16 +243,16 @@ func (*Client) existingRequiredChecks(prData *PullRequestData) []string {
 // This recalculates the entire check summary from ALL events to ensure we have the latest state.
 func (c *Client) recalculateCheckSummaryWithCheckRuns(_ /* ctx */ context.Context, prData *PullRequestData, _ /* checkRunEvents */ []Event) {
 	// Get existing required checks before we overwrite the summary
-	var requiredChecks []string
+	var required []string
 	if prData.PullRequest.CheckSummary != nil {
-		for check := range prData.PullRequest.CheckSummary.Pending {
-			requiredChecks = append(requiredChecks, check)
+		for chk := range prData.PullRequest.CheckSummary.Pending {
+			required = append(required, chk)
 		}
 	}
 
 	// Recalculate the entire check summary from ALL events (including the new check runs)
 	// This ensures we get the latest state based on timestamps
-	prData.PullRequest.CheckSummary = calculateCheckSummary(prData.Events, requiredChecks)
+	prData.PullRequest.CheckSummary = calculateCheckSummary(prData.Events, required)
 
 	// Update test state based on the recalculated check summary
 	prData.PullRequest.TestState = c.calculateTestStateFromCheckSummary(prData.PullRequest.CheckSummary)

pkg/prx/client_pullrequest_test.go

@@ -61,11 +61,7 @@ func TestClient_PullRequest(t *testing.T) {
 	client := NewClient("test-token", WithHTTPClient(httpClient))
 
 	// Override the API URL to point to our test server
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	prData, err := client.PullRequest(ctx, "testowner", "testrepo", 123)
@@ -142,11 +138,7 @@ func TestClient_PullRequestWithCache(t *testing.T) {
 	client := NewClient("test-token", WithCacheStore(store), WithHTTPClient(httpClient))
 
 	// Override the API URL
-	client.github = &githubClient{
-		client: httpClient,
-		token:  "test-token",
-		api:    server.URL,
-	}
+	client.github = newTestGitHubClient(httpClient, "test-token", server.URL)
 
 	ctx := context.Background()
 	refTime := time.Now()