Uh oh!
There was an error while loading. Please reload this page.
Fix the V165/V166 manifest entries, revive S21, and close the squash-equivalence evidence gap - #700
Merged
Conversation
…d form
The V165/V166 objects were the only manifest entries written by hand rather
than generated from a live database, and all four were recorded in source-SQL
form instead of the canonical form Postgres actually stores and reports:
phoenix_kit_mentions_kind_check
phoenix_kit_access_requests_status_check
phoenix_kit_comments_attribution_mode_check
phoenix_kit_access_requests_open_index (predicate)
The differ compares against pg_get_constraintdef/pg_get_indexdef output, so
`CHECK (kind IN ('user', 'resource'))` never matched the stored
`CHECK ((kind = ANY (ARRAY['user'::text, 'resource'::text])))`. Consequence:
`mix phoenix_kit.repair` and `mix phoenix_kit.doctor` reported four
error-severity divergences on a perfectly healthy database at V166 — false
alarms on every install, and noise that would train operators to ignore a
report that is supposed to mean something.
The index predicate shows where the mistake came from: it was copied from
phoenix_kit_org_invitations_pending_unique_idx, whose `status` column is
varchar, so Postgres emits `((status)::text = 'pending'::text)` there. The
access_requests `status` column is text, so the cast never appears. Same
predicate, different column type, different deparsed output.
Verified against a live PostgreSQL 17.6 install of the full V135..V166 chain:
verify.exs s7 (tamper matrix) and s8 (repair idempotence) now pass, where s8
previously failed with exactly these four wrong_shape findings. No migration
file changes, so chain_hash is untouched.s21 computed the migration under test as `current_version() - 1 .. current_version()`, which was correct only while V164 was the head of the chain. Once V165 and V166 landed, the delta became 165..166 and V164 never ran — so the scenario simulated the V56/V57 flush-order defect, skipped the migration that repairs it, and then reported ~70 unenforced foreign keys and ~44 nullable columns as a product defect. Pinned to @s21_repair_version instead: the delta under test is a property of the migration being exercised, not of wherever the chain happens to end. With this, V164's repair is verified against a live database for the first time: it restores every FK and NOT NULL, is byte-identical on a second pass, and leaves an orphan-blocked FK NOT VALID with the operator warning intact.
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.
… the README an upgrade section
The upgrade guide named `~> 1.7.235` as the bridge; the tagged bridge — and
the version `@bridge_version` in postgres.ex names in the refusal message — is
1.7.236. Two further staleness spots in the same guide predated V165/V166: the
chain was described as ending at `V164`, and the healthy-install FK count was
labelled as measured against a `V135..V164` build. Re-measured against a fresh
V135..V166 build: still 86, so only the label was wrong.
The README was a bigger gap. It told every reader to install
`{:phoenix_kit, "~> 1.7"}` — which does not resolve to 2.0 at all — and said
nothing anywhere about upgrading. 2.0 refuses to migrate a database below
V135, so an existing user who bumped the pin would have met a BelowFloorError
with no warning in the one document they were most likely to read. Added an
"Upgrading to 2.0" section covering the floor, the bridge pin, the
`mix phoenix_kit.update` step, and the comments-FK change from ON DELETE
CASCADE to SET NULL, linking to the full guide.
Also re-ran the two gates that were not re-run in the last pass: `mix dialyzer`
passes (225 errors, all skipped by the ignore file) and `mix docs` builds
clean. Both green.`mix docs` emitted 14 "undefined or private" warnings across seven unique references, all of which would have shipped as dead links in the 2.0 hexdocs. Six were reference-syntax problems rather than wrong facts: - `PhoenixKit.Integrations.owner/0` and `PhoenixKit.Module.permission_meta/0` are types, so they need the `t:` prefix. - `PhoenixKit.Module.email_settings_sections/0` is a callback, so it needs `c:`. - `Registry.get_tabs/1` / `get_admin_tabs/1` were written as the local alias. ExDoc resolves a bare `Registry` to Elixir's stdlib module, which has neither function; both now name `PhoenixKit.Dashboard.Registry` in full. - `PhoenixKit.Migrations.Repair.delegate_oban/2` is genuinely private, so the arity is dropped — the prose still names it, without asking ExDoc to link it. The seventh was a real error: `Ecto.Adapters.SQL.checkout/2` does not exist at that arity, and `checkout/3` — which does — is `@doc false`, so correcting only the arity traded an undefined warning for a hidden one. The code actually calls `repo.checkout(fn -> ... end)`, so the reference is now `c:Ecto.Repo.checkout/2`, which is both accurate and public. Docs go from 14 undefined references to 0. The 54 remaining "hidden" warnings are unchanged and are deliberate: internal modules carrying `@moduledoc false` referenced from other internal docs.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the verification gap that every reviewer kept flagging before the 2.0 release — and it was not only a gap. Running the checks found two real defects.
The manifest was wrong on a healthy database
The V165/V166 objects are the only manifest entries written by hand rather than generated from a live DB, and all four were recorded in source-SQL form instead of the canonical form Postgres stores:
The differ compares against
pg_get_constraintdef/pg_get_indexdef, so these could never match.mix phoenix_kit.repairandmix phoenix_kit.doctorreported four error-severity divergences on every healthy V166 install — false alarms that train operators to ignore a report meant to mean something.The index predicate shows the origin: it was copied from
phoenix_kit_org_invitations_pending_unique_idx, whosestatuscolumn isvarchar(so PG emits((status)::text = 'pending'::text)).access_requests.statusistext, where that cast never appears. Same predicate, different column type, different deparsed output — verified against both columns in a live DB.No migration files changed, so
chain_hashis untouched.S21 had silently stopped testing V164
s21derived the migration under test ascurrent_version() - 1 .. current_version(). That was correct only while V164 was the head. Once V165/V166 landed the delta became 165→166, so the scenario simulated the V56/V57 flush-order defect, skipped the migration that repairs it, and then reported ~70 unenforced foreign keys and ~44 nullable columns as a product defect.Pinned to
@s21_repair_version— the delta under test is a property of the migration, not of wherever the chain ends. V164's repair is now verified against a live database for the first time: restores every FK and NOT NULL, byte-identical on a second pass, orphan-blocked FK correctly leftNOT VALID.S1/S2 equivalence evidence now matches HEAD
The saved references were built at chain v163, so every run since skipped S1/S2 as
reference-stale— #689's squash-equivalence evidence covered neither V164 nor V165/V166.Regenerated from a
git worktreeat the bridge commit (113dddbf, the last pre-squash checkout) with V164–V166 grafted on and@current_versionbumped to 166, so the old chain runs all 166 versions individually.That graft also needed
uuid_fk_columns.exfrom HEAD — V164 drives its repair offfk_constraints/0, which the bridge copy does not expose. Worth a reviewer's eye: HEAD's copy also drops{:phoenix_kit_users_tokens, "user_uuid"}from the NOT NULL list, a deliberate 2026-08-08 fix (magic-link registration tokens are authorless by design; the V135 baseline does not enforce it either). Taking HEAD's copy keeps both sides of the comparison expressing the same intent — keeping the bridge's would have manufactured a "divergence" that is a documented fix rather than a squash defect.This also unblocks S4: the below-floor handoff schema seeds from the same worktree, so the
BelowFloorErrorguard protecting pre-2.0 upgrades is verified rather than skipped.Docs
~> 1.7.235as the bridge; the tagged bridge, and the version@bridge_versionnames in the refusal message, is 1.7.236. Two further stale spots predated V165/V166: the chain was described as ending at V164, and the healthy-install FK count was labelled as measured against aV135..V164build. Re-measured against a fresh V135..V166 build: still 86, so only the label was wrong.{:phoenix_kit, "~> 1.7"}— which does not resolve to 2.0 — and said nothing anywhere about upgrading. Since 2.0 refuses to migrate a database below V135, an existing user bumping the pin would have met aBelowFloorErrorwith no warning in the document they were most likely to read. Added an "Upgrading to 2.0" section covering the floor, the bridge pin, themix phoenix_kit.updatestep, and the comments-FK change fromON DELETE CASCADEtoSET NULL.t:/c:prefixes, a bareRegistryalias resolving to Elixir's stdlib, and one genuine error whereEcto.Adapters.SQL.checkout/2does not exist andcheckout/3is@doc false(the code callsrepo.checkout/2).Verification
Full scenario harness against live PostgreSQL 17.6, from a clean slate (fresh DB, worktree removed):
22 PASS — s1, s2, s1_self, s2_self, s3, s4, s5, s6, s7, s8, s8_pre, s9, s10, s11, s11_pre, s13, s15, s17, s19, s20, s21, s22
4 SKIP —
s4_seed(bridge-side half of a two-checkout handoff, by design),s12(needs a pooled endpoint),s16(P2 body unwritten),s18(needs a manual trigger)Also green:
mix precommit·mix dialyzer·mix docs· release check (current_version == v166.ex, floor == V135, V135..V166 contiguous, chain_hash matches 32 on-disk files).Pre-existing and unrelated: the full suite shows one flaky failure per run, a different test each time, all passing in isolation — shared global settings state across async tests.