Stop reporting a comment-less database as version 1 by timujinne · Pull Request #694 · BeamLabEU/phoenix_kit

timujinne · 2026-08-09T20:16:41Z

Three findings from the post-merge review of the migration work, published as a
comment on #689. All verified against the tree before this branch was cut.

The one that matters

Common.query_version_directly/2 ended with, verbatim:

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

A database that is current but lost its comment — the half-installed or
adopted state repair --adopt exists for — therefore reported
{:current_version, 1}, which is below the floor, so mix phoenix_kit.update
answered: install the 1.7.x bridge first.

Postgres.migrated_version/1 refuses that same input, and says why:

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

Downstream that is not abstract: UUIDFKColumns.set_not_null/4 backfills
legitimately-NULL phoenix_kit_files rows with invented uuids, and
cleanup_orphaned_fk_refs/5 deletes them for matching no user.

Two halves of one release gave opposite instructions for one state, and the
operator met the destructive one first. Worth noting the sibling fallback in the
same module was already hardened against exactly this guess — its comment
records that it "used to fall back to a fabricated {:current_version, 1}…
Report honestly instead". The fabrication survived one level down.

:unknown_version is now distinct from both a real version and "not installed",
and the update task routes it to doctor + restamp.

Also fixed

Repair.Probe.read_comment/2 raised on the anomaly it exists to diagnose.
String.to_integer/1, eleven lines below a docstring that says "Never raises".
The exact hand-edits the migrator documents ('v164', ' 164') ended
mix phoenix_kit.repair in a bare ** (ArgumentError) argument error, while
doctor survived only because its check wraps everything in rescue.
The below-floor notice did not name the bridge.@bridge_version was
threaded into both BelowFloorError raise sites but not into the notice shown
first, at generation time — the raises come later. bridge_version/0 exposes
it so the two agree.

Not in this PR

Two findings from the same round need a decision rather than a patch, so they
are left for the maintainer:

The manifest pins uuid_generate_v7's body with public.gen_random_bytes, so
repair can never converge where pgcrypto lives in another schema — a
supported topology. Wants the pgcrypto schema normalised out of prosrc
before hashing on both sides, like the array-cast canonicalisation in
Differ.
The manifest's pre-164 revisions for idx_publishing_posts_group_slug carry
predicate: nil, next to a comment saying "Every real install has the
predicate". Both cannot hold: a public install at 135..163 gets :wrong_shape
and exit 2 on a byte-correct database. Wants :legacy_optional or a bimodal
revision.

And one that is already yours: the review also flagged that
generate_baseline.exs does not encode the hand-applied post-generation
corrections. 90077b79 answers that better than the finding did — the generator
cannot emit V164+ objects, because regeneration runs from a pre-squash
checkout whose chain ends at V163. Recorded here only so the thread is complete.

Verification

mix compile --warnings-as-errors, mix format --check-formatted and
mix credo --strict (10290 mods/funs) — clean. No test run: these paths are
exercised by mix phoenix_kit.update against a real database rather than by the
suite, and the shared PostgreSQL has been at its connection ceiling all day.
@version and CHANGELOG.md untouched.

