diff --git a/apps/ai_agent/docs/configuration.md b/apps/ai_agent/docs/configuration.md new file mode 100644 index 0000000..e83c80f --- /dev/null +++ b/apps/ai_agent/docs/configuration.md @@ -0,0 +1,289 @@ +# AI agent configuration & environment reference + +Every setting the `ai_agent` service reads, what happens when one is missing, and +which settings are not configurable at all. + +The headline is short, and it is deliberately stated up front because it is the +opposite of what most services look like: **the service reads exactly one +environment variable, `OPENAI_API_KEY`.** Everything else — the model, the +Weaviate connection, the bind host and port — is hardcoded in +[`apps/ai_agent/main.py`](../main.py). Changing any of them is a code change, not +a deployment setting. + +--- + +## 1. Environment variables + +| Variable | Required | Default | Read at | Effect when unset | +| ---------------- | -------- | ------- | ---------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `OPENAI_API_KEY` | Yes\* | _none_ | Per request, inside `_openai_client()` — not at import and not at boot | The process still starts and `/health` still returns 200; every OpenAI-backed endpoint returns 500. See [§3](#3-startup-and-missing-api-key-behaviour). | + +\* Required for the service to do anything useful. It is _not_ required for the +process to start, and that distinction is the single most important thing in this +document. + +It is the same key the backend uses — it is declared once in the repo-root +[`.env.example`](../../../.env.example) under `# AI Service`: + +```bash +# AI Service +OPENAI_API_KEY= +``` + +There is no `.env` loading in `main.py` — no `python-dotenv`, no +`pydantic-settings`. The variable must be present in the process environment. In +local development that is whatever your shell or process manager exports; in +Docker or Kubernetes it is the container environment. + +### Variables the service does **not** read + +Worth stating explicitly, because their absence is easy to mistake for a bug: + +| Variable | Status | +| -------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------- | +| `OPENAI_MODEL` (or any model-selection variable) | Not read. Models are hardcoded — see [§2](#2-hardcoded-settings). | +| `WEAVIATE_URL` / `WEAVIATE_HOST` / `WEAVIATE_PORT` | Not read. The connection is `weaviate.connect_to_local()` — see [§4](#4-weaviate-connection). | +| `WEAVIATE_API_KEY` | Not read. The client connects unauthenticated. | +| `HOST` / `PORT` | Not read. See [§2](#2-hardcoded-settings). | +| `OPENAI_BASE_URL` | Not read. `OpenAI(api_key=...)` is constructed with no base URL override, so the SDK's own environment handling is the only way to redirect it. | + +Setting any of these has no effect on this service. + +--- + +## 2. Hardcoded settings + +| Setting | Value | Where | +| -------------------- | ------------------------------------------------------ | ---------------------------------------------------- | +| Chat model | `gpt-4o-mini` | `chat`, `analyse_transfer`, `summarise_proposal` | +| Embedding model | `text-embedding-3-small` | `index_message`, `search_messages` | +| Bind host | `0.0.0.0` | `uvicorn.run(...)` under `if __name__ == "__main__"` | +| Bind port | `8000` | same | +| Weaviate collection | `Message` | `index_message`, `search_messages` | +| Search result limit | `5` | `search_messages` | +| High-value threshold | `10_000.0` XLM | `_HIGH_VALUE_THRESHOLD`, used by `analyse_transfer` | +| Request timeouts | 30 s for `/chat`, 10 s for the two JSON-mode endpoints | passed per call as `timeout=` | +| System prompt | `_SYSTEM_PROMPT` | module level | + +Two consequences: + +- **Model changes require a code change and a deploy.** There is no way to move + to a different model, or to run two environments on different models, through + configuration. +- **The host and port literals only apply to `python main.py`.** Running the + module directly executes the `__main__` block and binds `0.0.0.0:8000`. Under + any ASGI server invoked against the app object — which is how the repo README + runs it (`uv run fastapi dev main.py`) and how a container normally runs it — + the `__main__` block never executes and the host/port come from that command: + + ```bash + # __main__ block runs: binds 0.0.0.0:8000 + python main.py + + # __main__ block does NOT run: fastapi/uvicorn decides the bind address + uv run fastapi dev main.py # defaults to 127.0.0.1:8000 + uv run uvicorn main:app --host 0.0.0.0 --port 9000 + ``` + + So the port is effectively set by the launch command, and the `8000` in + `main.py` is a default only for the direct-execution path. A deployment that + needs a different port sets it on the server command line. + +--- + +## 3. Startup and missing-API-key behaviour + +`OPENAI_API_KEY` is read **per request**, inside the `_openai_client()` helper: + +```python +def _openai_client(): + api_key = os.environ.get("OPENAI_API_KEY") + if not api_key: + raise HTTPException(status_code=500, detail="OPENAI_API_KEY is not configured") + if OpenAI is None: + raise HTTPException(status_code=500, detail="openai package is not installed") + + return OpenAI(api_key=api_key) +``` + +Nothing validates it at import time or at startup. The service therefore **starts +successfully with no API key at all** and fails only when an OpenAI-backed +endpoint is called. + +### The exact behaviour, endpoint by endpoint + +| Endpoint | With no `OPENAI_API_KEY` | +| --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `GET /health` | **200** `{"status": "ok"}` | +| `POST /chat` | **500** `{"detail": "OPENAI_API_KEY is not configured"}` | +| `POST /transfers/analyse` | **500** — unless `amount > 10000`, which is answered by the rule-based branch **before** any OpenAI client is constructed, and still returns **200** | +| `POST /proposals/summarise` | **500** | +| `POST /index/message` | **500** if Weaviate is reachable (the embedding call needs the key); **503** if Weaviate is unreachable, because the connection is attempted first | +| `GET /search` | Same as above — **503** on a Weaviate failure, **500** on the missing key when Weaviate is up, and **200** with `{"results": []}` when the `Message` collection does not exist, because that path returns before any embedding is requested | + +### This is deliberate, and it is load-balancer-visible + +`/health` is a liveness probe: it answers whether the process is up and serving, +not whether every downstream dependency is configured. Because it does not touch +`_openai_client()`, a misconfigured deployment presents as: + +- **healthy** to any load balancer, orchestrator probe, or uptime monitor + pointed at `/health`, and +- **completely broken** to every real caller, with a 500 on each request. + +That combination is the failure mode to watch for. An instance rolled out with a +missing or misspelled key will pass its health check, be added to the pool, and +serve nothing but 500s. Kubernetes will not restart it; a load balancer will not +drain it. + +The test suite pins this behaviour rather than leaving it incidental — +`apps/ai_agent/tests/test_health.py::test_health_works_without_api_key` asserts +the 200, and `apps/ai_agent/tests/test_chat.py::test_missing_api_key_returns_500` +asserts the 500. Changing either is a deliberate contract change. + +**Operational recommendation.** Do not rely on `/health` to catch a +misconfiguration. Either: + +- assert the key is present at deploy time (a startup check in your orchestration, + or a required secret rather than an optional env var), or +- monitor the 5xx rate on `/chat` separately from the liveness probe, or +- add a readiness endpoint that checks configuration — deliberately distinct from + `/health`, which should keep its current semantics. + +### Related failure: the `openai` package missing + +`main.py` imports `openai` inside a `try/except ImportError` and sets +`OpenAI = None` when it is absent, so an environment without the dependency also +starts cleanly and fails per-request with **500** +`{"detail": "openai package is not installed"}`. Same shape of problem, same +`/health`-still-green consequence. + +--- + +## 4. Weaviate connection + +### How it connects + +```python +client = weaviate.connect_to_local() +``` + +No host, no port, no API key, no gRPC configuration. `connect_to_local()` from +`weaviate-client` (pinned at 4.22.0 in [`uv.lock`](../uv.lock), declared as +`>=4.0.0` in [`pyproject.toml`](../pyproject.toml)) uses its own defaults — +HTTP on `localhost:8080` and gRPC on `localhost:50051` — and this service +overrides none of them. + +A connection is opened **per request** on `POST /index/message` and `GET /search`, +and closed in a `finally` block. There is no pooled or long-lived client, and no +connection is attempted at startup. + +**There is no Weaviate service in [`infra/docker-compose.yml`](../../../infra/docker-compose.yml).** +That file brings up Postgres, Redis, and MinIO only. A Weaviate instance must be +run separately, and it must be reachable on the loopback defaults above from the +perspective of the `ai_agent` process — which means a containerised `ai_agent` +talking to a Weaviate in another container will not connect without host +networking or a code change, since `localhost` inside the container is the +container itself. + +### Failure mode when Weaviate is unreachable + +Both Weaviate-backed endpoints wrap the connection in `try/except` and translate +any failure to **503**: + +```python +try: + client = weaviate.connect_to_local() +except Exception: + raise HTTPException(status_code=503, detail="Weaviate connection failed") +``` + +| Condition | Response | +| ----------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------- | +| Weaviate down / unreachable | **503** `{"detail": "Weaviate connection failed"}` | +| Weaviate up, later operation fails | **503** with the underlying exception text as `detail` — the second `except Exception as e` block re-raises as 503 with `str(e)` | +| Weaviate up, `Message` collection missing | `GET /search` → **200** `{"results": []}`; `POST /index/message` creates the collection and proceeds | +| Weaviate up, `OPENAI_API_KEY` missing | **500** — the embedding call fails after the connection succeeds | + +Two points that matter operationally: + +- **`/health` does not check Weaviate either.** A deployment with no Weaviate + reachable is green on its liveness probe and serves 503s on `/search` and + `/index/message`, exactly as with a missing API key. Chat and proposal + summarisation are unaffected. +- **The 503 detail can leak internals.** The second handler passes `str(e)` + straight into the response body, so a client can see raw client-library error + text. Treat that as an internal-facing detail, not a stable contract. + +`apps/ai_agent/tests/test_search.py::test_weaviate_connection_failure_returns_503` +pins the 503 behaviour, and `test_missing_collection_returns_empty_results` pins +the empty-result path, including that no query is issued. Note that the +equivalent failure on `POST /index/message` has no test of its own — the coverage +is on the `/search` side only. + +### Collection schema + +The collection is created on demand by `POST /index/message` with +`client.collections.create(name="Message")` — no property schema and no +vectoriser, because vectors are supplied explicitly from OpenAI embeddings. +Properties written per object are `conversationId`, `messageId`, `senderId`, and +`content`; searches filter on `conversationId` and return the top 5 by vector +similarity. See +[`contracts-weaviate-schema.md`](./contracts-weaviate-schema.md) for the full +shape and [`concepts-rag-search-architecture.md`](./concepts-rag-search-architecture.md) +for how it is used. + +--- + +## 5. Running the service + +From the repo root, as documented in the [main README](../../../README.md): + +```bash +cd apps/ai_agent && uv run fastapi dev main.py +``` + +With the API key set for the process: + +```bash +OPENAI_API_KEY=sk-... uv run fastapi dev main.py +``` + +Tests (Python 3.12+, per `requires-python` in `pyproject.toml`): + +```bash +cd apps/ai_agent +uv sync --group dev +uv run pytest +``` + +The suite sets `OPENAI_API_KEY=test-key` for every test through an autouse +fixture in [`tests/conftest.py`](../tests/conftest.py) and mocks both `OpenAI` +and `weaviate.connect_to_local`, so **no network access and no real key are +needed** — and, as a corollary, a real misconfiguration will never be caught by +running the tests. + +CI runs ruff, ruff format, mypy, and pytest with coverage via +[`.github/workflows/ai-agent-ci.yml`](../../../.github/workflows/ai-agent-ci.yml). + +--- + +## 6. Configuration checklist for a deployment + +- [ ] `OPENAI_API_KEY` is present in the process environment — verified by something other than `/health`. +- [ ] The `openai` package is installed in the runtime image (`uv sync`), or every request 500s. +- [ ] If `/search` or `/index/message` are used: a Weaviate instance is running and reachable at the client's local defaults **from the ai_agent process's own network namespace**. +- [ ] Bind host and port set on the server command line, not expected from env. +- [ ] Monitoring watches the 5xx rate on `/chat` and the 503 rate on `/search`, not just liveness. +- [ ] Model choice reviewed as a code change — `gpt-4o-mini` and `text-embedding-3-small` are compiled in. + +--- + +## 7. Related documents + +- [Repo-wide environment reference](../../../.env.example) — the single place every service's variables are declared, including the `OPENAI_API_KEY` this service shares with the backend. There is no separate env-reference document; `.env.example` is it. +- [Main README](../../../README.md) — how the AI service is started alongside the web and backend apps +- [`POST /chat`](./api-chat.md), [`POST /transfers/analyse`](./api-transfers-analyse.md), [`POST /proposals/summarise`](./api-proposals-summarise.md), [`/index` & `/search`](./api-index-search.md) — per-endpoint request/response contracts +- [Weaviate schema contract](./contracts-weaviate-schema.md) — the `Message` collection this service creates and queries +- [RAG search architecture](./concepts-rag-search-architecture.md) — how indexing and search fit together +- [Pydantic model contracts](./contracts-pydantic-models.md) — request/response models referenced above diff --git a/apps/web/docs/styling.md b/apps/web/docs/styling.md new file mode 100644 index 0000000..a29e7c0 --- /dev/null +++ b/apps/web/docs/styling.md @@ -0,0 +1,389 @@ +# Tailwind & styling conventions + +How styling works in `apps/web`: the Tailwind v4 setup, where global styles live, +the design tokens that are actually in use, the breakpoints in play, and the +conventions for writing and extracting classes. + +--- + +## 1. Setup + +| Thing | Value | +| ----------------- | ---------------------------------------------------------------------------------------------- | +| Tailwind version | **v4** — `tailwindcss: ^4` and `@tailwindcss/postcss: ^4` in [`package.json`](../package.json) | +| Config file | **None.** There is no `tailwind.config.js`/`.ts`. v4 configures itself from CSS. | +| PostCSS | [`postcss.config.mjs`](../postcss.config.mjs), one plugin: `@tailwindcss/postcss` | +| Global stylesheet | [`src/app/globals.css`](../src/app/globals.css), imported once from `src/app/layout.tsx` | +| Framework | Next.js 16 App Router, React 19 | +| Fonts | `Geist` and `Geist_Mono` via `next/font/google`, wired in `layout.tsx` | + +```js +// postcss.config.mjs — the entire build-side setup +const config = { + plugins: { + '@tailwindcss/postcss': {}, + }, +}; + +export default config; +``` + +The v4 detail worth internalising: **there is no JavaScript config to edit.** +Theme extension happens in CSS via `@theme`, and content scanning is automatic — +you do not maintain a `content: []` array. If you reach for +`tailwind.config.js`, you are working against the setup. + +### Global styles + +`globals.css` is short and holds everything global: + +```css +@import 'tailwindcss'; + +:root { + --background: #0a0a0f; + --foreground: #f0f0f5; + --accent: #7c5cfc; + --accent-light: #a78bfa; + --muted: #3f3f50; + --card: #13131f; + --border: #1e1e2e; +} + +@theme inline { + --color-background: var(--background); + --color-foreground: var(--foreground); + --color-accent: var(--accent); + --color-accent-light: var(--accent-light); + --color-muted: var(--muted); + --color-card: var(--card); + --color-border: var(--border); + --font-sans: var(--font-geist-sans); + --font-mono: var(--font-geist-mono); +} + +body { + background: var(--background); + color: var(--foreground); + font-family: var(--font-sans), Arial, Helvetica, sans-serif; +} + +html { + scroll-behavior: smooth; +} +``` + +Two layers, and the difference matters: + +- The `:root` block declares the **raw CSS variables**. These are what + `[var(--accent)]` reads. +- The `@theme inline` block maps those into **Tailwind's theme namespace**, which + is what generates utilities like `bg-accent`, `text-foreground`, and + `border-border`. + +Both layers exist, so both idioms work. See +[§3](#3-two-ways-to-reach-a-token-and-when-to-use-each). + +Anything not global belongs in a component. The only component-level CSS in the +codebase is styled-jsx in +[`Spinner.tsx`](../src/components/ui/Spinner.tsx) and +[`SkeletonLoader.tsx`](../src/components/ui/SkeletonLoader.tsx), both for +keyframe animations that utilities cannot express — including a +`prefers-reduced-motion` guard. That is the bar for reaching past utilities: +**a keyframe or a media query Tailwind has no utility for.** + +--- + +## 2. Design tokens + +Seven colour tokens, all defined in `globals.css`. There is no secondary palette, +no semantic status colours in the theme, and no spacing or typography tokens +beyond Tailwind's built-in scales. + +| Token | Value | Role | In use | +| ---------------- | --------- | --------------------------------------- | ------ | +| `--background` | `#0a0a0f` | Page background — near-black, blue-cast | ~23 | +| `--foreground` | `#f0f0f5` | Primary text | ~143 | +| `--accent` | `#7c5cfc` | Primary action / brand violet | ~71 | +| `--accent-light` | `#a78bfa` | Accent text on accent-tinted surfaces | ~10 | +| `--muted` | `#3f3f50` | Muted surface / de-emphasised element | ~15 | +| `--card` | `#13131f` | Raised surface — cards, panels | ~42 | +| `--border` | `#1e1e2e` | Hairline borders and dividers | ~71 | + +Counts are utility occurrences across `src/**/*.tsx`, summing both idioms in +[§3](#3-two-ways-to-reach-a-token-and-when-to-use-each). They are here to show +which tokens carry the UI, not as a target. + +**Status colours are not tokens.** Success, warning, and danger use Tailwind's +stock palette at fixed opacities, established by +[`Badge.tsx`](../src/components/ui/Badge.tsx): + +```ts +const VARIANT_CLASS: Record = { + default: 'border-[var(--accent)]/30 bg-[var(--accent)]/15 text-[var(--accent-light)]', + success: 'border-green-500/30 bg-green-500/15 text-green-300', + warning: 'border-yellow-500/30 bg-yellow-500/15 text-yellow-200', + danger: 'border-red-500/30 bg-red-500/15 text-red-300', +}; +``` + +That `/30` border, `/15` fill, `-300` text recipe is the house style for a tinted +status surface. Reuse `` rather than re-deriving it. + +### Spacing + +No custom spacing scale — Tailwind's default. What the existing screens actually +use, in descending frequency: + +| Purpose | Values in use | +| ----------------- | ------------------------------------------------------------------------------------------ | +| Control padding | `px-4 py-2` (most common), `px-3 py-2`, `px-5 py-2`, `px-6 py-3` | +| Container padding | `p-6` for cards and panels, `p-4` and `p-5` for tighter ones, `p-3`/`p-2` for compact rows | +| Flex/grid gaps | `gap-3` (most common), then `gap-4`, `gap-2`, `gap-1`, `gap-6` | +| Vertical rhythm | `space-y-4`, then `space-y-3` | +| Section padding | `py-32` on landing sections | + +Stay on this ladder — `2 / 3 / 4 / 6` covers nearly everything. Reach for an +arbitrary value only when matching a specific visual, and prefer the nearest step +otherwise. + +### Typography + +Two families, both from `next/font/google` and exposed through `@theme` as +`--font-sans` and `--font-mono`. `body` sets sans by default, so `font-sans` is +rarely written explicitly; `font-mono` is for addresses, hashes, and amounts. + +| Scale | Where it is used | +| -------------------- | --------------------------------------------------------------------------- | +| `text-xs` | Metadata, timestamps, badge-adjacent labels — very common | +| `text-sm` | **The default for body and UI copy** — the most-used size by a wide margin | +| `text-base` | Rare; `text-sm` is the norm, so reaching for `base` is a deliberate step up | +| `text-lg` | Sub-headings and emphasised rows | +| `text-xl`–`text-3xl` | Section and page headings; `text-3xl` is the common heading size | +| `text-4xl`+ | Landing page hero only | + +| Weight | Use | +| --------------- | -------------------------------------------------------- | +| `font-semibold` | The default for anything emphasised — most common by far | +| `font-medium` | Softer emphasis, secondary labels | +| `font-bold` | Headings and hero copy | + +Normal body text carries no weight class. If you find yourself writing +`font-normal`, something upstream is over-weighted. + +### Radius + +`rounded-full` (pills, avatars, badges) and `rounded-lg` dominate, with +`rounded-2xl` for cards and panels and `rounded-xl` for medium controls. Prefer +`rounded-2xl` for a new card and `rounded-full` for a new pill or button — that +is what surrounding screens do. + +### De-facto tokens that are not in `globals.css` + +Two things recur without being declared, and both are worth knowing before you +copy them: + +- **Opacity-modified tokens.** `bg-[var(--card)]/30`, + `text-[var(--foreground)]/50`, `border-[var(--accent)]/30` are used constantly + to derive a surface or a muted text colour from an existing token. This is + preferred over inventing a new colour: it stays on-palette automatically. +- **Off-token colours.** About 30 occurrences of `gray-*`/`slate-*` and a handful + of raw hex values (`[#13131f]`, `[#0F172A]`, `[#0C3F51]`) exist, mostly in + older components — `CopyButton.tsx` is entirely on a light-mode grey palette + that does not belong to this theme. **These are drift, not precedent.** Use the + tokens for new work, and prefer `[#13131f]` → `[var(--card)]` when you are + already editing a file that has one. + +--- + +## 3. Two ways to reach a token, and when to use each + +Because `globals.css` declares both the raw variables and the `@theme` mapping, +both of these compile and both appear in the codebase: + +```tsx +
{/* theme utility */} +
{/* arbitrary value */} +``` + +The arbitrary-value form is roughly twice as common in the current tree +(~193 occurrences vs ~107). Neither is wrong. Pick on this basis: + +- **Need an opacity modifier?** Use `[var(--token)]/NN`. This is why the + arbitrary form dominates — `bg-[var(--card)]/30` has no short theme-utility + equivalent here. +- **Plain, full-opacity colour?** Either works; `bg-card` is shorter and reads + better. +- **Editing an existing file?** Match what that file already does. Mixing both + idioms inside one `className` is the thing to avoid. + +--- + +## 4. Responsive breakpoints + +Tailwind's defaults, unmodified: `sm` 640px, `md` 768px, `lg` 1024px, `xl` +1280px, `2xl` 1536px. + +Only `sm`, `md`, and `lg` are used. There are **zero** `xl:` or `2xl:` +occurrences in `src/`, so the widest layout you can currently rely on having been +designed is the `lg` one. `max-w-6xl` is the usual page container cap. + +The codebase is **mobile-first**: base classes describe the narrow layout and +breakpoint prefixes widen it. Follow that — never write a desktop base and narrow +it with `max-*` variants. + +Established patterns: + +| Pattern | Examples in use | +| ---------------------- | ------------------------------------------------------------------------- | +| Grid columns | `sm:grid-cols-2`, `md:grid-cols-2/3/5`, `lg:grid-cols-3/4/5` | +| Progressive disclosure | `hidden md:inline`, `hidden sm:flex`, `hidden md:flex`, `hidden lg:block` | +| Container width | `max-w-6xl` for page shells; `max-w-sm`/`md`/`lg` for cards and dialogs | + +Chat and conversation views assume a working narrow layout, so test any change +there at a phone width before a desktop one. + +--- + +## 5. Class ordering + +**Nothing enforces class order.** There is no `prettier-plugin-tailwindcss` in +[`package.json`](../../../package.json) and no Tailwind ESLint rule in +[`eslint.config.mjs`](../eslint.config.mjs) — the config is +`eslint-config-next` core-web-vitals plus TypeScript, nothing more. Ordering is a +convention you follow by hand, and no CI job will correct you. + +The order the existing components follow, and the one to write: + +1. **Layout & display** — `flex`, `grid`, `inline-flex`, `hidden`, `block` +2. **Box model / sizing** — `h-full`, `w-full`, `min-h-40`, `max-w-md` +3. **Flex & grid children** — `items-center`, `justify-between`, `flex-col`, `gap-3` +4. **Spacing** — `p-6`, `px-4`, `py-2`, `mt-2`, `space-y-4` +5. **Border & radius** — `rounded-2xl`, `border`, `border-dashed`, `border-[var(--border)]` +6. **Background** — `bg-[var(--card)]/30` +7. **Typography** — `text-sm`, `font-semibold`, `leading-relaxed`, `text-[var(--foreground)]` +8. **Effects** — `shadow-lg`, `blur-[120px]`, `transition-opacity` +9. **State & responsive variants last** — `hover:opacity-90`, `focus:ring-2`, `md:inline`, `lg:grid-cols-3` + +[`EmptyState.tsx`](../src/components/ui/EmptyState.tsx) is a clean reference: + +```tsx +className = + 'flex h-full min-h-40 w-full flex-col items-center justify-center gap-3 rounded-2xl border border-dashed border-[var(--border)] bg-[var(--card)]/30 p-6 text-center'; +``` + +Keeping variants at the end matters most in practice — it makes the base +(mobile) layout readable in one glance. + +--- + +## 6. When to extract a component + +There is **no `cn()`/`clsx`/`tailwind-merge` helper** in this codebase. Classes +are composed with template literals and an appended `className` prop: + +```tsx +className={`... base classes ... ${VARIANT_CLASS[variant]} ${className ?? ''}`} +``` + +Follow that pattern rather than introducing a class-merging utility for a single +component; if one is added, it should land as its own change with every call site +converted. + +### Extract when + +- **The same cluster appears three or more times.** Two occurrences are a + coincidence; three is a component. The tinted-pill recipe hit that bar and + became ``. +- **It has variants.** The moment you write a conditional that picks between + class strings, that belongs in a `Record` map inside a + component — the shape `Badge` and `Spinner` both use. +- **It carries accessibility or behaviour.** `Spinner` exists as much for its + `role="status"`/`aria-hidden` logic and reduced-motion handling as for its + look. Anything with ARIA, focus management, or an animation should not be + copy-pasted. +- **It needs non-utility CSS.** Keyframes or a media query utilities cannot + express force a component boundary anyway. +- **It is a recognisable UI noun.** Avatar, badge, modal, empty state, skeleton, + spinner — if you can name it, it belongs in + [`src/components/ui/`](../src/components/ui/). + +### Repeat the utilities when + +- **It is used once or twice.** Do not build an abstraction for a single caller. +- **The variation is layout, not identity.** A card that is `p-4` here and `p-6` + there with a different grid is two layouts, not one component with two props. +- **Extracting would need more props than the classes it saves.** A wrapper + taking six props to configure spacing is worse than the spacing. +- **It is page-specific composition.** Section layout in a route belongs in that + route. + +### Where components live + +| Directory | Contents | +| --------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | +| [`src/components/ui/`](../src/components/ui/) | Generic primitives — `Avatar`, `Badge`, `CopyButton`, `EmptyState`, `Modal`, `ProposalCard`, `SkeletonLoader`, `Spinner` | +| `src/components//` | Feature-scoped components — `auth/`, `chat/`, `conversations/`, `landing/`, `messaging/`, `search/`, `treasury/`, `wallet/` | + +A component reused across two features moves to `ui/`; one that knows about a +domain type stays in its feature directory. + +--- + +## 7. Dark mode + +**There is no dark-mode support, and there is nothing to toggle.** The app is +**dark-only** by design: + +- The single `:root` palette in `globals.css` _is_ the dark theme — `#0a0a0f` + background, `#f0f0f5` foreground. There is no light palette anywhere. +- There is **no `@media (prefers-color-scheme: ...)` block** in the codebase. +- There is **no `darkMode` configuration** — v4 would configure it in CSS via a + custom variant, and none is declared. +- There is **no theme provider, no `next-themes`, and no theme toggle.** + `layout.tsx` wraps the tree in `WalletProvider`, `AuthProvider`, and + `ToastProvider` only. +- `` carries no `class="dark"` or `data-theme` attribute. + +Exactly **one** `dark:` variant exists in the whole of `src/` — +`text-emerald-600 dark:text-emerald-500` in +[`CopyButton.tsx`](../src/components/ui/CopyButton.tsx). Because no dark-mode +strategy is configured, **that variant never activates.** It is a leftover from a +component authored against light-mode assumptions, which is also why that file +uses `slate-50`, `gray-100`, and `#0C3F51` — colours that belong to no theme +here. + +Consequences for new work: + +- **Do not write `dark:` variants.** They are dead code. Style for the dark + palette directly. +- **Do not assume a light background.** A component built with light-mode + defaults will be illegible; `CopyButton` is what that looks like in practice. +- **Adding light mode is a project, not a patch.** It would need a light palette, + a variant strategy declared in `globals.css`, a theme provider with persistence, + and an audit of every hardcoded colour listed in + [§2](#de-facto-tokens-that-are-not-in-globalscss). Nothing today is written to + support it. + +--- + +## 8. Checklist for a new component + +- [ ] Colours come from the seven tokens — theme utility or `[var(--token)]`, with `/NN` for tints. No new raw hex, no `gray-*`/`slate-*`. +- [ ] Spacing is on the `2 / 3 / 4 / 6` ladder; padding matches the `px-4 py-2` / `p-6` conventions. +- [ ] Type is `text-sm` unless there is a reason; emphasis is `font-semibold`. +- [ ] Radius matches neighbours — `rounded-2xl` for cards, `rounded-full` for pills. +- [ ] Written mobile-first, with `sm:`/`md:`/`lg:` widening it. No `xl:`/`2xl:` unless you are also designing that width. +- [ ] Classes ordered per [§5](#5-class-ordering); variants last. +- [ ] No `dark:` variants. +- [ ] Third occurrence of the same cluster? Extract it to `src/components/ui/`. +- [ ] Non-utility CSS is styled-jsx inside the component, with a `prefers-reduced-motion` guard on any animation. + +--- + +## 9. Related documents + +- [Wallet & treasury UI](./concepts-wallet-treasury-ui.md) — the screens most of these conventions were established on +- [Auth & device lifecycle](./concepts-auth-device-lifecycle.md) +- [Message pipeline](./concepts-message-pipeline.md) — the chat views to test at narrow widths +- [`src/app/globals.css`](../src/app/globals.css) — the source of truth for every token above diff --git a/contracts/docs/concepts-upgrades.md b/contracts/docs/concepts-upgrades.md new file mode 100644 index 0000000..fb42a06 --- /dev/null +++ b/contracts/docs/concepts-upgrades.md @@ -0,0 +1,337 @@ +# Contract upgrades & versioning + +How a deployed Soroban contract in this repo is replaced with new code, who is +allowed to do it, and which changes to the contract's storage layout are safe +across that swap. + +The whole mechanism is a single function, `upgrade`, on **one** contract: +`token_transfer`. Everything below is anchored to +[`contracts/contracts/token_transfer/src/lib.rs`](../contracts/token_transfer/src/lib.rs). + +--- + +## 1. Which contracts are upgradeable + +| Contract | Path | Upgradeable | Mechanism | +| ---------------- | ------------------------------------ | ----------- | -------------------------------------------------------------------------- | +| `token_transfer` | `contracts/contracts/token_transfer` | **Yes** | `upgrade(env, new_wasm_hash)` — admin-gated `update_current_contract_wasm` | +| `group_treasury` | `contracts/contracts/group_treasury` | **No** | No upgrade entrypoint exists | +| `proposals` | `contracts/contracts/proposals` | **No** | No upgrade entrypoint exists | + +`group_treasury` and `proposals` expose no function that calls +`env.deployer().update_current_contract_wasm(...)`. Their deployed wasm is +immutable for the life of the contract instance. Changing their behaviour means +**deploying a new contract instance** and repointing every consumer at the new +contract ID (see [§7](#7-what-to-do-for-a-non-upgradeable-contract)). + +`proposals` carries an admin address that its source comments describe as a hook +"a future upgrade can wire" for governance parameters. That is a note about +future work, not an upgrade path — there is no upgrade entrypoint on that +contract today. + +--- + +## 2. Authorization: who may call `upgrade` + +```rust +pub fn upgrade(env: Env, new_wasm_hash: BytesN<32>) { + let admin: Address = env + .storage() + .instance() + .get(&DataKey::Admin) + .expect("not initialized"); + admin.require_auth(); + env.deployer().update_current_contract_wasm(new_wasm_hash); +} +``` + +The gate is exactly one thing: `admin.require_auth()`, where `admin` is the +address written to `DataKey::Admin` by `initialize` and **never written again**. +`token_transfer` has no `set_admin`, no admin-transfer function, and no +multi-signer or timelock logic. The admin is fixed at initialization for the +lifetime of the contract instance. + +Consequences that follow directly from that: + +- **Any caller who can produce a valid signature for the admin address can + replace the contract's entire code.** There is no second approval, no delay, + and no on-chain veto. +- **The admin key cannot be rotated.** If the key is compromised there is no + in-contract remedy — the attacker and the legitimate operator have identical + authority, and the attacker can upgrade the contract to wasm that removes the + legitimate operator's access. +- **A compromised admin key is total loss of control over the contract.** The + attacker can upgrade to wasm that redirects every `transfer` to an address + they control, or that drains any allowance the contract holds. Existing + storage (`Admin`, `TokenContract`) survives the swap, so the new code starts + with the contract's full state. +- **`set_token_contract` shares the same key.** The same compromise lets an + attacker point the router at a malicious token contract without upgrading + anything. + +Because a compromised admin is unrecoverable, the admin address should be held to +the same standard as a treasury key — hardware-backed, or a Stellar multisig +account so that no single signer can act alone. This is an operational control; +the contract itself enforces nothing beyond a single signature. + +### Verified by tests + +[`contracts/contracts/token_transfer/src/test.rs`](../contracts/token_transfer/src/test.rs) +covers both sides of the gate: + +- `test_upgrade_requires_admin_auth` — with the admin's auth mocked, the call + passes the auth check and fails later at the wasm lookup, proving admin auth + is _sufficient_. +- `test_upgrade_non_admin_panics` — with only a non-admin address authorized, + the call panics at `require_auth()`, proving admin auth is _necessary_. + +--- + +## 3. End-to-end upgrade procedure + +All commands run from `contracts/` and use the Stellar CLI, matching +[the deployment guide](./api-deployment-invocation.md). Replace `testnet` with +your target network. + +### 3.1 Build the new wasm + +```bash +cargo build -p token_transfer --target wasm32-unknown-unknown --release +``` + +Output: `target/wasm32-unknown-unknown/release/token_transfer.wasm`. + +Build with the workspace's release profile as committed — `contracts/Cargo.toml` +pins `opt-level = "z"`, `lto = true`, and `strip = "symbols"`. Changing profile +flags changes the binary and therefore its hash, so a hash produced under a +different profile will not match one anybody else builds. + +Run the same gates CI runs before uploading anything: + +```bash +cargo test -p token_transfer +cargo fmt --all -- --check +cargo clippy --workspace --target wasm32-unknown-unknown -- -D warnings -A dead_code -A clippy::too-many-arguments +``` + +### 3.2 Produce the wasm hash + +Uploading the binary installs the code on the ledger without deploying an +instance, and returns its hash — this is the value `upgrade` takes: + +```bash +stellar contract upload \ + --wasm target/wasm32-unknown-unknown/release/token_transfer.wasm \ + --source \ + --network testnet +# → 6ddb28e0980f643bb97350f7e3bacb0ff1fe74d846c6d4f2c625e766210fbb5b +``` + +The 32-byte value it prints is the `BytesN<32>` that `upgrade` expects. + +### 3.3 Verify the hash before invoking + +The hash is the _only_ thing binding the on-chain code to the source that was +reviewed. Verify it locally rather than trusting the upload output: + +```bash +# The uploaded hash is the sha256 of the wasm file bytes. +sha256sum target/wasm32-unknown-unknown/release/token_transfer.wasm +``` + +That value must equal the hash printed by `stellar contract upload`. If they +differ, the binary that was uploaded is not the one on disk — stop. + +A reviewer should independently reproduce the build from the reviewed commit and +confirm they get the same hash. Because the release profile is pinned in +`Cargo.toml` and the toolchain is pinned in +[`contracts/rust-toolchain.toml`](../rust-toolchain.toml), an independent build +of the same source should reproduce it. A mismatch means the toolchain or the +source differs, and the upgrade should not proceed until that is explained. + +### 3.4 Invoke `upgrade` + +```bash +stellar contract invoke \ + --id \ + --source \ + --network testnet \ + -- upgrade --new_wasm_hash +``` + +`--source` must be the admin identity — any other signer panics at +`require_auth()`. + +### 3.5 Confirm the upgrade took effect + +The contract emits no upgrade event, so confirmation is by observation: + +```bash +# 1. State survived the swap — this must still return the configured token. +stellar contract invoke --id --source --network testnet \ + -- token_contract + +# 2. New behaviour is live — call a function whose behaviour changed, +# or one that only exists in the new wasm. +stellar contract invoke --id --source --network testnet \ + -- +``` + +A read of `token_contract` that panics with `not initialized` after an upgrade +means the new wasm is reading a different storage key than the old wasm wrote — +see [§4](#4-storage-layout-compatibility). Treat that as a live incident: the +contract still holds the old data, but the new code cannot see it. + +The contract ID does **not** change across an upgrade. Backend and frontend +configuration (`TOKEN_TRANSFER_CONTRACT_ID`, the corresponding `NEXT_PUBLIC_*`) +needs no update. Clients holding a generated contract client **do** need +regenerating if function signatures changed. + +--- + +## 4. Storage layout compatibility + +`update_current_contract_wasm` swaps the code and **leaves storage untouched**. +The new wasm inherits every key the old wasm wrote, byte for byte. Nothing +migrates automatically and nothing is validated — a mismatch is silent until a +read fails or, worse, succeeds with the wrong data. + +`token_transfer` stores two instance-scoped keys, defined in +[`storage.rs`](../contracts/token_transfer/src/storage.rs): + +```rust +#[contracttype] +pub enum DataKey { + TokenContract, // Address of the SEP-41 token this contract routes through + Admin, // Address permitted to upgrade / set the token contract +} +``` + +Both are read via `env.storage().instance()`. There is no persistent or +temporary storage, so TTL and rent do not enter into the compatibility question +here. + +### 4.0 How the key encoding actually works + +The compatibility rules below follow from one fact about `#[contracttype]`, and +it is worth stating plainly because the intuition from other chains is wrong +here: **Soroban encodes by name, not by position.** + +- A `#[contracttype]` **enum** encodes each variant as an `ScSymbol` of the + variant's _identifier_ — `DataKey::Admin` is stored under the symbol + `"Admin"`. Declaration order plays no part in the encoding. +- A `#[contracttype]` **struct** encodes as an `ScMap` keyed by the field names, + sorted by name. Declaration order plays no part there either. + +So the dangerous edits are the ones that change a **name** or a **type**, not the +ones that change an order. (`#[contracterror]` / `contracttype` _integer_ enums +are the exception — those do encode by discriminant — but `token_transfer` uses +neither.) + +### 4.1 Safe changes + +| Change | Why it is safe | +| -------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| Adding a **new** `DataKey` variant | Existing variants keep their own name symbols, so old keys still resolve. The new key simply has no value yet — reads must handle absence. | +| **Reordering** existing `DataKey` variants | Variants encode by name, so order is not part of the key. Harmless, though a pointless diff. | +| **Reordering** fields in a stored struct | Struct fields encode as a name-keyed map sorted by name, so declaration order is not part of the encoding. | +| Adding, removing, or changing contract **functions** | Functions are not storage. Callers of a removed function break, but stored state is unaffected. | +| Changing function **bodies** (logic, validation, events emitted) | No storage encoding is involved. | +| Adding a field to an **event** struct such as `TransferEvent` | Events are emitted, never read back from storage. Off-chain consumers must tolerate the new shape, but no on-ledger state is corrupted. | +| Adding a field to a **stored** struct, if reads tolerate its absence | Old values decode without the new key. The decode fails unless the field is optional or the read path handles the missing key, so this needs a deliberate migration path — see [§4.3](#43-when-a-layout-change-is-unavoidable). | +| Changing the release profile or the SDK **patch** version | Produces a different wasm hash, not a different storage layout. | + +### 4.2 Unsafe changes — these corrupt or orphan existing state + +| Change | What breaks | +| ----------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| **Renaming** a `DataKey` variant | The key is the variant name. Renaming `Admin` to `Owner` makes the new wasm read a key nothing was ever stored under: `upgrade` and `set_token_contract` panic with `not initialized`, and the old admin value is orphaned on the ledger permanently. | +| **Removing** a variant that still has a value on the ledger | The value stays on-ledger, unreachable and unrecoverable, and continues to occupy the instance entry. | +| Changing a stored value's **type** (`Address` → `BytesN<32>`, `i128` → `u64`) | The old bytes are decoded as the new type. This either panics on read or, where the encodings happen to overlap, silently yields a wrong value. | +| Changing an enum variant's **payload** (unit → tuple, or its inner types) | `DataKey::Admin` and `DataKey::Admin(u32)` are different keys with different encodings. The old value becomes unreachable. | +| **Renaming** or **retyping** a field in a stored struct | Same as the enum: the map key is the field name. The old field's value is orphaned and the new field reads as missing. `token_transfer` stores no structs today, so this is a rule to preserve rather than a present hazard. | +| Moving a key between **storage scopes** (instance → persistent/temporary) | The scopes are separate namespaces. The old value stays in instance storage and the new read finds nothing. | +| Re-running `initialize` after an upgrade | It panics — `initialize` guards on `DataKey::Admin` already existing. There is no re-initialization path, by design. | + +The worst case in that table is the rename, because it fails _closed_ in the most +expensive way possible: `Admin` renamed means nobody can authorize an `upgrade` +any more, so the mistake cannot be fixed by upgrading again. That is +unrecoverable for `token_transfer`. Treat any rename of a `DataKey` variant as a +change that must not ship. + +### 4.3 When a layout change is unavoidable + +There is no migration hook on this contract: no post-upgrade initializer, no +stored schema version, and no way to run code once at the moment of the swap. If +the layout must change, the migration has to be written into the new wasm itself +— an admin-only function invoked in the same operational window as the upgrade: + +1. Upgrade to a wasm that understands **both** layouts and exposes a `migrate` + function. +2. Invoke `migrate` as the admin. It reads the old keys, writes the new ones, and + records that it ran so it cannot run twice. +3. Optionally upgrade again to a wasm that has dropped the old-layout code. + +Rehearse the whole sequence on testnet, against a contract instance holding +representative state, before touching mainnet. + +--- + +## 5. Versioning + +The contract exposes no version. There is no `version()` function, no stored +version key, and no event announcing an upgrade — an observer cannot ask a +deployed `token_transfer` which build it is running. + +In practice the deployed version is identified by the **wasm hash**, which is +what the ledger records and what `stellar contract upload` returns. Keep an +operational record mapping each upgrade to the git commit it was built from, the +hash that build produced, the network, and the date. The chain does not keep that +mapping for you. + +Adding a `version()` entrypoint returning a compile-time constant is a safe +change under [§4.1](#41-safe-changes); storing the version under a new `DataKey` +variant is safe too — it is a new name, so nothing existing is disturbed. + +--- + +## 6. Pre-upgrade checklist + +- [ ] Change is reviewed and merged; the build is from a known commit. +- [ ] `cargo test -p token_transfer`, `cargo fmt --check`, and `cargo clippy` pass. +- [ ] Storage diff reviewed against [§4.2](#42-unsafe-changes--these-corrupt-or-orphan-existing-state) — no rename, removal, payload change, or type change of any `DataKey` variant. +- [ ] Local `sha256sum` matches the hash returned by `stellar contract upload`. +- [ ] A reviewer independently reproduced the build and got the same hash. +- [ ] Full sequence rehearsed on testnet against an initialized contract holding state. +- [ ] Admin key access confirmed and the signer identified before the invoke. +- [ ] Post-upgrade reads (`token_contract`, `balance`) planned as the confirmation step. + +--- + +## 7. What to do for a non-upgradeable contract + +`group_treasury` and `proposals` cannot be swapped in place. Changing them means: + +1. Deploy a new instance from the new wasm — `contracts/scripts/deploy_group_treasury.sh` + or `contracts/scripts/deploy_proposals.sh`. +2. Initialize it and re-establish its state. Members, thresholds, and balances do + not carry across. Funds held by a `group_treasury` instance must be withdrawn + through the old contract's own rules before the old instance is abandoned. +3. Update `GROUP_TREASURY_CONTRACT_ID` / `PROPOSALS_CONTRACT_ID` in the backend + env and the corresponding `NEXT_PUBLIC_*` values in the frontend, per + [§6 of the deployment guide](./api-deployment-invocation.md#6-how-a-deployed-contract-id-reaches-the-frontend--backend). +4. Accept that in-flight proposals on the old instance are stranded — they can + still be voted on and executed against the old contract, which no client is + pointed at any more. + +Because there is no in-place path, treat a `group_treasury` or `proposals` +deployment as final and get the logic right before mainnet. + +--- + +## 8. Related documents + +- [Contract build, deployment & invocation guide](./api-deployment-invocation.md) — toolchain, build, deploy, and the invoke syntax used above +- [Token transfer storage layout & token interface](./contracts-token-transfer-storage.md) — the storage keys these compatibility rules apply to +- [Token transfer API](./api-token-transfer.md) — the function surface an upgrade may change +- [Token transfer flow](./concepts-token-transfer-flow.md) — how the contract is used end to end diff --git a/docs/api-versioning.md b/docs/api-versioning.md new file mode 100644 index 0000000..b9232de --- /dev/null +++ b/docs/api-versioning.md @@ -0,0 +1,347 @@ +# API versioning & deprecation policy + +The backend's REST routes and socket events are consumed by a web client that +ships on its own schedule. Once a user has a tab open, or a service worker +cached, the client talking to production is not necessarily the client that was +built alongside the running server. This document states where the API stands on +compatibility today, what counts as a breaking change, and the procedure for +retiring a field, route, or event. + +--- + +## 1. Current position + +**REST routes are unversioned.** There is no `/v1` prefix, no `Accept` header +negotiation, and no version query parameter. Every router in +[`apps/backend/src/app.ts`](../apps/backend/src/app.ts) mounts at a bare path: + +```ts +app.use('/auth', authRouter); +app.use('/conversations', conversationsRouter); +app.use('/devices', devicesRouter); +app.use('/messages', messagesRouter); +app.use('/users', usersRouter); +app.use('/treasury', treasuryRouter); +app.use('/uploads', uploadsRouter); +app.use('/files', filesRouter); +app.use('/push', pushRouter); +app.use('/sync', syncRouter); +app.use('/user-devices', userDevicesRouter); +app.use('/security', securityRouter); +``` + +**Socket events are unversioned too.** Events are addressed by bare name +(`send_message`, `new_message`, `read_receipt`, …) with no version field in the +payload and none in the handshake. The `dispatch` envelope carries `type`, +`payload`, and `eventId` — no schema version. + +**There is no deprecation machinery.** The string `deprecat` does not appear +anywhere in `apps/backend/src`. No route sets a `Deprecation` or `Sunset` header, +no response carries a warning field, and no sunset dates are recorded anywhere. + +**Breaking changes have already shipped without a policy.** Three that are +visible in the current tree and its history: + +| Change | What broke for an older client | +| ------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `sequenceNumber` removed from `GET /sync` envelopes | A client ordering an offline catch-up by `sequenceNumber` gets `undefined` on every envelope. `apps/backend/src/__tests__/sync.routes.test.ts` asserts the field's absence, so the removal is deliberate and enforced. The field still exists on socket receipt payloads, so it is gone from one surface and present on another. | +| `device` became **required** on `POST /auth/verify` | `VerifySchema` moved from `device: DeviceSchema.partial().optional()` to `device: DeviceSchema`. A client that authenticated without a device block now gets a 400 at login — the hardest possible failure, because it locks the user out entirely rather than degrading a feature. | +| `POST /devices` retired in favour of the link-challenge flow | JWT-only device registration is gone; devices must complete `POST /devices/link/challenge` then `POST /devices/link/verify`. | + +The third of those is the one that was handled well, and it is the model this +policy generalises — see [§4.1](#41-the-existing-precedent-post-devices). + +Note also that `VerifySchema` and the message schemas are `.strict()`: an +unrecognised field in a request body is rejected outright rather than ignored. +That is a deliberate safety property for key material (a client bug that sends +private state should be surfaced, not silently dropped) but it means **adding an +optional field to a strict request body is a breaking change for any client that +sends it to an older server**. Request bodies and response bodies do not follow +the same compatibility rules here. + +### The intended policy going forward + +1. **Additive-by-default.** New behaviour ships as new fields, new events, or new + routes. Existing shapes keep working. +2. **No silent removals.** Anything removed goes through the deprecation + procedure in [§4](#4-deprecation-procedure), with an announced sunset. +3. **Negotiate rather than version, where the change is in the protocol.** The + device capability mechanism ([§5](#5-capability-negotiation-the-working-precedent)) + already ships protocol changes without breaking older clients, and it is the + preferred tool. +4. **Version the path only as a last resort.** Introducing `/v2/` beside + the existing route is reserved for a change that cannot be made additive or + negotiated. When it happens, the unprefixed route remains the v1 route — it is + not retroactively moved — and it is deprecated on the normal schedule. + +The policy applies to what the server promises going forward. It does not +retroactively re-open the three changes above. + +--- + +## 2. What counts as a breaking change + +### 2.1 REST responses + +Breaking: + +- Removing a field, or making a previously always-present field optional. +- Renaming a field. (This is a removal plus an addition, and it is the most + common accidental break.) +- Changing a field's type or its representation — `number` → `string`, an ISO + timestamp → epoch millis, a bare id → an object. +- Narrowing an enum-like value set, or adding a new value to one that clients + switch on exhaustively. +- Changing a success status code (`200` → `204`), or turning a previously + successful request into an error. +- Changing pagination semantics — cursor format, page size limits, ordering. + `GET /sync` encodes its cursor as `:`; a client persists that + string across sessions, so changing the format strands every stored cursor. +- Moving a route to a different path, or adding a newly required parameter. +- Tightening validation on an existing field so previously accepted input is + rejected. + +Not breaking: + +- Adding a new optional field to a response. +- Adding a new route. +- Adding a new optional field to a **non-strict** request body. +- Making a previously required request field optional. +- Loosening validation. +- Changing an error _message_ string while the status code and error shape hold. + +### 2.2 Socket event payloads + +The same field-level rules apply, plus: + +Breaking: + +- Removing an outbound event, or renaming one. A client listening for the old + name simply never fires — there is no error, so this fails silently and is + worse than a REST break. +- Removing a field from an outbound payload, or changing its type. +- Requiring a new field on an inbound event. +- Changing which room or audience an event is broadcast to, so a client that + used to receive it no longer does. +- Changing the ack/response contract of an inbound event. +- Changing the handshake requirements. Auth already rejects tokens without + `deviceId` — `verifyToken` throws `Token missing deviceId — re-authentication +required` — which is a hard break for any pre-device-auth token. + +Not breaking: + +- Adding a new event. +- Adding an optional field to an outbound payload. +- Accepting a new optional field on an inbound event. + +Socket breaks deserve extra caution for two reasons: a long-lived connection +means an old client can be attached to a new server for hours, and a missing +event produces no error the client can detect or report. + +### 2.3 Database-backed shapes + +Some response shapes are the database schema in a thin wrapper, so a migration +can break the API without anybody editing a route. Treat a migration as an API +change whenever the column is reachable from a response: + +- Dropping or renaming a column that a route serialises is a **response-breaking + change**, even though the diff touches only `apps/backend/drizzle/`. +- Changing a column's type or nullability changes the response's type or + optionality. +- Changing a default changes what clients observe for existing rows. +- Removing an enum value from a column constraint narrows a value set clients may + already be switching on. + +Two rules follow: + +1. **Expand, migrate, contract.** Add the new column, dual-write, migrate readers, + and only then drop the old column — with the drop treated as a deprecation + under [§4](#4-deprecation-procedure) rather than as a schema tidy-up. +2. **Never serialise a table row directly.** Route handlers should map explicitly + to a response shape, so that a column rename is a compile error rather than a + silent contract change. `apps/backend/src/lib/messages.ts` already does this + deliberately for `content`, destructuring it out so a legacy plaintext column + can never reach a serialised response. + +--- + +## 3. Compatibility windows + +| Consumer | Assume it can be stale for | +| ---------------------------- | ------------------------------------------------------------------------- | +| Open browser tab | Hours to days — until reload | +| Cached service worker | Until the next activation | +| Device with queued envelopes | The envelope retention window, `ENVELOPE_TTL_SECONDS` (7 days by default) | + +The retention window is a floor, not a ceiling: a device offline for the whole +window reconnects, syncs, and immediately starts speaking whatever protocol it +knew a week ago. **Minimum support window for a deprecated field, event, or +route: 90 days** from the announcement in [§4.2](#42-procedure). Shorten it only +for a security fix, and say so explicitly when you do. + +--- + +## 4. Deprecation procedure + +### 4.1 The existing precedent: `POST /devices` + +When JWT-only device registration was retired, the route was not deleted. It was +left mounted, returning an explicit, actionable error: + +```ts +// ─── POST /devices — retired (#333) ────────────────────────────────────────── +// Kept only to return an explicit, actionable error: bare JWT-only device +// registration is gone. Clients must complete the link challenge instead. + +devicesRouter.post('/', (_req: AuthRequest, res) => { + res.status(403).json({ + error: + 'Device registration requires a fresh wallet signature. Use POST /devices/link/challenge then POST /devices/link/verify.', + }); +}); +``` + +An old client hitting it gets a status it can branch on and a message naming its +replacement, instead of a 404 that is indistinguishable from a typo or an outage. +That is the standard for every retirement: **the removed thing keeps answering, +and its answer says what to do instead.** + +### 4.2 Procedure + +1. **Announce.** Open an issue describing the change, the reason, the + replacement, and the sunset date (≥ 90 days out). Record it in the + [deprecation register](#6-deprecation-register) in this file. +2. **Mark it.** How depends on what is being deprecated: + + | Surface | How it is marked | + | ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------- | + | REST route | Respond with `Deprecation: true` and `Sunset: ` headers, plus a `Link: ; rel="deprecation"` pointing at the issue. Keep serving normally. | + | REST field | Keep returning it, populated. Document it as deprecated in the route's doc under `apps/backend/docs/` and note the replacement field. | + | Socket event | Keep emitting it alongside its replacement. There is no header channel on a socket, so the register in this file and the event catalog are the notice. | + | Socket field | Keep populating it alongside the replacement field in the same payload. | + | DB column | Stop reading it, keep writing it, and mark it deprecated in the schema. | + +3. **Ship the replacement first.** The new field, event, or route must be live and + documented before the old one is marked, so a client can migrate the moment it + sees the notice. +4. **Notify.** Update the relevant doc under `apps/backend/docs/` (the + [WebSocket event catalog](../apps/backend/docs/api-websocket-events.md) for + events, the matching `api-*.md` for routes), add the register entry here, and + flag it in the PR description. There is no runtime client-notification channel + today; adding a deprecation array to `GET /health` would be the natural place + if one is wanted. +5. **Wait out the window.** Both paths work for the full period. +6. **Retire.** Replace the implementation with an explicit error in the style of + §4.1 — a `410 Gone` for a removed route, `403` where a different flow must be + used — naming the replacement. Do not delete the handler. Move the register + entry to _Retired_. + +### 4.3 Emergency changes + +A security fix may skip the window. It must still: ship an explicit error rather +than a silent removal, get a register entry recording that the window was +skipped and why, and state what a stranded client should do. `verifyToken` +rejecting tokens without `deviceId` is an example of this class — the break is +deliberate and the error message tells the client to re-authenticate. + +--- + +## 5. Capability negotiation: the working precedent + +For protocol-level changes, the codebase already has a mechanism that ships +breaking protocol work without breaking older clients: +[`apps/backend/src/lib/capabilities.ts`](../apps/backend/src/lib/capabilities.ts). + +Each device advertises a small JSON document at registration, and can update it +later without re-registering its identity key: + +```ts +protocols; // e.g. ["sealed_box", "signal", "mls"] — schemes this device can decrypt +ciphersuites; // MLS/Signal ciphersuite identifiers, meaningful when "mls" is present +fileTransfer; // file-encryption scheme versions, e.g. ["file-v1"] +``` + +The properties that make it work are worth naming, because they are what any +future negotiation mechanism should copy: + +- **A universal baseline.** `sealed_box` is the scheme every device in the + codebase implements, and it is the floor of every negotiation. +- **Absence is a valid answer.** A device that never advertised capabilities — + including rows written before the column existed — normalises to the + sealed_box-only baseline instead of erroring. `normalizeCapabilities` returns + the baseline for `null`, `undefined`, and malformed input alike. +- **Unknown values are preserved and ignored.** `selectProtocol` walks a known + priority list rather than rejecting names it does not recognise, so an older + server meeting a newer client's advertisement degrades instead of failing. +- **Negotiation is pairwise and per-message.** `selectProtocol(a, b)` picks the + strongest scheme _both_ sides support, so a rollout proceeds device by device + with no flag day. +- **Advertising is a non-breaking addition.** `capabilities` is optional on + `DeviceSchema`; omitting it is well-defined. + +The same shape applies beyond encryption. `fileTransfer` already carries versioned +scheme identifiers (`file-v1`) checked by `supportsFileTransfer`, which is exactly +how a versioned payload format should be gated. + +**Use this first.** A protocol or payload-format change that can be expressed as +a capability should be, rather than as a versioned route: it needs no client +coordination, no sunset date, and no dual-serving window. + +Its limit is that it is scoped to devices and to encryption/file-transfer +concerns. It says nothing about REST response shapes, and there is no equivalent +negotiation for a client's understanding of a JSON field. Those still need the +deprecation procedure in [§4](#4-deprecation-procedure). + +A second, narrower precedent lives in the socket layer: the +[envelope-wrapper vs. legacy-raw-emit](../apps/backend/docs/api-websocket-events.md#envelope-wrapper-vs-legacy-raw-emit) +split, where both emission styles are supported concurrently and new client code +is directed at the envelope. That is the dual-serving half of §4.2 already in +practice — what it lacks is an announced sunset for the legacy style. + +--- + +## 6. Deprecation register + +The canonical list of what is deprecated, what replaces it, and when it goes. +Every deprecation adds a row here as step 1 of [§4.2](#42-procedure). + +### Active deprecations + +| Surface | Deprecated | Replacement | Announced | Sunset | +| --------------- | ---------- | ----------- | --------- | ------ | +| _none recorded_ | | | | | + +Legacy-raw-emit on the socket is a candidate for the first entry: it is already +dual-served and already documented as non-preferred, but has no announced sunset. + +### Retired + +| Surface | Retired | Replacement | Behaviour now | +| ------------------------------------ | ------------------------------------------------- | ------------------------------------------------------------------ | -------------------------------------------------------------------------- | +| `POST /devices` | JWT-only device registration (#333) | `POST /devices/link/challenge` → `POST /devices/link/verify` | `403` naming the replacement flow | +| `GET /sync` → `sequenceNumber` | Per-conversation sequence number as a sync cursor | Opaque `nextCursor` (`:`), stable across conversations | Field absent; asserted by `sync.routes.test.ts` | +| `POST /auth/verify` without `device` | Deviceless authentication | `device` block required on every verify | `400` from schema validation | +| JWT without `deviceId` | Pre-device-aware tokens | Re-authenticate | `verifyToken` throws `Token missing deviceId — re-authentication required` | + +--- + +## 7. Checklist for a change that touches the API + +- [ ] Diff reviewed against [§2](#2-what-counts-as-a-breaking-change) — response fields, socket payloads, and any migration. +- [ ] If breaking: can it be additive instead? Can it be a capability instead? +- [ ] If it must break: replacement shipped and documented first. +- [ ] Register row added with a sunset date ≥ 90 days out. +- [ ] Route doc under `apps/backend/docs/` updated; event catalog updated for socket changes. +- [ ] Retirement returns an explicit error naming the replacement — never a bare 404 or a silently missing event. +- [ ] Migrations reviewed as API changes, not just schema changes. + +--- + +## 8. Related documents + +- [WebSocket event catalog](../apps/backend/docs/api-websocket-events.md) — every event, its payload, and the envelope/legacy split +- [REST schemas](../apps/backend/docs/contracts-rest-schemas.md) — request/response shapes this policy governs +- [WebSocket payload contracts](../apps/backend/docs/contracts-websocket-payloads.md) +- [Devices & prekeys API](../apps/backend/docs/api-devices.md) — the routes carrying the retired `POST /devices` and the link flow that replaced it +- [Message sync API](../apps/backend/docs/api-messages-sync.md) — the cursor contract referenced in §2.1 +- [Message encryption migration](../apps/backend/docs/message-encryption-migration.md) and [Signal migration](../apps/backend/docs/signal-migration.md) — capability negotiation applied to a live protocol rollout