Skip to content

Review request: phoenix_kit_consent_logs ownership — three DDLs for one table, resolved in 0.3.0/0.3.1 #13

Description

@ddon

@timujinne — you wrote the original Legal module (801e66ca, "Add Legal Module Phase 1 with V43 migration") and PR #8, so flagging this for your review. Changes are already released (0.3.0, 0.3.1) because they were blocking the phoenix_kit 2.0 ecosystem sweep, but the reasoning deserves a second pair of eyes — especially the PR #8 point below.

Full writeup, with the evidence and the commands to re-derive it:
dev_docs/reports/2026-08-10-module-migration-versioning.md

The finding

phoenix_kit_consent_logs is a core table, and this package had grown two more definitions of it:

# Where Ran? Shape
1 core V43 → squashed into V135 yes, every install canonical
2 lib/phoenix_kit_legal/migrations/consent_logs.ex never varchar(255/50/50), own index names
3 priv/migrations/add_phoenix_kit_consent_logs.exs only if you followed the README all varchar(255), no if_not_exists

Core V43 created the table in the same commit that first added Legal — back when Legal was part of core. When Legal was extracted, core kept V43. Copies #2 and #3 were written fresh rather than copied from it, which is why they drifted. Core 2.0's ExpectedSchema now names the table, all 11 columns, 6 indexes and the pkey as core-owned, and mix phoenix_kit.doctor verifies against it.

The bit I'd most like you to check

PR #8 was correct, but its stated justification wasn't — and it had a side effect. The review says:

"On a clean install this meant phoenix_kit_consent_logs was never created."

Core's V43 had been creating the table for five months by then, so that can't have been the consequence. What PR #8 actually changed is that the coordinator went from never invoked (core's try/rescue swallowed the missing current_version/0) to invoked — making down/1 reachable for the first time:

execute("DROP TABLE IF EXISTS #{prefix_str}phoenix_kit_consent_logs CASCADE")

against a core-owned table that outlives this package. Nothing was lost, because nothing rolled Legal back — but it was one mix ecto.rollback from taking the consent audit trail with it. This is exactly the trap phoenix_kit_hello_world#34 documents: while the version is inferred from table existence, an always-dropping down/1 looks unreachable; fix the marker and you arm it.

If you read that differently, I'd genuinely like to know — it's the one claim here that reinterprets your earlier work.

What changed

0.3.0 — removed migration_module/0 and deleted copy #2. Added the length validations the drift was masking: ConsentLog had no length validation on any field, while core's columns are narrower than copy #2 assumed. ConsentLog.create/1 is public API, so an over-long session_id (>64) or consent_version (>20) came back as a raw Postgrex varchar overflow rather than a changeset error.

0.3.1 — deleted copy #3 and fixed README step 1, which told users to copy that template and run mix ecto.migrate. It used create table with no if_not_exists, so it would fail outright on any install where core had already created the table. mix phoenix_kit_legal.install has always printed mix phoenix_kit.update instead — the README and the install task had been contradicting each other. Added test/consent_logs_ownership_test.exs and an AGENTS.md section.

No runtime behaviour changed by the removals: a migration that never ran can't stop running.

Open question for you

Do any hosts you know of predate core creating this table — i.e. actually ran copy #2 or #3 successfully and have 255-wide columns? If so they'll show as divergent under mix phoenix_kit.doctor on core 2.0, and mix phoenix_kit.repair is the reconciliation path. Worth knowing whether that's a real population or an empty set.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions