Document the v1 data model (Database epic, task #1) #11
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/data-model"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements task #1 of the Database Design epic. Documentation only — no entity classes,
DbContext, migration or EF configuration, since those are tickets #3 and #4 and writing them here would collide.docs/data-model.mdplus ADR-0016 (physical types), marked Proposed rather than Accepted because it needs your sign-off.Three things need your decision
Nothing here is claimed as signed off. The ticket's "sign-off recorded from product owner/stakeholder" criterion is yours and cannot be delegated.
1. Account deletion versus sole group ownership. This was already an open question; modelling narrowed it usefully and eliminated one option. It reduces to two named foreign keys, and
SET NULLis impossible ongroup_memberships.user_idbecause that column is half the primary key. So the choice is:RESTRICT— blocks account deletion until ownership is transferred, making ADR-0011's self-service promise conditionalCASCADE— deletes the membership, silently leaving an ownerless group, which ADR-0003 forbidsThe same conflict reappears on
groups.personal_for_user_idonce a personal group has been shared. The initial migration cannot be final on those two foreign keys until this is answered.2. Do you accept ADR-0016? Particularly
double precisionfor coordinates andcitextfor email — both are data migrations to reverse.3. May an Editor delete places? Recorded as open. No schema impact — the role is already stored and constrained; it changes an authorisation policy, so it does not block the migration.
The coordinate type decision
double precision, notnumeric(9,6):numericdoes not survive to any consumer — Leaflet and GeoJSON are IEEE 754 binary64, so the value is converted before anything draws it.numericprotects against cannot arise without a schema change.double precisionis the lossless path togeography(Point,4326)if PostGIS is ever adopted.Precision is not the deciding factor either way — binary64 carries ~12 decimal places at these magnitudes. The accepted cost is stated prominently: coordinates must never be compared with
=.A by-product worth knowing:
CHECK (latitude >= -90 AND latitude <= 90)also rejectsNaNand both infinities, because PostgreSQL sortsNaNabove every other float. Noisfinitecheck needed.Every foreign key states its delete behaviour, and four differ from the EF default
external_identities.user_id→users.idCASCADEgroups.personal_for_user_id→users.idCASCADEgroups.created_by_user_id→users.idSET NULLgroup_memberships.group_id→groups.idCASCADEgroup_memberships.user_id→users.idCASCADEgroup_memberships.invited_by_user_id→users.idSET NULLplaces.group_id→groups.idCASCADEplaces.created_by_user_id→users.idSET NULLThis is the part most likely to go wrong silently. EF Core's default for an optional relationship is
ClientSetNull, which emitsON DELETE NO ACTION— so left to defaults, the three audit columns would quietly become "the database refuses to delete the user", which is exactly the failure ADR-0011's self-service deletion cannot have. ADR-0005 already warns that relying on EF defaults is how the cascade decision becomes untrue; this is the concrete instance.A fifth table the ticket did not name
external_identities, keyed on(issuer, subject). ADR-0002's linked external identities have nowhere else to live, and a nullable(issuer, subject)pair onuserswould need the same two constraints in a worse place plus a cap of one provider. The ticket's "and any join/audit tables" arguably anticipates it.The ticket also says nothing about
email_verified_at, which ADR-0002 makes load-bearing for security — an unverified email must never link an external identity. Modelled, with the linking rule listed as an application invariant needing a test that fails loudly.Other decisions
UUIDv7 primary keys, application-generated — sequential integers in URLs make every authorisation check the only thing between a user and someone else's data.
citextfor email, so case-insensitive uniqueness lives in the column rather than in every call site. Role and status astext+CHECKrather than native enums orsmallint.timestamptzthroughout, no triggers.Two modelling choices worth a look: the personal group is a nullable unique
groups.personal_for_user_idpointing at the user rather thanusers.default_group_id, which avoids a circular foreign key entirely. Andgroup_membershipsuses a composite primary key(group_id, user_id)— that isCLAUDE.md's "unique on user+group", and it saves an index a surrogate key would have needed.Noticed, not elevated
Declining an invitation deletes the membership row, consistent with hard delete — so a declined user can be invited again indefinitely. If declines should be remembered, that changes this table's primary key meaning, and it is much cheaper to decide before the migration than after.
Verification
dotnet build0 warnings 0 errors;dotnet format --verify-no-changesexit 0 (regression check — this adds only docs).mermaid-cli11.16.0, then the PNG viewed — five entities, eight relationships, correct cardinalities. The newer comma-separatedPK,FKmarker was deliberately avoided so it renders on whatever Mermaid version Forgejo ships.Note
docs/adr/README.mdmay conflict with the concurrent logging branch — both add an index row to the same table. This one is 0016; the gap at 0015 is left for that branch.Verdict: changes needed
Independent review. I checked every ADR claim against the record it cites, rendered the diagram, and verified the four factual claims empirically rather than from memory — a throwaway
postgres:18.4-trixiecontainer for theNaNandcitextclaims, and a throwaway EF Core 10 / Npgsql 10.0.3 model for the delete-behaviour and naming claims. Both probes have been destroyed.This is a good document. The traceability is real, the reasoning is mostly right, and the two hardest calls (coordinate type, delete behaviour) are argued honestly with their costs stated. The changes below are four specific defects, three of which would be implemented literally and wrongly by the next ticket.
Verified claims — all four hold
CHECK (latitude >= -90 AND latitude <= 90)rejectsNaNand both infinities. Confirmed onpostgres:18.4-trixie:Correct, and the stated reason (
NaNsorts above every float) is the right reason.citextis available in the image the project runs.CREATE EXTENSION citextsucceeds onpostgres:18.4-trixie, version 1.8.HasPostgresExtension("citext")is the right EF Core API and the document is right to say the migration must create it.EF Core defaults. Confirmed on EF Core 10.0.10 with
Npgsql.EntityFrameworkCore.PostgreSQL10.0.3 — see the caveat below, but the substance holds: optional relationship →ClientSetNull, required →Cascade→ON DELETE CASCADE.UUIDv7 is available and sorts correctly through Npgsql.
Guid.CreateVersion7()is on the pinned SDK, and five keys generated in order came back fromORDER BY idin the same order — so the index-locality argument in ADR-0016 is not defeated by .NET'sGuidbyte layout, which was the obvious way for it to be quietly wrong.Mermaid renders.
minlag/mermaid-cliproduced a PNG: five entities, eight relationships, cardinalities as described. Avoiding thePK,FKmarker was the right call.Links, build, format, spelling. 140 relative links and heading anchors resolve (my script agrees with yours exactly).
dotnet build0/0,dotnet format --verify-no-changesexit 0. No American spellings. Existing ADRs untouched; only the index row was added, which ADR-0001 permits.Blocking
1. Invariant 4 is stated incompletely, and the incomplete version is the vulnerable one
Follow that literally and you build the takeover it is meant to prevent. The flow is: an external provider hands you an
emailclaim, you look up the local user by that address, you check the local account'semail_verified_at— which is set, because it is the victim's real, verified address — and you link.The unverified email in the attack is the provider's, not PlaceMark's. An attacker registers at any permissive IdP with the victim's address and signs in.
Both sides must be verified: the provider must assert
email_verified: trueand the localemail_verified_atmust be non-NULL. Given the document says of this invariant "this is the one whose failure is a security hole rather than an inconvenience", it needs to be the one stated completely.2. Nothing says changing
emailmust clearemail_verified_atThe
external_identitiessection says "A user who later changes their email address therefore keeps their linked identities" — so email is mutable. Butemailandemail_verified_atare independent columns and no invariant couples them.If a user can change
emailwhileemail_verified_atstays set, then invariant 4 is bypassable without any provider being involved: change your address to the victim's, remain "verified" from the old one, link. This is the same hole by a shorter route, and it is invisible in the schema because both columns look independently fine.It needs to be invariant 6, or the two columns need to be described as a pair that only ever changes together.
3. EF Core auto-indexes every foreign key — "Deliberately not indexed" will not survive the next ticket
The document makes exactly the right warning about delete behaviour and then omits the parallel one about indexes. From my probe, with no index configuration at all:
ix_places_created_by_user_idis one of the three indexes the document says must not exist. EF Core creates an index on every foreign key by convention, sogroups.created_by_user_id,places.created_by_user_idandgroup_memberships.invited_by_user_idwill all be indexed unless the next ticket explicitly removes them.This is the same failure mode as the delete-behaviour one — a well-reasoned decision silently reversed by a default — and it is the more likely of the two to go unnoticed, because an extra index breaks nothing. The Indexes section should say what has to be done (drop the FK-index convention, or
HasIndex(...).Metadataremoval per relationship) rather than only that the indexes are unwanted.4. "A personal group can be shared" is asserted, not decided — and it manufactures half the open question
I cannot find this anywhere in the ADRs. ADR-0004 says a personal default group is created at registration so a user "never encounters the concept"; it says nothing about whether that group may then be shared.
This matters because the PR asks the user to resolve an open question that is partly constructed by this assumption. If personal groups cannot be shared — which is a cheap product decision, consistent with ADR-0004's framing of the personal group as the thing a user who ignores groups never sees — then
groups.personal_for_user_idCASCADEis unconditionally safe and one of the two undecided foreign keys is decided immediately. Onlygroup_memberships.user_idremains genuinely open.That should be surfaced to the user as its own question ("may the personal default group be shared?") alongside the three already listed, because answering it is much easier than the ownership conflict and it removes half of it.
Non-blocking
ON DELETE NO ACTIONis not what EF emits — it emits nothing. Verified: for the optional relationship the migration iswith no
onDelete:argument, and the SQL isFOREIGN KEY (...) REFERENCES users (id)with noON DELETEclause at all. The behaviour isNO ACTIONbecause that is the SQL default, so your conclusion is right — but someone checking a generated migration against this document by grepping forNO ACTIONwill find nothing and conclude the document is wrong. Say "emits noON DELETEclause, which isNO ACTION".snake_case is a third-party package, not "EF Core's naming convention".
UseSnakeCaseNamingConvention()does not exist in EF Core 10 or the Npgsql provider — it isEFCore.NamingConventions(latest 10.0.1, by the Npgsql author). Worth naming, since the next ticket has to add it.ux_will not come out of that convention. Verified: it lowercases EF's default names, sopk_users,fk_places_users_created_by_user_idandix_places_group_idall match your forms exactly — but a unique index comes out asix_..., notux_....ux_users_emailandux_groups_personal_for_user_idneed explicitHasDatabaseName. Small, but the whole point of the naming section is that a violation should name its rule.ADR-0016 does not consider a non-deterministic ICU collation.
CREATE COLLATION ... (provider = icu, deterministic = false)on atextcolumn is the alternative PostgreSQL's own documentation points at these days, and it puts the rule in the column exactly ascitextdoes without an extension. It may well lose — it is newer, less familiar, and has its own pattern-matching restrictions — but ADR-0016's stated test is "each has an obvious-looking alternative that will be proposed again by someone who was not here", and this is one of them.Personal group deletion breaks invariant 1 and is not listed. Invariant 1 is enforced "by creating the group in the same transaction as the user" — which covers creation. Nothing covers deletion, and an Owner deleting a group is a first-class feature (ADR-0005). Delete your personal group and you are in the broken state ADR-0004 says every code path would otherwise have to tolerate. Should be an invariant: the personal group cannot be deleted while the user exists.
display_nameisNOT NULLwith a not-blank check, and OIDC may not supply one. Thenameclaim is optional in OIDC. Registration through an external provider therefore has to synthesise a display name, and nothing says what from. It is a small decision but it is one the next ticket will have to invent silently.external_identitieshas nocreated_at/updated_at. The Conventions section reads as universal ("Every timestamp istimestamptz...created_atandupdated_atare bothNOT NULL").linked_atis a fine substitute and the row is immutable, but the exception should say so.issuercapped atvarchar(255). The 255-character cap is onsub, per OIDC Core;issis a URL with no specified limit. Unlikely to bite, but the column comment implies the cap applies to both."nothing can start to [compare coordinates] without a schema change" is too strong. Nothing prevents someone writing
WHERE latitude = @latin an endpoint tomorrow — ADR-0006 excludes search as a scope decision, not as a mechanism. The guardrail is review, which is fine and is what ADR-0016 admits in its last paragraph; the data model claims something stronger than it has.The
bigintrejection slightly overstates the case. "Turning every authorisation check into the only thing standing between a user and someone else's data" — authorisation is the control, at any key type. Non-enumerable identifiers are defence in depth and a reduction in incidental data leakage, which is a good enough reason on its own. The decision is right; the framing invites someone to treat opaque keys as a security boundary.Testcontainers. The
citextextension requirement pins the integration test image too, not just Compose. Worth a line, since a test container on a different tag is exactly the "hand-built environment" the ADR's consequences warn about.ADR number gap at 0015. Understood and reasonable given the concurrent branch, but if that branch does not land the index reads 0014, 0016 with no explanation. Worth confirming before merge.
Judgements you asked for
The coordinate type argument is sound and I would take the same decision. The three reasons hold:
numeric's exactness genuinely is fictional at the point of use, since Leaflet and GeoJSON are binary64; nothing in v1 compares coordinates; andgeography(Point, 4326)stores binary64, so the PostGIS path is lossless. The precision framing is correct too — binary64 carries roughly 2.8e-14 degrees of resolution at these magnitudes.On the boundary case you asked me to consider: the round trip is safer than the document admits.
System.Text.Jsonwritesdoublein shortest-round-trippable form and parses correctly-rounded, andJSON.stringify/JSON.parsedo the same, so a storeddoublesurvives an edit-and-save unchanged bit for bit. The stated cost ("may come back differing in the last bits") is conservative rather than wrong, which is the right direction to be wrong in.On whether "never compare with
=" will hold: for v1, yes — the realistic implicit comparisons are EF change tracking (a spuriousUPDATEat worst) and test assertions (exact for the same literal, and Shouldly has a tolerance overload). What I would not rely on is the claim that a schema change is required before it can be violated; see above. But the discipline is thin because the surface is thin, not because anyone is being careful, and that is fine while the surface stays thin.The
ClientSetNullclaim is correct, with the wording caveat above. The in-memory description is accurate too — it nulls loaded dependents and leaves the rest, which is precisely why the failure is intermittent and looks like a bug rather than a configuration decision. This is the most valuable paragraph in the document.(issuer, subject)is the right key andexternal_identitiesis justified, not scope creep.subis only unique within an issuer, so the composite is the only correct natural key, and the ticket's "any join/audit tables" covers it. The rejection of nullable columns onusersis right for the reason given — the one-provider cap is the part that would be expensive to undo.The composite primary key
(group_id, user_id)is sound and EF Core handles it without complaint (verified:CONSTRAINT pk_memberships PRIMARY KEY (group_id, user_id)from a plainHasKey). The only friction is thatFindbecomes order-sensitive and the entity has no single-value identity for a URL — neither of which matters when a membership is always addressed as a user within a group, which is exactly the argument the document makes.The three open questions are genuinely open, and the document is right that the third has no bearing on it. What is missing is the fourth (personal group shareability, above), and the answer to it changes the shape of the first.
Verdict: changes needed
Second review, at
394cf53. All four blocking findings and all twelve non-blocking items from my review atfb37f20are genuinely actioned — I re-verified the ones that made empirical claims rather than taking the summary on trust. One new blocking item, which comes out of the escalation you asked me to sanity-check, and one non-blocking off-by-one.Re-verification of the round-one findings
Blocking 1 — fixed, and fixed better than I asked for. Invariant 4 now carries all three conditions with "All three, not any of them", names the provider's claim as the unverified thing in the attack, and closes with "the short version of it is wrong". That last sentence is the one that will stop someone reimplementing the short version from memory.
Blocking 2 — fixed. Invariant 4a plus the coupling paragraph under
users. "The two columns look independently reasonable in every schema dump, which is exactly why this is written here" is the right framing. Theexternal_identitiesparagraph now says the stable(issuer, subject)key is "a property of the key, not a permission to leave verification stale", which closes the loophole the old wording opened.Blocking 3 — fixed, and I reproduced both halves independently. Building the five tables as documented, with a
HasDatabaseNameon eachux_index and everyOnDeleteset explicitly:With
ForeignKeyIndexConventionin place:With
configurationBuilder.Conventions.Remove(typeof(ForeignKeyIndexConvention))and the three wanted indexes declared:All three surplus indexes gone, every
ON DELETEclause intact,CREATE EXTENSION IF NOT EXISTS citext;emitted ahead ofemail citext NOT NULL. The prescription in the document is correct and complete.Blocking 4 — fixed, and this is the change that most improves the document. The assertion is now a question, "nothing in the schema prevents it" is the honest framing, and the foreign key table's Conditional — settled if a personal group cannot be shared makes the dependency legible in the one table someone will actually read. "Answer that 'no' and this foreign key is settled as
CASCADEimmediately, leaving one genuinely open rather than two" is exactly the shape the escalation needed.All twelve non-blocking items — done. The ones worth naming:
NO ACTIONcorrection is better than my suggestion. "Grepping a migration for it proves nothing, and an absent clause is the thing to look for", ending on "from a line of SQL that is not there", is the version that survives contact with a reviewer.ux_note is verified in both directions, and I confirmed both: an unnamed unique index comes outix_, andHasDatabaseName("ux_users_email")survives the naming convention rather than being rewritten by it.HasCollationplus a hand-orderedmigrationBuilder.Sqlagainst a one-lineHasPostgresExtensionis a real cost, and calling it "a weak reason with a short shelf life" and inviting re-examination is what an ADR is for. Naming that non-deterministic collations are constrained in some index contexts is the caveat I would have wanted.bigintrewrite is a genuine improvement on my note: "the authorisation check is the security boundary, and an unguessable identifier is not a substitute for it", and "no endpoint may treat possession of an identifier as evidence of anything" in the consequences.issnote lands the point I was reaching for and adds the better one — a truncated issuer would silently match the wrong provider, so widen rather than truncate.external_identitiesnow earns its exception explicitly ("anupdated_atthere would be a column that can never change") rather than merely having one.Mermaid, links, build. The fenced block is byte-identical to the one I rendered at
fb37f20— five entities, eight relationships — so that render still stands. 153 relative links and heading anchors resolve, including the four new question anchors; my count matches yours exactly. No non-documentation file differs betweenfb37f20and394cf53, so thedotnet build0/0 anddotnet formatexit 0 from round one carry unchanged. No American spellings in either file.Blocking — the escalation is right, and understated
You asked me to sanity-check the reading. It is correct, and it is bigger than the document says. The document sites it here:
That is all true, but it is filed under may a user change their email address, as though the gap were created by email change. It is not. Email change is one way to fall into it; registration is the other, and it comes first.
With no email sending,
email_verified_atcan only ever be set by an external provider assertingemail_verified: true. There is no other source of proof in v1. Nothing else in the product can prove an address. So:ux_users_emailforbids the address twice. They sign in with a provider, get refused, and have nowhere to go.email_verified: true, then add a password. So v1 supports OIDC-then-local but not local-then-OIDC — an asymmetry that is a real product constraint and is written down nowhere.This is a genuine contradiction between ADR-0002 and ADR-0006 that no record names, and finding it is exactly what the "Raised by this modelling" section is for. It should be stated as its own item, not as a consequence of the email-change question, because it holds whatever the answer to that question is. The natural home is a fifth question — how is an email address ever verified, given ADR-0006? — with the candidates being: accept the asymmetry and document it; treat a provider's
email_verifiedclaim as the only source of verification and say so underusers; or supersede part of ADR-0006 for transactional mail only.Why this blocks rather than being a note. The user is about to make a scope decision from this list, and the item as written asks them a smaller question than the one that exists. Answering "no, email cannot be changed" — which the document offers as the option that makes the coupling problem disappear — would leave them believing the verification gap is closed, when local registration still cannot verify and the hybrid decision's main scenario is still dead. That is the wrong thing to hand someone as a decision.
It is one paragraph plus a heading. Everything else here is ready.
Non-blocking
The verification paragraph in Indexes miscounts, twice. From my reproduction:
The table immediately above lists ten entries, five of them primary keys and five separate indexes —
ux_users_email,ix_external_identities_user_id,ux_groups_personal_for_user_id,ix_group_memberships_user_id,ix_places_group_id. My script emitted exactly those five. And the foreign key table two sections earlier has eight rows; my script emitted eightON DELETEclauses, one per row, all correct.Neither number is reachable from the document's own tables. "Four" would be right for foreign key indexes only, but the sentence before it says three. This matters more than an ordinary typo because these two sentences exist solely to assert that the check was done — a reader who counts the table and gets a different answer has to decide whether the count or the check was sloppy. Both should be five and eight.
Everything else is verified and, on the four items I raised as blocking last round, materially better than what I asked for. Fix the escalation's scope and the count, and I expect this to be mergeable.
Verdict: mergeable
Third review, at
2c3afdf. The blocking item is fully resolved and the miscount is fixed the right way — by correcting the probe rather than the digits, which is the harder and more honest of the two options. Nothing blocks. What follows is my view on the two new candidates, a confirmation of the reasoning you asked me to check, and four notes for whoever writes the resolving ADR. None of them is a reason to hold this.Re-verification
The verification question is now stated correctly and in the right place. It leads "Raised by this modelling", it is framed as a contradiction between two accepted records rather than a gap in the model, and it says "unreachable" rather than "degraded". The three-way siting works: a reader who arrives at the column definition, at invariant 4, or at the ADR index all hit it. The invariant 4 note — "That refusal is correct; the defect is upstream... Implement the invariant as stated and do not relax it to make the flow work" — is the sentence that will do the most work, because relaxing it is precisely what an implementer under time pressure would do to make the flow pass.
The
userssubsection is well placed at the end of that section rather than mid-body, and the two-row table is the clearest statement of the asymmetry in the document. The correction in May a user change their email address? — "answering it 'no' must not be mistaken for closing it" — closes the exact trap I was worried about.The miscount is fixed by fixing the probe. Your corrected numbers — eight indexes with the convention, five without, eight
ON DELETEclauses — match my independent reproduction at394cf53exactly, which is why they disagreed with the document before. Adding "primary key indexes are not included in either count: PostgreSQL creates them for the constraint, not for the convention" removes the ambiguity that let a wrong number look plausible.Mechanical checks. 168 relative links and heading anchors resolve, including the two long new ones — my count matches yours. The Mermaid block is byte-identical to the one I rendered at
fb37f20, so that render stands unchanged. Heading structure is sound:## usersruns 150–203 with the new###at 204–232 and## external_identitiesat 233, so the body is intact — the slip you caught is genuinely gone. Againstmainthe branch touches onlyREADME.md,docs/adr/README.md,docs/adr/0016anddocs/data-model.md; no accepted ADR body is modified, and 0016 is still Proposed so amending it remains within ADR-0001. No code file differs fromfb37f20, so the build 0/0 and format exit 0 carry. No American spellings.The escalation list is eight, and the headings agree: three under "Already recorded", five under "Raised by this modelling".
The reasoning you asked me to check — it is right, and it has a name
Correct, and correct in the direction that is counter-intuitive, which is why it was worth writing down. Worked through concretely: Mallory registers
alice@example.comlocally with a password — v1 proves nothing, so this succeeds.email_verified_atisNULL. Alice, who owns the address, later signs in through a provider that assertsemail_verified: true. Under candidate 2 the system matches on address, finds Mallory's row, and links Alice's provider identity into it. Alice is now signed into Mallory's account, believing it is hers; Mallory keeps password access and sees everything Alice does from that point on. The victim is the genuine owner, and the account they land in is the squatter's. Exactly as stated.Two things to add for whichever ADR resolves this:
This attack class has a name: account pre-hijacking, specifically the Classic-Federated Merge variant (Sudhodanan and Paverd, 2022). Naming it in the resolving ADR is worth a line — it gives the implementer something to read, and it stops the mitigation being re-derived from first principles by whoever picks the ticket up.
The standard mitigation set has a second half the document does not mention. Refusing the link is the first half. The second is that on any link or merge that does go ahead, every existing session on the pre-existing account is invalidated and its credential reset — otherwise a squatter who linked first keeps a live session into an account that has since become someone else's. That applies to candidates 2 and 3 and belongs in the resolving record.
My view on candidate 4
Valid, and the author was right that only one side of the contradiction had been offered. Two accepted records collide, so dropping either resolves it, and not noticing that is how a false dichotomy gets escalated to a user. Good catch.
Its real strength is not stated: candidate 4 is the only one that reduces v1 scope, and it retires the cost ADR-0002 itself identified as the largest in the release — "Two authentication paths to build, test and keep secure — meaningfully more work than either alone, and the largest single cost accepted in the v1 scope". It also removes password storage, reset flows and the associated breach risk, which is the thing ADR-0002's Context named as the downside of local accounts. For a spare-time project whose stated risk is never shipping (ADR-0006), that is a serious argument and it deserves to be in the candidate.
Two caveats:
It re-proposes something ADR-0002 rejected on record. ADR-0002's alternatives say: "OIDC only. Removes password handling entirely — genuinely attractive from a security standpoint. Rejected because it hard-couples a personal tool's availability to a third party, and forces an account requirement on users who may not want one." The contradiction is new information ADR-0002 did not have, so revisiting is legitimate — but the resolving ADR has to engage with that stated reason rather than reverse it silently. That is exactly the failure mode ADR-0001's supersession discipline exists to prevent, and it is worth a clause in the candidate so the next author does not have to find it.
"Partially supersedes" is generous for this one. Candidates 1, 2 and 5 displace a mechanism or a claim inside ADR-0002 while its decision stands. Candidate 4 displaces the decision itself — "Support both" becomes "support one" for v1. What survives is the bearer-token/JWT paragraph. Under ADR-0001's test ("a partially superseded record stays Accepted, because most of it still stands") that is at best borderline, and recording it as partial when it is closer to full is how an index row ends up presenting a reversed decision as untouched. Worth naming so the strength gets recorded correctly.
My view on candidate 5 — this is the one I would take
It is the strongest candidate on the list, and it is stronger than the document quite says. If you want a recommendation to put to the user alongside the options, it is this one.
It dissolves the contradiction instead of deciding it. Every other candidate declares one of two accepted records wrong. Candidate 5 leaves both intact in substance and replaces only the linking mechanism, which is the narrowest possible supersession. That is qualitatively better than picking a side.
The security property is the right one, and it is not a trade-off. Control of the local account is proved by signing in to it; control of the provider account by completing the flow. Neither depends on an email address being trustworthy, from either party. It is immune to the pre-hijacking attack above by construction rather than by a check that can be relaxed — a squatter's account cannot be linked into, because linking requires already being signed in to it.
It removes the most delicate machinery in this document from the security path. Invariant 4, invariant 4a, and the
email/email_verified_atcoupling are the three things here most likely to be got wrong by a later change, and they are load-bearing only because linking depends on verified email. Under candidate 5 they stop being security-critical. Fewer ways to be wrong is worth more at this scale than any of the other candidates' benefits.And it is the only candidate that is forward-compatible with all the others. If email sending arrives later, verification-based linking can be added on top of explicit linking without unpicking anything — candidate 5 does not need superseding to get candidate 3 as well. Every other candidate has to be revisited when the constraint that produced it lifts. That is the argument I would lead with, and it is not in the document.
The cost is stated honestly. I would only sharpen one point: refusing an OIDC sign-in with "this address is already registered, sign in and link from settings" is an account-enumeration oracle for an unauthenticated caller. It is not a new one — local registration already leaks it through the unique email — but ADR-0016 was careful about enumeration elsewhere, so the resolving record should say it is a knowing acceptance rather than an oversight.
Non-blocking notes
The schema-impact exclusivity is slightly wrong. "Candidates 1 to 4 change nothing here. Candidate 5 is the only one that could remove a column" — candidate 4 could equally remove
password_hash. Under OIDC-only v1 it is a column nothing ever writes, and this document's own thesis is that every column earns its place; "simply stays unused" is a defensible choice but it is a choice, not a fact. Candidate 4 also changes whatemail_verified_atmeans — every account is provider-verified, so the column is always set and the whole asymmetry disappears. The substance of the note is right; only the word "only" is doing work it cannot support. One clause.Candidate 3 needs an address-takeover rule it does not have. It is described as "the only one that... restores the model ADR-0002 describes", but it does not quite, on its own. With verification email available, a squatter still holds an unverified local row on the genuine owner's address, invariant 4 still correctly refuses, and the owner is now denied their own address by someone who never proved anything. Candidate 3 therefore also needs "a party who verifies an address may take it from an unverified row holding it". That is standard and cheap, but unstated it makes candidate 3 look more complete than it is. Worth a sentence, or leave it to the resolving ADR — either is fine, since the candidates are explicitly not being chosen here.
Everything I raised across three rounds is resolved, and on the items that mattered the result is better than what I asked for. The four notes above are inputs to the ADR that resolves this, not defects in this document. Merge it and put the list to the user; if you want a recommendation attached, mine is candidate 5.
2c3afdff89abbe054fbcVerdict: mergeable
Fresh token at
abbe054. The rebase is clean and the conflict resolution is correct. My verdict at2c3afdfcarries unchanged.What I checked
The two reviewed documents are byte-identical. Not "look unchanged" — SHA-256 over the blobs at
2c3afdfandabbe054:So everything I established at
2c3afdftransfers without re-derivation, including the Mermaid render and the migration probes.The index resolution is coherent. Sixteen rows for sixteen files, numerically contiguous, no gap: 0015 (logging) Accepted then 0016 (physical types) Proposed, with the data-model pointer paragraph intact beneath the table. The 0015 row's title matches that record's H1 exactly. The 0015 gap I flagged at
fb37f20is now closed by the record it was left for, which is how it was supposed to go.The open-questions section merged additively. Against current
mainthe branch only adds there — the two amended bullets and the new email-verification one — with nothing of PR #12's removed. That was the other way this particular conflict could have gone wrong, since both branches touched that file.No existing ADR body was modified.
git diff main..abbe054 -- docs/adr/00*.mdis empty apart from 0016, so ADR-0001's immutability rule is respected through the rebase — the risk being that a conflict resolution quietly reflows a frozen record.No conflict markers anywhere in the tree.
Against
mainthe branch changes four files, all Markdown —README.md,docs/adr/README.md,docs/adr/0016-*.md,docs/data-model.md. No compilable file differs frommain, so the build state ismain's, which passed CI when #12 merged; the 0/0 and format exit 0 are inherited rather than needing a re-run. TheREADME.mdhunks are the same two I reviewed atfb37f20, carried onto a file #12 had also touched.181 relative links and heading anchors resolve, 0 bad.
One note, not a blocker
Your link count of 157 disagrees with mine, and it moved the wrong way. I get 181, up from the 168 we both measured at
2c3afdf— which is what a rebase onto amainthat gained an ADR should do. ADR-0015 alone contributes 8, and the index row and its cross-references most of the rest.A rebase that only adds a file cannot reduce the number of links in the tree, so 168 → 157 has to be a change in what your script looked at rather than a change in the repository.
181 − 19 (README) − 6 (CONTRIBUTING) − 1 (CLAUDE)is 155, so scanningdocs/only would land near your figure.Nothing is wrong with the artefact — I have independently verified all 181 resolve. I raise it because it is the same signature as the round-two miscount, where a number of yours disagreed with mine and the cause turned out to be a probe silently omitting part of its input. Worth a look before the next "links resolve" claim, so that claim keeps its weight.
Carried forward
Agreed on all of it, and the placement is right — the pre-hijacking name, the session-invalidation half of the mitigation, the
password_hashcorrection to candidate 4's schema impact, ADR-0002's recorded rejection of OIDC-only, the partial-versus-full supersession strength, and candidate 3's missing address-takeover rule all belong to the ADR that resolves the verification question, not to a reference document that describes tables.Good to merge.