V151: supplier-info source/primary columns + CRM citext emails

[timujinne]

Summary (reworked on top of the upstream V149/V150 sourcing layer)

V151 — the follow-up that makes the merged sourcing layer coherent:

1. supplier_source + is_primary on phoenix_kit_cat_item_supplier_info. The merged catalogue layer (catalogue PR Updated settings page with 4 new settings - Site URL, Allow anyone to resister, New user default role, Week starts on #44) reads and writes both fields (create defaults, auto-primary promotion, set_primary/1, Suppliers.resolve/1 routing, the audit_supplier_refs report), but V149 ships the junction without them — on current upstream every junction INSERT crashes on an undefined column. V151 adds: supplier_source VARCHAR(20) NOT NULL DEFAULT 'local' with a CHECK vocabulary (crm_company | crm_contact | local — disambiguates the polymorphic soft supplier_uuid without a trial cascade), and is_primary BOOLEAN NOT NULL DEFAULT FALSE with the partial-unique one-primary-per-item index backing Suppliers.primary_for_item/1.
2. CRM email normalization to citext (crm_contacts.email, crm_companies.email) — prerequisite for CRM v2 backfill match-by-email and the user↔contact bridge; the extension is a core dependency since V01, so ensure_extension!/1 no-ops on existing installs. Moduledoc documents the table-rewrite cost and the verified absence of email indexes.

The V146 scalar primary_supplier_uuid is left untouched per V149's design ("junction alongside the scalar"). Note for a follow-up decision: the merged catalogue schema no longer maps the scalar, so the warehouse 0.2.2 scalar-first resolver head can never match — a companion warehouse PR proposes the junction-primary path as the working default-supplier mechanism.

Verification

mix compile clean; DDL idempotent both directions; prefix-hardened (bare index name on CREATE, escaped_prefix anchors, regclass only in queued DO-blocks); mirrored down restores V138 email shape and marker '150'. A dev host running the catalogue layer with these columns has the full flow live-verified (auto-primary, set-primary idempotence, resolve, warehouse supplier-order generation, receipt unit_value).

Required testing before merge

No scratch DB in our dev environment — please run from scratch: fresh chain 0→151 (public + prefix oracle test/integration/prefix_migration_test.exs), upgrade 150→151 on a DB with junction rows (columns backfill with defaults), down(version: 150) round-trip, idempotent re-runs, mix precommit. Then the catalogue suite against this branch (PHOENIX_KIT_PATH) — the junction context is unusable without these columns.

[timujinne]

Code review (GLM-5.2 @ max thinking, static full-file review + chain context V87/V138/V146/V148)

Code Review — PR #640: V149 migration (catalogue supplier-info junction + CRM email normalization)

Scope: one commit (f7e43a8d), two files — new lib/phoenix_kit/migrations/postgres/v149.ex + @current_version bump (148→149) and moduledoc entry in lib/phoenix_kit/migrations/postgres.ex.

Verified against: v149.ex (read in full), v146.ex, v148.ex, v138.ex (origin of the CRM email columns), helpers.ex, postgres.ex dispatch path, v87.ex (origin of phoenix_kit_cat_items), and the AGENTS.md prefix-safety rules.

What's solid

• Idempotency is complete in both directions. Every DDL is guarded: CREATE TABLE IF NOT EXISTS, CREATE [UNIQUE] INDEX IF NOT EXISTS, ADD/DROP COLUMN IF [NOT] EXISTS, DROP INDEX IF EXISTS, COMMENT ON TABLE (overwrite), and DO $$ blocks that check pg_constraint / information_schema.columns before mutating. Re-running up or down is a no-op.
• Bare index names on every CREATE INDEX (v149.ex:96, 101, 107, 122) — correct per the "index names stay bare on CREATE" rule; the qualified #{p} appears only on DROP INDEX (v149.ex:148), which is the one place Postgres accepts it.
• down/1 re-add of primary_supplier_uuid is exact parity with V146 (v149.ex:231-250): same column type (UUID), same constraint name, same FK target phoenix_kit_cat_suppliers(uuid), same ON DELETE SET NULL, same partial index name + WHERE primary_supplier_uuid IS NOT NULL. The constraint_exists?/1 helper (v149.ex:295-311) mirrors V146's name-based JOIN (pg_class/pg_namespace, n.nspname = $1) — the idiom CLAUDE.md prefers over ::regclass.
• citext revert target is correct. V138 created both phoenix_kit_crm_contacts.email and phoenix_kit_crm_companies.email as VARCHAR(255); down/1 reverts to exactly VARCHAR(255) (v149.ex:264, 281). No USING clause is needed — the varchar↔citext assignment cast is implicit.
• Helpers.ensure_extension!("citext") is placed correctly (v149.ex:162): it runs immediately (repo().query!) before the queued citext ALTERs flush, so the type provably exists when the DO blocks run, and the low-privilege-role path (checks pg_extension before CREATE EXTENSION) is honored. Good.
• Marker handling (v149.ex:198 → '149', :286 → '148') matches the V146/V148 convention.

Findings

[major] v149.ex:13-18, 151-154 — Release-order coupling: confirm V146 was never independently published before this ships.
up drops primary_supplier_uuid (FK + partial index + column) from phoenix_kit_cat_items, a column that backs phoenix_kit_catalogue's schema field (V146 moduledoc: "upstream catalogue commit 2e47cdf"). The moduledoc warns that phoenix_kit_catalogue and phoenix_kit_warehouse must ship in lockstep — which is the right call-out — but this is a hard runtime break: any host that runs this migration without the matching catalogue release gets SELECT crashes on a missing column. PhoenixKit is a library where hosts own deployment cadence, so a column drop gated only by a moduledoc note is fragile. The "deprecated in this wave" wording implies V146/V148/V149 + the module changes are one unpublished batch, in which case the column never reached Hex and dropping it now (before adoption) is the correct, clean move. Action: verify V146 was not already published to Hex. If it was, prefer a deprecation cycle (keep the column nullable/unused one release) over a hard drop. If it wasn't, this is fine — but the lockstep requirement should also be enforced somewhere a deployer can't miss (release notes / mix phoenix_kit.update warning), not only the migration moduledoc.

[minor] v149.ex:170, 187 (up) and 258, 275 (down) — information_schema checks interpolate raw #{prefix} instead of #{escaped_prefix}.
CLAUDE.md's prefix-safe rule and the runner's own checks (postgres.ex:1384, 1492, 1581) use table_schema = '#{escaped_prefix}'; the dispatch path (with_defaults / down, postgres.ex:1698-1702) populates :escaped_prefix and calls validate_prefix! before v149.up/1 runs. This is safe in practice — validate_prefix! restricts the prefix to [a-z_][a-z0-9_]*, so there are no quotes to escape and escaped_prefix == prefix. But it diverges from the documented defense-in-depth convention (and down/1 already binds escaped_prefix at :214 and uses it for constraint_exists?). Recommend switching the four table_schema = '#{prefix}' literals to #{escaped_prefix} (and binding escaped_prefix in up/1) for consistency. No correctness/security impact today.

[minor] v149.ex:175-176, 192-193 — citext ALTER COLUMN … TYPE takes an ACCESS EXCLUSIVE lock and rewrites the whole table.
Converting varchar → citext is a base-type change: Postgres rewrites every row and blocks all reads/writes for the duration. The two usual gotchas are both clear here — I verified there is no index and no unique constraint on email in either CRM table across the entire chain (only idx_crm_contacts_user_uuid, idx_crm_contacts_status, idx_crm_companies_status exist, none on email), so there's no index rebuild and no risk of a latent case-duplicate surfacing as a unique-violation. Worth adding a one-line moduledoc note that this is a rewrite op (the moduledoc currently doesn't mention the cost), so operators of large CRM tables know to run it in a maintenance window. Separately: the citext conversion makes previously-distinct case-variant emails compare equal — harmless only because there's no unique constraint; worth a comment so a future unique index isn't added naively.

