fix(storage): canonicalize remaining identity keys

[Kiran01bm]

Fold the remaining store identity keys — plans, plan comments, and webhook events — completing the storage-layer canonicalization backstop.

Why

With locks/checks and applies/tasks folded by sibling PRs, any store left unfolded would make the layer half-canonical: cross-table predicates and cleanup paths would diverge between MySQL (ai_ci, forgiving) and PostgreSQL (byte-wise). A survey of every remaining store file confirmed these three are the only ones carrying identity predicates.

What

• plans.go: Create, GetByPR, DeleteByPR, List fold identity fields (repository, database, type, environment).
• plan_comments.go: Insert, ListUnminimizedForSlot, ListUnminimizedForRepoPR fold the same fields.
• webhook_events.go: Create, HasEventForHead, HasCoveringSuccessor, SupersedeIfCovered fold the repository key.
• No identity predicates found in apply_comments, apply_logs, control_requests, identity, or settings stores — left unchanged.
• Go-side folds only; no SQL LOWER().
• Cross-dialect parity subtests for all three stores.

Before / after

Before (PostgreSQL):
  plans row: repository = "SomeOrg/Repo"
  DeleteByPR("someorg/repo", 42) ──▶ 0 rows   ← orphaned plan rows
  HasEventForHead("someorg/repo", sha) ──▶ false ← duplicate dispatch

After (both dialects):
  all identity keys stored and queried as "someorg/repo"
  delete/dedup predicates match identically on MySQL and PostgreSQL

[Copilot]

Pull request overview

This PR completes storage-layer identity canonicalization for the remaining stores (plans, plan comments, webhook events) by folding repository/database/environment identity keys to a single canonical spelling at store boundaries, ensuring consistent behavior across MySQL (case-forgiving collation) and PostgreSQL (byte-wise comparisons).

Changes:

• Introduces storage.CanonicalKey and applies it to identity fields on create/insert paths and on read/list/delete predicates in the affected SQL stores.
• Adds/extends storage parity tests to assert identity canonicalization behavior for plans, plan comments, and webhook event coalescing/head-coverage queries.

Reviewed changes

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

Show a summary per file

File	Description
pkg/storage/canonical.go	Adds shared helper for lowercasing identity keys to a canonical form.
pkg/storage/internal/sqlstore/plans.go	Canonicalizes plan identity fields on Create and folds repo/db/env filters on query/delete paths.
pkg/storage/internal/sqlstore/plan_comments.go	Canonicalizes plan comment identity fields on Insert and folds repo/db/dbType on list queries.
pkg/storage/internal/sqlstore/webhook_events.go	Canonicalizes repository key on Create and on repository-based lookup/coalescing paths.
pkg/storage/storagetest/plans.go	Adds parity test ensuring PlanStore canonicalizes identity keys across dialects.
pkg/storage/storagetest/plan_comments.go	Adds parity test ensuring PlanCommentStore canonicalizes identity keys across dialects.
pkg/storage/storagetest/webhook_events.go	Adds parity test ensuring WebhookEventStore canonicalizes repository key for head-coverage and coalescing behaviors.

---

💡 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 47d1c802.

Verdict: the survey behind this one holds up — I checked every remaining store myself and the three you folded really are the only ones carrying identity predicates — but one of the four columns folded here is the wrong kind of key, and it is the one nothing queries on. Worth noting for whoever merges: with #1216 in and #1217 still open, the storage layer is half-folded on main right now, so this family wants to finish rather than pause.

Findings

1. environment_scope is folded on write, is never a SQL predicate, and its only two consumers compare it in Go against values folded inconsistently. No query in plan_comments.go filters on environment_scope — ListUnminimizedForSlot keys on repository/pull_request/database_name/database_type and ListUnminimizedForRepoPR on repository/pull_request — so the fold buys no cross-dialect parity. It does change two == comparisons. planCommentSupersedes (plan_comment_minimize.go:273) is safe by accident: posted has already been through Insert, so both sides are folded. pull_request.go:320 is not — expectedScope is built fresh from planCommentSlot.environmentScope() and never folded, while v.EnvironmentScope now comes back folded. Today that is inert, because configured environment names are validated lowercase, but the safety of this column now rests on an invariant enforced a layer away in config rather than on anything in this store; if it ever loosens, auto-plan stops recognizing its own prior comment and posts a redundant plan comment on every webhook delivery, with no error and no log to explain it. Separately, and independent of that invariant: environmentScope() sorts case-sensitively before joining, so folding the joined result is not canonical — I ran it, and {Staging, production} folds to "staging,production" while the same set spelled lowercase yields "production,staging". Two canonical keys for one environment set. The cleanest fix is to drop this one field from the fold, since nothing queries it; if you want it canonical anyway, fold the elements before sorting and fold expectedScope at the comparison site too.

