diff --git a/CLAUDE.md b/CLAUDE.md index 7bcb684..1087ce0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -110,8 +110,9 @@ a session, a transaction or a repository directly. Provider scopes: `BigIntBase`. `app/database/tables.py` shares metadata with `orm.DeclarativeBase.metadata` (`METADATA = orm_registry.metadata; orm.DeclarativeBase.metadata = METADATA`) so Alembic autogen sees everything — -this line mutates a third-party base class at import time; see the comment -above it in the source for why. Repositories are +this line mutates a third-party base class at import time, because otherwise +models register on `orm.DeclarativeBase`'s own metadata and autogen sees no +tables at all. Repositories are `SQLAlchemyAsyncRepositoryService[Model]` with a nested `BaseRepository(SQLAlchemyAsyncRepository[Model])`, same shape as the template, but every service here is constructed with `auto_commit=False`. @@ -172,6 +173,16 @@ env vars (see `docker-compose.yml`). `api_bootstrapper_config` builds the mapping table and the one deliberate exception (login's `401` via Litestar's own `NotAuthorizedException`) are in `architecture/messages.md` and `architecture/auth.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. + 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 + `ForeignKey`, a `return` from inside a transaction block, discarded work that + is not dead code (`AuthenticateUserUseCase`'s hash-anyway). Alembic's own + `# ###` autogenerate markers stay — they are regenerated on every migration. - `ruff` is configured with `select = ["ALL"]` and a line length of 120 — expect strict lint. Type-check with `ty`; use `# ty: ignore[]` for suppressions. diff --git a/app/api/app.py b/app/api/app.py index b64dfd9..c4228d5 100644 --- a/app/api/app.py +++ b/app/api/app.py @@ -35,10 +35,6 @@ def build_app() -> litestar.Litestar: ConflictError: exception_handlers.conflict_error_handler, }, route_handlers=[auth_endpoints.ROUTER, chats_endpoints.ROUTER, messages_endpoints.ROUTER], - # autowired_groups exposes one Litestar dependency per UseCases provider, named - # after the provider attribute - which is what every handler parameter is already - # called. Database and Repositories are deliberately left out: route handlers have - # no business resolving a session, a transaction or a repository directly. plugins=[ modern_di_litestar.ModernDIPlugin(di_container, autowired_groups=[ioc.UseCases]), JWTCookieAuthPlugin(), @@ -47,8 +43,7 @@ def build_app() -> litestar.Litestar: ), opentelemetry_instrumentors=[ SQLAlchemyInstrumentor(), - # False: bound query parameters include argon2 password hashes (every registration - # INSERTs one) - capturing them would ship credential material to the OTel collector. + # True would ship argon2 password hashes, bound as INSERT parameters, to the collector. AsyncPGInstrumentor(capture_parameters=False), ], ) diff --git a/app/api/auth.py b/app/api/auth.py index 47a0a41..c57264f 100644 --- a/app/api/auth.py +++ b/app/api/auth.py @@ -14,21 +14,14 @@ from app.settings import settings -# Every authenticated handler annotates its request with this; `request.user` is a UsersTable -# because retrieve_user_handler below is what populates it. type AuthedRequest = litestar.Request[tables.UsersTable, Token, typing.Any] async def retrieve_user_handler(token: Token, connection: ASGIConnection) -> tables.UsersTable | None: - # Auth middleware runs before request-scoped DI is available, so resolve the app-scoped - # engine and open a short-lived session through the same factory the container uses. That - # factory sets join_transaction_mode="create_savepoint", which is what keeps the per-test - # rollback fixture intact when the engine provider is overridden with a live connection. + # Auth middleware runs before request-scoped DI exists; see architecture/auth.md. try: user_id = int(token.sub) except ValueError: - # Token.sub is only guaranteed to be a non-empty string; a malformed/forged subject - # must fail authentication (401 via the middleware), not crash the request (500). return None di_container: typing.Final = modern_di_litestar.fetch_di_container(connection.app) engine: typing.Final = di_container.resolve_provider(ioc.Database.database_engine) @@ -44,32 +37,17 @@ async def retrieve_user_handler(token: Token, connection: ASGIConnection) -> tab token_secret=settings.jwt_secret, default_token_expiration=datetime.timedelta(seconds=settings.jwt_lifetime_seconds), secure=settings.jwt_cookie_secure, + # Anchored: Litestar matches the joined patterns with an unanchored findall. exclude=[ - # /auth/register and /auth/login opt out via exclude_from_auth=True on the handlers - # themselves (see app/api/endpoints/auth.py) - that is their one policy home, not here. - # Litestar joins these into a single alternation and matches with an unanchored findall - # (litestar/middleware/_utils.py), so each pattern is anchored to the path start to avoid - # accidentally un-authenticating a future route that merely contains "/docs" or "/health" - # as a substring (e.g. "/api/chats/{id}/health"). "^/docs", "^/health", - # Swagger's offline assets (settings.swagger_offline_docs=True) are served from here; - # without this the docs page loads but every asset request 401s for an anonymous visitor. "^/static", - # A Prometheus scrape target must be reachable without a session cookie; the endpoint - # carries no user data, only process/request metrics. "^/metrics", ], ) class JWTCookieAuthPlugin(InitPlugin): - # AppConfig has no on_app_init field (that hook is a Litestar.__init__-only parameter, - # unavailable through LitestarBootstrapper's AppConfig -> Litestar.from_config path), and - # jwt_cookie_auth itself is an unhashable dataclass so it cannot sit in `plugins` directly - # (PluginRegistry stores plugins in a frozenset). This plugin wrapper is hashable by identity - # and forwards to jwt_cookie_auth.on_app_init, which Litestar.__init__ calls for every - # InitPluginProtocol member of `plugins` after the bootstrapper has finished mutating - # application_config (so openapi_config is already populated by then). + # jwt_cookie_auth is an unhashable dataclass, so it cannot go in `plugins` itself. def on_app_init(self, app_config: AppConfig) -> AppConfig: return jwt_cookie_auth.on_app_init(app_config) diff --git a/app/api/exception_handlers.py b/app/api/exception_handlers.py index 84b4f76..59807b7 100644 --- a/app/api/exception_handlers.py +++ b/app/api/exception_handlers.py @@ -29,8 +29,7 @@ def duplicate_key_error_handler(_: object, __: DuplicateKeyError) -> litestar.Re def foreign_key_error_handler(_: object, __: ForeignKeyError) -> litestar.Response[dict[str, typing.Any]]: return litestar.Response( media_type=litestar.MediaType.JSON, - # Constant detail, not str(exc): the underlying integrity error can carry bound - # parameter values (e.g. ids from other tables) that shouldn't be echoed back verbatim. + # Not str(exc): integrity errors can carry bound parameter values from other rows. content={"detail": "Invalid reference"}, status_code=status_codes.HTTP_400_BAD_REQUEST, ) diff --git a/app/database/resources.py b/app/database/resources.py index f88291d..49a70d3 100644 --- a/app/database/resources.py +++ b/app/database/resources.py @@ -22,11 +22,7 @@ async def close_database_engine(engine: sa.AsyncEngine) -> None: def create_session(engine: sa.AsyncEngine | sa.AsyncConnection) -> sa.AsyncSession: - # join_transaction_mode is inert in production (the session binds to an engine); when tests bind - # the session to a connection already in a transaction, it makes the session own a savepoint so - # the outer transaction survives commits and the per-test rollback stays clean. The `db_session` - # test fixture overrides `Database.database_engine` with a live `AsyncConnection`, so DI-resolved - # sessions built through this same function get that connection, not just an `AsyncEngine`. + # join_transaction_mode is inert in production; it makes test-bound sessions nest as savepoints. return sa.AsyncSession( engine, expire_on_commit=False, diff --git a/app/database/tables.py b/app/database/tables.py index 27776d5..315dedb 100644 --- a/app/database/tables.py +++ b/app/database/tables.py @@ -10,11 +10,7 @@ METADATA: typing.Final = orm_registry.metadata -# Redirects SQLAlchemy's shared declarative base onto advanced-alchemy's registry metadata so -# that every model below - which inherits BigIntAuditBase/BigIntBase, themselves built on -# orm.DeclarativeBase - registers its table on METADATA. Alembic's env.py autogenerates against -# METADATA directly; without this reassignment, models would register on orm.DeclarativeBase's -# own separate metadata instead, and autogen would see no tables at all. +# Without this, models register on orm.DeclarativeBase's own metadata and autogen sees no tables. orm.DeclarativeBase.metadata = METADATA @@ -56,19 +52,8 @@ class ChatsTable(BigIntAuditBase): "ChatMembersTable", lazy="noload", uselist=True, viewonly=True ) - # Per-viewer state that the chat listing needs alongside the chat's own columns. Both are - # mapped here rather than assembled in Python so that a listed chat is a plain ChatsTable - # whose attribute names already match the response schema - no per-row DTO, no aliases. - # - # unread_count is only populated when the query asks for it via with_expression(); every - # other query gets default_expr's literal 0, so the attribute is never None. unread_count: orm.Mapped[int] = orm.query_expression(default_expr=sa.literal(0)) - # last_message_id deliberately carries no ForeignKey (a chats -> messages FK would close a - # cycle with messages.chat_id), so the join column has to be annotated foreign() by hand. - # The soft-delete guard lives in the join rather than at the call site: a chat must never - # preview a deleted message, whatever loads it. DeleteMessageUseCase still repoints - # last_message_id in the same commit as the delete - this only keeps the mapping correct on - # its own if some other write path ever fails to. + # last_message_id carries no ForeignKey (it would cycle with messages.chat_id), hence foreign(). last_message: orm.Mapped[MessagesTable | None] = orm.relationship( "MessagesTable", primaryjoin=lambda: sa.and_( @@ -96,11 +81,7 @@ class ChatMembersTable(BigIntBase): class MessagesTable(BigIntBase): __tablename__ = "messages" __table_args__ = ( - # No standalone index on chat_id: this composite index already serves every query - # that would use one. sa.Index("ix_messages_chat_id_id", "chat_id", "id"), - # Idempotency is a property of "send this message to this chat" - two different chats - # are two different operations, so the key is unique per chat, not table-wide. sa.UniqueConstraint("chat_id", "idempotency_key", name="uk_messages_chat_id_idempotency_key"), ) diff --git a/app/repositories/chats_repository.py b/app/repositories/chats_repository.py index fbb3d74..1c318e2 100644 --- a/app/repositories/chats_repository.py +++ b/app/repositories/chats_repository.py @@ -49,10 +49,7 @@ async def list_for_user(self, user_id: int) -> Sequence[tables.ChatsTable]: .join(tables.ChatMembersTable, tables.ChatMembersTable.chat_id == tables.ChatsTable.id) .where(tables.ChatMembersTable.user_id == user_id) .order_by(sa.func.coalesce(tables.ChatsTable.last_message_id, 0).desc()) - # Sessions run expire_on_commit=False, so a ChatsTable already in the identity map - # keeps whatever unread_count/last_message it was loaded with; without this, a second - # listing in the same session would hand back the first one's values. Safe here only - # because this query is read-only - populate_existing overwrites in-memory state. + # expire_on_commit=False would otherwise leave identity-mapped chats holding stale values. .execution_options(populate_existing=True) ) result: typing.Final = await self.repository.session.execute(statement) diff --git a/app/schemas/api.py b/app/schemas/api.py index 28b8279..42c0c0c 100644 --- a/app/schemas/api.py +++ b/app/schemas/api.py @@ -87,8 +87,6 @@ class Messages(Collection[Message]): class ChatListItem(Chat): - # ChatsTable maps unread_count and last_message itself (see app/database/tables.py), so a - # listed chat validates straight through from_attributes like any other ORM instance. last_message: Message | None = None unread_count: int = 0 diff --git a/app/settings.py b/app/settings.py index a5a771d..ce8cb86 100644 --- a/app/settings.py +++ b/app/settings.py @@ -13,10 +13,6 @@ class Settings(pydantic_settings.BaseSettings): service_name: str = "chat-app" service_version: str = "1.0.0" service_environment: str = "local" - # Enabling this turns on SQLAlchemy's echo/echo_pool (app/database/resources.py), which logs - # every statement WITH ITS BOUND PARAMETERS - including argon2 password_hash values on every - # registration - and makes Litestar return stack traces in responses. Never set True outside - # a throwaway local session. service_debug: bool = False log_level: str = "info" @@ -30,8 +26,7 @@ class Settings(pydantic_settings.BaseSettings): jwt_secret: str = INSECURE_JWT_SECRET jwt_lifetime_seconds: int = 60 * 60 * 24 * 7 - # Litestar leaves this unset by default. Production MUST set this to True (it requires - # serving over HTTPS); left False here so local http:// development still gets the cookie. + # False so local http:// development still gets the cookie; production must set True. jwt_cookie_secure: bool = False opentelemetry_endpoint: str = "" @@ -47,8 +42,6 @@ class Settings(pydantic_settings.BaseSettings): request_max_body_size: int = 1024 * 1024 def ensure_jwt_secret_is_configured(self) -> None: - # The whole auth boundary is a token signed with jwt_secret: with the shipped default, - # anyone can forge a token for any user.id. Only "local" may run with it. if self.service_environment != "local" and self.jwt_secret == INSECURE_JWT_SECRET: message = ( f"jwt_secret is still the insecure default while service_environment=" diff --git a/app/use_cases/authenticate_user.py b/app/use_cases/authenticate_user.py index 7160920..e830466 100644 --- a/app/use_cases/authenticate_user.py +++ b/app/use_cases/authenticate_user.py @@ -16,8 +16,7 @@ class AuthenticateUserUseCase: async def __call__(self, *, username: str, password: str) -> tables.UsersTable | None: user: typing.Final = await self.users_repository.get_one_or_none(username=username) if user is None: - # Hash anyway: skipping the argon2 work on an unknown username makes the - # response measurably faster and turns login into a username oracle. + # Discarded, but skipping the argon2 work would turn login timing into a username oracle. security.hash_password(password) return None if not security.verify_password(user.password_hash, password): diff --git a/app/use_cases/create_chat.py b/app/use_cases/create_chat.py index 3b5633e..2435ac9 100644 --- a/app/use_cases/create_chat.py +++ b/app/use_cases/create_chat.py @@ -32,16 +32,7 @@ async def __call__(self, *, actor: tables.UsersTable, data: CreateChatRequest) - low, high = sorted(member_ids) direct_key = tables.build_direct_key(low, high) - # Kept outside the `async with` block below: Transaction.__aexit__ unconditionally - # rolls back and closes the session whenever it is left with an open, uncommitted - # transaction (session.in_transaction() True with no prior commit()), which expires - # and detaches every loaded attribute. A `return` from inside the block after this - # read - with no commit following it - would trigger exactly that on `existing`. - # (For direct chats specifically, this SELECT autobegins the session's transaction, - # so the `async with self.transaction:` block below actually *joins* that same - # transaction rather than starting a new one - see Transaction.__aenter__. That does - # not change the __aexit__ hazard above; it only means direct and group chats reach - # the block by different routes.) + # Outside the block: __aexit__ rolls back an uncommitted transaction and detaches this. existing = await self.chats_repository.fetch_direct_by_key(direct_key) if existing is not None: return existing, False @@ -58,16 +49,9 @@ async def __call__(self, *, actor: tables.UsersTable, data: CreateChatRequest) - ) ) except DuplicateKeyError: - # Two concurrent requests to open the same direct chat both passed the - # fetch_direct_by_key pre-check above and both tried to insert; the loser hits - # uq_chats_direct_key here. Group chats have no unique constraint on `chats` to - # violate, so an unexpected DuplicateKeyError there re-raises and maps to the - # standard 409 instead of being funnelled into this direct-chat recovery path. + # Group chats have no unique constraint here, so a collision is not recoverable. if direct_key is None: raise - # Roll back and re-read the winner's row outside this block (same __aexit__ - # hazard as the comment above): a `return` from in here without a commit - # first would detach whatever `fetch_direct_by_key` loaded, same as before. await self.transaction.rollback() else: for user_id in sorted(member_ids): @@ -81,5 +65,4 @@ async def __call__(self, *, actor: tables.UsersTable, data: CreateChatRequest) - raise RuntimeError(msg) return existing, False - # Kept outside the `async with` block for the same reason as the lookup above. return await self.chats_repository.fetch_with_members(chat.id), True diff --git a/app/use_cases/create_message.py b/app/use_cases/create_message.py index 53a21a0..27acfc5 100644 --- a/app/use_cases/create_message.py +++ b/app/use_cases/create_message.py @@ -43,11 +43,7 @@ async def __call__( ) ) except DuplicateKeyError: - # Two concurrent retries of the same (chat_id, key); the loser reads the winner's - # row. Roll back and re-read outside this block (Transaction.__aexit__ - # unconditionally rolls back and closes the session on an open, uncommitted - # transaction, which expires every loaded attribute - a `return` from in here - # would detach whatever fetch_by_idempotency_key loaded). + # Re-read outside the block: __aexit__ rolls back and detaches on an open transaction. await self.transaction.rollback() else: await self.chats_repository.update( diff --git a/app/use_cases/delete_message.py b/app/use_cases/delete_message.py index 977a565..32b98eb 100644 --- a/app/use_cases/delete_message.py +++ b/app/use_cases/delete_message.py @@ -28,20 +28,13 @@ async def __call__(self, *, actor: tables.UsersTable, message_id: int) -> None: action="delete", ) if message.deleted_at is not None: - # DELETE is idempotent under HTTP semantics: a second delete of an - # already-deleted message is not an error, unlike PATCH via EditMessageUseCase. + # DELETE is idempotent; a second delete is not an error, unlike an edit. return message.deleted_at = datetime.datetime.now(tz=datetime.UTC) await self.messages_repository.update(message, item_id=message_id) chat = await self.chats_repository.get_one(id=message.chat_id) if chat.last_message_id == message_id: - # chats.last_message_id means "the newest non-deleted message in this chat" - the - # chat listing's preview and its ordering both read this column, so repointing it - # here (in the same commit as the soft delete) is what keeps a delete from being - # only half-effective: leaving it pointed at a deleted message would make the - # listing preview show deleted text and rank the chat by a message that no longer - # counts. newest = await self.messages_repository.fetch_latest_active(message.chat_id) await self.chats_repository.update( tables.ChatsTable(id=message.chat_id, last_message_id=newest.id if newest is not None else None), diff --git a/app/use_cases/edit_message.py b/app/use_cases/edit_message.py index 7721601..21bf332 100644 --- a/app/use_cases/edit_message.py +++ b/app/use_cases/edit_message.py @@ -36,8 +36,5 @@ async def __call__( message.edited_at = datetime.datetime.now(tz=datetime.UTC) updated = await self.messages_repository.update(message, item_id=message_id) await self.transaction.commit() - # Returned from inside the block, right after commit(): __aexit__ then sees no open - # transaction (commit ended it) and only closes the session - it does not roll back, - # so `updated`'s already-loaded attributes (no relationships here to eager-load) stay - # usable for the caller. + # Safe inside the block: commit() ended the transaction, so __aexit__ only closes. return updated diff --git a/app/use_cases/fetch_chats.py b/app/use_cases/fetch_chats.py index 03f606c..e8ae31e 100644 --- a/app/use_cases/fetch_chats.py +++ b/app/use_cases/fetch_chats.py @@ -13,7 +13,4 @@ class FetchChatsUseCase: @postgres_retry async def __call__(self, *, actor: tables.UsersTable) -> Sequence[tables.ChatsTable]: - # Each returned chat already carries its unread_count and last_message: the count comes - # from a correlated subquery on the same statement and the preview from one selectinload, - # so this is two round trips for the whole list regardless of how many chats it holds. return await self.chats_repository.list_for_user(actor.id) diff --git a/app/use_cases/mark_read.py b/app/use_cases/mark_read.py index 7188e23..d7d87f1 100644 --- a/app/use_cases/mark_read.py +++ b/app/use_cases/mark_read.py @@ -25,23 +25,13 @@ async def __call__( msg = "Not a member of this chat" raise PermissionDeniedError(msg) - # The requested marker must name a real message in *this* chat - otherwise a client could - # set it to an arbitrary large id and permanently zero its own unread count. target_message = await self.messages_repository.get_one_or_none(id=data.last_read_message_id, chat_id=chat_id) if target_message is None: msg = "last_read_message_id does not name a message in this chat" raise ValidationError(msg) async with self.transaction: - # Monotonic: an out-of-order or replayed request naming an earlier message must not - # move the marker backwards and resurrect messages that were already marked read. - # The GREATEST(...) that enforces this lives in the UPDATE itself (see - # ChatMembersRepository.mark_read) rather than being computed here from `member`'s - # already-read value - that would be a read-modify-write race between concurrent - # POST /read/ calls. updated = await self.chat_members_repository.mark_read(member.id, data.last_read_message_id) await self.transaction.commit() - # Returned from inside the block, right after commit(): __aexit__ then sees no open - # transaction (commit ended it) and only closes the session - it does not roll back, - # so `updated`'s already-loaded attributes stay usable for the caller. + # Safe inside the block: commit() ended the transaction, so __aexit__ only closes. return updated diff --git a/architecture/auth.md b/architecture/auth.md index 4c8bd0c..826f653 100644 --- a/architecture/auth.md +++ b/architecture/auth.md @@ -16,6 +16,14 @@ inside its own transaction and returns `201` with the cookie set via 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`), diff --git a/migrations/env.py b/migrations/env.py index 86f1311..5b80f49 100644 --- a/migrations/env.py +++ b/migrations/env.py @@ -8,8 +8,7 @@ from app.settings import settings -# Imported for its side effect: registering the autogenerate hooks that render CREATE TYPE / -# ALTER TYPE ... ADD VALUE for native Postgres enums. +# Imported for its side effect: enum-aware autogenerate hooks. _ = alembic_postgresql_enum diff --git a/migrations/script.py.mako b/migrations/script.py.mako index a65d971..a3bae58 100644 --- a/migrations/script.py.mako +++ b/migrations/script.py.mako @@ -10,7 +10,6 @@ from alembic import op import sqlalchemy as sa ${imports if imports else ""} -# revision identifiers, used by Alembic. revision = ${repr(up_revision)} down_revision = ${repr(down_revision)} branch_labels = ${repr(branch_labels)} diff --git a/migrations/versions/2026-08-21_chat_type_native_enum.py b/migrations/versions/2026-08-21_chat_type_native_enum.py index db76f0d..f1e7607 100644 --- a/migrations/versions/2026-08-21_chat_type_native_enum.py +++ b/migrations/versions/2026-08-21_chat_type_native_enum.py @@ -10,7 +10,6 @@ from alembic import op -# revision identifiers, used by Alembic. revision = "fa15d87677c3" down_revision = "1be68642e392" branch_labels = None diff --git a/migrations/versions/2026-08-21_chats_and_members.py b/migrations/versions/2026-08-21_chats_and_members.py index a425e69..e6e382f 100644 --- a/migrations/versions/2026-08-21_chats_and_members.py +++ b/migrations/versions/2026-08-21_chats_and_members.py @@ -11,7 +11,6 @@ from alembic import op -# revision identifiers, used by Alembic. revision = "88ba0ea3f7e6" down_revision = "b8565e6bbe4b" branch_labels = None diff --git a/migrations/versions/2026-08-21_init.py b/migrations/versions/2026-08-21_init.py index 1895d08..255e5f2 100644 --- a/migrations/versions/2026-08-21_init.py +++ b/migrations/versions/2026-08-21_init.py @@ -11,7 +11,6 @@ from alembic import op -# revision identifiers, used by Alembic. revision = "b8565e6bbe4b" down_revision = None branch_labels = None diff --git a/migrations/versions/2026-08-21_messages.py b/migrations/versions/2026-08-21_messages.py index 8a5b45a..fe4cf3f 100644 --- a/migrations/versions/2026-08-21_messages.py +++ b/migrations/versions/2026-08-21_messages.py @@ -11,7 +11,6 @@ from alembic import op -# revision identifiers, used by Alembic. revision = "1be68642e392" down_revision = "88ba0ea3f7e6" branch_labels = None diff --git a/planning/changes/2026-08-21.04-comment-sweep.md b/planning/changes/2026-08-21.04-comment-sweep.md new file mode 100644 index 0000000..b89a10c --- /dev/null +++ b/planning/changes/2026-08-21.04-comment-sweep.md @@ -0,0 +1,76 @@ +--- +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/tests/api/test_auth_api.py b/tests/api/test_auth_api.py index bc38f69..35f712b 100644 --- a/tests/api/test_auth_api.py +++ b/tests/api/test_auth_api.py @@ -94,8 +94,6 @@ 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: - # Exercises retrieve_user_handler's int(token.sub) ValueError guard: Token only requires - # sub to be a non-empty string, so a forged/malformed subject must 401, not 500. 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/") @@ -114,8 +112,6 @@ async def test_metrics_are_reachable_without_a_cookie(client: AsyncClient) -> No @pytest.mark.usefixtures("db_session") async def test_me_rejects_token_for_a_user_that_no_longer_exists(client: AsyncClient) -> None: - # A validly signed token whose subject has no matching row: session.get returns None and - # the middleware must turn that into 401, not treat it as an authenticated request. token = jwt_cookie_auth.create_token(identifier="999999999") client.cookies.set(jwt_cookie_auth.key, token) response = await client.get("/api/auth/me/") diff --git a/tests/api/test_chat_listing_api.py b/tests/api/test_chat_listing_api.py index da0af7a..7db98c6 100644 --- a/tests/api/test_chat_listing_api.py +++ b/tests/api/test_chat_listing_api.py @@ -10,7 +10,7 @@ @pytest.mark.usefixtures("db_session") async def test_listing_returns_only_the_callers_chats(client: AsyncClient) -> None: chat_id, _ = await _create_direct_chat(client) - await _register(client, "mallory") # mallory is not a member of any chat + await _register(client, "mallory") mallory_response = await client.get("/api/chats/") await _login(client, "alice") diff --git a/tests/api/test_message_mutations_api.py b/tests/api/test_message_mutations_api.py index b906c70..690c1f3 100644 --- a/tests/api/test_message_mutations_api.py +++ b/tests/api/test_message_mutations_api.py @@ -23,7 +23,7 @@ async def test_author_can_edit_message(client: AsyncClient) -> None: async def test_non_author_member_cannot_edit_message(client: AsyncClient) -> None: chat_id, _ = await _create_direct_chat(client) message = await _send(client, chat_id, "hi") - await _login(client, "bob") # bob is a member of the chat but not the author + await _login(client, "bob") response = await client.patch(f"/api/messages/{message['id']}/", json={"text": "nope"}) @@ -34,7 +34,7 @@ async def test_non_author_member_cannot_edit_message(client: AsyncClient) -> Non async def test_non_member_cannot_edit_message(client: AsyncClient) -> None: chat_id, _ = await _create_direct_chat(client) message = await _send(client, chat_id, "hi") - await _register(client, "mallory") # mallory is not in the chat at all + await _register(client, "mallory") response = await client.patch(f"/api/messages/{message['id']}/", json={"text": "nope"}) @@ -55,7 +55,7 @@ async def test_author_can_delete_message(client: AsyncClient) -> None: async def test_non_author_member_cannot_delete_message(client: AsyncClient) -> None: chat_id, _ = await _create_direct_chat(client) message = await _send(client, chat_id, "hi") - await _login(client, "bob") # bob is a member of the chat but not the author + await _login(client, "bob") response = await client.delete(f"/api/messages/{message['id']}/") @@ -66,7 +66,7 @@ async def test_non_author_member_cannot_delete_message(client: AsyncClient) -> N async def test_non_member_cannot_delete_message(client: AsyncClient) -> None: chat_id, _ = await _create_direct_chat(client) message = await _send(client, chat_id, "hi") - await _register(client, "mallory") # mallory is not in the chat at all + await _register(client, "mallory") response = await client.delete(f"/api/messages/{message['id']}/") diff --git a/tests/migrations/conftest.py b/tests/migrations/conftest.py index 4f49702..d43fe80 100644 --- a/tests/migrations/conftest.py +++ b/tests/migrations/conftest.py @@ -8,8 +8,7 @@ @pytest.fixture def alembic_engine() -> typing.Iterator[Engine]: - # Overrides pytest-alembic's default in-memory SQLite engine: these tests are only - # meaningful against the Postgres the migrations actually target. + # Replaces pytest-alembic's default in-memory SQLite engine. engine: typing.Final = create_engine(settings.sync_db_dsn_parsed) yield engine engine.dispose() diff --git a/tests/test_main.py b/tests/test_main.py index efd97cb..02dcc2f 100644 --- a/tests/test_main.py +++ b/tests/test_main.py @@ -43,9 +43,6 @@ async def test_permission_denied_handler_defaults_message_when_empty() -> None: async def test_create_database_engine_reads_settings_and_can_be_disposed() -> None: - # Exercises `close_database_engine` too: nothing else calls it, since the `db_session` - # fixture always overrides `Database.database_engine` before it is ever resolved, so its - # cache finalizer never fires. engine = create_database_engine() try: assert isinstance(engine.pool, QueuePool) @@ -73,10 +70,6 @@ async def test_di_resolved_session_shares_the_overridden_connection( di_container: modern_di.Container, db_session: AsyncSession, ) -> None: - # Proves the load-bearing part of the `db_session` fixture: a request-scoped session - # resolved through the real DI provider (`create_session`/`close_session`, the path - # production route handlers use) sees writes made on the fixture's own session, because - # both share the connection that `db_session` overrode `Database.database_engine` with. user = UserFactory.build() db_session.add(user) await db_session.flush() diff --git a/tests/test_schemas.py b/tests/test_schemas.py index 3d76f28..cfb6b81 100644 --- a/tests/test_schemas.py +++ b/tests/test_schemas.py @@ -3,8 +3,6 @@ def test_collection_builds_from_models() -> None: - # Every real caller passes SQLAlchemy ORM rows, which is why `Base` sets - # from_attributes=True; a dict here wouldn't exercise the attribute-access path. user = UserFactory.build(id=1, username="alice", display_name="Alice") collection = Collection[User].from_models([user]) assert collection.items == [User(id=1, username="alice", display_name="Alice")] diff --git a/tests/test_settings.py b/tests/test_settings.py index b424dbd..0162f88 100644 --- a/tests/test_settings.py +++ b/tests/test_settings.py @@ -31,8 +31,7 @@ def test_ensure_jwt_secret_is_configured_allows_a_real_secret_outside_local() -> def test_ensure_jwt_secret_is_configured_rejects_the_default_secret_outside_local() -> None: - # jwt_secret set explicitly (rather than left to the JWT_SECRET env var, which the test - # container sets) to isolate this test from the environment it happens to run in. + # Explicit, not left to the JWT_SECRET the test container sets. settings = Settings(service_environment="production", jwt_secret=INSECURE_JWT_SECRET) with pytest.raises(RuntimeError, match="jwt_secret"): settings.ensure_jwt_secret_is_configured() diff --git a/tests/use_cases/conftest.py b/tests/use_cases/conftest.py index 7cdb048..3b53b26 100644 --- a/tests/use_cases/conftest.py +++ b/tests/use_cases/conftest.py @@ -23,9 +23,7 @@ async def request_container( yield container -# One pytest fixture per provider on both groups, named after the class attribute. -# Every use case and repository added in later tasks becomes a fixture automatically, -# so no test file has to hand-assemble dependencies. +# Declares one same-named pytest fixture per provider on both groups. expose(ioc.Repositories, ioc.UseCases, container_fixture="request_container") diff --git a/tests/use_cases/test_create_chat.py b/tests/use_cases/test_create_chat.py index 27d9b76..4d0d4b2 100644 --- a/tests/use_cases/test_create_chat.py +++ b/tests/use_cases/test_create_chat.py @@ -82,27 +82,19 @@ async def test_direct_chat_is_idempotent_for_the_same_pair( assert first.id == second.id assert first_created is True assert second_created is False - # `second` is returned from the early-return, no-write path (existing direct chat found). - # Its relationship must still be readable without triggering a lazy load on a closed/rolled-back session. assert {member.user_id for member in second.members} == {alice.id, bob.id} async def test_direct_chat_creation_recovers_from_a_concurrent_duplicate_key( create_chat_use_case: CreateChatUseCase, alice: tables.UsersTable, bob: tables.UsersTable ) -> None: - # A real winner: create the direct chat normally first, so a genuinely committed row exists. winner, winner_created = await create_chat_use_case( actor=alice, data=schemas.CreateChatRequest(chat_type=tables.ChatType.DIRECT, member_ids=[bob.id]) ) assert winner_created is True - # Captured now, not read off `winner` after the racer runs: the racer shares this session, - # and its own recovery `rollback()` expires every object already loaded on that session - - # including `winner` - exactly the hazard the surrounding comments describe, just now - # crossing between two calls that happen to share a session instead of within one call. + # Captured now: the racer's recovery rollback() expires `winner` on the shared session. winner_id = winner.id - # The losing side of the race, sharing `create_chat_use_case`'s own transaction/session so - # the winner row (committed above) is visible to the recovery re-read. racer = CreateChatUseCase( transaction=create_chat_use_case.transaction, chats_repository=_RacingChatsRepository( @@ -134,9 +126,6 @@ async def test_direct_chat_recovery_raises_if_the_winners_row_is_unreadable( async def test_group_chat_reraises_an_unexpected_duplicate_key( create_chat_use_case: CreateChatUseCase, alice: tables.UsersTable, bob: tables.UsersTable ) -> None: - # Group chats have no unique constraint to race on `chats`; a DuplicateKeyError there is - # unexpected and must propagate (mapping to the standard 409), not be funnelled into the - # direct-chat recovery path. broken = CreateChatUseCase( transaction=create_chat_use_case.transaction, chats_repository=_AlwaysDuplicateChatsRepository( diff --git a/tests/use_cases/test_create_message.py b/tests/use_cases/test_create_message.py index 530473a..598a503 100644 --- a/tests/use_cases/test_create_message.py +++ b/tests/use_cases/test_create_message.py @@ -115,8 +115,6 @@ async def test_concurrent_duplicate_key_recovers_the_winners_message( assert winner_created is True winner_id = winner.id - # Shares the winner's session/transaction so the committed row is visible to the recovery - # re-read, same setup as CreateChatUseCase's equivalent race test. racer = CreateMessageUseCase( transaction=create_message_use_case.transaction, chats_repository=create_message_use_case.chats_repository, @@ -159,9 +157,6 @@ async def test_same_idempotency_key_in_two_different_chats_creates_two_messages( alice: tables.UsersTable, carol: tables.UsersTable, ) -> None: - # Idempotency is scoped per (chat_id, idempotency_key): the key identifies a retry of - # "send to this chat", not a retry across the whole table, so reusing it in a different - # chat is a second, independent send. other_chat, _ = await create_chat_use_case( actor=alice, data=schemas.CreateChatRequest(chat_type=tables.ChatType.DIRECT, member_ids=[carol.id]) ) diff --git a/tests/use_cases/test_edit_message.py b/tests/use_cases/test_edit_message.py index a483634..e74fa55 100644 --- a/tests/use_cases/test_edit_message.py +++ b/tests/use_cases/test_edit_message.py @@ -26,7 +26,6 @@ async def test_author_can_edit( async def test_other_member_cannot_edit( edit_message_use_case: EditMessageUseCase, alice_message: tables.MessagesTable, bob: tables.UsersTable ) -> None: - # bob is a member of the chat, not the author - membership alone must not authorize the edit. with pytest.raises(PermissionDeniedError): await edit_message_use_case( actor=bob, message_id=alice_message.id, data=schemas.EditMessageRequest(text="nope") @@ -46,9 +45,6 @@ async def test_author_without_membership_cannot_edit( alice_message: tables.MessagesTable, alice: tables.UsersTable, ) -> None: - # alice is still the message's author but no longer a member of its chat (e.g. removed) - - # the one state where authorship and membership disagree, and the only state that can prove - # the membership check does anything the authorship check doesn't already cover. await _remove_alice_from_chat(chat_members_repository, alice_message, alice) with pytest.raises(PermissionDeniedError): await edit_message_use_case( @@ -59,8 +55,6 @@ async def test_author_without_membership_cannot_edit( async def test_non_member_cannot_edit( edit_message_use_case: EditMessageUseCase, alice_message: tables.MessagesTable, carol: tables.UsersTable ) -> None: - # carol isn't in direct_chat at all - the membership gate must refuse her before authorship - # is even considered. with pytest.raises(PermissionDeniedError): await edit_message_use_case( actor=carol, message_id=alice_message.id, data=schemas.EditMessageRequest(text="nope") @@ -73,8 +67,6 @@ async def test_editing_a_deleted_message_raises_conflict( alice_message: tables.MessagesTable, alice: tables.UsersTable, ) -> None: - # The author is authorized; the request conflicts with the message's current state, so this - # is a 409-shaped ConflictError, not a 403-shaped PermissionDeniedError. await delete_message_use_case(actor=alice, message_id=alice_message.id) with pytest.raises(ConflictError): await edit_message_use_case( @@ -96,7 +88,6 @@ async def test_author_can_delete( async def test_other_member_cannot_delete( delete_message_use_case: DeleteMessageUseCase, alice_message: tables.MessagesTable, bob: tables.UsersTable ) -> None: - # Same distinction as edit: bob is a member of the chat but not the author. with pytest.raises(PermissionDeniedError): await delete_message_use_case(actor=bob, message_id=alice_message.id) @@ -107,7 +98,6 @@ async def test_author_without_membership_cannot_delete( alice_message: tables.MessagesTable, alice: tables.UsersTable, ) -> None: - # Same distinction as edit. await _remove_alice_from_chat(chat_members_repository, alice_message, alice) with pytest.raises(PermissionDeniedError): await delete_message_use_case(actor=alice, message_id=alice_message.id) @@ -116,7 +106,6 @@ async def test_author_without_membership_cannot_delete( async def test_non_member_cannot_delete( delete_message_use_case: DeleteMessageUseCase, alice_message: tables.MessagesTable, carol: tables.UsersTable ) -> None: - # Same distinction as edit: carol isn't in direct_chat at all. with pytest.raises(PermissionDeniedError): await delete_message_use_case(actor=carol, message_id=alice_message.id) @@ -143,8 +132,6 @@ async def test_deleted_message_disappears_from_listing( alice_message: tables.MessagesTable, alice: tables.UsersTable, ) -> None: - # A second, undeleted message proves the listing filters *deleted* messages specifically - - # an empty result here would prove nothing, since the chat would just be empty either way. other, _ = await create_message_use_case( actor=alice, chat_id=alice_message.chat_id, diff --git a/tests/use_cases/test_unread_counts.py b/tests/use_cases/test_unread_counts.py index 5db319b..702679c 100644 --- a/tests/use_cases/test_unread_counts.py +++ b/tests/use_cases/test_unread_counts.py @@ -117,9 +117,6 @@ async def test_unread_counts_differ_per_chat( carol: tables.UsersTable, send: SendFixture, ) -> None: - # A correlated subquery that returned the same count for every chat would still pass a test - # that only checks one chat - two chats with two different counts is what proves it's - # actually correlated per-chat rather than computed once and reused. other_chat, _ = await create_chat_use_case( actor=alice, data=schemas.CreateChatRequest(chat_type=tables.ChatType.DIRECT, member_ids=[carol.id]) ) @@ -183,7 +180,6 @@ async def test_marking_read_is_monotonic( actor=alice, chat_id=direct_chat.id, data=schemas.MarkReadRequest(last_read_message_id=second.id) ) - # An out-of-order/replayed request naming an earlier message must not move the marker back. member = await mark_read_use_case( actor=alice, chat_id=direct_chat.id, data=schemas.MarkReadRequest(last_read_message_id=first.id) ) @@ -206,8 +202,6 @@ async def test_deleting_the_newest_message_updates_preview_and_ordering( actor=alice, data=schemas.CreateChatRequest(chat_type=tables.ChatType.DIRECT, member_ids=[carol.id]) ) await send(alice, direct_chat.id, "direct chat message") - # other_chat's only message - deleting it must also cover the "deleting the only message" - # case: last_message becomes null and the chat sorts last. newest, _ = await send(alice, other_chat.id, "other chat message") before = await fetch_chats_use_case(actor=alice)