Skip to content

Squash the migration chain at floor V135, add verify-and-repair, and repair the V56/V57 flush-order fallout (V164) - #689

Merged
ddon merged 40 commits into
BeamLabEU:mainfrom
timujinne:squash-migrations
Aug 9, 2026
Merged

Squash the migration chain at floor V135, add verify-and-repair, and repair the V56/V57 flush-order fallout (V164)#689
ddon merged 40 commits into
BeamLabEU:mainfrom
timujinne:squash-migrations

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

Squash the migration chain at floor V135, ship verify-and-repair

Branch: squash-migrations. This document is the PR description — written for
the maintainer, who has not followed the multi-week working history behind it.
Full design rationale lives in dev_docs/plans/2026-07-14-squash-migrations-spec.md
(spec, REVIEWED DRAFT r2) and its companion
dev_docs/plans/2026-07-14-squash-inventory.md (per-version classification of
every migration folded into the baseline, plus addenda for everything added
since). This description summarizes; it does not replace either.

What changes, and why

PhoenixKit's migration chain had grown to 164 versioned modules
(lib/phoenix_kit/migrations/postgres/v01.ex .. v164.ex, ~28,000 lines) at a
rate of roughly 16 new versions a month. Nothing in that chain was wrong, but
almost none of it does anything on a fresh install except reproduce
intermediate shapes a later version immediately changes again — V01..V134
alone account for ~23,000 of those lines and every one of them has already run,
unmodified, on every install this project has committed to supporting.

This PR:

  1. Replaces V01..V134 with one generated baseline, V135. It applies
    the cumulative schema those 134 versions produce, directly, in one step —
    no intermediate drops, renames, or backfills replayed. It was generated by
    tool (dev_docs/squash/generate_baseline.exs), not hand-written, from a
    version-by-version catalog introspection of a real migrated database, and
    is regenerable the same way after any future rebase.
  2. Refuses to migrate a database below the floor, rather than guessing. A
    database at 0 < migrated_version < 135 gets a PhoenixKit.Migrations.BelowFloorError
    naming the bridge release, from up/1, down/1, and mix phoenix_kit.update's
    generation step — not a silent attempt to replay a chain that no longer
    exists. A fresh install (migrated_version == 0) instead clamps straight to
    the baseline, so an unpinned installer or a below-floor-pinned consumer
    wrapper both keep working unmodified (spec §5.2 D13, §5.3).
  3. Ships a mix phoenix_kit.repair verify-and-repair capability, built
    from the same generated manifest the baseline uses
    (lib/phoenix_kit/migrations/expected_schema.ex) — additive-only,
    never re-executes a delta module, never touches data, reports (never
    auto-fixes) anything divergent. This exists independently of the squash and
    is useful against the full pre-squash chain too; it landed first, as
    preparatory work, and is what backs mix phoenix_kit.doctor's new
    verify-read-only pass.
  4. Repairs a real, previously undetected data-shape defect as part of the
    same release — see "The V164/V163 repair" below. This is the one item in
    this PR that changes behavior on an already-migrated database, not just the
    shape of the migration files.

Everything else — @current_version, the version-comment tracking mechanism,
the delta modules above the floor (V136..V164, 29 files, untouched in
content) — is unchanged.

The floor: V135

Floor = 135, picked per the spec's D1/D2 rule (minimum confirmed
migrated_version across every install this maintainer has committed to
seamless-upgrade support — one install, the local host app, confirmed live at
V160 on 2026-08-04, well above 135; 135 was chosen for headroom over the
minimum, not as the minimum itself — see spec §4 for the full floor-candidate
comparison). Below the floor there is nothing to bridge additively — several
versions in that range perform non-additive transforms (V58's timestamp→
timestamptz retype, V74's bigint-id-to-uuid-PK promotion, V114's settings-key
identity rewrite) that only the real, unfolded 1.7.x chain can produce, which
is exactly why the guard raises instead of attempting a partial repair.

The bridge

The last 1.7.x release remains on Hex permanently (packages are
immutable) as the stopover for anything below the floor. On this branch that
release is {:phoenix_kit, "~> 1.7.235"} (mix.exs@version, unmodified by
this PR — version bumps are maintainer-owned, see "Open decisions" below).
Path for a below-floor consumer: pin the bridge, run mix phoenix_kit.update,
confirm the version comment reaches V135 or above on every environment
(dev/staging/prod each have their own database), then move the pin to ~> 2.0.
Full walkthrough: dev_docs/guides/2026-08-07-upgrading-to-2.0-guide.md.

The V164/V163 repair

Two migrations at the top of the chain repair schema defects that predate this
PR and are unrelated to the squash mechanism itself — one is this branch's own
work, the other is yours, merged in while this branch was in flight, and the
two are now load-bearing in a specific order.