2. No backfill, so the "before" behavior in your diagram survives for rows already written. Same shape as the rest of the family, and this is the fourth PR carrying it, so it is worth one decision rather than four. On PostgreSQL a pre-canonical plans row stays invisible to the folded DeleteByPR, which is exactly the orphaned-plan symptom the PR body shows as "before" — the fix prevents new ones and leaves existing ones stranded. It is narrower than it looks: Plans().Get keys on the plan identifier, so an apply still resolves its plan, and the affected reads are the PR-scoped cleanup and listing paths. The webhook-event side points the safe way in every direction I could find — a missed HasEventForHead costs a duplicate row and a duplicate dispatch, and a missed successor in HasCoveringSuccessor/SupersedeIfCovered means the event gets processed rather than discarded — but the same window reopens briefly during a rolling deploy, when one pod writes folded and another writes raw.

3. (nit) One store now has two opposite mutation contracts, and the interface documents only one. Create folds the caller's event in place (and does so before the DeliveryID == "" validation, so a rejected call still mutates the argument), while HasCoveringSuccessor and SupersedeIfCovered deliberately copy into a local queryEvent and leave the caller's value alone — the new subtest pins that asymmetry with assert.Equal(t, "MIXEDCASE/SAMPLE-REPO", claimed.Repository). That is a defensible split, but the added interface doc only mentions Create, so the deliberate half reads as an oversight.

Action items

1. (Finding 1) Drop EnvironmentScope from canonicalizePlanCommentIdentity — nothing queries the column — or, if it should stay canonical, fold the environment names before sort.Strings in environmentScope() and fold expectedScope at pull_request.go:320 so both comparands agree.
2. (Finding 2) Settle the family's backfill story once — a one-shot fold of stored identity columns, or an explicit statement that pre-canonical rows are abandoned and why that is acceptable — and point this PR, fix(webhook): canonicalize repository identity at ingress #1213, and fix(storage): canonicalize apply and task identity keys #1217 at it.
3. (optional) Say in the WebhookEventStore doc that the coalescing reads leave the caller's event untouched, so the split contract reads as intended.
4. (optional) Fold in Create after the DeliveryID validation, so a rejected call leaves the caller's struct alone.