Post-merge review of the migration work (BeamLabEU#688/BeamLabEU#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.

…first The two findings from the same review round that I dropped from the first commit without saying so. V164's `fk_exists?/3` matched a constraint on `conname + relname + nspname` and nothing else. Detection before the ADD is shape-based, so a constraint that merely owns the name — a CHECK, or an FK to a different column — reads as `:absent`; the guarded ADD then fails with 42710 and is swallowed by the `EXCEPTION WHEN OTHERS` handler; the verification sees the colliding constraint, `validate_fk/7` issues a no-op VALIDATE against it, and the outcome is reported `:created`. One buried RAISE WARNING, no `{:failed, …}`, no SUMMARY line, comment stamped 164, declared foreign key absent. `contype = 'f'` closes it. This is the same shape as the defects V164 exists to repair, inside the repair itself. V163's `repair/3` asked `castable?/3` — a full-table `count(*) … !~*` scan — before the row-count guard, so the deferral path paid an unbounded sequential scan on exactly the tables the limit exists to keep out of `mix ecto.migrate`. Read-only, so not the ACCESS EXCLUSIVE outage the guard is about, but on a PgBouncer-fronted pool it pins a connection for minutes before deciding to skip. The catalog read goes first now. The reordering does change which message an oversized AND uncastable table gets: the size warning rather than the cast error. That is the right way round — the size branch tells the operator to run `mix phoenix_kit.repair_uuid` in a maintenance window, where the cast failure surfaces with the table in hand, and `doctor` keeps reporting it either way. compile --warnings-as-errors, format --check-formatted and credo --strict (10290 mods/funs) clean.

timujinne · 2026-08-09T20:21:51Z

Two more findings from the same review round pushed to this branch — I had
dropped them from the first commit without listing them anywhere, so the
"Not in this PR" section above was incomplete.

fk_exists?/3 verified a foreign key by name only. Detection before the
ADD is shape-based, so a constraint that merely owns the name reads as
:absent; the ADD then fails 42710 into the EXCEPTION WHEN OTHERS handler,
the name-only check sees the colliding constraint, and the outcome is reported
:created — comment stamped 164, declared FK absent. contype = 'f' closes it.
V163 scanned before it checked the size guard.castable?/3 is a
full-table count(*) … !~*; asking it before estimated_rows/3 made the
deferral path pay an unbounded sequential scan on exactly the tables the limit
targets.

The reorder changes which message an oversized and uncastable table gets —
the size warning instead of the cast error. Deliberate: that branch points at
mix phoenix_kit.repair_uuid in a maintenance window, where the cast failure
surfaces with the table in hand.

Still not in this PR, unchanged from the description: the pgcrypto-schema
normalisation in the manifest's uuid_generate_v7 body, and the predicate: nil
revisions for idx_publishing_posts_group_slug below 164. Both want a decision
about manifest semantics rather than a patch from me.

…troduced Review of this PR by GLM-5.2 at max effort. Both findings are mine, and the first one means the PR's headline fix did nothing. `check_installation_status/1` calls `Postgres.migrated_version_runtime/1` first and only consults the fallback when that returns <= 0. For a comment-less table the runtime twin returns **1**, so `query_version_directly/2` — the function the first commit rewrote — was never reached, `{:current_version, 1}` still escaped, and `mix phoenix_kit.update` still answered "install the 1.7.x bridge first". The `{:unknown_version}` clause added to the update task was dead for the very state it was written for. I verified the fallback and never checked which twin runs first; the fix landed on the wrong one. The runtime twin's guess is correct for what it is — a DISPLAY path that must keep `status`, `doctor` and the admin UI rendering, and it warns loudly. Its own warning is the specification: "`mix phoenix_kit.update` will REFUSE this database until the comment is restamped." It did not refuse. `check_installation_status/1` now re-asks the strict reader when, and only when, the runtime value is exactly 1, so "the comment says 1" stays distinct from "there is no comment". Second: `:unknown_version` IS reachable today for a present-but-unparseable comment, and it crashed `mix phoenix_kit.status` — `StatusReport.next_action/3` had no clause for it, in a module I never opened. That is a regression this PR introduced: before it, the same input was swallowed by a `rescue` into 0 and the task printed "Not installed". Wrong, but it printed. It now reports the state and asks for a restamp, and `show_installation_diagnostics/2` handles it under `--verbose` rather than raising one step later. `StatusReport.next_action({:unknown_version}, [], "public")` is now a test — the one line GLM pointed out would have caught this instantly. Verified it goes red on revert: removing the clause fails 2 tests with FunctionClauseError. test/phoenix_kit/install — 86 tests, 6 doctests, 0 failures, nothing excluded (unit tests, no database needed). compile --warnings-as-errors, format --check-formatted and credo --strict (10293 mods/funs) clean.

@doc

Post-merge review of #694. The headline fix is real and the PR self-corrected two significant defects before merge (the first commit fixed the wrong twin; the new atom crashed status through a clause the author had not extended) — both documented plainly in c661f1b. Two gaps left, one of them red on main. - StatusReport's `action()` type never gained `{:fix_version_comment, String.t()}`. next_action/3 is @SPEC'd to return `action()`, so dialyzer proved the new `format_next_action({:fix_version_comment, _})` clause unreachable and exited 2. `mix precommit` therefore fails on main as merged; every later PR inherits a red gate. The PR's stated verification covers compile, format and credo — three of the four steps precommit runs, and dialyzer is the one that catches this. - The @doc return lists for check_installation_status/1 and check_update_needed/2 both still enumerated the old states. That list is the contract callers read to decide which clauses to write, and this PR already paid for it being wrong once. Verified rather than assumed: all three consumers do handle the new state; repair_uuid does check castability, so V163's reordered deferral is not a dead end; the three version-comment parsers agree except on non-positive integers, where the new one is the stricter; 86 tests + 6 doctests pass. The mentions/users.ex dialyzer ignore #692 added is load-bearing — removing it goes red. Note for the release: #694's v163/v164 edits are guard-ordering and detection shape only, so they stay restamp-safe. V165/V166 remain the only versions that add manifest objects. Review: dev_docs/pull_requests/2026/694-stop-reporting-comment-less-database-as-version-1/CLAUDE_REVIEW.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

I created a second directory for #694 without checking for an existing one. Moved CLAUDE_REVIEW.md alongside Pincer's phase1.md, which was there first, matching how #692's two halves sit together. phase1.md is not my work — committing it so both halves are on record and the tree stays clean for the release gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Second finding from the #694 review, and the more consequential one. The guard tested for a specific answer: current_version == 1 and try_direct_database_version_check(opts) == :unknown_version so it fired only when the re-check positively reported "there is no comment". Every way of FAILING to answer fell through to {:current_version, 1} — below the floor — and the operator was told to install the 1.7.x bridge, which is the destructive advice the whole PR exists to prevent. Reachable, not theoretical: the two halves of the cond reach the database by different routes. migrated_version_runtime/1, which produces the 1, resolves the repo through get_repo_with_fallback/0 — config, then start the app, then auto-detect from the Mix project. try_direct_database_version_check/1 uses Config.get(:repo, nil) alone. On a host that never sets `config :phoenix_kit, repo:` and relies on that auto-detection, the first finds the repo and the second does not, so the guard silently did not fire on exactly the state it was written for. Same shape as the twin mixup c661f1b corrected, one level down. Now requires an affirmative "the comment reads 1" to report version 1. Fail closed, because the errors are asymmetric: a genuine V01 install sent to doctor loses a minute; a current database sent to the bridge gets invented uuids backfilled into still-NULL tracked columns and the rows deleted for matching no user. No test — check_installation_status/1 needs a live database and none is reachable here. precommit green; 86 tests + 6 doctests still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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>

The previous branch was merged as #694 at its third commit; everything after that never reached main. This is that remainder, re-applied onto current main — with my version of the comment-less guard dropped, because `80fc6784` replaced it with a better one. Mine required a positive "there is no comment" and so fell through to the destructive branch on any failure to answer; the maintainer's `comment_literally_says_one?/1` requires a positive "the comment reads 1" and treats everything else as unknown. That is the correct direction and it is already in main. **`mix phoenix_kit.status --verbose` still crashes on main.** The non-verbose path reports the unreadable-comment state correctly; `show_installation_diagnostics/2` has no clause for it, so the same run dies with CaseClauseError immediately after printing a correct tree. The clause I claimed to have added two commits ago and had not. **A flaky FK probe aborted the whole chain.** Moving V164's post-ADD check to `fk_shape_present/5` was right for correctness and wrong for failure mode: that probe raises, while the `fk_exists?/3` it replaced returned false. V164 has no `rescue` and no `run_isolated/3`, so one failed catalog read on one of ~70 constraints aborted mid-run with earlier repairs already auto-committed and the `COMMENT … IS '164'` never reached. Wrapped; a probe failure is now a `{:failed, …}` line in the summary. **`comments_fk_on_delete/1` read a constraint by name alone**, so a CHECK or an FK on a different column owning `fk_comments_user_uuid` answered for the real one — `confdeltype` 'n' from an impostor reads as "already SET NULL", and the genuinely missing FK is never created. Now anchored on type, arity and column. `fk_validated?/3` stays name-only and says why: every caller reaches it through the shape gate. **`Repair.Probe` collapsed a corrupt comment into "absent".** Both returned `nil`, so `adopt_required_message/0` told the operator the comment was "missing" while the table said `v164`, and `--adopt` stamped the floor over it. A fourth value, `:unparseable`, now maps to its own `:comment_unreadable` branch that refuses instead of adopting. **`estimated_rows/3` reported a never-analyzed table as 0 rows** — PostgreSQL >= 14's `reltuples = -1`, measured at 200k rows read as zero. It returns `:unknown`, V163 defers on it, and `repair_uuid` prints "size unknown — never analyzed" rather than "~0 rows" to whoever is sizing a maintenance window. V163's deferral text also no longer names a remedy that cannot work on a table whose values are not castable. test/phoenix_kit — 1153 tests, 38 doctests, 0 failures (359 excluded: the integration half). format --check-formatted, compile --warnings-as-errors, credo --strict (10297 mods/funs) clean.

Close the remaining review findings from the #694 round

timujinne added 2 commits August 9, 2026 23:16

ddon merged commit e9dc1ab into BeamLabEU:mainAug 9, 2026

timujinne mentioned this pull request Aug 9, 2026
Close the remaining review findings from the #694 round #695
Merged

ddon added a commit that referenced this pull request Aug 9, 2026
Merge pull request#695from timujinne/fix/review-round-remainder
fe3bb9e
Close the remaining review findings from the #694 round

Navigation Menu

Uh oh!

Stop reporting a comment-less database as version 1 - #694

Stop reporting a comment-less database as version 1#694
ddon merged 3 commits into
BeamLabEU:mainfrom
timujinne:fix/migration-review-findings

timujinne commented Aug 9, 2026

Uh oh!

timujinne commented Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

timujinne commented Aug 9, 2026

The one that matters

Also fixed

Not in this PR

Verification

Uh oh!

timujinne commented Aug 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants