fix(storage): canonicalize apply and task identity keys

[Kiran01bm]

Fold apply and task identity keys Go-side at the store boundary, keeping the checks↔applies correlated guards byte-consistent.

Why

The stored-check guards run correlated subqueries joining checks to applies on repository/database. With the checks side folded (sibling PR) but applies unfolded, those guards stop matching on PostgreSQL — the newer-apply guard fails open. Folding the applies/tasks side keeps every cross-table predicate byte-consistent on both dialects.

What

• applies.go: Create, Update, GetRecent/CountRecentByState filters, GetByDatabase, GetByPR, DeleteByPR, ExistsForDatabaseHead fold identity fields (repository, database, type, environment).
• tasks.go: Create/insertTask, shard-progress upserts, GetByDatabase, GetByPR, List, FindTableOwners fold the same fields.
• apply_operations.go unchanged: operations carry no program identity fields (deployment names, operation keys, targets, lease owners are out of scope).
• Go-side folds only; no SQL LOWER().
• Cross-dialect parity subtests, including assertions that deployment and table-name casing stays untouched.

Before / after

Before (PostgreSQL):
  applies row: repository = "SomeOrg/Repo"
  checks guard: newer.repository = checks.repository ("someorg/repo")
        ──▶ no match → newer-apply guard fails open

After (both dialects):
  applies row: repository = "someorg/repo"
  checks guard join matches byte-wise
        ──▶ guard enforces as designed

[Copilot]

Pull request overview

This PR makes storage identity comparisons cross-dialect consistent by canonicalizing (lowercasing) key identity fields for applies and tasks at the SQL store boundary. This prevents PostgreSQL’s byte-wise comparisons from diverging from MySQL’s case-insensitive behavior, keeping check↔apply correlation predicates byte-consistent.

Changes:

• Add storage.CanonicalKey() and apply it to repository/database/database type/environment at write boundaries (applies + tasks) and in key read filters.
• Canonicalize identity-filter inputs for Applies.GetByDatabase/GetByPR/GetRecent/CountRecentByState/ExistsForDatabaseHead/DeleteByPR and Tasks.GetByDatabase/GetByPR/List/FindTableOwners.
• Add storagetest parity subtests asserting identity keys are treated case-insensitively while deployment/table casing is preserved.

Reviewed changes

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

Show a summary per file

File	Description
pkg/storage/storagetest/tasks.go	Adds a parity subtest asserting task identity keys are case-insensitive across store methods.
pkg/storage/storagetest/applies.go	Adds a parity subtest asserting apply identity keys are case-insensitive across store methods.
pkg/storage/internal/sqlstore/tasks.go	Canonicalizes task identity keys on insert/upsert and canonicalizes identity-based query filters.
pkg/storage/internal/sqlstore/applies.go	Canonicalizes apply identity keys on create paths and canonicalizes identity-based query filters.
pkg/storage/canonical.go	Introduces CanonicalKey() helper for consistent identity folding.

---

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

[chatgpt-codex-connector]

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

[morgo]

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

Pure addition — 0 deletions across all six files, 37/37 green, and the folding is inert on MySQL where utf8mb4_0900_ai_ci already matched case-insensitively. Consistent with #1213/#1215/#1216.

Worth stating plainly: this closes a window main is in right now. #1216 merged at 02:53 and folded the checks side; this PR folds the applies/tasks side. If the premise in your description holds — correlated subqueries joining checks to applies on repository/database, with one side folded and the other not — then the newer-apply guard is fail-open on PostgreSQL on main today, until this lands. That's an argument for merging it promptly rather than a reason to hold it, and it's the kind of intermediate state worth calling out in a split series so nobody parks the second half.

apply_operations.go staying out of scope is right — operations carry no program identity, and #1224 is separately adding external_id there, so keeping the two changes disjoint avoids a needless conflict.

One nit, non-blocking: canonicalizeApplyIdentity(apply *storage.Apply) mutates the caller's struct in place, and Create/createWithRows/Update all call it before doing anything else. Two small consequences worth knowing about:

