From 3ff013d4894ccbc6be425cd2ab61869ba6233b06 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 6 Jan 2026 11:18:06 -0500 Subject: [PATCH 1/3] upgrade go mods --- go.mod | 8 ++++---- go.sum | 8 ++++++++ 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index e929a97..7add07d 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,9 @@ module github.com/codeGROOVE-dev/sprinkler go 1.24.0 require ( - github.com/codeGROOVE-dev/retry v1.2.0 - golang.org/x/crypto v0.43.0 - golang.org/x/net v0.46.0 + github.com/codeGROOVE-dev/retry v1.3.1 + golang.org/x/crypto v0.46.0 + golang.org/x/net v0.48.0 ) -require golang.org/x/text v0.30.0 // indirect +require golang.org/x/text v0.32.0 // indirect diff --git a/go.sum b/go.sum index 86e9c01..1213994 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,16 @@ github.com/codeGROOVE-dev/retry v1.2.0 h1:xYpYPX2PQZmdHwuiQAGGzsBm392xIMl4nfMEFApQnu8= github.com/codeGROOVE-dev/retry v1.2.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= +github.com/codeGROOVE-dev/retry v1.3.1 h1:BAkfDzs6FssxLCGWGgM97bb+6/8GTa40Cs147vXkJOg= +github.com/codeGROOVE-dev/retry v1.3.1/go.mod h1:+b3huqYGY1+ZJyuCmR8nBVLjd3WJ7qAFss+sI4s6FSc= golang.org/x/crypto v0.43.0 h1:dduJYIi3A3KOfdGOHX8AVZ/jGiyPa3IbBozJ5kNuE04= golang.org/x/crypto v0.43.0/go.mod h1:BFbav4mRNlXJL4wNeejLpWxB7wMbc79PdRGhWKncxR0= +golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= +golang.org/x/crypto v0.46.0/go.mod h1:Evb/oLKmMraqjZ2iQTwDwvCtJkczlDuTmdJXoZVzqU0= golang.org/x/net v0.46.0 h1:giFlY12I07fugqwPuWJi68oOnpfqFnJIJzaIIm2JVV4= golang.org/x/net v0.46.0/go.mod h1:Q9BGdFy1y4nkUwiLvT5qtyhAnEHgnQ/zd8PfU6nc210= +golang.org/x/net v0.48.0 h1:zyQRTTrjc33Lhh0fBgT/H3oZq9WuvRR5gPC70xpDiQU= +golang.org/x/net v0.48.0/go.mod h1:+ndRgGjkh8FGtu1w1FGbEC31if4VrNVMuKTgcAAnQRY= golang.org/x/text v0.30.0 h1:yznKA/E9zq54KzlzBEAWn1NXSQ8DIp/NYMy88xJjl4k= golang.org/x/text v0.30.0/go.mod h1:yDdHFIX9t+tORqspjENWgzaCVXgk0yYnYuSZ8UzzBVM= +golang.org/x/text v0.32.0 h1:ZD01bjUt1FQ9WJ0ClOL5vxgxOI/sVCNgX1YtKwcY0mU= +golang.org/x/text v0.32.0/go.mod h1:o/rUWzghvpD5TXrTIBuJU77MTaN0ljMWE47kxGJQ7jY= From a6ea31a649116d8d99269fe3aa870dcaa1076878 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 6 Jan 2026 19:56:47 -0500 Subject: [PATCH 2/3] allow slog context to be passed in --- go.mod | 8 +- go.sum | 4 + pkg/client/client.go | 2 +- pkg/github/client.go | 123 ++++++++------- pkg/github/client_test.go | 102 ++++++------ pkg/srv/commit_cache.go | 101 ++++++++++++ pkg/srv/commit_cache_test.go | 160 +++++++++++++++++++ pkg/srv/hub.go | 19 ++- pkg/srv/websocket.go | 2 +- pkg/webhook/handler.go | 295 +++++++++++++++++++---------------- pkg/webhook/handler_test.go | 40 ----- 11 files changed, 562 insertions(+), 294 deletions(-) create mode 100644 pkg/srv/commit_cache.go create mode 100644 pkg/srv/commit_cache_test.go diff --git a/go.mod b/go.mod index 7add07d..c4c9a66 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/codeGROOVE-dev/sprinkler -go 1.24.0 +go 1.25.4 require ( github.com/codeGROOVE-dev/retry v1.3.1 @@ -8,4 +8,8 @@ require ( golang.org/x/net v0.48.0 ) -require golang.org/x/text v0.32.0 // indirect +require ( + github.com/codeGROOVE-dev/fido v1.10.0 // indirect + github.com/puzpuzpuz/xsync/v4 v4.2.0 // indirect + golang.org/x/text v0.32.0 // indirect +) diff --git a/go.sum b/go.sum index 1213994..9964b7c 100644 --- a/go.sum +++ b/go.sum @@ -1,7 +1,11 @@ +github.com/codeGROOVE-dev/fido v1.10.0 h1:i4Wb6LDd5nD/4Fnp47KAVUVhG1O1mN5jSRbCYPpBYjw= +github.com/codeGROOVE-dev/fido v1.10.0/go.mod h1:/mqfMeKCTYTGt/Y0cWm6gh8gYBKG1w8xBsTDmu+A/pU= github.com/codeGROOVE-dev/retry v1.2.0 h1:xYpYPX2PQZmdHwuiQAGGzsBm392xIMl4nfMEFApQnu8= github.com/codeGROOVE-dev/retry v1.2.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= github.com/codeGROOVE-dev/retry v1.3.1 h1:BAkfDzs6FssxLCGWGgM97bb+6/8GTa40Cs147vXkJOg= github.com/codeGROOVE-dev/retry v1.3.1/go.mod h1:+b3huqYGY1+ZJyuCmR8nBVLjd3WJ7qAFss+sI4s6FSc= +github.com/puzpuzpuz/xsync/v4 v4.2.0 h1:dlxm77dZj2c3rxq0/XNvvUKISAmovoXF4a4qM6Wvkr0= +github.com/puzpuzpuz/xsync/v4 v4.2.0/go.mod h1:VJDmTCJMBt8igNxnkQd86r+8KUeN1quSfNKu5bLYFQo= golang.org/x/crypto v0.43.0 h1:dduJYIi3A3KOfdGOHX8AVZ/jGiyPa3IbBozJ5kNuE04= golang.org/x/crypto v0.43.0/go.mod h1:BFbav4mRNlXJL4wNeejLpWxB7wMbc79PdRGhWKncxR0= golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= diff --git a/pkg/client/client.go b/pkg/client/client.go index 7184867..b079a83 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -757,7 +757,7 @@ func (c *Client) readEvents(ctx context.Context, ws *websocket.Conn) error { "type", event.Type, "cache_hit", false) - gh := github.NewClient(c.config.Token) + gh := github.NewClient(c.config.Token, c.logger) var err error prs, err = gh.FindPRsForCommit(ctx, owner, repo, event.CommitSHA) if err != nil { diff --git a/pkg/github/client.go b/pkg/github/client.go index 9f06818..5cd8a74 100644 --- a/pkg/github/client.go +++ b/pkg/github/client.go @@ -8,7 +8,7 @@ import ( "errors" "fmt" "io" - "log" + "log/slog" "net/http" "strings" "time" @@ -23,16 +23,22 @@ const ( // Client provides GitHub API functionality. type Client struct { httpClient *http.Client + logger *slog.Logger token string } // NewClient creates a new GitHub API client with the provided token. -func NewClient(token string) *Client { +// If logger is nil, a default discarding logger is used. +func NewClient(token string, logger *slog.Logger) *Client { + if logger == nil { + logger = slog.New(slog.DiscardHandler) + } return &Client{ httpClient: &http.Client{ Timeout: clientTimeout, }, - token: token, + token: token, + logger: logger, } } @@ -71,12 +77,12 @@ func (c *Client) AuthenticatedUser(ctx context.Context) (*User, error) { resp, err := c.httpClient.Do(req) if err != nil { lastErr = fmt.Errorf("failed to make request: %w", err) - log.Printf("GitHub API request failed (will retry): %v", err) + c.logger.Warn("GitHub API request failed (will retry)", "error", err) return err // Retry on network errors } defer func() { if err := resp.Body.Close(); err != nil { - log.Printf("failed to close response body: %v", err) + c.logger.Warn("failed to close response body", "error", err) } }() @@ -102,24 +108,24 @@ func (c *Client) AuthenticatedUser(ctx context.Context) (*User, error) { case http.StatusUnauthorized: // Don't retry on auth failures - log.Print("GitHub API: 401 Unauthorized - invalid token for /user endpoint") + c.logger.Warn("GitHub API: 401 Unauthorized - invalid token for /user endpoint") return retry.Unrecoverable(errors.New("invalid GitHub token")) case http.StatusForbidden: // Check if rate limited if resp.Header.Get("X-RateLimit-Remaining") == "0" { //nolint:canonicalheader // GitHub API header resetTime := resp.Header.Get("X-RateLimit-Reset") //nolint:canonicalheader // GitHub API header - log.Printf("GitHub API: 403 Forbidden - rate limit exceeded for /user endpoint, reset at %s", resetTime) + c.logger.Warn("GitHub API: 403 Forbidden - rate limit exceeded for /user endpoint", "reset_at", resetTime) lastErr = errors.New("GitHub API rate limit exceeded") return lastErr // Retry after backoff } - log.Print("GitHub API: 403 Forbidden - access denied for /user endpoint") + c.logger.Warn("GitHub API: 403 Forbidden - access denied for /user endpoint") return retry.Unrecoverable(errors.New("access forbidden")) case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable: // Retry on server errors lastErr = fmt.Errorf("GitHub API server error: %d", resp.StatusCode) - log.Printf("GitHub API server error %d (will retry)", resp.StatusCode) + c.logger.Warn("GitHub API server error (will retry)", "status", resp.StatusCode) return lastErr default: @@ -164,12 +170,12 @@ func (c *Client) AppInstallationInfo(ctx context.Context) (*AppInstallation, err resp, err := c.httpClient.Do(req) if err != nil { lastErr = fmt.Errorf("failed to make request: %w", err) - log.Printf("GitHub API: /installation/repositories request failed (will retry): %v", err) + c.logger.Warn("GitHub API: /installation/repositories request failed (will retry)", "error", err) return err // Retry on network errors } defer func() { if err := resp.Body.Close(); err != nil { - log.Printf("failed to close response body: %v", err) + c.logger.Warn("failed to close response body", "error", err) } }() @@ -186,8 +192,8 @@ func (c *Client) AppInstallationInfo(ctx context.Context) (*AppInstallation, err if len(bodySnippet) > 200 { bodySnippet = bodySnippet[:200] + "..." } - log.Printf("GitHub API DEBUG: /installation/repositories returned status=%d, body=%s", - resp.StatusCode, bodySnippet) + c.logger.Debug("GitHub API: /installation/repositories returned non-OK", + "status", resp.StatusCode, "body", bodySnippet) } // Handle status codes @@ -234,7 +240,7 @@ func (c *Client) AppInstallationInfo(ctx context.Context) (*AppInstallation, err case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable: // Retry on server errors lastErr = fmt.Errorf("GitHub API server error: %d", resp.StatusCode) - log.Printf("GitHub API: /installation server error %d (will retry)", resp.StatusCode) + c.logger.Warn("GitHub API: /installation server error (will retry)", "status", resp.StatusCode) return lastErr default: @@ -254,8 +260,8 @@ func (c *Client) AppInstallationInfo(ctx context.Context) (*AppInstallation, err return nil, err } - log.Printf("GitHub API: App installation detected - Account: %s (%s)", - installation.Account.Login, installation.Account.Type) + c.logger.Info("GitHub API: App installation detected", + "account", installation.Account.Login, "type", installation.Account.Type) return installation, nil } @@ -279,32 +285,32 @@ func (c *Client) UserAndOrgs(ctx context.Context) (username string, orgs []strin tokenType = "unknown" } - log.Printf("GitHub API: Starting authentication (token_type=%s)", tokenType) + c.logger.Info("GitHub API: Starting authentication", "token_type", tokenType) // First, try to detect if this is a GitHub App token by checking for /installation/repositories endpoint // Try for all tokens, not just ghs_ prefix - let the API tell us what it is - log.Print("GitHub API: Checking if token works with /installation/repositories endpoint...") + c.logger.Debug("GitHub API: Checking if token works with /installation/repositories endpoint...") installation, appErr := c.AppInstallationInfo(ctx) if appErr == nil { // This is a GitHub App - return the org it's installed in if installation.Account.Type == "Organization" { - log.Printf("GitHub API: SUCCESS - App installation token authenticated for organization: %s", installation.Account.Login) + c.logger.Info("GitHub API: App installation token authenticated for organization", "org", installation.Account.Login) return "app[installation]", []string{installation.Account.Login}, nil } // App installed on user account - treat the personal account as an "org" for subscription purposes - log.Printf("GitHub API: SUCCESS - App installation token authenticated for user account: %s", installation.Account.Login) + c.logger.Info("GitHub API: App installation token authenticated for user account", "account", installation.Account.Login) return "app[installation]", []string{installation.Account.Login}, nil } - log.Printf("GitHub API: Token did not work with /installation/repositories (error: %v), trying /user endpoint...", appErr) + c.logger.Debug("GitHub API: Token did not work with /installation/repositories, trying /user endpoint...", "error", appErr) // Fall back to user authentication - log.Print("GitHub API: Getting authenticated user info...") + c.logger.Debug("GitHub API: Getting authenticated user info...") user, err := c.AuthenticatedUser(ctx) if err != nil { - log.Printf("GitHub API: Failed to get authenticated user: %v", err) + c.logger.Warn("GitHub API: Failed to get authenticated user", "error", err) return "", nil, fmt.Errorf("failed to get authenticated user: %w", err) } - log.Printf("GitHub API: Successfully authenticated as user '%s'", user.Login) + c.logger.Info("GitHub API: Successfully authenticated as user", "user", user.Login) // Get user's organizations orgList, err := c.userOrganizations(ctx) @@ -318,7 +324,7 @@ func (c *Client) UserAndOrgs(ctx context.Context) (username string, orgs []strin orgNames[i] = o.Login } - log.Printf("GitHub API: User '%s' is member of %d organizations", user.Login, len(orgList)) + c.logger.Info("GitHub API: User organizations loaded", "user", user.Login, "org_count", len(orgList)) return user.Login, orgNames, nil } @@ -332,7 +338,7 @@ func (c *Client) userOrganizations(ctx context.Context) ([]Organization, error) var orgs []Organization var lastErr error - log.Print("GitHub API: Fetching user's organizations...") + c.logger.Debug("GitHub API: Fetching user's organizations...") // Retry org membership check with exponential backoff err := retry.Do( @@ -349,12 +355,12 @@ func (c *Client) userOrganizations(ctx context.Context) ([]Organization, error) resp, err := c.httpClient.Do(req) if err != nil { lastErr = fmt.Errorf("failed to make request: %w", err) - log.Printf("GitHub API org fetch failed (will retry): %v", err) + c.logger.Warn("GitHub API org fetch failed (will retry)", "error", err) return err // Retry on network errors } defer func() { if err := resp.Body.Close(); err != nil { - log.Printf("failed to close response body: %v", err) + c.logger.Warn("failed to close response body", "error", err) } }() @@ -373,24 +379,24 @@ func (c *Client) userOrganizations(ctx context.Context) ([]Organization, error) return nil case http.StatusUnauthorized: - log.Print("GitHub API: 401 Unauthorized - invalid token for /user/orgs endpoint") + c.logger.Warn("GitHub API: 401 Unauthorized - invalid token for /user/orgs endpoint") return retry.Unrecoverable(errors.New("invalid GitHub token")) case http.StatusForbidden: // Check if it's a rate limit issue if resp.Header.Get("X-Ratelimit-Remaining") == "0" { resetTime := resp.Header.Get("X-Ratelimit-Reset") - log.Printf("GitHub API: 403 Forbidden - rate limit exceeded for /user/orgs endpoint, reset at %s", resetTime) + c.logger.Warn("GitHub API: 403 Forbidden - rate limit exceeded for /user/orgs endpoint", "reset_at", resetTime) lastErr = errors.New("GitHub API rate limit exceeded") return lastErr // Retry after backoff } - log.Print("GitHub API: 403 Forbidden - access denied for /user/orgs endpoint") + c.logger.Warn("GitHub API: 403 Forbidden - access denied for /user/orgs endpoint") return retry.Unrecoverable(errors.New("access forbidden")) case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable: // Retry on server errors lastErr = fmt.Errorf("GitHub API server error: %d", resp.StatusCode) - log.Printf("GitHub API server error %d (will retry)", resp.StatusCode) + c.logger.Warn("GitHub API server error (will retry)", "status", resp.StatusCode) return lastErr default: @@ -415,7 +421,7 @@ func (c *Client) userOrganizations(ctx context.Context) ([]Organization, error) // ValidateOrgMembership checks if the authenticated user has access to the specified organization. // Returns the authenticated user's username, list of all their organizations, and nil error if successful. func (c *Client) ValidateOrgMembership(ctx context.Context, org string) (username string, orgs []string, err error) { - log.Printf("GitHub API: Starting authentication and org membership validation for org '%s'", org) + c.logger.Debug("GitHub API: Starting authentication and org membership validation", "org", org) // Sanitize org name org = strings.TrimSpace(org) @@ -439,15 +445,13 @@ func (c *Client) ValidateOrgMembership(ctx context.Context, org string) (usernam // Check if the requested organization is in the user's membership list for _, userOrg := range orgNames { if strings.EqualFold(userOrg, org) { - log.Printf("GitHub API: User '%s' is a member of organization '%s'", username, org) - log.Printf("GitHub API: User is member of %d total organizations", len(orgNames)) + c.logger.Info("GitHub API: User is a member of organization", "user", username, "org", org, "total_orgs", len(orgNames)) return username, orgNames, nil } } // User is not a member of the requested organization - log.Printf("GitHub API: User '%s' is NOT a member of organization '%s'", username, org) - log.Printf("GitHub API: User is member of %d organizations: %v", len(orgNames), orgNames) + c.logger.Warn("GitHub API: User is NOT a member of organization", "user", username, "org", org, "member_orgs", orgNames) return username, orgNames, errors.New("user is not a member of the requested organization") } @@ -467,7 +471,7 @@ func (c *Client) FindPRsForCommit(ctx context.Context, owner, repo, commitSHA st // Use GitHub's API to list PRs associated with a commit url := fmt.Sprintf("https://api.github.com/repos/%s/%s/commits/%s/pulls", owner, repo, commitSHA) - log.Printf("GitHub API: Looking up PRs for commit %s in %s/%s", commitSHA[:8], owner, repo) + c.logger.Debug("GitHub API: Looking up PRs for commit", "commit", commitSHA[:8], "owner", owner, "repo", repo) err := retry.Do( func() error { @@ -481,16 +485,16 @@ func (c *Client) FindPRsForCommit(ctx context.Context, owner, repo, commitSHA st req.Header.Set("Accept", "application/vnd.github.v3+json") req.Header.Set("User-Agent", "webhook-sprinkler/1.0") - log.Printf("GitHub API: GET %s (attempt %d)", url, attemptNum) + c.logger.Debug("GitHub API: GET", "url", url, "attempt", attemptNum) resp, err := c.httpClient.Do(req) if err != nil { lastErr = fmt.Errorf("failed to make request: %w", err) - log.Printf("GitHub API request failed (will retry): %v", err) + c.logger.Warn("GitHub API request failed (will retry)", "error", err) return err // Retry on network errors } defer func() { if err := resp.Body.Close(); err != nil { - log.Printf("failed to close response body: %v", err) + c.logger.Warn("failed to close response body", "error", err) } }() @@ -517,45 +521,50 @@ func (c *Client) FindPRsForCommit(ctx context.Context, owner, repo, commitSHA st prNumbers[i] = pr.Number } - // If empty on first attempt, retry once after short delay - // This handles GitHub's indexing race condition - if len(prNumbers) == 0 && attemptNum == 1 { - log.Printf("GitHub API: Empty result on first attempt for commit %s - will retry once (race condition?)", commitSHA[:8]) - time.Sleep(500 * time.Millisecond) - return errors.New("empty result on first attempt, retrying") - } - + // Handle empty results with progressive backoff + // GitHub's indexing can take a moment after PR events if len(prNumbers) == 0 { - log.Printf("GitHub API: Empty result for commit %s after %d attempts - "+ - "may be push to main or PR not yet indexed", commitSHA[:8], attemptNum) + switch attemptNum { + case 1: + c.logger.Info("GitHub API: Empty result on attempt 1 - retrying after 500ms", "commit", commitSHA[:8]) + time.Sleep(500 * time.Millisecond) + return errors.New("empty result, retrying") + case 2: + c.logger.Info("GitHub API: Empty result on attempt 2 - retrying after 1s", "commit", commitSHA[:8]) + time.Sleep(1 * time.Second) + return errors.New("empty result, retrying") + default: + c.logger.Info("GitHub API: Empty result after retries - may be push to main or PR not yet indexed", + "commit", commitSHA[:8], "attempts", attemptNum) + } } else { - log.Printf("GitHub API: Found %d PR(s) for commit %s: %v", len(prNumbers), commitSHA[:8], prNumbers) + c.logger.Info("GitHub API: Found PRs for commit", "count", len(prNumbers), "commit", commitSHA[:8], "prs", prNumbers) } return nil case http.StatusNotFound: // Commit not found - could be a commit to main or repo doesn't exist - log.Printf("GitHub API: Commit %s not found (404) - may not exist or indexing delayed", commitSHA[:8]) + c.logger.Debug("GitHub API: Commit not found (404) - may not exist or indexing delayed", "commit", commitSHA[:8]) return retry.Unrecoverable(fmt.Errorf("commit not found: %s", commitSHA)) case http.StatusUnauthorized, http.StatusForbidden: // Don't retry on auth errors - log.Printf("GitHub API: Auth failed (%d) for commit %s", resp.StatusCode, commitSHA[:8]) + c.logger.Warn("GitHub API: Auth failed for commit", "status", resp.StatusCode, "commit", commitSHA[:8]) return retry.Unrecoverable(fmt.Errorf("authentication failed: status %d", resp.StatusCode)) case http.StatusInternalServerError, http.StatusBadGateway, http.StatusServiceUnavailable: // Retry on server errors lastErr = fmt.Errorf("GitHub API server error: %d", resp.StatusCode) - log.Printf("GitHub API: Server error %d for commit %s (will retry)", resp.StatusCode, commitSHA[:8]) + c.logger.Warn("GitHub API: Server error for commit (will retry)", "status", resp.StatusCode, "commit", commitSHA[:8]) return lastErr default: // Don't retry on other errors - log.Printf("GitHub API: Unexpected status %d for commit %s: %s", resp.StatusCode, commitSHA[:8], string(body)) + c.logger.Warn("GitHub API: Unexpected status for commit", "status", resp.StatusCode, "commit", commitSHA[:8], "body", string(body)) return retry.Unrecoverable(fmt.Errorf("unexpected status: %d, body: %s", resp.StatusCode, string(body))) } }, - retry.Attempts(3), + retry.Attempts(4), retry.DelayType(retry.FullJitterBackoffDelay), retry.MaxDelay(2*time.Minute), retry.Context(ctx), diff --git a/pkg/github/client_test.go b/pkg/github/client_test.go index 908b325..1eb84fd 100644 --- a/pkg/github/client_test.go +++ b/pkg/github/client_test.go @@ -17,7 +17,7 @@ func TestNewClient(t *testing.T) { t.Parallel() token := "ghp_test123" - client := NewClient(token) + client := NewClient(token, nil) if client == nil { t.Fatal("NewClient returned nil") @@ -53,7 +53,7 @@ func TestAuthenticatedUser_Success(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) // Override the base URL by creating a custom request originalURL := "https://api.github.com/user" client.httpClient.Transport = &redirectTransport{ @@ -81,7 +81,7 @@ func TestAuthenticatedUser_Unauthorized(t *testing.T) { })) defer server.Close() - client := NewClient("invalid-token") + client := NewClient("invalid-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -114,7 +114,7 @@ func TestAuthenticatedUser_RateLimit(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -151,7 +151,7 @@ func TestAuthenticatedUser_ServerError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -180,7 +180,7 @@ func TestAuthenticatedUser_EmptyUsername(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -220,7 +220,7 @@ func TestAppInstallationInfo_Success(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_test-app-token") + client := NewClient("ghs_test-app-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -249,7 +249,7 @@ func TestAppInstallationInfo_NotAnAppToken(t *testing.T) { })) defer server.Close() - client := NewClient("ghp_user-token") + client := NewClient("ghp_user-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -278,7 +278,7 @@ func TestAppInstallationInfo_NoRepositories(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_test-token") + client := NewClient("ghs_test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -318,7 +318,7 @@ func TestUserAndOrgs_AppToken(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_app-token") + client := NewClient("ghs_app-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -361,7 +361,7 @@ func TestUserAndOrgs_UserToken(t *testing.T) { })) defer server.Close() - client := NewClient("ghp_user-token") + client := NewClient("ghp_user-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -399,7 +399,7 @@ func TestValidateOrgMembership_Success(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -436,7 +436,7 @@ func TestValidateOrgMembership_NotMember(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -453,7 +453,7 @@ func TestValidateOrgMembership_NotMember(t *testing.T) { func TestValidateOrgMembership_EmptyOrgName(t *testing.T) { t.Parallel() - client := NewClient("test-token") + client := NewClient("test-token", nil) ctx := context.Background() _, _, err := client.ValidateOrgMembership(ctx, "") @@ -469,7 +469,7 @@ func TestValidateOrgMembership_EmptyOrgName(t *testing.T) { func TestValidateOrgMembership_InvalidOrgFormat(t *testing.T) { t.Parallel() - client := NewClient("test-token") + client := NewClient("test-token", nil) ctx := context.Background() invalidNames := []string{ @@ -511,7 +511,7 @@ func TestValidateOrgMembership_CaseInsensitive(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -539,7 +539,7 @@ func TestFindPRsForCommit_Success(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -576,7 +576,7 @@ func TestFindPRsForCommit_EmptyResultRetry(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -602,7 +602,7 @@ func TestFindPRsForCommit_NotFound(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -624,7 +624,7 @@ func TestFindPRsForCommit_Unauthorized(t *testing.T) { })) defer server.Close() - client := NewClient("invalid-token") + client := NewClient("invalid-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -655,7 +655,7 @@ func TestUserOrganizations_Success(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -684,7 +684,7 @@ func TestUserOrganizations_RateLimit(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -745,7 +745,7 @@ func TestContextCancellation(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -771,7 +771,7 @@ func TestMalformedJSON(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -799,7 +799,7 @@ func TestResponseBodyReadError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) // Use a custom transport with very small timeout to trigger error client.httpClient.Timeout = 1 * time.Nanosecond client.httpClient.Transport = &redirectTransport{ @@ -826,7 +826,7 @@ func TestLargeResponseBody(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -876,7 +876,7 @@ func TestUnexpectedStatusCodes(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -1063,7 +1063,7 @@ func TestAppInstallationInfo_ServerError(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_test-token") + client := NewClient("ghs_test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1092,7 +1092,7 @@ func TestAppInstallationInfo_Forbidden(t *testing.T) { })) defer server.Close() - client := NewClient("ghp_user-token") + client := NewClient("ghp_user-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1117,7 +1117,7 @@ func TestAppInstallationInfo_Unauthorized(t *testing.T) { })) defer server.Close() - client := NewClient("invalid-token") + client := NewClient("invalid-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1152,7 +1152,7 @@ func TestUserAndOrgs_AppTokenOnUserAccount(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_app-token") + client := NewClient("ghs_app-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1180,7 +1180,7 @@ func TestUserOrganizations_Unauthorized(t *testing.T) { })) defer server.Close() - client := NewClient("invalid-token") + client := NewClient("invalid-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1205,7 +1205,7 @@ func TestUserOrganizations_Forbidden(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1238,7 +1238,7 @@ func TestUserOrganizations_ServerError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1267,7 +1267,7 @@ func TestUserOrganizations_MalformedJSON(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1294,7 +1294,7 @@ func TestFindPRsForCommit_BadGateway(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1317,7 +1317,7 @@ func TestFindPRsForCommit_MalformedJSON(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1340,7 +1340,7 @@ func TestAuthenticatedUser_Forbidden(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -1372,7 +1372,7 @@ func TestAuthenticatedUser_BadGateway(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -1402,7 +1402,7 @@ func TestAuthenticatedUser_ServiceUnavailable(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user", to: server.URL + "/user", @@ -1428,7 +1428,7 @@ func TestAppInstallationInfo_MalformedJSON(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_test-token") + client := NewClient("ghs_test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1475,7 +1475,7 @@ func TestUserAndOrgs_TokenTypeDetection(t *testing.T) { })) defer server.Close() - client := NewClient(tt.token) + client := NewClient(tt.token, nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1497,7 +1497,7 @@ func TestFindPRsForCommit_ServiceUnavailable(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1519,7 +1519,7 @@ func TestFindPRsForCommit_Forbidden(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1549,7 +1549,7 @@ func TestUserOrganizations_BadGateway(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1594,7 +1594,7 @@ func TestAppInstallationInfo_BadGateway(t *testing.T) { })) defer server.Close() - client := NewClient("ghs_test-token") + client := NewClient("ghs_test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1626,7 +1626,7 @@ func TestFindPRsForCommit_PersistentEmptyResult(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1668,7 +1668,7 @@ func TestFindPRsForCommit_NetworkError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &multiPathTransport{server: server} ctx := context.Background() @@ -1705,7 +1705,7 @@ func TestUserOrganizations_RateLimitRetry(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", @@ -1750,7 +1750,7 @@ func TestAppInstallationInfo_NetworkError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/installation/repositories", to: server.URL + "/installation/repositories", @@ -1795,7 +1795,7 @@ func TestUserOrganizations_NetworkError(t *testing.T) { })) defer server.Close() - client := NewClient("test-token") + client := NewClient("test-token", nil) client.httpClient.Transport = &redirectTransport{ from: "https://api.github.com/user/orgs", to: server.URL + "/user/orgs", diff --git a/pkg/srv/commit_cache.go b/pkg/srv/commit_cache.go new file mode 100644 index 0000000..d44b0dd --- /dev/null +++ b/pkg/srv/commit_cache.go @@ -0,0 +1,101 @@ +// Package srv provides a WebSocket hub for managing client connections and broadcasting +// GitHub webhook events to subscribed clients based on their subscription criteria. +package srv + +import ( + "context" + "time" + + "github.com/codeGROOVE-dev/fido" + "github.com/codeGROOVE-dev/sprinkler/pkg/logger" +) + +const ( + // commitCacheSize is the maximum number of commit→PR mappings to cache. + // 16K entries should be sufficient for most deployments. + commitCacheSize = 16384 + + // commitCacheTTL is how long to keep commit→PR mappings. + // 24 hours is sufficient since PRs are typically merged or closed within that time. + commitCacheTTL = 24 * time.Hour +) + +// PRInfo contains cached information about a pull request. +type PRInfo struct { + URL string // Full PR URL (e.g., https://github.com/owner/repo/pull/123) + Number int // PR number + RepoURL string // Repository URL (e.g., https://github.com/owner/repo) + CachedAt time.Time +} + +// CommitCache maps commit SHAs to their associated pull requests. +// This enables reliable PR association for check_run/check_suite events +// even when GitHub's pull_requests array is empty. +type CommitCache struct { + cache *fido.Cache[string, PRInfo] +} + +// NewCommitCache creates a new commit→PR cache. +func NewCommitCache() *CommitCache { + return &CommitCache{ + cache: fido.New[string, PRInfo]( + fido.Size(commitCacheSize), + fido.TTL(commitCacheTTL), + ), + } +} + +// Set caches a commit SHA → PR mapping. +func (c *CommitCache) Set(ctx context.Context, commitSHA string, info PRInfo) { + if commitSHA == "" || info.URL == "" { + return + } + + info.CachedAt = time.Now() + c.cache.Set(commitSHA, info) + + logger.Info(ctx, "commit cache: stored PR mapping", logger.Fields{ + "commit_sha": truncateSHA(commitSHA), + "pr_url": info.URL, + "pr_number": info.Number, + "cache_size": c.cache.Len(), + }) +} + +// Get retrieves PR info for a commit SHA. +// Returns the PRInfo and true if found, or zero value and false if not cached. +func (c *CommitCache) Get(ctx context.Context, commitSHA string) (PRInfo, bool) { + if commitSHA == "" { + return PRInfo{}, false + } + + info, found := c.cache.Get(commitSHA) + if found { + logger.Info(ctx, "commit cache: HIT", logger.Fields{ + "commit_sha": truncateSHA(commitSHA), + "pr_url": info.URL, + "pr_number": info.Number, + "cached_ago": time.Since(info.CachedAt).Round(time.Second).String(), + }) + } else { + logger.Info(ctx, "commit cache: MISS", logger.Fields{ + "commit_sha": truncateSHA(commitSHA), + "cache_size": c.cache.Len(), + }) + } + + return info, found +} + +// Len returns the number of cached entries. +func (c *CommitCache) Len() int { + return c.cache.Len() +} + +// truncateSHA returns the first 8 characters of a SHA for logging. +func truncateSHA(sha string) string { + if len(sha) > 8 { + return sha[:8] + } + return sha +} diff --git a/pkg/srv/commit_cache_test.go b/pkg/srv/commit_cache_test.go new file mode 100644 index 0000000..1b4e3f5 --- /dev/null +++ b/pkg/srv/commit_cache_test.go @@ -0,0 +1,160 @@ +package srv + +import ( + "context" + "testing" + "time" +) + +func TestCommitCache_SetAndGet(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Test setting and getting a value + info := PRInfo{ + URL: "https://github.com/owner/repo/pull/123", + Number: 123, + RepoURL: "https://github.com/owner/repo", + } + cache.Set(ctx, "abc123def456", info) + + // Verify we can get it back + got, found := cache.Get(ctx, "abc123def456") + if !found { + t.Fatal("expected to find cached entry") + } + if got.URL != info.URL { + t.Errorf("URL mismatch: got %q, want %q", got.URL, info.URL) + } + if got.Number != info.Number { + t.Errorf("Number mismatch: got %d, want %d", got.Number, info.Number) + } + if got.RepoURL != info.RepoURL { + t.Errorf("RepoURL mismatch: got %q, want %q", got.RepoURL, info.RepoURL) + } + if got.CachedAt.IsZero() { + t.Error("CachedAt should be set") + } +} + +func TestCommitCache_Miss(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Test cache miss + _, found := cache.Get(ctx, "nonexistent123") + if found { + t.Error("expected cache miss for nonexistent key") + } +} + +func TestCommitCache_EmptyKey(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Empty key should not be cached + cache.Set(ctx, "", PRInfo{URL: "https://example.com"}) + if cache.Len() != 0 { + t.Error("empty key should not be cached") + } + + // Empty key lookup should return not found + _, found := cache.Get(ctx, "") + if found { + t.Error("empty key lookup should return not found") + } +} + +func TestCommitCache_EmptyURL(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Empty URL should not be cached + cache.Set(ctx, "abc123", PRInfo{URL: ""}) + if cache.Len() != 0 { + t.Error("empty URL should not be cached") + } +} + +func TestCommitCache_Overwrite(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Set initial value + cache.Set(ctx, "abc123", PRInfo{ + URL: "https://github.com/owner/repo/pull/1", + Number: 1, + }) + + // Overwrite with new value + cache.Set(ctx, "abc123", PRInfo{ + URL: "https://github.com/owner/repo/pull/2", + Number: 2, + }) + + // Should get the new value + got, found := cache.Get(ctx, "abc123") + if !found { + t.Fatal("expected to find cached entry") + } + if got.Number != 2 { + t.Errorf("expected PR #2, got #%d", got.Number) + } +} + +func TestCommitCache_MultipleEntries(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + // Add multiple entries + for i := 1; i <= 100; i++ { + cache.Set(ctx, "sha"+string(rune(i)), PRInfo{ + URL: "https://github.com/owner/repo/pull/" + string(rune(i)), + Number: i, + }) + } + + if cache.Len() != 100 { + t.Errorf("expected 100 entries, got %d", cache.Len()) + } +} + +func TestTruncateSHA(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"abc123def456", "abc123de"}, + {"short", "short"}, + {"exactly8", "exactly8"}, + {"", ""}, + } + + for _, tt := range tests { + got := truncateSHA(tt.input) + if got != tt.want { + t.Errorf("truncateSHA(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} + +func TestCommitCache_CachedAtSet(t *testing.T) { + ctx := context.Background() + cache := NewCommitCache() + + before := time.Now() + cache.Set(ctx, "abc123", PRInfo{ + URL: "https://github.com/owner/repo/pull/1", + Number: 1, + }) + after := time.Now() + + got, found := cache.Get(ctx, "abc123") + if !found { + t.Fatal("expected to find cached entry") + } + + if got.CachedAt.Before(before) || got.CachedAt.After(after) { + t.Errorf("CachedAt %v not between %v and %v", got.CachedAt, before, after) + } +} diff --git a/pkg/srv/hub.go b/pkg/srv/hub.go index a9d4a93..f329823 100644 --- a/pkg/srv/hub.go +++ b/pkg/srv/hub.go @@ -51,6 +51,7 @@ type Hub struct { stopped chan struct{} mu sync.RWMutex periodicCheckInterval time.Duration // For testing; 0 means use default (1 minute) + commitCache *CommitCache // Maps commit SHA → PR info for check event association } // broadcastMsg contains an event and the payload for matching. @@ -69,12 +70,13 @@ const ( // NewHub creates a new client hub. func NewHub() *Hub { return &Hub{ - clients: make(map[string]*Client), - register: make(chan *Client, registerBufferSize), // Buffer to prevent blocking - unregister: make(chan string, unregisterBufferSize), // Buffer to prevent blocking - broadcast: make(chan broadcastMsg, broadcastBufferSize), // Limited buffer to prevent memory exhaustion - stop: make(chan struct{}), - stopped: make(chan struct{}), + clients: make(map[string]*Client), + register: make(chan *Client, registerBufferSize), // Buffer to prevent blocking + unregister: make(chan string, unregisterBufferSize), // Buffer to prevent blocking + broadcast: make(chan broadcastMsg, broadcastBufferSize), // Limited buffer to prevent memory exhaustion + stop: make(chan struct{}), + stopped: make(chan struct{}), + commitCache: NewCommitCache(), } } @@ -247,6 +249,11 @@ func (h *Hub) ClientCount() int { return len(h.clients) } +// CommitCache returns the hub's commit→PR cache for populating from webhook events. +func (h *Hub) CommitCache() *CommitCache { + return h.commitCache +} + // trySendEvent attempts to send an event to a client's send channel. // Returns true if sent successfully, false if channel is full or closed. // diff --git a/pkg/srv/websocket.go b/pkg/srv/websocket.go index c4d3679..e08e0f0 100644 --- a/pkg/srv/websocket.go +++ b/pkg/srv/websocket.go @@ -392,7 +392,7 @@ func (h *WebSocketHandler) validateAuth(ctx context.Context, ws *websocket.Conn, if h.githubClientFactory != nil { ghClient = h.githubClientFactory(githubToken) } else { - ghClient = github.NewClient(githubToken) + ghClient = github.NewClient(githubToken, nil) } if sub.Organization != "" { diff --git a/pkg/webhook/handler.go b/pkg/webhook/handler.go index 26f9e7d..5a6d987 100644 --- a/pkg/webhook/handler.go +++ b/pkg/webhook/handler.go @@ -8,7 +8,6 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" - "fmt" "io" "log" "net/http" @@ -141,22 +140,10 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // For check events, always log the full payload to help with debugging - if eventType == "check_run" || eventType == "check_suite" { - payloadJSON, err := json.Marshal(payload) - if err != nil { - logger.Warn(ctx, "failed to marshal check event payload", logger.Fields{ - "event_type": eventType, - "delivery_id": deliveryID, - "error": err.Error(), - }) - } else { - logger.Info(ctx, "received check event - full payload for debugging", logger.Fields{ - "event_type": eventType, - "delivery_id": deliveryID, - "payload": string(payloadJSON), - }) - } + // Extract commit SHA early for check/pull_request events (used for cache and event) + var commitSHA string + if eventType == "check_run" || eventType == "check_suite" || eventType == "pull_request" { + commitSHA = extractCommitSHA(eventType, payload) } // Extract PR URL @@ -183,42 +170,40 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // For check events without PR URL, this is a known race condition - // GitHub webhooks can fire before the pull_requests array is populated - commitSHA := extractCommitSHA(eventType, payload) - // Extract repo URL as fallback for org-based matching - repoURL := "" - if repo, ok := payload["repository"].(map[string]any); ok { - if htmlURL, ok := repo["html_url"].(string); ok { - repoURL = htmlURL + // For check events without PR URL, try cache lookup first + if commitSHA != "" { + if prInfo, found := h.hub.CommitCache().Get(ctx, commitSHA); found { + prURL = prInfo.URL + logger.Info(ctx, "check event: PR found via cache", logger.Fields{ + "event_type": eventType, + "delivery_id": deliveryID, + "commit_sha": truncateSHA(commitSHA), + "pr_url": prURL, + "pr_number": prInfo.Number, + }) } } - // If we can't extract repo URL, drop the event - if repoURL == "" { - // Can't extract even repo URL - must drop the event - logger.Warn(ctx, "⛔ DROPPING CHECK EVENT - no PR URL or repo URL", logger.Fields{ + // Fall back to repo URL if cache miss + if prURL == "" { + repoURL := extractRepoURL(payload) + if repoURL == "" { + logger.Warn(ctx, "check event: dropping - no repo URL", logger.Fields{ + "event_type": eventType, + "delivery_id": deliveryID, + "commit_sha": commitSHA, + }) + w.WriteHeader(http.StatusOK) + return + } + prURL = repoURL + logger.Info(ctx, "check event: using repo URL (cache miss)", logger.Fields{ "event_type": eventType, "delivery_id": deliveryID, "commit_sha": commitSHA, - "issue": "cannot extract repository information from payload", + "repo_url": repoURL, }) - w.WriteHeader(http.StatusOK) - return } - - // We can still broadcast using repo URL - org-based subscriptions will work - logger.Warn(ctx, "⚠️ CHECK EVENT RACE CONDITION DETECTED", logger.Fields{ - "event_type": eventType, - "delivery_id": deliveryID, - "commit_sha": commitSHA, - "repo_url": repoURL, - "issue": "pull_requests array not yet populated by GitHub", - "workaround": "broadcasting with repo URL for org-based subscriptions", - }) - - // Use repo URL as fallback - org subscriptions will still work - prURL = repoURL } // Create and broadcast event @@ -227,13 +212,37 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { Timestamp: time.Now(), Type: eventType, DeliveryID: deliveryID, + CommitSHA: commitSHA, } - // Extract commit SHA for cache population and PR lookup - // - pull_request: allows client-side cache population for commit->PR mapping - // - check events: enables PR lookup when URL is repo-only (GitHub race condition) - if eventType == "check_run" || eventType == "check_suite" || eventType == "pull_request" { - event.CommitSHA = extractCommitSHA(eventType, payload) + // For pull_request events, cache the commit SHA → PR URL mapping + // This enables reliable PR association for subsequent check events + if eventType == "pull_request" { + if commitSHA == "" { + logger.Warn(ctx, "pull_request event: no commit SHA extracted", logger.Fields{ + "delivery_id": deliveryID, + "pr_url": prURL, + }) + } else if !strings.Contains(prURL, "/pull/") { + logger.Warn(ctx, "pull_request event: URL not a PR URL", logger.Fields{ + "delivery_id": deliveryID, + "commit_sha": truncateSHA(commitSHA), + "url": prURL, + }) + } else { + // Extract PR number inline (only used here) + var prNumber int + if pr, ok := payload["pull_request"].(map[string]any); ok { + if num, ok := pr["number"].(float64); ok { + prNumber = int(num) + } + } + h.hub.CommitCache().Set(ctx, commitSHA, srv.PRInfo{ + URL: prURL, + Number: prNumber, + RepoURL: extractRepoURL(payload), + }) + } } // Get client count before broadcasting (for debugging delivery issues) @@ -246,25 +255,25 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { logger.Error(ctx, "failed to write response", err, logger.Fields{"delivery_id": deliveryID}) } - // Log successful webhook processing with client count for debugging + // Log webhook processing result logFields := logger.Fields{ "event_type": eventType, "delivery_id": deliveryID, "url": prURL, - "remote_addr": r.RemoteAddr, - "payload_size": len(body), "connected_clients": clientCount, } - // Indicate if this is a repo URL fallback (check event race condition) - if (eventType == "check_run" || eventType == "check_suite") && !strings.Contains(prURL, "/pull/") { - logFields["url_type"] = "repository_fallback" - logFields["note"] = "using repo URL due to missing pull_requests array (GitHub timing issue)" - } else { - logFields["url_type"] = "pull_request" + // For check events, add context about PR association + if eventType == "check_run" || eventType == "check_suite" { + if strings.Contains(prURL, "/pull/") { + logFields["pr_associated"] = true + } else { + logFields["pr_associated"] = false + logFields["commit_sha"] = event.CommitSHA + } } - logger.Info(ctx, "webhook processed successfully", logFields) + logger.Info(ctx, "webhook broadcast", logFields) } // VerifySignature validates the GitHub webhook signature. @@ -306,108 +315,103 @@ func ExtractPRURL(ctx context.Context, eventType string, payload map[string]any) } } case "check_run", "check_suite": - // Extract PR URLs from check events if available + // Extract PR URL from check events if available in pull_requests array if checkRun, ok := payload["check_run"].(map[string]any); ok { - if url := extractPRFromCheckEvent(ctx, checkRun, payload, eventType); url != "" { - return url + result := extractCheckEventInfo(checkRun, payload) + if result.prURL != "" { + logger.Info(ctx, "check event: PR found in payload", logger.Fields{ + "event_type": eventType, + "pr_url": result.prURL, + "pr_number": result.prNumber, + "pr_count": result.prCount, + "source": result.source, + "check_name": result.checkName, + "conclusion": result.conclusion, + }) + return result.prURL } } if checkSuite, ok := payload["check_suite"].(map[string]any); ok { - if url := extractPRFromCheckEvent(ctx, checkSuite, payload, eventType); url != "" { - return url + result := extractCheckEventInfo(checkSuite, payload) + if result.prURL != "" { + logger.Info(ctx, "check event: PR found in payload", logger.Fields{ + "event_type": eventType, + "pr_url": result.prURL, + "pr_number": result.prNumber, + "pr_count": result.prCount, + "source": result.source, + }) + return result.prURL } } - // Log when we can't extract PR URL from check event - payloadKeys := make([]string, 0, len(payload)) - for k := range payload { - payloadKeys = append(payloadKeys, k) - } - logger.Warn(ctx, "no PR URL found in check event", logger.Fields{ - "event_type": eventType, - "has_check_run": payload["check_run"] != nil, - "has_check_suite": payload["check_suite"] != nil, - "payload_keys": payloadKeys, - }) + // No PR URL in payload - will try cache lookup in caller default: // For other event types, no PR URL can be extracted } return "" } -// extractPRFromCheckEvent extracts PR URL from check_run or check_suite events. -func extractPRFromCheckEvent(ctx context.Context, checkEvent map[string]any, payload map[string]any, eventType string) string { +// checkEventResult contains the result of extracting PR info from a check event. +type checkEventResult struct { + prURL string + prNumber int + prCount int // Number of PRs in the pull_requests array + source string // How the PR URL was found: "html_url", "constructed", or "" + commitSHA string + checkName string + conclusion string +} + +// extractCheckEventInfo extracts PR and check info from check_run or check_suite events. +func extractCheckEventInfo(checkEvent map[string]any, payload map[string]any) checkEventResult { + result := checkEventResult{} + + // Extract check metadata + if name, ok := checkEvent["name"].(string); ok { + result.checkName = name + } + if conclusion, ok := checkEvent["conclusion"].(string); ok { + result.conclusion = conclusion + } + if sha, ok := checkEvent["head_sha"].(string); ok { + result.commitSHA = sha + } + + // Check for pull_requests array prs, ok := checkEvent["pull_requests"].([]any) if !ok || len(prs) == 0 { - logger.Info(ctx, "check event has no pull_requests array", logger.Fields{ - "event_type": eventType, - "has_pr_array": ok, - "pr_array_length": len(prs), - "check_event_keys": getMapKeys(checkEvent), - }) - return "" + return result } + result.prCount = len(prs) pr, ok := prs[0].(map[string]any) if !ok { - logger.Warn(ctx, "pull_requests[0] is not a map", logger.Fields{ - "event_type": eventType, - "pr_type": fmt.Sprintf("%T", prs[0]), - }) - return "" - } - - // Try html_url first - if htmlURL, ok := pr["html_url"].(string); ok { - logger.Info(ctx, "extracted PR URL from check event html_url", logger.Fields{ - "event_type": eventType, - "pr_url": htmlURL, - }) - return htmlURL + return result } - // Fallback: construct from number - num, ok := pr["number"].(float64) - if !ok { - logger.Warn(ctx, "PR number not found in check event", logger.Fields{ - "event_type": eventType, - "pr_keys": getMapKeys(pr), - }) - return "" + // Extract PR number + if num, ok := pr["number"].(float64); ok { + result.prNumber = int(num) } - repo, ok := payload["repository"].(map[string]any) - if !ok { - logger.Warn(ctx, "repository not found in payload", logger.Fields{ - "event_type": eventType, - }) - return "" + // Try html_url first (preferred) + if htmlURL, ok := pr["html_url"].(string); ok { + result.prURL = htmlURL + result.source = "html_url" + return result } - repoURL, ok := repo["html_url"].(string) - if !ok { - logger.Warn(ctx, "repository html_url not found", logger.Fields{ - "event_type": eventType, - "repo_keys": getMapKeys(repo), - }) - return "" + // Fallback: construct from number + repo URL + if result.prNumber > 0 { + if repo, ok := payload["repository"].(map[string]any); ok { + if repoURL, ok := repo["html_url"].(string); ok { + result.prURL = repoURL + "/pull/" + strconv.Itoa(result.prNumber) + result.source = "constructed" + } + } } - constructedURL := repoURL + "/pull/" + strconv.Itoa(int(num)) - logger.Info(ctx, "constructed PR URL from check event", logger.Fields{ - "event_type": eventType, - "pr_url": constructedURL, - "pr_number": int(num), - }) - return constructedURL -} - -// getMapKeys returns the keys from a map for logging. -func getMapKeys(m map[string]any) []string { - keys := make([]string, 0, len(m)) - for k := range m { - keys = append(keys, k) - } - return keys + return result } // extractCommitSHA extracts the commit SHA from pull_request, check_run, or check_suite events. @@ -438,3 +442,22 @@ func extractCommitSHA(eventType string, payload map[string]any) string { } return "" } + +// extractRepoURL extracts the repository HTML URL from the payload. +func extractRepoURL(payload map[string]any) string { + if repo, ok := payload["repository"].(map[string]any); ok { + if htmlURL, ok := repo["html_url"].(string); ok { + return htmlURL + } + } + return "" +} + +// truncateSHA returns the first 8 characters of a SHA for logging. +func truncateSHA(sha string) string { + if len(sha) > 8 { + return sha[:8] + } + return sha +} + diff --git a/pkg/webhook/handler_test.go b/pkg/webhook/handler_test.go index 0a9a450..bcc67b1 100644 --- a/pkg/webhook/handler_test.go +++ b/pkg/webhook/handler_test.go @@ -385,46 +385,6 @@ func TestExtractCommitSHA(t *testing.T) { } } -// TestGetMapKeys tests the getMapKeys utility function. -func TestGetMapKeys(t *testing.T) { - tests := []struct { - name string - input map[string]any - expected int // Just check length since order is undefined - }{ - { - name: "empty map", - input: map[string]any{}, - expected: 0, - }, - { - name: "single key", - input: map[string]any{ - "key1": "value1", - }, - expected: 1, - }, - { - name: "multiple keys", - input: map[string]any{ - "key1": "value1", - "key2": "value2", - "key3": "value3", - }, - expected: 3, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := getMapKeys(tt.input) - if len(result) != tt.expected { - t.Errorf("getMapKeys() returned %d keys, want %d", len(result), tt.expected) - } - }) - } -} - // TestWebhookHandlerNoPRURL tests events with no PR URL. func TestWebhookHandlerNoPRURL(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) From d4ed8c36c1d70746c9f3c1d3b3b8593b7577ba3f Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 6 Jan 2026 19:57:46 -0500 Subject: [PATCH 3/3] update go deps --- go.mod | 12 ++++++++---- go.sum | 20 ++++++++++++-------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/go.mod b/go.mod index 9f0ea48..a4231c7 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,13 @@ module github.com/codeGROOVE-dev/sprinkler go 1.25.4 require ( - github.com/codeGROOVE-dev/retry v1.3.0 - golang.org/x/crypto v0.44.0 - golang.org/x/net v0.47.0 + github.com/codeGROOVE-dev/fido v1.10.0 + github.com/codeGROOVE-dev/retry v1.3.1 + golang.org/x/crypto v0.46.0 + golang.org/x/net v0.48.0 ) -require golang.org/x/text v0.31.0 // indirect +require ( + github.com/puzpuzpuz/xsync/v4 v4.2.0 // indirect + golang.org/x/text v0.32.0 // indirect +) diff --git a/go.sum b/go.sum index 33ce84c..12e3225 100644 --- a/go.sum +++ b/go.sum @@ -1,8 +1,12 @@ -github.com/codeGROOVE-dev/retry v1.3.0 h1:/+ipAWRJLL6y1R1vprYo0FSjSBvH6fE5j9LKXjpD54g= -github.com/codeGROOVE-dev/retry v1.3.0/go.mod h1:8OgefgV1XP7lzX2PdKlCXILsYKuz6b4ZpHa/20iLi8E= -golang.org/x/crypto v0.44.0 h1:A97SsFvM3AIwEEmTBiaxPPTYpDC47w720rdiiUvgoAU= -golang.org/x/crypto v0.44.0/go.mod h1:013i+Nw79BMiQiMsOPcVCB5ZIJbYkerPrGnOa00tvmc= -golang.org/x/net v0.47.0 h1:Mx+4dIFzqraBXUugkia1OOvlD6LemFo1ALMHjrXDOhY= -golang.org/x/net v0.47.0/go.mod h1:/jNxtkgq5yWUGYkaZGqo27cfGZ1c5Nen03aYrrKpVRU= -golang.org/x/text v0.31.0 h1:aC8ghyu4JhP8VojJ2lEHBnochRno1sgL6nEi9WGFGMM= -golang.org/x/text v0.31.0/go.mod h1:tKRAlv61yKIjGGHX/4tP1LTbc13YSec1pxVEWXzfoeM= +github.com/codeGROOVE-dev/fido v1.10.0 h1:i4Wb6LDd5nD/4Fnp47KAVUVhG1O1mN5jSRbCYPpBYjw= +github.com/codeGROOVE-dev/fido v1.10.0/go.mod h1:/mqfMeKCTYTGt/Y0cWm6gh8gYBKG1w8xBsTDmu+A/pU= +github.com/codeGROOVE-dev/retry v1.3.1 h1:BAkfDzs6FssxLCGWGgM97bb+6/8GTa40Cs147vXkJOg= +github.com/codeGROOVE-dev/retry v1.3.1/go.mod h1:+b3huqYGY1+ZJyuCmR8nBVLjd3WJ7qAFss+sI4s6FSc= +github.com/puzpuzpuz/xsync/v4 v4.2.0 h1:dlxm77dZj2c3rxq0/XNvvUKISAmovoXF4a4qM6Wvkr0= +github.com/puzpuzpuz/xsync/v4 v4.2.0/go.mod h1:VJDmTCJMBt8igNxnkQd86r+8KUeN1quSfNKu5bLYFQo= +golang.org/x/crypto v0.46.0 h1:cKRW/pmt1pKAfetfu+RCEvjvZkA9RimPbh7bhFjGVBU= +golang.org/x/crypto v0.46.0/go.mod h1:Evb/oLKmMraqjZ2iQTwDwvCtJkczlDuTmdJXoZVzqU0= +golang.org/x/net v0.48.0 h1:zyQRTTrjc33Lhh0fBgT/H3oZq9WuvRR5gPC70xpDiQU= +golang.org/x/net v0.48.0/go.mod h1:+ndRgGjkh8FGtu1w1FGbEC31if4VrNVMuKTgcAAnQRY= +golang.org/x/text v0.32.0 h1:ZD01bjUt1FQ9WJ0ClOL5vxgxOI/sVCNgX1YtKwcY0mU= +golang.org/x/text v0.32.0/go.mod h1:o/rUWzghvpD5TXrTIBuJU77MTaN0ljMWE47kxGJQ7jY=