diff --git a/pkg/api/aggregate_config.go b/pkg/api/aggregate_config.go index 794d81eb0..b49d9c4e0 100644 --- a/pkg/api/aggregate_config.go +++ b/pkg/api/aggregate_config.go @@ -3,6 +3,8 @@ package api import ( "fmt" "strings" + + "github.com/block/schemabot/pkg/storage" ) // Aggregate-check roles for a repository. In a multi-tenant aggregate check, @@ -65,7 +67,7 @@ func (c *ServerConfig) AggregateRoleForRepo(repo string) string { if c == nil { return "" } - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok || repoConfig.Aggregate == nil { return "" } @@ -107,7 +109,7 @@ func (c *ServerConfig) ExpectedParticipantChecksForPR(repo string, changedFiles if c == nil { return nil } - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok || !repoConfig.Aggregate.isLeader() { return nil } diff --git a/pkg/api/config.go b/pkg/api/config.go index f1253b389..06f9760d7 100644 --- a/pkg/api/config.go +++ b/pkg/api/config.go @@ -1204,7 +1204,7 @@ type RepoConfig struct { // RepoAdmins returns the repository-scoped admin principals configured for // repo. A repository with no config entry has no repo admins. func (c *ServerConfig) RepoAdmins(repo string) (teams, users []string) { - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok { return nil, nil } @@ -1339,6 +1339,9 @@ func LoadServerConfigFromFile(path string) (*ServerConfig, error) { return nil, fmt.Errorf("parse config file: %w", err) } + if err := config.canonicalizeRepositories(); err != nil { + return nil, fmt.Errorf("invalid config: %w", err) + } if err := config.Validate(); err != nil { return nil, fmt.Errorf("invalid config: %w", err) } @@ -1346,6 +1349,36 @@ func LoadServerConfigFromFile(path string) (*ServerConfig, error) { return &config, nil } +func (c *ServerConfig) canonicalizeRepositories() error { + repoNames := make([]string, 0, len(c.Repos)) + for repo := range c.Repos { + repoNames = append(repoNames, repo) + } + slices.Sort(repoNames) + + canonicalRepos := make(map[string]RepoConfig, len(c.Repos)) + originalNames := make(map[string]string, len(c.Repos)) + for _, repo := range repoNames { + canonical := storage.CanonicalKey(repo) + if original, ok := originalNames[canonical]; ok { + return fmt.Errorf("repos contains keys %q and %q that canonicalize to %q", original, repo, canonical) + } + canonicalRepos[canonical] = c.Repos[repo] + originalNames[canonical] = repo + } + if c.Repos != nil { + c.Repos = canonicalRepos + } + + for database, dbConfig := range c.Databases { + for i, repo := range dbConfig.AllowedRepos { + dbConfig.AllowedRepos[i] = storage.CanonicalKey(repo) + } + c.Databases[database] = dbConfig + } + return nil +} + // Validate checks the configuration for required fields and consistency. func (c *ServerConfig) Validate() error { // The database registry is required for the control plane and for a @@ -2349,7 +2382,7 @@ func (c *ServerConfig) IsRepoAllowed(repo string) bool { if c == nil || len(c.Repos) == 0 { return true } - _, ok := c.Repos[repo] + _, ok := c.Repos[storage.CanonicalKey(repo)] return ok } @@ -2360,7 +2393,7 @@ func (c *ServerConfig) AreChecksEnabled(repo string) bool { if c == nil || len(c.Repos) == 0 { return true } - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok || repoConfig.EnableChecks == nil { return true } @@ -2411,7 +2444,7 @@ func (c *ServerConfig) ResolveGitHubAppForRepo(repo string) (ResolvedGitHubApp, return ResolvedGitHubApp{}, fmt.Errorf("server config is nil") } if len(c.Apps) > 0 { - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok { return ResolvedGitHubApp{}, fmt.Errorf("repository %q: %w", repo, ErrRepoNotConfigured) } @@ -2682,7 +2715,7 @@ func (c *ServerConfig) GitHubCheckNameBaseForRepo(repo string) string { return DefaultGitHubCheckName } if len(c.Apps) > 0 { - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok { return DefaultGitHubCheckName } @@ -2704,7 +2737,7 @@ func (c *ServerConfig) PromotionCheckNameBaseForRepo(repo string) string { return DefaultGitHubCheckName } if len(c.Apps) > 0 { - repoConfig, ok := c.Repos[repo] + repoConfig, ok := c.Repos[storage.CanonicalKey(repo)] if !ok { return DefaultGitHubCheckName } diff --git a/pkg/api/config_test.go b/pkg/api/config_test.go index b03617874..8970ab1a0 100644 --- a/pkg/api/config_test.go +++ b/pkg/api/config_test.go @@ -176,6 +176,55 @@ repos: assert.False(t, cfg.AreChecksEnabled("org/repo")) } +func TestLoadServerConfigFromFileCanonicalizesRepositories(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + content := ` +databases: + testapp: + type: mysql + allowed_repos: + - MixedCase/Sample-Repo + environments: + staging: + target: testapp-staging + deployment: default +tern_deployments: + default: + staging: tern-staging:9090 +repos: + MixedCase/Sample-Repo: {} +` + require.NoError(t, os.WriteFile(configPath, []byte(content), 0644)) + + cfg, err := LoadServerConfigFromFile(configPath) + require.NoError(t, err) + assert.Contains(t, cfg.Repos, "mixedcase/sample-repo") + assert.NotContains(t, cfg.Repos, "MixedCase/Sample-Repo") + assert.Equal(t, []string{"mixedcase/sample-repo"}, cfg.Databases["testapp"].AllowedRepos) +} + +func TestLoadServerConfigFromFileRejectsCanonicalRepositoryCollision(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + content := ` +databases: + testapp: + type: mysql + environments: + staging: + target: testapp-staging + deployment: default +repos: + MixedCase/Sample-Repo: {} + mixedcase/sample-repo: {} +` + require.NoError(t, os.WriteFile(configPath, []byte(content), 0644)) + + _, err := LoadServerConfigFromFile(configPath) + require.Error(t, err) + assert.ErrorContains(t, err, `"MixedCase/Sample-Repo"`) + assert.ErrorContains(t, err, `"mixedcase/sample-repo"`) +} + func TestLoadServerConfigFromFile_DSNFrom(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.yaml") @@ -2970,6 +3019,8 @@ func TestServerConfig_IsRepoAllowed(t *testing.T) { }, } assert.True(t, cfg.IsRepoAllowed("org/allowed-repo")) + assert.True(t, cfg.IsRepoAllowed("ORG/ALLOWED-REPO")) + assert.True(t, cfg.IsRepoAllowed("Org/Allowed-Repo")) }) t.Run("populated repos rejects unlisted repo", func(t *testing.T) { diff --git a/pkg/api/source_policy.go b/pkg/api/source_policy.go index 48ce17a21..1df54936d 100644 --- a/pkg/api/source_policy.go +++ b/pkg/api/source_policy.go @@ -5,6 +5,8 @@ import ( "fmt" "path" "strings" + + "github.com/block/schemabot/pkg/storage" ) const ( @@ -217,9 +219,11 @@ func validateAllowedRepos(field string, repos []string) error { } func repoAllowed(allowedRepos []string, repo string) bool { - repo = strings.TrimSpace(repo) + repo = storage.CanonicalKey(strings.TrimSpace(repo)) for _, allowed := range allowedRepos { - switch strings.TrimSpace(allowed) { + // Entries loaded through config are already canonical; folding here + // too keeps the match correct for allow-lists built any other way. + switch storage.CanonicalKey(strings.TrimSpace(allowed)) { case "*": return true case repo: diff --git a/pkg/api/source_policy_test.go b/pkg/api/source_policy_test.go index 69a568109..123cb111a 100644 --- a/pkg/api/source_policy_test.go +++ b/pkg/api/source_policy_test.go @@ -45,6 +45,12 @@ func TestRepoAllowed(t *testing.T) { repository: " octocat/hello-world ", want: true, }, + { + name: "uppercase allow-list entry matches canonical request", + allowedRepos: []string{"Octocat/Hello-World"}, + repository: "octocat/hello-world", + want: true, + }, { name: "similar repo name is blocked", allowedRepos: []string{"octocat/hello-world"}, diff --git a/pkg/storage/canonical_test.go b/pkg/storage/canonical_test.go new file mode 100644 index 000000000..066b28a3f --- /dev/null +++ b/pkg/storage/canonical_test.go @@ -0,0 +1,25 @@ +package storage + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCanonicalKey(t *testing.T) { + tests := []struct { + name string + key string + want string + }{ + {name: "mixed case", key: "MixedCase/Sample-Repo", want: "mixedcase/sample-repo"}, + {name: "lowercase", key: "mixedcase/sample-repo", want: "mixedcase/sample-repo"}, + {name: "empty", key: "", want: ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, CanonicalKey(tt.key)) + }) + } +} diff --git a/pkg/webhook/check_run.go b/pkg/webhook/check_run.go index 648be7b76..1ad8e6969 100644 --- a/pkg/webhook/check_run.go +++ b/pkg/webhook/check_run.go @@ -6,6 +6,7 @@ import ( "net/http" "github.com/block/schemabot/pkg/metrics" + "github.com/block/schemabot/pkg/storage" ) type checkRunPayload struct { @@ -34,6 +35,7 @@ func (h *Handler) handleCheckRun(ctx context.Context, metricApp string, w http.R h.writeError(w, http.StatusBadRequest, "invalid check_run payload") return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) switch payload.Action { case "rerequested": diff --git a/pkg/webhook/check_suite.go b/pkg/webhook/check_suite.go index 78a348e4c..ccbcdd091 100644 --- a/pkg/webhook/check_suite.go +++ b/pkg/webhook/check_suite.go @@ -79,6 +79,7 @@ func (h *Handler) handleCheckSuite(ctx context.Context, metricApp string, w http h.writeError(w, http.StatusBadRequest, "invalid check_suite payload") return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) repo := payload.Repository.FullName headSHA := payload.CheckSuite.HeadSHA @@ -210,7 +211,7 @@ func (h *Handler) processDurableCheckSuite(ctx context.Context, event *storage.W "delivery_id", event.DeliveryID, "repo", event.Repository, "head_sha", event.HeadSHA) return false, nil } - repo := event.Repository + repo := storage.CanonicalKey(event.Repository) headSHA := event.HeadSHA if repo == "" || headSHA == "" { return false, fmt.Errorf("durable check_suite delivery %s missing repo or head SHA", event.DeliveryID) diff --git a/pkg/webhook/check_suite_test.go b/pkg/webhook/check_suite_test.go index a1193d390..86dd86378 100644 --- a/pkg/webhook/check_suite_test.go +++ b/pkg/webhook/check_suite_test.go @@ -204,6 +204,27 @@ func TestCheckSuiteWebhookQueuesWithGrace(t *testing.T) { assert.WithinDuration(t, before.Add(defaultCheckSuiteRecoveryGrace), *row.RetryAfter, 10*time.Second) } +func TestCheckSuiteWebhookCanonicalizesRepository(t *testing.T) { + store := newRecordingWebhookEventStore() + h := newCheckSuiteIngressHandler(t, store, map[string]api.RepoConfig{"mixedcase/sample-repo": {}}) + + req := buildCheckSuiteWebhookRequest(t, "requested", "MixedCaseSHA", "MixedCaseBranch", 7) + body, err := io.ReadAll(req.Body) + require.NoError(t, err) + req.Body = io.NopCloser(strings.NewReader(strings.ReplaceAll(string(body), "octocat/hello-world", "MixedCase/Sample-Repo"))) + req.Header.Set(headerDeliveryID, "mixed-case-check-suite") + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + row, err := store.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-check-suite") + require.NoError(t, err) + require.NotNil(t, row) + assert.Equal(t, "mixedcase/sample-repo", row.Repository) + assert.Equal(t, "MixedCaseSHA", row.HeadSHA) +} + // Only "requested" carries recovery work: "rerequested" re-plans through // check_run.rerequested and "completed" is pure noise, so neither may occupy // an inbox row. @@ -428,6 +449,27 @@ func TestDurableCheckSuiteSynthesizesMissingCoverage(t *testing.T) { assert.Equal(t, "12345", row.TenantID) } +// A durable row is normalized before config and GitHub routing so replayed +// rows from any producer use the same repository identity as ingress rows. +func TestDurableCheckSuiteCanonicalizesStoredRepository(t *testing.T) { + store := newRecordingWebhookEventStore() + h, mux := newCheckSuiteProcessHandler(t, store, map[string]api.RepoConfig{"octocat/hello-world": {}}) + mux.HandleFunc("/repos/octocat/hello-world/pulls/7", func(w http.ResponseWriter, _ *http.Request) { + writeSinglePR(t, w, 7, "open", "suite-sha") + }) + event := durableCheckSuiteEvent(t, "suite-sha", 7) + event.Repository = "OctoCat/Hello-World" + + retry, err := h.processDurableCheckSuite(t.Context(), event) + + require.NoError(t, err) + require.False(t, retry) + row, err := store.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, synthesizedDeliveryGUID("octocat/hello-world", 7, "suite-sha")) + require.NoError(t, err) + require.NotNil(t, row) + assert.Equal(t, "octocat/hello-world", row.Repository) +} + // Every payload PR still open at the suite head gets its own PR-scoped // recovery row — multiple PRs can share one head SHA. func TestDurableCheckSuiteSynthesizesPerPR(t *testing.T) { diff --git a/pkg/webhook/durable_dispatch.go b/pkg/webhook/durable_dispatch.go index a7686dcd6..5de9ea69a 100644 --- a/pkg/webhook/durable_dispatch.go +++ b/pkg/webhook/durable_dispatch.go @@ -675,7 +675,7 @@ func (h *Handler) processDurablePullRequest(ctx context.Context, event *storage. // delivery under the delivery lease. runPRCloseCleanup is idempotent, so a // retry after a partial cleanup reconciles rather than double-acts. func (h *Handler) processDurablePullRequestClosed(ctx context.Context, event *storage.WebhookEvent, payload pullRequestPayload) (retry bool, err error) { - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) pr := payload.PullRequest.Number if repo == "" || pr == 0 { return false, fmt.Errorf("durable pull_request closed delivery %s missing repo or PR", event.DeliveryID) @@ -700,7 +700,7 @@ func (h *Handler) processDurablePullRequestAutoPlan(ctx context.Context, event * return false, err } - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) pr := payload.PullRequest.Number headSHA := payload.PullRequest.Head.SHA if repo == "" || pr == 0 || headSHA == "" { @@ -792,7 +792,7 @@ func (h *Handler) processDurableCheckRun(ctx context.Context, event *storage.Web // participant convergence returns no error and is handed to the in-memory // re-fold budget through the shared follow-up, exactly as the wrapper does. func (h *Handler) processDurableCheckRunCompleted(ctx context.Context, event *storage.WebhookEvent, payload checkRunPayload) (retry bool, err error) { - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) if h.service == nil || !h.service.Config().IsAggregateLeaderForRepo(repo) { h.logger.Info("durable check_run completion ignored because deployment is not the aggregate leader", "delivery_id", event.DeliveryID, "repo", repo, "pr", event.PullRequest, @@ -877,7 +877,7 @@ func (h *Handler) processDurableCheckRunCompleted(ctx context.Context, event *st // delivery. A GitHub failure verifying the head keeps the delivery retryable // rather than completing it, so a transient outage cannot drop the re-plan. func (h *Handler) processDurableCheckRunRerequest(ctx context.Context, event *storage.WebhookEvent, payload checkRunPayload) (retry bool, err error) { - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) pr, ok := checkRunPullRequestNumber(payload) if !ok { h.logger.Info("durable check_run rerequest ignored without pull request", @@ -970,7 +970,7 @@ func (h *Handler) enqueueDurablePullRequest(ctx context.Context, payload pullReq DeliveryID: deliveryID, Event: "pull_request", Action: payload.Action, - Repository: payload.Repository.FullName, + Repository: storage.CanonicalKey(payload.Repository.FullName), PullRequest: payload.PullRequest.Number, HeadSHA: payload.PullRequest.Head.SHA, TenantID: strconv.FormatInt(installationID, 10), @@ -984,7 +984,7 @@ func (h *Handler) enqueueDurableCheckRun(ctx context.Context, payload checkRunPa DeliveryID: deliveryID, Event: "check_run", Action: payload.Action, - Repository: payload.Repository.FullName, + Repository: storage.CanonicalKey(payload.Repository.FullName), PullRequest: pr, HeadSHA: payload.CheckRun.HeadSHA, TenantID: strconv.FormatInt(installationID, 10), diff --git a/pkg/webhook/durable_dispatch_test.go b/pkg/webhook/durable_dispatch_test.go index 3c91c3ceb..a52c07465 100644 --- a/pkg/webhook/durable_dispatch_test.go +++ b/pkg/webhook/durable_dispatch_test.go @@ -240,6 +240,36 @@ func TestDurablePullRequestWebhookQueuesAndAcks(t *testing.T) { } } +func TestDurablePullRequestWebhookCanonicalizesRepository(t *testing.T) { + events := newRecordingWebhookEventStore() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + service := api.New(&durableWebhookTestStorage{webhookEvents: events}, &api.ServerConfig{ + Repos: map[string]api.RepoConfig{"mixedcase/sample-repo": {}}, + }, nil, logger) + h := NewHandler(service, &fakeClientFactory{}, nil, logger, WithDurableWebhookDispatch()) + + req := buildPRWebhookRequest(t, prWebhookPayloadOpts{ + action: "opened", repo: "MixedCase/Sample-Repo", headSHA: "MixedCaseSHA", headRef: "MixedCaseBranch", + }, nil) + req.Header.Set(headerDeliveryID, "mixed-case-pull-request") + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + require.JSONEq(t, `{"message":"auto-plan queued"}`, rr.Body.String()) + event, err := events.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-pull-request") + require.NoError(t, err) + require.NotNil(t, event) + assert.Equal(t, "mixedcase/sample-repo", event.Repository) + assert.Equal(t, "mixedcase/sample-repo#1", fmt.Sprintf("%s#%d", event.Repository, event.PullRequest)) + assert.Equal(t, "MixedCaseSHA", event.HeadSHA) + + var payload pullRequestPayload + require.NoError(t, json.Unmarshal(event.Payload, &payload)) + assert.Equal(t, "MixedCaseBranch", payload.PullRequest.Head.Ref) +} + func TestDurablePullRequestWebhookDeduplicatesDelivery(t *testing.T) { events := newRecordingWebhookEventStore() logger := slog.New(slog.NewTextHandler(io.Discard, nil)) diff --git a/pkg/webhook/durable_issue_comment_test.go b/pkg/webhook/durable_issue_comment_test.go index 283831373..ebdc8a856 100644 --- a/pkg/webhook/durable_issue_comment_test.go +++ b/pkg/webhook/durable_issue_comment_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/block/schemabot/pkg/api" @@ -171,6 +172,34 @@ func TestDurableIssueCommentCommandQueuesAndAcks(t *testing.T) { } } +func TestIssueCommentWebhookCanonicalizesRepository(t *testing.T) { + events := newRecordingWebhookEventStore() + h := newDurableIssueCommentEnqueueHandler(t, events) + req := buildWebhookRequest(t, webhookPayloadOpts{ + comment: "schemabot apply -e production", repo: "MixedCase/Sample-Repo", + userLogin: "MixedCaseUser", isPR: true, + }, nil) + req.Header.Set(headerDeliveryID, "mixed-case-issue-comment") + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + h.DrainInProcessWebhookWork(t.Context()) + + require.Equal(t, http.StatusOK, rr.Code) + event, err := events.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-issue-comment") + require.NoError(t, err) + require.NotNil(t, event) + assert.Equal(t, "mixedcase/sample-repo", event.Repository) + assert.Equal(t, "mixedcase/sample-repo#1", fmt.Sprintf("%s#%d", event.Repository, event.PullRequest)) + + var payload webhookPayload + require.NoError(t, json.Unmarshal(event.Payload, &payload)) + require.NotNil(t, payload.Comment) + require.NotNil(t, payload.Comment.User) + assert.Equal(t, "MixedCaseUser", payload.Comment.User.Login) + assert.Equal(t, "schemabot apply -e production", payload.Comment.Body) +} + // A redelivered apply command (same delivery GUID) is deduplicated to a single // inbox row, so a GitHub redelivery cannot double-run the command. func TestDurableIssueCommentDeduplicatesDelivery(t *testing.T) { diff --git a/pkg/webhook/durable_merge_group_test.go b/pkg/webhook/durable_merge_group_test.go index 7a6ae25ef..7a63ff5e0 100644 --- a/pkg/webhook/durable_merge_group_test.go +++ b/pkg/webhook/durable_merge_group_test.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -76,6 +77,31 @@ func TestDurableMergeGroupWebhookQueuesAndAcks(t *testing.T) { } } +func TestDurableMergeGroupWebhookCanonicalizesRepository(t *testing.T) { + events := newRecordingWebhookEventStore() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + service := api.New(&durableWebhookTestStorage{webhookEvents: events}, &api.ServerConfig{ + Repos: map[string]api.RepoConfig{"mixedcase/sample-repo": {}}, + }, nil, logger) + h := NewHandler(service, &fakeClientFactory{}, nil, logger, WithDurableWebhookDispatch()) + + req := buildMergeGroupWebhookRequest(t, "checks_requested", "MixedCaseSHA", nil) + body, err := io.ReadAll(req.Body) + require.NoError(t, err) + req.Body = io.NopCloser(strings.NewReader(strings.ReplaceAll(string(body), "octocat/hello-world", "MixedCase/Sample-Repo"))) + req.Header.Set(headerDeliveryID, "mixed-case-merge-group") + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + event, err := events.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-merge-group") + require.NoError(t, err) + require.NotNil(t, event) + assert.Equal(t, "mixedcase/sample-repo", event.Repository) + assert.Equal(t, "MixedCaseSHA", event.HeadSHA) +} + // A webhook redelivery reuses the delivery GUID, so the inbox deduplicates it to // a single row rather than queuing the same merge-group check twice. func TestDurableMergeGroupWebhookDeduplicatesDelivery(t *testing.T) { diff --git a/pkg/webhook/durable_push_test.go b/pkg/webhook/durable_push_test.go index 115e4679d..361461821 100644 --- a/pkg/webhook/durable_push_test.go +++ b/pkg/webhook/durable_push_test.go @@ -7,6 +7,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -74,6 +75,31 @@ func TestDurablePushWebhookQueuesAndAcks(t *testing.T) { } } +func TestDurablePushWebhookCanonicalizesRepository(t *testing.T) { + events := newRecordingWebhookEventStore() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + service := api.New(&durableWebhookTestStorage{webhookEvents: events}, &api.ServerConfig{ + Repos: map[string]api.RepoConfig{"mixedcase/sample-repo": {}}, + }, nil, logger) + h := NewHandler(service, &fakeClientFactory{}, nil, logger, WithDurableWebhookDispatch()) + + req := buildPushWebhookRequest(t, "refs/heads/main", "MixedCaseSHA", false) + body, err := io.ReadAll(req.Body) + require.NoError(t, err) + req.Body = io.NopCloser(strings.NewReader(strings.ReplaceAll(string(body), "octocat/hello-world", "MixedCase/Sample-Repo"))) + req.Header.Set(headerDeliveryID, "mixed-case-push") + rr := httptest.NewRecorder() + + h.ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code) + event, err := events.GetByDeliveryID(t.Context(), storage.WebhookProviderGitHub, "mixed-case-push") + require.NoError(t, err) + require.NotNil(t, event) + assert.Equal(t, "mixedcase/sample-repo", event.Repository) + assert.Equal(t, "MixedCaseSHA", event.HeadSHA) +} + // A non-default-branch push is filtered before enqueue, so no inbox row is // created — only default-branch commits keep the ruleset check source current. func TestDurablePushWebhookIgnoresFeatureBranch(t *testing.T) { diff --git a/pkg/webhook/durable_reconcile.go b/pkg/webhook/durable_reconcile.go index 57d8ac82a..637ee8c49 100644 --- a/pkg/webhook/durable_reconcile.go +++ b/pkg/webhook/durable_reconcile.go @@ -316,6 +316,7 @@ func synthesizedDeliveryGUID(repo string, pr int, headSHA string) string { // the enqueue are not atomic; a concurrent pod inserting between them can at // worst mislabel one metric increment, never affect the row itself. func (h *Handler) synthesizeMissingHeadDelivery(ctx context.Context, repo string, pr int, headSHA string, installationID int64) (inserted, resynthesized bool, err error) { + repo = storage.CanonicalKey(repo) guid := synthesizedDeliveryGUID(repo, pr, headSHA) if store := h.webhookEventStore(); store != nil { prior, err := store.GetByDeliveryID(ctx, storage.WebhookProviderGitHub, guid) diff --git a/pkg/webhook/durable_reconcile_integration_test.go b/pkg/webhook/durable_reconcile_integration_test.go index b3615871b..744ac943e 100644 --- a/pkg/webhook/durable_reconcile_integration_test.go +++ b/pkg/webhook/durable_reconcile_integration_test.go @@ -51,7 +51,7 @@ func setupSynthesizedDispatchTest(t *testing.T, dbName, headSHA string) (*Handle h := NewHandler(svc, &fakeClientFactory{client: installClient}, nil, logger, WithDurableWebhookDispatch(), WithWebhookReconciler(), WithWebhookReconcileSynthesis()) - inserted, resynthesized, err := h.synthesizeMissingHeadDelivery(t.Context(), "octocat/hello-world", 1, headSHA, 12345) + inserted, resynthesized, err := h.synthesizeMissingHeadDelivery(t.Context(), "OctoCat/Hello-World", 1, headSHA, 12345) require.NoError(t, err) require.True(t, inserted) require.False(t, resynthesized, "first synthesis for a head must not be labeled a resynthesis") @@ -61,6 +61,7 @@ func setupSynthesizedDispatchTest(t *testing.T, dbName, headSHA string) (*Handle require.NoError(t, err) require.NotNil(t, row) require.Equal(t, storage.WebhookEventPending, row.State) + require.Equal(t, "octocat/hello-world", row.Repository) return h, result, row } diff --git a/pkg/webhook/handler.go b/pkg/webhook/handler.go index f6e0d1aa0..b7f15e11b 100644 --- a/pkg/webhook/handler.go +++ b/pkg/webhook/handler.go @@ -969,7 +969,7 @@ func webhookMetadata(body []byte) (action, repo string) { if err := json.Unmarshal(body, &payload); err != nil { return "", "" } - return payload.Action, payload.Repository.FullName + return payload.Action, storage.CanonicalKey(payload.Repository.FullName) } // verifyHMAC validates a GitHub-style "sha256=" signature against the diff --git a/pkg/webhook/issue_comment.go b/pkg/webhook/issue_comment.go index f9fc4cc37..803a28361 100644 --- a/pkg/webhook/issue_comment.go +++ b/pkg/webhook/issue_comment.go @@ -123,6 +123,7 @@ func (h *Handler) handleIssueComment(ctx context.Context, metricApp string, w ht }) return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) var payloadInstallationID int64 if payload.Installation != nil { @@ -949,6 +950,7 @@ func (h *Handler) processDurableIssueComment(ctx context.Context, event *storage "repo", event.Repository, "pr", event.PullRequest) return false, nil } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) if payload.Comment.User != nil && payload.Comment.User.Type == "Bot" { h.logger.Info("durable issue_comment delivery ignored because the comment author is a bot", "delivery_id", event.DeliveryID, "repo", event.Repository, "pr", event.PullRequest) @@ -1065,14 +1067,15 @@ func durableIssueCommentCommand(event *storage.WebhookEvent) (CommandResult, str if err != nil { return CommandResult{}, "", 0, 0, "", err } - if payload.Repository.FullName == "" || payload.Issue.Number == 0 { + repo := storage.CanonicalKey(payload.Repository.FullName) + if repo == "" || payload.Issue.Number == 0 { return CommandResult{}, "", 0, 0, "", fmt.Errorf("durable issue_comment terminal notification %s is missing repo or PR", event.DeliveryID) } requestedBy := "" if payload.Comment.User != nil { requestedBy = payload.Comment.User.Login } - return result, payload.Repository.FullName, payload.Issue.Number, installationID, requestedBy, nil + return result, repo, payload.Issue.Number, installationID, requestedBy, nil } // durableIssueCommentCommandReady reports whether the driver implements the diff --git a/pkg/webhook/merge_group.go b/pkg/webhook/merge_group.go index 412631e8b..35cfdccf1 100644 --- a/pkg/webhook/merge_group.go +++ b/pkg/webhook/merge_group.go @@ -50,6 +50,7 @@ func (h *Handler) handleMergeGroup(ctx context.Context, metricApp string, w http h.writeError(w, http.StatusBadRequest, "invalid merge_group payload") return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) // GitHub sends "checks_requested" when a PR joins the queue and "destroyed" // when it leaves. Only checks_requested needs a check run on the new SHA. @@ -183,7 +184,7 @@ func (h *Handler) enqueueDurableMergeGroup(ctx context.Context, payload mergeGro DeliveryID: deliveryID, Event: "merge_group", Action: payload.Action, - Repository: payload.Repository.FullName, + Repository: storage.CanonicalKey(payload.Repository.FullName), HeadSHA: payload.MergeGroup.HeadSHA, TenantID: strconv.FormatInt(installationID, 10), Payload: body, @@ -209,7 +210,7 @@ func (h *Handler) processDurableMergeGroup(ctx context.Context, event *storage.W return false, nil } - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) headSHA := payload.MergeGroup.HeadSHA if repo == "" || headSHA == "" { return false, fmt.Errorf("durable merge_group delivery %s missing repo or head SHA", event.DeliveryID) diff --git a/pkg/webhook/pull_request.go b/pkg/webhook/pull_request.go index 7dbb0c43f..4125a294f 100644 --- a/pkg/webhook/pull_request.go +++ b/pkg/webhook/pull_request.go @@ -95,6 +95,7 @@ func (h *Handler) handlePullRequest(ctx context.Context, metricApp string, w htt h.writeError(w, http.StatusBadRequest, "invalid pull_request payload") return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) // Repo-level webhook deliveries carry no installation id in the payload; the // dispatcher resolves it and stashes it on the context. diff --git a/pkg/webhook/push.go b/pkg/webhook/push.go index 4d22d82af..55880ac9f 100644 --- a/pkg/webhook/push.go +++ b/pkg/webhook/push.go @@ -52,6 +52,7 @@ func (h *Handler) handlePush(ctx context.Context, metricApp string, w http.Respo h.writeError(w, http.StatusBadRequest, "invalid push payload") return } + payload.Repository.FullName = storage.CanonicalKey(payload.Repository.FullName) repo := payload.Repository.FullName headSHA := payload.After @@ -190,7 +191,7 @@ func (h *Handler) enqueueDurablePush(ctx context.Context, payload pushPayload, b Provider: storage.WebhookProviderGitHub, DeliveryID: deliveryID, Event: "push", - Repository: payload.Repository.FullName, + Repository: storage.CanonicalKey(payload.Repository.FullName), HeadSHA: payload.After, TenantID: strconv.FormatInt(installationID, 10), Payload: body, @@ -211,7 +212,7 @@ func (h *Handler) processDurablePush(ctx context.Context, event *storage.Webhook return false, fmt.Errorf("decode durable push delivery %s: %w", event.DeliveryID, err) } - repo := payload.Repository.FullName + repo := storage.CanonicalKey(payload.Repository.FullName) headSHA := payload.After // The deletion sentinel is an all-zeros SHA, not an empty one; an empty diff --git a/pkg/webhook/testhelpers_test.go b/pkg/webhook/testhelpers_test.go index f94e9aec2..df38b6d6f 100644 --- a/pkg/webhook/testhelpers_test.go +++ b/pkg/webhook/testhelpers_test.go @@ -79,7 +79,8 @@ func setupGitHubServer(t *testing.T) (*gh.Client, *http.ServeMux) { // prWebhookPayloadOpts configures how buildPRWebhookRequest constructs the payload. type prWebhookPayloadOpts struct { action string // "opened", "synchronize", "reopened", "closed", etc. - merged bool // for "closed": whether the PR merged or was closed without merging + repo string + merged bool // for "closed": whether the PR merged or was closed without merging beforeSHA string headSHA string headRef string @@ -108,6 +109,9 @@ func buildPRWebhookRequest(t *testing.T, opts prWebhookPayloadOpts, secret []byt if opts.headRef == "" { opts.headRef = "feature-branch" } + if opts.repo == "" { + opts.repo = "octocat/hello-world" + } payload := map[string]any{ "action": opts.action, @@ -123,7 +127,7 @@ func buildPRWebhookRequest(t *testing.T, opts prWebhookPayloadOpts, secret []byt }, }, "repository": map[string]any{ - "full_name": "octocat/hello-world", + "full_name": opts.repo, }, "installation": map[string]any{ "id": 12345, @@ -210,6 +214,7 @@ func buildCheckRunWebhookRequest(t *testing.T, opts checkRunWebhookPayloadOpts, // webhookPayloadOpts configures how buildWebhookRequest constructs the payload. type webhookPayloadOpts struct { comment string + repo string userType string // "User" or "Bot" userLogin string isPR bool // whether the issue has a pull_request field @@ -225,6 +230,9 @@ func buildWebhookRequest(t *testing.T, opts webhookPayloadOpts, secret []byte) * if opts.userType == "" { opts.userType = "User" } + if opts.repo == "" { + opts.repo = "octocat/hello-world" + } payload := map[string]any{ "action": "created", @@ -237,7 +245,7 @@ func buildWebhookRequest(t *testing.T, opts webhookPayloadOpts, secret []byte) * }, }, "repository": map[string]any{ - "full_name": "octocat/hello-world", + "full_name": opts.repo, }, "installation": map[string]any{ "id": 12345,