• The caller's in-memory *storage.Apply is silently rewritten as a side effect of a store call. That's mostly desirable — their view now matches what's persisted — but it isn't documented on the function or at the call sites, and a caller that compares apply.Repository against something case-sensitive afterwards would see different behavior than before.
• The mutation happens even when the subsequent write fails, so a failed Create still leaves the caller's struct folded.

Neither is a bug in any current call path; a one-line doc comment saying the fold is applied to the passed struct would make it intentional rather than incidental.

On the read-path folding (GetRecent, GetByDatabase, GetByPR, List, FindTableOwners): same pre-existing-row question I raised on #1215 about PostgreSQL rows written before folding becoming unfindable. Not re-litigating it here — #1215 merged, so I'll take it as settled — but flagging that this PR widens the same surface, so if the answer there was "PG storage hasn't carried real traffic yet," that assumption is now load-bearing across four PRs.

[aparajon]

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

Verdict: the folding is right and the call sites are consistent — but the merge-order condition in your PR body has already been crossed, so this is now the fix for a live half-folded state on main rather than a precaution. #1216 merged ahead of it; the consequence is Finding 1 and it argues for landing this promptly. I went looking hard for a fold that would move the cross-pod apply lock and could not find one — that story is genuinely closed, and by a merged sibling rather than by luck.

Findings

1. The ordering your PR body warns about has already happened, and the resulting guards fail open. #1216 merged first, so on main today canonicalizeCheckIdentity (checks.go:37-40) folds repository into every check row while applyStore.Create still inserts apply.Repository verbatim. checks.go carries four guards that correlate the two tables on that column — newer.repository = checks.repository at lines 392, 469, and 541, and task_apply.repository = checks.repository at 576. On PostgreSQL's byte comparison, any repository whose GitHub full_name carries uppercase now stores one spelling in checks and another in applies, so the correlation matches nothing. Three of the four are NOT EXISTS newer-apply guards, which means they do not merely stop working — they become trivially true and let the write through, so a check can be updated out from under an apply that started after it. That is the "started applies remain authoritative" invariant failing in the unsafe direction, and it is only invisible on MySQL because utf8mb4_0900_ai_ci compares case-insensitively. The completed-forward-task predicate is the mixed case: fail-closed in its EXISTS form, fail-open in its NOT EXISTS one. This PR closes it, which is the argument for landing it soon rather than a reason to hold it.

2. No backfill, and Update structurally cannot repair a row. The in-place fold rewrites the struct, but Update's statement is SET state, error_message, external_id, started_at, completed_at, updated_at WHERE id = ? — identity columns are never written, which the new interface doc says out loud. So a row persisted before this lands keeps its original spelling permanently, and on PostgreSQL (byte comparison) it is invisible to every folded lookup; MySQL's utf8mb4_0900_ai_ci hides it entirely, which is why this only bites one dialect. The exposure is narrower than it first looks: #1214 is merged, so database, environment, and deployment names are lowercase by construction from config, and database type is a validated enum. That leaves repository, which arrives from GitHub and is folded at ingress only by #1213 — still open. Concretely, on PostgreSQL, GetByPR, DeleteByPR, ExistsForDatabaseHead, and List will all miss applies and tasks stored before ingress folding lands, and DeleteByPR is the quiet one: it deletes nothing and reports no error. With #1218 open too, this is the fourth PR in the family carrying the same gap, so the fix probably isn't four separate backfills — it's one story for the family, either a one-shot fold of the stored identity columns or a documented statement that non-canonical rows are abandoned and why that is acceptable.

3. The CanonicalKey doc's lock-owner paragraph is still one PR ahead of the code. It reasons that lock owner strings need no folding here because "the repository they are derived from is folded at ingress" — which is #1213's job, and #1213 has not merged. Copilot raised the ingress claim and it was addressed in 34f70df; that commit moved the assertion out of the opening paragraph but the lock-owner rationale still rests on it. Either land #1213 first or phrase it as the invariant that ingress folding establishes, so the comment is not briefly wrong on main.

Action items

