diff --git a/docs/superpowers/plans/2026-08-23-feature-flags.md b/docs/superpowers/plans/2026-08-23-feature-flags.md new file mode 100644 index 0000000..4e68f34 --- /dev/null +++ b/docs/superpowers/plans/2026-08-23-feature-flags.md @@ -0,0 +1,1106 @@ +# Feature Flags (Unleash) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let a merged feature reach production without becoming visible, and be switched on later from a UI. + +**Architecture:** A self-hosted Unleash instance in its own Portainer stack holds flag state. FastAPI is the only service with an Unleash SDK; it declares every flag in a code registry, evaluates them fail-closed, and exposes them at an internal-only `/api/flags`. Next reads that endpoint server-side. No webhook — time-based ISR already propagates a flip within five minutes on school pages. + +**Tech Stack:** Unleash OSS (Apache-2.0), `UnleashClient` (Python), FastAPI, Next.js 15 App Router, Playwright, pytest, Jest. + +**Spec:** `docs/superpowers/specs/2026-08-23-feature-flags-design.md` + +## Global Constraints + +- **Every flag defaults to `False`.** There is no per-flag default field. A flag that defaults on is a kill switch, which this design does not offer. +- **One string per flag**, used unchanged as the registry key, the Unleash flag name, and the JSON key in `/api/flags`. snake_case, matching the API's existing convention. No case transformation anywhere. +- **Fail closed.** Unleash unreachable, unconfigured, or erroring ⇒ every flag is `False`. Never raise out of a flag evaluation. +- **`/api/flags` must never be publicly reachable.** It names unreleased features. +- **An API field that is off is absent, not `null`,** and withheld at the source. `/api/schools/` is public and unauthenticated (precedent: commit `c9a1892`). +- **Never flag content on a `force-static` page.** `app/admissions/page.tsx` never revalidates. +- **Reading flags in a route pins that route to a 300s ISR floor** — Next uses the lowest `revalidate` among a route's fetches. +- Backend tests run via: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` +- Never push to `main`. Branch, PR, let checks pass. +- Do not start a local server to test the application; it does not work. + +## File Structure + +| File | Responsibility | +|---|---| +| `docker-compose.portainer.unleash.yml` (new) | The Unleash stack: server + its own Postgres. Owned by neither app stack. | +| `backend/flags.py` (new) | The whole flag layer: registry, client lifecycle, `is_enabled`, `all_flags`. One module, ~90 lines. | +| `backend/config.py` (modify) | Four `unleash_*` settings. | +| `backend/app.py` (modify) | `/api/flags` endpoint; `flags.init()` in `lifespan`; the `admission_distance` gate. | +| `backend/tests/test_flags.py` (new) | Registry shape, fail-closed evaluation, staleness tripwire, endpoint. | +| `nextjs-app/lib/flags.ts` (new) | `getFlags()` — one server-side fetch, typed, never throws. | +| `nextjs-app/app/api/[...path]/route.ts` (modify) | Internal-only denylist so the public proxy will not forward `/api/flags`. | +| `nextjs-app/__tests__/lib/flags.test.ts` (new) | `getFlags` fallback behaviour. | +| `nextjs-app/__tests__/api/proxyDenylist.test.ts` (new) | The proxy 404s internal paths. | +| `e2e/tests/journeys.spec.ts` (modify) | Flag-aware journeys; the explicit-gate rule from spec §7. | +| `requirements.txt` (modify) | `UnleashClient`. | +| `docker-compose.yml`, `docker-compose.portainer.yml`, `docker-compose.portainer.staging.yml` (modify) | `UNLEASH_*` env for the backend service. | + +--- + +### Task 1: The Unleash stack + +**Files:** +- Create: `docker-compose.portainer.unleash.yml` +- Modify: `docs/DEPLOY.md` + +**Interfaces:** +- Consumes: nothing. +- Produces: a reachable Unleash server, and two client tokens — one scoped to the `development` environment, one to `production` — for Task 2's `UNLEASH_API_TOKEN`. + +> **This task ends with a human step.** The stack must be deployed in Portainer and the tokens generated by hand; neither can be automated from here. Every later task works without it — flags simply evaluate `False` when `UNLEASH_URL` is unset, which is the correct local and CI behaviour. + +- [ ] **Step 1: Write the stack file** + +```yaml +# Portainer Stack Definition for School Compare — UNLEASH (feature flags) +# +# Deploy as a *separate* Portainer stack ("schoolcompare-unleash"), alongside +# the production and staging stacks. It deliberately belongs to neither: a +# staging redeploy must not be able to disturb production's flag state. +# +# One instance serves both environments. Open-source Unleash ships with +# `development` and `production` environments and environment-scoped client +# tokens, so the same flag holds independent state in each. +# +# Portainer environment variables (set in Portainer UI -> Stack -> Environment): +# UNLEASH_DB_PASSWORD — PostgreSQL password for the Unleash database +# UNLEASH_ADMIN_PASSWORD — initial admin password for the Unleash UI +# UNLEASH_IP — macvlan IP for the Unleash server (default 10.0.1.152) + +services: + + unleash_db: + container_name: sc_unleash_postgres + image: postgres:16-alpine + environment: + POSTGRES_USER: unleash + POSTGRES_PASSWORD: ${UNLEASH_DB_PASSWORD} + POSTGRES_DB: unleash + volumes: + - unleash_postgres_data:/var/lib/postgresql/data + networks: + - unleash + healthcheck: + test: ["CMD-SHELL", "pg_isready -U unleash"] + interval: 10s + timeout: 5s + retries: 5 + start_period: 10s + restart: unless-stopped + + unleash: + container_name: sc_unleash + image: unleashorg/unleash-server:6 + environment: + DATABASE_URL: postgres://unleash:${UNLEASH_DB_PASSWORD}@unleash_db:5432/unleash + DATABASE_SSL: "false" + INIT_ADMIN_API_TOKENS: "" + UNLEASH_DEFAULT_ADMIN_PASSWORD: ${UNLEASH_ADMIN_PASSWORD} + depends_on: + unleash_db: + condition: service_healthy + networks: + unleash: {} + macvlan: + ipv4_address: ${UNLEASH_IP:-10.0.1.152} + healthcheck: + test: ["CMD-SHELL", "wget -qO- http://localhost:4242/health || exit 1"] + interval: 30s + timeout: 10s + retries: 3 + start_period: 30s + restart: unless-stopped + +networks: + unleash: + driver: bridge + macvlan: + external: true + name: macvlan + +volumes: + unleash_postgres_data: +``` + +- [ ] **Step 2: Document the runbook** + +Append to `docs/DEPLOY.md`: + +```markdown +## Feature flags (Unleash) + +Flag state lives in a self-hosted Unleash instance, deployed as its own +Portainer stack from `docker-compose.portainer.unleash.yml`. It is separate +from the application stacks on purpose — redeploying staging must not be able +to disturb production's flags. + +### First-time setup + +1. Deploy the stack in Portainer. Set `UNLEASH_DB_PASSWORD`, + `UNLEASH_ADMIN_PASSWORD` and (optionally) `UNLEASH_IP`. +2. Log in to the UI at `http://:4242` as `admin`. +3. Create one **client** API token per environment: + - `schoolcompare-staging`, environment **development** + - `schoolcompare-prod`, environment **production** + Client tokens, not admin tokens — the backend only reads. +4. Put each token in the matching Portainer stack's `UNLEASH_API_TOKEN` + variable, and set `UNLEASH_URL` to `http://:4242/api`. +5. Redeploy the application stacks. + +### Turning a feature on + +Flags are declared in `backend/flags.py` and appear in the Unleash UI on the +first evaluation after the backend starts. Toggle the flag in the environment +you want. A flip reaches school pages within about five minutes and place pages +within the hour — Next's ISR does the propagating, there is no webhook. + +If `UNLEASH_URL` is unset, every flag evaluates to `False`. That is the correct +behaviour for local development and CI, and means nothing here is required to +run the tests. +``` + +- [ ] **Step 3: Commit** + +```bash +git add docker-compose.portainer.unleash.yml docs/DEPLOY.md +git commit -m "feat(flags): add the Unleash stack and its runbook" +``` + +- [ ] **Step 4: Hand off the manual step** + +Tell the human: the stack file is ready, and Steps 1–5 of the runbook are theirs to run. Note that the remaining tasks do not block on it. + +--- + +### Task 2: The flag registry and client + +**Files:** +- Create: `backend/flags.py` +- Create: `backend/tests/test_flags.py` +- Modify: `backend/config.py` +- Modify: `requirements.txt` + +**Interfaces:** +- Consumes: `settings` from `backend/config.py`. +- Produces: + - `backend.flags.Flag` — frozen dataclass with `name: str`, `description: str`, `added: date` + - `backend.flags.REGISTRY: dict[str, Flag]` + - `backend.flags.MAX_FLAG_AGE_DAYS: int` (= 90) + - `backend.flags.init() -> None` + - `backend.flags.is_enabled(name: str) -> bool` + - `backend.flags.all_flags() -> dict[str, bool]` + +- [ ] **Step 1: Write the failing tests** + +Create `backend/tests/test_flags.py`: + +```python +"""Tests for the feature flag layer (spec 2026-08-23). + +None of these need a running Unleash. That is the point: an unset UNLEASH_URL +means every flag is False, which is what local development and CI get. +""" + +from datetime import date, timedelta + +import pytest + +from backend import flags + + +def test_every_declared_flag_is_keyed_by_its_own_name(): + # One string is the registry key, the Unleash flag name and the JSON key. + # A mismatch here would mean the UI toggles a flag the code never reads. + for key, flag in flags.REGISTRY.items(): + assert key == flag.name + + +def test_flag_names_are_snake_case(): + # Matches the API's existing convention (admission_distance, + # rwm_expected_pct) so no case transformation exists to get wrong. + for name in flags.REGISTRY: + assert name == name.lower() + assert "-" not in name and " " not in name + + +def test_an_unconfigured_client_evaluates_every_flag_false(monkeypatch): + monkeypatch.setattr(flags, "_client", None) + for name in flags.REGISTRY: + assert flags.is_enabled(name) is False + + +def test_an_undeclared_flag_is_false_rather_than_an_error(monkeypatch): + # A typo'd flag name must not raise in a request path. It is logged as an + # error, because an undeclared flag is always a bug. + monkeypatch.setattr(flags, "_client", None) + assert flags.is_enabled("no_such_flag") is False + + +def test_an_exploding_client_is_false_rather_than_a_500(monkeypatch): + class Boom: + def is_enabled(self, *a, **kw): + raise RuntimeError("unleash is on fire") + + monkeypatch.setattr(flags, "_client", Boom()) + name = next(iter(flags.REGISTRY)) + assert flags.is_enabled(name) is False + + +def test_all_flags_reports_every_declared_flag(monkeypatch): + monkeypatch.setattr(flags, "_client", None) + assert set(flags.all_flags()) == set(flags.REGISTRY) + assert all(v is False for v in flags.all_flags().values()) + + +def test_a_flag_older_than_the_limit_fails_this_test(): + """A tripwire, not an assertion about correctness. + + Flags are temporary scaffolding and the failure mode of every flag system + is accumulation. This fails on the day a flag turns 90, on whatever PR + happens to be open — which is the point: someone has to decide. + + To fix: delete the flag and the branches that read it, or, if it genuinely + still needs to exist, move its `added` date and say why in the commit. + """ + stale = [ + f.name for f in flags.REGISTRY.values() + if date.today() - f.added > timedelta(days=flags.MAX_FLAG_AGE_DAYS) + ] + assert not stale, ( + f"Flags older than {flags.MAX_FLAG_AGE_DAYS} days: {stale}. " + "Remove the flag and the code branches it guards, or move its `added` " + "date deliberately." + ) +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_flags.py -q` + +Expected: FAIL — `ModuleNotFoundError: No module named 'backend.flags'` + +- [ ] **Step 3: Add the dependency** + +Append to `requirements.txt`: + +``` +UnleashClient==6.0.1 +``` + +- [ ] **Step 4: Add the settings** + +In `backend/config.py`, inside `class Settings`, after the Typesense block: + +```python + # Feature flags (Unleash). An empty unleash_url disables flags entirely and + # every flag evaluates False — the correct behaviour for local development + # and CI, and the reason no test here needs a running Unleash. + unleash_url: str = "" + unleash_api_token: str = "" + unleash_app_name: str = "schoolcompare-backend" + # On a named volume, so a restart during an Unleash outage keeps + # last-known state instead of reverting a released feature to dark. + unleash_cache_directory: str = "/app/.unleash" +``` + +- [ ] **Step 5: Write the module** + +Create `backend/flags.py`: + +```python +"""Feature flags: what can be switched, and what is switched right now. + +Ship-dark, not a kill switch. Flags let work merge and deploy without becoming +visible; they are expected to flip about monthly, by a person, deliberately. +Nothing here does percentage rollouts or user targeting — the site has no user +identity to target. + +Unleash holds the state. It does not hold the list. REGISTRY below is that +list, and it exists for three reasons: the SDK evaluates an unknown flag to +False, so without a registry that is an *undeclared* False, indistinguishable +from a typo; /api/flags needs a key set to return when Unleash is unreachable; +and a flag in the UI but not in the registry is orphaned and should be visibly +so rather than quietly authoritative. + +Every flag defaults to False. There is no per-flag default, because a flag that +defaults on is a kill switch, and this is not one. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from datetime import date + +from .config import settings + +logger = logging.getLogger(__name__) + +# A flag is temporary scaffolding. See test_a_flag_older_than_the_limit. +MAX_FLAG_AGE_DAYS = 90 + + +@dataclass(frozen=True) +class Flag: + # One string: the registry key, the Unleash flag name, and the JSON key in + # /api/flags. snake_case, matching the API's existing convention. No case + # transformation anywhere, so there is no mapping layer to get wrong. + name: str + description: str # one line: what turning this on reveals + added: date # for the staleness tripwire + + +REGISTRY: dict[str, Flag] = { + f.name: f for f in ( + Flag( + name="admission_distance", + description=( + "The last-distance-offered figure on the Admissions tile and " + "the 'How far away are you?' section on school pages." + ), + added=date(2026, 8, 23), + ), + ) +} + + +_client = None + + +def init() -> None: + """Start the Unleash client, or log why flags are all off. + + Called once from the app lifespan. Never raises: a flag system that can + stop the API from booting is worse than one that is switched off. + """ + global _client + if not settings.unleash_url or not settings.unleash_api_token: + logger.warning( + "Unleash is not configured (UNLEASH_URL / UNLEASH_API_TOKEN); " + "every feature flag evaluates to False.") + return + + try: + from UnleashClient import UnleashClient + + _client = UnleashClient( + url=settings.unleash_url, + app_name=settings.unleash_app_name, + custom_headers={"Authorization": settings.unleash_api_token}, + cache_directory=settings.unleash_cache_directory, + refresh_interval=15, + ) + _client.initialize_client() + logger.info("Unleash client initialised against %s", settings.unleash_url) + except Exception: + # Fail closed and keep serving. The SDK also evaluates everything False + # until its first successful sync, so this is the same direction. + _client = None + logger.exception("Unleash client failed to start; flags are all False.") + + +def is_enabled(name: str) -> bool: + """Whether `name` is on. False for anything unknown, unreachable or broken.""" + if name not in REGISTRY: + logger.error( + "undeclared feature flag %r was evaluated; returning False. " + "Add it to backend/flags.py REGISTRY or fix the name.", name) + return False + if _client is None: + return False + try: + return bool(_client.is_enabled( + name, fallback_function=lambda feature_name, context: False)) + except Exception: + logger.exception("flag %r failed to evaluate; returning False", name) + return False + + +def all_flags() -> dict[str, bool]: + """Every declared flag and its current value. Serves /api/flags.""" + return {name: is_enabled(name) for name in REGISTRY} +``` + +- [ ] **Step 6: Run the tests to verify they pass** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_flags.py -q` + +Expected: PASS, 7 tests. + +- [ ] **Step 7: Run the whole backend suite** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. Nothing else imports `flags` yet. + +- [ ] **Step 8: Commit** + +```bash +git add backend/flags.py backend/tests/test_flags.py backend/config.py requirements.txt +git commit -m "feat(flags): the registry and a fail-closed Unleash client" +``` + +--- + +### Task 3: `/api/flags`, and keeping it off the public internet + +**Files:** +- Modify: `backend/app.py` +- Modify: `nextjs-app/app/api/[...path]/route.ts` +- Modify: `backend/tests/test_flags.py` +- Create: `nextjs-app/__tests__/api/proxyDenylist.test.ts` + +**Interfaces:** +- Consumes: `backend.flags.init`, `backend.flags.all_flags`. +- Produces: `GET /api/flags` → `{"": bool, ...}`, reachable only from inside the Docker network. + +> The endpoint and its exposure control ship together on purpose. The moment +> `/api/flags` exists, the public proxy at `app/api/[...path]/route.ts` will +> forward it, publishing the name and state of every unreleased feature. + +- [ ] **Step 1: Write the failing backend test** + +Append to `backend/tests/test_flags.py`: + +```python +def _client(): + from fastapi.testclient import TestClient + from backend import app as app_module + return TestClient(app_module.app, raise_server_exceptions=False) + + +def test_the_flags_endpoint_lists_every_declared_flag(monkeypatch): + monkeypatch.setattr(flags, "_client", None) + body = _client().get("/api/flags").json() + assert set(body) == set(flags.REGISTRY) + + +def test_the_flags_endpoint_answers_false_when_unleash_is_unreachable(monkeypatch): + # The endpoint must still answer. A frontend that cannot read flags renders + # everything dark, which is right; one that gets a 500 renders nothing. + monkeypatch.setattr(flags, "_client", None) + res = _client().get("/api/flags") + assert res.status_code == 200 + assert all(v is False for v in res.json().values()) +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_flags.py -q` + +Expected: FAIL — the endpoint 404s, so `set(body)` is `{"detail"}`. + +- [ ] **Step 3: Add the import and the lifespan call** + +In `backend/app.py`, add to the existing relative imports (after `from .data_loader import get_data_info as get_db_info`): + +```python +from . import flags +``` + +Inside `async def lifespan(app: FastAPI):`, immediately after the `print("Loading school data from marts...")` line: + +```python + flags.init() +``` + +- [ ] **Step 4: Add the endpoint** + +In `backend/app.py`, directly above `@app.get("/api/data-info")`: + +```python +@app.get("/api/flags") +@limiter.limit(f"{settings.rate_limit_per_minute}/minute") +async def get_feature_flags(request: Request): + """Every declared flag and its current value. + + Internal only. The Next proxy denies this path, because the response names + every unreleased feature the codebase knows about — which is exactly what + shipping dark is meant to keep quiet. + """ + return flags.all_flags() +``` + +- [ ] **Step 5: Run the backend tests to verify they pass** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. + +- [ ] **Step 6: Write the failing proxy test** + +Create `nextjs-app/__tests__/api/proxyDenylist.test.ts`: + +```typescript +/** + * The /api/* proxy is public. Anything it forwards is on the internet. + * + * @jest-environment node + */ +// The docblock above is load-bearing. jest.config.js sets jsdom globally, and +// NextRequest/NextResponse need the Web Fetch API globals that only the node +// environment provides — under jsdom this suite fails on import, not on an +// assertion. +import { NextRequest } from 'next/server'; +import { GET } from '@/app/api/[...path]/route'; + +function request(path: string) { + return new NextRequest(`http://localhost:3000/api/${path}`); +} + +describe('public API proxy', () => { + it('refuses to forward internal-only paths', async () => { + // /api/flags names every unreleased feature and its state. Forwarding it + // publishes the thing shipping dark exists to keep quiet. + const res = await GET(request('flags'), { params: Promise.resolve({ path: ['flags'] }) }); + expect(res.status).toBe(404); + }); + + it('does not deny a path that merely starts with the same letters', async () => { + // A prefix match would take /api/flagship down with /api/flags. + const res = await GET( + request('flagship'), { params: Promise.resolve({ path: ['flagship'] }) }); + expect(res.status).not.toBe(404); + }); +}); +``` + +- [ ] **Step 7: Run it to verify it fails** + +Run: `cd nextjs-app && npx jest __tests__/api/proxyDenylist.test.ts` + +Expected: FAIL — the first case returns 502 (no backend running in the test env), not 404. + +- [ ] **Step 8: Add the denylist** + +In `nextjs-app/app/api/[...path]/route.ts`, after the `STRIPPED_RESPONSE_HEADERS` constant: + +```typescript +/* + * API paths this public proxy must not forward. + * + * Matched on the first segment, exactly — a prefix match would take + * /api/flagship down with /api/flags. + * + * `flags` is here because GET /api/flags names every unreleased feature the + * codebase knows about, along with whether it is on. Publishing that defeats + * the point of shipping dark. Next reads it server-side via FASTAPI_URL, on + * the Docker network, which never transits this route. + * + * Anything else internal-only belongs here too. + */ +const INTERNAL_ONLY_SEGMENTS = new Set(['flags']); +``` + +And at the top of `handler`, immediately after `const { path } = await ctx.params;`: + +```typescript + if (INTERNAL_ONLY_SEGMENTS.has(path[0])) { + return NextResponse.json({ detail: 'Not Found' }, { status: 404 }); + } +``` + +- [ ] **Step 9: Run the frontend tests** + +Run: `cd nextjs-app && npx jest && npx tsc --noEmit` + +Expected: all pass, typecheck clean. + +- [ ] **Step 10: Commit** + +```bash +git add backend/app.py backend/tests/test_flags.py 'nextjs-app/app/api/[...path]/route.ts' nextjs-app/__tests__/api/proxyDenylist.test.ts +git commit -m "feat(flags): serve /api/flags, and keep the public proxy off it" +``` + +--- + +### Task 4: Reading flags from Next + +**Files:** +- Create: `nextjs-app/lib/flags.ts` +- Create: `nextjs-app/__tests__/lib/flags.test.ts` + +**Interfaces:** +- Consumes: `GET /api/flags`. +- Produces: `getFlags(): Promise` where `type Flags = Record`, and `FLAGS_REVALIDATE = 300`. + +> This ships with no consumer. The first flag needs none — the backend +> withholds the field and the frontend follows (Task 5). It is built anyway +> because "UI elements on existing pages" is one of the three surfaces this +> capability is for, and a flag layer that cannot gate a UI element is +> incomplete. Keep it minimal; do not add helpers nothing calls. + +- [ ] **Step 1: Write the failing test** + +Create `nextjs-app/__tests__/lib/flags.test.ts`: + +```typescript +import { getFlags } from '@/lib/flags'; + +describe('getFlags', () => { + afterEach(() => { jest.restoreAllMocks(); }); + + it('returns the flags the API reports', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue({ + ok: true, + json: async () => ({ admission_distance: true }), + } as Response); + await expect(getFlags()).resolves.toEqual({ admission_distance: true }); + }); + + it('returns no flags rather than throwing when the API is down', async () => { + // A page that cannot read flags must render everything dark, not 500. + // Fail-closed is the same direction as the backend's default. + jest.spyOn(global, 'fetch').mockRejectedValue(new Error('ECONNREFUSED')); + await expect(getFlags()).resolves.toEqual({}); + }); + + it('returns no flags rather than throwing on a non-200', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue({ + ok: false, status: 503, + } as Response); + await expect(getFlags()).resolves.toEqual({}); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cd nextjs-app && npx jest __tests__/lib/flags.test.ts` + +Expected: FAIL — `Cannot find module '@/lib/flags'`. + +- [ ] **Step 3: Write the module** + +Create `nextjs-app/lib/flags.ts`: + +```typescript +/** + * Reading feature flags. + * + * Server-side only. No flag value reaches the browser bundle, and there is no + * Unleash dependency in package.json — the SDK lives in FastAPI, which already + * owns every other piece of data this app renders. + * + * Flags are declared in backend/flags.py. A purely front-end flag still has to + * be declared there; it is a flat data edit, and the return is that one list + * answers "what flags exist" for the whole system. + */ + +export type Flags = Record; + +/* + * Reading flags pins the calling route to this ISR floor: Next uses the LOWEST + * revalidate among a route's fetches to set the whole route's revalidation + * frequency. 300s matches what /school/[slug] already sits at, so a page that + * reads flags is no more dynamic than a school page already is. + * + * It is also what makes a flip propagate without a webhook: five minutes on + * school pages, an hour on place pages, against flags that flip monthly. + */ +export const FLAGS_REVALIDATE = 300; + +const API = process.env.FASTAPI_URL || process.env.NEXT_PUBLIC_API_URL + || 'http://localhost:8000/api'; + +/** Every flag and its value. Never throws: an unreadable flag is a dark one. */ +export async function getFlags(): Promise { + try { + const res = await fetch(`${API}/flags`, { + next: { revalidate: FLAGS_REVALIDATE }, + }); + if (!res.ok) return {}; + return await res.json(); + } catch { + return {}; + } +} +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd nextjs-app && npx jest __tests__/lib/flags.test.ts && npx tsc --noEmit` + +Expected: PASS, 3 tests, typecheck clean. + +- [ ] **Step 5: Commit** + +```bash +git add nextjs-app/lib/flags.ts nextjs-app/__tests__/lib/flags.test.ts +git commit -m "feat(flags): server-side getFlags for the frontend" +``` + +--- + +### Task 5: Put `admission_distance` behind the flag + +**Files:** +- Modify: `backend/app.py:809` +- Modify: `backend/tests/test_flags.py` + +**Interfaces:** +- Consumes: `backend.flags.is_enabled`. +- Produces: `/api/schools/{urn}` omits `admission_distance` unless the flag is on. + +> One gate, at the source. The frontend needs no change: `DistanceSection` +> already returns `null` when `admissionDistance?.distance_m == null`, and +> `PrimarySchoolSections`/`SecondarySchoolSections` already condition the +> admissions block on `(admissions || admissionDistance)`. Only 57 local +> authorities publish cut-offs, so the off-path is the commonest path on the +> site and is well covered already. + +- [ ] **Step 1: Write the failing tests** + +Append to `backend/tests/test_flags.py`: + +```python +def _school_payload(monkeypatch, *, flag_on: bool): + """Fetch one school's payload with the distance flag forced on or off. + + The DataFrame shape is copied from test_school_details.py rather than + minimised: the endpoint reads a wide set of GIAS columns, and a trimmed + frame fails for reasons that have nothing to do with flags. + """ + import numpy as np + import pandas as pd + from fastapi.testclient import TestClient + from backend import app as app_module + + df = pd.DataFrame([{ + "urn": 150275, + "school_name": "West London Performing Arts Academy", + "phase": "Secondary", + "school_type": "Special post 16 institution", + "trust_name": None, + "religious_denomination": "Does not apply", + "gender": None, + "age_range": "16-25", + "admissions_policy": None, + "capacity": np.nan, + "gias_total_pupils": np.nan, + "headteacher_name": None, + "website": None, + "ofsted_grade": np.nan, + "local_authority": "Ealing", + "address": "268 Northfield Avenue, London, W5 4UB", + "postcode": "W5 4UB", + "latitude": 51.4986, + "longitude": -0.3148, + "year": np.nan, + "total_pupils": np.nan, + "eligible_pupils": np.nan, + "rwm_expected_pct": np.nan, + }]) + + monkeypatch.setattr(app_module, "load_school_data", lambda: df) + # Two arguments: get_supplementary_data(db, urn). See backend/app.py:763. + monkeypatch.setattr( + app_module, "get_supplementary_data", + lambda db, urn: {"admission_distance": {"distance_m": 772.49, + "year": 2024}}) + monkeypatch.setattr(flags, "is_enabled", lambda name: flag_on) + + client = TestClient(app_module.app, raise_server_exceptions=False) + res = client.get("/api/schools/150275") + assert res.status_code == 200, res.text + return res.json() + + +def test_the_distance_field_is_absent_when_the_flag_is_off(monkeypatch): + """Absent, not null, and withheld at the source. + + /api/schools/ is public and unauthenticated. Leaving a withheld field in + the payload while declining to render it hands the record to anyone who + opens the network tab — the reasoning already recorded in c9a1892. + """ + body = _school_payload(monkeypatch, flag_on=False) + assert "admission_distance" not in body + + +def test_the_distance_field_is_present_when_the_flag_is_on(monkeypatch): + body = _school_payload(monkeypatch, flag_on=True) + assert body["admission_distance"]["distance_m"] == 772.49 +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests/test_flags.py -q` + +Expected: FAIL on `test_the_distance_field_is_absent_when_the_flag_is_off` — the key is present with value `None`. + +- [ ] **Step 3: Gate the field** + +In `backend/app.py`, replace line 809: + +```python + "admission_distance": supplementary.get("admission_distance"), +``` + +with: + +```python + # Behind a flag, and withheld at the source rather than rendered-but- + # hidden: this endpoint is public and unauthenticated, so a field left + # in the payload is a published field. The key is absent, not null — + # null would state that this school has no cut-off, which is a + # different claim from "we are not publishing cut-offs". + **({"admission_distance": supplementary.get("admission_distance")} + if flags.is_enabled("admission_distance") else {}), +``` + +- [ ] **Step 4: Run the backend suite** + +Run: `uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q` + +Expected: all pass. + +- [ ] **Step 5: Check the frontend type tolerates an absent key** + +Run: `cd nextjs-app && npx tsc --noEmit` + +`lib/types.ts:364` declares `admission_distance: SchoolAdmissionDistance | null`. An absent key is `undefined`, which that type does not admit. Change it to: + +```typescript + /** + * Absent — not null — when the admission_distance flag is off. Null means + * "this school has no published cut-off"; absent means "cut-offs are not + * being published at all". They are different claims and the type says so. + */ + admission_distance?: SchoolAdmissionDistance | null; +``` + +`app/school/[slug]/page.tsx` already reads it as `admission_distance ?? null`, so no call site changes. + +- [ ] **Step 6: Run the frontend suite** + +Run: `cd nextjs-app && npx jest && npx tsc --noEmit && npm run build` + +Expected: all pass, build green. + +- [ ] **Step 7: Commit** + +```bash +git add backend/app.py backend/tests/test_flags.py nextjs-app/lib/types.ts +git commit -m "feat(flags): ship last-distance-offered dark behind a flag" +``` + +--- + +### Task 6: Flag-aware E2E journeys + +**Files:** +- Modify: `e2e/tests/journeys.spec.ts` + +**Interfaces:** +- Consumes: `GET /api/flags` (reachable from Playwright, which runs against the frontend's own origin — note that the *proxy* denies it, so these read it through the same denial and must handle that; see Step 1). + +- [ ] **Step 1: Write the journeys** + +The existing distance journeys at `e2e/tests/journeys.spec.ts:1214–1310` use +`schoolWithCutoff()`, which returns `null` and skips when no school has a +figure. With the flag off they would skip silently and the suite would go +green — a flag accidentally off in staging would look like a pass. + +`/api/flags` is denied by the public proxy, so Playwright cannot read it +directly. The flag's observable effect is what the tests assert on instead: +whether the field appears in a school payload at all. + +Insert directly above the `schoolWithCutoff` helper: + +```typescript +/** + * Whether the last-distance-offered feature is switched on here. + * + * Read from the data rather than from /api/flags, which the public proxy + * denies on purpose — the endpoint names unreleased features. The observable + * effect is the field's presence: the flag is off iff no candidate school + * carries an `admission_distance` key at all. + * + * The distinction that matters: `admission_distance: null` means this school + * has no published cut-off, and the key being ABSENT means cut-offs are not + * being published at all. + */ +async function distanceFeatureIsOn(page: Page): Promise { + for (const urn of CUTOFF_CANDIDATE_URNS) { + const res = await page.request.get(`/api/schools/${urn}`); + if (!res.ok()) continue; + if ('admission_distance' in (await res.json())) return true; + } + return false; +} +``` + +Then add, directly after the `schoolWithCutoff` helper: + +```typescript +test('when the distance feature is on, a school with a cut-off is findable', async ({ page }) => { + /* + * The gate that stops the other distance journeys passing vacuously. + * + * They all skip when schoolWithCutoff() finds nothing, which is right when + * the feature is off — but it means a feature that is *supposed* to be on + * and is silently broken shows up as a green run full of skips. This test + * fails in that case. + */ + test.skip(!(await distanceFeatureIsOn(page)), + 'the admission_distance flag is off in this environment'); + + expect(await schoolWithCutoff(page), + 'the distance feature is on, but no candidate school has a cut-off — ' + + 'the flag is on and the data or the query behind it is broken') + .not.toBeNull(); +}); + +test('with the distance feature off, the section is absent rather than empty', async ({ page }) => { + // Shipping dark means the page renders as it did before the feature existed, + // not as a feature with its content removed. + test.skip(await distanceFeatureIsOn(page), + 'the admission_distance flag is on in this environment'); + + // A school that exists, found rather than hardcoded — a 404 page would + // satisfy the absent-heading assertion without proving anything. + // + // A plain loop, not Array.find: find's predicate is synchronous, so an async + // one returns a Promise, every Promise is truthy, and it would always hand + // back the first URN whether or not that school exists. + let urn: number | null = null; + for (const candidate of CUTOFF_CANDIDATE_URNS) { + if ((await page.request.get(`/api/schools/${candidate}`)).ok()) { + urn = candidate; + break; + } + } + expect(urn, 'no candidate school resolves in this environment').not.toBeNull(); + + await page.goto(`/school/${urn}`); + await expect(page.locator('h1')).toBeVisible(); + + await expect(page.getByRole('heading', { name: /How far away are you\?/ })) + .toHaveCount(0); +}); + +test('/api/flags is not reachable from the public internet', async ({ page }) => { + // It names every unreleased feature and whether it is on. Next reads it + // server-side over the Docker network; the public proxy must deny it. + const res = await page.request.get('/api/flags'); + expect(res.status()).toBe(404); +}); +``` + +- [ ] **Step 2: Verify the spec compiles and the tests are collected** + +Run: `cd e2e && npx playwright test --list` + +Expected: the three new titles appear, total count rises by 3. + +- [ ] **Step 3: Commit** + +```bash +git add e2e/tests/journeys.spec.ts +git commit -m "test(e2e): make the distance journeys fail loudly, not skip quietly" +``` + +--- + +### Task 7: Wire the environment variables + +**Files:** +- Modify: `docker-compose.yml` +- Modify: `docker-compose.portainer.yml` +- Modify: `docker-compose.portainer.staging.yml` + +**Interfaces:** +- Consumes: the tokens from Task 1. +- Produces: a backend that can reach Unleash in each environment. + +- [ ] **Step 1: Add the variables to all three stacks** + +In each file, in the **backend** service's `environment` block, after the +`TYPESENSE_API_KEY` line, add: + +```yaml + UNLEASH_URL: ${UNLEASH_URL:-} + UNLEASH_API_TOKEN: ${UNLEASH_API_TOKEN:-} +``` + +Unset by default on purpose: an environment without them has every flag off, +which is the correct dark state rather than a failure. + +- [ ] **Step 2: Add the cache volume to the two Portainer stacks** + +The SDK's disk cache must survive a container restart, or a restart during an +Unleash outage reverts a released feature to dark. + +In `docker-compose.portainer.yml` and `docker-compose.portainer.staging.yml`, +add to the backend service: + +```yaml + volumes: + - unleash_cache:/app/.unleash +``` + +(If the backend service already has a `volumes:` block, add the one line to it +rather than a second block.) + +And to each file's top-level `volumes:` block: + +```yaml + unleash_cache: +``` + +- [ ] **Step 3: Document the Portainer variables** + +In `docker-compose.portainer.staging.yml`, add to the header comment list: + +``` +# UNLEASH_URL — http://:4242/api (empty = all flags off) +# UNLEASH_API_TOKEN — Unleash *client* token, environment: development +``` + +And the same in `docker-compose.portainer.yml` with `environment: production`. + +- [ ] **Step 4: Commit** + +```bash +git add docker-compose.yml docker-compose.portainer.yml docker-compose.portainer.staging.yml +git commit -m "chore(flags): wire UNLEASH_URL and the SDK cache volume into the stacks" +``` + +--- + +### Task 8: Open the PR + +- [ ] **Step 1: Run everything** + +```bash +uv run --quiet --with-requirements requirements.txt --with pytest --with "httpx==0.27.0" python -m pytest backend/tests -q +cd nextjs-app && npx jest && npx tsc --noEmit && npm run build +cd ../e2e && npx playwright test --list +``` + +Expected: backend green, frontend green, typecheck clean, build green, e2e collects. + +- [ ] **Step 2: Open the PR against `main`** + +Body must state: the flag defaults off, so merging changes nothing visible in +any environment until someone toggles it in Unleash; and that Task 1's manual +Portainer steps are a prerequisite for the flag to be togglable at all. + +## Notes for the executor + +**Nothing here needs a running Unleash.** Every test passes with `UNLEASH_URL` +unset, which is also what CI gets. Task 1's manual steps only decide whether +the flag can be *turned on*, not whether the code works. + +**The staleness test will fail on 2026-11-21** if `admission_distance` is still +declared. That is intended — see the docstring. It fails on whatever PR is open +at the time, which is the mechanism, not a bug in it. + +**Do not add a webhook.** An earlier draft of the spec had one; §4 records why +it was removed and why time-based revalidation is enough.