From 6f52f51ae1a68235cc5f891e655976bb8503337d Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 31 Aug 2026 15:37:10 +1000 Subject: [PATCH 1/5] fix(storage): canonicalize remaining identity keys Completes the store-boundary backstop: plans, plan comments, and webhook events get the same Go-side identity fold as locks/checks and applies/tasks so no table is left half-canonical and cross-table predicates behave identically on MySQL and PostgreSQL. Survey confirmed no other store carries identity predicates. Includes cross-dialect parity subtests. --- pkg/storage/canonical.go | 16 ++++++++ .../internal/sqlstore/plan_comments.go | 13 +++++++ pkg/storage/internal/sqlstore/plans.go | 15 +++++++ .../internal/sqlstore/webhook_events.go | 5 +++ pkg/storage/storagetest/plan_comments.go | 21 ++++++++++ pkg/storage/storagetest/plans.go | 39 +++++++++++++++++++ pkg/storage/storagetest/webhook_events.go | 28 +++++++++++++ 7 files changed, 137 insertions(+) create mode 100644 pkg/storage/canonical.go diff --git a/pkg/storage/canonical.go b/pkg/storage/canonical.go new file mode 100644 index 000000000..0d02a5acf --- /dev/null +++ b/pkg/storage/canonical.go @@ -0,0 +1,16 @@ +package storage + +import "strings" + +// CanonicalKey folds an identity string — repository full name, database +// name, database type, environment — to its canonical single-spelling form. +// Identity strings are canonicalized at ingress (webhook payload extraction, +// API request decode, comment-command parsing) and backstopped at the store +// boundary, so byte-wise comparisons and unique indexes behave identically on +// MySQL and PostgreSQL: MySQL's utf8mb4_0900_ai_ci storage collation forgives +// case drift while PostgreSQL compares bytes. Folding is lowercase-only; +// accent folding is deliberately excluded because identity strings are ASCII +// in practice. +func CanonicalKey(s string) string { + return strings.ToLower(s) +} diff --git a/pkg/storage/internal/sqlstore/plan_comments.go b/pkg/storage/internal/sqlstore/plan_comments.go index 4c7c70fbe..0a708ddde 100644 --- a/pkg/storage/internal/sqlstore/plan_comments.go +++ b/pkg/storage/internal/sqlstore/plan_comments.go @@ -24,6 +24,8 @@ type planCommentStore struct { // Insert stores a newly posted plan comment and sets comment.ID. func (s *planCommentStore) Insert(ctx context.Context, comment *storage.PlanComment) error { + canonicalizePlanCommentIdentity(comment) + id, err := s.identity.InsertID(ctx, s.db, ` INSERT INTO plan_comments (repository, pull_request, database_name, database_type, environment_scope, head_sha, github_comment_id, github_node_id) VALUES (?, ?, ?, ?, ?, ?, ?, ?) @@ -39,6 +41,9 @@ func (s *planCommentStore) Insert(ctx context.Context, comment *storage.PlanComm // ListUnminimizedForSlot returns the not-yet-minimized comments for a // (repository, pull_request, database) slot, ordered by id ascending. func (s *planCommentStore) ListUnminimizedForSlot(ctx context.Context, repo string, pr int, database, databaseType string) ([]*storage.PlanComment, error) { + repo = storage.CanonicalKey(repo) + database = storage.CanonicalKey(database) + databaseType = storage.CanonicalKey(databaseType) rows, err := s.db.QueryContext(ctx, ` SELECT `+planCommentColumns+` FROM plan_comments @@ -58,6 +63,7 @@ func (s *planCommentStore) ListUnminimizedForSlot(ctx context.Context, repo stri // database has no other way to ask. The (repository, pull_request) prefix of // the slot index serves it. func (s *planCommentStore) ListUnminimizedForRepoPR(ctx context.Context, repo string, pr int) ([]*storage.PlanComment, error) { + repo = storage.CanonicalKey(repo) rows, err := s.db.QueryContext(ctx, ` SELECT `+planCommentColumns+` FROM plan_comments @@ -101,6 +107,13 @@ func (s *planCommentStore) MarkMinimized(ctx context.Context, id int64) error { return nil } +func canonicalizePlanCommentIdentity(comment *storage.PlanComment) { + comment.Repository = storage.CanonicalKey(comment.Repository) + comment.DatabaseName = storage.CanonicalKey(comment.DatabaseName) + comment.DatabaseType = storage.CanonicalKey(comment.DatabaseType) + comment.EnvironmentScope = storage.CanonicalKey(comment.EnvironmentScope) +} + // scanPlanComment scans plan comment data from any scanner (Row or Rows). func scanPlanComment(s scanner) (*storage.PlanComment, error) { var comment storage.PlanComment diff --git a/pkg/storage/internal/sqlstore/plans.go b/pkg/storage/internal/sqlstore/plans.go index 81c73aaae..f4412de47 100644 --- a/pkg/storage/internal/sqlstore/plans.go +++ b/pkg/storage/internal/sqlstore/plans.go @@ -36,6 +36,8 @@ type planStore struct { // Create stores a new plan and returns its ID. func (s *planStore) Create(ctx context.Context, plan *storage.Plan) (int64, error) { + canonicalizePlanIdentity(plan) + planDataJSON, err := json.Marshal(namespacesWithShardPlans(plan)) if err != nil { return 0, fmt.Errorf("marshal plan data: %w", err) @@ -98,6 +100,7 @@ func (s *planStore) GetByLock(ctx context.Context, lockID int64) ([]*storage.Pla // latestPlanForTarget take the first matching row as "the newest plan" and rely // on this deterministic order to avoid picking an older SHA on ties. func (s *planStore) GetByPR(ctx context.Context, repo string, pr int) ([]*storage.Plan, error) { + repo = storage.CanonicalKey(repo) rows, err := s.db.QueryContext(ctx, ` SELECT `+planColumns+` FROM plans @@ -119,6 +122,10 @@ func (s *planStore) GetByPR(ctx context.Context, repo string, pr int) ([]*storag // plans.created_at is datetime (second precision), so the id tiebreaker keeps // same-second plans in a deterministic newest-first order. func (s *planStore) List(ctx context.Context, opts storage.ListPlansOptions) ([]*storage.Plan, error) { + opts.Database = storage.CanonicalKey(opts.Database) + opts.Environment = storage.CanonicalKey(opts.Environment) + opts.Repository = storage.CanonicalKey(opts.Repository) + if opts.Limit <= 0 { return nil, fmt.Errorf("list plans for database %q environment %q: limit must be positive, got %d", opts.Database, opts.Environment, opts.Limit) } @@ -176,10 +183,18 @@ func (s *planStore) Delete(ctx context.Context, id int64) error { // DeleteByPR removes all plans for a PR. func (s *planStore) DeleteByPR(ctx context.Context, repo string, pr int) error { + repo = storage.CanonicalKey(repo) _, err := s.db.ExecContext(ctx, `DELETE FROM plans WHERE repository = ? AND pull_request = ?`, repo, pr) return err } +func canonicalizePlanIdentity(plan *storage.Plan) { + plan.Database = storage.CanonicalKey(plan.Database) + plan.DatabaseType = storage.CanonicalKey(plan.DatabaseType) + plan.Repository = storage.CanonicalKey(plan.Repository) + plan.Environment = storage.CanonicalKey(plan.Environment) +} + // scanPlan scans a single plan row, returning nil if not found. func scanPlan(row *sql.Row) (*storage.Plan, error) { plan, err := scanPlanInto(row) diff --git a/pkg/storage/internal/sqlstore/webhook_events.go b/pkg/storage/internal/sqlstore/webhook_events.go index 1b61e700b..5f0a9fd8a 100644 --- a/pkg/storage/internal/sqlstore/webhook_events.go +++ b/pkg/storage/internal/sqlstore/webhook_events.go @@ -27,6 +27,8 @@ type webhookEventStore struct { } func (s *webhookEventStore) Create(ctx context.Context, event *storage.WebhookEvent) (bool, error) { + event.Repository = storage.CanonicalKey(event.Repository) + if event.DeliveryID == "" { return false, fmt.Errorf("webhook delivery ID is required") } @@ -182,6 +184,7 @@ func webhookClaimableArgs() []any { } func (s *webhookEventStore) HasEventForHead(ctx context.Context, provider, repository string, pullRequest int, headSHA string) (bool, error) { + repository = storage.CanonicalKey(repository) if provider == "" { provider = storage.WebhookProviderGitHub } @@ -267,6 +270,7 @@ func (s *webhookEventStore) coveringSuccessorQuery(provider string, event *stora // the live PR is worth a GitHub call at all — the common no-successor claim // stays storage-only. func (s *webhookEventStore) HasCoveringSuccessor(ctx context.Context, event *storage.WebhookEvent) (bool, error) { + event.Repository = storage.CanonicalKey(event.Repository) if event.Repository == "" || event.PullRequest == 0 { return false, fmt.Errorf("check covering successor for webhook event %d: repository and pull request are required for coalescing", event.ID) } @@ -311,6 +315,7 @@ func (s *webhookEventStore) HasCoveringSuccessor(ctx context.Context, event *sto // terminally failed / superseded rows never run — none of those may justify // discarding older work. func (s *webhookEventStore) SupersedeIfCovered(ctx context.Context, event *storage.WebhookEvent) (bool, error) { + event.Repository = storage.CanonicalKey(event.Repository) if event.LeaseToken == "" { return false, fmt.Errorf("webhook event lease token is required") } diff --git a/pkg/storage/storagetest/plan_comments.go b/pkg/storage/storagetest/plan_comments.go index 523e3433f..70659c685 100644 --- a/pkg/storage/storagetest/plan_comments.go +++ b/pkg/storage/storagetest/plan_comments.go @@ -40,6 +40,27 @@ func InsertPlanComment(t *testing.T, store storage.Storage, repo string, pr int, // behavior — a minimized comment drops out of the unminimized listings and // repeat or missing-id marks are no-ops. func TestPlanComments(t *testing.T, h Harness) { + t.Run("CanonicalizesIdentityKeys", func(t *testing.T) { + ctx := t.Context() + store := h.NewStorage(t) + + inserted := InsertPlanComment(t, store, "MixedCase/Sample-Repo", 42, "OrdersDB", "MySQL", "Production,Staging", "sha1", 100) + assert.Equal(t, "mixedcase/sample-repo", inserted.Repository) + assert.Equal(t, "ordersdb", inserted.DatabaseName) + assert.Equal(t, "mysql", inserted.DatabaseType) + assert.Equal(t, "production,staging", inserted.EnvironmentScope) + + comments, err := store.PlanComments().ListUnminimizedForSlot(ctx, "MIXEDCASE/SAMPLE-REPO", 42, "ORDERSDB", "MYSQL") + require.NoError(t, err) + require.Len(t, comments, 1) + assert.Equal(t, inserted.ID, comments[0].ID) + + comments, err = store.PlanComments().ListUnminimizedForRepoPR(ctx, "MIXEDCASE/SAMPLE-REPO", 42) + require.NoError(t, err) + require.Len(t, comments, 1) + assert.Equal(t, inserted.ID, comments[0].ID) + }) + t.Run("Insert_And_ListUnminimizedForSlot", func(t *testing.T) { ctx := t.Context() store := h.NewStorage(t) diff --git a/pkg/storage/storagetest/plans.go b/pkg/storage/storagetest/plans.go index 3ca60d7b8..af3f832cd 100644 --- a/pkg/storage/storagetest/plans.go +++ b/pkg/storage/storagetest/plans.go @@ -17,6 +17,45 @@ import ( // storage.ErrNotImplemented. When an implementation lands, it joins this // family. func TestPlans(t *testing.T, h Harness) { + t.Run("CanonicalizesIdentityKeys", func(t *testing.T) { + ctx := t.Context() + store := h.NewStorage(t) + plan := &storage.Plan{ + PlanIdentifier: "plan_mixed_case", + Database: "OrdersDB", + DatabaseType: "MySQL", + Repository: "MixedCase/Sample-Repo", + PullRequest: 42, + Environment: "Staging", + CreatedAt: time.Now().UTC().Truncate(time.Second), + } + _, err := store.Plans().Create(ctx, plan) + require.NoError(t, err) + + assert.Equal(t, "ordersdb", plan.Database) + assert.Equal(t, "mysql", plan.DatabaseType) + assert.Equal(t, "mixedcase/sample-repo", plan.Repository) + assert.Equal(t, "staging", plan.Environment) + + byPR, err := store.Plans().GetByPR(ctx, "MIXEDCASE/SAMPLE-REPO", 42) + require.NoError(t, err) + require.Len(t, byPR, 1) + assert.Equal(t, "plan_mixed_case", byPR[0].PlanIdentifier) + + listed, err := store.Plans().List(ctx, storage.ListPlansOptions{ + Database: "ORDERSDB", Environment: "STAGING", + Repository: "MIXEDCASE/SAMPLE-REPO", PullRequest: 42, Limit: 10, + }) + require.NoError(t, err) + require.Len(t, listed, 1) + assert.Equal(t, "plan_mixed_case", listed[0].PlanIdentifier) + + require.NoError(t, store.Plans().DeleteByPR(ctx, "MIXEDCASE/SAMPLE-REPO", 42)) + deleted, err := store.Plans().Get(ctx, "plan_mixed_case") + require.NoError(t, err) + assert.Nil(t, deleted) + }) + t.Run("Create_And_Get", func(t *testing.T) { ctx := t.Context() store := h.NewStorage(t) diff --git a/pkg/storage/storagetest/webhook_events.go b/pkg/storage/storagetest/webhook_events.go index 61844da36..43a8d2b71 100644 --- a/pkg/storage/storagetest/webhook_events.go +++ b/pkg/storage/storagetest/webhook_events.go @@ -107,6 +107,34 @@ func TestWebhookEvents(t *testing.T, h Harness) { return claimExpecting(t, store, "old") } + t.Run("CanonicalizesRepositoryKey", func(t *testing.T) { + ctx := t.Context() + store := h.NewStorage(t) + + event := newPullRequestEvent("delivery-mixed-case", "synchronize", 42, "mixed-head", time.Time{}) + event.Repository = "MixedCase/Sample-Repo" + createEvent(t, store, event) + assert.Equal(t, "mixedcase/sample-repo", event.Repository) + + found, err := store.WebhookEvents().HasEventForHead(ctx, storage.WebhookProviderGitHub, "MIXEDCASE/SAMPLE-REPO", 42, "mixed-head") + require.NoError(t, err) + assert.True(t, found) + + claimed := claimExpecting(t, store, "delivery-mixed-case") + claimed.Repository = "MIXEDCASE/SAMPLE-REPO" + successor := newPullRequestEvent("delivery-mixed-successor", "synchronize", 42, "new-head", time.Now().UTC().Add(time.Second)) + successor.Repository = "mixedcase/sample-repo" + createEvent(t, store, successor) + + covered, err := store.WebhookEvents().HasCoveringSuccessor(ctx, claimed) + require.NoError(t, err) + assert.True(t, covered) + claimed.Repository = "MIXEDCASE/SAMPLE-REPO" + superseded, err := store.WebhookEvents().SupersedeIfCovered(ctx, claimed) + require.NoError(t, err) + assert.True(t, superseded) + }) + t.Run("Create_DeduplicatesDeliveryID", func(t *testing.T) { ctx := t.Context() store := h.NewStorage(t) From 042d5470fab9d381aa6bae6e12f2c313f611221c Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 31 Aug 2026 17:00:29 +1000 Subject: [PATCH 2/5] fix(storage): stop mutating callers in webhook-event probes HasCoveringSuccessor and SupersedeIfCovered now fold into local copies so the documented read-only probe contract holds; Create/Insert in-place canonicalization is stated on the store godocs, and the cross-dialect load-bearing assertions are marked in storagetest. Addresses external review of pull/1218. --- pkg/storage/canonical.go | 17 ++++++++++------- .../internal/sqlstore/webhook_events.go | 19 +++++++++++-------- pkg/storage/storage.go | 3 +++ pkg/storage/storagetest/plan_comments.go | 1 + pkg/storage/storagetest/plans.go | 1 + pkg/storage/storagetest/webhook_events.go | 4 +++- 6 files changed, 29 insertions(+), 16 deletions(-) diff --git a/pkg/storage/canonical.go b/pkg/storage/canonical.go index 0d02a5acf..cf893ba4f 100644 --- a/pkg/storage/canonical.go +++ b/pkg/storage/canonical.go @@ -4,13 +4,16 @@ import "strings" // CanonicalKey folds an identity string — repository full name, database // name, database type, environment — to its canonical single-spelling form. -// Identity strings are canonicalized at ingress (webhook payload extraction, -// API request decode, comment-command parsing) and backstopped at the store -// boundary, so byte-wise comparisons and unique indexes behave identically on -// MySQL and PostgreSQL: MySQL's utf8mb4_0900_ai_ci storage collation forgives -// case drift while PostgreSQL compares bytes. Folding is lowercase-only; -// accent folding is deliberately excluded because identity strings are ASCII -// in practice. +// Identity strings are the cross-dialect row-identity keys: MySQL's +// utf8mb4_0900_ai_ci storage collation forgives case drift while PostgreSQL +// compares bytes, so every boundary that accepts an identity string folds it +// before matching or persisting. Folding is lowercase-only; accent folding is +// deliberately excluded because identity strings are ASCII in practice. +// +// Deliberately not folded: lock owner strings (audit metadata such as +// "Org/Repo#42", compared byte-wise on both sides of every ownership check), +// deployment names, table names, SHAs, and GitHub node IDs — these are either +// case-significant or opaque values, not row-identity keys. func CanonicalKey(s string) string { return strings.ToLower(s) } diff --git a/pkg/storage/internal/sqlstore/webhook_events.go b/pkg/storage/internal/sqlstore/webhook_events.go index 5f0a9fd8a..e2e149073 100644 --- a/pkg/storage/internal/sqlstore/webhook_events.go +++ b/pkg/storage/internal/sqlstore/webhook_events.go @@ -270,22 +270,23 @@ func (s *webhookEventStore) coveringSuccessorQuery(provider string, event *stora // the live PR is worth a GitHub call at all — the common no-successor claim // stays storage-only. func (s *webhookEventStore) HasCoveringSuccessor(ctx context.Context, event *storage.WebhookEvent) (bool, error) { - event.Repository = storage.CanonicalKey(event.Repository) - if event.Repository == "" || event.PullRequest == 0 { + queryEvent := *event + queryEvent.Repository = storage.CanonicalKey(queryEvent.Repository) + if queryEvent.Repository == "" || queryEvent.PullRequest == 0 { return false, fmt.Errorf("check covering successor for webhook event %d: repository and pull request are required for coalescing", event.ID) } provider := event.Provider if provider == "" { provider = storage.WebhookProviderGitHub } - query, args := s.coveringSuccessorQuery(provider, event) + query, args := s.coveringSuccessorQuery(provider, &queryEvent) var one int err := s.db.QueryRowContext(ctx, query, args...).Scan(&one) if errors.Is(err, sql.ErrNoRows) { return false, nil } if err != nil { - return false, fmt.Errorf("check covering successor for webhook event %d (repo=%s, pr=%d): %w", event.ID, event.Repository, event.PullRequest, err) + return false, fmt.Errorf("check covering successor for webhook event %d (repo=%s, pr=%d): %w", event.ID, queryEvent.Repository, event.PullRequest, err) } return true, nil } @@ -315,11 +316,11 @@ func (s *webhookEventStore) HasCoveringSuccessor(ctx context.Context, event *sto // terminally failed / superseded rows never run — none of those may justify // discarding older work. func (s *webhookEventStore) SupersedeIfCovered(ctx context.Context, event *storage.WebhookEvent) (bool, error) { - event.Repository = storage.CanonicalKey(event.Repository) + repository := storage.CanonicalKey(event.Repository) if event.LeaseToken == "" { return false, fmt.Errorf("webhook event lease token is required") } - if event.Repository == "" || event.PullRequest == 0 { + if repository == "" || event.PullRequest == 0 { return false, fmt.Errorf("supersede webhook event %d: repository and pull request are required for coalescing", event.ID) } provider := event.Provider @@ -327,7 +328,9 @@ func (s *webhookEventStore) SupersedeIfCovered(ctx context.Context, event *stora provider = storage.WebhookProviderGitHub } autoPlanIn := placeholders(len(storage.AutoPlanPullRequestActions)) - successorQuery, successorArgs := s.coveringSuccessorQuery(provider, event) + queryEvent := *event + queryEvent.Repository = repository + successorQuery, successorArgs := s.coveringSuccessorQuery(provider, &queryEvent) args := []any{storage.WebhookEventSuperseded, event.ID, event.LeaseToken, storage.WebhookEventProcessing} args = append(args, stringArgs(storage.AutoPlanPullRequestActions)...) args = append(args, successorArgs...) @@ -341,7 +344,7 @@ func (s *webhookEventStore) SupersedeIfCovered(ctx context.Context, event *stora ) `, args...) if err != nil { - return false, fmt.Errorf("supersede webhook event %d (repo=%s, pr=%d): %w", event.ID, event.Repository, event.PullRequest, err) + return false, fmt.Errorf("supersede webhook event %d (repo=%s, pr=%d): %w", event.ID, repository, event.PullRequest, err) } rows, err := result.RowsAffected() if err != nil { diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index e2e1cf4b2..eddc8376a 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -274,6 +274,7 @@ type SettingsStore interface { // primitive behind fast webhook acknowledgement: handlers can persist a delivery // before returning 2xx, and drivers can claim/retry the stored event after the // HTTP request has finished. +// Create canonicalizes the provided event's repository in place before persisting. type WebhookEventStore interface { // Create records a webhook delivery in the pending state. Returns // inserted=false when provider + delivery GUID already exists, so callers @@ -475,6 +476,7 @@ type ListPlansOptions struct { // PlanStore manages schema change plans. // Plans are created by Plan() and stored for Apply() and staleness detection. // Both GRPCClient and LocalClient are stateless - SchemaBot owns plan storage. +// Create canonicalizes the provided plan's repository, environment, database, and database type in place before persisting. type PlanStore interface { // Create stores a new plan and returns its ID. Returns error if plan_identifier already exists. Create(ctx context.Context, plan *Plan) (int64, error) @@ -1080,6 +1082,7 @@ const SummaryClaimStaleAfter = 2 * time.Minute // for comments actually posted; minimized_at is set only after the GitHub // minimize call succeeded, so an unminimized row is always retried by the next // supersede. +// Insert canonicalizes the provided comment's repository, environment scope, database, and database type in place before persisting. type PlanCommentStore interface { // Insert stores a newly posted plan comment and sets comment.ID. Insert(ctx context.Context, comment *PlanComment) error diff --git a/pkg/storage/storagetest/plan_comments.go b/pkg/storage/storagetest/plan_comments.go index 70659c685..f85f5eb09 100644 --- a/pkg/storage/storagetest/plan_comments.go +++ b/pkg/storage/storagetest/plan_comments.go @@ -45,6 +45,7 @@ func TestPlanComments(t *testing.T, h Harness) { store := h.NewStorage(t) inserted := InsertPlanComment(t, store, "MixedCase/Sample-Repo", 42, "OrdersDB", "MySQL", "Production,Staging", "sha1", 100) + // Stored-value equality is the cross-dialect check that identity keys are canonicalized before persistence. assert.Equal(t, "mixedcase/sample-repo", inserted.Repository) assert.Equal(t, "ordersdb", inserted.DatabaseName) assert.Equal(t, "mysql", inserted.DatabaseType) diff --git a/pkg/storage/storagetest/plans.go b/pkg/storage/storagetest/plans.go index af3f832cd..0023a1267 100644 --- a/pkg/storage/storagetest/plans.go +++ b/pkg/storage/storagetest/plans.go @@ -32,6 +32,7 @@ func TestPlans(t *testing.T, h Harness) { _, err := store.Plans().Create(ctx, plan) require.NoError(t, err) + // Stored-value equality is the cross-dialect check that identity keys are canonicalized before persistence. assert.Equal(t, "ordersdb", plan.Database) assert.Equal(t, "mysql", plan.DatabaseType) assert.Equal(t, "mixedcase/sample-repo", plan.Repository) diff --git a/pkg/storage/storagetest/webhook_events.go b/pkg/storage/storagetest/webhook_events.go index 43a8d2b71..a11a9469d 100644 --- a/pkg/storage/storagetest/webhook_events.go +++ b/pkg/storage/storagetest/webhook_events.go @@ -114,6 +114,7 @@ func TestWebhookEvents(t *testing.T, h Harness) { event := newPullRequestEvent("delivery-mixed-case", "synchronize", 42, "mixed-head", time.Time{}) event.Repository = "MixedCase/Sample-Repo" createEvent(t, store, event) + // Stored-value equality is the cross-dialect check that the repository is canonicalized before persistence. assert.Equal(t, "mixedcase/sample-repo", event.Repository) found, err := store.WebhookEvents().HasEventForHead(ctx, storage.WebhookProviderGitHub, "MIXEDCASE/SAMPLE-REPO", 42, "mixed-head") @@ -129,10 +130,11 @@ func TestWebhookEvents(t *testing.T, h Harness) { covered, err := store.WebhookEvents().HasCoveringSuccessor(ctx, claimed) require.NoError(t, err) assert.True(t, covered) - claimed.Repository = "MIXEDCASE/SAMPLE-REPO" + assert.Equal(t, "MIXEDCASE/SAMPLE-REPO", claimed.Repository) superseded, err := store.WebhookEvents().SupersedeIfCovered(ctx, claimed) require.NoError(t, err) assert.True(t, superseded) + assert.Equal(t, "MIXEDCASE/SAMPLE-REPO", claimed.Repository) }) t.Run("Create_DeduplicatesDeliveryID", func(t *testing.T) { From 19fe4560a4ff1e5d114cc03aaf984e5337ef36ff Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Mon, 31 Aug 2026 18:56:04 +1000 Subject: [PATCH 3/5] fix(storage): anchor coalescing successor timestamp to the claimed row Deriving the successor's ReceivedAt from the claimed event's stored timestamp instead of the wall clock makes the newness ordering in the canonicalization parity test deterministic. --- pkg/storage/storagetest/webhook_events.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/storage/storagetest/webhook_events.go b/pkg/storage/storagetest/webhook_events.go index a11a9469d..bf714e33f 100644 --- a/pkg/storage/storagetest/webhook_events.go +++ b/pkg/storage/storagetest/webhook_events.go @@ -123,7 +123,7 @@ func TestWebhookEvents(t *testing.T, h Harness) { claimed := claimExpecting(t, store, "delivery-mixed-case") claimed.Repository = "MIXEDCASE/SAMPLE-REPO" - successor := newPullRequestEvent("delivery-mixed-successor", "synchronize", 42, "new-head", time.Now().UTC().Add(time.Second)) + successor := newPullRequestEvent("delivery-mixed-successor", "synchronize", 42, "new-head", claimed.ReceivedAt.Add(time.Second)) successor.Repository = "mixedcase/sample-repo" createEvent(t, store, successor) From de9238cd788b275d4bfa6c27b483a1e2387d5df7 Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Tue, 1 Sep 2026 12:02:07 +1000 Subject: [PATCH 4/5] docs(storage): clarify which identity strings CanonicalKey skips and why --- pkg/storage/canonical.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/storage/canonical.go b/pkg/storage/canonical.go index cf893ba4f..3cd960951 100644 --- a/pkg/storage/canonical.go +++ b/pkg/storage/canonical.go @@ -10,10 +10,15 @@ import "strings" // before matching or persisting. Folding is lowercase-only; accent folding is // deliberately excluded because identity strings are ASCII in practice. // -// Deliberately not folded: lock owner strings (audit metadata such as -// "Org/Repo#42", compared byte-wise on both sides of every ownership check), -// deployment names, table names, SHAs, and GitHub node IDs — these are either -// case-significant or opaque values, not row-identity keys. +// Deliberately not folded here: lock owner strings ("org/repo#42") are the +// ownership predicate on lock acquire, release, and intent verification, but +// the repository they are derived from is folded at ingress, so owners are +// canonical by construction rather than re-folded at this boundary. +// Deployment names are identity keys too, but they are operator-controlled +// configuration that doubles as routing and schema-directory path components, +// so config validation rejects non-canonical spellings instead of silently +// rewriting them. Table names, SHAs, and GitHub node IDs are case-significant +// or opaque values, not row-identity keys. func CanonicalKey(s string) string { return strings.ToLower(s) } From 11d317a58b180be6bb1b309b903625a9a483bd2f Mon Sep 17 00:00:00 2001 From: Kiran Muddukrishna Date: Wed, 2 Sep 2026 08:25:29 +1000 Subject: [PATCH 5/5] fix(storage): stop folding plan comment environment scope EnvironmentScope is not a query predicate; the minimizer compares the stored value in Go against a scope built from configured environment names, so folding only the stored side breaks that comparison. Also fold the webhook event repository after input validation, and document that the coalescing reads leave the caller's event untouched. --- pkg/storage/internal/sqlstore/plan_comments.go | 6 +++++- pkg/storage/internal/sqlstore/webhook_events.go | 4 ++-- pkg/storage/storage.go | 10 ++++++++-- pkg/storage/storagetest/plan_comments.go | 4 +++- 4 files changed, 18 insertions(+), 6 deletions(-) diff --git a/pkg/storage/internal/sqlstore/plan_comments.go b/pkg/storage/internal/sqlstore/plan_comments.go index 0a708ddde..2842160d1 100644 --- a/pkg/storage/internal/sqlstore/plan_comments.go +++ b/pkg/storage/internal/sqlstore/plan_comments.go @@ -107,11 +107,15 @@ func (s *planCommentStore) MarkMinimized(ctx context.Context, id int64) error { return nil } +// canonicalizePlanCommentIdentity folds the identity keys that appear in this +// store's SQL predicates. EnvironmentScope is deliberately not folded: no +// query filters on it, and its consumers compare it in Go against values built +// fresh from configured environment names, so folding only the stored side +// would break those comparisons. func canonicalizePlanCommentIdentity(comment *storage.PlanComment) { comment.Repository = storage.CanonicalKey(comment.Repository) comment.DatabaseName = storage.CanonicalKey(comment.DatabaseName) comment.DatabaseType = storage.CanonicalKey(comment.DatabaseType) - comment.EnvironmentScope = storage.CanonicalKey(comment.EnvironmentScope) } // scanPlanComment scans plan comment data from any scanner (Row or Rows). diff --git a/pkg/storage/internal/sqlstore/webhook_events.go b/pkg/storage/internal/sqlstore/webhook_events.go index e2e149073..04ec83d59 100644 --- a/pkg/storage/internal/sqlstore/webhook_events.go +++ b/pkg/storage/internal/sqlstore/webhook_events.go @@ -27,14 +27,14 @@ type webhookEventStore struct { } func (s *webhookEventStore) Create(ctx context.Context, event *storage.WebhookEvent) (bool, error) { - event.Repository = storage.CanonicalKey(event.Repository) - if event.DeliveryID == "" { return false, fmt.Errorf("webhook delivery ID is required") } if event.Event == "" { return false, fmt.Errorf("webhook event type is required") } + event.Repository = storage.CanonicalKey(event.Repository) + provider := event.Provider if provider == "" { provider = storage.WebhookProviderGitHub diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 2d96ccba8..618887b27 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -276,7 +276,10 @@ type SettingsStore interface { // primitive behind fast webhook acknowledgement: handlers can persist a delivery // before returning 2xx, and drivers can claim/retry the stored event after the // HTTP request has finished. -// Create canonicalizes the provided event's repository in place before persisting. +// Create canonicalizes the provided event's repository in place before +// persisting. The coalescing reads (HasCoveringSuccessor, SupersedeIfCovered) +// fold the repository only inside their SQL predicates and leave the caller's +// event untouched. type WebhookEventStore interface { // Create records a webhook delivery in the pending state. Returns // inserted=false when provider + delivery GUID already exists, so callers @@ -1084,7 +1087,10 @@ const SummaryClaimStaleAfter = 2 * time.Minute // for comments actually posted; minimized_at is set only after the GitHub // minimize call succeeded, so an unminimized row is always retried by the next // supersede. -// Insert canonicalizes the provided comment's repository, environment scope, database, and database type in place before persisting. +// Insert canonicalizes the provided comment's repository, database, and +// database type in place before persisting. EnvironmentScope is stored as +// given: no query predicate filters on it, and its consumers compare it in Go +// against a scope built from the configured environment names. type PlanCommentStore interface { // Insert stores a newly posted plan comment and sets comment.ID. Insert(ctx context.Context, comment *PlanComment) error diff --git a/pkg/storage/storagetest/plan_comments.go b/pkg/storage/storagetest/plan_comments.go index f85f5eb09..9359ad067 100644 --- a/pkg/storage/storagetest/plan_comments.go +++ b/pkg/storage/storagetest/plan_comments.go @@ -49,7 +49,9 @@ func TestPlanComments(t *testing.T, h Harness) { assert.Equal(t, "mixedcase/sample-repo", inserted.Repository) assert.Equal(t, "ordersdb", inserted.DatabaseName) assert.Equal(t, "mysql", inserted.DatabaseType) - assert.Equal(t, "production,staging", inserted.EnvironmentScope) + // EnvironmentScope is not a query predicate and is compared in Go against + // a scope built from configured environment names, so it is stored as given. + assert.Equal(t, "Production,Staging", inserted.EnvironmentScope) comments, err := store.PlanComments().ListUnminimizedForSlot(ctx, "MIXEDCASE/SAMPLE-REPO", 42, "ORDERSDB", "MYSQL") require.NoError(t, err)