V163 (yours, PR #688, "UUID primary-key integrity") is a catalog-driven
repair for any phoenix_kit_* table whose uuid column is the wrong type,
nullable, or not the primary key — a state V40/V56/V74 each assumed
impossible and a production install reached anyway (the reported case:
phoenix_kit_email_events.uuid as varchar(255), nullable, no PK, invisible
to V40's existence-only guard and to V56's later-added type conversion).

V164 (this branch's) repairs the fallout of a genuine ordering defect in
V56/V57: both called a shared helper's up/1 (queues ADD COLUMN for
~80 *_uuid FK columns) immediately followed by that helper's own
add_constraints/1, whose guards run immediateinformation_schema queries
— with no flush() between the two. On an incremental, per-version chain run
this is invisible (Ecto flushes between migration modules regardless), but
on a single-shot run — which is every fresh install mix phoenix_kit.install
produces, since it emits one unpinned wrapper for the whole chain — the guards
ran against database state that had not yet seen the columns just queued
moments earlier, and failed closed: ~46 *_uuid columns were left nullable
instead of NOT NULL, and ~67 of the 70 declared foreign keys were never
created at all.
phoenix_kit_comments.fk_comments_user_uuid specifically was
never created; V72, finding it missing later, guessed ON DELETE CASCADE
instead of V56/V57's own already-declared SET NULL. V164 re-imposes
NOT NULL (only where the column currently has zero NULL rows — never
backfills live data with an invented value; otherwise warns with table, column
and row count and leaves it), creates the missing FKs NOT VALID + VALIDATE
(a failed validation is left NOT VALID with an orphan-count diagnostic, never
retried automatically, never a crash), and corrects fk_comments_user_uuid
from CASCADE to SET NULL. It also folds in two unrelated, unplanned-for-a-
second-migration fixes for objects that diverged between public and
named-schema installs because their DDL was not prefix-qualified
(idx_publishing_posts_group_slug's missing partial predicate from V68, and
the ..._subscription_plans_slug_uidx..._types_slug_uidx rename from
V65) — ships as one migration, not two, by explicit decision (see the
inventory's "Post-V161 additions" for the full reasoning). All three repairs
are additive/corrective and no-ops on a healthy install.

The order is load-bearing, not incidental.V163 runs immediately before
V164 in this merged chain, and it has to: a foreign key cannot reference a
column with no unique or primary key, so V164's FK repair depends on V163
having already promoted the affected uuid columns to primary keys. Both
apply in the same mix phoenix_kit.update run for a real upgrade; nobody
needs to sequence them by hand, but anyone re-deriving this chain (a fork, a
cherry-pick, a re-squash) needs to preserve that order.

Full operational detail for anyone running this against production — lock
behavior, expected orphan counts measured against a real pre-production
database, what a degraded (some constraints left NOT VALID) outcome looks
like, how to re-run after a partial failure — is in the upgrade guide, not
duplicated here.

How this was verified

dev_docs/squash/verify.exs implements the spec's §8.2 scenario matrix (S1
through S22); what each scenario actually proves, and what is deliberately
left unproven, is dev_docs/squash/COVERAGE.md — read that before trusting a
summary number. As of the full --mode b run on 2026-08-08: 21 PASS, 0
FAIL
, three named SKIPs (s4_seed — needs the pre-squash checkout by
construction; s16 — Oban-delegation assertion body still pending; s18
needs a manually triggered concurrent migration, not automatable). Concretely,
among what that run covers:

  • Fresh-install equivalence (S1/S2): a normalized pg_dump --schema-only
    diff and seed-row dump between the new (squashed) chain and the old
    (pre-squash) chain, empty modulo a committed, enumerated whitelist of known
    bimodal drift — any unlisted diff fails the scenario.
  • Below-floor guard (S4): up, down, and ensure_current/2 on a
    persistent below-floor install each raise the specific BelowFloorError
    struct with the bridge message, not just "some error."
  • Consumer wrapper replay (S5): replays the host application's real
    54-file wrapper set (1 unpinned installer + 48 pinned update wrappers + 5
    interleaved consumer-authored migrations that ALTER phoenix_kit tables),
    including a rollback→migrate round trip and the specific, documented failure
    mode an unguarded consumer migration produces against the new floor shape.
  • The V164 repair itself (S21): damages a healthy install the way the
    flush defect did — drops every declared FK, drops every re-imposed
    NOT NULL, flips the comments FK back to CASCADE — runs the chain, and
    asserts full restoration; re-runs it and requires a byte-identical no-op;
    and, on a separate schema, leaves an orphan row in place and requires the
    constraint to land NOT VALID with the row untouched.
  • Per-version composition (S22): applies each delta V136..V164 in its
    own separate migrator invocation and requires the result to equal one
    single-shot run — the property the historical V56/V57 defect violated.
  • A second install-path axis (Mode A, public-schema installs, as opposed
    to the named-schema installs every other scenario uses by default): this is
    how the two prefix-unsafe objects V164 normalizes were actually found — a
    public install and a named-schema install diverged on two objects that
    every purely named-schema scenario is structurally blind to. COVERAGE.md's
    "Hazard: install-PATH bimodality" section has the mechanism.

What is not yet proven, stated as plainly as what is. The "21 PASS" run
above predates this branch's two most recent commits — the final renumber of
this branch's own repair delta to V164, and the merge that brought your
V163 in. The generated manifest
(lib/phoenix_kit/migrations/expected_schema.ex) was not regenerated for the
V163 merge (confirmed: git show <merge-commit> --stat -- lib/phoenix_kit/migrations/expected_schema.ex
is empty) and has no knowledge of V163's objects yet. Regenerating the
manifest and re-running the full matrix against current HEAD is an open item
before this PR should be considered fully verified — flagged, not fixed, by
this pass (see "Open decisions" below).

Deliberately never proven here, and why: a second PostgreSQL major (needs
an operator-provided container with a different major; until then a finding
that rests only on expression-rendering differences between majors is reported
informational, not drift); the reverse-direction concurrency race (a migration
starting mid-repair — the advisory lock only prevents the opposite order; this
one is detected via a comment re-read, not prevented, and needs a manually
triggered two-process trigger to exercise, spec §6.1).

Test-surface changes

v106_test.exs and v114_test.exs deleted with nothing ported (both pinned
mechanism for migrations that no longer exist as distinct steps; the schema
facts they protected are already correct-by-omission in the generated
manifest). v107_test.exs/v112_test.exs/v113_test.exs/v125_test.exs had
their load-bearing final-shape assertions (column types, index predicates, FK
delete actions, CHECK enforcement) ported verbatim into the new
v135_baseline_schema_test.exs; their upgrade-only backfill-correctness
assertions were dropped as unreachable below the floor. v145_test.exs
survives untouched. Full accounting: inventory's "What the squash changed"
section.

Upgrading

Full walkthrough, including lock behavior and maintenance-window guidance for
the V163/V164 repair pair: dev_docs/guides/2026-08-07-upgrading-to-2.0-guide.md.
In short: land on the bridge (~> 1.7.235) first, confirm the version comment
reaches V135 or above on every environment, then move the pin to ~> 2.0
and run mix phoenix_kit.update as usual — the repair applies automatically
as part of that run, not as a separate step.

Open decisions — yours, not this branch's

This branch does not touch CHANGELOG.md or mix.exs's @version
deliberately — both are maintainer-owned by this project's own convention.
Specifically outstanding:

  1. The 2.0.0 version bump itself.mix.exs on this branch still reads
    1.7.235. Breaking upgrade contract (below-floor installs are refused, not
    migrated) argues for a MAJOR bump per the spec's D8; the actual number and
    the CHANGELOG entry are yours to write.
  2. Whether this lands as one PR or two. The spec's original rollout plan
    (§7.2) called for two independently-mergeable PRs — a "pre-squash" one
    (repair engine, manifest generator, gen.migration/release_check fixes,
    zero contention with v*.ex) landing first, then a separate "atomic
    squash" PR. In practice both stages were built as sequential commits on
    this one branch, and nothing has merged to upstream/main from it yet —
    this PR is a single PR carrying both. Splitting it before review, if you'd
    rather review the repair engine independently of the deletions, is your
    call; it can be done by cherry-picking the pre-squash-labeled commits onto
    a separate branch.
  3. Manifest regeneration + a fresh full verify run against current HEAD
    (see "What is not yet proven" above) — the V163 merge needs the manifest
    regenerated before the "21 PASS" result can be re-confirmed as still
    holding.
  4. The ~14-package module-ecosystem pin-widening wave (spec §7.4). Every
    phoenix_kit_* module checked at /www still pins a bare ~> 1.7.x with
    nothing that resolves against 2.0 (re-verified 2026-08-08 — e.g.
    catalogue/locations~> 1.7.189, warehouse/entities~> 1.7.214,
    ecommerce/manufacturing/projects~> 1.7.231). {:phoenix_kit, "~> 2.0"}
    plus any of these modules is a hard resolver conflict until each ships a
    widened pin (a patch release per module, since none call migration
    internals directly). Sequencing that coordinated wave against the 2.0.0
    publish is a maintainer-level decision this branch cannot make.
  5. Two small stale-text bugs left by this branch's own renumbering,
    confirmed live 2026-08-08 and not fixed by this docs pass
    (docs-only —
    no lib//test/ edits in this PR): v164.ex's moduledoc still names
    test/phoenix_kit/migrations/v163_relaxed_columns_test.exs and
    V163RelaxedColumnsTest (the real names are v164_relaxed_columns_test.exs
    / V164RelaxedColumnsTest), and — more visibly — the IO.warn a
    production run prints when a constraint is left NOT VALID says "the
    version comment now reads 163", which is simply wrong (up/1 self-stamps
    164). test/phoenix_kit/migrations/v164_relaxed_columns_test.exs carries
    the matching artifact, @exempt_version 163. None of this is a
    schema-correctness bug — only prose and one log message — but it should be
    fixed before merge, in a normal code commit, not this documentation pass.

A correction we owe you

Your V163's own moduledoc entry did not survive cleanly into this branch's
postgres.ex: after the merge, its heading landed positioned above V162's
body, and V162's own heading was lost — mechanical fallout of two
moduledoc-collapsing edits (this branch's V01..V134 collapse, and your
ordinary per-release entry) landing on the same lines from different
directions. This branch's merge resolution restored both headings, in the
correct order and with V163's content reconstructed from v163.ex's own
moduledoc (postgres.ex's V164/V163/V162/V161 block, as shipped in
this PR). This is not specific to anything this branch changed — your own
main, independent of this PR, likely has the same defect from whatever
follow-on release collapsed the moduledoc next; worth checking there too.

Snapshot of the migration-consolidation investigation, paused pending a clean
verification database (operator to provide CREATEDB or a scratch DB).
- dev_docs/plans/2026-06-15-squash-migrations-research.md: parallel-agent
research findings (version-tracking mechanism, Oban baseline pattern, deployed
versions, fork divergence, squash surface).
- dev_docs/plans/2026-06-15-squash-migrations-plan.md: implementation plan
(floor=110, 2.0.0, baseline-from-pg_dump, prefix-fix workstream, verification).
- dev_docs/squash/: maintainer-only verify-harness + baseline-generator scaffold
(excluded from the hex package). generate_baseline.exs is a DRAFT with known
output-template interpolation issues, to be finalized against the schema-diff
oracle once a clean DB exists.
No verification has been run yet. The prefix-safety fix is the prior commit.
Spec (V150/1.7.196 era): baseline V{floor} over a tool-generated since-tagged
ExpectedSchema manifest, Oban-style skip + below-floor guard + fresh-DB clamp,
additive-only verify-and-repair engine (mix phoenix_kit.repair), 2.0.0 + bridge
release, S1-S17 verification matrix. Inventory: per-version classification of
all 150 migrations (seeds / backfills / drops / hazards) from 4 parallel
readers. Supersedes the June 2026 plan.
Folds in ~50 findings from 4 internal adversarial reviewers (claims/design/
completeness/ops) + GLM-5.2, Kimi K2.7, Mistral Medium. Key changes: line refs
re-anchored to 1.7.196; manifest gains revisions/presence/data-invariants and
spans the full chain (Oban delegated, never manifested); repair reads the raw
comment, gains R6 (comment > current), --heal-comment, concurrency enforcement
via shared advisory lock, structural (not deparse-text) divergence detection,
:create_failed grace; --adopt restricted to the floor slice gated on data
invariants; down full-teardown split (range + direct V{floor}.down); D13 clamp
guarantee scoped to PK wrappers with consumer-authored interleaved migrations
as a documented breakage class (consolidate_wrappers promoted to deliverable);
two-stage rollout replacing the hold-window; new §7.4 module-ecosystem pin
coordination; every floor raise = major + own bridge; matrix grown to S20;
inventory customer_service→customer_support (V109) corrected.
Layer1 + verify harness finished and offline-gated (reports in job tmp);
generate_baseline.exs left mid-completion by the interrupted run (known open
defect: undefined maybe_dump/2 near lines 2329/2446). Paused by operator
before the finish/review/fix pass; resume via workflow script
squash-p1-finish-wf_39004ad0-201.js (all agents pinned sonnet@max).
…ires
GLM@max findings applied: whitelist filtering now matches only the identifier
a dump line DECLARES (references to whitelisted columns inside other objects'
definitions no longer tolerated — closes the false-S1-pass hole); InventoryGuard
doc-token for publishing_posts.status tracks the column, not its index; README
notes S1 dumps deliberately include oban_* (Oban-version sensitivity); hand-
review checklist gains the function-body schema-qualification item; generator's
drop_schema quotes the identifier and refuses public. Extra hardening: verify's
Mode-A reset gains a no-override tripwire refusing DROP SCHEMA public on any
database with populated phoenix_kit_users (live-install detector on top of the
PK_SQUASH_ALLOW_RESET gate). Kimi review skipped: provider quota exhausted
(403) until next billing cycle. All --check gates green.
…ma tripwires, qualified whitelist
All four findings applied: (1) below_floor_matcher docstring no longer claims
an unasserted field; (2) named-schema drops gain the same live-data tripwire
public got — generate_baseline drops --schema-* names PRE-RUN, so a typo
naming a live prefixed install would have wiped it (populated
phoenix_kit_users => refuse, no override, outside the rescue); (3) whitelist
self-checks now cover the ADD CONSTRAINT continuation-line and comma-in-type
column branches via a dedicated fixture pair; (4) column whitelist entries are
table-qualified (table.column) through generator tokens, verify default list,
and the differ's matcher — a same-named column in another table can never be
masked (negative self-check added). ALTER TABLE heads whose only action lines
were tolerated now count as structural. Both --check gates green.
…tasks, tooling fixes
Spec section 11 P2 (pre-squash PR; works on the un-squashed chain, forward-
ready for P3):
- PhoenixKit.Migrations.ExpectedSchema.Behaviour + typed Object contract
derived from the P1 generator's emitted shape (generator is ground truth);
Resolver with :not_generated degradation until the real manifest is
generated; test/support fixture manifest.
- PhoenixKit.Migrations.Repair (+ Probe/Executor/Scope/Environment/Report):
verify/repair per spec section 6 — raw comment read bypassing the legacy
no-comment->1 mapping, since<=comment scoping at comment-era revisions, no
delta re-execution, additive-only class-ordered executor with NOT
VALID+VALIDATE and :create_failed grace, structural catalog comparison with
forced empty search_path, comment policy R1-R6 incl. --adopt floor-slice +
data-invariant gate and --heal-comment, pooled detection with dry-run
bypass, advisory lock with comment re-read. mix phoenix_kit.repair CLI +
doctor wiring.
- mix phoenix_kit.consolidate_wrappers (consumer wrapper-history collapser,
dry-run by default, interleaved-migration refusal, re-run safe).
- gen.migration: from-version scan now matches add_phoenix_kit_tables (the
installer's actual filename) alongside the legacy create_ form.
- release_check: min==initial_version, contiguity + loadable-module range,
ExpectedSchema chain_hash freshness (SKIP-with-notice until generated);
range-completeness also as a plain unit test.
Two internal reviews (9 findings, 3 MAJOR) applied; mix precommit green incl.
dialyzer; mix test: 1252 tests, failures limited to 2 pre-existing
no-database timeouts unrelated to this work.
…se name
Matches the existing PGUSER/PGPASSWORD/PGHOST env-override pattern; lets the
suite run against an operator-provided scratch database whose role lacks
CREATEDB (ecto.create tolerates an existing DB). Default unchanged.
A schedulers*2 pool is antisocial on a shared PostgreSQL near its
max_connections ceiling (the target scratch server runs 37 projects at
400/400 slots); PGPOOL lets constrained environments run the suite with a
minimal pool. Default unchanged.
The third hunk (upstream's dashboard-deprecation + demo-routes checks vs P2's
manifest-repair check) was committed unresolved during the rebase; keep both
sides.
Inventory: V152-V160 addendum (email send-profiles move, CRM lists +
newsletters->CRM migration with its five-step guarded backfill, OG images,
publishing categories/views, settings.value TEXT widening; V157 introduces the
chain's first data-conditional down-guard). Sixth renumber event recorded
(V151->V152, da87ced). InventoryGuard gains 8 exclusion tuples for the
V152/V156 drops (newsletters_send_profiles + nl indexes, newsletters_lists +
list_members, broadcasts.list_uuid + its FK and index) — the V152->V155
recipient_check same-name redefinition is a native shape revision, not a
curation case. Spec counters: 160 files / 27,311 lines, floor table remainders
recalculated (v01..v147 verified untouched since 1.7.193). Mechanics verified
unchanged: postgres.ex diff is moduledoc + @current_version only; helpers.ex
and migration.ex byte-identical; version_checks/0 still the single V83 entry.
Seed lists and Catalog capture need no changes (zero new seeds/functions in
range). All gates green after the 1.7.227 rebase: compile
--warnings-as-errors, 304 unit tests, both --check gates incl. the
inventory-doc cross-check against the new guard entries.
Operator confirmation 2026-08-04: the external DEV/prod installs from the June
task statement are out of scope — their names are removed from these documents
(anonymized in the historical June prior-art docs, which are commit history
bound for the public fork). Floor data table reduced to the one verified
install (live at V160); D2 floor candidate moves 121 -> 160; floor-candidates
table gains the 160 row (159+1 files, 27,261 lines removed, zero delta files);
section 10 Q1 marked resolved. External/public Hex consumers are bridge-path
by design at any floor (permanent bridge + BelowFloorError).
…onical form
First live oracle run caught it: V56-era prefix_index_name/2 (v56.ex:574-576)
embeds the schema name INTO index names on named-schema installs
(<prefix>_phoenix_kit_*_uuid_idx) while public installs get the bare name, so
two named scratch installs never compared equal and public-vs-named never
could. substitute_schema/2 now strips '<schema>_' when immediately followed
by 'phoenix_kit' (comparison-layer fold only; runs before the word-boundary
rule which deliberately cannot reach inside identifiers). Self-check fixtures
pin named==public folding. The manifest-side handling of the same idiom
(templated index names, else baseline/repair on prefixed installs would
create a bare-name duplicate) is a tracked P3 item.
First live run of the repair integration suite + manifest generator against
the scratch DB surfaced and fixed:
- repair_test.exs: sandbox flipped to :auto per the prefix_migration_test
pattern (Repair's own repo.checkout is incompatible with :manual ownership);
cleanup registered before setup work so failures cannot leak schemas. 8/8
scenarios now pass on real PostgreSQL.
- Seed capture: text[] columns (storage_dimensions.alternative_formats, V98)
decode as native lists — captured as terms; Emitter renders native array
literals for *[] column types and canonical (sorted-key, local encoder)
JSON ::jsonb for genuinely-compound values; --check fixture now pins both
rendered forms byte-exactly.
- Differ: not_null excluded from :column comparison (an expected
not_null+no-default column can never be satisfied by the additive-only
executor by design, so comparing it manufactured a permanent error finding
and blocked --adopt's clean gate); unit-covered both ways.
- Oban delegation: Oban.Migration.up/1 requires an Ecto.Migration.Runner and
crashed every non-dry-run repair; new Repair.ObanRunner bootstraps
Runner.run/8 directly (no schema_migrations side effects), verified live.
- Fixture manifest: undeclared seed column and placeholder uuid-function
md5/definition replaced with empirically-captured real values.
Full generator run remains blocked by a separate pre-existing chain finding
(47-object stepwise-vs-single-shot SHAPE bimodality) — tracked for P3.
Root cause (empirically proven on the scratch DB): execute/1 only QUEUES DDL
while the chain's existence guards are immediate repo().query reads — v56
called UUIDFKColumns.add_constraints with no flush() after UUIDFKColumns.up,
and v57 had no flush() at all, so single-shot installs (fresh projects) never
generated SET NOT NULL for 46 *_uuid columns and never created the comments
FK at V56/57; v72 then filled the 'missing' FK with a guessed ON DELETE
CASCADE while every incrementally-upgraded install carries V57's SET NULL.
Fix: flush() discipline in v56/v57 (in-place hardening precedent PR BeamLabEU#628/631);
v72's comments entry aligned to the intended SET NULL (content anonymizes on
user deletion — likes/dislikes stay CASCADE); new V161 repairs
already-affected single-shot installs: SET NOT NULL only where zero NULL rows
exist (warn+skip otherwise — never backfills live data), comments FK corrected
metadata-only via name-anchored pg_constraint check. UUIDFKColumns gains the
not_null_uuid_fks/0 accessor so V161 shares the canonical list.
Verified: both modes now agree to V161 on all pairs + the FK; full generator
run completes with NO shape mismatch and emits the first complete
ExpectedSchema manifest (159 tables / 1869 columns / 417 constraints / 609
indexes / 114 seeds). Known follow-ups tracked for P3: prefix-embedded index
NAMES still enter the manifest untemplated (12 uuid-unique indexes,
:legacy_optional misclass), and UUIDFKColumns carries a dead symmetric
subscriptions.plan_uuid entry.
Names built from the prefix exist in TWO conventions, both now captured
canonically and resolved at run time from the target prefix: v56/v61's
prefix_index_name (bare on public, prefixed otherwise — marker
__PK_NAME_EXEMPT__) and v26's unconditional "#{prefix}_..." checksum index
(prefixed on public too — __PK_NAME_ALWAYS__; verified against a live public
schema). Catalog canonicalizes at capture (Differ/bimodality need no changes
— the 24 phantom :legacy_optional entries vanish), Emitter re-attaches
markers with a conditional pn binding in emitted code, Object.materialize/2
mirrors both markers for hand-written manifests and now routes {:catalog,
spec} values through substitution. Classification refuses to guess (raise)
on any unrecognized prefix-name shape. Dead UUIDFKColumns
subscriptions.plan_uuid entry deleted (column never existed; real FK is the
nullable subscription_type_uuid — verified live).
Generator re-run: zero scratch-schema leakage in the emitted manifest, 39
resolved markers, bimodality whitelist reduced to exactly
users.preferred_locale + its index; repair integration suite 8/8. Fixture
note: a redundant unique index on a PK column gets silently adopted by
Postgres as FK-backing and vanishes from Probe's index query — documented in
the fixture to prevent 'fixing' it back.
GLM external-review MAJOR on b213332: @not_null_uuid_fks is a V56-era
snapshot, and V113 deliberately relaxed phoenix_kit_files.user_uuid
(system-managed media rows insert user_uuid=NULL under the user_or_parent
CHECK) — V161 re-imposing NOT NULL on a fresh install broke tile generation.
V161 now carries @relaxed_after_v57 (audited to be exactly the one V113
entry: every DROP NOT NULL across v58..v161 + uuid_fk_columns was
intersected with not_null_uuid_fks/0 — all other hits are non-member tables,
bare uuid PK rollback paths, or integer-era columns) and skips it. New
DB-free static-scan test keeps the exclusion list honest against future
relaxation versions. Minors: comments on the FK DROP+ADD non-atomic window,
the null_count race (serialized by Ecto.Adapters.Postgres
lock_for_migrations' SHARE UPDATE EXCLUSIVE on schema_migrations), and
skip-path semantics pointing at mix phoenix_kit.repair. Generator re-run:
manifest revisions for files.user_uuid end at {113, not_null: false} — no
V161 entry; repair suite 8/8.
…sed)
Spec section 5.2, implemented floor-independently: up/1 and down/1 are now
thin dispatchers over pure plan_up/3 and plan_down/3 helpers (floor is an
argument, unit-testable at any value — 21 tests cover today's floor-1 shape
byte-for-byte AND the synthetic floor-121 dormant branches). New
PhoenixKit.Migrations.BelowFloorError carries db_version/floor/
bridge_version/context; ensure_current/2 reraises it with the test-DB reset
hint; phoenix_kit.update refuses wrapper generation for a below-floor DB at
status time (extracted handle_installation_status/5 keeps credo complexity
happy). down-to-zero teardown runs the delta range PLUS the floor module as
one change/3 list — no range ever crosses the floor boundary (review M3) and
the progress header stays intact; a clamped below-floor down leaves the
comment at the floor via V{floor+1}'s own self-stamp. verify.exs's
below_floor_matcher confirmed field-compatible unchanged. Also: alias
Ecto.Migration.Runner in ObanRunner (credo strict [D] from b213332).
At @initial_version 1 every new branch is unreachable — full unit dirs,
prefix oracle + repair suite (9/9, 8/8), both --check gates green;
integration mass-failures at default concurrency were pool-vs-max_cases
arithmetic, rerun clean at --max-cases 4 modulo pre-existing time-of-day
flakes (stash-baselined).
Owner field (modularization groundwork, spec 5.1): every manifest object
carries owner: derived from a data-driven table-family mapping with
false-positive guards (file_locations/settings stay :core); real-manifest
counts: core 1754, warehouse 187, crm 150, comments 143, document_creator
137, projects 127, ai 109, publishing 107, staff 104, locations 64,
newsletters 56, og_images 24, catalogue 3.
verify.exs: s7/s8/s9/s10/s12/s13/s17/s19/s20 now run against the REAL
generated manifest (new :generated_manifest requirement; s18 stays a
documented manual trigger). Along the way: tamper matrix scoped to
non-dependent objects; RETURNING uuid decoded via Ecto.UUID.load (raw
16-byte binaries interpolated into SQL reproduce
character_not_in_repertoire); print_exception_safely stops a non-UTF8
exception message from killing every queued scenario; s12 builds its schema
BEFORE switching to the pooled repo (PgBouncer really does eat DDL) and
scope-overrides repo config for dynamic instances; s13 adopts against a
floor-state install per spec wording.
Run: 8 PASS, s18 SKIP:manual, s13 FAIL — kept strict deliberately: it
exposes a genuine generator/Probe search_path asymmetry (manifest stores
'citext', live probes read 'public.citext'; blocks --adopt on every real
install via users.email) fixed in the follow-up commit.
Catalog.snapshot/2 now captures under SET search_path TO '' (checkout +
try/after RESET), mirroring Probe.snapshot/2: shared extension types were the
gap — citext lives in public, which sat on the generator connection's default
path, so the manifest stored 'citext' while live probes always read
'public.citext', permanently diverging all five real citext columns and
blocking --adopt on every install via users.email. Regenerated manifest
carries public.citext (5/5); bimodality section byte-identical (extension
types were the only affected class). The citext_qualification_gap tolerance
is deleted from verify.exs — s8/s10 strict again (s8's idempotence check
restored to the exact summary-comparison semantics). Matrix: s7/s8/s9/s10/
s12/s17/s19/s20 PASS; s13's citext-gated assertion confirmed fixed (composite
reports SKIP only for the orthogonal dormant-invariant sub-case, which needs
the floor itself to rise past 77 — P3); s18 documented-manual. Repair
integration suite 8/8 (fixture has no citext columns — proven untouched).
The 161 slot is taken upstream by two open PRs (BeamLabEU#681 our own citext-username
branch, BeamLabEU#682 mdon's payment-option linkage), so this repair migration moves to
the first free slot when they land. Records why it cannot be renumbered
pre-emptively (contiguity: a hole makes change(1..N) crash on the missing
module — release_check and its test both catch it, verified empirically) and
the exact renumber ritual incl. manifest regeneration.
…direct work) into squash-migrations
# Conflicts:
#	config/test.exs
#	lib/phoenix_kit/migrations/postgres.ex
#	lib/phoenix_kit/migrations/postgres/v161.ex
Upstream took 161 (our own citext-username PR BeamLabEU#681) and 162 (payment-option
linkage BeamLabEU#682, itself renumbered off 161), so the flush-bug repair migration is
now V163: file+module, self-stamp '163', down restamp '162',
@current_version 163, moduledoc LATEST marker, and the guard test renamed with
@exempt_version 163. Spec counters: 163 files / 28,000 lines, floor-candidate
remainders recalculated (deleted-line figures unchanged — v01..v147 have never
been touched). The generated manifest is now STALE (its chain_hash pins the
pre-merge file set) and must be regenerated before it is trusted.
External review (GLM-5.2 max effort) returned APPROVE with six items; all are
folded in, one of them load-bearing:
- V163 no longer enforces NOT NULL on
phoenix_kit_ticket_status_history.changed_by_uuid. V56/V57's own two lists
contradict each other there — @not_null_uuid_fks claims the column while
@fk_constraints declares its FK ON DELETE SET NULL, which NOT NULL makes
unsatisfiable: deleting a user who ever changed a ticket status would fail
with a not-null violation instead of blanking the author. On the broken
installs this repair targets the column is nullable, so deletion works
today; enforcing it would newly break them. A systematic audit found this
is the only such pair (47 NOT NULL entries vs 70 FK declarations), and a
new test pins that no other pair can appear unnoticed.
- The warn paths no longer name a task that does not exist in a plain core
checkout; they describe the one-line ALTER instead.
- Stale V161 references (v72, the UUIDFKColumns accessor docs) now say V163 —
V161 is upstream's citext-username migration.
- The root-cause narrative no longer claims incremental runs were immune: V56
and V57 are sub-calls sharing one command buffer, so ANY run crossing them
in a single migrator invocation is affected.
- The guard test also scans the Ecto DSL form and
documents the one shape it cannot anchor (interpolated column from a
module-attribute list, v73-style).
- Fork-internal references (squash generator, bimodality sweep) are gone from
the migration's moduledoc, which is bound for upstream.
Captured from the floor-135 generator run before any deletion happens: the
normalized schema dumps of a stepwise and a single-shot V01..V163 install plus
the seed-row reference and the generation report. After the squash these are
what baseline+deltas must reproduce — S1/S2 lose their oracle if the old chain
is deleted without them, since regenerating the reference requires the very
files the squash removes. The report records the pre-squash chain_hash
(2b158b7c…), object counts (1870 columns / 163 files) and the bimodality
whitelist (users.preferred_locale + index only).
build_config/0 hardcoded pool_size: 5, so every generate_baseline.exs and
verify.exs run against the shared, near-ceiling instance opened five
connections no matter what PGPOOL said — only config/test.exs (plain mix test)
honored the variable. connection_env/1 now parses it with the same semantics
config/test.exs settled on: blank is treated as absent rather than raising,
non-numeric or non-positive raises naming the variable and its value. Default
stays 5, matching the previous hardcoded behavior. Five self-check assertions
cover default / parsed / blank / non-positive.
…V134
The consolidation itself. 134 version modules (~26,000 lines) collapse into one
generated baseline; the chain is now V135..V163, 29 files.
- lib/phoenix_kit/migrations/postgres/v135.ex is the baseline: the cumulative
schema of V01..V135 emitted from a real migrated database (11,156 lines,
1,199 statements) in Helpers idioms — privilege-aware extensions,
schema-qualified uuid_generate_v7, bare index names on CREATE, catalog-guarded
constraints, final-state seeds, self-stamps '135'.
- lib/phoenix_kit/migrations/expected_schema.ex is the generated manifest the
repair engine and verify layer consume (objects with since/revisions/presence/
owner + data invariants + chain_hash).
- @initial_version 1 -> 135, which wakes the guard machinery that has been
dormant since it was written: an install below the floor now raises
BelowFloorError naming the bridge release instead of dying on a missing
module, a fresh database clamps to the baseline, and teardown never lets a
range cross the floor.
- Retired as unreachable at this floor: UUIDRepair (its < V40 gate) and the
{83, ...} heal entry. Version-pinned tests below the floor are gone; their
load-bearing assertions live on in the new v135_baseline_schema_test.
- Docs: the prefix-safe-migrations guide and the inventory now name the
baseline module and Helpers instead of the deleted V01/V27/V40/V51 sites.
Gates: compile --warnings-as-errors clean, format clean, 1115 unit tests 0
failures, release_check confirms current_version == v163.ex, min on disk ==
initial_version == 135, V135..V163 contiguous with every module loadable. The
manifest's chain_hash still pins the pre-deletion file set — regeneration over
the squashed chain follows in the next commit, and the S1/S2 equivalence run
against dev_docs/squash/reference/pre_squash_chain_v163_*.sql is what proves
the baseline reproduces the old chain.
The squash commit shipped a baseline generated BEFORE its own floor-carryover
logic landed, so objects created below the floor and dropped above it were
missing: a fresh install died at V152 with 'column list_uuid of relation
phoenix_kit_newsletters_broadcasts does not exist' — V79 created that column,
V152 drops its NOT NULL and V156 drops the column, so the final-state view the
manifest is built on legitimately omits it while the baseline must still create
it. Regenerated from a full pre-squash chain: list_uuid, its FK and its index
are back, and a stepwise V135..V163 run now completes cleanly (S1 mode
cross-check equal, strict).
Also: the generator's baseline comments said 'final-state' where the rule is
at-floor (that sentence was the defect in prose); the --check regression guard
gained a floor=3 case where since(1) < floor(3) < dropped_at(5), so 'carried
because it predates the floor' can no longer be confused with 'carried because
it equals the floor'; the generation report now tags every such object
[BASELINE-CARRIED] with a count (27-28 of ~657 drops). Spec 5.1 rewritten to
state the at-floor rule for the baseline and that the manifest stays
final-state.
release_check now fully PASSes: current_version == v163.ex, min on disk ==
initial_version == 135, V135..V163 contiguous with every module loadable, and
chain_hash matches the 29 on-disk files. Equivalence against the pre-squash
reference is down to 25 lines in two classes still under investigation (CHECK
deparse canonicalization, and 20 _uuid_idx lines whose duplication in the
reference may be the normalizer folding two really-distinct index names).
Both were reported clean by every offline gate and both fired on healthy
production installs. The S1 dump comparison is what surfaced them.
1. Indexes that are FOREIGN KEY targets were invisible. Probe and the
generator's Catalog both excluded constraint-backed indexes with
LEFT JOIN pg_constraint ON con.conindid = i.indexrelid ... WHERE con.oid IS
NULL, intending 'this index belongs to a PK/UNIQUE constraint on its own
table'. But Postgres also points a FOREIGN KEY's conindid at the unique
index on the REFERENCED table — so any uuid index another table's FK relies
on vanished from the inventory. Verified live on ai_endpoints_uuid_idx,
blanked by fk_ai_requests_endpoint_uuid, a constraint on a different table.
Consequence: mix phoenix_kit.repair reported 6-8 indexes as missing and
'repaired: created' on a byte-correct fresh install, so it could never
report clean — dozens of tables in this chain have that shape. Both copies
now join on contype IN ('p','u'); the manifest gained 14 previously
invisible indexes (594 -> 608).
2. CHECK constraint comparison was raw text equality, so Postgres's two
equivalent renderings of an array cast — ARRAY[(x::varchar)::text, ...] vs
(ARRAY[x::varchar, ...])::text[] — read as drift. Differ now canonicalizes
both sides (mirrored in DumpHelper's normalizer with its own self-check
fixture), covered by a DB-free test that also proves a genuine value-set
change is still caught.
s8 (repair idempotence), s10 (data preservation) and s13 (--adopt) flip from
FAIL to PASS with these two fixes and nothing else. Unit suites green (263
tests), release_check still PASS on contiguity and chain_hash.
Pins the element-wise and array-wise ANY(ARRAY[...]) renderings folding to one
form, that already-canonical text passes through unchanged, and that a genuine
value-set change still diffs.
…hash guard real
External review caught what a partial test run hid: three assertions still
described the pre-squash world and were red on this branch, in files outside
test/phoenix_kit/ (which is all the earlier 'green' claim had covered).
- phoenix_kit_test.exs asserted initial_version == 1; it now derives the
expectation from the oldest v*.ex on disk, so a future re-squash keeps it
honest, and additionally asserts the floor is above 1.
- release_check_test.exs assumed the manifest was NOT generated (it is, since
the squash promoted it into lib/): the not-generated path is now reached by
pointing the resolver at a nonexistent module, and check_migration_sync is
asserted in its real shape (no SKIP, chain_hash matches).
- The only previously-passing chain_hash test compared a stub against itself.
A new test compares the SHIPPED manifest's chain_hash to a freshly computed
one, so editing a v*.ex without regenerating now fails in the habitual
mix test loop instead of only in the manual release check.
Full suite: 1915 tests, 0 failures.
…le comments
- migration.ex named PhoenixKit.Migrations.SQLite and .MyXQL for the SQLite3
and MyXQL adapters; neither module has ever existed, so anything reaching
those clauses would have raised UndefinedFunctionError. The chain is
PostgreSQL-only by construction (citext, pgcrypto, pg_trgm, JSONB, partial
and expression indexes), so a non-Postgres adapter now either uses the host's
own configured :phoenix_kit_migrator or gets an ArgumentError that says so
and shows how to configure one.
- Repair's two remaining text-only comparisons — index predicates and
CHECK/exclusion definitions, the fields with no structural decomposition —
now carry a marker, and a mismatch whose every reason is rendering-derived is
reported as :info instead of :error when the connected PostgreSQL major is
outside the verified range. On a verified major a rendering difference is
still drift and stays :error; on an unverified one an operator no longer gets
a wall of false positives from pg_get_*def simply re-rendering the same
expression.
- Comments that described the guard branches as dormant 'while initial_version
is 1' now say they are live at floor 135 (update.ex's generation-time
refusal, postgres.ex's down-clamp, comment_policy's scope note), and
executor.ex no longer points at the retired UUIDRepair as a live reference.
Credo clean on the changed files; 348 tests in the migration and mix-task
suites pass.
…pends on
The equivalence evidence was sitting uncommitted, so the S1 PASS was not
reproducible from the tree. Committed: the v163 reference dumps (the previous
pair recorded chain_version=160 and the harness correctly REFUSED to compare
against them), the NAMEDATALEN-safe scratch-schema naming (equal-length slots
plus a hard budget check — unequal lengths truncated one side's
prefix-embedded object names and produced a phantom rename), and the duplicate
statement collapse with its self-checks (the old chain creates two physically
distinct indexes per uuid column, one prefix-embedded and one bare, which the
normalizer folds; collapsing is one-directional so a genuinely absent object
still diffs).
Covers what both reviews found undocumented: the bridge stopover for
below-floor databases and why every environment must be confirmed separately,
the repair's behavior changes (comments FK CASCADE -> SET NULL, ~46 columns
regaining NOT NULL, ~67 foreign keys the V56/V57 defect never created) with the
locking cost of validating them, the 20-byte prefix limit that the
prefix-embedded object names impose, rollback semantics below the floor, and the
one cosmetic divergence a single-shot old-chain install keeps. Includes the SQL
an operator runs to see whether the defect hit their install.
An independent review measured the engine against real databases and found it
could never exit 0 — not even on a fresh install this chain built itself. Four
causes, all fixed:
- Two required seeds delegate to helper modules that live in the separate
phoenix_kit_emails package, so a core-only install reported them missing
forever while an apply run reported them skipped — the two paths disagreed.
Both now stay silent when the companion module is genuinely absent. (Marking
the objects :legacy_optional, the alternative, would have made them invalid
against their own contract: that presence forbids a non-nil create.)
- uuid_generate_v7() was compared by the md5 of its body while the helper never
replaced an existing function, so any install predating the pgcrypto
qualification carried a permanent error with no path to fix it. The helper now
always issues CREATE OR REPLACE and repair can reissue it — but a function
owned by another role (the hardened-install topology: schema pre-created by a
DBA, app role without CREATE) would then abort the migration where the old
guard silently skipped, so an insufficient-privilege failure on an
already-present function now warns and leaves it alone instead of raising. A
genuinely absent function still fails loudly.
- Every healthy install at the chain head was told its comment was ahead of its
schema, because V163 contributes no manifest objects and the presence scan
could never reach it. Versions with no objects now count as vacuously present,
bounded by the comment rather than the code version, and the dry-run message
no longer claims findings 'have been addressed'.
- validate_prefix!/1 checked format but not length, so a prefix of 21+ bytes
silently truncated the 42-byte prefix-embedded object names against
PostgreSQL's 63-byte limit and repair then reported three permanent phantom
findings. The cap is now enforced with a message naming both numbers.
Integration tests that assumed the pre-squash floor (state built at V100 and
V142 — below the floor and between an object's revisions) were repaired, and two
scratch prefixes longer than the new cap were shortened.
Fresh V163 install now verifies with exit 0 and no actionable findings; full
suite 1918 tests 0 failures; credo and dialyzer clean on the changed files;
repair integration 8/8; S-matrix s7-s20 PASS.
Upstream merged its own V163 (uuid primary-key integrity, PR BeamLabEU#688) while this
branch was in flight, so the repair delta this branch carries moves to V164 to
leave that number to it. Their repair runs first by construction, which is the
order this one needs: it promotes uuid columns to primary keys, and a foreign
key cannot reference a column that has none.
Also folds in what a draft carried as a separate V164 — this release ships one
migration, not two — and closes the GLM/Opus review round: shape-based FK
matching, a guarded ADD CONSTRAINT, SQLSTATE in the validate handler, an
end-of-run NOT VALID summary, a bounded advisory-lock wait, plus the mode-A
public-path oracle and the s21/s22 scenarios that gate all of it.
# Conflicts:
#	lib/phoenix_kit/migrations/postgres.ex
The adversarial review pass (Kimi) found one blocking defect the two earlier
rounds missed: `phoenix_kit_users_tokens.user_uuid` was on V56/V57's NOT NULL
list while V64 later added a CHECK that deliberately permits NULL there —
magic-link REGISTRATION tokens have no user yet, and `MagicLinkRegistration`
inserts them as a bare struct, so a NOT NULL column raises rather than
returning a changeset error. Only the flush defect kept real installs working:
it swallowed the enforcement. The generated baseline came from the FIXED chain,
so a fresh 2.0 install would have broken registration from day one, and the
repair would have broken it on upgrade — the NULL count is virtually always
zero at deploy time, since those tokens live 15 minutes.
Fixed at the source (the list entry is simply wrong) rather than excluded in
the repair, so every path agrees: the chain, the baseline, the manifest and the
bridge's own replay. A new static guard fails on any NOT NULL-list column
covered by a conditional CHECK — verified to fail with the entry restored.
Also from that round: the comments-FK fallback no longer nulls live references
nor re-raises an error the guarded pass already handled (it aborted the chain
AFTER that UPDATE had auto-committed); a publishing-index replacement whose
CREATE failed is now recoverable instead of leaving the table permanently
without its unique index; NOT NULL is not re-issued on columns that already
have it; the FK shape probe anchors the referenced relation to the schema; a
wrong ON DELETE under the expected constraint name is reported instead of
silently adopted; a missing or non-numeric version comment now refuses to
migrate instead of guessing "V01 legacy" and routing a current database to the
bridge's backfill, while the read path reports rather than raising so the admin
UI stays up; and a negative rollback target takes the teardown path instead of
dispatching versions the squash deleted.
Verification on this tree: full matrix 0 FAIL (every scenario passing, three
documented SKIPs), mode-A public-path oracle passing, 1919 tests, and the
release gate green. The matrix run predates only the two version-comment
guards, whose paths the unit suite covers; a targeted re-run of the scenarios
they touch is in flight.
@ddon
ddon merged commit cbf70fe into BeamLabEU:mainAug 9, 2026
ddon pushed a commit that referenced this pull request Aug 9, 2026
`ensure_uuid_v7_function/1` dropped its existence guard so a stale function
body could be refreshed, and added a rescue to keep the hardened-install case
safe. In migration context the executor is `Ecto.Migration.execute/1`, which
only QUEUES the statement — insufficient_privilege arrives at flush time, past
where any rescue here can reach it, and the migration aborts. That is the
topology `PhoenixKit.Migration`'s own moduledoc tells a DBA to adopt, and
`up/1` calls this helper on every delta upgrade, so it was not confined to
fresh installs. The un-ownable case is now excluded before the statement is
queued, via `pg_has_role` — the same test Postgres applies for the replace, so
a function owned by a role we are a member of still gets refreshed.
`ExpectedSchema.chain_hash/0` was stale at the merge: the branch restamped it
over its own `v163.ex` while main advanced to 1aea3d2, and the merge took the
newer file with the older stamp. `mix phoenix_kit.release_check` failed and so
did two unit tests. Restamped over the 30 shipped files; neither edit since the
old stamp adds or alters a manifest object, and the header now records that
rather than leaving it to be re-derived. A restamp still asserts nothing about
the manifest body — verify.exs against a real database is what does.
V164 warned "Left untouched — Reconcile by hand" about `fk_comments_user_uuid`
under V72's guessed CASCADE, and then dropped and re-added it SET NULL ten
lines later. False, about the one defect the migration is best known for.
`BelowFloorError` carries a `:bridge_version` field documented as nil until the
bridge is tagged. It is tagged, but neither raise site passed it, so the most
operator-facing message in the release made them go find out which 1.7.x "the
last one" was.
Also five doc blocks left from the pre-squash draft, two of which contradict
themselves inside a sentence ("the below-floor branches are LIVE ... unreachable
today").
Review: dev_docs/pull_requests/2026/689-squash-migrations/CLAUDE_REVIEW.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timujinne

Copy link
Copy Markdown
ContributorAuthor

Post-merge review of the migration work — Kimi K3, verified

Second-opinion pass over what landed in #688, #689 and the two fix commits on
top (b3ef57b2, 1aea3d28), by Kimi K3 at max effort. It was explicitly told
not to re-report two defects already known (the pk_sqv schema left behind by
verify.exs combined with baseline tests that query pg_indexes without a
schemaname filter; and the post-rename V164 drift where the comment reads 164
while the index V164 creates does not exist), so everything below is new.

I verified every finding against the tree before posting this. Where the
code's own comments prove the point, I quote them rather than paraphrase. One
finding it flagged as unverified I checked myself and it holds.


BUG - HIGH — a comment-less database is routed to the bridge replay the migrator itself calls destructive

lib/phoenix_kit/install/common.ex:270-272lib/mix/tasks/phoenix_kit.update.ex:463-464

query_version_directly/2 ends with, verbatim:

_-># Table exists but no version comment - assume version 11

So a database that is current but lost its comment — a half-installed or
adopted state that repair --adopt exists to handle — reports
{:current_version, 1}. update.ex:463 then sees 1 > 0 and 1 < 135 and emits
the below-floor notice: "Install the last PhoenixKit 1.7.x release (the
migration bridge) first."

The migrator refuses the same input, and its refusal text says why that advice
is dangerous (postgres.ex:731-747):

Pre-squash a missing comment was read as "V01 legacy" and the chain simply
replayed from the start; this release cannot do that — below-floor versions no
longer exist here, so the guess would route a possibly CURRENT database to the
1.7.x bridge, whose backfill overwrites still-NULL tracked columns with freshly
generated uuids pointing at nothing.

Two refusal messages for one state, pointing opposite ways, and the operator
meets the update task's one first. Downstream on the bridge, the replay is not
benign on a populated database: UUIDFKColumns.set_not_null/4 backfills
legitimately-NULL phoenix_kit_files rows with invented uuids, and
cleanup_orphaned_fk_refs/5 then deletes them for matching no user.

Note the other fallback in the same module was already hardened — its comment
records that it "used to fall back to a fabricated {:current_version, 1}
Report honestly instead." The fabrication survives one level down.

Fix: distinguish "no comment" from "comment = 1" in
check_installation_status/1, and point the no-comment case at doctor +
restamp rather than the bridge.

BUG - HIGH — the manifest pins uuid_generate_v7's body to public.gen_random_bytes, so repair can never converge where pgcrypto lives elsewhere

lib/phoenix_kit/migrations/expected_schema.ex — the function object's stored
definition contains public.gen_random_bytes(10) and is compared raw in
repair/differ.ex. The self-heal path regenerates that body through
Helpers.ensure_uuid_v7_function/2, which writes that install's pgcrypto
schema.

Install with pgcrypto in a schema other than public — the DBA-provisioned
topology PhoenixKit.Migration's own moduledoc supports — and
mix phoenix_kit.repair reports :wrong_shape:repaired on every run,
forever, because the reissued body can never match the pinned md5. --adopt is
not blocked by it (:repaired is repairable severity), so adopt stamps the
floor while the finding persists.

Fix: normalise the pgcrypto schema out of prosrc on both sides before
hashing, the same way the array-cast canonicalisation already works in Differ.

BUG - HIGH — the manifest's pre-164 shape for idx_publishing_posts_group_slug disagrees with every healthy public install

lib/phoenix_kit/migrations/expected_schema.ex:36115-36161

The earlier revisions carry predicate: nil; the partial form appears only at
revision 164. The correction comment sitting between them says:

Every real install has the predicate; V164 normalizes the rest onto it.

Both statements cannot hold. shape_at(object, 163) selects a predicate: nil
revision, so a public install at comment 135..163 — every host that finishes
the bridge and runs repair or doctor before migrating — gets :wrong_shape,
error severity, exit 2, reporting expected nil, got "(slug IS NOT NULL)" on a
byte-correct database. Named-schema installs verify clean; this false-positives
the dominant topology only.

The sibling case one table away was handled as :legacy_optional. This one
needs the same, or a bimodal revision at 68.

BUG - MEDIUM — V164's post-add check matches a constraint by name only, so a collision reads as :created

lib/phoenix_kit/migrations/postgres/v164.ex:644-660

fk_exists?/3 queries conname + relname + nspname — no contype, no shape:

SELECT EXISTS (SELECT1FROM pg_constraint c … WHEREc.conname=''ANDt.relname=''ANDn.nspname='')

Pre-create detection is shape-based, so a differently-shaped constraint under
the expected name returns :absent; the guarded ADD then fails 42710 and is
swallowed by the EXCEPTION WHEN OTHERS handler; the verification sees the
colliding constraint and reports :created. One buried RAISE WARNING, no
{:failed, …}, no SUMMARY line, comment stamped 164, declared FK absent.

Same class as the known V164 drift — a verification step matching the wrong
thing — inside the migration whose purpose is repairing that class.

BUG - MEDIUM — repair dies on the anomaly it exists to diagnose

lib/phoenix_kit/migrations/repair/probe.ex:81 calls String.to_integer(version)
unguarded and untrimmed, eleven lines below a docstring that says "Never
raises"
(:51). A hand-edited comment — 'v164', ' 164', the exact examples
postgres.ex:682-685 cites — crashes mix phoenix_kit.repair with a bare
** (ArgumentError) argument error. doctor survives only because its check
wraps everything in rescue.

The same unguarded parse is in install/common.ex:268, on the path described in
the first finding.

IMPROVEMENT - MEDIUM — the below-floor notice still does not name the bridge

lib/mix/tasks/phoenix_kit.update.ex:611 says "the last PhoenixKit 1.7.x
release (the migration bridge)". The fix commit threaded @bridge_version "1.7.236" into both BelowFloorError raise sites but not into the message
below-floor operators see first, at generation time.

IMPROVEMENT - MEDIUM — v163.ex scans before it checks the size guard

castable?/3 (a full-table count(*) … !~*) runs before the 2M-row guard, so
the deferral path for a huge broken varchar table still pays an unbounded
sequential scan. Read-only, so not the ACCESS EXCLUSIVE outage the guard exists
for — but on a PgBouncer-fronted pool it pins a connection for minutes on
exactly the tables the limit targets. Check estimated_rows/3 first.

The one it could not verify — I checked, and it holds

It asked whether dev_docs/squash/generate_baseline.exs encodes the hand-applied
"DECLARED POST-GENERATION CORRECTIONS" that live in the generated files.

It does not. The phrase appears in expected_schema.ex and nowhere in the
generator. So the next regeneration — which release_check's own failure
message instructs an operator to perform — silently drops the
users_tokens.user_uuid nullability correction and both V164 index entries, and
chain_hash will still match after restamp. A baseline that loses a correction
while continuing to verify clean is the most expensive failure mode available
here, because it is inherited by every install created afterwards.


What it checked and found sound

Worth recording, so the findings above are read as a short list rather than a
verdict on the whole body of work.

The floor. Spot-checked against the documented old-chain end state with no
disagreement found: newsletters_broadcasts.list_uuid NOT NULL with its
RESTRICT FK and index; users_tokens.user_uuid nullable with the V64 CHECK
inline — the corrected shape, not the broken V56/V57 one; fk_comments_user_uuid
as SET NULL rather than V72's guessed CASCADE; preferred_locale correctly
absent and :legacy_optional in the manifest; both prefix-embedded naming
conventions reproduced; down/1 dropping no shared public functions and never
the schema.

Prefix safety. No violations in anything the squash rewrote — bare index
names on CREATE and qualified on DROP throughout V135/V163/V164/ShapeSql, every
existence probe anchored on table_schema/schemaname/nspname including the
referenced side, no regclass casts in any live immediate check, extensions
through the privilege-aware helper, schema creation through the guarded
ensure_schema!/2. The two remaining ::regclass uses are in UUIDFKColumns
mutation entry points that are dead code since V56/V57 were deleted — worth
deleting rather than leaving a live-data backfill loaded.

Data safety elsewhere. V164's NOT-NULL repair is zero-NULL-gated; FK
creation is NOT VALID + VALIDATE with orphans reported and never deleted; the
comments-FK drop/re-add window is NOT-VALID-covered; Repair.Executor is
additive-only by construction — no DROP or DELETE verbs, and its single UPDATE
targets IS NULL on a column the same call just added.

The happy upgrade path verified end to end: comment ≥ 135 → wrapper
vNNN→v164 with @disable_ddl_transactionplan_up{:run_delta, …}
uuid-function re-ensure → V161..V164 → stamp.

Verdict: NEEDS-WORK — the squash routing, baseline fidelity and prefix
discipline hold up under close reading. What does not is the edge: a
comment-less current database is advised toward a replay that deletes live rows,
and the manifest cannot report clean on two supported topologies.

ddon pushed a commit that referenced this pull request Aug 9, 2026
Post-merge review of the migration work (#688/#689 plus the two fix commits),
by Kimi K3 at max effort. Three of its findings, all verified against the tree.
`Common.query_version_directly/2` ended with `# Table exists but no version
comment - assume version 1`, so a database that is current but lost its comment
reported `{:current_version, 1}` — below the floor — and `phoenix_kit.update`
told the operator to install the 1.7.x bridge.
`Postgres.migrated_version/1` refuses that same state, in its own words: the
guess "would route a possibly CURRENT database to the 1.7.x bridge, whose
backfill overwrites still-NULL tracked columns with freshly generated uuids
pointing at nothing". On the bridge, `UUIDFKColumns.set_not_null/4` invents
uuids for legitimately-NULL `phoenix_kit_files` rows and
`cleanup_orphaned_fk_refs/5` then deletes them for matching no user.
So two halves of one release gave opposite instructions for one state, and the
destructive one was the one the operator met first. The sibling fallback in the
same module had already been hardened against the same guess — its comment
records that it "used to fall back to a fabricated {:current_version, 1}…
Report honestly instead" — but the fabrication survived one level down.
`:unknown_version` is now distinct from every real version and from "not
installed", and the update task routes it to doctor + restamp, matching what
the migrator says.
Two smaller findings from the same round:
- `Repair.Probe.read_comment/2` parsed the comment with `String.to_integer/1`
eleven lines below a docstring promising it never raises, so the exact
hand-edits the migrator documents (`'v164'`, `' 164'`) ended `mix
phoenix_kit.repair` in a bare `** (ArgumentError) argument error`. The tool an
operator reaches for when the comment is already anomalous now treats an
unparseable one as "no usable version", which its callers already handle. The
same unguarded parse in `Common` is fixed by the change above.
- The below-floor notice said "the last PhoenixKit 1.7.x release". The version
was already threaded into both `BelowFloorError` raise sites but not into the
notice shown FIRST, at generation time — the raises are only reached later.
`Postgres.bridge_version/0` exposes it so both name the same release.
compile --warnings-as-errors, format --check-formatted and credo --strict
(10290 mods/funs) clean.
ddon pushed a commit that referenced this pull request Aug 9, 2026
1.7.237 ships SIX PRs, not the four I had reviewed. #689 (the migration squash)
and #690 (security-p1 + the #689 review fixes) merged before this session and
had no CHANGELOG entry at all; #694 had none either.
#689 already had a CLAUDE_REVIEW.md from an earlier pass. Appended a second
pass rather than redoing it: its five mechanical findings are fixed (verified
in #690), blocker #6 is still open and still needs a database, and blocker #7
— the module ecosystem being unable to resolve 2.0 — is DISSOLVED by shipping
as a patch release. Re-verified all seven pins in /workspace: every one is
`~> 1.7.x`, which accepts 1.7.237 and rejects 2.0.0.
That matters more than it sounds, because the first pass recommended 2.0.0 and
I bumped to 1.7.237 before reading it. The override was accidental, so the
trade is now written down: 1.7.237 breaks no module but lets a below-floor host
be dragged across the floor by a routine `mix deps.update`; 2.0.0 prevents that
but makes `mix deps.get` unsatisfiable for every host running a feature module.
I think 1.7.237 is right — a refused migration with a precise remedy beats a
dependency resolver refusing to resolve — but only if the requirement is
impossible to miss, so the CHANGELOG now LEADS with it instead of listing it.
#690 reviewed clean; no new defects. Verified the pg_has_role fix (immediate
query, parameterized, absent function falls through to queue) and specifically
checked that the bridge_version fix reached every raise site — it does, because
the :ensure_current path re-raises the existing struct.
CHANGELOG now covers #689 through #694.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timujinne
timujinne deleted the squash-migrations branch August 10, 2026 08:35
ddon pushed a commit that referenced this pull request Aug 10, 2026
Tim's call: the release ships as 2.0.0, with the feature-module pins widened the
same day so the ecosystem lands with it. That removes the cost I had weighted
most heavily when I bumped to 1.7.237 — the unsatisfiable-dependency window is
coordinated away rather than endured — and it restores what the patch route
could not offer: `{:phoenix_kit, "~> 1.7"}` does not resolve to 2.0, so no
below-floor host is dragged across the floor by a routine `mix deps.update`.
That last point is why one paragraph had to be rewritten rather than kept. The
upgrade-requirement section warned that a routine deps.update WOULD carry a
below-floor host across; true of 1.7.237, false of 2.0.0. It now explains that
the major is precisely what prevents it, and carries the other half hosts need:
`~> 2.0` is unsatisfiable alongside any phoenix_kit_* package still pinning
`~> 1.7.x`, so modules must be upgraded together with core, not after it.
CHANGELOG now covers #689 through #697. The #695/#696 entries existed only in
the main working tree, which is dirty with another agent's in-flight work, so
they are re-created here rather than committed from there. #697's entries name
the two behaviour changes hosts will actually notice — the dev mailbox going
quiet by default, and /api/files/:uuid/info now requiring auth — and the
unauthenticated upload fix its own PR description never mentioned.
Written in an isolated worktree at origin/main so the other agent's uncommitted
CHANGELOG and lib/ changes are untouched; their entries stay in their tree and
will merge as ordinary changelog text.
Release gate at v2.0.0: CHANGELOG heading and body PASS, tag collision PASS.
Only the stale chain_hash remains, which that same agent is fixing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Aug 10, 2026
Two loose ends from committing the other agent's work.
The chain_hash they wrote was computed before #695 landed, and #695 edited
v163.ex (cond branch order) and v164.ex (a detection query's contype filter, a
wrapped probe). So the stamp was behind by exactly those two edits while the
manifest BODY — their hand-declared V165/V166 objects — was already correct.
Restamped over the 32 shipped files, which is legitimate here for the reason the
script requires: both #695 edits are guard, probe and message logic and add no
schema object, established in the #695 review. Recorded inline, including what a
restamp still does not assert.
release_check is now 5/6 and the two unit tests that had been red on main since
#692 pass again (test/mix/tasks/phoenix_kit_release_check_test.exs, 15 tests).
CHANGELOG: their three recheck entries re-applied on top of the 2.0.0 file, since
the copy in this tree predated the retitle and would have reverted it.
⚠️ Still outstanding, and not something a green gate should be read as covering:
the V165/V166 objects in the manifest have never been checked against a real
database. `verify.exs --scenario s7,s8` is what would do that, and #689's
equivalence evidence still predates HEAD. Both need the PostgreSQL that has not
been reachable in any of these passes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mdon added a commit to mdon/phoenix_kit that referenced this pull request Aug 10, 2026
The saved references were built at chain v163 and every run since has skipped
S1/S2 with `reference-stale` — so BeamLabEU#689's squash-equivalence evidence covered
neither V164's flush-order repair nor V165/V166. The harness was honest about
it; the evidence simply predated HEAD.
Regenerated from a worktree at the bridge commit (113dddb, the last
pre-squash checkout: @initial_version 1, 163 chain files) with V164/V165/V166
grafted on and @current_version bumped to 166, so the OLD chain runs all 166
versions individually and the dumps compare against the squashed chain on
equal footing.
That graft needed `uuid_fk_columns.ex` from HEAD as well — V164 drives its
repair off `fk_constraints/0` and `fk_constraint_name/2`, neither of which is
exposed in the bridge's copy. Worth noting what else rode along: HEAD's copy
also drops `{:phoenix_kit_users_tokens, "user_uuid"}` from the NOT NULL list,
which is a deliberate 2026-08-08 fix (magic-link registration tokens are
authorless by design, and the V135 baseline does not enforce it either).
Taking HEAD's copy is what keeps both sides of the comparison expressing the
same intent; keeping the bridge's would have manufactured a divergence that
is a known, documented fix rather than a squash defect.
Old chain at v166 (was v163): 1897 columns, 426 constraints, 601 indexes,
161 tables, 114 seeds.
S1 and S2 now PASS instead of skipping. Also unblocks S4: the below-floor
handoff schema is seeded from this same bridge worktree, so the BelowFloorError
guard that protects pre-2.0 upgrades is verified rather than skipped.
ddon pushed a commit that referenced this pull request Aug 10, 2026
Post-merge review of #700, which is billed as small docs work and contains two
commits that close release blockers: the hand-declared V165/V166 manifest
entries are corrected to Postgres' deparsed form (found by running verify
against a live PG 17.6 install, where s8 had been failing on exactly those four
wrong_shape findings), and the S1/S2 squash-equivalence references are
regenerated at chain v166, closing #689's blocker #6.
I verified the manifest diagnosis against Postgres' deparsing rules rather than
the commit message: the copied predicate came from an index whose status column
is varchar (v135.ex:8360, hence the (status)::text cast) while access_requests'
and comments' are text (v165.ex:112, v166.ex:56, hence no cast). chain_hash is
correctly untouched — it hashes v*.ex, not the manifest — and release_check
still passes.
One finding. #700's stated goal is zero dead links in the 2.0 hexdocs, and its
accounting is exactly right as far as it counts: 0 undefined references, and the
54 remaining "hidden" warnings are 28 module + 26 function, all deliberate. But
mix docs also emits two warnings of a third class it does not mention —
`references file "url"` — from a @doc that writes [links](url) as an
illustration of supported syntax. ExDoc reads it as a real link and publishes
<a href="url">, which 404s from the hexdocs page: a genuine dead link of exactly
the kind the commit set out to remove, which survived because it is a file
reference rather than an undefined one. Backticked, which also reads better
since the sentence is describing syntax.
mix docs is now 0 undefined, 0 broken file references, 54 hidden.
Two of the three caveats I had been repeating are closed by this PR: the
manifest body HAS now been checked against a live database, and the equivalence
evidence DOES now cover the current chain. Remaining: the module pin wave, and a
full mix test on a machine with PostgreSQL.
Review: dev_docs/pull_requests/2026/700-docs-references-and-squash-evidence/CLAUDE_REVIEW.md
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Aug 10, 2026
Max pushed back on my claim that the module pin wave must land before or with
the 2.0 publish, and he is right.
Publishing core first strands nobody, for the same reason this release is a
major. Measured rather than assumed: `~> 1.7`, `~> 1.7.189` and
`~> 1.7 and >= 1.7.211` all accept 1.7.237 and all reject 2.0.0. So a host on
`~> 1.7` plus any feature module cannot pull 2.0.0 even with
`mix deps.update --all`. The only host affected is one who deliberately moves
their own requirement to `~> 2.0` while a module still pins `~> 1.7.x`, and what
they get is a resolver error naming the conflict — opt-in and non-destructive,
not "stranded". I had contradicted my own argument two paragraphs earlier in the
CHANGELOG, where the major's whole justification is that `~> 1.7` does not
resolve to 2.0.
And core has to go first anyway: a module cannot be published with a bare
`{:phoenix_kit, "~> 2.0"}` until 2.0.0 is on Hex, because `mix hex.publish`
builds the package and `mix deps.get` cannot resolve a requirement no published
version satisfies. The only way to invert the order is an OR pin
("~> 1.7.231 or ~> 2.0"), which resolves against 1.7.x today. Testing the
modules against unpublished core is what PHOENIX_KIT_PATH / pk_dep/3 already
exist for.
CHANGELOG's module section reworded to say what it means for a host: staying on
`~> 1.7` changes nothing, and moving to `~> 2.0` waits on the modules you use.
The wrong sentence in the #689 first pass is annotated rather than rewritten,
since the 2.0.0 decision was reached partly by weighing a cost that turns out
not to exist.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@timujinne@ddon