[minor] No CHANGELOG entry / no @version bump accompanies V149.
mix.exs is still 1.7.193 (set in the prior commit d3efff7e for PR #637); grep for V149/primary_supplier_uuid/supplier_info/citext in CHANGELOG.md returns no matches. CLAUDE.md's version-management workflow expects a CHANGELOG entry against the bumped @version for a new migration. Confirm V149 is intended to ship under the still-unreleased 1.7.193, or bump to 1.7.194 and add the entry (Added: junction table + crm_company_uuid; Changed: CRM email → citext; Removed: scalar primary_supplier_uuid).

[suggestion] v149.ex:68, 86, 139 — ::regclass in the three DO $$pg_constraint checks diverges from the V146-preferred name-based JOIN.
This is safe: all three checks are queued (execute/1), and each target relation is created earlier in the same queue (the junction table at :40) or in an earlier version (phoenix_kit_cat_items ← V87, confirmed at v87.ex:151). The regclass RAISE documented in CLAUDE.md only bites immediaterepo().query/3 calls (V146's original bug) — queued DO blocks execute in-order at flush, after their CREATE TABLE. It also matches V148's established pattern. Flagging only for consistency with the name-based JOIN idiom that down/1's own helper already uses; no behavior change.

[suggestion] v149.ex:41 — raw #{p}uuid_generate_v7() instead of Helpers.uuid_v7_call/1.
CLAUDE.md recommends ensure_uuid_v7_function/1 + uuid_v7_call/1. The raw interpolation is consistent with V138/V148 and is safe (V01 creates the function on a fresh chain; Postgres.up/1 re-ensures it for upgrade chains ≥ V40). No correctness impact; lowest-value item of the set.

Required testing

The migration was not executed against a database in this review — all findings are from static analysis of the code. The dev environment has no scratch PostgreSQL, and no integration suite was run. Before merge/release, exercise the chain end-to-end on a real Postgres:

1. From-scratch install (the critical path): run the full V01→V149 chain into a fresh DB. This is what test/integration/prefix_migration_test.exs (the project's oracle) exists for — it runs the entire chain into a scratch schema on a DB that also carries a public install, catching both prefix-bug families. Run it. It specifically validates the ::regclass-in-DO-block pattern and the schema-anchored existence checks under a real queued-DDL flush — the one thing static reading can't fully prove. It flips the sandbox to :auto (see its moduledoc).
2. Prefixed install: run the chain with --prefix into a non-public schema (ideally the privilege-sensitive recipe — pre-created schema + low-privilege role, PG15+ non-writable public — which the oracle cannot cover under its superuser connection and must be verified manually per the 2026-07-12 field report). Confirm the junction-table FK/CHECK land in the named schema, the partial-unique index name is bare, and both citext ALTERs target the prefixed tables.
3. Upgrade path: migrate an existing install from V148 → V149 (the common production case) and confirm primary_supplier_uuid (column, FK …_fkey, partial index …_index) is cleanly removed and the two CRM email columns flip to citext (\d phoenix_kit_crm_contacts / \d phoenix_kit_crm_companies).
4. Idempotency: run up twice and down→up — expect no-ops and no errors (the IF NOT EXISTS / DO $$ guards).
5. down round-trip: V149 → V148 → V149, confirming the scalar column/FK/index reappear with the V146 shape and email reverts to VARCHAR(255) then back to citext.
6. Release-coordination smoke test (the [major] item): against a phoenix_kit_catalogue build that still has the primary_supplier field, confirm the documented break actually occurs on SELECT after up — and that the post-drop catalogue/warehouse releases no longer reference the column.

Verdict

APPROVE.

The migration is correct, fully idempotent, and prefix-safe in practice (bare index names, schema-anchored existence checks, correct citext revert target, V146-parity down, sound marker handling). There are no correctness blockers. The one item with real teeth — the [major] release-order coupling from dropping primary_supplier_uuid — is well-documented in the moduledoc and is a pre-release confirmation (verify V146 was not independently published to Hex; enforce the lockstep upgrade somewhere deployers will see it), not a merge-blocking code defect. The remaining items are minor hygiene (CHANGELOG/@version, escaped_prefix consistency, citext rewrite doc note) and two stylistic suggestions. Clear those and run the from-scratch + prefix-oracle tests before release.

[timujinne]

Review follow-ups applied in 5f4724d: the four information_schema anchors now interpolate escaped_prefix (bound in up/1, matching down/1); moduledoc documents the citext ALTERs as full-table-rewrite ops under ACCESS EXCLUSIVE plus the verified absence of email indexes (and the caveat for adding one later). On the review's [major] pre-release check: verified — no published catalogue release carries the schema field (0.10.0 shipped 2026-07-03, the primary_supplier_uuid schema commit landed 2026-07-06), so the V146 column exists on Hex hosts only as an unused column and the drop is clean; the lockstep requirement applies to git/path consumers only and stays called out in the PR body and moduledoc. CHANGELOG/@Version left to the maintainer per project convention. The from-scratch + prefix-oracle testing checklist stands.

[ddon]

We have a conflict here

[timujinne]

Back to draft: upstream shipped its own V149 (PR #638 — junction as a pricing layer alongside the V146 scalar) and further rework is in flight. This PR will be rebased onto the final upstream state — the remaining unique pieces here are the CRM email citext normalization and (pending the final sourcing design) the supplier_source disambiguation column. Will re-open once the upstream layer settles.

[timujinne]

Conflict resolved — the branch was rebuilt from scratch on current main as V151 (the upstream V149 junction + V150 shipped meanwhile). Now a pure additive follow-up: the two columns the merged catalogue sourcing layer already reads/writes (supplier_source + is_primary with the partial-unique index) plus the CRM email citext normalization. The V146 scalar is untouched per the V149 design. Status: MERGEABLE, ready for review.