feat(cli): add storage canonicalize-identity-keys admin subcommand

[Kiran01bm]

Adds a storage canonicalize-identity-keys admin subcommand that one-time folds stored identity strings (repository, database, environment, deployment, lock owner) to canonical lowercase on PostgreSQL storage.

Why

The canonicalization series folds identity strings to canonical lowercase at the write boundaries; rows written by releases without those boundaries keep their original spelling. MySQL's accent- and case-insensitive storage collation forgives that drift; PostgreSQL compares bytes, so legacy-cased rows are invisible to folded lookups — locks that can never be released, checks that duplicate instead of updating, release/cleanup predicates that miss existing rows.

Merge order: this PR merges last in the series. The command must only run once every writer — server and workers — runs a release that folds at the write boundaries; a writer still on an earlier release would keep writing mixed-case rows and can turn the fold into duplicate-key collisions. The docs and the command's own help state this precondition.

What

• CanonicalizePostgresIdentityKeys rewrites only non-canonical rows, table by table in per-table transactions, driven by a per-table column map kept in lockstep with the embedded schema files by a bidirectional parity test (every mapped column exists in the schema; every identity-named schema column is mapped or explicitly excluded with a rationale). Fold collisions fail naming the violated unique index for manual resolution — tables already folded stay folded, and a rerun folds the rest.
• New storage canonicalize-identity-keys CLI subcommand wires it up. The rewrite is one-way — original spellings are not recorded — so it prompts before touching rows (--auto-approve/-y for scripted maintenance windows). A direct --dsn that does not parse as PostgreSQL is refused up front, mirroring the config path's dialect refusal.
• Caller-supplied lock owners now fold at lock acquire and release, so the byte-exact owner match at release holds across the fold; GitHub-origin owners were already canonical by construction from the folded repository. The fold cures owner spellings stored by earlier releases.

Before / after

Before (legacy row, folded lookup):
  locks: repository = "Org/Repo"        (written by an old release)
  DELETE ... WHERE repository = 'org/repo'   ──▶  0 rows: lock stuck forever

After (one-time fold):
  canonicalize-identity-keys  ──▶  UPDATE locks SET repository = 'org/repo'
  DELETE ... WHERE repository = 'org/repo'   ──▶  1 row: lock released

[Copilot]

Pull request overview

Adds an operator-facing storage maintenance command to repair legacy PostgreSQL rows whose “identity key” strings were stored with mixed casing, making them invisible to newer lowercase-folded lookups (unlike MySQL’s case-insensitive collation). This fits the codebase’s operational tooling by extending the schemabot storage admin surface with a safe, idempotent one-time fix for existing storage data.

Changes:

• Added schemabot storage canonicalize-identity-keys CLI subcommand to fold stored identity strings to canonical lowercase on PostgreSQL.
• Implemented api.CanonicalizePostgresIdentityKeys to rewrite only non-canonical rows per table/column map, with explicit unique-collision failure reporting.
• Added unit + integration tests to pin the per-table column map to embedded schema and validate canonicalization behavior (including collision and non-storage DB refusal).

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
pkg/cmd/commands/storage.go	Adds the new admin subcommand and refactors DSN resolution into a shared helper.
pkg/cmd/commands/storage_test.go	Updates DSN-resolution tests to target the new shared resolveStorageDSN helper.
pkg/cmd/commands/storage_integration_test.go	Adds integration coverage for running the new CLI command end-to-end against Postgres storage.
pkg/api/canonicalize_postgres_identity.go	Implements table-by-table lowercase folding with collision detection and safety checks.
pkg/api/canonicalize_postgres_identity_test.go	Adds a parity test to ensure the canonicalization column map matches embedded Postgres schema files.
pkg/api/canonicalize_postgres_identity_integration_test.go	Adds integration tests validating folding behavior, idempotency, collision reporting, and non-storage refusal.
docs/configuration.md	Documents the upgrade-time operational procedure and collision-handling expectations.

---

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

[morgo]

🤖 Approved on Morgan's behalf by his AI agent. This is the backfill I asked for on #1218 — thanks for building it, and the merge-order precondition is the right call.

Now fully green. The design holds up where it counts: rewrites only non-canonical rows so reruns are safe, per-table implicit transactions so a collision in one table leaves earlier folds applied, unique-violation (SQLSTATE 23505) caught and re-raised naming the constraint, PostgreSQL-only with a dialect refusal, and a confirmation prompt because the rewrite is one-way. The "none of the N storage tables exist" guard is a nice touch against being pointed at the wrong database.

The 8-line live half in lock_handlers.go is correct and symmetric — both acquire and release fold, and the comment names the exact bug it prevents. It's inert on MySQL, where ai_ci still matches a folded release predicate against a mixed-case stored owner, so the live production path is untouched.

Two things worth having on the record before someone runs this.

