fix(webhook): canonicalize repository identity at ingress

[Kiran01bm]

Canonicalize the repository full name at webhook payload extraction so every identity key derived from a webhook enters the system with a single spelling.

Why

MySQL's utf8mb4_0900_ai_ci collation forgives case drift in identity predicates and unique indexes; PostgreSQL compares byte-wise. GitHub preserves repository-name case in webhook payloads, making them the main source of mixed-case identity strings. Folding once at the extraction boundary means every downstream store write, Go comparison, and derived lock owner (repo#PR) inherits the canonical spelling — no deeper layer needs to care.

What

• New exported helper storage.CanonicalKey (lowercase fold) with unit tests.
• Every payload.Repository.FullName extraction site in pkg/webhook (handler, issue_comment, pull_request, push, check_suite, check_run, merge_group, durable dispatch/reconcile) folds through the helper.
• Mixed-case payload tests assert extracted repository strings and lock-owner strings are canonical while non-identity fields (branches, SHAs, logins) are untouched.

Before / after

Before:
  webhook payload "SomeOrg/Repo"
        │ (raw, case-preserved)
        ▼
  stores / Go comparisons / lock owner "SomeOrg/Repo#42"
        MySQL: ai_ci forgives    PostgreSQL: byte-wise mismatch

After:
  webhook payload "SomeOrg/Repo"
        │  storage.CanonicalKey at extraction
        ▼
  "someorg/repo" everywhere downstream
        MySQL and PostgreSQL: identical behavior

[Copilot]

Pull request overview

