From a2bebc8a59ccf6ee2cc210af46d18e3ff128cbdd Mon Sep 17 00:00:00 2001 From: Artur Shiriev Date: Sat, 19 Sep 2026 14:18:37 +0300 Subject: [PATCH] docs(adr): compress to 10 records in the domain-modeling format Every surviving record is now a title plus one paragraph, with rejected alternatives named inside the paragraph. Dropped, for failing the admission test: - 0004 anonymous doc and metrics paths: an `exclude` list literal is not hard to reverse, and the anchoring rule already lives as a comment on the list. - 0011 per-user channel topology and 0012 no server-side event replay: both describe a realtime layer that does not exist in the repo. There is no channel, relay, SSE endpoint or Redis anywhere. Merged: - 0003 explicit cookie secure flag into 0002 cookie auth, not bearer: one decision about how the session cookie is carried and configured. The README Template section is rewritten to the one-paragraph shape and the sentence after it no longer requires a revisit-trigger section. Numbering, Status, the admission test and "Where other facts go" are unchanged. --- docs/adr/0001-sequence-ids-not-snowflakes.md | 34 ++++------- docs/adr/0002-cookie-auth-not-bearer.md | 46 +++++---------- docs/adr/0003-domain-error-vocabulary.md | 18 ++++++ docs/adr/0003-explicit-cookie-secure-flag.md | 39 ------------- .../0004-anonymous-doc-and-metrics-paths.md | 36 ------------ docs/adr/0004-mutation-requires-membership.md | 16 ++++++ docs/adr/0005-domain-error-vocabulary.md | 44 --------------- .../0005-upsert-via-duplicate-key-recovery.md | 16 ++++++ docs/adr/0006-idempotency-scoped-per-chat.md | 15 +++++ docs/adr/0006-mutation-requires-membership.md | 36 ------------ docs/adr/0007-read-marker-integrity.md | 16 ++++++ .../0007-upsert-via-duplicate-key-recovery.md | 39 ------------- docs/adr/0008-idempotency-scoped-per-chat.md | 35 ------------ .../0008-repoint-last-message-on-delete.md | 17 ++++++ ...009-coverage-exclusions-structural-only.md | 16 ++++++ docs/adr/0009-read-marker-integrity.md | 41 -------------- docs/adr/0010-auth-carries-an-actor-id.md | 17 ++++++ .../0010-repoint-last-message-on-delete.md | 32 ----------- docs/adr/0011-per-user-channel-topology.md | 33 ----------- docs/adr/0012-no-server-side-event-replay.md | 33 ----------- ...013-coverage-exclusions-structural-only.md | 44 --------------- docs/adr/0014-auth-carries-an-actor-id.md | 56 ------------------- docs/adr/README.md | 31 +++------- docs/agents/domain.md | 2 +- 24 files changed, 166 insertions(+), 546 deletions(-) create mode 100644 docs/adr/0003-domain-error-vocabulary.md delete mode 100644 docs/adr/0003-explicit-cookie-secure-flag.md delete mode 100644 docs/adr/0004-anonymous-doc-and-metrics-paths.md create mode 100644 docs/adr/0004-mutation-requires-membership.md delete mode 100644 docs/adr/0005-domain-error-vocabulary.md create mode 100644 docs/adr/0005-upsert-via-duplicate-key-recovery.md create mode 100644 docs/adr/0006-idempotency-scoped-per-chat.md delete mode 100644 docs/adr/0006-mutation-requires-membership.md create mode 100644 docs/adr/0007-read-marker-integrity.md delete mode 100644 docs/adr/0007-upsert-via-duplicate-key-recovery.md delete mode 100644 docs/adr/0008-idempotency-scoped-per-chat.md create mode 100644 docs/adr/0008-repoint-last-message-on-delete.md create mode 100644 docs/adr/0009-coverage-exclusions-structural-only.md delete mode 100644 docs/adr/0009-read-marker-integrity.md create mode 100644 docs/adr/0010-auth-carries-an-actor-id.md delete mode 100644 docs/adr/0010-repoint-last-message-on-delete.md delete mode 100644 docs/adr/0011-per-user-channel-topology.md delete mode 100644 docs/adr/0012-no-server-side-event-replay.md delete mode 100644 docs/adr/0013-coverage-exclusions-structural-only.md delete mode 100644 docs/adr/0014-auth-carries-an-actor-id.md diff --git a/docs/adr/0001-sequence-ids-not-snowflakes.md b/docs/adr/0001-sequence-ids-not-snowflakes.md index 0c2b464..0c41cae 100644 --- a/docs/adr/0001-sequence-ids-not-snowflakes.md +++ b/docs/adr/0001-sequence-ids-not-snowflakes.md @@ -1,24 +1,14 @@ # Sequence ids, not snowflakes -`messages.id` is a plain BigInt identity column. Postgres assigns values -monotonically, which supplies the total ordering that cursor pagination -(`before_id`), catch-up (`after_id`), and client-side gap detection all read off -the primary key index. - -## Rejected: snowflake ids - -64-bit time-sortable ids assembled from timestamp, machine id, and sequence. -Their advantage is coordination-free generation across independent writers while -staying k-sortable. This service has one writer, Postgres, so the coordination -problem it solves does not exist here. Adopting them would add an id-generation -component and a machine-id assignment concern to demonstrate nothing the repo is -about. - -Time-based ids would also be strictly worse for gap detection: clock skew makes -"is my next id contiguous with the last one I saw" unanswerable, whereas a -sequence makes it a comparison. - -## Revisit trigger - -Message writes originating from more than one process without passing through -Postgres, or a partitioned/sharded messages table. +`messages.id` is a plain BigInt identity column, so Postgres assigns values +monotonically and supplies the total ordering that cursor pagination +(`before_id`), catch-up (`after_id`) and client-side gap detection all read off +the primary key index. Snowflake ids, the usual choice for a chat backend, were +rejected: what they buy is coordination-free generation across independent +writers, and this service has exactly one writer, Postgres, so there is no +coordination problem to solve and an id-generation component plus a machine-id +assignment concern would be added to demonstrate nothing. Time-based ids would +also be strictly worse for gap detection, because clock skew makes "is my next +id contiguous with the last one I saw" unanswerable where a sequence makes it a +comparison. Message writes that reach storage without passing through Postgres, +or a partitioned `messages` table, would reopen this. diff --git a/docs/adr/0002-cookie-auth-not-bearer.md b/docs/adr/0002-cookie-auth-not-bearer.md index 5a231f4..7527397 100644 --- a/docs/adr/0002-cookie-auth-not-bearer.md +++ b/docs/adr/0002-cookie-auth-not-bearer.md @@ -1,34 +1,16 @@ # Cookie auth, not a bearer header -**Decision:** The JWT travels in a cookie (`JWTCookieAuth[Actor]`), not in -an `Authorization: Bearer` header. - -## Context - -Every endpoint shipped today is REST, where a bearer header is the more -conventional choice and keeps the token out of the browser's ambient -credential store. The realtime follow-on adds a server-sent-events stream. - -## Decision & rationale - -A browser `EventSource` cannot set request headers — there is no API for it. -An SSE endpoint authenticated by a bearer header would therefore need a -second authentication mechanism (a token in the query string, or a -short-lived ticket exchanged before connecting), which means two code paths -to keep in agreement and a token that lands in access logs. - -A cookie is sent automatically on the `EventSource` request, so the stream -authenticates identically to every other endpoint with no second path. That -this repository exists to demonstrate a realistic composition is what settles -it: carrying two auth mechanisms to avoid a cookie would be the less -realistic shape. - -The cost is accepted deliberately: cookie auth needs CSRF consideration on -state-changing endpoints, and `jwt_cookie_secure` must be `True` behind -HTTPS — see -[`0003-explicit-cookie-secure-flag.md`](0003-explicit-cookie-secure-flag.md). - -## Revisit trigger - -The SSE endpoint being dropped from scope, or a non-browser client becoming -the primary consumer. +The JWT travels in a cookie (`JWTCookieAuth[Actor]`), not in an +`Authorization: Bearer` header, although every endpoint shipped today is REST, +where a bearer header is conventional and keeps the token out of the browser's +ambient credential store. The planned server-sent-events stream settles it: a +browser `EventSource` cannot set request headers, so a bearer-authenticated +stream needs a second mechanism, a query-string token or a pre-connect ticket, +which is two auth paths to keep in agreement and a token that lands in access +logs. Two costs are accepted deliberately: state-changing endpoints need CSRF +consideration, and `jwt_cookie_secure` is an explicit setting defaulting to +`False` rather than derived from `service_environment`, because a security +property inferred from an unrelated string is one nobody audits, and because +`True` by default would break the local HTTP development this application is +demonstrated with. The anonymous surface is four anchored `exclude` prefixes: +`^/docs`, `^/health`, `^/static` and `^/metrics`. diff --git a/docs/adr/0003-domain-error-vocabulary.md b/docs/adr/0003-domain-error-vocabulary.md new file mode 100644 index 0000000..0a0cdfa --- /dev/null +++ b/docs/adr/0003-domain-error-vocabulary.md @@ -0,0 +1,18 @@ +# Three domain exceptions, not one + +`app/exceptions.py` defines `ChatAppError` with three subclasses, each mapped to +a status code by a handler registered in `build_app`: `PermissionDeniedError` to +`403` for authorization only, `ValidationError` to `400` for request shape ("a +direct chat must have exactly two distinct members"), and `ConflictError` to +`409` for state conflict, which today means editing a deleted message. The +initial design raised `PermissionDeniedError` for all three, and it is wrong +twice over: a body with three members in a direct chat is not a permissions +problem, and the author of a deleted message is authorized, so either inside a +"Permission denied" envelope tells the client to find credentials it already +has. Login failure is the mirror of that mistake, which is why `login` and +`GET /api/auth/me/` raise Litestar's `NotAuthorizedException`: a bad credential +is an identification failure, with no actor yet to authorize. +`advanced-alchemy`'s `NotFoundError`, `DuplicateKeyError` and `ForeignKeyError` +map to `404`, `409` and `400` with constant detail strings rather than +`str(exc)`, because an integrity error can carry bound parameters from another +row. diff --git a/docs/adr/0003-explicit-cookie-secure-flag.md b/docs/adr/0003-explicit-cookie-secure-flag.md deleted file mode 100644 index 4254b92..0000000 --- a/docs/adr/0003-explicit-cookie-secure-flag.md +++ /dev/null @@ -1,39 +0,0 @@ -# Cookie security is an explicit setting - -`Settings.jwt_cookie_secure` defaults to `False` and is passed straight to -`JWTCookieAuth(secure=...)`. Production must set it. - -Litestar sets `httponly=True` and `samesite="lax"` for you, but leaves `secure` -as `None`, so without this setting a session JWT with a seven-day lifetime -travels over plain HTTP. - -A companion guard lives in `Settings.ensure_jwt_secret_is_configured`, called as -the first statement of `build_app`: booting with the default `jwt_secret` -outside `service_environment="local"` raises rather than starting a service -whose every user's token is forgeable. - -## Rejected: deriving it from the environment - -`secure = service_environment != "local"` needs no new setting and is right by -default. It was rejected because a reader of a reference application should see -where the decision is made. A security property inferred from an unrelated -string is a property nobody audits, and the inference is silently wrong the -first time someone introduces an environment name the expression did not -anticipate. - -## Rejected: defaulting to True - -Correct for production and unusable for local development over HTTP, which is -how this application is demonstrated. - -## Consequence - -A deployment that forgets `JWT_COOKIE_SECURE` transmits session cookies in -clear. The startup guard covers the forged-token case but deliberately does not -cover this one, because there is no way to distinguish "HTTP because local" from -"HTTP by mistake" at boot. - -## Revisit trigger - -Adding HSTS or terminating TLS in-process, either of which would make `True` a -safe default. diff --git a/docs/adr/0004-anonymous-doc-and-metrics-paths.md b/docs/adr/0004-anonymous-doc-and-metrics-paths.md deleted file mode 100644 index 0ed8117..0000000 --- a/docs/adr/0004-anonymous-doc-and-metrics-paths.md +++ /dev/null @@ -1,36 +0,0 @@ -# The anonymous surface is four prefixes - -`jwt_cookie_auth`'s `exclude` list holds `^/docs`, `^/health`, `^/static` and -`^/metrics`. Every pattern is anchored, because Litestar joins them into one -alternation and matches it with an unanchored `findall`: an unanchored `/health` -would silently deauthenticate any future route containing that substring, such -as `/api/chats/{id}/health`. - -`/static` is Swagger's own offline asset directory, mounted because -`swagger_offline_docs` is on. Without the exclusion the docs page returns `200` -and then every asset request returns `401`, so the page loads and fails to -render for anonymous visitors. `/metrics` is a Prometheus scrape target, -registered because `prometheus_client` ships in `lite-bootstrap[litestar-all]`; -a scrape target behind a session cookie is a broken feature, and the endpoint -exposes process and request metrics, not user data. - -Route-level exemptions are expressed differently: `register` and `login` use -`exclude_from_auth=True` on the handler. Path-shaped exclusions go in the list; -route-shaped ones go on the route. Each policy has one home. - -## Rejected: leaving /metrics authenticated - -Defensible, and it is what shipped initially by omission. But it silently -disables a feature the bootstrapper registers, and the standard hardening for -metrics is a separate port or a network ACL, which is a deployment concern this -repository does not model. - -## Consequence - -Anything served under those four prefixes is public. A future route must not be -placed under them casually. - -## Revisit trigger - -Metrics carrying anything user-identifying, a deployment that exposes `/metrics` -to the internet, or Litestar changing where Swagger's offline assets are mounted. diff --git a/docs/adr/0004-mutation-requires-membership.md b/docs/adr/0004-mutation-requires-membership.md new file mode 100644 index 0000000..4902dbe --- /dev/null +++ b/docs/adr/0004-mutation-requires-membership.md @@ -0,0 +1,16 @@ +# Mutation requires membership, not just authorship + +`fetch_message_for_author` (`app/use_cases/message_authorization.py`) is the one +definition of the check order shared by `EditMessageUseCase` and +`DeleteMessageUseCase`: load the message (`404` if absent), verify the actor is +a member of its chat (`403`), then verify the actor is the author (`403`). +Authorship alone shipped first and left every authenticated user able to `PATCH` +or `DELETE` an arbitrary message id in a chat they had no visibility into, and +to distinguish "does not exist" from "exists, not mine" for it; authorship +happens to block the ordinary case, which is why the first round of tests passed +identically with and without the membership check. The rule is pinned by a test +that constructs the one state where the two disagree, deleting the author's +`chat_members` row. A non-member still learns whether a message id exists, +because the message must be loaded before its chat is known, and that residual +is accepted deliberately, mirroring `FetchChatUseCase` returning `403` rather +than pretending the chat does not exist. diff --git a/docs/adr/0005-domain-error-vocabulary.md b/docs/adr/0005-domain-error-vocabulary.md deleted file mode 100644 index 70d7cf9..0000000 --- a/docs/adr/0005-domain-error-vocabulary.md +++ /dev/null @@ -1,44 +0,0 @@ -# Three domain exceptions, not one - -`app/exceptions.py` defines `ChatAppError` and three subclasses, each with a -handler registered in `build_app`: - -- `PermissionDeniedError` to `403`, for authorization only: the caller may not - perform this action on this resource. -- `ValidationError` to `400`, for request shape: "a direct chat must have - exactly two distinct members", "before_id and after_id are mutually - exclusive", "limit must be at least 1", "that message is not in this chat". -- `ConflictError` to `409`, for state conflict: editing a message that has been - deleted. - -`advanced-alchemy`'s `NotFoundError` maps to `404`, `DuplicateKeyError` to -`409`, and `ForeignKeyError` to `400` with a constant detail string. Litestar -handles `NotAuthorizedException` natively as `401`. - -## Rejected: PermissionDeniedError for everything - -The initial design raised `PermissionDeniedError` for malformed request bodies -and for state conflicts as well as for authorization. It is the shape a reader -copies, and it is wrong twice over: a body with three members in a direct chat -is not a permissions problem, and the author of a deleted message *is* -authorized. Returning either inside a "Permission denied" envelope tells the -client to go find credentials it already has. - -Login failure is the mirror of that mistake, and it is why the login handler -raises Litestar's own `NotAuthorizedException` instead of -`PermissionDeniedError`: a bad credential is an *identification* failure, not -an authorization decision about an already-identified actor — at that point -there is no actor yet to authorize. `GET /api/auth/me/` raises it for the same -reason, on a token whose user row is gone. Those two are the only deliberate -uses of `litestar.exceptions`. - -## Consequence - -Every new use case must pick a category deliberately. Handlers must not -stringify the underlying exception when the query carried credential material, -which is why `DuplicateKeyError` and `ForeignKeyError` return constant details. - -## Revisit trigger - -A fourth failure category that fits none of the three, or an `RFC 9457` -problem-details response format, which would restructure all of them. diff --git a/docs/adr/0005-upsert-via-duplicate-key-recovery.md b/docs/adr/0005-upsert-via-duplicate-key-recovery.md new file mode 100644 index 0000000..bae8769 --- /dev/null +++ b/docs/adr/0005-upsert-via-duplicate-key-recovery.md @@ -0,0 +1,16 @@ +# Upsert by recovering from DuplicateKeyError + +`CreateChatUseCase` and `CreateMessageUseCase` both read first to see whether +the row already exists and then insert, but the read is only an optimisation: +the correctness guarantee is the `except DuplicateKeyError` branch, which rolls +back and re-reads the row the winner committed. The pre-check alone does not +make the insert safe. At READ COMMITTED two concurrent "open a DM with Bob" +requests both miss the read, both insert, and the loser violates +`uq_chats_direct_key`, which surfaces as a `409` to a user who should simply +have received the existing chat. `@postgres_retry` does not rescue it either, +because `db-retry` retries serialization and connection failures, not integrity +violations, and `SELECT ... FOR UPDATE` has nothing to take, since the race is +between two inserts of a row that does not yet exist. Both recovery paths are +exercised by repository subclasses (`_RacingChatsRepository`, +`_RacingMessagesRepository`) whose `create` raises and whose lookup misses once, +simulating the database condition at a seam the tests already own. diff --git a/docs/adr/0006-idempotency-scoped-per-chat.md b/docs/adr/0006-idempotency-scoped-per-chat.md new file mode 100644 index 0000000..d61797b --- /dev/null +++ b/docs/adr/0006-idempotency-scoped-per-chat.md @@ -0,0 +1,15 @@ +# Idempotency is scoped per chat + +`messages` carries `UniqueConstraint("chat_id", "idempotency_key")`, and the +pre-check lookup, the constraint and the `DuplicateKeyError` recovery re-read +all filter on that same pair. Idempotency is a property of an operation, and the +operation is "send this message to this chat", so two chats are two operations +and a key reused across them is not a retry of anything. A global constraint on +`idempotency_key` shipped first and made a reachable state look unreachable: +cross-chat key reuse under concurrency raised `DuplicateKeyError` from the +insert, the chat-scoped re-read missed, and control reached a guard whose only +justification was "the unique constraint guarantees a match here". Worse, a +client reusing a key across chats received the other chat's message while its +intended message was never written, a silent wrong-row return. The three +surfaces must agree, and aligning the constraint with the lookup was both the +cheaper direction and the one that matches the domain. diff --git a/docs/adr/0006-mutation-requires-membership.md b/docs/adr/0006-mutation-requires-membership.md deleted file mode 100644 index 2e3f074..0000000 --- a/docs/adr/0006-mutation-requires-membership.md +++ /dev/null @@ -1,36 +0,0 @@ -# Mutation requires membership, not just authorship - -`fetch_message_for_author` (`app/use_cases/message_authorization.py`) is the -single definition of the check order for both `EditMessageUseCase` and -`DeleteMessageUseCase`: load the message (`404` if absent), verify the actor is -a member of its chat (`403`), then verify the actor is the author (`403`). - -## Rejected: authorship alone - -Shipped first, and it left every authenticated user able to `PATCH`/`DELETE` an -arbitrary message id in a chat they had no visibility into, and to distinguish -"does not exist" from "exists, not mine" for it. Authorship happens to block the -ordinary case, which is why the first round of tests passed identically with and -without the membership check. - -It was also inconsistent with the rest of the codebase: every other actor-scoped -use case gates on membership first. A reference application that applies its own -authorization rule unevenly teaches the wrong habit. - -The check is proven by a test that constructs the one state where membership and -authorship disagree: the author's `chat_members` row is deleted, leaving her the -author of a message in a chat she is no longer in. - -## Consequence - -A non-member still learns whether a message id exists, because the message must -be loaded before its chat is known. That residual is accepted deliberately and -mirrors the decision that `FetchChatUseCase` returns `403` rather than -pretending the chat does not exist. See -[#21](https://github.com/modern-python/chat-app/issues/21). - -## Revisit trigger - -A moderator or administrator role that must act on messages in chats it does not -belong to, or a requirement to close the existence oracle, which would mean -scoping the lookup through a `chat_members` join. diff --git a/docs/adr/0007-read-marker-integrity.md b/docs/adr/0007-read-marker-integrity.md new file mode 100644 index 0000000..5948bce --- /dev/null +++ b/docs/adr/0007-read-marker-integrity.md @@ -0,0 +1,16 @@ +# Read-marker integrity + +`MarkReadUseCase` verifies membership, verifies that `last_read_message_id` +names a message in that chat, then advances the marker in one UPDATE that sets +it to `GREATEST(COALESCE(last_read_message_id, 0), :requested)`. Computing the +maximum in Python from a prior read would let two concurrent `POST /read/` calls +interleave and the lower id win, the regression the monotonic rule exists to +prevent; `GREATEST` inside the UPDATE makes it atomic without a lock. Accepting +any id would let a client set its marker arbitrarily high and permanently zero +its own unread counts, and persisting self-inflicted corruption is worse than +refusing the request. Unread counts the messages above the marker whose +`user_id IS DISTINCT FROM` the member's; `IS DISTINCT FROM` rather than `!=` is +load-bearing, because system messages carry `user_id IS NULL` and `NULL != 1` +evaluates to NULL, silently dropping every one of them. Advancing to a lower id +is a silent no-op, since a replayed request is not a client mistake worth +reporting. diff --git a/docs/adr/0007-upsert-via-duplicate-key-recovery.md b/docs/adr/0007-upsert-via-duplicate-key-recovery.md deleted file mode 100644 index cb9cd44..0000000 --- a/docs/adr/0007-upsert-via-duplicate-key-recovery.md +++ /dev/null @@ -1,39 +0,0 @@ -# Upsert by recovering from DuplicateKeyError - -Both `CreateChatUseCase` and `CreateMessageUseCase` read first to see whether -the row already exists, then insert. The read is an optimisation. The -correctness guarantee is the `except DuplicateKeyError:` branch, which rolls -back and re-reads the row the winner committed. - -Both recovery paths are exercised by tests, through repository subclasses -(`_RacingChatsRepository`, `_RacingMessagesRepository`) whose `create` raises -`DuplicateKeyError` and whose lookup misses once before delegating to the real -implementation. That simulates a database condition at a seam the tests already -own, rather than mocking the unit under test. - -## Rejected: the pre-check alone - -The original design assumed a read inside the transaction made the insert safe. -It does not. At READ COMMITTED two concurrent "open a DM with Bob" requests both -miss the read, both insert, and the loser violates `uq_chats_direct_key`. That -surfaces as a `409` to a user who should simply have received the existing chat, -which contradicts the reason `direct_key` exists at all. - -`@postgres_retry` does not rescue it either: `db-retry` retries serialization -and connection failures, not integrity violations. - -## Rejected: `SELECT ... FOR UPDATE` - -There is no row to lock. The race is between two inserts of a row that does not -yet exist, so row-level locking has nothing to take. - -## Consequence - -The happy path costs one extra read. The contended path costs a rolled-back -insert plus a re-read, which is strictly better than returning an error for a -request that should have succeeded. - -## Revisit trigger - -A write path where the losing racer's rollback is too expensive to accept, or a -move to an `INSERT ... ON CONFLICT` form that still needs the same re-read. diff --git a/docs/adr/0008-idempotency-scoped-per-chat.md b/docs/adr/0008-idempotency-scoped-per-chat.md deleted file mode 100644 index 348e7df..0000000 --- a/docs/adr/0008-idempotency-scoped-per-chat.md +++ /dev/null @@ -1,35 +0,0 @@ -# Idempotency is scoped per chat - -`messages` carries `UniqueConstraint("chat_id", "idempotency_key")`. The -pre-check lookup, the constraint, and the `DuplicateKeyError` recovery re-read -all filter on the same pair. - -Idempotency is a property of an operation, and the operation is "send this -message *to this chat*". Two different chats are two different operations; a key -reused across them is not a retry of anything. - -## Rejected: a global unique constraint on `idempotency_key` - -Shipped first, and it made a reachable state look unreachable. With a global -constraint and a chat-scoped lookup, a cross-chat key reuse under concurrency -raises `DuplicateKeyError` from the insert, the scoped re-read misses, and -control reaches a guard whose only justification was "the unique constraint -guarantees a match here". That guarantee no longer held. - -Keeping the constraint global while scoping only the lookup also meant a client -reusing a key across chats received the *other* chat's message and its intended -message was never written: a silent wrong-row return, which is worse than an -error. - -The three surfaces must agree. Aligning the constraint with the lookup was the -cheaper direction, and it is the one that matches the domain. - -## Consequence - -The same client-generated key may legitimately appear in two chats. Callers that -assume global uniqueness of `idempotency_key` are wrong. - -## Revisit trigger - -A cross-chat operation that must be idempotent as a unit, such as forwarding one -message into several chats in a single request. diff --git a/docs/adr/0008-repoint-last-message-on-delete.md b/docs/adr/0008-repoint-last-message-on-delete.md new file mode 100644 index 0000000..a93c8e8 --- /dev/null +++ b/docs/adr/0008-repoint-last-message-on-delete.md @@ -0,0 +1,17 @@ +# Repoint last_message_id on delete + +`DeleteMessageUseCase` soft-deletes the message and, when it was the chat's +`last_message_id`, repoints that column to the newest remaining message with +`deleted_at IS NULL`, or to `NULL` if none remains, committing both writes +together. The column therefore carries exactly one meaning, the newest +non-deleted message in this chat, and both the listing preview and the listing's +ordering (`coalesce(chats.last_message_id, 0) DESC`) read it. Adding +`deleted_at IS NULL` to the preview fetch is the smaller change and was +considered first, but it leaves two half-broken behaviours instead of one +correct one: the preview goes blank while older messages still exist, and the +chat keeps sorting by the deleted message's id, because ordering reads the same +column the preview stopped trusting. That filter is still present on the preview +relationship, as a self-defending invariant guard rather than as the mechanism. +`chats.last_message_id` stays a plain `BigInteger` rather than a foreign key, +because `chats` is created before `messages` exists and a circular constraint +pair would buy nothing at this scale. diff --git a/docs/adr/0009-coverage-exclusions-structural-only.md b/docs/adr/0009-coverage-exclusions-structural-only.md new file mode 100644 index 0000000..ce43236 --- /dev/null +++ b/docs/adr/0009-coverage-exclusions-structural-only.md @@ -0,0 +1,16 @@ +# Coverage exclusions are structural only + +The suite runs at `--cov-fail-under=100`, and exemptions are structural only: +`[tool.coverage.run] omit` for files the default pytest run never imports at +all, each carrying an inline reason in `pyproject.toml` where the path does not +explain itself, plus `exclude_also` for `if typing.TYPE_CHECKING:` blocks. No +`# pragma: no cover` appears in `app/`, `tests/` or `migrations/`. "Awkward to +reach" is not a warrant: where a branch looked untestable the answer was a +repository subclass that raises the condition the database would raise, which is +how both `DuplicateKeyError` recovery paths and both defensive `is None` guards +became executed code. Pragmas on unreachable-in-production guards were argued +for twice and refused, because an excluded branch is one nobody notices when it +stops being unreachable, and the coverage number then asserts something untrue +about what the tests exercise; tests written only to move the number were +removed for the same reason. `filterwarnings = ["error"]` enforces the companion +property, so a new warning fails a test rather than scrolling past. diff --git a/docs/adr/0009-read-marker-integrity.md b/docs/adr/0009-read-marker-integrity.md deleted file mode 100644 index eb79ade..0000000 --- a/docs/adr/0009-read-marker-integrity.md +++ /dev/null @@ -1,41 +0,0 @@ -# Read-marker integrity - -`MarkReadUseCase` does three things in order: verify membership, verify that -`last_read_message_id` names a message in *that* chat, then advance the marker -with a single statement: - -```sql -UPDATE chat_members -SET last_read_message_id = GREATEST(COALESCE(last_read_message_id, 0), :requested) -``` - -Unread is then `count(messages WHERE chat_id = ? AND id > COALESCE(marker, 0) -AND user_id IS DISTINCT FROM me AND deleted_at IS NULL)`. - -`IS DISTINCT FROM` rather than `!=` is load-bearing: system messages carry -`user_id IS NULL`, and `NULL != 1` evaluates to NULL, which silently drops every -system message from the count. A regression test asserts this. - -## Rejected: monotonicity enforced in Python - -Read the member row, compute `max(current, requested)`, write it back. Two -concurrent `POST /read/` calls interleave and the lower id wins, which is -exactly the regression the monotonic rule exists to prevent. `GREATEST` in the -UPDATE makes it atomic without a lock. - -## Rejected: accepting any id - -Without the message-in-chat check a client can set its marker to an arbitrarily -large id and permanently zero its own unread counts. Persisting self-inflicted -data corruption is worse than refusing the request. - -## Consequence - -Marking read costs one extra lookup. Advancing to a lower id is a silent no-op -rather than an error, because a replayed or out-of-order request is not a client -mistake worth reporting. - -## Revisit trigger - -Per-device read markers, or a requirement to move a marker backwards -deliberately, such as "mark as unread". diff --git a/docs/adr/0010-auth-carries-an-actor-id.md b/docs/adr/0010-auth-carries-an-actor-id.md new file mode 100644 index 0000000..9a5e71e --- /dev/null +++ b/docs/adr/0010-auth-carries-an-actor-id.md @@ -0,0 +1,17 @@ +# Authentication carries an actor id, not a user row + +`retrieve_user_handler` resolves an `Actor`, a frozen dataclass holding only the +id proved by the JWT, from `token.sub` alone, reading no row. Auth middleware +runs before request-scoped DI exists, so anything it loads comes from a session +it opens and closes itself: loading the user row there cost a second DB session +per authenticated request against `db_pool_size=5` and `db_max_overflow=0`, and +handed every use case a detached ORM instance from a closed session. Against +that, every read of `actor` across `app/use_cases/` is `actor.id`. Making the id +a UUID or uuid7 was rejected for the reason in +[ADR-0001](0001-sequence-ids-not-snowflakes.md), one writer and so no id +coordination to solve, and because it would overflow `direct_key`'s `String(64)` +and break the public `User` schema. The accepted cost is that authentication no +longer proves the user exists: a token whose row is gone still authenticates, +reads come back empty and writes hit the `messages.user_id` foreign key. Nothing +reaches that state today because there is no delete-user path, and adding one +reopens this. diff --git a/docs/adr/0010-repoint-last-message-on-delete.md b/docs/adr/0010-repoint-last-message-on-delete.md deleted file mode 100644 index 3ebd71a..0000000 --- a/docs/adr/0010-repoint-last-message-on-delete.md +++ /dev/null @@ -1,32 +0,0 @@ -# Repoint last_message_id on delete - -`DeleteMessageUseCase` soft-deletes the message and, if it was the chat's -`last_message_id`, repoints that column to the newest remaining message with -`deleted_at IS NULL`, or to `NULL` if none remains. Both writes commit together. - -The column therefore has one meaning: the newest non-deleted message in this -chat. The listing preview and the listing's ordering -(`coalesce(chats.last_message_id, 0) DESC`) both read it, and both stay correct. - -## Rejected: filtering the preview query instead - -Adding `deleted_at IS NULL` to the preview fetch is the smaller change and was -considered first. It leaves two half-broken behaviours instead of one correct -one: the preview goes blank while older messages still exist, and the chat -continues to sort by the deleted message's id, because ordering reads the same -column the preview stopped trusting. - -The filter is still present on the preview fetch, but as a self-defending -invariant guard rather than as the mechanism. - -## Consequence - -Deleting the newest message costs one extra query. `chats.last_message_id` -remains a plain `BigInteger` rather than a foreign key, because `chats` is -created before `messages` exists and a circular constraint pair would buy -nothing at this scale. - -## Revisit trigger - -A hard-delete path, a bulk delete, or any other writer of -`chats.last_message_id` that would need the same repointing logic. diff --git a/docs/adr/0011-per-user-channel-topology.md b/docs/adr/0011-per-user-channel-topology.md deleted file mode 100644 index 08206c3..0000000 --- a/docs/adr/0011-per-user-channel-topology.md +++ /dev/null @@ -1,33 +0,0 @@ -# Per-user channel topology - -Every connected client subscribes to exactly one channel, `user:{user_id}`. A -use case resolves the recipient set inside its transaction and carries it in the -event payload; the relay publishes the event once per recipient channel. - -## Rejected: per-chat channels - -`chat:{chat_id}`, one publish per event regardless of member count. Rejected -because the client must track and resubscribe to N channels as membership -changes, and lifecycle events ("you were added to a chat") have no channel to -arrive on until the client already knows about the chat. Solving that requires a -per-user control channel, so the design ends up with both topologies and the -simplicity of neither. - -## Rejected: hybrid - -`user:{id}` for chat lifecycle and unread counts, `chat:{id}` for message -traffic. This is the correct choice at scale, because it bounds publish -amplification where the volume is. Rejected for a reference application: the -subscription lifecycle grows a resubscribe path in both the service and the -client, and that machinery obscures the outbox and DI patterns the repo exists -to show. - -## Consequence - -Publish amplification is O(members) per message. Irrelevant at demo scale, -material in production. - -## Revisit trigger - -A group chat exceeding ~100 members, or a measured relay bottleneck in the -fan-out loop. diff --git a/docs/adr/0012-no-server-side-event-replay.md b/docs/adr/0012-no-server-side-event-replay.md deleted file mode 100644 index 0af1252..0000000 --- a/docs/adr/0012-no-server-side-event-replay.md +++ /dev/null @@ -1,33 +0,0 @@ -# No server-side event replay - -On reconnect the client recovers in two layers. Fast path: -`RedisChannelsStreamBackend(history=N)` replays the last N events on the channel -at subscribe time. Correctness path: on every connect the client refetches -`GET /api/chats/` and `GET /api/chats/{id}/messages/?after_id=` -for the open chat, which resyncs state regardless of what the stream did. - -SSE `id:` is set on message events, so the browser sends `Last-Event-ID` on -reconnect. It is treated as a hint that seeds the resync, not as a replay -cursor. - -## Rejected: durable per-user event log - -A table of events per user with monotonic ids, letting the SSE endpoint honour -`Last-Event-ID` by replaying exactly the missed range. This is the correct -answer for a system that must not drop an event. - -Rejected because Litestar's channel history is a Redis stream keyed by its own -ids and is not addressable by ours, so honouring the header properly means -bypassing channel history entirely and owning the log. That is a second delivery -mechanism alongside the outbox, and the resync path already makes the client -correct at a fraction of the machinery. - -## Consequence - -A client disconnected longer than the channel history window recovers by -refetching rather than by replay. Correct, but O(state) instead of O(missed). - -## Revisit trigger - -Clients that cannot afford a full resync (mobile on metered connections), or an -event type whose effect cannot be reconstructed from current state. diff --git a/docs/adr/0013-coverage-exclusions-structural-only.md b/docs/adr/0013-coverage-exclusions-structural-only.md deleted file mode 100644 index 79d659a..0000000 --- a/docs/adr/0013-coverage-exclusions-structural-only.md +++ /dev/null @@ -1,44 +0,0 @@ -# Coverage exclusions are structural only - -The suite runs at `--cov-fail-under=100`. Two mechanisms can exempt code, and -each has a narrow warrant: - -- `[tool.coverage.run] omit` is reserved for files the default pytest run never - imports at all. The live list is `omit` in `pyproject.toml`, which carries an - inline reason wherever the exclusion is not evident from the path itself; - enumerating it here would be a copy that goes stale. -- `# pragma: no cover` is not used anywhere in `app/`, `tests/` or - `migrations/`. - -"Awkward to reach" is not a warrant. Where a branch looked untestable, the -answer was a repository subclass that raises the condition the database would -raise. That is how both `DuplicateKeyError` recovery paths and both defensive -`is None` guards became executed code. - -`filterwarnings = ["error"]` enforces the companion property: the suite runs at -zero warnings, and a new warning fails a test rather than scrolling past. - -## Rejected: pragmas on unreachable-in-production guards - -Argued for twice during implementation, on the grounds that the guards cannot -fire while the unique constraint holds. Rejected because an excluded branch is -one nobody notices when it stops being unreachable, and because the coverage -number then asserts something untrue about what the tests exercise. The two -guards in question were reachable through a seam the tests already owned. - -## Rejected: tests written only to move the number - -Also seen and removed: an `assert __name__ != "__main__"` that could not fail, -and an `isinstance` check against a function whose body constructs that type. -The gate exists to make untested code visible; satisfying it with assertions -that cannot fail defeats it more thoroughly than a lower number would. - -## Consequence - -Adding genuinely unexecutable code requires an `omit` entry with a stated -reason, reviewed as a decision rather than applied inline. - -## Revisit trigger - -A dependency that emits an unfixable warning, or platform-specific code paths -that cannot run in CI. diff --git a/docs/adr/0014-auth-carries-an-actor-id.md b/docs/adr/0014-auth-carries-an-actor-id.md deleted file mode 100644 index cffcd29..0000000 --- a/docs/adr/0014-auth-carries-an-actor-id.md +++ /dev/null @@ -1,56 +0,0 @@ -# Authentication carries an actor id, not a user row - -**Decision:** `retrieve_user_handler` resolves an `Actor` — a frozen dataclass -holding the `id` proved by the JWT — from `token.sub` alone, reading no row. - -Auth middleware runs before request-scoped DI exists, so anything it loads must -come from a session it opens and closes itself. Loading the user row there cost -a second DB session per authenticated request against `db_pool_size=5` / -`db_max_overflow=0`, and handed every use case a detached ORM instance from a -closed session — safe only because every column happened to be loaded and -nobody mutated it. Against that, all ten reads of `actor` across -`app/use_cases/` were `actor.id`. `Actor` lives in `app/actor.py`, a top-level -module, so `app/use_cases/` never imports `app.api`. - -## Rejected: loading the user row in middleware - -The shape this replaces. Its one real benefit was that authentication proved -the user still existed; see Consequence for why that is not worth a session. -`GET /api/auth/me/` is the only consumer of the full row and now fetches it -through `FetchUserUseCase` on the request-scoped session, like every other -read. Reintroducing the lookup is invisible in API responses either way, which -is why the claim is pinned by -`test_retrieve_user_handler_resolves_an_actor_without_reading_the_database` -rather than by an endpoint test. - -## Rejected: UUID or uuid7 user ids - -Raised as the alternative to passing a bare integer around. There is one -writer, so there is no id-coordination problem to solve. `direct_key` -(`String(64)`) no longer fits two ids; three FK columns widen 8→16 bytes, one -of them indexed (`chat_members.user_id`); and `schemas.User.id` and -`member_ids` become a breaking API change. uuid7 is also the wrong tool for the -benefit usually wanted here — it publishes registration time in its first 48 -bits. [`0001-sequence-ids-not-snowflakes.md`](0001-sequence-ids-not-snowflakes.md) -already carries the ordering argument for message ids. - -## Rejected: an opaque public id - -Deferred rather than refused. If opacity is ever wanted, the shape is the -two-id pattern — the BigInt PK stays internal, a separate opaque public id -faces outward — which is strictly additive on top of this decision. Adopting it -now would buy nothing and cost a second identifier to keep in agreement. - -## Consequence - -Authentication no longer proves the user exists. A token whose row is gone -authenticates: reads come back empty, writes hit the `messages.user_id` foreign -key. Nothing can reach that state today — there is no delete-user or -disable-user path — and the accepted cost is recorded in the invariant test's -docstring, not as an open issue. - -## Revisit trigger - -A delete-user or disable-user path being added. It meets the same problem as -[#20](https://github.com/modern-python/chat-app/issues/20) — a credential -outliving what it names — and both should be solved once, together. diff --git a/docs/adr/README.md b/docs/adr/README.md index 9f78b24..214b568 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -25,7 +25,7 @@ When a later ADR supersedes an earlier one, add to the earlier file: ```yaml --- -superseded_by: 0014-its-slug +superseded_by: 0010-its-slug --- ``` @@ -44,30 +44,15 @@ All three must be true, or it is not an ADR: ```md # One-line capitalized title -**Decision:** What was decided, in a sentence. - -What the code actually does, and the constraint that forced it. - -## Rejected: deriving it from the environment - -Why it was not taken. Enough that a future explorer does not re-litigate it. - -## Rejected: defaulting to True - -One heading per alternative, named in the heading so it gets its own anchor. - -## Consequence - -The non-obvious downstream effect, including what this deliberately leaves -uncovered. - -## Revisit trigger - -The concrete signal that should reopen this decision. +One paragraph: what the context is, what was decided, and why, naming the +rejected alternative where the rejection is not obvious. Typically 60 to 150 +words. ``` -`## Consequence` is optional. `## Revisit trigger` is required; a reviewer is -what enforces it. +That is the whole record: no `**Decision:**` line, no `## Rejected:` headings, +no `## Consequence`, no `## Revisit trigger` section. A consequence or a +revisit condition earns a sentence in the paragraph only when it is the real +boundary of the decision; a reviewer is what enforces that. ## Where other facts go diff --git a/docs/agents/domain.md b/docs/agents/domain.md index 1ec1863..c0e567a 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -5,7 +5,7 @@ How the engineering skills should consume this repo's domain documentation when ## Before exploring, read these - **`CONTEXT.md`** at the repo root. -- **`docs/adr/`**: read ADRs that touch the area you're about to work in. `docs/adr/README.md` carries the local standard, which is stricter than the stock ADR format: rejected alternatives and a revisit trigger are required, not optional. +- **`docs/adr/`**: read ADRs that touch the area you're about to work in. `docs/adr/README.md` carries the local standard: one paragraph per record, naming the rejected alternative inside it where the rejection is not obvious. If any of these files don't exist, **proceed silently**. Don't flag their absence; don't suggest creating them upfront. The `/domain-modeling` skill (reached via `/grill-with-docs` and `/improve-codebase-architecture`) creates them lazily when terms or decisions actually get resolved.