diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000..30e5ce1 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,48 @@ + + +## Why + +The problem or need. What is wrong, missing, or costly today — not what you did +about it. + +## Design + +The approach, and the trade-off it takes. Show a sketch if the design needs code; +never the full diff-to-be. Most PRs fit well under ~700 words — length must buy +information. + +## Non-goals + +What this deliberately does **not** do, and why. This is the scope boundary that +stops "why didn't you also fix X" in review and six months from now. + +## Verification + +How you know it works: the tests added, `just test` (give the pass count and the +coverage your run reported — 100% line coverage is the gate), +`just test-migrations`, `just lint`. State the numbers, not "tested". + +--- + +### Before merging + +- [ ] **Behaviour changed?** If a wrong change here could pass silently, pin it + with a test whose name is the claim and whose docstring opens `INVARIANT:` + and says what breaks it. Do **not** write prose about mechanism — there is + no page for it. See [`planning/README.md`](../planning/README.md#where-a-fact-goes). +- [ ] **Adding a fact anywhere?** Run the admission check: derivable from `app/` + → don't write it; enforceable → a test; otherwise it does not get written. +- [ ] **Rejected an alternative** with reasoning that would otherwise be + re-litigated? File it in [`planning/decisions/`](../planning/decisions/) + with a revisit trigger — not here. +- [ ] **Found real work you are not doing now?** File it in + [`planning/deferred/`](../planning/deferred/), self-contained, with a + revisit trigger — not here. +- [ ] `just lint`, `just check-planning`, `just check-links`, `just test` and + `just test-migrations` all pass. diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 2c3daaf..ee43506 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -24,6 +24,7 @@ jobs: uv run ruff check . --no-fix uv run ty check uv run python planning/index.py --check + uv run python planning/links.py pytest: runs-on: ubuntu-latest diff --git a/CLAUDE.md b/CLAUDE.md index 1087ce0..4c1dc4d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,8 +26,7 @@ template breaks the transaction model or the DI wiring. `chats.last_message_id` update together. This exists because a single operation can span more than one repository write and they must succeed or fail as a unit; giving that back to individually auto-committing - repositories would make that impossible. See `architecture/messages.md` - and `architecture/chats.md` for the two hazards this creates around + repositories would make that impossible. This creates a hazard around `Transaction.__aexit__`'s unconditional rollback-on-open-transaction behavior (returning a loaded ORM object from inside an uncommitted `async with self.transaction:` block detaches it). @@ -39,9 +38,10 @@ only covers what isn't obvious from the recipe names. Almost everything runs through Docker Compose: the app and Postgres come up together, and running tests/migrations outside Docker is **not** the -supported path (`just install` and `just lint` are the exceptions — they run -on the host). Inside the container, raw commands look like `uv run pytest -...`, `uv run alembic ...`. +supported path (`just install`, `just lint`, `just index`, `just +check-planning` and `just check-links` are the exceptions — they run on the +host). Inside the container, raw commands look like `uv run pytest ...`, `uv +run alembic ...`. - `just test` cycles the DB (downgrade to `base`, upgrade to `head`) before pytest and tears the stack down before and after. Pass pytest args through, @@ -66,12 +66,34 @@ on the host). Inside the container, raw commands look like `uv run pytest - `just lint` runs `eof-fixer`, `ruff format`, `ruff check --fix`, then `ty check` — this project uses `ty`, not mypy; suppress with `# ty: ignore[]` (not `# type: ignore`). -- `just index` prints the planning change/decision listing; `just - check-planning` validates `planning/changes/` and `planning/decisions/` - frontmatter (CI-equivalent check, run before pushing a planning change). +- `just index` prints the deferred/decision listing; `just check-planning` + validates `planning/deferred/` and `planning/decisions/` frontmatter (and + that every deferred item carries a revisit trigger); `just check-links` + validates every relative Markdown link and heading anchor in the repo. Python is 3.14, dependencies managed by `uv`. The API is exposed on `:8000`. +## Workflow + +**The spec for a change is its PR body**, not a committed file. +`.github/PULL_REQUEST_TEMPLATE.md` carries the shape (why, design, non-goals, +verification); it is reviewed with the diff. There is no change file and no lane +to choose. A trivial PR (typo, dep bump, formatter) deletes the template and +ships a conventional-commit title. + +Two things outlive the PR and are committed under `planning/`: an alternative +**rejected** with reasoning goes to `planning/decisions/`, and real work **not +scheduled** goes to `planning/deferred/` (self-contained, with a revisit +trigger). There is no capability-page home — the living truth about behaviour is +the code and its `INVARIANT:`-marked tests, and a behaviour change is reviewed +with the diff, not promoted to a page. See `planning/README.md` for the full +convention, including the admission check that decides where a given fact +belongs. + +An invariant is a test whose name is the claim, with a docstring opening +`INVARIANT:` and a second paragraph naming what breaks it. Applied to new +claims; the existing suite is not retrofitted. + ## Architecture **Stack**: Litestar + SQLAlchemy 2 (async) + advanced-alchemy + Alembic + @@ -128,10 +150,11 @@ back. `app`/`client` fixtures build the real app and run it through `modern_di_pytest.expose(ioc.Repositories, ioc.UseCases, container_fixture="request_container")` (`tests/use_cases/conftest.py`) exposes every repository/use case provider as a same-named pytest fixture — -the template predates this and hand-assembles dependencies instead. Full -detail, including the race-simulation pattern used to test the -concurrent-retry paths without a second real connection, is in -`architecture/testing.md`. +the template predates this and hand-assembles dependencies instead. The +race-simulation pattern used to test the concurrent-retry paths without a second +real connection is the `_Racing*Repository` classes in +`tests/use_cases/test_create_chat.py` and `tests/use_cases/test_create_message.py`; +the invariant each one pins is in the `INVARIANT:` docstring on the test that uses it. **Migrations**: `migrations/env.py` reads the shared `METADATA` and rewrites the DSN driver from `postgresql+asyncpg` → `postgresql` (Alembic uses sync @@ -169,14 +192,15 @@ env vars (see `docker-compose.yml`). `api_bootstrapper_config` builds the - Domain exceptions (`app/exceptions.py`: `PermissionDeniedError`, `ValidationError`, `ConflictError`) are registered as handlers in `build_app`'s `exception_handlers` dict alongside the `advanced_alchemy` - exceptions (`NotFoundError`, `DuplicateKeyError`, `ForeignKeyError`). Full - mapping table and the one deliberate exception (login's `401` via Litestar's - own `NotAuthorizedException`) are in `architecture/messages.md` and - `architecture/auth.md`. + exceptions (`NotFoundError`, `DuplicateKeyError`, `ForeignKeyError`). Every + mapping, and why login's `401` deliberately uses Litestar's own + `NotAuthorizedException` instead, is described in + `planning/decisions/2026-08-21-domain-error-vocabulary.md`. - **Comments.** None, unless the code would read as a bug without one; then a single line. Rationale, design decisions and "why not X" belong in - `architecture/.md` and `planning/changes/`, never in the source — - those are the durable homes, and a comment restating them goes stale in place. + `planning/decisions/` and the PR body, never in the source — those are where + such reasoning is reviewed and kept, and a comment restating it goes stale in + place. What survives in `app/` today is the whole permitted category: a setting that looks arbitrary (`join_transaction_mode`, `populate_existing`, `capture_parameters=False`), an `orm.foreign()` on a column with no @@ -186,3 +210,29 @@ env vars (see `docker-compose.yml`). `api_bootstrapper_config` builds the - `ruff` is configured with `select = ["ALL"]` and a line length of 120 — expect strict lint. Type-check with `ty`; use `# ty: ignore[]` for suppressions. + +## Vocabulary + +A term is listed only when there is a synonym to reject, or a meaning subtle +enough that code and docs must agree on it. + +- **Chat** — a row in `chats`: a type (`direct` or `group`), an optional title, + its creator, and a pointer to its newest non-deleted message. *Avoid:* + conversation, room, thread. +- **Direct chat** — a chat between exactly two users, identified by `direct_key`, + the canonical `min(user_id):max(user_id)` string under a unique constraint. + That key is what makes opening one twice an upsert instead of a read-then-race. + *Avoid:* DM, 1:1. +- **Member** — the `(chat_id, user_id)` row granting access to a chat, plus that + user's read marker. Necessary for every read or write on a chat; not + sufficient for editing or deleting a message. *Avoid:* participant, subscriber. +- **Idempotency key** — the client-supplied UUID on a send, unique per + `(chat_id, idempotency_key)`. Scoped to one chat, because the key identifies a + retry of "send this message to this chat". *Avoid:* dedupe key, request id. +- **Unread** — a count computed at read time against one marker per member, not + a set of per-message receipt rows. *Avoid:* unseen, badge count. +- **Cursor** — a message id passed as `before_id` or `after_id`. The two are + mutually exclusive on one request. *Avoid:* page token, offset. +- **Read marker** — a member's `last_read_message_id`, the highest id they have + acknowledged. Advances only forward, via `GREATEST` inside the UPDATE. *Avoid:* + read receipt, watermark. diff --git a/Justfile b/Justfile index 529bf1c..fe359c5 100644 --- a/Justfile +++ b/Justfile @@ -35,10 +35,14 @@ lint: uv run ruff check . --fix uv run ty check -# Print the planning change index (flat, newest-first) to stdout. +# Print the planning index (deferred, then decisions) to stdout. index: uv run python planning/index.py -# Validate planning changes + decisions (frontmatter, lanes, spec links); CI runs this. +# Validate planning/deferred/ + planning/decisions/ frontmatter and naming; CI runs this. check-planning: uv run python planning/index.py --check + +# Check every relative Markdown link and heading anchor in the repo. +check-links: + uv run python planning/links.py diff --git a/app/api/auth.py b/app/api/auth.py index c57264f..31631ab 100644 --- a/app/api/auth.py +++ b/app/api/auth.py @@ -18,7 +18,7 @@ async def retrieve_user_handler(token: Token, connection: ASGIConnection) -> tables.UsersTable | None: - # Auth middleware runs before request-scoped DI exists; see architecture/auth.md. + # Auth middleware runs before request-scoped DI exists, so this opens its own session. try: user_id = int(token.sub) except ValueError: diff --git a/app/settings.py b/app/settings.py index ce8cb86..08076b7 100644 --- a/app/settings.py +++ b/app/settings.py @@ -13,6 +13,7 @@ class Settings(pydantic_settings.BaseSettings): service_name: str = "chat-app" service_version: str = "1.0.0" service_environment: str = "local" + # echo/echo_pool log bound parameters (password_hash on registration); Litestar returns stack traces in responses. service_debug: bool = False log_level: str = "info" diff --git a/architecture/README.md b/architecture/README.md deleted file mode 100644 index 28b8c74..0000000 --- a/architecture/README.md +++ /dev/null @@ -1,24 +0,0 @@ -# Architecture - -The living truth about what `chat-app` does **now** — one file per capability, -updated by hand whenever a change ships. The *why* and *how it got here* live -in [`../planning/changes/`](../planning/changes/), and decisions deliberately -taken (including options rejected) in -[`../planning/decisions/`](../planning/decisions/); this directory is the -present. - -These files carry **no frontmatter** — they are prose, dated by git. - -## Capabilities - -- [auth.md](auth.md) — registration, login, the JWT cookie, `retrieve_user_handler`. -- [chats.md](chats.md) — direct/group chats, the direct-chat upsert, membership. -- [messages.md](messages.md) — idempotent send, cursor pagination, edit/delete authorization, unread counts. -- [testing.md](testing.md) — the per-test rollback fixture, DI-fixture exposure, the race-simulation pattern. -- [glossary.md](glossary.md) — the domain's ubiquitous language. - -## Promotion rule - -Shipping a change hand-edits the affected capability file(s) here to match the -new reality, in the same PR as the code. The change file stays in place under -[`../planning/changes/`](../planning/changes/) — no folder move. diff --git a/architecture/auth.md b/architecture/auth.md deleted file mode 100644 index 826f653..0000000 --- a/architecture/auth.md +++ /dev/null @@ -1,91 +0,0 @@ -# Auth - -Litestar's `JWTCookieAuth[UsersTable]` (`app/api/auth.py`), configured with -`token_secret=settings.jwt_secret` and a 7-day default expiration -(`jwt_lifetime_seconds`). Cookie rather than bearer header: a browser -`EventSource` (planned for the realtime follow-on) cannot set an -`Authorization` header, so the cookie is the one auth variant every endpoint — -REST today, SSE later — can share identically. - -## Registration and login - -`POST /api/auth/register/` (`app/api/endpoints/auth.py::register`) runs -`RegisterUserUseCase`, which hashes the password with `argon2` (`app/security.py`) -inside its own transaction and returns `201` with the cookie set via -`jwt_cookie_auth.login`. A duplicate username raises `DuplicateKeyError` from -the unique constraint on `users.username`, mapped to `409` by the app-wide -handler — there is no auth-specific duplicate check. - -Two settings exist to keep those hashes out of telemetry. `service_debug` -(`app/settings.py`) turns on SQLAlchemy's `echo`/`echo_pool`, which log every -statement *with its bound parameters* — including the `password_hash` on every -registration — and additionally make Litestar return stack traces in responses; -it must stay `False` outside a throwaway local session. `AsyncPGInstrumentor` is -constructed `capture_parameters=False` (`app/api/app.py`) for the same reason on -the OpenTelemetry side. - -`POST /api/auth/login/` runs `AuthenticateUserUseCase`, which looks the user up -by username and verifies the password hash. On failure — unknown username or -wrong password — it raises Litestar's own `NotAuthorizedException` (`401`), -not `app.exceptions.PermissionDeniedError`: this is the one place the -`litestar.exceptions` vocabulary is used directly, because login failure is -not an authorization decision to gate downstream of an already-identified -actor, it *is* the identification step. On success it returns `200` (not -`201` — nothing was created) with a fresh cookie. - -`AuthenticateUserUseCase` hashes the submitted password even when the -username doesn't exist (`app/use_cases/authenticate_user.py`) specifically so -an unknown-username response isn't measurably faster than a -wrong-password response — skipping the argon2 work would turn login into a -username oracle. - -`POST /api/auth/logout/` deletes the cookie and returns `204`. It does not -revoke the JWT: a token copied before logout stays valid for the rest of its -lifetime, because no `revoked_token_handler` is configured on -`jwt_cookie_auth`. See `planning/deferred.md`. - -Both `register` and `login` opt out of the auth middleware with -`exclude_from_auth=True` on the handler, not through `jwt_cookie_auth`'s -`exclude` list — that list is reserved for path-shaped exclusions, each -anchored with `^` so a future route merely containing `/docs` as a path -segment isn't accidentally deauthenticated. - -The anonymous surface is therefore exactly four prefixes: `/docs` and -`/health`, plus `/static` (Swagger's offline assets, served from there -because `swagger_offline_docs` is on — without the exclusion the docs page -loads but every asset request 401s) and `/metrics` (a Prometheus scrape -target must be reachable without a session cookie; it carries process and -request metrics, no user data). - -## Request-time identity - -`retrieve_user_handler` (`app/api/auth.py`) runs inside Litestar's auth -middleware, which executes *before* request-scoped DI is available. It cannot -resolve a use case or repository, so it resolves the app-scoped -`Database.database_engine` provider directly off the DI container -(`modern_di_litestar.fetch_di_container(connection.app)`) and opens its own -short-lived session through the same `database_resources.create_session` -factory the container uses, then closes it in a `finally`. This means every -authenticated request opens **two** sessions — one here, one for the -request-scoped repositories — against a pool sized `db_pool_size=5`, -`db_max_overflow=0`. See `planning/deferred.md`. - -`Token.sub` is only guaranteed to be a non-empty string; `retrieve_user_handler` -converts it with `int(token.sub)` and returns `None` (→ `401` via the -middleware) on `ValueError` rather than letting a forged or malformed subject -crash the request. A validly signed token whose subject names a user that no -longer exists resolves to `None` from `session.get` the same way. - -`GET /api/auth/me/` returns the authenticated `request.user` — no separate use -case, since the middleware has already loaded it. - -## Configuration - -`jwt_cookie_secure` (`app/settings.py`) defaults `False` so local `http://` -development still receives the cookie; it must be `True` in any deployment -served over HTTPS. `Settings.ensure_jwt_secret_is_configured`, called at the -top of `build_app`, raises `RuntimeError` at startup if -`service_environment != "local"` and `jwt_secret` is still the shipped -`INSECURE_JWT_SECRET` — the whole auth boundary is a token signed with that -secret, so running any non-local environment on the default would let anyone -forge a token for any `user.id`. diff --git a/architecture/chats.md b/architecture/chats.md deleted file mode 100644 index 55529da..0000000 --- a/architecture/chats.md +++ /dev/null @@ -1,106 +0,0 @@ -# Chats - -## Shape - -`ChatsTable` (`app/database/tables.py`): `chat_type`, optional `title` -(group chats only — `CreateChatUseCase` forces it to `None` for direct chats -even if the request supplied one), `created_by_id`, `last_message_id` -(nullable, repointed by message send/delete — see `messages.md`), and -`direct_key` (nullable, unique). `chat_type` is stored as `sa.Enum(ChatType, -native_enum=False, create_constraint=True, values_callable=...)` — a `VARCHAR` -plus a `CHECK` constraint storing the lowercase string values (`"direct"`, -`"group"`), not a native Postgres enum type. A native enum would need -`alembic-postgresql-enum` for autogenerate to emit correct `ALTER TYPE` -migrations, a dependency not worth buying to store two values. See -`planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md` for the -adjacent id-strategy call. - -`ChatMembersTable` is `(chat_id, user_id)` under `uk_chat_members_chat_id_user_id`, -plus `last_read_message_id` and `joined_at`. - -## Creating a chat - -`POST /api/chats/` → `CreateChatUseCase` (`app/use_cases/create_chat.py`). -`member_ids` from the request is unioned with the actor's own id, so the -creator is always a member even if they omitted themselves. - -**Direct** (`chat_type = "direct"`) requires the union to resolve to exactly -two distinct users (`ValidationError` → `400` otherwise), builds -`direct_key = build_direct_key(low, high)`, and checks -`fetch_direct_by_key` first: if a direct chat for this pair already exists, -it's returned as-is with `created=False` (→ `200`). This pre-check does not -close the race — two concurrent requests can both miss it before either -commits. The `INSERT` itself is the real guard: it hits `uq_chats_direct_key`, -and the loser catches `DuplicateKeyError`, rolls back, and re-reads -`fetch_direct_by_key` to return the winner's row. **Group** chats have no -unique constraint on `chats` to collide on, so an unexpected -`DuplicateKeyError` from a group-chat insert is not funnelled into this -recovery path — it re-raises and maps to the standard `409`. - -The rollback-then-reread shape (here and in `CreateMessageUseCase`, see -`messages.md`) exists because `Transaction.__aexit__` unconditionally rolls -back and closes the session on an open, uncommitted transaction, which expires -every loaded attribute — returning the just-loaded row from *inside* the -`async with self.transaction:` block without a preceding `commit()` would hand -the caller a detached object. - -## Membership and 403-vs-404 - -Every chat- and message-scoped use case checks membership before doing -anything else (`chat_members_repository.is_member` / -`fetch_member`), and a non-member gets `PermissionDeniedError` → `403` — not -`404`. `FetchChatUseCase` (`app/use_cases/fetch_chat.py`) deliberately returns -`403` for a chat that exists but that the actor isn't in, rather than `404` -pretending it doesn't exist; other use cases follow the same posture for -consistency. One accepted consequence: a non-member can distinguish an -existing message id from a nonexistent one via `404` vs `403` on -`PATCH`/`DELETE /api/messages/{id}/` (see `messages.md` and -`planning/deferred.md`). - -## Listing and unread counts - -`GET /api/chats/` → `FetchChatsUseCase` (`app/use_cases/fetch_chats.py`), backed -by `ChatsRepository.list_for_user`, which returns plain `ChatsTable` instances -carrying two extra attributes mapped for exactly this query — so the endpoint -validates them straight through `schemas.Chats.from_models(...)` with no -per-row DTO in between. Unread count is a correlated scalar -subquery per row, not a Python loop: `count(messages WHERE chat_id = ? AND id -> COALESCE(member.last_read_message_id, 0) AND user_id IS DISTINCT FROM -member.user_id AND deleted_at IS NULL)`, joined against `chat_members` and -ordered by `COALESCE(last_message_id, 0) DESC` so the most recently active -chat sorts first (a chat with no messages yet sorts last, not first). `IS -DISTINCT FROM` rather than `!=` matters because system messages carry -`user_id IS NULL`, and `NULL != me` evaluates to `NULL` in SQL, which would -silently drop every system message from the count. - -The count reaches the ORM instance through `ChatsTable.unread_count`, an -`orm.query_expression()` that `list_for_user` fills with `with_expression(...)`; -any other query that loads a chat gets its `default_expr` literal `0`, so the -attribute is never `None`. The preview comes from `ChatsTable.last_message`, a -`viewonly` many-to-one on `last_message_id` loaded by one `selectinload` — one -extra round trip for the whole page, not one per row. `last_message_id` carries -no `ForeignKey` (that would close a cycle with `messages.chat_id`), so the -relationship annotates the join column `orm.foreign()` by hand and folds -`deleted_at IS NULL` into its `primaryjoin`: a self-defending guard, not the -source of truth, since `DeleteMessageUseCase` already repoints -`last_message_id` off a deleted message in the same commit as the delete (see -`messages.md`). - -`list_for_user` runs with `populate_existing=True`. Sessions are built -`expire_on_commit=False`, so a `ChatsTable` already in the identity map would -otherwise keep the `unread_count` and `last_message` it was first loaded with, -and a second listing in the same session would hand back the first one's -numbers. That is safe only because this query is read-only — `populate_existing` -overwrites in-memory state on the entities it returns. - -## Marking read - -`POST /api/chats/{id}/read/` → `MarkReadUseCase` (`app/use_cases/mark_read.py`). -The requested `last_read_message_id` must name a real message in *this* chat -— `ValidationError` → `400` otherwise, since accepting an arbitrary id would -let a client zero its own unread count by naming a message from another chat -or one that doesn't exist. The marker only ever advances: `ChatMembersRepository.mark_read` -computes `GREATEST(COALESCE(current, 0), requested)` inside the `UPDATE` -itself rather than in Python from a prior read, so two concurrent `POST -/read/` calls can't race a read-modify-write and let the lower id win — the -row lock on the `UPDATE` serializes them. diff --git a/architecture/glossary.md b/architecture/glossary.md deleted file mode 100644 index ce5ccd0..0000000 --- a/architecture/glossary.md +++ /dev/null @@ -1,62 +0,0 @@ -# Glossary - -The project's ubiquitous language — the domain terms that code, specs, and -capability pages share. Living prose, no frontmatter, dated by git. Each entry -is a term, what it *is* (not what it does), and the synonyms to avoid. - -**Chat**: -A row in `chats`: a `chat_type` (`direct` or `group`), an optional `title`, -the `id` of the user who created it, and a pointer (`last_message_id`) to its -newest non-deleted message. Owns a set of `Member` rows through `chat_members`. -_Avoid_: conversation, room, thread - -**Direct chat**: -A `Chat` with `chat_type = "direct"` between exactly two users, identified by -`direct_key` — the canonical `min(user_id):max(user_id)` string under a unique -constraint. Opening a direct chat with the same pair twice returns the same -row; the key is what makes that an upsert instead of a read-then-race. A -`group` chat has no `direct_key` and no member-count ceiling. -_Avoid_: DM, 1:1 - -**Member**: -A row in `chat_members`: the `(chat_id, user_id)` pair that grants access to a -`Chat`, plus that user's `last_read_message_id`. Membership is what -`is_member`/`fetch_member` check before any read or write on a chat is -authorized; it is necessary but, for editing or deleting a message, not -sufficient — see `Read marker`. -_Avoid_: participant, subscriber - -**Idempotency key**: -The client-supplied `idempotency_key` (a UUID) on a send-message request, -unique per `(chat_id, idempotency_key)` — scoped to one chat, not global, -because the key identifies a retry of "send this message to this chat," and -the same key reused in a different chat is a second, independent send. A -repeated key returns the first send's row with `200` instead of creating a -second one with `201`. -_Avoid_: dedupe key, request id - -**Unread**: -A message counted by `chats_repository.list_for_user`'s correlated subquery: -`id > member.last_read_message_id` (treating `NULL` as `0`), not authored by -the viewing member (`user_id IS DISTINCT FROM`, so system messages with -`user_id IS NULL` still count), and not soft-deleted. There is no per-message -receipt row — unread is a count computed at read time against one marker per -member, not a set of rows written per message per recipient. -_Avoid_: unseen, badge count - -**Cursor**: -A message `id` passed as `before_id` or `after_id` to page `GET -.../messages/`. `before_id` returns older messages, newest-first, excluding -the cursor row; `after_id` returns newer messages, oldest-first, excluding the -cursor row. The two are mutually exclusive on one request. Message ids are a -Postgres identity sequence, so "greater id" is a total order a cursor can walk -without an offset. -_Avoid_: page token, offset - -**Read marker**: -A member's `last_read_message_id` — the highest message id that member has -acknowledged reading in that chat. Advanced only forward: `mark_read` sets it -to `GREATEST(current, requested)` inside the UPDATE itself, so an out-of-order -or replayed request naming an earlier message can never move it backwards and -resurrect messages that were already read. -_Avoid_: read receipt, watermark diff --git a/architecture/messages.md b/architecture/messages.md deleted file mode 100644 index 7231d7e..0000000 --- a/architecture/messages.md +++ /dev/null @@ -1,107 +0,0 @@ -# Messages - -## Shape - -`MessagesTable` (`app/database/tables.py`): `chat_id`, nullable `user_id` -(`NULL` for system messages, e.g. "Bob joined" — no sentinel user), an -`idempotency_key` (UUID), `text`, `created_at`, nullable `edited_at` / -`deleted_at`. `ix_messages_chat_id_id` is a composite index on `(chat_id, id)` -— there is no standalone index on `chat_id` alone, because every query that -would use one (membership-scoped listing, cursor pagination) is already served -by the composite. `uk_messages_chat_id_idempotency_key` is a unique constraint -on `(chat_id, idempotency_key)`, **not** a global unique constraint on the key -alone — see `Idempotency key` in `glossary.md`. - -## Sending: idempotent with a concurrent-retry fallback - -`POST /api/chats/{id}/messages/` → `CreateMessageUseCase` -(`app/use_cases/create_message.py`). After the membership check, it pre-reads -`fetch_by_idempotency_key(chat_id, key)`; a hit returns that row with -`created=False` (→ `200`) without writing anything. A miss proceeds to -`INSERT`, then updates `chats.last_message_id` to the new message's id in the -same commit, and returns `created=True` (→ `201`). - -The pre-check does not close the race between two concurrent sends of the -same key: both can miss it before either commits. The unique constraint is -the real guard — the loser's `INSERT` raises `DuplicateKeyError`, which is -caught, the transaction is rolled back (`await self.transaction.rollback()`, -not a `return` from inside the `async with` block — see the same -`__aexit__`-detaches-loaded-attributes hazard documented in `chats.md`), and -the loser re-reads `fetch_by_idempotency_key` outside the block to return the -winner's row with `created=False`. If that re-read still finds nothing, it's -treated as impossible (`RuntimeError`, `# pragma: no cover`) — the unique -constraint that just fired guarantees a matching row exists. - -Reusing the same key in a different chat is a second, independent send: -idempotency is scoped `(chat_id, key)` because the key identifies a retry of -"send to this chat," not a retry across the table. - -## Pagination - -`GET /api/chats/{id}/messages/` → `FetchMessagesUseCase` -(`app/use_cases/fetch_messages.py`) → `MessagesRepository.list_page`. -`before_id` and `after_id` are mutually exclusive (`ValidationError` → `400` -if both are set). `before_id` returns strictly older messages, newest-first, -excluding the cursor row itself; `after_id` returns strictly newer messages, -oldest-first, excluding the cursor row — the ascending form exists for -resync-on-reconnect in the realtime follow-on and is why the composite index -is shaped the way it is. `limit` is clamped to `MAX_PAGE_SIZE = 100` (silently -capped, not rejected) but rejected outright below `1` (`ValidationError` → -`400`). All variants filter `deleted_at IS NULL` — a soft-deleted message -disappears from every listing on its next fetch rather than appearing as a -tombstone. - -## Edit and delete: author **and** member - -`PATCH /api/messages/{id}/` and `DELETE /api/messages/{id}/` are gated by -`fetch_message_for_author` (`app/use_cases/message_authorization.py`), shared -by `EditMessageUseCase` and `DeleteMessageUseCase` so the check order is -defined in exactly one place: existence (`get_one` → `NotFoundError` → `404`), -then chat membership (→ `403`), then authorship — `message.user_id != -actor.id` (→ `403`). Authorship alone is not sufficient: an author who has -been removed from the chat (membership deleted) can no longer edit or delete -their own message, because the membership check runs first and unconditionally -— this is the one state where authorship and membership disagree, and the -only state that proves the membership check does something the authorship -check doesn't already cover on its own. - -One accepted consequence of checking membership before authorship: a -non-member gets `403` for both an existing message and (via the `404` from -`get_one`) a nonexistent one, so the two are distinguishable by status code. -This mirrors the same `FetchChatUseCase` 403-vs-404 posture in `chats.md`, and -is recorded, not treated as a bug, in `planning/deferred.md`. - -`EditMessageUseCase` additionally rejects editing an already-deleted message -with `ConflictError` → `409` (the actor is authorized; the request conflicts -with the message's current state). `DeleteMessageUseCase` treats a second -delete of an already-deleted message as a no-op returning `204` — DELETE is -idempotent under HTTP semantics where PATCH is not. - -Deleting a chat's newest message repoints `chats.last_message_id` atomically, -in the same commit as the soft delete: `DeleteMessageUseCase` checks whether -`chat.last_message_id == message_id`, and if so looks up -`fetch_latest_active` (the next-newest non-deleted message, or `None` if none -remains) and writes that back. Without this, the chat listing's preview and -its activity ordering (`chats.md`) would both keep reading a deleted message -until something else happened to send a new one. - -## Error vocabulary - -Registered in `build_app` (`app/api/app.py`) via `app/api/exception_handlers.py`, -mapping `app/exceptions.py`'s domain hierarchy plus a few `advanced_alchemy` -exceptions: - -| Exception | Status | Meaning | -|---|---|---| -| `advanced_alchemy.exceptions.NotFoundError` | 404 | the resource doesn't exist | -| `app.exceptions.PermissionDeniedError` | 403 | authenticated, but not authorized for this action | -| `app.exceptions.ValidationError` | 400 | well-formed request, violates a domain invariant | -| `app.exceptions.ConflictError` | 409 | authorized, but conflicts with the resource's current state | -| `advanced_alchemy.exceptions.DuplicateKeyError` | 409 | unique-constraint violation not otherwise recovered | -| `advanced_alchemy.exceptions.ForeignKeyError` | 400 | a referenced id doesn't exist | - -Litestar's own `NotAuthorizedException` (401) is used exactly once, for a -failed login (`app/api/endpoints/auth.py::login`) — see `auth.md`. It is the -one place `app.exceptions` is deliberately not used, because a bad -credential is an identification failure, not a downstream authorization -decision on an already-identified actor. diff --git a/architecture/testing.md b/architecture/testing.md deleted file mode 100644 index cf84631..0000000 --- a/architecture/testing.md +++ /dev/null @@ -1,90 +0,0 @@ -# Testing - -`just test` cycles the DB (`alembic downgrade base && alembic upgrade head`) -and runs `pytest` in Compose against a migrated Postgres, gated at -`--cov-fail-under=100` with zero warnings. - -## Per-test rollback via a container override - -`db_session` (`tests/conftest.py`) opens its own `AsyncConnection`, begins a -transaction on it, then calls -`di_container.override(ioc.Database.database_engine, connection)` — every -provider downstream of `Database.database_engine` in the DI graph (sessions, -repositories, use cases, and `retrieve_user_handler`'s own ad-hoc session in -`app/api/auth.py`) now resolves against that one connection instead of the -real pooled engine. `database_resources.create_session` sets -`join_transaction_mode="create_savepoint"`, so every session opened against -that connection — whether by a fixture or by a route handler mid-request — -nests inside the outer transaction as a savepoint rather than committing past -it. Teardown does `if connection.in_transaction(): await -transaction.rollback()`, which discards every write the test made. - -That `if` guard is fail-silent: it exists to tolerate tests that already -closed their own transaction, but if a session were ever able to commit the -*outer* transaction rather than nesting a savepoint under it, teardown would -skip the rollback without raising and the next test would see leaked state. -See `planning/deferred.md`. - -`di_container` (`tests/conftest.py`) itself comes from the already-built -`app` fixture (`modern_di_litestar.fetch_di_container(app)`), so `db_session` -overrides the same container instance production request handling resolves -providers from — a request-scoped child container built during a test -(`tests/use_cases/conftest.py::request_container`) inherits the override. - -`tests/test_main.py::test_db_session_insert_is_visible_within_test` and -`test_db_session_rolls_back_between_tests` are a paired proof of this -mechanism: the first inserts a user and commits (on the fixture's own -session, inside the savepoint), the second asserts the table is empty. The -pair only proves rollback if pytest runs them in file order — running the -second alone (`-k test_db_session_rolls_back_between_tests`) passes -vacuously, since an empty table before any insert looks identical to a -successfully rolled-back one. See `planning/deferred.md`. - -## DI providers as pytest fixtures - -`tests/use_cases/conftest.py` calls `modern_di_pytest.expose(ioc.Repositories, -ioc.UseCases, container_fixture="request_container")` once, which generates -one pytest fixture per provider on both groups, named after the class -attribute (`create_chat_use_case`, `messages_repository`, …). Every -repository or use case added to `app/ioc.py` becomes an injectable test -fixture automatically — no test file hand-assembles a use case's dependency -graph. `request_container` itself is a child container built at -`modern_di.Scope.REQUEST`, depending on `db_session` (via an unused parameter -that forces the engine override to run first). - -Layered fixtures build on top: `alice`/`bob`/`carol` (users via -`UserFactory`, a `polyfactory.SQLAlchemyFactory`), `direct_chat` (a real -`CreateChatUseCase` call between alice and bob), `alice_message` (a real -`CreateMessageUseCase` call), and `send` — a callable that stamps a fresh -`idempotency_key` per invocation, so ordinary test bodies never collide with -each other on retries. - -## API-level tests - -`tests/conftest.py::client` runs the real `build_app()` output through -`httpx.ASGITransport` plus `asgi_lifespan.LifespanManager`, so these tests -exercise the actual route handlers, middleware, and DI wiring — not a stub. -`tests/api/*.py` drive it with plain `AsyncClient` calls and helper functions -(`register`, `login`, `create_direct_chat`, `send`, shared from -`tests/api/helpers.py` and imported with a leading-underscore alias per the -local call-site convention) rather than fixtures, since the cookie-carrying -`client` instance is itself the shared state across a test's sequence of -requests. - -## Simulating a DB race at the repository seam - -`tests/use_cases/test_create_chat.py::_RacingChatsRepository` and -`tests/use_cases/test_create_message.py::_RacingMessagesRepository` subclass -the real repository and override exactly two methods: the pre-check read -(`fetch_direct_by_key` / `fetch_by_idempotency_key`) returns `None` once, as -if the winner's row weren't visible yet, then delegates to the real -implementation; `create` always raises `DuplicateKeyError`, as if the insert -collided with a row a concurrent request just committed. A second use case -instance is built by hand with the racing repository swapped in but sharing -the *same* `transaction`/session as the real winner call, so the winner's -already-committed row is visible to the loser's recovery re-read — this is -what lets a single-process test prove the two-request race without an actual -second connection. `_AlwaysDuplicateChatsRepository` is the companion negative -case: `create` always raises, with no direct-chat recovery path available -(group chat), proving the exception still propagates instead of being -funnelled into recovery it doesn't apply to. diff --git a/planning/.convention-version b/planning/.convention-version index 227cea2..ccbccc3 100644 --- a/planning/.convention-version +++ b/planning/.convention-version @@ -1 +1 @@ -2.0.0 +2.2.0 diff --git a/planning/README.md b/planning/README.md new file mode 100644 index 0000000..2544c3d --- /dev/null +++ b/planning/README.md @@ -0,0 +1,129 @@ +# Planning + +The standing record for `chat-app`. The living truth about *what the system +does now* lives in the code itself and in its tests. This directory holds what +code and tests cannot: the decisions taken (especially the options rejected) +and the work deliberately not scheduled. + +> **Local deviation.** This repo tracks the portable convention from +> [`lesnik512/planning-convention`](https://github.com/lesnik512/planning-convention) +> (applied version in `.convention-version`, beside this file), but **deviates +> from it** on six counts, listed under [Deviations](#deviations) below. The +> lean shape follows `modern-di`, which runs the same deviation; if it holds +> across both repos it goes upstream as convention 3.0.0. + +## Quick path (start here) + +**1. Write the spec in the PR body.** +[`.github/PULL_REQUEST_TEMPLATE.md`](../.github/PULL_REQUEST_TEMPLATE.md) +carries the shape — why, design, non-goals, verification. There is no change +file to write and nothing to commit: the PR body *is* the spec, reviewed inline +with the diff. A trivial PR (typo, dep bump, formatter, mechanical rename) may +delete the template and ship a conventional-commit title. + +**2. File what outlives the PR:** + +- an alternative you **rejected** with reasoning → [`decisions/`](decisions/) +- work that is real but **not scheduled** → [`deferred/`](deferred/) + +**3. Run `just check-planning` and `just check-links` before pushing.** + +## Where a fact goes + +Four homes, one owner each: + +| Home | Holds | +|---|---| +| `app/` | anything readable from the module — the default | +| a named test | an **invariant**: must stay true, and a change could silently break it | +| `decisions/` | a rejected alternative, with the reasoning that would otherwise be re-litigated | +| `deferred/` | real work, not scheduled, with a revisit trigger | + +Before writing a line anywhere: + +> Can an agent get this by reading `app/`? → **don't write it.** +> Would a wrong change here fail a test? → it belongs **in the test**, not in prose. +> Otherwise it does not get written. + +**Prose about mechanism has no home. There is no file to add a paragraph to.** + +This repo kept an `architecture/` directory of capability pages until 2026-08-22 +and removed it. The pages had become a second telling of `decisions/` — the +decision files referenced them zero times, while the pages re-narrated the +decisions at length — and one had gone silently wrong: `chats.md` still +described `chat_type` as a non-native enum after #4 converted it to a native +Postgres enum, and nothing caught it, because the convention's promotion rule +was a habit with nothing enforcing it. A prose copy of a fact the code already +owns goes stale in the copy nobody edits. The absence of the directory is the +mechanism. + +`decisions/` and `INVARIANT:` docstrings inherit the same risk from the other +direction: nothing prunes a record once its call is settled. Keeping both lean +is a habit this repo owes them, not a one-time fix earned by deleting a +directory. + +An invariant is written as a test whose name is the claim, with a docstring +opening `INVARIANT:` and a second paragraph naming **what breaks it**. That +second paragraph is design rationale — an anti-refactor warning — not a report +of what this one test happens to catch. + +## Artifacts + +- **[`decisions/-.md`](decisions/)** — one file per design + decision taken, especially options *rejected*, each with a revisit trigger, so + reviews don't re-litigate them. Frontmatter: `summary`, plus `superseded_by` + once something supersedes it. +- **[`deferred/-.md`](deferred/)** — one file per open item, + each **self-contained**: it inlines the evidence and reasoning needed to pick + it up cold. Frontmatter: `summary`. A required `**Revisit trigger:**` section — + an item with no trigger is abandoned, not deferred. +- **[`_templates/`](_templates/)** — `decision.md`, `deferred.md`. + +### Location is status + +Neither artifact carries a `status:` field. Where a file sits, and which keys it +has, is what its state means. + +A **deferred item's presence in `deferred/` is its status**. When it resolves: + +- **it ships** → delete the file. Its truth is now in the code and its tests. +- **it is declined** → move it to `decisions/`, so the refusal is on record. + +A **decision is accepted unless it says otherwise**. There is no exit from +`decisions/` — a superseded decision stays readable, or it gets re-litigated — +so the one state worth recording is marked by adding `superseded_by: `, +which `just index` renders. + +`date` and `slug` are derived from the file name and never repeated in +frontmatter. `summary` is one line; it is the only field the index renders. + +## Index + +The listing is **generated**, not maintained — run `just index` to print it: +deferred first (the open queue), then decisions, newest-first. The frontmatter +in each file is the single source of truth; there is no committed copy to drift. +`just check-planning` validates it, and `just check-links` validates every +relative Markdown link and heading anchor in the repo. + +## Deviations + +Against upstream convention 2.2.0: + +1. `changes/`, `audits/` and `retros/` are removed; the per-change spec is the + PR body. +2. `architecture/` is removed; there is no capability-page home and no promotion + rule. Enforceable claims are `INVARIANT:`-marked tests; the ubiquitous + language lives in [`../CLAUDE.md`](../CLAUDE.md)'s Vocabulary section. +3. `deferred.md` is a `deferred/` directory of indexed, trigger-bearing items. +4. Decision frontmatter drops `status` and `supersedes`. +5. `index.py` is edited to match that schema, and both `index.py` and `links.py` + drop the canonical `# ruff: noqa: INP001` line — this repo ignores `INP` + globally, so the directive is an unused `noqa` and fails `RUF100`. +6. There is no `lint-ci` recipe; CI inlines its lint steps, so `links.py` runs + as a step in the workflow's `lint` job rather than via a recipe CI calls. + `just check-links` exists for running it locally. + +Deviations 1–5 match `modern-di`'s practice. Applying a future convention +version runs upstream's `APPLY.md`, which copies `index.py` and `links.py` over +any local version by design — that reverts the edits in 5, so re-apply them +afterwards. diff --git a/planning/_templates/change.md b/planning/_templates/change.md deleted file mode 100644 index 5aa7e81..0000000 --- a/planning/_templates/change.md +++ /dev/null @@ -1,32 +0,0 @@ ---- -summary: One line — shown in the generated index. Written at creation; finalize at ship to state the realized result. ---- - -# Change: One-line capitalized title - -**Lane:** lightweight — ≲30 LOC net, ≤2 files, no new file, no public-API -change, a single straightforward test. If it outgrows this, rewrite it from -the design template. - -## Goal - -One or two sentences: what changes and why. - -## Approach - -The shape of the change in brief — enough that a reviewer sees the design -without a full spec. Link the truth home (`architecture/.md`) if a -capability contract moves. - -## Files - -- `path/to/file.py` — what changes -- `tests/test_x.py` — test added / updated - -## Verification - -- [ ] Failing test first — command + expected error. -- [ ] Apply the change. -- [ ] Test passes — command. -- [ ] `just test` — full suite green. -- [ ] `just lint` — clean. diff --git a/planning/_templates/decision.md b/planning/_templates/decision.md index 45ccaf0..23f2a54 100644 --- a/planning/_templates/decision.md +++ b/planning/_templates/decision.md @@ -1,10 +1,12 @@ --- -status: accepted # accepted | superseded summary: One line — shown in `just index`. -supersedes: null -superseded_by: null --- + + + # One-line capitalized title **Decision:** What was decided, in a sentence. diff --git a/planning/_templates/deferred.md b/planning/_templates/deferred.md new file mode 100644 index 0000000..43146f4 --- /dev/null +++ b/planning/_templates/deferred.md @@ -0,0 +1,18 @@ +--- +summary: One line — shown in `just index`. +--- + +# One-line capitalized title + +What the item is, in a sentence or two. + +## Why it is open + +The substance: the evidence, measurements, and reasoning needed to pick this up +cold. Inline it — a deferred item cites no report and no change file, because +this file is the only place the reasoning lives. + +## Revisit trigger + +The concrete signal that should make someone act on this. An item with no +trigger is not deferred, it is abandoned. diff --git a/planning/_templates/design.md b/planning/_templates/design.md deleted file mode 100644 index 17dbee1..0000000 --- a/planning/_templates/design.md +++ /dev/null @@ -1,39 +0,0 @@ ---- -summary: One line — shown in the generated index. Written at creation; finalize at ship to state the realized result. ---- - -# Design: One-line capitalized title - - - -## Summary - -One paragraph. What changes, at the level a reader needs to decide if this -spec is worth reading in full. - -## Motivation - -Why now. What is broken or missing. Concrete observations / numbers, not -abstract complaints. - -## Design - -What changes, in enough detail that a reader who has not seen the codebase -can follow. Sketches and interface fragments welcome; never the full -diff-to-be. Reference rejected alternatives in `decisions/` instead of -retelling them. - -## Non-goals - -What is deliberately out of scope and (when nontrivial) why. One line each. - -## Testing - -How we know it landed correctly. Be specific: the command and the expected -signal. - -## Risk - -What could go wrong, ranked by likelihood × impact. Mitigations. diff --git a/planning/changes/2026-08-21.01-chat-app-bootstrap.md b/planning/changes/2026-08-21.01-chat-app-bootstrap.md deleted file mode 100644 index e668d8c..0000000 --- a/planning/changes/2026-08-21.01-chat-app-bootstrap.md +++ /dev/null @@ -1,228 +0,0 @@ ---- -summary: Shipped the chat-app showcase repo — modern-di container, JWT cookie auth, and chats/members/messages over REST with idempotent send, cursor pagination, author-and-member-gated edit/delete, and per-member read markers with unread counts, at 100% coverage. ---- - -# Design: Bootstrap chat-app skeleton and core chat domain - -## Summary - -`chat-app` is a reference application for the `modern-python` organisation: a -single-package chat service whose purpose is to demonstrate the org's libraries -working together on a domain more realistic than a CRUD template. This change -lays the foundation and the synchronous half of the domain: repository skeleton -derived from `litestar-sqlalchemy-template`, a `modern-di` container behind the -Litestar entrypoint, JWT cookie authentication, and the chats / members / -messages model exposed over REST. The second entrypoint that shares the same -container, which is the composition the repo most wants to demonstrate, arrives -with the realtime change. Real-time delivery and the browser client -follow in separate changes; the schema and use-case boundaries here are shaped -to receive them without rework. - -## Motivation - -The org publishes twenty-odd libraries and two templates. The templates show -each library in isolation on a two-table domain; nothing shows them composed. -The questions a prospective user actually has are compositional: how does one -DI container serve both a Litestar app and a FastStream worker, how does the -transactional outbox interact with a use case's transaction, what does the -per-test rollback fixture look like once `modern-di-pytest` is involved. A chat -app answers all three because it genuinely needs them. - -The private `rchat` service already answers them, but it is closed, depends on -internal packages (`chat-schemas`, `platform-jwt-tools`, `chat-settings`), and -is coupled to Kafka. This repo is the open, Kafka-free distillation. - -## Design - -### Stack and repository shape - -Python 3.14, `uv`, `just`, Docker Compose. Litestar + SQLAlchemy 2 async + -`advanced-alchemy` + Alembic + Postgres 17 + Granian, `lite-bootstrap` for -observability wiring, `modern-di` (+ `-litestar`, `-pytest`), `db-retry`. -`ruff select=ALL` -at line length 120, `ty` for type checking, `eof-fixer`. Coverage gate stays at -`--cov-fail-under=100`. - -Deliberately absent: Kafka, `httpware`, `modern-di-typer`, `modern-di-taskiq`, -`faststream-redis-timers`, `that-depends`. Each would need a feature invented to -justify it, and an invented feature teaches nothing. - -The template keeps flat modules (`app/models.py`, `app/repositories.py`); this -domain is large enough to warrant packages, following `rchat`: - -``` -app/ -├── api/ __main__.py (granian) · app.py (build_app) -│ endpoints/{auth,chats,messages}.py · exception_handlers.py -├── use_cases/ one frozen dataclass per operation, __call__ -├── repositories/ advanced-alchemy services -├── database/ tables.py · resources.py -├── schemas/ api.py -├── exceptions.py · ioc.py · settings.py -migrations/ · tests/ · architecture/ · planning/ -``` - -`app/outbox/`, `app/resources/`, and `static/` arrive with the follow-on -changes, as do Redis and the `modern-di-faststream` / `faststream-outbox` -dependencies. Compose runs `api` and `db` only. Nothing is wired ahead of a -consumer: an unused provider cannot be covered, and the 100% gate is the -mechanism that keeps this honest. - -### Data model - -``` -users id · username (uniq) · password_hash · display_name · audit -chats id · chat_type (direct|group) · title? · created_by_id - last_message_id? · direct_key? (uniq) · audit -chat_members id · chat_id · user_id · last_read_message_id? · joined_at - uniq(chat_id, user_id) -messages id · chat_id (idx) · user_id? · idempotency_key (uniq) - text · created_at · edited_at? · deleted_at? -``` - -Three load-bearing choices: - -**Message ids are a BigInt identity sequence.** Postgres assigns them -monotonically, which is the entire ordering guarantee a single-writer service -needs, and it is what lets `before_id` cursor pagination and client-side gap -detection work off the primary key index. Snowflakes buy coordination-free -generation across independent writers, which this service does not have. See -`planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md`. - -`chat_type` is `sa.Enum(ChatType, native_enum=False)`, a VARCHAR plus a CHECK -constraint rather than a native Postgres enum. Native enums need -`alembic-postgresql-enum` for autogeneration to emit correct `ALTER TYPE` -migrations, and that is a dependency bought to store two values. - -**`direct_key`** is a canonical `min(uid):max(uid)` string under a unique -constraint, so opening a DM is an upsert rather than a read-then-race. It is -NULL for group chats, which the unique constraint permits without limit because -Postgres does not treat NULLs as equal. - -**`last_read_message_id` per member, no per-message receipt rows.** Unread count -is `count(messages WHERE chat_id = ? AND id > last_read 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 != me` -evaluates to NULL, which would silently drop every system message from the -count. Per-message receipts generate one row and one event per -member per message, so receipt traffic exceeds message traffic by the member -count; the cheap representation is also the correct one. - -`user_id` on `messages` is nullable to admit system messages ("Bob joined") -without a sentinel user. - -### DI wiring - -One `ioc.py` with groups `Database` (engine app-scoped with a dispose -finalizer; session and `db-retry` `Transaction` request-scoped), `Repositories` -(request-scoped, `auto_commit=False`), and `UseCases` (request-scoped). The -`Redis` group and the second container consumer arrive with the realtime -change. - -`auto_commit=False` is the departure from the template, and it is deliberate: -the use case owns the transaction boundary because a single operation writes a -message, updates `chats.last_message_id`, and (from the realtime change onward) -an outbox row, and those must commit together. Repositories that commit on -their own make that impossible. - -Use cases are `@dataclasses.dataclass(kw_only=True, frozen=True, slots=True)` -with an async `__call__`, decorated `@postgres_retry` from `db-retry`, following -`rchat`. One class per operation keeps each file small enough to read whole. - -### Auth - -`litestar[jwt]`'s `JWTCookieAuth` with an argon2 password hash. Cookie rather -than bearer header specifically because the browser's `EventSource` cannot set -an `Authorization` header, so this is the only variant where the SSE stream -added in the realtime change authenticates identically to every other endpoint. -`retrieve_user_handler` loads the user through the request-scoped session. - -### HTTP surface - -``` -POST /api/auth/register → 201 + cookie -POST /api/auth/login /logout -GET /api/auth/me - -GET /api/chats/ → chats + last_message + unread_count -POST /api/chats/ → direct (upsert on direct_key) | group -GET /api/chats/{id}/ -POST /api/chats/{id}/read/ → {last_read_message_id} - -GET /api/chats/{id}/messages/?before_id=&limit= → page, newest-first -GET /api/chats/{id}/messages/?after_id= → catch-up, ascending -POST /api/chats/{id}/messages/ → {idempotency_key, text} -PATCH /api/messages/{id}/ → edit -DELETE /api/messages/{id}/ → soft delete -``` - -Pagination is cursor-only. Offset degrades on deep history and silently skips -rows inserted behind the cursor, which in a chat is a dropped message. - -A repeated `idempotency_key` returns the existing message with `200` instead of -`201`. A retried send is not an error, and the client needs the server's id to -reconcile its optimistic bubble. - -`PATCH` and `DELETE` are author-only; membership alone is not sufficient. There -is no moderator role in this change. Delete is soft, and listings exclude rows -with `deleted_at IS NOT NULL` rather than returning tombstones, so a deleted -message vanishes on the next fetch. `POST /api/chats/` takes an explicit -`member_ids` list; the creator is always added as a member, and a `direct` chat -must resolve to exactly two distinct members or the request is rejected. - -The `?after_id=` form exists for the realtime client's resync-on-reconnect and -is specified here because it constrains the index on `messages(chat_id, id)`. - -### Errors - -`advanced_alchemy.NotFoundError` → 404 and `NotAuthorizedException` → 401 are -registered in `build_app`. Membership and ownership checks live in the use case -and raise domain exceptions from `app/exceptions.py`, mapped to 403. - -## Non-goals - -- Real-time delivery, the outbox table and worker, SSE, typing, presence: - follow-on change. -- The browser client: follow-on change. -- Kafka, and any library that would need an invented feature to justify it. -- True server-side `Last-Event-ID` replay. See - `planning/decisions/2026-08-21-no-server-side-event-replay.md`. -- Workarounds for open Litestar channels bugs. See `planning/deferred.md`. -- Attachments, message types beyond user/system, threads, reactions, - chat administration. - -## Testing - -`just test` runs the suite in Compose against a migrated database, and must -report 100% coverage. - -The template's per-test rollback fixture carries over intact: a connection with -an open transaction and a savepoint, `Database.database_engine` overridden in -the DI container to hand back that connection, `join_transaction_mode= -"create_savepoint"`, and the `CustomAsyncSession.close()` override that keeps -the outer transaction alive. Added on top, `modern-di-pytest` exposes providers -as fixtures directly, which the template predates. - -Per layer: use cases tested directly against the database through -container-resolved providers, covering the permission denials and the -idempotent-resend path; endpoints tested through `httpx.ASGITransport` plus -`asgi_lifespan.LifespanManager`; polyfactory `SQLAlchemyFactory` for model -factories and pydantic factories for payloads. Specific signals worth naming: -posting the same `idempotency_key` twice yields one row and a `200` on the -second call; unread count for a member with `last_read_message_id = NULL` -counts every message from others and none of their own. - -## Risk - -**The showcase drifts into a product.** Most likely failure, and the reason the -non-goals list is long. Mitigation: every feature must be traceable to a library -or pattern it demonstrates; anything else is rejected. - -**The 100% coverage gate becomes noise.** A gate met with assertion-free tests is -worse than none. Mitigation: gate stays, but review treats a test that only -raises coverage as a defect. - -**Litestar internals shift under the realtime change.** The channels API is where -`rchat` needed private-attribute workarounds. Contained here by keeping channels -out of this change entirely, so a breaking upgrade lands inside one follow-on -change rather than across the foundation. diff --git a/planning/changes/2026-08-21.02-di-wiring-and-chat-listing.md b/planning/changes/2026-08-21.02-di-wiring-and-chat-listing.md deleted file mode 100644 index f610865..0000000 --- a/planning/changes/2026-08-21.02-di-wiring-and-chat-listing.md +++ /dev/null @@ -1,101 +0,0 @@ ---- -summary: Removed the hand-written DI dependency dict, the per-row chat-listing DTO and its schema translation, and made every use case `__call__` keyword-only — 108 tests green at 100% coverage with no API or schema change. ---- - -# Design: Autowired DI, mapped chat-listing state, keyword-only use cases - -## Summary - -Four wiring simplifications with no change to the HTTP contract. `ModernDIPlugin` -autowires `ioc.UseCases`, replacing the twelve-entry `dependencies=` dict in -`build_app`. `ChatsTable` maps `unread_count` and `last_message` itself, so the -chat listing no longer assembles a `ChatListRow` per chat and no longer -translates it back into a schema field-by-field. Every use case `__call__` takes -keyword-only arguments. Authenticated handlers annotate the request -`AuthedRequest` instead of restating `litestar.Request[UsersTable, Any, Any]`. - -## Motivation - -Each of the four is small on its own; together they remove the places where -adding a feature meant editing a list that exists only to repeat something the -code already knows. - -- Every new use case had to be named three times: as a provider in - `ioc.UseCases`, as an import in `app/api/app.py`, and as a `dependencies=` - entry — under the name it already had as a provider attribute. -- `GET /api/chats/` ran the chat's four fields through two hand-written - mappings: `FetchChatsUseCase` packed a `ChatsTable` into `ChatListRow`, and - `ChatListItem.from_row` unpacked it again into a dict for validation. Neither - mapping renamed anything — `ChatsTable.id` was already `ChatListItem.id`. -- `create_chat_use_case(request.user, data)` gave no clue at the call site which - argument was the actor, on operations whose authorization turns on it. - -## Design - -**Autowiring.** `ModernDIPlugin(di_container, autowired_groups=[ioc.UseCases])` -registers one Litestar dependency per provider on the group, named after the -provider attribute — already the handler parameter names. `Database` and -`Repositories` stay out: a route handler resolving a session, a transaction or a -repository directly is a boundary violation, and autowiring them would also let -a handler parameter that happens to be called `transaction` become a silent DI -injection instead of a query parameter. - -**Chat listing.** `ChatsTable` gains two attributes that only the listing query -populates: - -```python -unread_count: orm.Mapped[int] = orm.query_expression(default_expr=sa.literal(0)) -last_message: orm.Mapped[MessagesTable | None] = orm.relationship(...) -``` - -`list_for_user` fills the first with `with_expression(...)` and loads the second -with one `selectinload`, so it returns `Sequence[ChatsTable]` rather than -`Sequence[Row[tuple[ChatsTable, int]]]`. `FetchChatsUseCase` becomes a single -repository call — the hand-rolled `WHERE id IN (...)` batch for previews is now -the `selectinload`, same one extra round trip. `ChatListItem` keeps inheriting -`Chat` and loses `from_row`; the endpoint is one `Chats.from_models(...)`. -Response JSON is unchanged. - -Two mapping details carry their own reasons, both documented in -`architecture/chats.md`: `last_message_id` has no `ForeignKey` (it would close a -cycle with `messages.chat_id`), so the relationship annotates the join column -`orm.foreign()` by hand and folds the soft-delete guard into its `primaryjoin`; -and `list_for_user` runs `populate_existing=True` because sessions are -`expire_on_commit=False`, so an identity-mapped chat would otherwise keep the -`unread_count` it was first loaded with. - -**Keyword-only.** All ten `__call__`s, e.g. -`create_chat_use_case(actor=request.user, data=data)`. `db_retry.postgres_retry` -forwards `**kwargs`, so the decorator is unaffected. - -## Non-goals - -- Injecting the actor through `modern-di`'s `ContextProvider`. It is operation - input, not a collaborator, and binding it to the request container would force - every use-case test that drives one instance as several actors to build a - container per actor. -- Nesting the listing response as `{"chat": {...}, "unread_count": ...}`. It - would spare four lines of Python at the cost of `item.chat.title` in every - client. - -## Testing - -`just test` — 108 passed, 100% coverage. `just lint` — clean. -`tests/use_cases/test_unread_counts.py` was rewritten to the flat shape first -and failed on `AttributeError: 'ChatListRow' object has no attribute 'id'` -before the mapping landed. `tests/api/test_chat_listing_api.py` needed no -change, which is the evidence that the response contract held. -`alembic check` reports the same pre-existing `ck_chats_chattype` diff as -`main` and nothing else: the two new attributes emit no DDL. - -## Risk - -- **Stale listing values within one session.** `expire_on_commit=False` plus an - ORM-mapped `unread_count` is exactly the combination that returns a previous - load's numbers. Mitigated by `populate_existing=True` on `list_for_user`, and - covered by the tests that list twice around a mutation - (`test_deleting_the_newest_message_updates_preview_and_ordering`, - `test_marking_read_is_monotonic`). -- **`last_message` lazy-loading in async context.** `lazy="noload"` means a chat - loaded by any other query has `last_message` as `None` rather than emitting IO; - a caller that wants it must ask for the loader option. diff --git a/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md b/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md deleted file mode 100644 index f56bdd2..0000000 --- a/planning/changes/2026-08-21.03-native-enum-and-migration-tests.md +++ /dev/null @@ -1,93 +0,0 @@ ---- -summary: Converted chats.chat_type to a native Postgres enum with alembic-postgresql-enum and added a pytest-alembic suite, turning a permanently-dirty autogenerate into a green CI gate. ---- - -# Design: Native chat_type enum and a pytest-alembic migration suite - -## Summary - -`chats.chat_type` becomes a native Postgres enum instead of `VARCHAR` plus a -CHECK constraint, `migrations/env.py` imports `alembic-postgresql-enum`, and -`tests/migrations/` runs the four `pytest-alembic` built-ins. `alembic check` is -clean for the first time, and migration health is now enforced by CI rather than -by a comment asking readers to ignore a diff. - -## Motivation - -`sa.Enum(ChatType, native_enum=False, create_constraint=True)` produced a -permanent autogenerate false positive: Postgres reflects the CHECK body back as -`chat_type::text = ANY (ARRAY[...])`, which never textually matches what Alembic -renders from the model, so every run proposed -`op.drop_constraint('ck_chats_chattype')`. The cost was not cosmetic — it meant -`alembic check` could never be a drift gate, and every `just migration` produced -a spurious operation a human had to remember to delete. A note in -`migrations/versions/2026-08-21_messages.py` documented that trap rather than -removing it. - -The private `rchat` service already solved this: native `postgresql.ENUM` -columns plus `alembic-postgresql-enum`, with `pytest-alembic` covering migration -health in a separate CI job. This repo exists to show that stack working, so it -should show that part too. - -## Design - -**Native enum.** `sa.Enum(ChatType, name="chattype", values_callable=...)` — the -`values_callable` stays, so the type's labels remain the lowercase member values. -`alembic-postgresql-enum`, imported for its side effect in `migrations/env.py`, -autogenerated the whole conversion including the `USING` clause and a working -downgrade: - -```python -sa.Enum("direct", "group", name="chattype").create(op.get_bind()) -op.alter_column("chats", "chat_type", ..., postgresql_using="chat_type::chattype") -op.drop_constraint(op.f("ck_chats_chattype"), "chats", type_="check") -``` - -Producing that by hand is exactly the work the library exists to remove, and it -is what makes a future value addition a one-command change. - -**Migration tests.** `tests/migrations/test_migrations.py` imports -`test_single_head_revision`, `test_upgrade`, `test_up_down_consistency` and -`test_model_definitions_match_ddl` from `pytest_alembic.tests`. The last is -`alembic check` as a test; the other three are coverage `just test` never had — -it only ran `downgrade base && upgrade head`, which proves neither per-revision -reversibility nor the absence of branched heads. A local `alembic_engine` -fixture replaces pytest-alembic's default in-memory SQLite engine with the real -Postgres DSN. - -The suite is excluded from the default run — `--ignore=tests/migrations` in -`addopts`, `tests/migrations/*` in coverage's `omit` — because it cycles the -schema out from under `db_session`'s transaction-rollback fixture. `just -test-migrations` runs it with `--override-ini=addopts=`, and CI runs both. - -**Incidental.** `Settings.sync_db_dsn_parsed` now owns the -`postgresql+asyncpg` → `postgresql` rewrite, which `migrations/env.py` and the -`alembic_engine` fixture both need. `alembic.ini` gains `path_separator = os`: -without it Alembic emits a `DeprecationWarning` that `filterwarnings = ["error"]` -turns into a test failure. - -## Non-goals - -- Converting anything else. `chat_type` is the only enum column in the schema. -- Suppressing the diff with an `include_object` filter in `env.py`. It would - have been two lines, but it hides a class of real diffs and keys on a - constraint name that the next enum column would not share. - -## Testing - -- `just test` — 109 passed, 100% coverage. -- `just test-migrations` — 4 passed. -- `just lint` — clean. -- `alembic upgrade head && alembic check` — clean, where it previously reported - `remove_constraint ck_chats_chattype`. `alembic downgrade -1 && alembic - upgrade head` round-trips. - -## Risk - -- **Adding an enum value is now a migration.** That is the point — it was - previously an unenforced CHECK constraint edit — but it does mean a value - added to `ChatType` without running `just migration` fails - `test_model_definitions_match_ddl` rather than passing silently. -- **The conversion is not free on a large table.** `ALTER COLUMN ... TYPE` - rewrites `chats`. It is cheap now, while the table is effectively empty, and - gets more expensive the longer it waits. diff --git a/planning/changes/2026-08-21.04-comment-sweep.md b/planning/changes/2026-08-21.04-comment-sweep.md deleted file mode 100644 index b89a10c..0000000 --- a/planning/changes/2026-08-21.04-comment-sweep.md +++ /dev/null @@ -1,76 +0,0 @@ ---- -summary: Cut authored comments from 203 lines to 25 across app/, tests/ and migrations/, keeping only single lines on code that reads as a bug without one, and recorded the rule in CLAUDE.md. ---- - -# Design: Comment policy sweep - -## Summary - -A repo-wide pass under one rule: no comment unless the code would read as a bug -without it, and then a single line. Rationale moves to `architecture/` and -`planning/changes/`, which is where this repo already keeps it. `app/` goes from -127 comment lines to 19, `tests/` from 54 to 5, and `migrations/` from 6 authored -lines to 1 (the remaining 16 are Alembic's own generated markers). The rule -itself is now in `CLAUDE.md` so it holds for future work. - -## Motivation - -The prose had grown to roughly one comment line for every eight lines of code, -and most of it duplicated `architecture/*.md` verbatim — the `Transaction.__aexit__` -hazard was written out three times in `app/use_cases/` and once more in -`architecture/chats.md`. Duplicated rationale goes stale in the copy nobody -edits, and a reader who cannot tell load-bearing comments from narration stops -reading all of them. - -The test suite showed the sharpest version: most test comments explained what -the test proved, which is the test's name's job. -`test_non_author_member_cannot_edit_message` carried -`# bob is a member of the chat but not the author`. - -## Design - -A comment survives only where removing it would make correct code look wrong. -What that leaves in `app/`, and nothing else: - -- a setting that reads as arbitrary or inert — `join_transaction_mode`, - `populate_existing`, `capture_parameters=False`, `path_separator` -- `orm.foreign()` on a column that carries no `ForeignKey` -- a `return` from inside an `async with self.transaction:` block -- discarded work that is not dead code — `AuthenticateUserUseCase` hashing a - password for an unknown username -- an import kept only for its side effect - -Everything else was deleted after confirming the reasoning already lived in -`architecture/`. One gap turned up and was filled rather than dropped: -`service_debug`'s credential-leak hazard — `echo`/`echo_pool` log bound -parameters, including `password_hash` on every registration — had no doc home -and is now in `architecture/auth.md` next to the `capture_parameters=False` -note it parallels. - -Two mechanical points. `# revision identifiers, used by Alembic.` came from our -own `migrations/script.py.mako`, so it was removed at the source as well as from -the four existing migrations — otherwise the next `just migration` reintroduces -it. Alembic's `# ### commands auto generated ###` markers stay: they come from -the autogenerate renderer, not from the template, so deleting them is a fight -that restarts with every migration. - -## Non-goals - -- Docstrings. A different construct with a different audience; untouched. -- `# noqa` / `# ty: ignore` directives, which are instructions to tooling. -- The eight `# noqa: PLR0913, PLR0917` annotations, already removed in #4 by - raising `pylint.max-args` instead. - -## Testing - -`just test` — 109 passed, 100% coverage. `just test-migrations` — 4 passed. -`just lint` — clean. The suites are the check that matters here: this change -removes no code, so a green run means every deletion was in fact a comment. - -## Risk - -- **Reasoning lost with a deleted comment.** Mitigated by grepping - `architecture/` for each block's subject before deleting it, and by writing - the one uncovered case into `architecture/auth.md`. -- **The rule drifting back.** `CLAUDE.md` now states it and enumerates the - permitted category, so the next change has something to check against. diff --git a/planning/decisions/2026-08-21-anonymous-doc-and-metrics-paths.md b/planning/decisions/2026-08-21-anonymous-doc-and-metrics-paths.md index b165fe4..dcb2d83 100644 --- a/planning/decisions/2026-08-21-anonymous-doc-and-metrics-paths.md +++ b/planning/decisions/2026-08-21-anonymous-doc-and-metrics-paths.md @@ -1,5 +1,4 @@ --- -status: accepted summary: The auth exclude list carries four anchored prefixes — /docs, /health, /static and /metrics — and nothing else. --- diff --git a/planning/decisions/2026-08-21-cookie-auth-not-bearer.md b/planning/decisions/2026-08-21-cookie-auth-not-bearer.md new file mode 100644 index 0000000..19f6d2c --- /dev/null +++ b/planning/decisions/2026-08-21-cookie-auth-not-bearer.md @@ -0,0 +1,38 @@ +--- +summary: Authentication is a JWT in a cookie rather than a bearer header, because EventSource cannot set an Authorization header. +--- + +# Cookie auth, not a bearer header + +**Decision:** The JWT travels in a cookie (`JWTCookieAuth[UsersTable]`), 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 +[`2026-08-21-explicit-cookie-secure-flag.md`](2026-08-21-explicit-cookie-secure-flag.md). + +## Revisit trigger + +The SSE endpoint being dropped from scope, or a non-browser client becoming +the primary consumer. diff --git a/planning/decisions/2026-08-21-coverage-exclusions-structural-only.md b/planning/decisions/2026-08-21-coverage-exclusions-structural-only.md index 2a64b99..ab7f3ef 100644 --- a/planning/decisions/2026-08-21-coverage-exclusions-structural-only.md +++ b/planning/decisions/2026-08-21-coverage-exclusions-structural-only.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Coverage exclusions are reserved for code pytest structurally cannot execute; unreachable-in-production branches are tested through repository seams instead. --- @@ -8,8 +7,10 @@ summary: Coverage exclusions are reserved for code pytest structurally cannot ex The suite runs at `--cov-fail-under=100`. Two mechanisms can exempt code, and each has a narrow warrant: -- `[tool.coverage.run] omit` lists `migrations/*`, `app/api/__main__.py` and - `planning/index.py` — files pytest never imports at all. +- `[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/`. diff --git a/planning/decisions/2026-08-21-domain-error-vocabulary.md b/planning/decisions/2026-08-21-domain-error-vocabulary.md index a2c6272..0060eff 100644 --- a/planning/decisions/2026-08-21-domain-error-vocabulary.md +++ b/planning/decisions/2026-08-21-domain-error-vocabulary.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Domain failures are split across PermissionDeniedError (403), ValidationError (400) and ConflictError (409) rather than expressed as authorization failures. --- @@ -29,6 +28,13 @@ 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. It is the one place `litestar.exceptions` +is used deliberately. + ## Consequence Every new use case must pick a category deliberately. Handlers must not diff --git a/planning/decisions/2026-08-21-explicit-cookie-secure-flag.md b/planning/decisions/2026-08-21-explicit-cookie-secure-flag.md index 6a44231..5a052dd 100644 --- a/planning/decisions/2026-08-21-explicit-cookie-secure-flag.md +++ b/planning/decisions/2026-08-21-explicit-cookie-secure-flag.md @@ -1,5 +1,4 @@ --- -status: accepted summary: The session cookie's Secure flag comes from an explicit jwt_cookie_secure setting, not from inspecting service_environment. --- diff --git a/planning/decisions/2026-08-21-idempotency-scoped-per-chat.md b/planning/decisions/2026-08-21-idempotency-scoped-per-chat.md index d994ac2..7a5f612 100644 --- a/planning/decisions/2026-08-21-idempotency-scoped-per-chat.md +++ b/planning/decisions/2026-08-21-idempotency-scoped-per-chat.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Message idempotency is scoped to (chat_id, idempotency_key), not to the key alone. --- diff --git a/planning/decisions/2026-08-21-mutation-requires-membership.md b/planning/decisions/2026-08-21-mutation-requires-membership.md index 447451a..9a2a7e9 100644 --- a/planning/decisions/2026-08-21-mutation-requires-membership.md +++ b/planning/decisions/2026-08-21-mutation-requires-membership.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Editing and deleting a message requires chat membership as well as authorship; the check order is existence, membership, authorship. --- @@ -31,7 +30,8 @@ author of a message in a chat she is no longer in. 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 `planning/deferred.md`. +pretending the chat does not exist. See +`planning/deferred/2026-08-21-message-id-existence-404-vs-403.md`. ## Revisit trigger diff --git a/planning/decisions/2026-08-21-no-server-side-event-replay.md b/planning/decisions/2026-08-21-no-server-side-event-replay.md index 83ae81a..efa1a8e 100644 --- a/planning/decisions/2026-08-21-no-server-side-event-replay.md +++ b/planning/decisions/2026-08-21-no-server-side-event-replay.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Reconnect recovery uses channel history plus a REST resync; no durable per-user event log honouring Last-Event-ID. --- diff --git a/planning/decisions/2026-08-21-per-user-channel-topology.md b/planning/decisions/2026-08-21-per-user-channel-topology.md index 85bf537..39c053e 100644 --- a/planning/decisions/2026-08-21-per-user-channel-topology.md +++ b/planning/decisions/2026-08-21-per-user-channel-topology.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Event fan-out uses one Redis channel per user; per-chat and hybrid topologies rejected. --- diff --git a/planning/decisions/2026-08-21-read-marker-integrity.md b/planning/decisions/2026-08-21-read-marker-integrity.md index 8abb5ff..45f42e8 100644 --- a/planning/decisions/2026-08-21-read-marker-integrity.md +++ b/planning/decisions/2026-08-21-read-marker-integrity.md @@ -1,5 +1,4 @@ --- -status: accepted summary: The read marker advances only to a message in its own chat, and advances atomically via GREATEST so it can never move backwards. --- diff --git a/planning/decisions/2026-08-21-repoint-last-message-on-delete.md b/planning/decisions/2026-08-21-repoint-last-message-on-delete.md index 0839dad..0f6e40e 100644 --- a/planning/decisions/2026-08-21-repoint-last-message-on-delete.md +++ b/planning/decisions/2026-08-21-repoint-last-message-on-delete.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Soft-deleting a chat's newest message repoints chats.last_message_id in the same transaction, rather than filtering the deleted row out of the listing preview. --- diff --git a/planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md b/planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md index 5b8d59f..ddc29fd 100644 --- a/planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md +++ b/planning/decisions/2026-08-21-sequence-ids-not-snowflakes.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Message ids are a Postgres BigInt identity sequence, not snowflake ids. --- diff --git a/planning/decisions/2026-08-21-upsert-via-duplicate-key-recovery.md b/planning/decisions/2026-08-21-upsert-via-duplicate-key-recovery.md index d610f79..1210517 100644 --- a/planning/decisions/2026-08-21-upsert-via-duplicate-key-recovery.md +++ b/planning/decisions/2026-08-21-upsert-via-duplicate-key-recovery.md @@ -1,5 +1,4 @@ --- -status: accepted summary: Direct-chat creation and message send recover from a unique-constraint violation and re-read, rather than trusting a pre-check. --- diff --git a/planning/deferred.md b/planning/deferred.md deleted file mode 100644 index d6c55d0..0000000 --- a/planning/deferred.md +++ /dev/null @@ -1,125 +0,0 @@ -# Deferred - -Real-but-unscheduled items. Each carries a revisit trigger. - -## Litestar channels: subscriber orphaned on mid-subscribe disconnect - -`ChannelsPlugin.subscribe()` registers the subscriber into `_channels` before -awaiting the history fetch, so a client disconnecting mid-subscribe leaves a -registered subscriber that is never unsubscribed. Upstream: -[litestar#4871](https://github.com/litestar-org/litestar/issues/4871). - -`rchat` works around it by reordering the operations, which requires reaching -into `plugin._subscriber_class`, `plugin._channels`, and `plugin._backend`. Not -shipped here: a reference repository demonstrating private-attribute access -teaches the wrong lesson, and the leak is inert at demo scale. - -**Revisit trigger:** upstream fix released, or a deployment where connection -churn is high enough for the leak to matter. - -## Litestar channels: empty channel entries retained after unsubscribe - -`unsubscribe` removes the subscriber but leaves the now-empty `set()` and its key -in `self._channels`. With per-user channel names that dict grows by one entry per -distinct user that ever connects, in a singleton that lives for the whole -process. Upstream: -[litestar#4867](https://github.com/litestar-org/litestar/issues/4867). - -`rchat`'s `PruningChannelsPlugin` overrides the public `unsubscribe` to drop -empty entries, so this one needs no private access. Still not shipped, for -symmetry with the item above and because the growth is bounded by distinct users -in a demo. - -**Revisit trigger:** upstream fix released, or the app being run anywhere with a -non-trivial user population. - -## Presence beyond a TTL key - -Presence is planned as a Redis key with a TTL refreshed by the SSE heartbeat. -This reports "has an open stream", not "is looking at this chat", and a client -killed between heartbeats stays online until expiry. - -**Revisit trigger:** the demo needing per-chat presence or accurate last-seen. - -## Per-test rollback is fail-silent on an unexpected commit - -The `if connection.in_transaction():` guard in `tests/conftest.py`'s -`db_session` teardown skips the rollback without error whenever the outer -transaction is already closed. It exists to tolerate tests that legitimately -closed their own transaction, but it can't distinguish that from a session -somewhere having committed the outer transaction instead of nesting a -savepoint under it — that failure mode would leak state into the next test -with no diagnostic. - -**Revisit trigger:** a test suite flake that looks like cross-test state -leakage, or before adding any code path that opens a session without going -through `database_resources.create_session`. - -## Isolation test pair is order-dependent - -`tests/test_main.py::test_db_session_insert_is_visible_within_test` and -`test_db_session_rolls_back_between_tests` together prove the per-test -rollback fixture, but only when pytest runs them in file order: the first -inserts and commits, the second asserts the table is empty. Run the second -alone (e.g. `-k test_db_session_rolls_back_between_tests`) and it passes -vacuously — an empty table before any insert is indistinguishable from a -correctly rolled-back one. - -**Revisit trigger:** test order ever becomes non-deterministic (parallel -pytest execution, `pytest-randomly`), or before trusting `-k` output from just -this pair as proof the fixture works. - -## Logout does not revoke the JWT - -`POST /api/auth/logout/` deletes the cookie but the token itself stays valid -for the rest of its `jwt_lifetime_seconds` (7 days by default) if it was -copied out of the cookie beforehand — no `revoked_token_handler` is -configured on `jwt_cookie_auth`. - -**Revisit trigger:** any deployment where a leaked/copied token is a realistic -threat model, or before shipping a "log out of all devices" feature. - -## Every authenticated request opens two DB sessions - -Auth middleware runs before request-scoped DI is available, so -`retrieve_user_handler` (`app/api/auth.py`) opens its own short-lived session -for the user lookup, separate from the request-scoped session the resolved -use case's repositories use. That's two sessions per authenticated request -against `db_pool_size=5` / `db_max_overflow=0`. - -**Revisit trigger:** before deploying this anywhere with real concurrent -traffic — pool exhaustion under load is the first thing to check if requests -start timing out waiting for a connection. - -## Message id existence is distinguishable via 404-vs-403 - -A non-member issuing `PATCH`/`DELETE /api/messages/{id}/` gets `404` for an -id that doesn't exist and `403` for one that does but belongs to a chat -they're not in — the two status codes leak whether the id is real. Accepted -deliberately: it mirrors the spec's own decision that `FetchChatUseCase` -returns `403` for a chat the actor isn't a member of rather than pretending -the chat doesn't exist (see `architecture/chats.md`), and checking membership -before authorship on every message use case keeps that posture consistent -rather than making message mutation the one place that hides existence. - -**Revisit trigger:** a threat model where message-id enumeration by a -non-member is a real concern (e.g. ids that encode something sensitive). - -## `EditMessageRequest` duplicates `SendMessageRequest`'s text constraints - -Both `app/schemas/api.py::SendMessageRequest.text` and `EditMessageRequest.text` -independently declare `pydantic.Field(min_length=1, max_length=4000)`. A -change to one's bounds is silently not a change to the other's. - -**Revisit trigger:** the two are ever meant to diverge deliberately, or a bug -report about edit accepting/rejecting text that send doesn't (or vice versa). - -## No query-count instrumentation - -Nothing in the test suite counts queries per request, so an N+1 regression in -the chat listing (e.g. `FetchChatsUseCase`'s bounded `last_message` lookup -regressing back to one query per chat) would keep `just test` green as long -as the returned data is still correct. - -**Revisit trigger:** a reported latency regression on `GET /api/chats/`, or -before adding another listing endpoint that joins per-row data. diff --git a/planning/deferred/2026-08-21-edit-message-duplicates-text-constraints.md b/planning/deferred/2026-08-21-edit-message-duplicates-text-constraints.md new file mode 100644 index 0000000..1953205 --- /dev/null +++ b/planning/deferred/2026-08-21-edit-message-duplicates-text-constraints.md @@ -0,0 +1,16 @@ +--- +summary: `SendMessageRequest.text` and `EditMessageRequest.text` independently declare the same length constraints, so a change to one silently doesn't affect the other. +--- + +# EditMessageRequest duplicates SendMessageRequest's text constraints + +## Why it is open + +Both `app/schemas/api.py::SendMessageRequest.text` and `EditMessageRequest.text` +independently declare `pydantic.Field(min_length=1, max_length=4000)`. A +change to one's bounds is silently not a change to the other's. + +## Revisit trigger + +The two are ever meant to diverge deliberately, or a bug report about edit +accepting/rejecting text that send doesn't (or vice versa). diff --git a/planning/deferred/2026-08-21-isolation-test-pair-order-dependent.md b/planning/deferred/2026-08-21-isolation-test-pair-order-dependent.md new file mode 100644 index 0000000..6030a6c --- /dev/null +++ b/planning/deferred/2026-08-21-isolation-test-pair-order-dependent.md @@ -0,0 +1,21 @@ +--- +summary: The two tests proving the per-test rollback fixture only prove it when run together in file order; run the second alone and it passes vacuously. +--- + +# Isolation test pair is order-dependent + +## Why it is open + +`tests/test_main.py::test_db_session_insert_is_visible_within_test` and +`test_db_session_rolls_back_between_tests` together prove the per-test +rollback fixture, but only when pytest runs them in file order: the first +inserts and commits, the second asserts the table is empty. Run the second +alone (e.g. `-k test_db_session_rolls_back_between_tests`) and it passes +vacuously — an empty table before any insert is indistinguishable from a +correctly rolled-back one. + +## Revisit trigger + +Test order ever becomes non-deterministic (parallel pytest execution, +`pytest-randomly`), or before trusting `-k` output from just this pair as +proof the fixture works. diff --git a/planning/deferred/2026-08-21-litestar-channels-empty-entries-retained.md b/planning/deferred/2026-08-21-litestar-channels-empty-entries-retained.md new file mode 100644 index 0000000..406fb4b --- /dev/null +++ b/planning/deferred/2026-08-21-litestar-channels-empty-entries-retained.md @@ -0,0 +1,24 @@ +--- +summary: `unsubscribe` removes the subscriber but leaves the now-empty channel entry behind in `self._channels`, growing unboundedly with distinct users. +--- + +# Litestar channels: empty channel entries retained after unsubscribe + +## Why it is open + +`unsubscribe` removes the subscriber but leaves the now-empty `set()` and its key +in `self._channels`. With per-user channel names that dict grows by one entry per +distinct user that ever connects, in a singleton that lives for the whole +process. Upstream: +[litestar#4867](https://github.com/litestar-org/litestar/issues/4867). + +`rchat`'s `PruningChannelsPlugin` overrides the public `unsubscribe` to drop +empty entries, so this one needs no private access. Still not shipped, for +symmetry with +[2026-08-21-litestar-channels-subscriber-orphaned.md](2026-08-21-litestar-channels-subscriber-orphaned.md) +and because the growth is bounded by distinct users in a demo. + +## Revisit trigger + +Upstream fix released, or the app being run anywhere with a non-trivial user +population. diff --git a/planning/deferred/2026-08-21-litestar-channels-subscriber-orphaned.md b/planning/deferred/2026-08-21-litestar-channels-subscriber-orphaned.md new file mode 100644 index 0000000..7d83607 --- /dev/null +++ b/planning/deferred/2026-08-21-litestar-channels-subscriber-orphaned.md @@ -0,0 +1,22 @@ +--- +summary: `ChannelsPlugin.subscribe()` registers a subscriber before awaiting the history fetch, so a client disconnecting mid-subscribe leaves it orphaned and never unsubscribed. +--- + +# Litestar channels: subscriber orphaned on mid-subscribe disconnect + +## Why it is open + +`ChannelsPlugin.subscribe()` registers the subscriber into `_channels` before +awaiting the history fetch, so a client disconnecting mid-subscribe leaves a +registered subscriber that is never unsubscribed. Upstream: +[litestar#4871](https://github.com/litestar-org/litestar/issues/4871). + +`rchat` works around it by reordering the operations, which requires reaching +into `plugin._subscriber_class`, `plugin._channels`, and `plugin._backend`. Not +shipped here: a reference repository demonstrating private-attribute access +teaches the wrong lesson, and the leak is inert at demo scale. + +## Revisit trigger + +Upstream fix released, or a deployment where connection churn is high enough +for the leak to matter. diff --git a/planning/deferred/2026-08-21-logout-does-not-revoke-jwt.md b/planning/deferred/2026-08-21-logout-does-not-revoke-jwt.md new file mode 100644 index 0000000..5390b54 --- /dev/null +++ b/planning/deferred/2026-08-21-logout-does-not-revoke-jwt.md @@ -0,0 +1,17 @@ +--- +summary: Logout deletes the cookie but does not revoke the JWT, which stays valid for the rest of its lifetime if it was copied beforehand. +--- + +# Logout does not revoke the JWT + +## Why it is open + +`POST /api/auth/logout/` deletes the cookie but the token itself stays valid +for the rest of its `jwt_lifetime_seconds` (7 days by default) if it was +copied out of the cookie beforehand — no `revoked_token_handler` is +configured on `jwt_cookie_auth`. + +## Revisit trigger + +Any deployment where a leaked/copied token is a realistic threat model, or +before shipping a "log out of all devices" feature. diff --git a/planning/deferred/2026-08-21-message-id-existence-404-vs-403.md b/planning/deferred/2026-08-21-message-id-existence-404-vs-403.md new file mode 100644 index 0000000..60cc7b4 --- /dev/null +++ b/planning/deferred/2026-08-21-message-id-existence-404-vs-403.md @@ -0,0 +1,21 @@ +--- +summary: A non-member issuing `PATCH`/`DELETE /api/messages/{id}/` gets `404` for a nonexistent id and `403` for one that exists but isn't theirs, leaking whether the id is real. +--- + +# Message id existence is distinguishable via 404-vs-403 + +## Why it is open + +A non-member issuing `PATCH`/`DELETE /api/messages/{id}/` gets `404` for an +id that doesn't exist and `403` for one that does but belongs to a chat +they're not in — the two status codes leak whether the id is real. Accepted +deliberately: it mirrors the spec's own decision that `FetchChatUseCase` +returns `403` for a chat the actor isn't a member of rather than pretending +the chat doesn't exist (see `planning/decisions/2026-08-21-mutation-requires-membership.md`), and checking membership +before authorship on every message use case keeps that posture consistent +rather than making message mutation the one place that hides existence. + +## Revisit trigger + +A threat model where message-id enumeration by a non-member is a real +concern (e.g. ids that encode something sensitive). diff --git a/planning/deferred/2026-08-21-no-query-count-instrumentation.md b/planning/deferred/2026-08-21-no-query-count-instrumentation.md new file mode 100644 index 0000000..1de7cb2 --- /dev/null +++ b/planning/deferred/2026-08-21-no-query-count-instrumentation.md @@ -0,0 +1,17 @@ +--- +summary: Nothing in the test suite counts queries per request, so an N+1 regression in chat listing would keep `just test` green as long as the returned data stays correct. +--- + +# No query-count instrumentation + +## Why it is open + +Nothing in the test suite counts queries per request, so an N+1 regression in +the chat listing (e.g. `FetchChatsUseCase`'s bounded `last_message` lookup +regressing back to one query per chat) would keep `just test` green as long +as the returned data is still correct. + +## Revisit trigger + +A reported latency regression on `GET /api/chats/`, or before adding another +listing endpoint that joins per-row data. diff --git a/planning/deferred/2026-08-21-per-test-rollback-fail-silent.md b/planning/deferred/2026-08-21-per-test-rollback-fail-silent.md new file mode 100644 index 0000000..ffe4d5c --- /dev/null +++ b/planning/deferred/2026-08-21-per-test-rollback-fail-silent.md @@ -0,0 +1,21 @@ +--- +summary: The `if connection.in_transaction():` guard in `tests/conftest.py`'s `db_session` teardown silently skips the rollback whenever the outer transaction is already closed. +--- + +# Per-test rollback is fail-silent on an unexpected commit + +## Why it is open + +The `if connection.in_transaction():` guard in `tests/conftest.py`'s +`db_session` teardown skips the rollback without error whenever the outer +transaction is already closed. It exists to tolerate tests that legitimately +closed their own transaction, but it can't distinguish that from a session +somewhere having committed the outer transaction instead of nesting a +savepoint under it — that failure mode would leak state into the next test +with no diagnostic. + +## Revisit trigger + +A test suite flake that looks like cross-test state leakage, or before adding +any code path that opens a session without going through +`database_resources.create_session`. diff --git a/planning/deferred/2026-08-21-presence-beyond-ttl-key.md b/planning/deferred/2026-08-21-presence-beyond-ttl-key.md new file mode 100644 index 0000000..884fde5 --- /dev/null +++ b/planning/deferred/2026-08-21-presence-beyond-ttl-key.md @@ -0,0 +1,15 @@ +--- +summary: Presence is planned as a Redis key with a TTL refreshed by the SSE heartbeat, which reports stream-open rather than actively-viewing. +--- + +# Presence beyond a TTL key + +## Why it is open + +Presence is planned as a Redis key with a TTL refreshed by the SSE heartbeat. +This reports "has an open stream", not "is looking at this chat", and a client +killed between heartbeats stays online until expiry. + +## Revisit trigger + +The demo needing per-chat presence or accurate last-seen. diff --git a/planning/deferred/2026-08-21-two-sessions-per-authenticated-request.md b/planning/deferred/2026-08-21-two-sessions-per-authenticated-request.md new file mode 100644 index 0000000..0a49abd --- /dev/null +++ b/planning/deferred/2026-08-21-two-sessions-per-authenticated-request.md @@ -0,0 +1,19 @@ +--- +summary: Auth middleware runs before request-scoped DI is available, so `retrieve_user_handler` opens its own session for the user lookup, separate from the request-scoped session used later. +--- + +# Every authenticated request opens two DB sessions + +## Why it is open + +Auth middleware runs before request-scoped DI is available, so +`retrieve_user_handler` (`app/api/auth.py`) opens its own short-lived session +for the user lookup, separate from the request-scoped session the resolved +use case's repositories use. That's two sessions per authenticated request +against `db_pool_size=5` / `db_max_overflow=0`. + +## Revisit trigger + +Before deploying this anywhere with real concurrent traffic — pool +exhaustion under load is the first thing to check if requests start timing +out waiting for a connection. diff --git a/planning/index.py b/planning/index.py index 60116da..cb630b6 100644 --- a/planning/index.py +++ b/planning/index.py @@ -1,13 +1,18 @@ # planning/ is not a Python package (this file is vendored into consumers' planning/) """Generate the planning index from frontmatter. -Run via ``just index``. Globs ``planning/changes/*.md`` and +Run via ``just index``. Globs ``planning/deferred/*.md`` and ``planning/decisions/*.md``, reads their frontmatter, and prints a Markdown -listing to stdout — changes then decisions, newest-first. Never writes a file: -the listing is a query over the files, not a committed artifact. +listing to stdout — deferred (the open queue) then decisions, newest-first. +Never writes a file: the listing is a query over the files, not a committed +artifact. ``date`` and ``slug`` are derived from the file name, not frontmatter — the name is the single source of truth for both. + +Both artifact kinds carry ``summary`` and nothing else required. A decision has +no ``status`` field: absent ``superseded_by`` means accepted, and its presence +means superseded. """ import pathlib @@ -16,11 +21,10 @@ ROOT = pathlib.Path(__file__).parent -VALID_DECISION_STATUS = {"accepted", "superseded"} -CHANGE_RE = re.compile(r"^(?P\d{4}-\d{2}-\d{2})\.\d{2}-(?P.+)$") +DEFERRED_RE = re.compile(r"^(?P\d{4}-\d{2}-\d{2})-(?P.+)$") DECISION_RE = re.compile(r"^(?P\d{4}-\d{2}-\d{2})-(?P.+)$") -SPEC_REQUIRED = ("summary",) -DECISION_REQUIRED = ("status", "summary") +DEFERRED_REQUIRED = ("summary",) +DECISION_REQUIRED = ("summary",) def parse_frontmatter(text: str) -> dict[str, str]: @@ -51,20 +55,20 @@ def _named(fields: dict[str, str], name: str, pattern: re.Pattern[str]) -> dict[ return fields -def load_changes(root: pathlib.Path) -> list[dict[str, str]]: - """Read each change file's summary; derive date/slug from the file name.""" - changes_dir = root / "changes" - changes: list[dict[str, str]] = [] - if not changes_dir.is_dir(): - return changes - for path in sorted(changes_dir.glob("*.md")): +def load_deferred(root: pathlib.Path) -> list[dict[str, str]]: + """Read each deferred item's summary; derive date/slug from the file name.""" + deferred_dir = root / "deferred" + deferred: list[dict[str, str]] = [] + if not deferred_dir.is_dir(): + return deferred + for path in sorted(deferred_dir.glob("*.md")): if path.name == "README.md" or path.name.startswith(("_", ".")): continue - fields = _named(parse_frontmatter(path.read_text(encoding="utf-8")), path.stem, CHANGE_RE) - fields["path"] = f"changes/{path.name}" + fields = _named(parse_frontmatter(path.read_text(encoding="utf-8")), path.stem, DEFERRED_RE) + fields["path"] = f"deferred/{path.name}" fields["name"] = path.stem - changes.append(fields) - return changes + deferred.append(fields) + return deferred def load_decisions(root: pathlib.Path) -> list[dict[str, str]]: @@ -84,24 +88,22 @@ def load_decisions(root: pathlib.Path) -> list[dict[str, str]]: def format_row(row: dict[str, str]) -> str: - """Render one change or decision as a Markdown list item.""" + """Render one deferred item or decision as a Markdown list item.""" slug = row.get("slug", "?") path = row.get("path", "") date = row.get("date", "") summary = row.get("summary") or "(no summary)" line = f"- **[{slug}]({path})** ({date}) — {summary}" - if row.get("supersedes"): - line += f" _(supersedes {row['supersedes']})_" if row.get("superseded_by"): line += f" _(superseded by {row['superseded_by']})_" return line -def render(changes: list[dict[str, str]], decisions: list[dict[str, str]]) -> str: - """Render the full Markdown listing: changes then decisions, newest-first.""" - out = ["# Planning index", "", "_Generated by `just index` — do not edit._", "", "## Changes", ""] - change_rows = sorted(changes, key=lambda b: b.get("name", ""), reverse=True) - out += [format_row(b) for b in change_rows] if change_rows else ["_None._"] +def render(deferred: list[dict[str, str]], decisions: list[dict[str, str]]) -> str: + """Render the full Markdown listing: deferred then decisions, newest-first.""" + out = ["# Planning index", "", "_Generated by `just index` — do not edit._", "", "## Deferred", ""] + deferred_rows = sorted(deferred, key=lambda b: b.get("name", ""), reverse=True) + out += [format_row(b) for b in deferred_rows] if deferred_rows else ["_None._"] out += ["", "## Decisions", ""] decision_rows = sorted(decisions, key=lambda d: d.get("name", ""), reverse=True) out += [format_row(d) for d in decision_rows] if decision_rows else ["_None._"] @@ -114,46 +116,40 @@ def _require(fields: dict[str, str], keys: tuple[str, ...], rel: str, violations violations.extend(f"{rel}: missing or empty frontmatter key '{key}'" for key in keys if not fields.get(key)) -def _check_change(path: pathlib.Path, violations: list[str]) -> None: - """Validate one change file (requires `summary`).""" - rel = f"changes/{path.name}" - if CHANGE_RE.match(path.stem) is None: - violations.append(f"{rel}: file name is not 'YYYY-MM-DD.NN-slug.md'") - fields = parse_frontmatter(path.read_text(encoding="utf-8")) - _require(fields, SPEC_REQUIRED, rel, violations) +def _check_deferred(path: pathlib.Path, violations: list[str]) -> None: + """Validate one deferred item (requires `summary` + a revisit trigger).""" + rel = f"deferred/{path.name}" + if DEFERRED_RE.match(path.stem) is None: + violations.append(f"{rel}: file name is not 'YYYY-MM-DD-slug.md'") + text = path.read_text(encoding="utf-8") + _require(parse_frontmatter(text), DEFERRED_REQUIRED, rel, violations) + if "Revisit trigger" not in text: + violations.append( + f"{rel}: no '**Revisit trigger:**' section — an item with no trigger is abandoned, not deferred" + ) def _check_decision(path: pathlib.Path, violations: list[str]) -> None: - """Validate one decision file (requires `status` + `summary`).""" + """Validate one decision file (requires `summary`).""" rel = f"decisions/{path.name}" if DECISION_RE.match(path.stem) is None: violations.append(f"{rel}: file name is not 'YYYY-MM-DD-slug.md'") - fields = parse_frontmatter(path.read_text(encoding="utf-8")) - _require(fields, DECISION_REQUIRED, rel, violations) - status = fields.get("status", "") - if status and status not in VALID_DECISION_STATUS: - violations.append(f"{rel}: invalid status '{status}' (allowed: {', '.join(sorted(VALID_DECISION_STATUS))})") + _require(parse_frontmatter(path.read_text(encoding="utf-8")), DECISION_REQUIRED, rel, violations) def check(root: pathlib.Path) -> list[str]: - """Validate every change file and decision; return the list of violation strings.""" + """Validate every deferred item and decision; return the list of violation strings.""" violations: list[str] = [] - changes_dir = root / "changes" + deferred_dir = root / "deferred" decisions_dir = root / "decisions" - if changes_dir.is_dir(): - for path in sorted(changes_dir.iterdir()): - if path.is_dir(): - violations.append( - f"changes/{path.name}: directory found — convention 2.0.0 uses flat change files " - f"(changes/YYYY-MM-DD.NN-slug.md; see CHANGELOG 2.0.0 for the migration)" - ) - continue + if deferred_dir.is_dir(): + for path in sorted(deferred_dir.iterdir()): if path.name == "README.md" or path.name.startswith(("_", ".")): continue if path.suffix != ".md": - violations.append(f"changes/{path.name}: unexpected non-md file in changes/") + violations.append(f"deferred/{path.name}: unexpected non-md file in deferred/") else: - _check_change(path, violations) + _check_deferred(path, violations) if decisions_dir.is_dir(): for path in sorted(decisions_dir.glob("*.md")): if path.name == "README.md" or path.name.startswith("_"): @@ -163,7 +159,7 @@ def check(root: pathlib.Path) -> list[str]: def main(argv: list[str] | None = None, root: pathlib.Path | None = None) -> int: - """Print the listing to stdout, or validate change files and decisions with --check.""" + """Print the listing to stdout, or validate deferred items and decisions with --check.""" argv = sys.argv[1:] if argv is None else argv root = ROOT if root is None else root if "--check" in argv: @@ -175,7 +171,7 @@ def main(argv: list[str] | None = None, root: pathlib.Path | None = None) -> int return 1 sys.stdout.write("planning: OK\n") return 0 - sys.stdout.write(render(load_changes(root), load_decisions(root))) + sys.stdout.write(render(load_deferred(root), load_decisions(root))) return 0 diff --git a/planning/links.py b/planning/links.py new file mode 100644 index 0000000..a9d5df8 --- /dev/null +++ b/planning/links.py @@ -0,0 +1,148 @@ +# planning/ is not a Python package (this file is vendored into consumers' planning/) +"""Check every relative Markdown link and heading anchor in the repository. + +Run via ``just check-links``. Exists because a site builder only validates the +directory it publishes: a repo's ``architecture/`` and ``planning/`` trees usually +sit outside it, are read on GitHub, and rot silently. In the repo this convention +came from, anchors in ``architecture/`` broke three times in one week, each caught +only by a human re-deriving slugs by hand. + +Slugs follow **GitHub's** algorithm, because that is where these files are read — +including the ones a site builder also publishes. Where the two disagree, the fix +is to change the heading rather than to teach this checker both dialects: a heading +containing an em dash yields ``a--b`` on GitHub (the dash is dropped, both spaces +become hyphens) and ``a-b`` under python-markdown (the whitespace run collapses). + +External links are not fetched; this checks the repository's internal consistency. +A relative link that resolves outside the repository is reported rather than followed: +it is a 404 on GitHub, and whether it resolves on disk depends on what the author +happens to have cloned next to the repo — a verdict a lint gate must never depend on. +""" + +import argparse +import collections +import pathlib +import re +import sys + + +SKIP_DIRS = frozenset({".git", ".venv", ".tox", "site", "node_modules", "__pycache__", ".ruff_cache", ".superpowers"}) +FENCE = re.compile(r"^\s*(```|~~~)") +INLINE_CODE = re.compile(r"(`+).+?\1") # any run of backticks delimits a span: `x`, ``a`b`` +HEADING = re.compile(r"^(#{1,6})\s+(.+?)\s*$") +LINK = re.compile(r"\[[^\]]*\]\(\s*([^)\s]+)(?:\s+\"[^\"]*\")?\s*\)") +EXTERNAL = re.compile(r"^(?:[a-z][a-z0-9+.-]*:|//)", re.IGNORECASE) + + +def repo_root(start: pathlib.Path) -> pathlib.Path: + """Nearest ancestor holding ``.git``, else ``start``. + + Found rather than computed because this file has two homes: the canonical repo's + root, and a consumer's ``planning/`` — a fixed relative depth is wrong in one of them. + """ + for candidate in [start, *start.parents]: + if (candidate / ".git").exists(): + return candidate + return start + + +def strip_fences(text: str) -> str: + """Blank out fenced blocks, keeping line count, so code is never read as a heading.""" + out, fenced = [], False + for line in text.splitlines(): + if FENCE.match(line): + fenced = not fenced + out.append("") + continue + out.append("" if fenced else line) + return "\n".join(out) + + +def link_lines(text: str) -> list[str]: + """Lines with fenced blocks and inline spans removed — what to scan for real links. + + Only link scanning strips inline spans. A page documenting the markup an author should + copy is not linking anywhere, while a heading's backticked content is part of its slug. + """ + return [INLINE_CODE.sub("", line) for line in strip_fences(text).splitlines()] + + +def slugify(heading: str) -> str: + """GitHub's heading slug: drop formatting and punctuation, lowercase, spaces to hyphens.""" + text = re.sub(r"`([^`]*)`", r"\1", heading) + text = re.sub(r"\[([^\]]*)\]\([^)]*\)", r"\1", text) + # `*` and `~` are emphasis; `_` is kept because GitHub keeps it and headings name + # identifiers (`bound_type`) far more often than they use underscore-italics. + text = re.sub(r"[*~]", "", text) + text = "".join(ch for ch in text.lower() if ch.isalnum() or ch in " -_") + return text.strip().replace(" ", "-") + + +def anchors(text: str) -> set[str]: + """Every anchor a reader can target, including GitHub's ``-1``/``-2`` duplicate suffixes.""" + seen: collections.Counter[str] = collections.Counter() + found: set[str] = set() + for line in strip_fences(text).splitlines(): + match = HEADING.match(line) + if not match: + continue + base = slugify(match.group(2)) + found.add(base if not seen[base] else f"{base}-{seen[base]}") + seen[base] += 1 + return found + + +def check(root: pathlib.Path) -> list[str]: + """Return one message per broken link; empty means every internal link resolves.""" + root = root.resolve() + files = sorted(p for p in root.rglob("*.md") if not SKIP_DIRS & set(p.relative_to(root).parts)) + cache: dict[pathlib.Path, set[str]] = {} + violations: list[str] = [] + for path in files: + text = path.read_text(encoding="utf-8") + for line_no, line in enumerate(link_lines(text), 1): + for target in LINK.findall(line): + if EXTERNAL.match(target): + continue + rel, _, fragment = target.partition("#") + # A bare `#frag` targets this same file — the anchor is still checkable, + # and a same-page link rots exactly like a cross-page one. + dest = (path.parent / rel).resolve() if rel else path + where = f"{path.relative_to(root)}:{line_no}" + if dest != root and root not in dest.parents: + # Judged before existence: a sibling repo cloned alongside this one makes + # ../../../other-repo/… resolve on one machine and nowhere else, and it is + # a 404 on GitHub either way. The verdict must not depend on the checkout layout. + violations.append(f"{where}: leaves the repository -> {rel}") + continue + if not dest.exists(): + violations.append(f"{where}: no such file -> {rel}") + continue + if not fragment or dest.suffix != ".md": + continue + if dest not in cache: + cache[dest] = anchors(dest.read_text(encoding="utf-8")) + if fragment.lower() not in cache[dest]: + violations.append(f"{where}: no such anchor -> {target}") + return violations + + +def main(argv: list[str] | None = None, root: pathlib.Path | None = None) -> int: + """Report every broken link; return 1 if any, else 0.""" + parser = argparse.ArgumentParser(description="Check Markdown links and heading anchors.") + parser.add_argument("--root", type=pathlib.Path, default=None) + args = parser.parse_args(sys.argv[1:] if argv is None else argv) + + target = args.root or root or repo_root(pathlib.Path(__file__).resolve().parent) + violations = check(target) + if violations: + sys.stderr.write(f"links: {len(violations)} broken\n") + for violation in violations: + sys.stderr.write(f" - {violation}\n") + return 1 + sys.stdout.write("links: OK\n") + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/pyproject.toml b/pyproject.toml index 7f5268d..7086185 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -94,7 +94,8 @@ omit = [ "tests/migrations/*", "app/api/__main__.py", # No __init__.py under planning/ (see the comment atop planning/index.py), so coverage's - # package walk never reaches this file on its own - omit it explicitly rather than relying + # package walk never reaches these on its own - omit them explicitly rather than relying # on that as an accident of discovery. "planning/index.py", + "planning/links.py", ] diff --git a/readme.md b/readme.md index a143f44..498d8b1 100644 --- a/readme.md +++ b/readme.md @@ -47,8 +47,8 @@ domain. Nothing shows them composed under load-bearing decisions — a transaction that must span two writes, a unique constraint that two concurrent requests can both hit, a count that must not cost a row per event. This repo answers that with a domain that actually needs it. See -`planning/changes/2026-08-21.01-chat-app-bootstrap.md` for the full design and -`architecture/` for the capabilities as shipped. +[PR #1](https://github.com/modern-python/chat-app/pull/1) for the full design +and `planning/decisions/` for the calls taken along the way. | Pattern | Where to look | |---|---| diff --git a/tests/api/test_auth_api.py b/tests/api/test_auth_api.py index 35f712b..b341bd2 100644 --- a/tests/api/test_auth_api.py +++ b/tests/api/test_auth_api.py @@ -94,6 +94,12 @@ async def test_me_rejects_tampered_cookie(client: AsyncClient) -> None: @pytest.mark.usefixtures("db_session") async def test_me_rejects_token_with_non_numeric_subject(client: AsyncClient) -> None: + """INVARIANT: a token whose subject is not an integer yields 401, not a 500. + + Broken by letting int(token.sub) raise out of retrieve_user_handler — it runs + inside auth middleware, so an uncaught ValueError there is an unhandled server + error on a request an attacker fully controls the token for. + """ token = jwt_cookie_auth.create_token(identifier="not-a-number") client.cookies.set(jwt_cookie_auth.key, token) response = await client.get("/api/auth/me/") diff --git a/tests/use_cases/test_create_chat.py b/tests/use_cases/test_create_chat.py index 4d0d4b2..c26cd07 100644 --- a/tests/use_cases/test_create_chat.py +++ b/tests/use_cases/test_create_chat.py @@ -88,6 +88,13 @@ async def test_direct_chat_is_idempotent_for_the_same_pair( async def test_direct_chat_creation_recovers_from_a_concurrent_duplicate_key( create_chat_use_case: CreateChatUseCase, alice: tables.UsersTable, bob: tables.UsersTable ) -> None: + """INVARIANT: a direct-chat insert losing the uq_chats_direct_key race recovers the winner's row. + + Broken by returning from inside the `async with self.transaction:` block, or + by re-reading before the rollback: Transaction.__aexit__ rolls back and closes + the session on an open transaction, expiring every loaded attribute, so the + recovery read must happen after the block exits. + """ winner, winner_created = await create_chat_use_case( actor=alice, data=schemas.CreateChatRequest(chat_type=tables.ChatType.DIRECT, member_ids=[bob.id]) ) diff --git a/tests/use_cases/test_create_message.py b/tests/use_cases/test_create_message.py index 598a503..68ef8d3 100644 --- a/tests/use_cases/test_create_message.py +++ b/tests/use_cases/test_create_message.py @@ -108,6 +108,12 @@ async def test_non_member_cannot_send( async def test_concurrent_duplicate_key_recovers_the_winners_message( create_message_use_case: CreateMessageUseCase, direct_chat: tables.ChatsTable, alice: tables.UsersTable ) -> None: + """INVARIANT: a message insert losing the (chat_id, idempotency_key) race returns the winner's row. + + Broken by dropping the explicit `await self.transaction.rollback()` before the + recovery read, or by widening the caught exception beyond DuplicateKeyError so + an unrelated integrity error is silently funnelled into recovery. + """ key = uuid.uuid4() winner, winner_created = await create_message_use_case( actor=alice, chat_id=direct_chat.id, data=schemas.SendMessageRequest(idempotency_key=key, text="hi")