1. (Finding 1) Land this promptly — main is currently half-folded across checks and applies, and three PostgreSQL guards fail open until it lands. Worth saying so in the verdict so the state is visible to whoever merges.
2. (Finding 2) Decide the family's backfill story once — a one-shot fold of stored identity columns, or an explicit note that pre-canonical rows are abandoned and why that is safe — and reference it from this PR, fix(webhook): canonicalize repository identity at ingress #1213, and fix(storage): canonicalize remaining identity keys #1218 rather than solving it four times.
3. (Finding 3) Land fix(webhook): canonicalize repository identity at ingress #1213 ahead of this, or reword the lock-owner paragraph so it describes the invariant rather than asserting current behavior.
4. (optional) Update folds the caller's struct even when the write is then rejected by the lease guard; harmless today, but worth a word in the interface doc since it already documents the in-place rewrite.

Verified (tried to break, couldn't)

The failure I most expected is not there: applyTargetLockName hashes database + dbType + environment, so folding any of the three would change the advisory lock name and let an old pod and a new pod hold different locks for the same physical target through a rolling deploy — two concurrent applies against one database. That cannot happen, because #1214 already rejects non-lowercase database, environment, and deployment identifiers at config load and database type is a validated enum, so the fold is a no-op on exactly that path and the lock name cannot move. I also enumerated every query in applies.go and tasks.go that filters on an identity column and checked the two the diff leaves unfolded — checkNoActiveApplyForTargets and verifyExpectedLockIntent — and both are canonical by construction: every call site passes either a struct the new fold has already rewritten or values re-read from the stored row, so neither is a missed read path. Where identity mismatch can still surface, it points the safe way: classifyDestructiveChange compares owner.Repository == repo exactly, so a stale spelling makes it over-annotate a destructive change rather than miss an owner. Deployment names and table names are deliberately left alone and the new subtests assert that rather than assuming it; UpsertShardProgress's conflict target is not identity, so folding cannot split a row into two; RecentAppliesFilter's only other string is the deliberately-unfolded Deployment, and TaskFilter's only identity string is folded. No test functions were deleted and no assertions weakened. Build and the pkg/storage suites pass locally at head, and CI is 37/37 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. Not a blocking approval with a caveat attached — the review's first finding is that main is already carrying the half-folded state this PR fixes, so the ordering note there is a reason to land it sooner, not a reason to hold it. The rest are yours to pick up as follow-ups.

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

[aparajon]

🤖 Adversarial correctness review (sweep pickup), performed by Cato at head c3023433.

Verdict: correct, additive-only, and safe to land — land it promptly, because main is currently half-folded and three PostgreSQL guards fail open until this merges. A prior adversarial review at this head (here) already covers the merge-order consequence of #1216, the missing backfill story, and the CanonicalKey doc's dependence on unmerged #1213 — I verified those independently and they hold; I won't restate them. One new finding below.

Findings

1. (doc) The TaskStore godoc promises an in-place rewrite that taskStore.Update does not perform. storage.go:843-845 says "Methods accepting *Task canonicalize repository, database, database type, and environment in place before persisting; Update rewrites these fields on the struct…" — but taskStore.Update (tasks.go:117) never calls canonicalizeTaskIdentity; only Create/insertTask (tasks.go:66) and UpsertShardProgress (tasks.go:192) do. The ApplyStore doc is accurate (applyStore.Update folds at applies.go:989), but the TaskStore copy of the same sentence is not. Behavior is safe — task Update's statement writes only state/progress columns and filters on id plus lease predicates — but a caller relying on the documented rewrite would keep a stale-cased struct after Update and could feed it into a byte-compared path later.

Action items

1. (Finding 1) Either fold in taskStore.Update for symmetry with applyStore.Update, or correct the TaskStore godoc to say Update does not touch identity fields. Doc fix is sufficient.

Verified (tried to break, couldn't)

Lock-name stability (the scariest failure):

• applyTargetLockName hashes database + dbType + environment (applies.go:238); a fold that changed any of the three mid-fleet would let old and new pods hold different advisory locks for the same target. Confirmed it cannot move: feat(config): require lowercase identity identifiers #1214 (merged) rejects non-lowercase database/environment/deployment at config load and database type is a validated enum, so the fold is a no-op on that path.
• applyTargetForUpdate (applies.go:504) reloads identity from the stored row when the struct is incomplete; those columns are canonical by construction for the lock-relevant fields, so the reload cannot produce a divergent lock name either.

Fold coverage and the paths left unfolded:

• Enumerated every identity-filtering query in applies.go and tasks.go: write paths fold via canonicalizeApplyIdentity/canonicalizeTaskIdentity (Create, createWithRows, apply Update, insertTask, UpsertShardProgress); read/delete paths fold arguments (GetByDatabase, GetByPR, ExistsForDatabaseHead, DeleteByPR, recentAppliesWhere, FindTableOwners, List).
• The two left unfolded — verifyExpectedLockIntent (applies.go:774) and checkNoActiveApplyForTargets — receive only already-folded struct fields (both run after the Create-path fold) or row-derived values, so neither is a missed path.
• Deployment and table names deliberately unfolded, and the new parity subtests assert that rather than assume it (storagetest/applies.go, storagetest/tasks.go — Primary-Region and CaseSensitiveTable casing preserved).

Correlated guards this PR exists for:

• The four checks↔applies correlations (checks.go:376, 452, 523, 558) all join on repository; with fix(storage): canonicalize lock and check identity keys #1216's check-side fold merged, this PR's apply-side fold is exactly what restores byte-consistency on PostgreSQL.

Build, tests, CI:

• go build ./... and go test ./pkg/storage/... ./pkg/webhook/... green locally at head c3023433, including both new cross-dialect IdentityKeys_AreCaseInsensitive subtests.
• CI green across the matrix; diff is purely additive (178 insertions, 0 deletions), no test weakened or removed.

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 one new finding is a doc-level fix — flagging it, not gating on it. Land this promptly: main is half-folded until it merges.

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

[morgo]

🤖 Re-approving on Morgan's behalf at 99834b73 (automated review).

Force-pushed past my earlier approval, so I re-verified rather than assuming a clean rebase.

The substantive delta is fix(storage): keep value builders free of identity mutation, which drops canonicalizeTaskIdentity(task) from shardTaskInsertValues. That's the right hygiene call — a function that builds placeholders and args shouldn't mutate its argument — but it's only safe if the fold still runs on every path that reaches it, so I traced it rather than trusting the commit message.

It does. shardTaskInsertValues has exactly two callers, insertShardTaskGuarded (:253) and insertShardTaskGuardedByApply (:255), and both are inside UpsertShardProgress, which folds at its entry (:192). The other insert path, insertTask, folds at :66. No path reaches the value builder unfolded, so shard task rows still get canonical identity keys on both dialects.

The added require.Equal assertions in storagetest/applies.go and storagetest/tasks.go are a good addition — they pin the in-place folding as contract rather than leaving it implied by the later lookup assertions, which would still pass if the fold moved to the SQL layer.

CI green. One cross-PR note below, filed identically on #1231.

Merge-order collision: this PR and #1231 both rewrite the same CanonicalKey doc block in pkg/storage/canonical.go, with different accounts of lock owners. Here they're "deliberately not folded … compared byte-wise on both sides of every ownership check"; on #1231 the same block says "the lock API folds a caller-supplied owner (cli:user@host) with this function at acquire and release." Those describe different boundaries and can both be true, but they'll conflict textually, and a careless resolution leaves the doctrine reading inconsistently. Whichever lands second should reconcile to text that states both: the store boundary doesn't fold owners, the lock API does for caller-supplied ones.

[Kiran01bm]

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

The one new finding (a TaskStore godoc that overstated what Update does) is fixed as a doc correction; the re-verified first-round items stand as dispositioned in the earlier response.

#	Concern	Status
1	(doc) TaskStore godoc promises Update rewrites identity fields on the struct, but taskStore.Update never calls canonicalizeTaskIdentity — only Create/insertTask and UpsertShardProgress do	fixed — godoc corrected: Create and UpsertShardProgress canonicalize in place; Update writes only state/progress columns and leaves identity fields untouched. Doc-only, as the review allowed; folding in Update would add a write to a statement that has no identity predicate