1. This folds in SQL, and the rest of the series folds in Go. #1218 states the convention explicitly — "Go-side folds only; no SQL LOWER()." Here foldPostgresIdentityKeys emits SET col = lower(col) WHERE col <> lower(col). That's two different implementations of "canonical" now, and PostgreSQL's lower() is collation-dependent while Go's strings.ToLower is locale-independent Unicode folding. They agree on ASCII, which is why CanonicalKey's doc says the ASCII assumption is deliberate — but that assumption is now load-bearing in two places with two different functions behind it.

The pathological case is a Turkish-collated database, where lower('I') is 'ı' (dotless) but Go produces 'i': the fold would rewrite a row to a spelling the Go-side lookups can never generate, turning "invisible row" into "permanently invisible row." Vanishingly unlikely for a storage DB, and GitHub restricts repo and owner names to ASCII — but deployment, environment, and database_name are operator-controlled and not similarly constrained. A sentence in the function doc pinning the ASCII assumption (or asserting the DB collation) would close it; I wouldn't hold the PR for it.

2. The ordering creates a transient stuck-lock window, by construction. The live owner fold ships with this PR, but the command is documented to run last, after every writer is upgraded. In between, a lock acquired by an earlier release stores CLI:User@Host verbatim; a release request arriving after the deploy folds the incoming owner to cli:user@host and compares byte-exact, so it won't match — exactly the "acquire a lock it can never release" case the new comment describes, just with the two spellings on opposite sides of the deploy rather than within it.

It's narrow (PostgreSQL only, caller-supplied owners only, only locks spanning the deploy, and only when the release path supplies an owner — it isn't required there), and lock leases should time it out rather than leaving it wedged forever. But it's inherent to the sequencing rather than avoidable, so it belongs in the runbook next to the merge-order note: operators may need to wait out or hand-clear locks held across the upgrade, and the fold cures the rest.

Nit: nothing enforces the "run with the server quiesced" precondition — it's documented on the function and in the help text, but an operator running it under live traffic gets row locks that make FOR UPDATE SKIP LOCKED claims silently skip rows. Given --auto-approve exists for scripted windows, that's a reasonable place to trust the operator; just noting the guard is prose, not code.

[aparajon]

🤖 Adversarial correctness review, requested by Armand and performed by his review agent. Reviewed at head 6fee86b.

Verdict: correct, fail-closed, and safe to land — no blocking findings. I attacked the fold's failure directions, the collision path, the rerun semantics, the owner-fold symmetry, and the wrong-database guard, and the design held everywhere it matters: the WHERE col <> lower(col) predicate means the worst failure mode is an incomplete fold reported per-table, never a wrong rewrite. Two minor findings, one of them a real doc contradiction.

Findings

1. (doc) pkg/storage/canonical.go's comment now contradicts the lock API. The CanonicalKey doc says lock owner strings are "canonical by construction rather than re-folded at this boundary" — but this PR folds req.Owner through CanonicalKey at both acquire and release (pkg/api/lock_handlers.go:71,144), precisely because CLI-origin owners like cli:user@Host are not canonical by construction. The new map comment in canonicalize_postgres_identity.go:23-26 states the new behavior correctly, so the codebase now says both things. A future reader deciding whether a new boundary must fold owners will trust whichever comment they find first.

2. (minor, optional) The fold's canonical form is SQL lower(); the write boundaries' is Go strings.ToLower — they agree only on ASCII. PostgreSQL's lower() is locale-dependent (a C-locale database folds only ASCII; ICU locales disagree with Go on characters like dotted İ), so a non-ASCII legacy identity string can survive the fold still invisible to folded lookups while the summary reports success. The failure direction is safe — a row is skipped, never mangled — and canonical.go already scopes identity strings to ASCII in practice, but the map comment is the natural place for one sentence acknowledging the fold inherits that ASCII assumption.

Action items

1. (Finding 1) Update the CanonicalKey doc comment in pkg/storage/canonical.go so the owner-folding story matches lock_handlers.go — owners now fold at the lock API boundary because caller-supplied owners aren't canonical by construction.
2. (optional) (Finding 2) Add a sentence to the postgresIdentityKeyColumns comment noting the SQL-lower()/Go-ToLower equivalence holds for ASCII identity strings, which is the documented scope.

