Skip to content

Close the remaining review findings from the #694 round - #695

Merged
ddon merged 1 commit into
BeamLabEU:mainfrom
timujinne:fix/review-round-remainder
Aug 9, 2026
Merged

Close the remaining review findings from the #694 round#695
ddon merged 1 commit into
BeamLabEU:mainfrom
timujinne:fix/review-round-remainder

Conversation

@timujinne

Copy link
Copy Markdown
Contributor

#694 merged at its third commit; the work after that never reached main. This is
that remainder, rebuilt onto current main.

Dropped deliberately: my version of the comment-less guard. 80fc6784
replaced it with a better one — mine required a positive "there is no comment"
and fell through to the destructive branch on any failure to answer, while
comment_literally_says_one?/1 requires a positive "the comment reads 1" and
treats everything else as unknown. That is the right direction and it is already
in main.

Still live on main

mix phoenix_kit.status --verbose crashes. The non-verbose path reports the
unreadable-comment state correctly; show_installation_diagnostics/2 has no
clause for it, so the run dies with CaseClauseError right after printing a
correct tree. This is the clause a previous commit message claimed and did not
contain.

Introduced by #694 and fixed here

A flaky FK probe aborted the entire chain run. 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 equivalent, so one failed catalog read on one
of ~70 constraints aborted mid-run — earlier repairs already auto-committed under
@disable_ddl_transaction, COMMENT … IS '164' never reached, whole version
replays next time. A probe failure is now a {:failed, …} entry in the summary.

Pre-existing, found by the same review

  • comments_fk_on_delete/1 matched by name alone. A CHECK, or an FK on a
    different column, owning fk_comments_user_uuid answered for the real one — a
    confdeltype of 'n' from an impostor reads as "already SET NULL", so
    repair_comments_fk/2 returns [] and the genuinely missing FK is never
    created. Now anchored on contype, arity and column. fk_validated?/3 stays
    name-only and now documents 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 said the comment was "missing" while the
    table held v164, and --adopt stamped the floor over it — destroying the only
    record of what the last operator believed, in the state where that belief is the
    most useful thing available. A fourth value, :unparseable, maps to its own
    :comment_unreadable branch that refuses rather than adopting.
  • estimated_rows/3 reported a never-analyzed table as 0 rows. PostgreSQL
    ≥ 14 leaves reltuples = -1 until the first vacuum or analyze — pg_restore,
    CREATE TABLE AS, logical replication, autovacuum off. Measured: 200k rows read
    as zero, so the size guard passed and V163 would take ACCESS EXCLUSIVE for a
    full rewrite on exactly the table the limit exists to protect. It now returns
    :unknown, V163 defers on it, and mix phoenix_kit.repair_uuid prints
    "size unknown — never analyzed" instead of "~0 rows" to whoever is sizing the
    maintenance window.
  • V163's deferral text named a remedy that cannot work on a table that is
    also uncastable — since the reorder, such a table takes that branch.

Verification

test/phoenix_kit — 1153 tests, 38 doctests, 0 failures (359 excluded: the
integration half, no database reachable). mix format --check-formatted,
mix compile --warnings-as-errors, mix credo --strict (10297 mods/funs) clean.

⚠️mix dialyzer halts on this branch, on two LocaleSlug warnings in
lib/phoenix_kit/utils/slug.ex — a file this branch does not touch, with no
entry in .dialyzer_ignore.exs. Pre-existing, so mix quality / precommit
fail here for a reason unrelated to this PR; worth someone's attention
separately.

@version and CHANGELOG.md untouched.