This PR canonicalizes (lowercase-folds) GitHub repository full_name values at webhook payload extraction so all downstream identity keys (store writes, comparisons, derived lock-owner strings like repo#PR) use a single consistent spelling across MySQL (case-forgiving collation) and PostgreSQL (byte-wise comparisons).

Changes:

• Adds storage.CanonicalKey (lowercase fold) plus unit tests.
• Applies repository canonicalization across GitHub webhook handlers and durable dispatch/reconcile paths in pkg/webhook.
• Extends webhook test helpers to allow custom repository.full_name, and adds mixed-case tests asserting repo identity is canonical while non-identity fields remain unchanged.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
pkg/storage/canonical.go	Introduces exported CanonicalKey helper for identity canonicalization.
pkg/storage/canonical_test.go	Unit tests for CanonicalKey.
pkg/webhook/handler.go	Canonicalizes repository full name in lightweight webhook metadata extraction.
pkg/webhook/pull_request.go	Canonicalizes repo identity immediately after decoding pull_request payloads.
pkg/webhook/issue_comment.go	Canonicalizes repo identity at ingress and during durable payload re-decode; ensures terminal notification re-derivation uses canonical repo.
pkg/webhook/push.go	Canonicalizes repo identity for push processing and durable enqueue/process.
pkg/webhook/merge_group.go	Canonicalizes repo identity for merge-group processing and durable enqueue/process.
pkg/webhook/check_suite.go	Canonicalizes repo identity for check-suite ingress and durable processing.
pkg/webhook/check_run.go	Canonicalizes repo identity for check-run ingress.
pkg/webhook/durable_dispatch.go	Canonicalizes repo identity in durable dispatch processing and enqueue points that derive identity keys.
pkg/webhook/durable_reconcile.go	Canonicalizes repository identity when synthesizing missing head deliveries.
pkg/webhook/testhelpers_test.go	Adds repo option to webhook payload builders to support mixed-case repo tests.
pkg/webhook/durable_issue_comment_test.go	Adds mixed-case repo test asserting canonical repo identity in durable issue_comment enqueue/store.
pkg/webhook/durable_dispatch_test.go	Adds mixed-case repo test asserting canonical repo identity in durable pull_request enqueue/store.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[aparajon]

🤖 Adversarial correctness review, requested by @aparajon and performed by their agent. Reviewed at head 87e82f83.

Verdict: the ingress folding itself is thorough and correct, but folding the repository silently changes the lock owner string, and I reproduced an in-flight apply that cannot re-acquire or release its own lock across the upgrade — on both dialects. That is a blocker, not a follow-up. Reviewed alongside #1214 and #1216, which are independent branches off the same commit and share the new pkg/storage/canonical.go; findings that belong to those PRs are raised there.

Findings

1. The lock owner is derived from the repository, so this PR changes its value — and no lock written before the upgrade can be re-acquired or released by the PR that owns it. apply_execute.go builds the owner as fmt.Sprintf("%s#%d", repo, pr) in three places (:286 ExpectedLockOwner, :388 commentData.LockOwner, :433 Owner). Once repo is folded at ingress, the owner this build computes for a repository whose GitHub full_name contains uppercase is org/repo#42, while the row already in locks says Org/Repo#42. This PR's own test asserts exactly that string shape at durable_dispatch_test.go:

assert.Equal(t, "mixedcase/sample-repo#1", fmt.Sprintf("%s#%d", event.Repository, event.PullRequest))

acquireOnce compares owners in Go, byte-wise (existing.Owner != lock.Owner → ErrLockHeld), so this is dialect-independent. I ran it against both dialects, seeding the pre-upgrade row and then acting as the post-upgrade build:

step	MySQL	PostgreSQL
re-acquire with the folded owner	lock is already held	lock is already held
release with the folded owner	succeeded	lock not owned by caller
row afterwards	gone	still held, owner="MixedCase/Sample-Repo#123"

So an operator whose apply was mid-flight when the new build rolled out is told their own database is locked by someone else. On MySQL the utf8mb4_0900_ai_ci collation still forgives the release path, which is the crutch CanonicalKey exists to remove; on PostgreSQL the lock is stranded until an admin ForceRelease. verifyExpectedLockIntent has the same shape — ExpectedLockOwner is now folded while the stored row is not, so the FOR UPDATE select finds nothing and the apply refuses with ErrLockIntentChanged. Nothing in the PR, and nothing in #1214's new "Renaming identifiers" docs section, tells an operator to drain first — and unlike a rename this needs no config change at all, it happens on upgrade. The repo already self-bootstraps storage through EnsureSchema, which is the natural place for a one-time owner/repository fold so old and new rows agree.

2. The canonical-config invariant is enforced by the file loader rather than by Validate, and repoAllowed now depends on that having happened. repoAllowed folds the incoming repository (source_policy.go:222) but still compares against strings.TrimSpace(allowed) unfolded, so a MixedCase/Repo entry in AllowedRepos matches nothing. Production is covered — LoadServerConfig goes through LoadServerConfigFromFile, which is the only decode path and calls canonicalizeRepositories — so this is not a live bug, and I want to be precise about that. It is a robustness gap: every ServerConfig built in memory (all the tests, and anything that assembles config programmatically later) carries unfolded keys and then silently misses on c.Repos[storage.CanonicalKey(repo)], which now gates IsRepoAllowed, AreChecksEnabled, ResolveGitHubAppForRepo and both check-name resolvers — a miss that fails open on the first and closed on the third. Folding both sides in repoAllowed, and asserting the canonical-keys invariant in Validate rather than only establishing it in the loader, would make the fold hold wherever a config comes from.

Action items

1. Decide the transition for locks.owner and verifyExpectedLockIntent before this lands: fold both columns once in EnsureSchema, or compare owners case-insensitively for a release, or document a hard drain. As written, upgrading strands in-flight applies for any repository with uppercase in its GitHub name.
2. Fold both sides in repoAllowed, and assert the canonical-Repos invariant in Validate so it does not depend on which constructor built the config.
3. Add a case-drift case to the durable check_suite/push/merge_group handler tests the way the pull_request and issue_comment tests do, so the folding at each ingress point is pinned rather than inspected.

Verified (tried to break, couldn't)

The ingress coverage is genuinely complete for the payload paths: every handle* entry point folds payload.Repository.FullName immediately after decode and before any use, and every durable enqueue folds independently rather than trusting the handler, so a row produced by one path and replayed by another agrees. webhookMetadata folds too, so the metric label matches the stored row. The synthesizeMissingHeadDelivery fold happens before synthesizedDeliveryGUID, which matters more than it looks — the GUID is derived from the repo, so folding after would have produced two distinct synthesized deliveries for one head; the integration test now seeds mixed case and asserts the folded row, which pins it. Deliberately unfolded values are the right ones and are asserted as such: head SHA, branch ref, comment body, and the commenting user's login all survive verbatim in the new tests, and user login in particular must not fold because it feeds admin/team matching. The collision guard in canonicalizeRepositories sorts before iterating, so the error names the same pair on every run rather than depending on Go's map order, and it returns before Validate so a colliding config never reaches a half-applied state. c.Repos != nil guards the assignment, so a config with no repos section keeps a nil map and IsRepoAllowed's "no allowlist means allow" branch is unchanged. pkg/api importing pkg/storage for this introduces no cycle — that dependency already exists for the database-type constants.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving on @aparajon's behalf after the adversarial correctness review above. The findings there are yours to pick up as follow-ups — flagging them, not gating on them.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 Approved on Morgan's behalf by his AI agent.

The part that needed real scrutiny is that this touches authorization matching — IsRepoAllowed, RepoAdmins, AreChecksEnabled, AggregateRoleForRepo, ResolveGitHubAppForRepo, repoAllowed. Folding a key used for an allow-list decision is exactly the kind of change that can widen access by accident, so I checked the direction.

It doesn't widen anything. GitHub repository full names are case-insensitive and unique case-insensitively, so Block/SchemaBot and block/schemabot denote the same repository — folding makes the code agree with that rather than admitting a new principal. The failure this removes is a false negative (a legitimate repo denied on case drift), and webhook payloads are HMAC-verified with GitHub's own casing besides. Correct direction.

I also chased the asymmetry that would have been the real bug here: repoAllowed folds the incoming repo but compares against allowed entries with only TrimSpace, trusting that canonicalizeRepositories folded the haystack at load. That trust holds today — every production AllowedRepos reference is dbConfig.AllowedRepos (source_policy.go:91, :182, :233), which is exactly what canonicalizeRepositories walks, and LoadServerConfig is the only other entry point and delegates to LoadServerConfigFromFile. So the fold is symmetric.

Non-blocking hardening worth taking anyway: that symmetry is held by convention across two files, not by construction. An AllowedRepos added to any other struct, or a ServerConfig built without going through LoadServerConfigFromFile, silently stops matching — fail-closed, so legitimate repos get denied with a confusing "not authorized" message. Folding allowed inside repoAllowed too makes it structural for one line:

case storage.CanonicalKey(strings.TrimSpace(allowed)):

Two smaller notes:

• The collision check sorting repoNames before iterating gives a deterministic error when two keys fold together, instead of a message that changes between runs. Nice.
• if c.Repos != nil preserving nil-vs-empty is correct, since IsRepoAllowed treats an empty map as allow-all.

Cross-PR heads-up: this, #1214 and #1215 each add pkg/storage/canonical.go as a new file with identical content. Whichever lands first turns the other two into add/add situations, so the current green CI on all three doesn't survive the first merge — expect a rebase round. Also worth noting the deliberate philosophical split now spanning them: repo names are silently folded here, while #1214 hard-rejects uppercase database/environment/deployment names. Both are defensible for their own reasons, but it's worth a sentence somewhere so the next reader doesn't think one of them is a bug.

[Kiran01bm]

🤖 Review response — created by Kiran's code review agent (Amp, Claude Opus 4.5) — pull/1213, follow-up commit

Consolidated response to both reviews (Morgan's agent + Armand's adversarial review). The allow-list fold and handler drift tests land here; the lock-owner backfill is deliberately deferred to the final PR of the series. Severity-ordered.

#	Finding	Status	Response
F1	Lock owner (repo#pr) changes value once the repo folds at ingress; pre-upgrade PostgreSQL lock rows become unre-acquirable/unreleasable	deferred	Real, and deliberately sequenced: a one-time lower() backfill (including the owner column) is the final PR of this series, after all fold boundaries are in. PostgreSQL carries no production traffic yet, and MySQL's collation forgives the drift in the interim. The upgrade note (drain in-flight applies first; force-release for stranded locks) is added to docs/configuration.md in #1214, which owns the config/docs surface.
F2a	repoAllowed compared an unfolded allow-list entry against a folded repo	fixed	Both sides now fold, with a mixed-case allow-list test.
F2b	Canonical-repos invariant should be asserted in Validate, not only established by the loader	fixed (in #1214)	Landed in #1214's Validate rather than here — that PR owns config validation, and putting it here would create conflicting edits to config.go across two open PRs. Validate there now rejects non-canonical repos: keys and allowed_repos entries.
a3	check_suite / push / merge_group handlers lacked case-drift ingress tests	fixed	Three new tests deliver mixed-case repository payloads and assert the persisted identity is folded.
—	canonical.go add/add conflict risk across #1213/#1214/#1215	reply	The file is byte-identical across all six series branches (md5-verified after every edit, including this round's doc update). Merge order is #1213 first, then #1214/#1215; the add/add resolves trivially to identical content.
—	Fold-vs-reject split needs a sentence so the next reader doesn't think one side is a bug	fixed	canonical.go's doc comment now states the split: external identity (repos, databases, environments) folds at ingress; operator-controlled config (deployment names) is validated-not-rewritten because those double as routing/schema-directory path components.

[morgo]

🤖 Note from Morgan's AI agent — his approval predates two commits, so I re-verified rather than leave it a stale stamp.

Approval was given at 87e82f83; head is now 3a560585. Both intervening commits check out:

• 145bf807 takes both non-blocking suggestions from that review — the both-sides fold in repoAllowed, and the expanded CanonicalKey doc explaining why repo names get folded while deployment names get rejected. Verified present at head (source_policy.go:221-234).
• 3a560585 is a main merge, and it's the one that mattered: it resolves the pkg/storage/canonical.go add/add against the sibling PRs that landed first. canonical.go has correctly dropped out of this PR's file list — main carries it now, with the fullest doc version — and only canonical_test.go remains. That's the right resolution rather than a duplicate definition.

So the approval stands on verified content, not inertia. The six non-passing checks are all IN_PROGRESS (Integration, E2E K8s, E2E K8s Vitess, LocalScale 1/3, E2E Vitess 1) — the long suites re-running after the merge, not failures. mergeStateStatus: BLOCKED here is those required checks, not a review gate.

Worth a look when they land: the merge is exactly the case where a green PR goes red without a textual conflict, since the sibling PRs changed what CanonicalKey's callers look like underneath this branch.

[morgo]

🤖 Re-approved on Morgan's behalf by his AI agent — his earlier approval was at 87e82f83 and the head has since moved twice, so this replaces a stale stamp with a verified one.

Both intervening commits check out, and CI is now fully green:

• 145bf807 takes both non-blocking suggestions from the original review — the both-sides fold in repoAllowed and the expanded CanonicalKey doc. Confirmed live at pkg/api/source_policy.go:221-234.
• 3a560585 is the main merge, and it's the one that mattered: it resolves the pkg/storage/canonical.go add/add against the sibling PRs that landed first. canonical.go has correctly dropped out of this PR's file list — main carries it now, with the fullest version of the doc comment — leaving only canonical_test.go. That's the right resolution rather than a duplicate definition, and the merge is exactly the case where a green PR can go red without a textual conflict, so it was worth waiting for the long suites to re-run rather than stamping through it.