Verified (tried to break, couldn't)

The survey claim is the load-bearing one and it holds: I read every remaining store file and apply_comments, apply_logs, control_requests, identity, and settings key exclusively on apply_id, id, or setting_key, with no identity predicate among them, and reaper.go carries none either — so these three really are the remainder. I also checked that this PR cannot create the cross-table hazard its siblings can: plans, plan comments, and webhook events are each keyed by their own columns or by numeric ids, with no correlated a.repository = b.repository join anywhere in the three files, so folding them cannot desynchronize a guard the way a half-folded checks/applies pair does. ListPlansOptions has no DatabaseType field, so folding plan.DatabaseType on Create is write-side only and cannot strand a row from List; List's three folds line up exactly with its three optional string predicates, and the PullRequest-requires-Repository guard still fires on the folded value. SupersedeIfCovered's UPDATE keys on event.ID plus the lease token rather than on identity, so folding cannot widen what it supersedes, and the lease-token and provider-default branches are untouched. Copilot's point about anchoring the successor timestamp is addressed on the head — the new subtest uses claimed.ReceivedAt.Add(time.Second) rather than wall clock. No test functions were removed and no assertions weakened anywhere in the diff, and the three new subtests assert specific stored spellings rather than mere non-emptiness. go build ./... plus the pkg/storage/... and pkg/webhook/... suites pass locally at head, and CI is 38/38 green including both dialect parity jobs.

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.

37/37 green, +137/-6, and it completes the storage-layer series cleanly. I verified the parts that could have made it a partial no-op rather than trusting the description or the tests:

• SupersedeIfCovered actually folds into the query, not just the guard. The diff only shows the error string switching to the local repository, which would be cosmetic on its own. Read at head, it also does queryEvent := *event; queryEvent.Repository = repository and passes &queryEvent into coveringSuccessorQuery, so the fold reaches successorArgs. The outer UPDATE predicate is id = ? AND lease_token = ? with no repository term, so the subquery was the only place it mattered — correct.
• The read/write mutation split is deliberate and right. Write paths (plans.Create, plan_comments.Insert, webhook_events.Create) fold in place; read paths (HasCoveringSuccessor, SupersedeIfCovered) copy the struct or use a local and leave the caller's event alone. The storagetest assertions pin both halves (assert.Equal(t, "MIXEDCASE/SAMPLE-REPO", claimed.Repository) after two read calls). That's a better contract than the uniform in-place folding in #1217, and documenting it on the interfaces in storage.go closes the nit I left there.
• Guard equivalence holds: CanonicalKey("") is "", so moving the empty checks onto the folded value changes nothing.

The finding worth carrying forward — and it isn't specific to this PR. The Before/After frames orphaned plan rows and duplicate dispatch as the bug being fixed, but folding fixes them going forward only. There's no backfill here, and identity keys arrive from GitHub with stable casing, so for any repository whose name contains capitals, rows written before this deploys are stored SomeOrg/Repo and rows written after are someorg/repo. On PostgreSQL, DeleteByPR("someorg/repo", …) will never match the old ones — so the orphan class described in the PR body is prevented, not remediated, and the pre-deploy orphans become permanent. Same shape for HasEventForHead (one duplicate dispatch per in-flight head at deploy time) and for ListUnminimizedForSlot (old comments stop being minimized).

Not blocking, for three reasons: MySQL is unaffected (utf8mb4_0900_ai_ci still matches the old rows against folded predicates, so the live production path is inert here); it isn't a regression, since those rows were already mismatched against the stores folded by #1213/#1216/#1217; and the direction of travel is right. But this is the fourth PR widening the same surface, and the backfill is a single cross-cutting job rather than something any one of these PRs should carry. Worth one follow-up issue covering all four stores — or an explicit "PostgreSQL deployments predating the canonicalization series need a one-time UPDATE … SET repository = LOWER(repository) sweep" note in the release notes, so an operator isn't left to discover it through a cleanup that silently stops working.

Two nits, neither blocking:

• The new sentences in storage.go are appended to the type doc comment (// Create canonicalizes … directly above type PlanStore interface), while Create has its own doc comment a few lines below. The method comment is the natural home; as placed, the contract reads as belonging to the interface rather than the call.
• plans.List folds opts.Database/Environment before the limit validation, so a bad-limit error now echoes the folded spelling back at the caller (list plans for database "proddb" when they passed ProdDB). Purely diagnostic.

[aparajon]

🤖 Adversarial correctness review (sweep pickup). Reviewed at head 47d1c80.

Verdict: correct and safe to land. Two prior agent reviews already sit on this PR; I verified their load-bearing claims independently rather than taking them on trust, and everything checked out. My own attack pass found no new correctness failure — the findings below are cross-references plus one piece of good news.

Findings

1. The prior review's environment-scope finding stands, and I confirmed both halves independently. canonicalizePlanCommentIdentity folds EnvironmentScope, which is never a SQL predicate, and its two Go consumers compare it against values folded inconsistently: planCommentSupersedes compares store-round-tripped values on both sides (safe), but pull_request.go builds expectedScope fresh from planCommentSlot.environmentScope() — unfolded — against a now-folded stored value. Today this is inert only because config validation (#1214) rejects non-lowercase environment names, so the column's correctness rests on an invariant enforced a layer away. I also confirmed the sort-before-fold non-canonicality: environmentScope() sorts case-sensitively, so {Staging, production} and {staging, production} produce differently-ordered joined strings. See the prior review's action item 1 — dropping the field from the fold remains the cleanest fix.

2. (resolved) The backfill question the prior review asked the family to settle is settled — by #1231. The storage canonicalize-identity-keys admin subcommand (approved, in this same series) is the one-shot fold of pre-canonical stored rows that action item 2 asked for. Once #1231 lands and runs, the "pre-canonical plans row invisible to folded DeleteByPR" window closes, leaving only the brief rolling-deploy overlap, which points the safe way (duplicate dispatch / extra processing, never lost work). Worth one line in the merge order: this PR is safe to land before #1231, but the backfill should run before anyone relies on PR-scoped cleanup against pre-canonical rows on PostgreSQL.

3. (nit, carried) WebhookEventStore.Create folds the caller's event in place before the DeliveryID validation, so a rejected call still mutates its argument — while HasCoveringSuccessor/SupersedeIfCovered deliberately leave the caller's event untouched (the new subtest pins that asymmetry). Already flagged as optional in the prior review; still worth the two-line fix.

Action items

1. (Finding 1) Same as the prior review's item 1 — drop EnvironmentScope from the fold, or make both comparands agree.
2. (Finding 2) In whichever PR merges last in this family, note that feat(cli): add storage canonicalize-identity-keys admin subcommand #1231's backfill must run before pre-canonical PostgreSQL rows are relied on by folded predicates.

Verified (tried to break, couldn't)

The survey claim (the load-bearing one)

• Independently grepped every remaining store file at head: apply_comments, apply_logs, control_requests, identity, settings, apply_operations, and reaper carry no identity-string predicates — repository/database/environment appear in none of their WHERE clauses. The three folded stores really are the remainder.

Fold placement and callers

• Every read predicate folds the same fields its write path folds — List's three folds line up with its three optional predicates, GetByPR/DeleteByPR/HasEventForHead fold the repository they filter on.
• The in-place Create mutation is safe at both real plan call sites: pkg/tern/local_client.go only sets plan.ID afterward, and pkg/api/plan_handlers.go discards the struct. The plan-comment Insert caller feeds the folded posted into minimizeSupersededPlanComments, where both comparands round-trip through the store.
• SupersedeIfCovered's UPDATE keys on event ID + lease token, so folding cannot widen what it supersedes; the lease and provider-default branches are untouched.

Tests

• No test deletions or weakened assertions anywhere in the diff; the three new parity subtests assert exact stored spellings, and the webhook subtest anchors its successor timestamp to claimed.ReceivedAt rather than wall clock.
• go build ./... plus pkg/storage/... and pkg/webhook/... green locally at head 47d1c80; CI fully green including both dialect parity jobs.

This review was performed by Cato, an AI review agent operated by Armand.

[aparajon]

🤖 Approving on Armand'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 Cato, an AI review agent operated by Armand.

[morgo]

🤖 Re-approving on Morgan's behalf at 11d317a5 (automated review).

Force-pushed past my earlier approval, and this one carries a genuine semantic reversal, so I checked the premise rather than the commit message.

fix(storage): stop folding plan comment environment scope removes EnvironmentScope from canonicalizePlanCommentIdentity. The whole change rests on one claim — that no query predicate filters on it — and that claim is true. In plan_comments.go at this head, environment_scope appears only in planCommentColumns, the INSERT column list, and the scan target. All three WHERE clauses filter on repository, pull_request, database_name, database_type, id, and minimized_at; none touches it.

So the fold was buying nothing on the SQL side while actively breaking the Go side, where consumers compare the stored value against a scope rebuilt from configured environment names. Folding only the stored half of that comparison is precisely the asymmetry that produces a silent mismatch. Reversing it is correct, and the storagetest change asserting "Production,Staging" survives verbatim pins it properly.

The two mutation-hygiene commits are also right: moving the Create fold below the required-field validation means a rejected event no longer comes back with its Repository quietly rewritten, and leaving the caller's event untouched in the coalescing probes matches what the interface doc now promises.

CI green after the main merge, which is the part that actually matters for a merge commit in the stack.

Merge-order note, filed identically on #1231: that PR drops environment_scope from plan_comments in postgresIdentityKeyColumns, so the backfill and the store agree. They need to land together or in that order — a deploy running #1218's store logic against #1231's older column list would fold stored rows the store has stopped folding, reintroducing exactly the mismatch this commit removes.

[Kiran01bm]

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

All four findings addressed: the environment-scope fold is removed (the column is compared in Go, never queried), the backfill question is settled by #1231, and both nits are fixed. Severity-ordered; # follows review comment 5490689805's numbering, with its two optional action items as rows 3 and 4. Review comment 5497699951 (sweep) re-confirms findings 1–3 and marks the backfill question resolved; it is covered by the same rows.

#	Concern	Status
1	environment_scope is folded on write but is never a SQL predicate; its two Go consumers compare it against inconsistently folded values (pull_request.go builds expectedScope unfolded), and sort-before-fold makes the fold non-canonical ({Staging, production} → "staging,production" vs "production,staging")	fixed — EnvironmentScope dropped from canonicalizePlanCommentIdentity; the fold helper documents why (comparison-only column, compared in Go against a freshly built scope, no query filters on it). Verified both consumers: plan_comment_minimize.go compares a stored scope against a freshly built one, and no plan_comments predicate filters on the column. The storagetest subtest now asserts a mixed-case scope ("Production,Staging") is stored verbatim, and the PlanCommentStore godoc no longer lists environment scope among the folded fields. #1231's backfill map drops the column in lockstep so the writer and the backfill agree
2	No backfill; pre-canonical plans rows stay invisible to the folded DeleteByPR on PostgreSQL	reply — settled by #1231 (storage canonicalize-identity-keys), which merges last in this family and whose docs, subcommand help, and function doc all require it to run once every writer runs a folding release. That covers the merge-order line the sweep asked for: this PR is safe to land before #1231; the backfill runs before PR-scoped cleanup against pre-canonical PostgreSQL rows is relied on. The rolling-deploy overlap points the safe way (duplicate dispatch / extra processing, never lost work)
3	(nit) WebhookEventStore interface doc mentions only Create's in-place fold, so the deliberate copy-in-place contract of HasCoveringSuccessor/SupersedeIfCovered reads as an oversight	fixed — the godoc now states that the coalescing reads fold into a local copy and leave the caller's event untouched
4	(nit) Create folds before the DeliveryID/Event validation, so a rejected call still mutates the caller's struct	fixed — the fold now runs after validation