The previous branch was merged as BeamLabEU#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.
@timujinne
timujinne marked this pull request as draft August 9, 2026 21:48
@timujinne
timujinne marked this pull request as ready for review August 9, 2026 22:43
@ddon
ddon merged commit fe3bb9e into BeamLabEU:mainAug 9, 2026
ddon pushed a commit that referenced this pull request Aug 9, 2026
Both clean — no new defects. Verified the parts this series has repeatedly got
wrong rather than the parts the diffs touch:
#695 introduces two new return values and both propagate completely.
estimated_rows/3's :unknown is handled by an explicit pattern match in V163
(not by term ordering, which would have made it correct by accident) and named
out loud by repair_uuid. Probe's :unparseable reaches classify/3, Repair.run,
error_message/1, the error type, and the JSON error_tag — traced each. The '0'
behaviour change it implies is unreachable: nothing stamps IS '0'.
#696's "two siblings" claim is exhaustive: no `_system ->` left in auth.ex, and
delete_user was already fail-closed by a different mechanism
(validate_can_delete_user/2's fallback refuses even nil).
It also corrects a test I added in the #692 pass, and the correction is worse
than its commit message claims. My version poisoned all five identity fields in
one payload; "attacker-user_timezone" fails validate_user_timezone/1, one
invalid member invalidates the whole profile_changeset, so nothing was written
either way. It passed with the filter DELETED and failed with the filter
working — inverted, not merely vacuous, and red on main since I added it. The
exact defect I spent three reviews naming in other people's tests, in a test I
could not run. Written up in full in the #696 review.
CHANGELOG deliberately NOT committed: another agent is mid-edit in this tree
(implementing the mentions N+1 and access-request findings from my #692 review,
and hand-declaring V165/V166 in the manifest), and CHANGELOG.md now holds their
in-flight entries alongside mine. Committing it would separate their notes from
their code.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timujinne

Copy link
Copy Markdown
ContributorAuthor

Post-merge review — chain #689#694#695#696 (two-track ultra-review)

Two independent review tracks (3-pass A: broad → deepen → verify/narrow; 2-pass B: broad → deepen), each on the default model, then reconciled. Both tracks read files at chain HEAD 855c17f9 and re-derived the load-bearing claims against source. No runtime execution — no Elixir/PG available to either track, so closure claims rest on code-reading + Ecto source inspection.

Headline: the two tracks are strongly convergent, not contradictory. They agree on every cross-cutting verdict. One HIGH-severity finding raised in A1 (mentions "drops every @-ping") was refuted by both tracks against Ecto source. The one finding worth acting on before release is MEDIUM, B-only.


✅ Verified CLOSED — chain's headline claims hold

#695 closes all six #694-round findings (full A/B consensus, re-derived end-to-end):

  1. read_comment/2 nil/corrupt conflation → probe.ex:94 returns :unparseable; comment_policy.classify(:unparseable,...) -> :comment_unreadable; --adopt cannot stamp over it (routes to error, never to run_adopt_branch).
  2. estimated_rows collapsed reltuples = -1 to 0uuid_integrity returns :unknown, guarded.
  3. status --verbose crash on {:unknown_version} → handled.
  4. V164 post-ADD probe raised mid-run → wrapped in try/rescue.
  5. comments_fk_on_delete read by name alone → anchored on contype='f' + arity + column.
  6. V163 deferral text → fixed.

No seventh finding dropped. #694's own BUG-HIGH items were fixed in the #694 pass (common.ex:200), correctly outside #695's scope.

#696 closes both F-2 and F-7 from the #691 round, no same-series sibling left fail-open. Both tracks audited every actor-gated write in auth.ex: update_user_status/3, toggle_user_confirmation/2 now have the three-branch shape; admin_update_user_password/3 already fail-closed; delete_user/2 fail-closed by a different mechanism. git grep "_system ->" auth.ex → nothing. No sibling left open.


🔴 B-#1 · MEDIUM · Differ NOT-NULL masking defeats --adopt's clean gate

differ.ex:222reason_not_null(reasons, %{not_null: true, default: nil}, _observed) short-circuits and never compares _observed. Three sharpenings Track B alone made:

  1. Blast radius is the dominant class, not a corner case: 632 manifest shapes match %{not_null:true, default:nil}, 170 of them type:"uuid" — the V56/V57 flush-fallout columns V164 exists to repair.
  2. --adopt is defeated:floor_verify_clean?/1 (comment_policy.ex:213) cannot see the suppressed :wrong_shape, so a comment-less DB whose *_uuid PKs/FKs are nullable passes the clean gate and gets the floor stamped over unconverged drift. This raises it from "verify under-reports" to "the adopt contract can be silently violated."
  3. Masks a known manifest/migration disagreement:changed_by_uuid is deliberately nullable in V164 (ON DELETE SET NULL) but the manifest says NOT NULL (expected_schema.ex:29786) — the exclusion hides the disagreement rather than surfacing it.

Fix: in differ.ex:222 and/or a "just-created" gate in repair.ex:587presence_present_finding. The --adopt sharpening is the release-impacting part. Also resolve the changed_by_uuid manifest/migration disagreement so the differ stops hiding it.


Next (low-effort hardening, batchable)

  • B-N1 · LOWpostgres.ex:619 plan_up/3 + :704,:721 parsers lack the n > 0 guard that Probe.read_comment/2 (probe.ex:94) enforces. A hand-edited comment '-5'{:run_delta, -4..166}Module.concat([Postgres, "V-4"]) → opaque UndefinedFunctionError before any DDL. Status path is safe (current_version > 0 guard). Notably probe.ex:90-92 already documents this exact risk on the path it guards.
  • B-N2 / A-N6 · LOWmentions.ex:771-776claim_for_delivery: drop returning: falseand collapse the case to {count, claimed} when is_list(claimed) so the silent-[] trap cannot exist under a future Ecto precedence change. Correct today; latent.
  • A-N1 · LOWv164.ex:827: literal 'user_uuid' should be '#{@comments_fk_column}' for consistency with the rest of the query. Identical value today, no functional bug.
  • B-Updated the comments in users_live html file, and added Modules link in dashboard. #2 · LOWv164.ex:654-655: FK-retry summary says "re-running does not retry them" — accurate for FK constraints but the NOT-NULL path does retry. One-line scoping fix.

Optional (cleanup / docs / test gaps)

Watch (open by design — flag at release time)

  • pgcrypto non-public schema:verify/1 shows permanent spurious :wrong_shape on the uuid function; repair converges via the helper. Noisy, not blocking. Confirm operator runbook note exists.
  • idx_publishing_posts_group_slug predicate contradiction (manifest revs 59/62 predicate: nil vs rev 164 predicate: "(slug IS NOT NULL)"): self-heals after V164; only affects pre-V164 DBs at version 59-163.
  • Stale chain_hash blocker: 2 failures in phoenix_kit_release_check_test.exs since Add cross-module mentions, one canonical display name, and stop publishing email addresses #692predates this chain, not closed by it. Track separately.

Cross-cutting verdict

Did #695 really close all #694-round findings? Yes — all six, high confidence. Two findings #694 deliberately left open (pgcrypto, idx predicate) are open by design, both tracks agree. No finding was dropped or silently partial.

The residual risk is operational, not logical: B-#1 (Differ NOT-NULL masking) is a separate, pre-existing verify blindness that #695 did not address and was not asked to address — but it means that on *_uuid columns, verify/1 (and therefore --adopt's clean gate) can report convergence that hasn't happened. That does not contradict "#695 closed its six findings"; it is a distinct gap surfaced by this review for the release decision.

Full artifact trace (prep, both tracks' iterations, synthesis) in internal review directory. Method: two-track ultra-review, both tracks on the default model, reconciled by a fresh synthesizer.

@timujinne
timujinne deleted the fix/review-round-remainder 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
…6 manifest
Not my work — another agent's changes, uncommitted in the working tree while I
was reviewing #695/#696/#697. Committing at the maintainer's request so nothing
is stranded. Verified it compiles with warnings-as-errors before committing.
Three pieces, all closing findings from my earlier review passes:
- Mentions.Users.search/2 no longer calls Scope.for_user/1 per candidate; the
admin-area rule became two SQL EXISTS subqueries with the limit applied in the
database. This is the ~128-queries-per-keystroke item I reported in the #692
review and deliberately did not fix, because rewriting an authorization filter
as hand-written SQL without a database to test against was not a change to
make blind. It has a database now.
- AccessRequests.request/4 validates the resource type against
ResourceLinks.handlers/0, requires a castable uuid, and rate-limits per
account — the unvalidated/unthrottled path I filed as a nitpick on the same PR.
- ExpectedSchema hand-declares V165/V166 objects and restamps chain_hash, which
is the release blocker. Their choice of the hand-declare route matches what the
#692 review concluded was necessary: no generator run can emit V164+ objects,
because regeneration replays a pre-squash checkout whose chain ends at V163.
CHANGELOG.md is deliberately NOT in this commit. The copy in this tree predates
the 2.0.0 retitle and the #695-#697 entries now on main, so committing it would
have reverted them. Their three entries were extracted verbatim first and are
re-applied on top of the current file in the next commit.
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>
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