Verified (tried to break it, couldn't)

Fail-closed behavior and blast radius

• A database with none of the storage tables is refused up front ("does not look like SchemaBot's storage database") — pointing the command at the wrong DSN cannot fold arbitrary tables. Verified against missingPostgresTables source and the integration test.
• A partially-bootstrapped database warns per missing table and folds the rest; the summary counts tables_skipped_missing, so an incomplete fold is visible, not silent.
• A non-PostgreSQL --dsn is refused before any connection (resolveStorageDSN parses via postgresconn.ConnectionDSN), and the error does not echo DSN credentials — pinned by test.

Rewrite semantics

• The UPDATE ... WHERE col <> lower(col) shape can only rewrite non-canonical rows: reruns are provably no-ops (asserted by test), already-canonical rows keep their updated_at byte-identical (asserted), and lease/staleness heuristics keep their meaning.
• Fold collisions (rows differing only by case) fail the affected table's fold naming the violated unique index via typed pgconn.PgError SQLSTATE 23505 handling — never a silent row collapse. Verified on both single-table and multi-index (apply_target_locks) shapes; earlier tables' folds persist per-table and the rerun tolerates that, exactly as documented.
• Non-identity values provably don't fold: SHAs, plan identifiers, and delivery IDs asserted unchanged in the integration test; lease/observer owners excluded from the map with rationale.

Map ↔ schema lockstep

• The bidirectional parity test is real protection: every mapped column must exist in the embedded schema, and every identity-named schema column must be mapped or excluded with a rationale — a new table or renamed column breaks the build, not the maintenance window.

Owner-fold symmetry

• Acquire and release both fold req.Owner, so the byte-exact ownership match holds across the fold; force-release (no owner predicate) is unaffected. The updated handler tests pin both directions with explanatory assertions.

Tests and CI

• Built at head; the PR's unit tests pass with -race and all five new integration tests pass against real PostgreSQL containers locally at 6fee86b.
• CI is fully green after a failed-jobs rerun; the original K8s E2E Tests failure was a runner cancellation mid-suite ("The operation was canceled", no test assertion failed), not a regression from this change.

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 (no blocking findings; the two doc-level action items are trusted to the author before merge). Merge-order condition from the PR body carries to merge time: land this PR last in the canonicalization series, and run the command only after every writer runs a folding release. This stamp was left by Cato, an AI review agent operated by Armand.

[morgo]

🤖 Re-approving on Morgan's behalf at 617ec986 (automated review).

Force-pushed past my earlier approval, so I re-verified the delta.

fix(cli): exclude plan comment environment scope from the identity fold drops environment_scope from the plan_comments entry in postgresIdentityKeyColumns, matching #1218's store-side reversal. I verified the shared premise on #1218: environment_scope never appears in a WHERE clause in plan_comments.go, so folding it gained nothing in SQL and broke the Go-side comparison against a scope rebuilt from configured environment names. Excluding it from the backfill is the correct other half — a backfill that folded rows the store no longer folds would recreate the mismatch on existing data.

The added note that lower() in SQL and storage.CanonicalKey in Go agree because identity strings are ASCII is worth having written down; it's the assumption that makes a SQL-side backfill and a Go-side ingress fold interchangeable, and it was previously implicit.

CI green.

Two notes, neither blocking.

Merge-order collision with #1217. Both PRs rewrite the same CanonicalKey doc block in pkg/storage/canonical.go with different accounts of lock owners — here "the lock API folds a caller-supplied owner (cli:user@host) with this function at acquire and release", on #1217 "deliberately not folded … compared byte-wise on both sides." Different boundaries, both true, but they conflict textually and a careless resolution leaves the doctrine inconsistent. Whichever lands second should state both.

Deploy ordering for the lock-owner fold. Folding caller-supplied owners at acquire and release is self-consistent going forward, but a lock acquired before this ships stores an unfolded owner (CLI:User@Host), and a release afterwards folds the supplied value and no longer matches — the "acquire a lock it can never release" hazard, inverted across the deploy boundary. locks.owner is in postgresIdentityKeyColumns, so the backfill closes it; worth calling out in the rollout notes that it should run before or promptly after, rather than leaving it to lock expiry.

[Kiran01bm]

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

Both doc findings fixed; a third row records a cross-PR consistency fix made in the same change so the backfill map matches what #1218's writer now stores.

#	Concern	Status
1	(doc) pkg/storage/canonical.go says lock owners are "canonical by construction rather than re-folded", contradicting lock_handlers.go, which now folds req.Owner at acquire and release	fixed — the paragraph now distinguishes GitHub-origin owners (canonical by construction, built from the folded repository) from caller-supplied owners (CLI cli:user@Host and similar), which the lock API folds at acquire and release; it matches the postgresIdentityKeyColumns comment
2	(optional) The fold's canonical form is SQL lower(), the write boundaries' is Go strings.ToLower; they agree only on ASCII, so a non-ASCII legacy string could survive still-invisible while the summary reports success	fixed — the map comment now states the lower()/ToLower equivalence holds for the ASCII identity strings CanonicalKey documents as its scope, and that the failure direction is a skipped row, never a mangled one
—	Cross-PR consistency: the backfill map still folded plan_comments.environment_scope, which #1218's second round now leaves unfolded at the writer	fixed — environment_scope removed from the plan_comments entry; TestPostgresIdentityKeyColumns_CoverEmbeddedSchema's exclusion list gains it with the rationale (comparison-only column, never a SQL predicate, compared in Go against a freshly built scope). Folding it here while the writer preserves case would have made the backfill and the writer disagree on one column