From 96a2c87f388b83461500c1f3cd785e29bcc594d9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 07:33:02 +0200 Subject: [PATCH 01/30] docs: issue-batch-413-424 design Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../2026-10-09-issue-batch-413-424-design.md | 296 ++++++++++++++++++ 1 file changed, 296 insertions(+) create mode 100644 docs/superpowers/specs/2026-10-09-issue-batch-413-424-design.md diff --git a/docs/superpowers/specs/2026-10-09-issue-batch-413-424-design.md b/docs/superpowers/specs/2026-10-09-issue-batch-413-424-design.md new file mode 100644 index 00000000..1b751679 --- /dev/null +++ b/docs/superpowers/specs/2026-10-09-issue-batch-413-424-design.md @@ -0,0 +1,296 @@ +# Issue batch #413–#424 — design + +**Date:** 2026-10-09 · **Status:** approved · **Branch:** `feat/issue-batch-413-424` · **One combined PR** + +Twelve open issues filed on 2026-10-08, fixed together. #317 (re-verify the 28 +hi-fi screens) is out: it needs the external design file, which neither the repo +nor this session can read. + +## Scope + +| # | Area | One-line fix | +|---|---|---| +| 413 | background_tasks | Admin page 500s on Postgres: four timestamp columns declared naive in the model | +| 414 | ui | Sign-in aside footer token below WCAG AA | +| 415 | i18n | Host override layer for module copy | +| 416 | auth | Only top-level navigations record the post-login target | +| 417 | db | Tenant filter turns an outer join into an inner join | +| 418 | hosting | Public tenancy API: mode, single-tenant id, `require_tenant`, complete `Vary` | +| 419 | hosting | gen-pages `@source` reaches subdirectories of wheel modules | +| 420 | ui | Brand foreground ink follows the brand colour; sidebar active row uses it | +| 421 | ui | `NativeSelect` gets a `wrapperClassName` | +| 422 | admin | `/admin/users/` title, `/admin/settings/` h1, admin errors inside the admin shell | +| 423 | hosting | `TenantMiddleware` merges every `Vary` line and never drops `*` | +| 424 | hosting | `tenant_source` is `None` whenever no tenant is bound | + +Out of scope: #317; the optional route-walking test helper floated in #418; the +branding-settings alternative for #415. + +## Design + +### #413 — naive timestamps in `TaskExecution` + +`queued_at`, `started_at`, `finished_at` and `heartbeat_at` +(`modules/background_tasks/background_tasks/models.py`) are bare +`datetime | None`, so SQLModel maps them to `timestamp without time zone`. +Every writer and comparer already uses aware UTC (`now_utc()`, +`datetime.now(UTC)`), and asyncpg refuses an aware parameter against a naive +column. + +The schema side is already done: migration `e5f2a8c1d7b3_timestamps_timezone_aware` +retypes these four columns, and `users_refresh_token`'s, to `timestamptz` on +Postgres. Only the model is wrong. Fix: `Field(default=None, sa_type=DateTime(timezone=True))` +on the four fields. **No new migration.** + +The same pass covers the model of every column that migration retyped +(`users_refresh_token.created_at/expires_at/revoked_at`), so model and schema +agree everywhere `alembic check` looks. The other bare-`datetime` fields the +audit turned up (`user_role.assigned_at`, `permissions`, `audit_log`) are +checked against their migrated column types. If a column is still naive in the +schema, it stays as it is: changing it needs a migration and has no reported bug +behind it. + +### #414 — aside footer contrast + +`--color-dark-text-subtle` goes from `oklch(0.45 0.01 250)` (2.7:1 on the aside) +to `oklch(0.62 0.01 250)`. That clears 4.5:1 and stays a shade lighter than +`--color-dark-text-muted` (0.6), so the two tokens remain distinct. The footer is +its only consumer. + +### #415 — host override layer + +A host ships `host/locales/overrides/.json`, keyed by full dotted path, for +example `{"users": {"login": {"aside_heading": "…"}}}` or the flat +`{"users.login.aside_heading": "…"}`. The overrides are applied after every module, +framework and host source. + +- `I18nRegistry.add_overrides(dir)` applies them at the end of `load()`, before + locale layering and snapshots, so the server-side `Translator`, menus and + Inertia shared props all see them. +- **An override may only replace an existing key.** An unknown key is skipped + with a warning and reported by `make doctor` as a new locale diagnostic. A + typo can't silently invent a key, and `keys.generated.ts` doesn't churn. +- **Audience is preserved.** An override of a key that is only in the admin + catalog stays admin-only. The public snapshot is updated only for keys it + already holds, so the #248 split cannot leak. +- `build_i18n_registry` registers `host/locales/overrides` when it exists. + Documented in the i18n docs next to the `host` namespace. + +### #416 — post-login target + +`AuthMiddleware` keeps redirecting every unauthenticated, non-public request +exactly as today. It writes `SESSION_NEXT_KEY`, and passes a `next` to the +provider, only for a top-level navigation: + +1. If `Sec-Fetch-Dest` is present, the request must be `document`. +2. Else, if `Sec-Fetch-Mode` is present, it must be `navigate`. +3. Else, `Accept` must include `text/html`. +4. **And always** an Inertia visit (`X-Inertia: true`), so a session that + expires mid-navigation still returns the user to the page they clicked. + +A favicon, script, image or `fetch()` no longer overwrites the target. The +favicon half of the issue is already fixed on `main` (`branding_head()` falls +back to a data-URI icon). Only the scaffold template is re-checked. + +### #417 — tenant filter and outer joins + +A table referenced only inside a function (`func.count(Child.id)`) loses its ORM +annotation. `_plain_tables` then treats it as a Core table and puts +`child.tenant_id = :t` in `WHERE`, which defeats the outer join. + +The obvious fix is to drop every outer-join target from the `WHERE` candidates, +and it is a tenant leak in one shape. When the target is a raw +`Model.__table__`, nothing puts its predicate in the `ON` clause: +`with_loader_criteria` only covers ORM entities. With the `WHERE` predicate gone, +another tenant's child rows would match the `ON` and inflate the count. So: + +- For each outer-join entry in `stmt._setup_joins` whose **target is an ORM + entity** (its loader criteria land in `ON`), exclude that entity's table from + `_plain_tables`. Full outer joins are treated the same way. +- **A raw-table outer-join target keeps today's `WHERE` predicate.** That + over-filters but is never a leak. Moving it into `ON` would need the statement's + join rewritten, which is outside this fix. +- The soft-delete path (`_soft_delete_criteria`) uses the same helper and gets the + same fix. + +Tests, in `framework/db/tests/test_query_filters.py`, with a new +tenant-scoped parent/child pair: + +- The issue's four shapes keep the unused parent with count 0. +- A second tenant's child rows never appear in any count. This is the leak guard. +- An inner join through `_setup_joins` still filters the child. +- A raw-table outer join stays filtered. +- The soft-delete variant. + +This task goes to an `opus` implementer plus a separate security review. + +### #418 — public tenancy API + +New public module `simple_module_hosting.tenancy`. Modules import from it instead +of reading the middleware stack. + +| API | Behaviour | +|---|---| +| `TenancyMode` | `StrEnum` `SINGLE` / `MULTI`, in `simple_module_core.tenancy` so core and modules can name it without importing hosting. | +| `tenancy_mode(app)` | Read from `app.state.sm.tenancy`, a new defaulted field on `Services` that the app builder sets where it decides on `TenantMiddleware`: `MULTI` iff `multi_tenant`. | +| `single_tenant_id(app)` | `app.state.sm.db.default_tenant_id`: the host's `default_tenant`, else `DEFAULT_TENANT_ID`. | +| `require_tenant(*, on_missing=403, detail="tenant_required")` | Async yield-dependency factory. `SINGLE` binds `single_tenant_id(app)`; `MULTI` takes `request.state.tenant_id`, or raises `on_missing`, which is a status code or a `Callable[[Request], Exception]` (a public surface passes its own 404). It enters `tenant_context(tenant)` for the rest of the request and yields the tenant id. Documented to be listed before `get_db`, and verified by a test that a write commits under the bound tenant. | +| `tenant_vary(request)` | The headers the resolved tenant depended on. `TenantMiddleware` now records `request.state.tenant_vary`. Meant for a route that builds its own cache headers before the middleware sees the response, e.g. a 304. | + +`Vary` completeness, as chosen at the question batch: + +- The tenants resolver adds `Cookie` when the source is `"session"`. +- The no-resolver `"claim"` path adds `Cookie` and `Authorization`, since the claim + came from whichever credential authenticated the request. + +Docs: the multi-tenancy guide gains a "For module authors" section with the four +calls and the dependency-order rule. + +### #419 — `@source` for wheel-module subdirectories + +uv writes `.venv/.gitignore` = `*`, and Tailwind's scanner honours ignore files +below an `@source` base, so only the base directory's own files are scanned. + +`render_modules_css()` (`framework/hosting/simple_module_hosting/assets.py`) keeps +the base line for an out-of-repo module and adds one `@source "/**/*.{ts,tsx}"` +for every descendant directory that directly holds a `.ts`/`.tsx` file: + +- skipping `__pycache__` and `node_modules`; +- de-duplicated and sorted, so the output is deterministic; +- tolerant of a directory that doesn't exist, as today. + +In-repo modules still get no lines. + +### #420 — brand foreground ink + +`deriveBrandRamp` (`packages/ui/src/lib/color.ts`) also sets +`--primary-foreground` and `--sidebar-primary-foreground`. It picks by **WCAG +contrast ratio**, not a fixed lightness threshold: white if white reaches 4.5:1 on +the brand colour, otherwise a dark ink (`oklch(0.2 0.02 250)`); if neither +reaches 4.5:1, whichever is higher. + +- That gives dark ink on `#62B8E2` and `#9AD3EF`, and white on `#2E6DB0` and + `#16276E`, matching the issue's measurements. +- `BrandingHead` already applies and clears every key the ramp returns, so it + needs no change. +- `sidebar-theme.ts` `activeClass` becomes `bg-primary text-primary-foreground`. +- `BrandingMark`'s gradient badge is checked for the same light-brand problem and + fixed the same way if it has it. + +### #421 — `NativeSelect` width + +New `wrapperClassName` prop, merged with `cn()` onto +`[data-slot=native-select-wrapper]`. `className` keeps targeting the ``. Width utilities belong on + * `wrapperClassName`: the select is `w-full` of a `w-fit` wrapper, so a width + * on the select alone cannot widen it (#421). + */ +function NativeSelect({ + className, + wrapperClassName, + size = 'default', + ...props +}: Omit, 'size'> & { + size?: 'sm' | 'default'; + wrapperClassName?: string; +}) { + return ( +
+``` + +- [ ] **Step 4: Run** the test again. Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add packages/ui/src/components/ui/native-select.tsx packages/ui/src/components/ui/native-select.test.tsx +git commit -m "feat(ui): NativeSelect wrapperClassName so callers can size the control (#421)" +``` + +--- + +### Task 11: Admin titles, settings h1, admin errors inside the shell (#422) + +**Independent of the others, but adds locale keys, so it regenerates i18n.** + +**Files:** +- Modify: `modules/users/users/pages/Users/Index.tsx` (add ``) +- Modify: every other page assigned `.layout = [AdminLayout]` that lacks ``) +- Modify: `modules/settings/settings/locales/*.json` (add `settings.modules_edit.heading`, or reuse an existing title key if one already names the screen, e.g. `settings.*.title`; check first) +- Modify: `packages/ui/src/components/ErrorScreen.tsx` (add `inline?: boolean`) +- Modify: `host/client_app/pages/Error.tsx` (function layout) +- Create: `host/client_app/pages/ErrorShell.tsx` (keeps `Error.tsx` under the cap) +- Create: `host/client_app/admin-pages-head.test.ts` (guard) +- Modify: `host/client_app/pages/Error.test.tsx` +- Modify: `tests/e2e/test_document_titles.py` (add `/admin/users/`), `tests/e2e/test_error_pages.py` (an admin 404 inside the shell) +- Regenerate: `packages/i18n/src/keys.generated.ts`, `packages/i18n/src/generated-resources.ts` + +**Interfaces:** +- Produces: `ErrorScreen` prop `inline?: boolean`. Default `false` keeps today's full-viewport frame; `true` drops `min-h-screen` and the page background. + +- [ ] **Step 1: Write the failing tests** + +`host/client_app/admin-pages-head.test.ts`: + +```ts +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; + +/** Every page rendered in AdminLayout sets a document title (#422: /admin/users/ had none). */ +function pagesUnder(dir: string): string[] { + return readdirSync(dir).flatMap((name) => { + const path = join(dir, name); + if (statSync(path).isDirectory()) return pagesUnder(path); + return name.endsWith('.tsx') && !name.endsWith('.test.tsx') ? [path] : []; + }); +} + +const ROOT = join(__dirname, '..', '..'); +const moduleDirs = readdirSync(join(ROOT, 'modules')).map((m) => join(ROOT, 'modules', m, m, 'pages')); +const files = moduleDirs + .filter((d) => { try { return statSync(d).isDirectory(); } catch { return false; } }) + .flatMap(pagesUnder) + .filter((f) => /\.layout\s*=\s*\[\s*AdminLayout/.test(readFileSync(f, 'utf8'))); + +describe('admin pages', () => { + it('found some', () => expect(files.length).toBeGreaterThan(5)); + it.each(files)('%s renders ', (file) => { + expect(readFileSync(file, 'utf8')).toMatch(/]*title=/); + }); +}); +``` + +`Error.test.tsx`: add cases using its existing `usePage` mock. Make the mocked page `url` and props configurable per test. + +```tsx +it('renders inside AdminLayout for a signed-in admin under /admin', () => { /* url '/admin/users/999999', auth.isAuthenticated true, adminSidebar non-empty → expect the admin nav landmark / AdminLayout marker */ }); +it('renders bare for an anonymous 401 under /admin', () => { /* url '/admin/users/', auth.isAuthenticated false → no AdminLayout */ }); +it('renders bare outside /admin', () => { /* url '/administer' → no AdminLayout (segment-aware) */ }); +it('renders bare when adminSidebar is empty', () => { /* signed-in non-admin 403 */ }); +``` + +Write each case fully. Detect `AdminLayout` the way existing layout tests do (e.g. mock `@simple-module-py/ui` `AdminLayout` as `({children}) =>
{children}
`). + +`ModulesEdit`: a vitest test that renders the page with minimal props (follow any existing `ModulesEdit` or settings page test; if none exists, mock `usePage` the way other page tests do) and asserts `getByRole('heading', { level: 1 })`. + +- [ ] **Step 2: Run to confirm failures** + +Run: `npx vitest run host/client_app/admin-pages-head.test.ts host/client_app/pages/Error.test.tsx` +Expected: the guard fails for `Users/Index.tsx` (and any other offenders); the Error admin cases fail. + +- [ ] **Step 3: Implement** + +`Users/Index.tsx`: `import { Head } from '@inertiajs/react';` and render `` as the first child of the returned fragment. Wrap in `<>…` if the page returns a single `PageShell`. Do the same for every other offender the guard lists, each with its existing title key. + +`ModulesEdit.tsx`: add an `

` that names the screen, styled like `PageShell`'s heading (`PageShell.tsx:80`), placed at the top of the main pane. Use a translation key; add `settings.modules_edit.heading` to every `modules/settings/settings/locales/.json` if no suitable key exists. + +`ErrorScreen.tsx`: + +```tsx + /** Inside an app shell: no full-viewport frame or page background (#422). */ + inline?: boolean; +... +
+``` + +`ErrorShell.tsx`: + +```tsx +import { usePage } from '@inertiajs/react'; +import { AdminLayout } from '@simple-module-py/ui'; +import type { SharedProps } from '@simple-module-py/ui/types'; +import { createContext, type ReactNode, useContext } from 'react'; + +/** `/admin` or anything under `/admin/` — not `/administer`. */ +export function isAdminPath(url: string): boolean { + const path = url.split(/[?#]/, 1)[0]; + return path === '/admin' || path.startsWith('/admin/'); +} + +const InShell = createContext(false); +export const useInAdminShell = () => useContext(InShell); + +/** + * Keeps an admin inside the admin shell when a page under /admin fails (#422). + * Only for a signed-in viewer whose admin menu reached the page: an anonymous + * 401/419 has no admin to keep in context, and a 500 raised before the shared + * props were built has no menu to render. + */ +export function ErrorShell({ children }: { children: ReactNode }) { + const page = usePage(); + const props = page.props as unknown as Partial & { adminSidebar?: unknown[] }; + const inShell = + isAdminPath(page.url) && + Boolean(props.auth?.isAuthenticated) && + Array.isArray(props.adminSidebar) && + props.adminSidebar.length > 0; + if (!inShell) return <>{children}; + return ( + + {children} + + ); +} +``` + +Check where `adminSidebar` actually lives in the shared props: `menus.adminSidebar` or top-level. Grep `adminSidebar` in `packages/ui/src` and `host/client_app/pages/Admin.tsx`, and use the same path. + +`Error.tsx`: `ErrorPage.layout = (page: ReactNode) => {page};` and pass `inline={useInAdminShell()}` to `ErrorScreen`. + +Regenerate i18n: `uv run --project host python scripts/gen_i18n.py`. + +- [ ] **Step 4: Run tests** + +Run: `npx vitest run host/client_app packages/ui modules/settings modules/users` +Expected: PASS. + +Then extend the e2e files: +- `tests/e2e/test_document_titles.py`: add `("/admin/users/", "Users")` to `_TITLED_PAGES`. +- `tests/e2e/test_error_pages.py`: add a test that, signed in as admin, visits `/admin/users/00000000-0000-0000-0000-000000000000` (or the module's real id format), expects the 404 copy, and expects the admin sidebar navigation to be present. Follow the file's existing login helper. + +These run in the integration task against a live server. + +- [ ] **Step 5: Commit** + +```bash +git add modules/users/users/pages modules/settings/settings packages/ui/src/components/ErrorScreen.tsx host/client_app packages/i18n/src tests/e2e +git commit -m "fix(admin): page titles, settings h1, admin errors inside the admin shell (#422)" +``` + +--- + +### Task 12: Integration — full gates, changelog, live checks + +**Sequential, last. Done by the lead (or a sonnet agent), not in parallel with anything.** + +- [ ] **Step 1:** `uv run --project host python scripts/gen_i18n.py`. Confirm the generated keys are committed and every installed namespace is present. +- [ ] **Step 2:** `make lint`. Expected exit 0. Fix anything (e.g. `ruff format docs/` for Python snippets in the new docs). +- [ ] **Step 3:** Run the full Python suite with the worktree's `.env` moved aside: `uv run pytest -q`. Expected: 0 failures. +- [ ] **Step 4:** Postgres: + + `SM_TEST_DATABASE_URL=postgresql+asyncpg://postgres:postgres@localhost:5432/sm_test uv run pytest -p no:anyio framework/db/tests framework/hosting/tests/test_tenancy_api.py modules/tenants/tests modules/background_tasks/tests modules/users/tests -q` + + Then `SM_MIGRATIONS_PG_URL=postgresql+asyncpg://postgres:postgres@localhost:5432/sm_migrations make migrations-roundtrip-pg` (create the DB first). Expected: green. +- [ ] **Step 5:** `npm test`. Expected: green. +- [ ] **Step 6:** Start the app on spare ports and run the e2e files touched in Task 11: `tests/e2e/test_document_titles.py tests/e2e/test_error_pages.py`. Follow `docs/e2e-testing.md` and the memory notes on ports and `E2E_PASSWORD`. If no `running-the-stack` skill exists, run `/runbook` once the app is up, and commit the skill. +- [ ] **Step 7:** `CHANGELOG.md` `[Unreleased]`: one entry per issue under Added (#415, #418, #421), Fixed (#413 pinned, #414, #416, #417, #419, #420, #422, #423, #424), and Changed (Vary: Cookie/Authorization). Commit as `docs: changelog for #413–#424`. + +--- + +## Self-review notes + +- **Spec coverage:** every row of the spec's scope table maps to a task (413→5, 414/420→9, 415→7, 416→6, 417→4, 418→1+2+3, 419→8, 421→10, 422→11, 423/424→1). +- **Deviations recorded:** + - #413 needs no model change: it is already fixed on `main` by #406, unreleased. + - #416 falls back to recording when no fetch metadata is present, rather than requiring `Accept: text/html`, so existing clients and tests keep working. +- **Type consistency:** `tenant_vary` is `tuple[str, ...]` in both Task 1 and Task 2. `TenancyMode` is defined once (core), re-exported by hosting. `ErrorScreen.inline` is used only by `Error.tsx`. From cb6d1ce36f81adeaecc1a2db1b30c1f56a6eba8a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 07:42:13 +0200 Subject: [PATCH 03/30] fix(hosting): merge every Vary line and blank tenant_source with no tenant (#423, #424) Also records the resolution's vary on request.state.tenant_vary and makes the claim path vary on Cookie, Authorization. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../hosting/simple_module_hosting/_tenant.py | 47 +++++++--- .../hosting/tests/test_tenant_vary_merge.py | 91 +++++++++++++++++++ 2 files changed, 127 insertions(+), 11 deletions(-) create mode 100644 framework/hosting/tests/test_tenant_vary_merge.py diff --git a/framework/hosting/simple_module_hosting/_tenant.py b/framework/hosting/simple_module_hosting/_tenant.py index 0513bd36..7b73420a 100644 --- a/framework/hosting/simple_module_hosting/_tenant.py +++ b/framework/hosting/simple_module_hosting/_tenant.py @@ -15,6 +15,8 @@ TENANT_HEADER = "X-Tenant-ID" +_CREDENTIAL_VARY = ("Cookie", "Authorization") + @dataclass(frozen=True, slots=True) class TenantResolution: @@ -48,6 +50,10 @@ class TenantResolution: def _normalise(result: object) -> TenantResolution: if isinstance(result, TenantResolution): + # No tenant, no source (#424) — whatever the resolver said. The vary + # names stay: the header's absence was an input too. + if result.tenant_id is None and result.source is not None: + return TenantResolution(None, None, result.vary) return result if isinstance(result, tuple): tenant_id, source = result[0], (result[1] if len(result) > 1 else None) @@ -55,17 +61,26 @@ def _normalise(result: object) -> TenantResolution: return TenantResolution(result, "resolver" if result is not None else None) # type: ignore[arg-type] +def _tokens(value: str) -> list[str]: + return [v.strip() for v in value.split(",") if v.strip()] + + def merge_vary(existing: str | None, names: tuple[str, ...]) -> str | None: - """``existing`` with ``names`` appended, case-insensitively de-duplicated.""" - present = [v.strip() for v in (existing or "").split(",") if v.strip()] + """``existing`` plus ``names``, de-duplicated case-insensitively (first spelling wins). + + ``*`` anywhere in ``existing`` returns it unchanged: the response is + already uncacheable, and a finite list would make it cacheable. + """ + present = _tokens(existing or "") if "*" in present: return existing - seen = {v.lower() for v in present} - for name in names: + out: list[str] = [] + seen: set[str] = set() + for name in (*present, *names): if name.lower() not in seen: - present.append(name) + out.append(name) seen.add(name.lower()) - return ", ".join(present) if present else None + return ", ".join(out) if out else None class TenantMiddleware: @@ -78,7 +93,8 @@ class TenantMiddleware: Also stores the resolved value on ``request.state.tenant_id`` and where it came from on ``request.state.tenant_source`` (``fixed``, ``subdomain``, ``header``, ``session``, ``claim``, ``anon_header``, ``resolver`` or ``None``). - Headers the answer depended on are added to the response ``Vary``. + Headers the answer depended on are added to the response ``Vary`` and + recorded as ``request.state.tenant_vary`` (a tuple of header names). Resolution: @@ -111,6 +127,7 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: tenant_id = resolution.tenant_id request.state.tenant_id = tenant_id request.state.tenant_source = resolution.source + request.state.tenant_vary = resolution.vary send = self._vary_sender(send, resolution.vary) if tenant_id is not None: @@ -131,9 +148,15 @@ def _vary_sender(send: Send, vary: tuple[str, ...]) -> Send: async def wrapped(message: MutableMapping[str, Any]) -> None: if message["type"] == "http.response.start": headers = MutableHeaders(scope=message) - merged = merge_vary(headers.get("vary"), vary) - if merged is not None: - headers["vary"] = merged + lines = headers.getlist("vary") + # Every line, not just the first (#423): MutableHeaders.get + # reads one, and assigning drops the rest — including a + # ``Vary: *`` on a later line. + if not any("*" in _tokens(line) for line in lines): + merged = merge_vary(", ".join(lines) or None, vary) + if merged is not None: + del headers["vary"] + headers["vary"] = merged await send(message) return wrapped @@ -151,7 +174,9 @@ async def _resolve(self, request: Request, scope: Scope) -> TenantResolution: user = getattr(request.state, "user", None) if user is not None: claim = getattr(user, "tenant_id", None) - return TenantResolution(claim, "claim" if claim is not None else None) + # The claim came from whichever credential authenticated the + # request, so the answer varies with it (#418). + return TenantResolution(claim, "claim" if claim is not None else None, _CREDENTIAL_VARY) if self.header: value = Headers(scope=scope).get(self.header) diff --git a/framework/hosting/tests/test_tenant_vary_merge.py b/framework/hosting/tests/test_tenant_vary_merge.py new file mode 100644 index 00000000..582f5c10 --- /dev/null +++ b/framework/hosting/tests/test_tenant_vary_merge.py @@ -0,0 +1,91 @@ +"""TenantMiddleware merges every Vary line (#423); no tenant bound means no source (#424).""" + +from __future__ import annotations + +from types import SimpleNamespace + +from simple_module_hosting._tenant import merge_vary +from simple_module_hosting.middleware import TenantMiddleware, TenantResolution + + +def _scope(resolver=None, user=None): + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [], + "state": {}, + "app": SimpleNamespace(state=SimpleNamespace(tenant_resolver=resolver)), + } + if user is not None: + scope["state"]["user"] = user + return scope + + +async def _receive(): # pragma: no cover + return {"type": "http.request", "body": b"", "more_body": False} + + +async def _run(scope, vary_lines: list[bytes]): + sent: list[dict] = [] + + async def inner(scope, receive, send): + await send( + { + "type": "http.response.start", + "status": 200, + "headers": [(b"vary", v) for v in vary_lines], + } + ) + + async def send(message): + sent.append(message) + + await TenantMiddleware(inner)(scope, _receive, send) + return scope["state"], [v.decode() for k, v in sent[0]["headers"] if k == b"vary"] + + +def _resolver(result): + async def resolve(_request): + return result + + return resolve + + +async def test_every_vary_line_is_merged_into_one(): + _, vary = await _run( + _scope(_resolver(TenantResolution("acme", "header", ("Host",)))), + [b"Accept", b"Accept-Language"], + ) + assert vary == ["Accept, Accept-Language, Host"] + + +async def test_a_wildcard_on_any_line_leaves_the_response_untouched(): + _, vary = await _run( + _scope(_resolver(TenantResolution("acme", "header", ("Host",)))), [b"Accept", b"*"] + ) + assert vary == ["Accept", "*"] + + +def test_existing_duplicates_are_collapsed_case_insensitively(): + assert merge_vary("Accept, accept", ("Host",)) == "Accept, Host" + + +async def test_no_tenant_means_no_source_but_vary_is_kept(): + state, vary = await _run( + _scope(_resolver(TenantResolution(None, "header", ("X-Tenant-ID",)))), [] + ) + assert state["tenant_id"] is None + assert state["tenant_source"] is None + assert vary == ["X-Tenant-ID"] + + +async def test_resolution_vary_is_recorded_on_request_state(): + state, _ = await _run(_scope(_resolver(TenantResolution("acme", "subdomain", ("Host",)))), []) + assert state["tenant_vary"] == ("Host",) + + +async def test_claim_source_varies_on_the_credential_headers(): + state, vary = await _run(_scope(user=SimpleNamespace(tenant_id="acme")), []) + assert state["tenant_source"] == "claim" + assert vary == ["Cookie, Authorization"] From 0213b1e713d7a3026df518b7387c71ce063d4da1 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 07:48:28 +0200 Subject: [PATCH 04/30] feat(hosting): publish tenancy mode, single-tenant id and require_tenant (#418) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- docs/framework/multi-tenancy.md | 53 ++++++ framework/core/simple_module_core/services.py | 4 + framework/core/simple_module_core/tenancy.py | 12 ++ .../simple_module_hosting/app_builder.py | 2 + .../hosting/simple_module_hosting/tenancy.py | 89 ++++++++++ framework/hosting/tests/test_tenancy_api.py | 156 ++++++++++++++++++ 6 files changed, 316 insertions(+) create mode 100644 framework/hosting/simple_module_hosting/tenancy.py create mode 100644 framework/hosting/tests/test_tenancy_api.py diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 1201123a..dd833d8d 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -201,6 +201,44 @@ Most auth providers set no `tenant_id` claim, so `multi_tenant` with no resolver fails every tenant-scoped query closed; the boot reports that as `SM025`. +## For module authors + +`simple_module_hosting.tenancy` answers the questions a module used to answer by +reading the middleware stack or hardcoding `"default"` (#418): + +| Name | What it gives you | +|---|---| +| `TenancyMode` | `SINGLE` (one tenant for every request: no `TenantMiddleware`, or one pinned by `default_tenant`) or `MULTI` (`multi_tenant` on, each request resolves its own). | +| `tenancy_mode(app)` | The host's mode, recorded on `app.state.sm.tenancy` where `create_app` decides on `TenantMiddleware`. | +| `single_tenant_id(app)` | The tenant a single-tenant host runs in: `default_tenant`, else `DEFAULT_TENANT_ID`. Use it instead of a literal `"default"`. | +| `require_tenant(*, on_missing=403, detail="tenant_required")` | A dependency that binds the request's tenant and yields it. | +| `tenant_vary(request)` | The request headers the resolved tenant depended on, for a route that sets its own cache headers. | + +Put `require_tenant()` on every router that reads or writes `MultiTenantMixin` +tables. On a single-tenant host it binds `single_tenant_id(app)`, so the route +works whether or not `TenantMiddleware` is installed; on a multi-tenant host it +binds what the middleware resolved and refuses the request when nothing was: + +```python +from fastapi import APIRouter, Depends, HTTPException +from simple_module_hosting.tenancy import require_tenant + +# Admin surface: no tenant is a 403 {"detail": "tenant_required"}. +admin = APIRouter(dependencies=[Depends(require_tenant())]) + +# Public surface: a page that does not exist for this visitor is a 404. +public = APIRouter(dependencies=[Depends(require_tenant(on_missing=lambda _r: HTTPException(404)))]) +``` + +`on_missing` is a status code, or a callable taking the request and returning +the exception to raise. + +**Order matters.** List `require_tenant()` *before* `get_db`. Yield +dependencies exit in reverse order, and the session's commit has to run while +the tenant is still bound. A router-level dependency, as above, always runs +before the route's own parameters, so `db: RequestSession = Depends(get_db)` on +the endpoint is safe. + ## Tenant roles A membership role — `owner`, `admin` or `member` — reaches the request @@ -308,6 +346,21 @@ alone). The `tenants` resolver reports `Host` when subdomains are enabled and the tenant header whenever it is configured, even if the answer was `None`, so a shared cache cannot serve one tenant's response to another. +The rules for the merged header: + +- Every `Vary` line already on the response is merged into one, and a `*` in + any of them is never dropped. +- `tenant_source` is `None` whenever no tenant is bound, whatever the resolver + reported as its source. +- A tenant taken from the principal's `tenant_id` claim varies on + `Cookie, Authorization`: the claim came from whichever credential + authenticated the request. +- The `tenants` resolver adds `Cookie` when the tenant came from the session, + since the session cookie carries that choice. + +A route that builds its own cache headers reads the same list with +`tenant_vary(request)` (see [For module authors](#for-module-authors)). + ## Unique keys On a tenant-scoped table every business key is per tenant: put `tenant_id` in diff --git a/framework/core/simple_module_core/services.py b/framework/core/simple_module_core/services.py index ea29957a..b939a1af 100644 --- a/framework/core/simple_module_core/services.py +++ b/framework/core/simple_module_core/services.py @@ -17,6 +17,7 @@ from typing import TYPE_CHECKING from simple_module_core.invalidation import InvalidationBus +from simple_module_core.tenancy import TenancyMode if TYPE_CHECKING: from collections.abc import Callable @@ -100,3 +101,6 @@ class Services: #: Defaulted so an app built outside ``create_app`` still has a holder to #: read; that one simply reports the checks as unsupported. diagnostics: DiagnosticsState = field(default_factory=DiagnosticsState) + #: How the host scopes requests (#418), recorded where ``create_app`` + #: decides on ``TenantMiddleware``. Defaulted for apps built outside it. + tenancy: TenancyMode = TenancyMode.SINGLE diff --git a/framework/core/simple_module_core/tenancy.py b/framework/core/simple_module_core/tenancy.py index 00a79424..4ed1a8df 100644 --- a/framework/core/simple_module_core/tenancy.py +++ b/framework/core/simple_module_core/tenancy.py @@ -28,6 +28,18 @@ class TenantRole(StrEnum): MEMBER = "member" +class TenancyMode(StrEnum): + """Whether a host scopes requests to tenants (#418). + + ``SINGLE``: one tenant for every request: no ``TenantMiddleware``, or one + pinned with ``fixed=``. ``MULTI``: ``multi_tenant`` is on and each request + resolves its own. + """ + + SINGLE = "single" + MULTI = "multi" + + def tenant_role(name: str) -> str: """The effective principal role for membership role ``name``: ``tenant:``. diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 15b83d10..59daa7a2 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -48,6 +48,7 @@ from simple_module_hosting.setup_gate import register_migration_step from simple_module_hosting.setup_wizard import mount_setup_wizard from simple_module_hosting.static_files import PrecompressedStaticFiles +from simple_module_hosting.tenancy import mode_for logger = logging.getLogger(__name__) @@ -262,6 +263,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: modules=tuple(modules), invalidation=invalidation_bus, diagnostics=diagnostics_state, + tenancy=mode_for(settings), ) return app diff --git a/framework/hosting/simple_module_hosting/tenancy.py b/framework/hosting/simple_module_hosting/tenancy.py new file mode 100644 index 00000000..611a9cec --- /dev/null +++ b/framework/hosting/simple_module_hosting/tenancy.py @@ -0,0 +1,89 @@ +"""The host's tenancy, for modules, without reading the middleware stack (#418). + +Use ``require_tenant()`` as a router dependency on any surface that reads or +writes ``MultiTenantMixin`` tables. List it **before** ``get_db``: yield +dependencies exit in reverse order, and the session's commit has to run while +the tenant is still bound. +""" + +from __future__ import annotations + +from collections.abc import AsyncIterator, Callable +from typing import Any + +from fastapi import HTTPException, Request +from simple_module_core.tenancy import TenancyMode +from simple_module_db import DEFAULT_TENANT_ID, is_valid_tenant_id, tenant_context + +__all__ = [ + "TenancyMode", + "mode_for", + "require_tenant", + "single_tenant_id", + "tenancy_mode", + "tenant_vary", +] + + +def mode_for(settings: Any) -> TenancyMode: + """The mode ``create_app`` builds for ``settings`` — the one place it is decided.""" + return TenancyMode.MULTI if getattr(settings, "multi_tenant", False) else TenancyMode.SINGLE + + +def tenancy_mode(app: Any) -> TenancyMode: + """Whether ``app`` scopes requests to tenants. + + ``SINGLE`` for an app built outside ``create_app``. + """ + sm = getattr(app.state, "sm", None) + return getattr(sm, "tenancy", TenancyMode.SINGLE) + + +def single_tenant_id(app: Any) -> str: + """The tenant every request of a single-tenant host runs in. + + ``default_tenant`` when set, else ``DEFAULT_TENANT_ID`` (``"default"``). + """ + sm = getattr(app.state, "sm", None) + db = getattr(sm, "db", None) + return getattr(db, "default_tenant_id", None) or DEFAULT_TENANT_ID + + +def tenant_vary(request: Request) -> tuple[str, ...]: + """Request headers the resolved tenant depended on. + + For routes that set their own cache headers; ``()`` when none were recorded. + """ + return tuple(getattr(request.state, "tenant_vary", ()) or ()) + + +def require_tenant( + *, + on_missing: int | Callable[[Request], Exception] = 403, + detail: str = "tenant_required", +) -> Callable[[Request], AsyncIterator[str]]: + """FastAPI yield-dependency that binds the request's tenant for the rest of the request. + + On a single-tenant host it binds :func:`single_tenant_id`, so the route + works with or without ``TenantMiddleware``. On a multi-tenant host it binds + what the middleware resolved, and with nothing resolved raises + ``HTTPException(on_missing, detail)`` — or ``on_missing(request)`` when that + is a callable, for a surface that should answer, say, 404 instead. + + It is an *async* generator on purpose: a sync generator dependency runs in + a threadpool, and the contextvar it set would never reach the endpoint. + """ + + async def dependency(request: Request) -> AsyncIterator[str]: + if tenancy_mode(request.app) is TenancyMode.SINGLE: + tenant = single_tenant_id(request.app) + else: + tenant = getattr(request.state, "tenant_id", None) + if tenant is None or not is_valid_tenant_id(tenant): + if isinstance(on_missing, int): + raise HTTPException(status_code=on_missing, detail=detail) + raise on_missing(request) + with tenant_context(tenant): + yield tenant + + return dependency diff --git a/framework/hosting/tests/test_tenancy_api.py b/framework/hosting/tests/test_tenancy_api.py new file mode 100644 index 00000000..495c465f --- /dev/null +++ b/framework/hosting/tests/test_tenancy_api.py @@ -0,0 +1,156 @@ +"""simple_module_hosting.tenancy — the tenancy facts modules used to reverse-engineer (#418).""" + +from __future__ import annotations + +import httpx +import pytest +from fastapi import APIRouter, Depends, HTTPException, Request +from simple_module_db import DEFAULT_TENANT_ID, all_tenants, current_tenant_id +from simple_module_db.base import create_module_base +from simple_module_db.deps import get_db +from simple_module_db.mixins import MultiTenantMixin +from simple_module_hosting.tenancy import ( + TenancyMode, + require_tenant, + single_tenant_id, + tenancy_mode, + tenant_vary, +) +from simple_module_test.fixtures import _build_app +from sqlmodel import Field, select + +_Base = create_module_base("tenancy_api_test") + + +class _Note(_Base, MultiTenantMixin, table=True): # ty: ignore[unsupported-base] + __tablename__ = "tenancy_api_test_note" + id: int | None = Field(default=None, primary_key=True) + body: str = Field(max_length=100) + + +@pytest.fixture +async def single_app(settings): + single = settings.model_copy(update={"multi_tenant": False, "default_tenant": ""}) + async for application in _build_app(single, seed_admin=True): + yield application + + +@pytest.fixture +async def pinned_app(settings): + pinned = settings.model_copy(update={"multi_tenant": False, "default_tenant": "main"}) + async for application in _build_app(pinned, seed_admin=True): + yield application + + +async def _client(app): + return httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://t") + + +async def test_multi_tenant_host_reports_multi(app): + assert tenancy_mode(app) is TenancyMode.MULTI + + +async def test_single_host_reports_single_and_default_id(single_app): + assert tenancy_mode(single_app) is TenancyMode.SINGLE + assert single_tenant_id(single_app) == DEFAULT_TENANT_ID + + +async def test_pinned_single_host_reports_its_tenant(pinned_app): + assert tenancy_mode(pinned_app) is TenancyMode.SINGLE + assert single_tenant_id(pinned_app) == "main" + + +async def test_require_tenant_binds_single_tenant_without_middleware(single_app): + router = APIRouter(dependencies=[Depends(require_tenant())]) + + @router.get("/api/probe-tenant") + async def probe() -> dict: + return {"tenant": current_tenant_id.get()} + + single_app.include_router(router) + single_app.state.public_routes.add_prefix("/api/probe-tenant", methods={"GET"}) + async with await _client(single_app) as c: + assert (await c.get("/api/probe-tenant")).json() == {"tenant": DEFAULT_TENANT_ID} + + +async def test_require_tenant_403_when_multi_and_unresolved(app): + router = APIRouter(dependencies=[Depends(require_tenant())]) + + @router.get("/api/probe-tenant") + async def probe() -> dict: + return {} + + app.include_router(router) + app.state.public_routes.add_prefix("/api/probe-tenant", methods={"GET"}) + async with await _client(app) as c: + resp = await c.get("/api/probe-tenant") + assert resp.status_code == 403 + assert resp.json()["detail"] == "tenant_required" + + +async def test_require_tenant_binds_the_resolved_tenant_when_multi(app, monkeypatch): + async def resolver(_request: Request) -> str: + return "acme" + + monkeypatch.setattr(app.state, "tenant_resolver", resolver, raising=False) + router = APIRouter(dependencies=[Depends(require_tenant())]) + + @router.get("/api/probe-tenant") + async def probe() -> dict: + return {"tenant": current_tenant_id.get()} + + app.include_router(router) + app.state.public_routes.add_prefix("/api/probe-tenant", methods={"GET"}) + async with await _client(app) as c: + resp = await c.get("/api/probe-tenant") + assert resp.json() == {"tenant": "acme"} + + +async def test_require_tenant_custom_missing_response(app): + dep = require_tenant(on_missing=lambda _r: HTTPException(404, "Page not found")) + router = APIRouter(dependencies=[Depends(dep)]) + + @router.get("/api/probe-tenant") + async def probe() -> dict: + return {} + + app.include_router(router) + app.state.public_routes.add_prefix("/api/probe-tenant", methods={"GET"}) + async with await _client(app) as c: + assert (await c.get("/api/probe-tenant")).status_code == 404 + + +async def test_require_tenant_stamps_writes_single_host(single_app): + """Review focus 2: a write after require_tenant() commits under the bound tenant.""" + async with single_app.state.sm.db.engine.begin() as conn: + await conn.run_sync(_Base.metadata.create_all) + + # Router-level dependency: require_tenant() is entered before the route's + # get_db, so it exits after it — the commit runs while the tenant is bound. + router = APIRouter(dependencies=[Depends(require_tenant())]) + + @router.post("/api/probe-notes") + async def create_note(db=Depends(get_db)) -> dict: + note = _Note(body="hello") + db.add(note) + await db.flush() + return {"id": note.id, "bound": current_tenant_id.get()} + + single_app.include_router(router) + single_app.state.public_routes.add_prefix("/api/probe-notes", methods={"POST"}) + async with await _client(single_app) as c: + resp = await c.post("/api/probe-notes") + assert resp.status_code == 200, resp.text + assert resp.json()["bound"] == DEFAULT_TENANT_ID + + # A fresh session proves the row was committed, not just flushed. + async with single_app.state.sm.db.session_factory() as session: + with all_tenants(): + rows = (await session.execute(select(_Note))).scalars().all() + assert [(r.body, r.tenant_id) for r in rows] == [("hello", DEFAULT_TENANT_ID)] + + +async def test_tenant_vary_reads_what_the_middleware_recorded(): + request = Request({"type": "http", "headers": [], "state": {"tenant_vary": ("Host",)}}) + assert tenant_vary(request) == ("Host",) + assert tenant_vary(Request({"type": "http", "headers": [], "state": {}})) == () From 044a35e532f07fc171f4465a29d64ace025ac0dc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 07:52:47 +0200 Subject: [PATCH 05/30] test(hosting): pin require_tenant commit-time binding (#418) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/hosting/tests/test_tenancy_api.py | 62 +++++++++++++++------ 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/framework/hosting/tests/test_tenancy_api.py b/framework/hosting/tests/test_tenancy_api.py index 495c465f..fb20a385 100644 --- a/framework/hosting/tests/test_tenancy_api.py +++ b/framework/hosting/tests/test_tenancy_api.py @@ -120,34 +120,62 @@ async def probe() -> dict: assert (await c.get("/api/probe-tenant")).status_code == 404 -async def test_require_tenant_stamps_writes_single_host(single_app): - """Review focus 2: a write after require_tenant() commits under the bound tenant.""" - async with single_app.state.sm.db.engine.begin() as conn: +async def _post_unflushed_note(app, *dependencies) -> list[tuple[str, str | None]]: + """POST a row the endpoint only ``add``s, then read back what was committed. + + No flush in the endpoint: the row is stamped when the request's session + commits (``CommitBeforeResponseMiddleware``), so the tenant ``require_tenant`` + bound has to still be bound at that point, not just inside the handler. + """ + async with app.state.sm.db.engine.begin() as conn: await conn.run_sync(_Base.metadata.create_all) - # Router-level dependency: require_tenant() is entered before the route's - # get_db, so it exits after it — the commit runs while the tenant is bound. - router = APIRouter(dependencies=[Depends(require_tenant())]) + # Router-level dependencies run before the route's own get_db. + router = APIRouter(dependencies=[*dependencies, Depends(require_tenant())]) @router.post("/api/probe-notes") async def create_note(db=Depends(get_db)) -> dict: - note = _Note(body="hello") - db.add(note) - await db.flush() - return {"id": note.id, "bound": current_tenant_id.get()} + db.add(_Note(body="hello")) + return {"bound": current_tenant_id.get()} - single_app.include_router(router) - single_app.state.public_routes.add_prefix("/api/probe-notes", methods={"POST"}) - async with await _client(single_app) as c: + app.include_router(router) + app.state.public_routes.add_prefix("/api/probe-notes", methods={"POST"}) + async with await _client(app) as c: resp = await c.post("/api/probe-notes") assert resp.status_code == 200, resp.text - assert resp.json()["bound"] == DEFAULT_TENANT_ID - # A fresh session proves the row was committed, not just flushed. - async with single_app.state.sm.db.session_factory() as session: + # A fresh session proves the row was committed, not just added. + async with app.state.sm.db.session_factory() as session: with all_tenants(): rows = (await session.execute(select(_Note))).scalars().all() - assert [(r.body, r.tenant_id) for r in rows] == [("hello", DEFAULT_TENANT_ID)] + return [(r.body, r.tenant_id) for r in rows] + + +async def test_require_tenant_stamps_writes_single_host(single_app): + """Review focus 2: a write after require_tenant() commits under the bound tenant.""" + assert await _post_unflushed_note(single_app) == [("hello", DEFAULT_TENANT_ID)] + + +async def test_require_tenant_binding_outlives_the_commit_in_strict_mode(app, monkeypatch): + """Review focus 2, pinned: only require_tenant() binds, and strict mode is on. + + The resolver answers ``None``, so ``TenantMiddleware`` binds nothing; a + dependency ahead of ``require_tenant()`` records ``acme`` on + ``request.state`` the way the middleware would. If the binding were gone + by the time the session commits, the strict-mode flush would raise instead + of stamping ``acme``. + """ + + async def no_tenant(_request: Request) -> None: + return None + + async def resolved_elsewhere(request: Request) -> None: + request.state.tenant_id = "acme" + + monkeypatch.setattr(app.state, "tenant_resolver", no_tenant, raising=False) + assert app.state.sm.db.tenant_strict + rows = await _post_unflushed_note(app, Depends(resolved_elsewhere)) + assert rows == [("hello", "acme")] async def test_tenant_vary_reads_what_the_middleware_recorded(): From 6b11a995e92a8a756b870d3f55678891e30975d6 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 07:55:02 +0200 Subject: [PATCH 06/30] fix(tenants): vary on Cookie when the tenant comes from the session (#418) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- modules/tenants/tenants/resolver.py | 4 ++++ modules/tenants/tests/test_resolver_source.py | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/modules/tenants/tenants/resolver.py b/modules/tenants/tenants/resolver.py index 9803e746..bf18142e 100644 --- a/modules/tenants/tenants/resolver.py +++ b/modules/tenants/tenants/resolver.py @@ -170,6 +170,10 @@ async def _resolve(request: Request) -> tuple[str | None, str | None, tuple[str, return None, None, vary return _enter(request, user, active), "header", vary + # From here the answer depends on the session cookie (the stored + # preference) — say so, or a shared cache serves one member's tenant to + # another on the same URL (#418). + vary = (*vary, "Cookie") session = request.scope.get("session") preferred = session.get(SESSION_ACTIVE_TENANT) if session is not None else None active = pick_active(memberships, preferred) diff --git a/modules/tenants/tests/test_resolver_source.py b/modules/tenants/tests/test_resolver_source.py index 9500a7e3..55d3925e 100644 --- a/modules/tenants/tests/test_resolver_source.py +++ b/modules/tenants/tests/test_resolver_source.py @@ -75,3 +75,12 @@ async def test_vary_has_no_duplicates(probe, user_client): resp = await a.get("/probe", headers={"X-Tenant-ID": "nope"}) tokens = _vary(resp) assert len(tokens) == len(set(tokens)) + + +async def test_session_source_varies_on_cookie(probe, user_client): + """The tenant came from the session cookie, so a shared cache must key on it (#418).""" + async with user_client("a@x.io") as (a, _): + await _create(a, "One") + resp = await a.get("/probe") + assert resp.json()["source"] == "session" + assert "cookie" in _vary(resp) From 32959532f79b2d7dfe3824959399950dd390fe83 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:04:00 +0200 Subject: [PATCH 07/30] fix(db): keep ORM outer joins outer under the tenant filter (#417) A child column used only inside a function (func.count(Child.id)) loses its ORM annotation, so _plain_tables took the outer-joined child for a Core table and added WHERE child.tenant_id = :t, turning the LEFT JOIN into an inner join. Exclude a columns-clause table from the WHERE candidates only when it is the target of an outer/full join in _setup_joins whose target is a plain ORM entity carrying loader criteria (they already sit in ON). Raw Model.__table__ targets keep the WHERE predicate; strict mode without a tenant still refuses. Pins the pre-existing Core outer join leak as a strict xfail. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/_outer_joins.py | 61 +++++ framework/db/simple_module_db/query_filter.py | 29 ++- .../db/tests/test_query_filter_outer_joins.py | 214 ++++++++++++++++++ 3 files changed, 298 insertions(+), 6 deletions(-) create mode 100644 framework/db/simple_module_db/_outer_joins.py create mode 100644 framework/db/tests/test_query_filter_outer_joins.py diff --git a/framework/db/simple_module_db/_outer_joins.py b/framework/db/simple_module_db/_outer_joins.py new file mode 100644 index 00000000..b37fd2b1 --- /dev/null +++ b/framework/db/simple_module_db/_outer_joins.py @@ -0,0 +1,61 @@ +"""Tables an ORM entity outer-joins in, whose criteria already sit in ``ON`` (#417). + +``select(Parent, func.count(Child.id)).outerjoin(Child, ...)``: the column +wrapped in ``func.count`` strips the ORM annotation, so ``columns_clause_froms`` +lists a bare ``Table child``. ``_plain_tables`` took that for a Core +``Model.__table__`` reference and added ``WHERE child.tenant_id = :t``, which +is false for every row the outer join padded with NULLs — the LEFT JOIN became +an inner one and parents without children vanished. + +Excluding such a table from the ``WHERE`` is only safe when something else +filters it. That holds for exactly one shape: the join target is a plain ORM +entity (``.outerjoin(Child, ...)``) whose class carries the loader criteria, +because ``with_loader_criteria`` renders them into the join's ``ON``. So: + +* a raw ``Model.__table__`` target gets no loader criteria and keeps its + ``WHERE`` predicate (over-filters, never leaks); +* an ``aliased(Model)`` target is left alone: its name never matches a table + name, so it was never a ``WHERE`` candidate in the first place; +* a target whose mapper does not map exactly one table (joined inheritance) + is left alone, conservatively; +* a target whose class is not in ``covered`` (the classes the caller attaches + loader criteria for) is left alone — no criteria, no ``ON`` predicate. +""" + +from __future__ import annotations + +from typing import Any + +from sqlalchemy.orm import Mapper + + +def _plain(element: Any) -> Any: + deannotate = getattr(element, "_deannotate", None) + return deannotate() if deannotate is not None else element + + +def orm_outer_join_tables(stmt: Any, covered: set[type] | frozenset[type]) -> list[Any]: + """Un-annotated tables of ORM entities ``stmt`` outer- or full-joins in. + + Only entities whose class is in ``covered`` count: those are the ones the + filter gives ``with_loader_criteria``, which is what puts their predicate + in ``ON``. Compare the result by identity against FROM candidates. + """ + tables: list[Any] = [] + for entry in getattr(stmt, "_setup_joins", ()): + # SQLAlchemy 2.0: (target, onclause, from_, {"isouter": .., "full": ..}) + target, flags = entry[0], entry[-1] + if not isinstance(flags, dict) or not (flags.get("isouter") or flags.get("full")): + continue + mapper = getattr(target, "_annotations", {}).get("parententity") + if not isinstance(mapper, Mapper) or mapper.class_ not in covered: + continue + table = _plain(target) + if table is mapper.local_table and mapper.persist_selectable is mapper.local_table: + tables.append(table) + return tables + + +def is_excluded(from_obj: Any, excluded: list[Any]) -> bool: + plain = _plain(from_obj) + return any(plain is t for t in excluded) diff --git a/framework/db/simple_module_db/query_filter.py b/framework/db/simple_module_db/query_filter.py index ef6d0004..8efdd0b3 100644 --- a/framework/db/simple_module_db/query_filter.py +++ b/framework/db/simple_module_db/query_filter.py @@ -19,6 +19,7 @@ from sqlalchemy.sql.selectable import Alias, Join from simple_module_db import model_registry as registry +from simple_module_db._outer_joins import is_excluded, orm_outer_join_tables from simple_module_db.insert_guard import assigned_tenant_ids, guard_insert from simple_module_db.subquery_guard import scope_exists_subqueries from simple_module_db.tenancy import ( @@ -136,7 +137,10 @@ def filter_statements(execute_state: ORMExecuteState) -> Any: def _soft_delete_criteria(execute_state: ORMExecuteState) -> list[Any]: - for table in _plain_tables(execute_state, registry.soft_delete_table_names): + plain = _plain_tables( + execute_state, registry.soft_delete_table_names, registry.soft_delete_classes + ) + for table in plain: execute_state.statement = execute_state.statement.where(table.c.is_deleted.is_(False)) return [ with_loader_criteria(c, c.is_deleted.is_(False), include_aliases=True) @@ -167,18 +171,28 @@ def predicates_for(from_obj: Any) -> list[Any]: return predicates_for -def _plain_tables(execute_state: ORMExecuteState, names: set[str]) -> list[Any]: +def _plain_tables( + execute_state: ORMExecuteState, names: set[str], covered: set[type] | None = None +) -> list[Any]: """Tables a statement uses as plain Core tables (``Model.__table__``). Only the top level: the FROM list of a select (including the sides of a join), the target of an update/delete. ORM-annotated occurrences are covered by loader criteria. + + ``covered`` names the classes the caller gives loader criteria. A bare + columns-clause table that one of them outer-joins in (``func.count( + Child.id)`` strips the annotation) is that join's own ``FROM``, already + filtered in ``ON``; a ``WHERE`` on it would make the join inner (#417). """ stmt = execute_state.statement - if execute_state.is_select: - froms = [*getattr(stmt, "columns_clause_froms", ()), *getattr(stmt, "_from_obj", ())] - else: + if not execute_state.is_select: froms = [getattr(stmt, "table", None)] + return [f for f in _where_able(froms) if registry.is_plain_table(f, names)] + columns = getattr(stmt, "columns_clause_froms", ()) + outer = orm_outer_join_tables(stmt, covered) if covered else [] + columns = [f for f in columns if not (outer and is_excluded(f, outer))] + froms = [*columns, *getattr(stmt, "_from_obj", ())] return [f for f in _where_able(froms) if registry.is_plain_table(f, names)] @@ -203,7 +217,10 @@ def _where_able(froms: list[Any]) -> list[Any]: def _tenant_criteria( execute_state: ORMExecuteState, named: list[type], tenant_id: str | None, strict: bool ) -> list[Any]: - core = _plain_tables(execute_state, registry.tenant_table_names) + # The ON-covered exclusion only applies once a tenant is bound: strict mode + # without one still refuses a table it would otherwise have WHERE-filtered. + covered = registry.tenant_classes if tenant_id is not None else None + core = _plain_tables(execute_state, registry.tenant_table_names, covered) op = ( "SELECT" if execute_state.is_select else ("UPDATE" if execute_state.is_update else "DELETE") ) diff --git a/framework/db/tests/test_query_filter_outer_joins.py b/framework/db/tests/test_query_filter_outer_joins.py new file mode 100644 index 00000000..0e0aeaab --- /dev/null +++ b/framework/db/tests/test_query_filter_outer_joins.py @@ -0,0 +1,214 @@ +"""The tenant / soft-delete filter keeps ORM outer joins outer (GH #417). + +A column of the outer-joined table that only appears inside a function +(``func.count(Child.id)``) loses its ORM annotation, so the child looked like +a raw Core table and got a ``WHERE child.tenant_id = :t`` on top of the +``ON`` criteria — turning the LEFT JOIN into an inner one and dropping every +parent without children. + +Tenant B's links point at tenant A's ``used`` tag: if the child's predicate +ever went missing from both ``ON`` and ``WHERE``, A's count would read 4. +""" + +from __future__ import annotations + +from collections.abc import AsyncGenerator + +import pytest +from simple_module_db.base import create_module_base +from simple_module_db.listeners import current_tenant_id, register_listeners +from simple_module_db.mixins import MultiTenantMixin, SoftDeleteMixin +from simple_module_db.session import init_db +from sqlalchemy import func, select +from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.pool import StaticPool +from sqlmodel import Field + +_TagBase = create_module_base("qfoj_tag") +_LinkBase = create_module_base("qfoj_link") +_SoftBase = create_module_base("qfoj_soft") + + +class _Tag(_TagBase, MultiTenantMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "qfoj_tag_tag" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(default="", max_length=100) + + +class _TagLink(_LinkBase, MultiTenantMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "qfoj_link_tag_link" + id: int | None = Field(default=None, primary_key=True) + tag_id: int | None = None # no FK: each model has its own MetaData + article_id: int = 0 + + +class _SoftLink(_SoftBase, SoftDeleteMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + __tablename__ = "qfoj_soft_soft_link" + id: int | None = Field(default=None, primary_key=True) + tag_id: int | None = None # no FK: each model has its own MetaData + + +@pytest.fixture +async def seeded() -> AsyncGenerator[AsyncSession, None]: + db_state = init_db("sqlite+aiosqlite:///:memory:", poolclass=StaticPool) + try: + register_listeners(db_state) + async with db_state.engine.begin() as conn: + for base in (_TagBase, _LinkBase, _SoftBase): + await conn.run_sync(base.metadata.create_all) + async with db_state.session_factory() as session: + token = current_tenant_id.set("A") + used, unused = _Tag(name="used"), _Tag(name="unused") + session.add_all([used, unused]) + await session.flush() + session.add(_TagLink(tag_id=used.id, article_id=1)) + session.add_all([_SoftLink(tag_id=used.id), _SoftLink(tag_id=used.id, is_deleted=True)]) + await session.flush() + current_tenant_id.reset(token) + token = current_tenant_id.set("B") + session.add(_Tag(name="b-tag")) + # The leak probe: tenant B's links aimed at tenant A's tag. + session.add_all([_TagLink(tag_id=used.id, article_id=n) for n in (7, 8, 9)]) + await session.flush() + current_tenant_id.reset(token) + await session.commit() + async with db_state.session_factory() as session: + token = current_tenant_id.set("A") + try: + yield session + finally: + current_tenant_id.reset(token) + finally: + await db_state.engine.dispose() + + +COUNT_SHAPES = { + "entity+count": lambda: ( + select(_Tag, func.count(_TagLink.article_id)) + .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.id) + ), + "columns+count, select_from": lambda: ( + select(_Tag.name, func.count(_TagLink.article_id)) + .select_from(_Tag) + .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ), + "entity+entity": lambda: select(_Tag, _TagLink).outerjoin(_TagLink, _TagLink.tag_id == _Tag.id), + "entity+column": lambda: select(_Tag, _TagLink.article_id).outerjoin( + _TagLink, _TagLink.tag_id == _Tag.id + ), +} + + +@pytest.mark.parametrize("shape", list(COUNT_SHAPES)) +async def test_outer_join_keeps_the_unused_parent(seeded, shape): + rows = (await seeded.execute(COUNT_SHAPES[shape]())).all() + names = {r[0] if isinstance(r[0], str) else r[0].name for r in rows} + assert names == {"used", "unused"} + + +async def test_outer_join_count_never_sees_other_tenants(seeded): + """Review focus 1: tenant B's links point at A's tag; A's count stays 1.""" + rows = dict((await seeded.execute(COUNT_SHAPES["columns+count, select_from"]())).all()) + assert rows == {"used": 1, "unused": 0} + + +async def test_outer_join_entity_count_never_sees_other_tenants(seeded): + stmt = COUNT_SHAPES["entity+count"]() + rows = {t.name: n for t, n in (await seeded.execute(stmt)).all()} + assert rows == {"used": 1, "unused": 0} + + +async def test_outer_join_from_keeps_the_parent_and_isolates(seeded): + """``outerjoin_from`` records its target in ``_setup_joins`` the same way.""" + stmt = ( + select(_Tag.name, func.count(_TagLink.article_id)) + .outerjoin_from(_Tag, _TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ) + assert dict((await seeded.execute(stmt)).all()) == {"used": 1, "unused": 0} + + +async def test_inner_join_still_filters_the_child(seeded): + stmt = ( + select(_Tag.name, func.count(_TagLink.article_id)) + .join(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ) + assert dict((await seeded.execute(stmt)).all()) == {"used": 1} + + +async def test_raw_table_outer_join_stays_filtered(seeded): + """A Core table target gets no ON criteria, so it keeps the WHERE predicate (no leak).""" + link = _TagLink.__table__ + stmt = ( + select(_Tag.name, func.count(link.c.article_id)) + .select_from(_Tag) + .outerjoin(link, link.c.tag_id == _Tag.id) + .group_by(_Tag.name) + ) + rows = dict((await seeded.execute(stmt)).all()) + assert rows.get("used") == 1 # never 4 (tenant B's links) + + +async def test_raw_column_of_orm_outer_target_never_sees_other_tenants(seeded): + """``Model.__table__.c.x`` beside an ORM outer join is that join's own FROM. + + The un-annotated reference renders as the same ``FROM`` occurrence the + ORM join put its ``ON`` criteria on, so dropping its ``WHERE`` cannot let + tenant B's links through. + """ + link = _TagLink.__table__ + stmt = ( + select(_Tag.name, link.c.article_id) + .select_from(_Tag) + .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + ) + assert sorted((await seeded.execute(stmt)).all()) == [("unused", None), ("used", 1)] + + +async def test_soft_deleted_outer_child_keeps_the_parent(seeded): + stmt = ( + select(_Tag.name, func.count(_SoftLink.id)) + .select_from(_Tag) + .outerjoin(_SoftLink, _SoftLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ) + assert dict((await seeded.execute(stmt)).all()) == {"used": 1, "unused": 0} + + +@pytest.mark.xfail(strict=True, reason="pre-existing: Core outer join right side unfiltered") +async def test_core_outer_join_right_side_is_tenant_filtered(seeded): + """A pure Core ``tag.outerjoin(link)`` gets no ``ON`` criteria and no ``WHERE``. + + ``_where_able`` keeps the nullable side out of ``WHERE`` (correctly), but + nothing else filters a Core table, so ``count(*)`` counts tenant B's links. + """ + tag, link = _Tag.__table__, _TagLink.__table__ + stmt = ( + select(tag.c.name, func.count()) + .select_from(tag.outerjoin(link, link.c.tag_id == tag.c.id)) + .group_by(tag.c.name) + ) + assert dict((await seeded.execute(stmt)).all()) == {"used": 1, "unused": 1} + + +async def test_strict_mode_without_tenant_still_refuses_an_outer_child(seeded): + """The ``ON``-covered exclusion needs a bound tenant; strict mode keeps refusing.""" + from simple_module_db.query_filter import EngineTenancy, bind_engine_policy + from simple_module_db.tenancy import TenantIsolationError + + bind_engine_policy(seeded.bind.sync_engine, EngineTenancy(tenant_strict=True)) + current_tenant_id.set(None) + # Neither selected entity is tenant-scoped (a raw column names no mapper), + # so only the child's would-be WHERE predicate makes this a refusal + # rather than a silent empty join. + link = _TagLink.__table__ + stmt = ( + select(_SoftLink.id, link.c.article_id) + .select_from(_SoftLink) + .outerjoin(_TagLink, _TagLink.tag_id == _SoftLink.tag_id) + ) + with pytest.raises(TenantIsolationError): + await seeded.execute(stmt) From 2c7420cc9c35ac168ca194e4753d3221f66d7840 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:12:29 +0200 Subject: [PATCH 08/30] fix(db): never exempt FULL outer joins from the tenant WHERE (#417) A FULL join preserves its right side, and an ON predicate never removes a preserved row: with the child's WHERE gone, other tenants' (and trashed) child rows came back as unmatched rows. Exempt LEFT outer joins only, and pin it with FULL-join tests over a non-tenant left model. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- framework/db/simple_module_db/_outer_joins.py | 22 ++++++++--- .../db/tests/test_query_filter_outer_joins.py | 38 ++++++++++++++++++- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/framework/db/simple_module_db/_outer_joins.py b/framework/db/simple_module_db/_outer_joins.py index b37fd2b1..f7da846a 100644 --- a/framework/db/simple_module_db/_outer_joins.py +++ b/framework/db/simple_module_db/_outer_joins.py @@ -1,4 +1,4 @@ -"""Tables an ORM entity outer-joins in, whose criteria already sit in ``ON`` (#417). +"""Tables an ORM entity LEFT-joins in, whose criteria already sit in ``ON`` (#417). ``select(Parent, func.count(Child.id)).outerjoin(Child, ...)``: the column wrapped in ``func.count`` strips the ORM annotation, so ``columns_clause_froms`` @@ -8,9 +8,16 @@ an inner one and parents without children vanished. Excluding such a table from the ``WHERE`` is only safe when something else -filters it. That holds for exactly one shape: the join target is a plain ORM -entity (``.outerjoin(Child, ...)``) whose class carries the loader criteria, -because ``with_loader_criteria`` renders them into the join's ``ON``. So: +filters it. That holds for exactly one shape: the target of a LEFT outer join +is a plain ORM entity (``.outerjoin(Child, ...)``) whose class carries the +loader criteria, because ``with_loader_criteria`` renders them into the join's +``ON``, and an ``ON`` predicate fully filters a LEFT join's nullable side. So: + +* a FULL outer join (``full=True``) is deliberately NOT exempted: it preserves + its right side too, and an ``ON`` predicate never removes a preserved row — + another tenant's (or a trashed) child that fails ``ON`` still comes back as + an unmatched row. Only the ``WHERE`` predicate keeps it out (over-filters, + never leaks); * a raw ``Model.__table__`` target gets no loader criteria and keeps its ``WHERE`` predicate (over-filters, never leaks); @@ -35,7 +42,7 @@ def _plain(element: Any) -> Any: def orm_outer_join_tables(stmt: Any, covered: set[type] | frozenset[type]) -> list[Any]: - """Un-annotated tables of ORM entities ``stmt`` outer- or full-joins in. + """Un-annotated tables of ORM entities ``stmt`` LEFT-outer-joins in (never FULL). Only entities whose class is in ``covered`` count: those are the ones the filter gives ``with_loader_criteria``, which is what puts their predicate @@ -45,7 +52,8 @@ def orm_outer_join_tables(stmt: Any, covered: set[type] | frozenset[type]) -> li for entry in getattr(stmt, "_setup_joins", ()): # SQLAlchemy 2.0: (target, onclause, from_, {"isouter": .., "full": ..}) target, flags = entry[0], entry[-1] - if not isinstance(flags, dict) or not (flags.get("isouter") or flags.get("full")): + # LEFT only: ON cannot filter a FULL join's preserved right side. + if not isinstance(flags, dict) or not flags.get("isouter") or flags.get("full"): continue mapper = getattr(target, "_annotations", {}).get("parententity") if not isinstance(mapper, Mapper) or mapper.class_ not in covered: @@ -57,5 +65,7 @@ def orm_outer_join_tables(stmt: Any, covered: set[type] | frozenset[type]) -> li def is_excluded(from_obj: Any, excluded: list[Any]) -> bool: + # Table-object identity on purpose: matching by name would also drop the + # WHERE on a raw duplicate occurrence (an alias, another FROM) of the table. plain = _plain(from_obj) return any(plain is t for t in excluded) diff --git a/framework/db/tests/test_query_filter_outer_joins.py b/framework/db/tests/test_query_filter_outer_joins.py index 0e0aeaab..035880a7 100644 --- a/framework/db/tests/test_query_filter_outer_joins.py +++ b/framework/db/tests/test_query_filter_outer_joins.py @@ -27,6 +27,7 @@ _TagBase = create_module_base("qfoj_tag") _LinkBase = create_module_base("qfoj_link") _SoftBase = create_module_base("qfoj_soft") +_PlainBase = create_module_base("qfoj_plain") class _Tag(_TagBase, MultiTenantMixin, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] @@ -48,13 +49,21 @@ class _SoftLink(_SoftBase, SoftDeleteMixin, table=True): # type: ignore[call-ar tag_id: int | None = None # no FK: each model has its own MetaData +class _Plain(_PlainBase, table=True): # type: ignore[call-arg] # ty: ignore[unsupported-base] + """Neither tenant-scoped nor soft-deletable: a FULL join's left side with no filter.""" + + __tablename__ = "qfoj_plain_plain" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(default="", max_length=100) + + @pytest.fixture async def seeded() -> AsyncGenerator[AsyncSession, None]: db_state = init_db("sqlite+aiosqlite:///:memory:", poolclass=StaticPool) try: register_listeners(db_state) async with db_state.engine.begin() as conn: - for base in (_TagBase, _LinkBase, _SoftBase): + for base in (_TagBase, _LinkBase, _SoftBase, _PlainBase): await conn.run_sync(base.metadata.create_all) async with db_state.session_factory() as session: token = current_tenant_id.set("A") @@ -62,6 +71,8 @@ async def seeded() -> AsyncGenerator[AsyncSession, None]: session.add_all([used, unused]) await session.flush() session.add(_TagLink(tag_id=used.id, article_id=1)) + # Same id as ``used``: every link and soft link points at it too. + session.add(_Plain(id=used.id, name="p1")) session.add_all([_SoftLink(tag_id=used.id), _SoftLink(tag_id=used.id, is_deleted=True)]) await session.flush() current_tenant_id.reset(token) @@ -212,3 +223,28 @@ async def test_strict_mode_without_tenant_still_refuses_an_outer_child(seeded): ) with pytest.raises(TenantIsolationError): await seeded.execute(stmt) + + +async def test_full_join_never_returns_other_tenants_children(seeded): + """A FULL join preserves its right side: ON cannot filter it, WHERE must. + + Tenant B's links fail the ON predicate and would come back as unmatched + ``(None, …)`` rows if the child's WHERE predicate were dropped. + """ + stmt = ( + select(_Plain.name, func.count(_TagLink.id)) + .select_from(_Plain) + .outerjoin(_TagLink, _TagLink.tag_id == _Plain.id, full=True) + .group_by(_Plain.name) + ) + assert (await seeded.execute(stmt)).all() == [("p1", 1)] + + +async def test_full_join_never_returns_trashed_children(seeded): + stmt = ( + select(_Plain.name, func.count(_SoftLink.id)) + .select_from(_Plain) + .outerjoin(_SoftLink, _SoftLink.tag_id == _Plain.id, full=True) + .group_by(_Plain.name) + ) + assert (await seeded.execute(stmt)).all() == [("p1", 1)] From 64de87cfc80c50fbd01348626583416efca79f74 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:18:34 +0200 Subject: [PATCH 09/30] test(background_tasks): pin timezone-aware timestamps for success_count_since (#413) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_background_tasks_tz_columns.py | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 modules/background_tasks/tests/test_background_tasks_tz_columns.py diff --git a/modules/background_tasks/tests/test_background_tasks_tz_columns.py b/modules/background_tasks/tests/test_background_tasks_tz_columns.py new file mode 100644 index 00000000..5f0e02ee --- /dev/null +++ b/modules/background_tasks/tests/test_background_tasks_tz_columns.py @@ -0,0 +1,56 @@ +"""#413: every TaskExecution timestamp is timezone-aware, so aware cutoffs bind on Postgres.""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta +from unittest.mock import MagicMock + +import pytest +from background_tasks.constants import TaskStatus +from background_tasks.models import TaskExecution +from background_tasks.service import BackgroundTaskService +from simple_module_core.events import EventBus +from simple_module_db import RequestSession +from users.models.refresh_token import RefreshToken + +_AWARE = { + TaskExecution: ( + "queued_at", + "started_at", + "finished_at", + "heartbeat_at", + "created_at", + "updated_at", + ), + RefreshToken: ("created_at", "expires_at", "revoked_at"), +} + + +@pytest.mark.parametrize(("model", "column"), [(m, c) for m, cols in _AWARE.items() for c in cols]) +def test_timestamp_columns_are_timezone_aware(model, column): + assert model.__table__.c[column].type.timezone is True + + +async def test_success_count_since_binds_an_aware_cutoff(db_session: RequestSession): + """Raised asyncpg DataError on Postgres before #406; runs there under make test-py-pg.""" + service = BackgroundTaskService(db=db_session, celery=MagicMock(), event_bus=EventBus()) + assert await service.success_count_since() == 0 + + now = datetime.now(UTC) + db_session.add( + TaskExecution( + celery_task_id=str(uuid.uuid4()), + task_name="demo.task", + status=TaskStatus.SUCCESS, + queue="default", + args=[], + kwargs={}, + queued_at=now - timedelta(minutes=5), + finished_at=now - timedelta(minutes=1), + ) + ) + await db_session.flush() + + assert await service.success_count_since(hours=1) == 1 + assert await service.success_count_since(hours=1, queue="other") == 0 From da58ea3830212904950db4afb9595d5399b425b9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:20:21 +0200 Subject: [PATCH 10/30] fix(auth): record the post-login target only for navigations (#416) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../templates/host/templates/index.html | 1 + modules/auth/auth/middleware.py | 29 +++++- modules/auth/tests/test_auth_next_target.py | 98 +++++++++++++++++++ 3 files changed, 127 insertions(+), 1 deletion(-) create mode 100644 modules/auth/tests/test_auth_next_target.py diff --git a/framework/cli/simple_module_cli/templates/host/templates/index.html b/framework/cli/simple_module_cli/templates/host/templates/index.html index 073f29ae..5e17296c 100644 --- a/framework/cli/simple_module_cli/templates/host/templates/index.html +++ b/framework/cli/simple_module_cli/templates/host/templates/index.html @@ -9,6 +9,7 @@ {% set brand = branding_head(request) %} {{ brand.app_name }} {% if brand.theme_color %}{% endif %} + {% if brand.favicon_url %}{% endif %} diff --git a/modules/auth/auth/middleware.py b/modules/auth/auth/middleware.py index 12074d50..9f6683fd 100644 --- a/modules/auth/auth/middleware.py +++ b/modules/auth/auth/middleware.py @@ -33,6 +33,27 @@ def _query_suffix(request: Request) -> str: return f"?{request.url.query}" if request.url.query else "" +def _is_navigation(request: Request) -> bool: + """Whether this request is the page the visitor asked for, not something it loaded. + + Only a navigation may become the post-login target: the sign-in page's + own favicon request used to overwrite it, so sign-in landed on + /favicon.ico (#416). Inertia visits count — a session expiring mid-click + must still return the user to that page. Without fetch metadata (old + clients, scripts) keep the previous behaviour. + """ + headers = request.headers + if headers.get("x-inertia") == "true": + return True + dest = headers.get("sec-fetch-dest") + if dest is not None: + return dest == "document" + mode = headers.get("sec-fetch-mode") + if mode is not None: + return mode == "navigate" + return True + + class AuthMiddleware: """Authenticate requests via the registered AuthProvider. @@ -108,7 +129,13 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: # goes to the provider — session-based providers ignore it, # but a redirect-based one (OIDC) needs it in the auth URL. session = scope.get("session", {}) - next_url = safe_next_or_none(request.url.path + _query_suffix(request)) + # Only a navigation is a target: a subresource (the sign-in + # page's own favicon) would overwrite it (#416). + next_url = ( + safe_next_or_none(request.url.path + _query_suffix(request)) + if _is_navigation(request) + else None + ) if next_url is not None: session[SESSION_NEXT_KEY] = next_url response = RedirectResponse( diff --git a/modules/auth/tests/test_auth_next_target.py b/modules/auth/tests/test_auth_next_target.py new file mode 100644 index 00000000..38a1ca19 --- /dev/null +++ b/modules/auth/tests/test_auth_next_target.py @@ -0,0 +1,98 @@ +"""Only top-level navigations may become the post-login target (#416).""" + +from __future__ import annotations + +import httpx +import pytest +from auth.middleware import AuthMiddleware +from auth.state import AuthState +from fastapi import FastAPI, Request +from simple_module_core.redirect_safety import SESSION_NEXT_KEY +from starlette.middleware.sessions import SessionMiddleware +from starlette.responses import JSONResponse + +SECRET = "test-next-target-secret" + + +class _StubProvider: + name = "stub" + + async def resolve_user(self, request): + return None + + def get_login_url(self, request, next_url=None): + return "/stub/login" + + def get_logout_url(self, request): + return "/stub/logout" + + def get_public_paths(self): + return (("/stub/login", "/stub/public/"), ()) + + def is_bearer_request(self, request): + return False + + +def _build_app(provider) -> FastAPI: + app = FastAPI() + app.state.auth = AuthState(auth_provider=provider, principal_resolvers=[]) + + # /static/ is framework-public, so this reads the session without auth. + async def _next(request: Request): + return JSONResponse({"next": request.session.get(SESSION_NEXT_KEY)}) + + async def _any(path: str = ""): + return JSONResponse({}) + + app.add_api_route("/static/next", _next, methods=["GET"]) + app.add_api_route("/{path:path}", _any, methods=["GET"]) + app.add_middleware(AuthMiddleware) + app.add_middleware(SessionMiddleware, secret_key=SECRET) + return app + + +CASES = [ + ({"Sec-Fetch-Dest": "image", "Sec-Fetch-Mode": "no-cors"}, None), # favicon + ({"Sec-Fetch-Dest": "script", "Sec-Fetch-Mode": "no-cors"}, None), + ({"Sec-Fetch-Dest": "empty", "Sec-Fetch-Mode": "cors"}, None), # fetch() + ({"Sec-Fetch-Dest": "document", "Sec-Fetch-Mode": "navigate"}, "/protected/page"), + ({"Sec-Fetch-Mode": "navigate"}, "/protected/page"), + ({"X-Inertia": "true", "Sec-Fetch-Dest": "empty", "Sec-Fetch-Mode": "cors"}, "/protected/page"), + ({}, "/protected/page"), # no metadata: previous behaviour +] + + +@pytest.mark.parametrize(("headers", "expected"), CASES) +async def test_only_navigations_record_the_target(headers, expected): + seen: list[str | None] = [] + + class _Recording(_StubProvider): + def get_login_url(self, request, next_url=None): + seen.append(next_url) + return "/stub/login" + + app = _build_app(_Recording()) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://test" + ) as c: + resp = await c.get("/protected/page", headers=headers) + recorded = (await c.get("/static/next")).json()["next"] + assert resp.status_code == 302 + assert resp.headers["location"] == "/stub/login" + assert seen == [expected] + assert recorded == expected + + +async def test_a_subresource_does_not_overwrite_a_recorded_target(): + """The bug: the sign-in page's favicon request replaced the real target.""" + app = _build_app(_StubProvider()) + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://test" + ) as c: + nav = {"Sec-Fetch-Dest": "document", "Sec-Fetch-Mode": "navigate"} + assert (await c.get("/protected/page", headers=nav)).status_code == 302 + icon = {"Sec-Fetch-Dest": "image", "Sec-Fetch-Mode": "no-cors"} + resp = await c.get("/favicon.ico", headers=icon) + assert resp.status_code == 302 # the redirect itself is unchanged + recorded = (await c.get("/static/next")).json()["next"] + assert recorded == "/protected/page" From ff52b39a0a88ef80e2e5fd37b2ba43227fe3659a Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:21:55 +0200 Subject: [PATCH 11/30] refactor(i18n): split flatten and registry build out of capped files (#415) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/_i18n_flatten.py | 31 +++++++++++ framework/core/simple_module_core/i18n.py | 30 ++--------- .../simple_module_hosting/_i18n_build.py | 52 +++++++++++++++++++ .../simple_module_hosting/i18n_manifest.py | 43 +-------------- 4 files changed, 88 insertions(+), 68 deletions(-) create mode 100644 framework/core/simple_module_core/_i18n_flatten.py create mode 100644 framework/hosting/simple_module_hosting/_i18n_build.py diff --git a/framework/core/simple_module_core/_i18n_flatten.py b/framework/core/simple_module_core/_i18n_flatten.py new file mode 100644 index 00000000..8f1d027d --- /dev/null +++ b/framework/core/simple_module_core/_i18n_flatten.py @@ -0,0 +1,31 @@ +"""Flatten nested locale dicts to dotted keys.""" + +from __future__ import annotations + +from typing import Any + + +def flatten_messages( + nested: dict[str, Any], + *, + prefix: str = "", +) -> dict[str, str]: + """Flatten a nested dict of string leaves to dotted keys. + + {"browse": {"title": "X"}} -> {"browse.title": "X"} + + Raises ValueError if any leaf is not a string. + """ + out: dict[str, str] = {} + for key, value in nested.items(): + composed = f"{prefix}.{key}" if prefix else key + if isinstance(value, dict): + out.update(flatten_messages(value, prefix=composed)) + elif isinstance(value, str): + out[composed] = value + else: + raise ValueError( + f"Locale value at '{composed}' must be string or nested dict, " + f"got {type(value).__name__}" + ) + return out diff --git a/framework/core/simple_module_core/i18n.py b/framework/core/simple_module_core/i18n.py index a2a016f7..37c40ec1 100644 --- a/framework/core/simple_module_core/i18n.py +++ b/framework/core/simple_module_core/i18n.py @@ -12,6 +12,10 @@ from babel import Locale +from simple_module_core._i18n_flatten import flatten_messages + +__all__ = ["PLURAL_CATEGORIES", "I18nRegistry", "Translator", "flatten_messages"] + logger = logging.getLogger(__name__) #: CLDR plural categories, in spec order. Used both for runtime resolution and @@ -38,32 +42,6 @@ def _plural_form(locale: str, count: float) -> str: return rule(count) -def flatten_messages( - nested: dict[str, Any], - *, - prefix: str = "", -) -> dict[str, str]: - """Flatten a nested dict of string leaves to dotted keys. - - {"browse": {"title": "X"}} -> {"browse.title": "X"} - - Raises ValueError if any leaf is not a string. - """ - out: dict[str, str] = {} - for key, value in nested.items(): - composed = f"{prefix}.{key}" if prefix else key - if isinstance(value, dict): - out.update(flatten_messages(value, prefix=composed)) - elif isinstance(value, str): - out[composed] = value - else: - raise ValueError( - f"Locale value at '{composed}' must be string or nested dict, " - f"got {type(value).__name__}" - ) - return out - - class I18nRegistry: """Merged view of all module locale JSON files, keyed by locale. diff --git a/framework/hosting/simple_module_hosting/_i18n_build.py b/framework/hosting/simple_module_hosting/_i18n_build.py new file mode 100644 index 00000000..6dad9b0c --- /dev/null +++ b/framework/hosting/simple_module_hosting/_i18n_build.py @@ -0,0 +1,52 @@ +"""Construct the i18n registry from module, host and UI sources.""" + +from __future__ import annotations + +from pathlib import Path + +from simple_module_core import ModuleBase +from simple_module_core.i18n import I18nRegistry + +from simple_module_hosting.settings import Settings + + +def build_i18n_registry( + settings: Settings, + modules: list[ModuleBase], + project_root: Path, +) -> tuple[I18nRegistry, list[tuple[str, str, Path]]]: + """Construct the i18n registry from module + host + UI sources. + + Returns ``(registry, extra_sources)`` where ``extra_sources`` is the list + of ``(reporter_name, namespace, dir)`` triples the diagnostic runner + needs to validate non-module locale directories. + """ + registry = I18nRegistry( + default_locale=settings.i18n_default_locale, + supported_locales=settings.i18n_supported_locales, + ) + extra_sources: list[tuple[str, str, Path]] = [] + + for mod in modules: + audience = getattr(mod.meta, "i18n_audience", "public") + for namespace, locale_dir in mod.locale_dirs().items(): + registry.add_source(namespace, locale_dir, audience=audience) + + # The framework's own catalog — the setup wizard ships with this package, + # so its strings must too, whatever host is serving it. + hosting_locales = Path(__file__).resolve().parent / "locales" + registry.add_source("hosting", hosting_locales) + extra_sources.append(("simple_module_hosting", "hosting", hosting_locales)) + + host_locales = project_root / "host" / "locales" + if host_locales.is_dir(): + registry.add_source("host", host_locales) + extra_sources.append(("host", "host", host_locales)) + + ui_locales = project_root / "packages" / "ui" / "locales" + if ui_locales.is_dir(): + registry.add_source("ui", ui_locales) + extra_sources.append(("packages/ui", "ui", ui_locales)) + + registry.load() + return registry, extra_sources diff --git a/framework/hosting/simple_module_hosting/i18n_manifest.py b/framework/hosting/simple_module_hosting/i18n_manifest.py index 79e258a3..59a6d9b2 100644 --- a/framework/hosting/simple_module_hosting/i18n_manifest.py +++ b/framework/hosting/simple_module_hosting/i18n_manifest.py @@ -22,54 +22,13 @@ from simple_module_core import ModuleBase from simple_module_core.i18n import PLURAL_CATEGORIES, I18nRegistry +from simple_module_hosting._i18n_build import build_i18n_registry from simple_module_hosting.manifest import _write_if_changed from simple_module_hosting.settings import Settings logger = logging.getLogger(__name__) -def build_i18n_registry( - settings: Settings, - modules: list[ModuleBase], - project_root: Path, -) -> tuple[I18nRegistry, list[tuple[str, str, Path]]]: - """Construct the i18n registry from module + host + UI sources. - - Returns ``(registry, extra_sources)`` where ``extra_sources`` is the list - of ``(reporter_name, namespace, dir)`` triples the diagnostic runner - needs to validate non-module locale directories. - """ - registry = I18nRegistry( - default_locale=settings.i18n_default_locale, - supported_locales=settings.i18n_supported_locales, - ) - extra_sources: list[tuple[str, str, Path]] = [] - - for mod in modules: - audience = getattr(mod.meta, "i18n_audience", "public") - for namespace, locale_dir in mod.locale_dirs().items(): - registry.add_source(namespace, locale_dir, audience=audience) - - # The framework's own catalog — the setup wizard ships with this package, - # so its strings must too, whatever host is serving it. - hosting_locales = Path(__file__).resolve().parent / "locales" - registry.add_source("hosting", hosting_locales) - extra_sources.append(("simple_module_hosting", "hosting", hosting_locales)) - - host_locales = project_root / "host" / "locales" - if host_locales.is_dir(): - registry.add_source("host", host_locales) - extra_sources.append(("host", "host", host_locales)) - - ui_locales = project_root / "packages" / "ui" / "locales" - if ui_locales.is_dir(): - registry.add_source("ui", ui_locales) - extra_sources.append(("packages/ui", "ui", ui_locales)) - - registry.load() - return registry, extra_sources - - def emit_frontend_types_for_modules( settings: Settings, installed_modules: list[ModuleBase], From 8875dd9550c139c7cf962c5d805fd02dd06ace38 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:25:58 +0200 Subject: [PATCH 12/30] feat(i18n): host override layer for module copy (#415) host/locales/overrides/.json replaces existing keys by full dotted path after all catalogs; unknown keys are skipped and reported as SM027. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CLAUDE.md | 2 +- docs/framework/i18n.md | 17 +++++ docs/reference/diagnostic-codes.md | 1 + .../simple_module_core/diagnostics/_i18n.py | 17 +++++ .../simple_module_core/diagnostics/_runner.py | 7 ++ framework/core/simple_module_core/i18n.py | 22 ++++++ .../core/simple_module_core/i18n_overrides.py | 43 +++++++++++ framework/core/tests/test_i18n_overrides.py | 72 +++++++++++++++++++ .../simple_module_hosting/_dev_boot.py | 2 + .../simple_module_hosting/_i18n_build.py | 4 ++ .../simple_module_hosting/app_builder.py | 1 + .../tests/test_i18n_build_overrides.py | 36 ++++++++++ 12 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 framework/core/simple_module_core/i18n_overrides.py create mode 100644 framework/core/tests/test_i18n_overrides.py create mode 100644 framework/hosting/tests/test_i18n_build_overrides.py diff --git a/CLAUDE.md b/CLAUDE.md index 9aa714a8..b68cd46b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -113,7 +113,7 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b ## Diagnostic codes -Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). `SM025` `multi_tenant` is on but no module registered `app.state.tenant_resolver` (warn — checked at boot after module registration, in every environment, not by the `make doctor` CLI). `SM026` the database is SQLite and a model declares an expression index it cannot verify (info — autogenerate and `alembic check` skip those on SQLite; add them by hand and run `make migrations-roundtrip-pg`). In production, errors fail boot. +Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). `SM025` `multi_tenant` is on but no module registered `app.state.tenant_resolver` (warn — checked at boot after module registration, in every environment, not by the `make doctor` CLI). `SM026` the database is SQLite and a model declares an expression index it cannot verify (info — autogenerate and `alembic check` skip those on SQLite; add them by hand and run `make migrations-roundtrip-pg`). `SM027` a host locale override (`host/locales/overrides/.json`) names a key no catalog defines (warn — skipped; dev boot only). In production, errors fail boot. ## Tests & fixtures diff --git a/docs/framework/i18n.md b/docs/framework/i18n.md index 3b6680d5..46e148b1 100644 --- a/docs/framework/i18n.md +++ b/docs/framework/i18n.md @@ -169,6 +169,23 @@ Module-scope `t(...)` calls capture the first-render locale and never update. Both are auto-discovered alongside module contributions — no manual wiring. +### Overriding module copy + +A host cannot reach a module's keys through `host.*`. To change copy a module ships (for +example the sign-in aside), add `host/locales/overrides/.json`, keyed by the **full +dotted path** — nested or flat both work: + +```json +{ "users.login.aside_heading": "Kuri for teams" } +``` + +Overrides are applied after every module, framework and host catalog, per locale. An +override only **replaces an existing key**: a key no catalog defines is skipped, logged, and +reported as `SM027`, so a typo cannot silently invent copy nothing renders. Audience is +preserved: overriding a key from an admin-only catalog does not expose it to anonymous +visitors. The overrides directory is not part of the `host` namespace and does not change the +generated TypeScript key list. + ## Configuration The three i18n fields are declared on `HostSettings`, but they are **read from the environment at boot**: diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index ff44a3cf..44c7405e 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -35,6 +35,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM024` | WARNING | A unique column, constraint or index on a `MultiTenantMixin` table does not include `tenant_id`, so the first tenant to claim a value locks every other tenant out of it. | Make the key per tenant: add `tenant_id` to it (`Index(..., "tenant_id", "slug", unique=True)`). | | `SM025` | WARNING | `multi_tenant` is on but no module registered `app.state.tenant_resolver`, so only the principal's `tenant_id` claim can bind a tenant — with most auth providers every tenant-scoped query then fails closed. Checked at boot (it needs the built app), in every environment, not by `make doctor`. | Install the `tenants` module, or register your own `async (Request) -> str \| None` resolver on `app.state.tenant_resolver`. | | `SM026` | INFO | The configured database is SQLite and a module model declares an expression index (e.g. `lower(email)`). SQLite cannot reflect those, so autogenerate and `alembic check` cannot verify them and a clean diff says nothing about them. | Add the index to a revision by hand and verify with `make migrations-roundtrip-pg` (PostgreSQL). See [Migrations](/module-authoring#reviewing-a-revision-autogenerated-on-sqlite). | +| `SM027` | WARNING | A key in `host/locales/overrides/.json` matches no catalog key, so the override was skipped. Reported at dev boot. | Fix the key: overrides replace an existing key by its full dotted path (e.g. `users.login.aside_heading`). See [i18n](/framework/i18n#overriding-module-copy). | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). diff --git a/framework/core/simple_module_core/diagnostics/_i18n.py b/framework/core/simple_module_core/diagnostics/_i18n.py index 749724b0..d7158001 100644 --- a/framework/core/simple_module_core/diagnostics/_i18n.py +++ b/framework/core/simple_module_core/diagnostics/_i18n.py @@ -143,3 +143,20 @@ def _locales_on_disk(locale_dir: Path) -> list[str]: if not locale_dir.is_dir(): return [] return sorted(path.stem for path in locale_dir.glob("*.json")) + + +def check_unknown_overrides(unknown: dict[str, list[str]]) -> list[Diagnostic]: + """SM027: host locale overrides naming keys no catalog defines (skipped).""" + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code="SM027", + message=( + f"Host locale override ({locale}) names keys no catalog defines " + f"(skipped): {', '.join(keys)}" + ), + module_name="host", + suggestion="Overrides replace an existing key by its full dotted path; fix the typo.", + ) + for locale, keys in sorted(unknown.items()) + ] diff --git a/framework/core/simple_module_core/diagnostics/_runner.py b/framework/core/simple_module_core/diagnostics/_runner.py index 93f020bd..e7346375 100644 --- a/framework/core/simple_module_core/diagnostics/_runner.py +++ b/framework/core/simple_module_core/diagnostics/_runner.py @@ -31,6 +31,7 @@ def run_diagnostics( i18n_default_locale: str | None = None, i18n_extra_sources: list[tuple[str, str, Path]] | None = None, database_dialect: str | None = None, + i18n_unknown_overrides: dict[str, list[str]] | None = None, ) -> list[Diagnostic]: """Convenience function to run all diagnostics. @@ -42,6 +43,7 @@ def run_diagnostics( host/ui locale dirs that aren't owned by a ``ModuleBase``. ``database_dialect`` (``"sqlite"``, ``"postgresql"``...) enables SM026, which flags expression indexes a SQLite database cannot verify. + ``i18n_unknown_overrides`` (``I18nRegistry.unknown_override_keys``) enables SM027. """ diagnostics = ModuleDiagnostics().run(modules) @@ -64,6 +66,11 @@ def run_diagnostics( ).run(modules) ) + if i18n_unknown_overrides: + from simple_module_core.diagnostics._i18n import check_unknown_overrides + + diagnostics.extend(check_unknown_overrides(i18n_unknown_overrides)) + if migration_state is not None: migration_diag = MigrationDiagnostics() diagnostics.extend( diff --git a/framework/core/simple_module_core/i18n.py b/framework/core/simple_module_core/i18n.py index 37c40ec1..b27a86ab 100644 --- a/framework/core/simple_module_core/i18n.py +++ b/framework/core/simple_module_core/i18n.py @@ -13,6 +13,7 @@ from babel import Locale from simple_module_core._i18n_flatten import flatten_messages +from simple_module_core.i18n_overrides import apply_overrides, load_overrides __all__ = ["PLURAL_CATEGORIES", "I18nRegistry", "Translator", "flatten_messages"] @@ -71,8 +72,15 @@ def __init__(self, default_locale: str, supported_locales: list[str]) -> None: self._available_locales_list: list[str] = [] self._empty_view: MappingProxyType[str, str] = MappingProxyType({}) self._empty_snapshot: dict[str, str] = {} + self._override_dirs: list[Path] = [] + #: locale -> override keys that matched no catalog key (set by ``load()``). + self.unknown_override_keys: dict[str, list[str]] = {} self._loaded = False + def add_overrides(self, overrides_dir: Path) -> None: + """Queue a host directory of ``.json`` overrides, applied last by ``load()``.""" + self._override_dirs.append(Path(overrides_dir)) + def add_source(self, namespace: str, locale_dir: Path, *, audience: str = "public") -> None: """Queue a module's locale directory for loading under a namespace. @@ -115,6 +123,20 @@ def load(self) -> None: if audience != "admin": public_messages[locale].update(flat) + self.unknown_override_keys = {} + for overrides_dir in self._override_dirs: + for locale in self.supported_locales: + skipped = apply_overrides( + load_overrides(overrides_dir, locale), + self._messages[locale], + public_messages[locale], + ) + if skipped: + self.unknown_override_keys.setdefault(locale, []).extend(skipped) + logger.warning( + "Ignoring host overrides for unknown keys (%s): %s", locale, skipped + ) + # Cache the derived views now that loading is complete. Downstream # (middleware, translator, switcher) reads these on every request. self._message_views = { diff --git a/framework/core/simple_module_core/i18n_overrides.py b/framework/core/simple_module_core/i18n_overrides.py new file mode 100644 index 00000000..1edc83ec --- /dev/null +++ b/framework/core/simple_module_core/i18n_overrides.py @@ -0,0 +1,43 @@ +"""Host overrides of module copy, keyed by full dotted path (#415). + +``host/locales/overrides/.json`` is applied after every module, +framework and host catalog. An override replaces an existing key and never +creates one: a typo would otherwise invent a key nothing renders, and a new +key would churn the generated TypeScript key list. Audience is kept: a key +only an admin catalog defines stays out of the anonymous snapshot. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +from simple_module_core._i18n_flatten import flatten_messages + + +def load_overrides(overrides_dir: Path, locale: str) -> dict[str, str]: + path = overrides_dir / f"{locale}.json" + if not path.is_file(): + return {} + try: + raw = json.loads(path.read_text(encoding="utf-8")) + except json.JSONDecodeError as exc: + raise ValueError(f"invalid JSON in {path}: {exc}") from exc + if not isinstance(raw, dict): + raise ValueError(f"{path} must contain a JSON object at the top level") + return flatten_messages(raw, prefix="") + + +def apply_overrides( + overrides: dict[str, str], messages: dict[str, str], public: dict[str, str] +) -> list[str]: + """Apply ``overrides`` in place; return the keys that matched nothing.""" + unknown: list[str] = [] + for key, value in overrides.items(): + if key not in messages: + unknown.append(key) + continue + messages[key] = value + if key in public: + public[key] = value + return sorted(unknown) diff --git a/framework/core/tests/test_i18n_overrides.py b/framework/core/tests/test_i18n_overrides.py new file mode 100644 index 00000000..6a122320 --- /dev/null +++ b/framework/core/tests/test_i18n_overrides.py @@ -0,0 +1,72 @@ +"""Host locale overrides replace module copy by full dotted key (#415).""" + +from __future__ import annotations + +import json + +from simple_module_core.diagnostics._i18n import check_unknown_overrides +from simple_module_core.i18n import I18nRegistry + + +def _write(dir_, locale, data): + dir_.mkdir(parents=True, exist_ok=True) + (dir_ / f"{locale}.json").write_text(json.dumps(data), encoding="utf-8") + + +def _registry(tmp_path, overrides, *, audience="public", locales=("en",)): + _write(tmp_path / "users", "en", {"login": {"aside_heading": "One admin surface"}}) + if "es" in locales: + _write(tmp_path / "users", "es", {"login": {"aside_heading": "Una superficie"}}) + for locale, data in overrides.items(): + _write(tmp_path / "overrides", locale, data) + reg = I18nRegistry(default_locale="en", supported_locales=list(locales)) + reg.add_source("users", tmp_path / "users", audience=audience) + reg.add_overrides(tmp_path / "overrides") + reg.load() + return reg + + +def test_nested_override_replaces_module_copy(tmp_path): + reg = _registry(tmp_path, {"en": {"users": {"login": {"aside_heading": "Kuri for teams"}}}}) + assert reg.messages("en")["users.login.aside_heading"] == "Kuri for teams" + snap = reg.messages_snapshot("en", include_admin=False) + assert snap["users.login.aside_heading"] == "Kuri for teams" + + +def test_flat_dotted_override_works_too(tmp_path): + reg = _registry(tmp_path, {"en": {"users.login.aside_heading": "Kuri"}}) + assert reg.messages("en")["users.login.aside_heading"] == "Kuri" + + +def test_unknown_key_is_skipped_and_reported(tmp_path): + reg = _registry(tmp_path, {"en": {"users.login.asid_heading": "typo"}}) + assert "users.login.asid_heading" not in reg.messages("en") + assert reg.unknown_override_keys == {"en": ["users.login.asid_heading"]} + + +def test_admin_only_key_stays_out_of_the_public_snapshot(tmp_path): + reg = _registry(tmp_path, {"en": {"users.login.aside_heading": "Kuri"}}, audience="admin") + assert reg.messages("en")["users.login.aside_heading"] == "Kuri" + assert "users.login.aside_heading" not in reg.messages_snapshot("en", include_admin=False) + + +def test_per_locale_override_beats_the_default_layer(tmp_path): + reg = _registry( + tmp_path, {"es": {"users.login.aside_heading": "Kuri ES"}}, locales=("en", "es") + ) + assert reg.messages_snapshot("es", include_admin=True)["users.login.aside_heading"] == "Kuri ES" + assert reg.messages("en")["users.login.aside_heading"] == "One admin surface" + + +def test_missing_override_dir_is_fine(tmp_path): + reg = _registry(tmp_path, {}) + assert reg.unknown_override_keys == {} + + +def test_unknown_override_keys_yield_sm027(tmp_path): + reg = _registry(tmp_path, {"en": {"users.login.asid_heading": "typo"}}) + findings = check_unknown_overrides(reg.unknown_override_keys) + assert [f.code for f in findings] == ["SM027"] + assert findings[0].level.value == "warning" + assert "users.login.asid_heading" in findings[0].message + assert check_unknown_overrides({}) == [] diff --git a/framework/hosting/simple_module_hosting/_dev_boot.py b/framework/hosting/simple_module_hosting/_dev_boot.py index 76d7adc5..1d8d05b2 100644 --- a/framework/hosting/simple_module_hosting/_dev_boot.py +++ b/framework/hosting/simple_module_hosting/_dev_boot.py @@ -35,6 +35,7 @@ def run_dev_boot( installed_modules: list[ModuleBase], *, i18n_extra, + i18n_unknown_overrides: dict[str, list[str]] | None = None, diagnostics_state: DiagnosticsState, project_root: Path, ) -> None: @@ -54,6 +55,7 @@ def run_dev_boot( i18n_supported_locales=settings.i18n_supported_locales, i18n_default_locale=settings.i18n_default_locale, i18n_extra_sources=i18n_extra, + i18n_unknown_overrides=i18n_unknown_overrides, database_dialect=settings.database_url.split(":", 1)[0].split("+", 1)[0], ) diagnostics = diagnostics_state.rerun() diff --git a/framework/hosting/simple_module_hosting/_i18n_build.py b/framework/hosting/simple_module_hosting/_i18n_build.py index 6dad9b0c..88e599ec 100644 --- a/framework/hosting/simple_module_hosting/_i18n_build.py +++ b/framework/hosting/simple_module_hosting/_i18n_build.py @@ -43,6 +43,10 @@ def build_i18n_registry( registry.add_source("host", host_locales) extra_sources.append(("host", "host", host_locales)) + overrides = project_root / "host" / "locales" / "overrides" + if overrides.is_dir(): + registry.add_overrides(overrides) + ui_locales = project_root / "packages" / "ui" / "locales" if ui_locales.is_dir(): registry.add_source("ui", ui_locales) diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 59daa7a2..3aae5cff 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -123,6 +123,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: modules, installed_modules, i18n_extra=i18n_extra, + i18n_unknown_overrides=i18n_registry.unknown_override_keys, diagnostics_state=diagnostics_state, project_root=_PROJECT_ROOT, ) diff --git a/framework/hosting/tests/test_i18n_build_overrides.py b/framework/hosting/tests/test_i18n_build_overrides.py new file mode 100644 index 00000000..7d6671a8 --- /dev/null +++ b/framework/hosting/tests/test_i18n_build_overrides.py @@ -0,0 +1,36 @@ +"""build_i18n_registry registers host/locales/overrides (#415).""" + +from __future__ import annotations + +import json + +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_hosting.i18n_manifest import build_i18n_registry +from simple_module_hosting.settings import Settings + + +class _Users(ModuleBase): + meta = ModuleMeta(name="users") + + def __init__(self, locales): + self._locales = locales + + def locale_dirs(self): + return {"users": self._locales} + + +def test_build_applies_host_overrides_and_not_as_host_namespace(tmp_path): + mod_dir = tmp_path / "mod" + mod_dir.mkdir() + (mod_dir / "en.json").write_text(json.dumps({"login": {"aside_heading": "Old"}})) + ov = tmp_path / "host" / "locales" / "overrides" + ov.mkdir(parents=True) + (ov / "en.json").write_text(json.dumps({"users.login.aside_heading": "New"})) + (tmp_path / "host" / "locales" / "en.json").write_text(json.dumps({"x": "y"})) + + settings = Settings(database_url="sqlite+aiosqlite:///:memory:") + registry, _ = build_i18n_registry(settings, [_Users(mod_dir)], tmp_path) + msgs = registry.messages("en") + assert msgs["users.login.aside_heading"] == "New" + assert msgs["host.x"] == "y" + assert not any(k.startswith("host.users") for k in msgs) From 0f56061660a3fd56714399598ccfd3c13cc883cc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:31:03 +0200 Subject: [PATCH 13/30] fix(i18n): doctor reports SM027; overrides reach default-only keys (#415) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CLAUDE.md | 2 +- docs/reference/diagnostic-codes.md | 2 +- framework/core/simple_module_core/__main__.py | 4 ++ .../simple_module_core/diagnostics/_i18n.py | 31 ++++++++++++ framework/core/simple_module_core/i18n.py | 2 + .../core/simple_module_core/i18n_overrides.py | 19 +++++-- .../core/tests/test_doctor_cli_overrides.py | 40 +++++++++++++++ .../test_i18n_overrides_default_layer.py | 50 +++++++++++++++++++ 8 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 framework/core/tests/test_doctor_cli_overrides.py create mode 100644 framework/core/tests/test_i18n_overrides_default_layer.py diff --git a/CLAUDE.md b/CLAUDE.md index b68cd46b..5c65cb19 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -113,7 +113,7 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b ## Diagnostic codes -Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). `SM025` `multi_tenant` is on but no module registered `app.state.tenant_resolver` (warn — checked at boot after module registration, in every environment, not by the `make doctor` CLI). `SM026` the database is SQLite and a model declares an expression index it cannot verify (info — autogenerate and `alembic check` skip those on SQLite; add them by hand and run `make migrations-roundtrip-pg`). `SM027` a host locale override (`host/locales/overrides/.json`) names a key no catalog defines (warn — skipped; dev boot only). In production, errors fail boot. +Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues, `SM017` module ships `.tsx` pages but is missing `package.json`/`tsconfig.json` (warn), `SM018` Inertia `router.{post,patch,put,delete}()` in a page targets a JSON `/api/*` endpoint (warn — Inertia rejects non-Inertia responses), `SM019` module registers view routes (non-empty `view_prefix` + overrides `register_routes`) but overrides neither `register_menu_items` nor `register_permissions` (warn — pages exist with no sidebar entry and no role-editor visibility; admins can't reach them through the UI). Modules whose views are sub-pages of another module typically register permissions to stay discoverable in the role editor without needing their own sidebar entry. `SM020` multiple auth provider modules installed (error), `SM021` no auth provider module installed (warn), `SM022` `@theme`/`@custom-variant`/`@utility` in a module's `styles.css`, where `layer(components)` makes them inert (warn), `SM023` an unlayered rule in a module's `theme.css`, which outranks every Tailwind utility (warn). `SM024` a unique key on a `MultiTenantMixin` table that omits `tenant_id` (warn). `SM025` `multi_tenant` is on but no module registered `app.state.tenant_resolver` (warn — checked at boot after module registration, in every environment, not by the `make doctor` CLI). `SM026` the database is SQLite and a model declares an expression index it cannot verify (info — autogenerate and `alembic check` skip those on SQLite; add them by hand and run `make migrations-roundtrip-pg`). `SM027` a host locale override (`host/locales/overrides/.json`) names a key no catalog defines (warn — skipped; reported by `make doctor` and at dev boot). In production, errors fail boot. ## Tests & fixtures diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index 44c7405e..0be572b9 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -35,7 +35,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM024` | WARNING | A unique column, constraint or index on a `MultiTenantMixin` table does not include `tenant_id`, so the first tenant to claim a value locks every other tenant out of it. | Make the key per tenant: add `tenant_id` to it (`Index(..., "tenant_id", "slug", unique=True)`). | | `SM025` | WARNING | `multi_tenant` is on but no module registered `app.state.tenant_resolver`, so only the principal's `tenant_id` claim can bind a tenant — with most auth providers every tenant-scoped query then fails closed. Checked at boot (it needs the built app), in every environment, not by `make doctor`. | Install the `tenants` module, or register your own `async (Request) -> str \| None` resolver on `app.state.tenant_resolver`. | | `SM026` | INFO | The configured database is SQLite and a module model declares an expression index (e.g. `lower(email)`). SQLite cannot reflect those, so autogenerate and `alembic check` cannot verify them and a clean diff says nothing about them. | Add the index to a revision by hand and verify with `make migrations-roundtrip-pg` (PostgreSQL). See [Migrations](/module-authoring#reviewing-a-revision-autogenerated-on-sqlite). | -| `SM027` | WARNING | A key in `host/locales/overrides/.json` matches no catalog key, so the override was skipped. Reported at dev boot. | Fix the key: overrides replace an existing key by its full dotted path (e.g. `users.login.aside_heading`). See [i18n](/framework/i18n#overriding-module-copy). | +| `SM027` | WARNING | A key in `host/locales/overrides/.json` matches no catalog key, so the override was skipped. Reported by `make doctor` and at dev boot. | Fix the key: overrides replace an existing key by its full dotted path (e.g. `users.login.aside_heading`). See [i18n](/framework/i18n#overriding-module-copy). | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 6ccbdd50..4b502c93 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -30,6 +30,7 @@ print_diagnostics, run_diagnostics, ) +from simple_module_core.diagnostics._i18n import unknown_overrides_for from simple_module_core.discovery import ( discover_modules, resolve_auth_provider, @@ -140,11 +141,14 @@ def main() -> int: supported, default = _load_i18n_settings_from_env() extra = _discover_extra_locale_sources() + unknown_overrides = unknown_overrides_for(modules, extra, supported, default) + diagnostics = run_diagnostics( modules, i18n_supported_locales=supported, i18n_default_locale=default, i18n_extra_sources=extra, + i18n_unknown_overrides=unknown_overrides, database_dialect=_database_dialect(), ) print_diagnostics(diagnostics) diff --git a/framework/core/simple_module_core/diagnostics/_i18n.py b/framework/core/simple_module_core/diagnostics/_i18n.py index d7158001..41ed433e 100644 --- a/framework/core/simple_module_core/diagnostics/_i18n.py +++ b/framework/core/simple_module_core/diagnostics/_i18n.py @@ -160,3 +160,34 @@ def check_unknown_overrides(unknown: dict[str, list[str]]) -> list[Diagnostic]: ) for locale, keys in sorted(unknown.items()) ] + + +def unknown_overrides_for( + modules: list[ModuleBase], + extra_sources: list[tuple[str, str, Path]], + supported_locales: list[str] | None, + default_locale: str, +) -> dict[str, list[str]]: + """Load the catalogs like the host does and return the unknown override keys. + + Lets ``make doctor`` report SM027 without booting the app. Overrides live + in ``overrides/`` beside the ``host`` source's locale directory. + """ + from simple_module_core.i18n import I18nRegistry + + host_dirs = [d for _, ns, d in extra_sources if ns == "host" and (d / "overrides").is_dir()] + if not host_dirs: + return {} + overrides_dir = host_dirs[0] / "overrides" + locales = list(supported_locales or []) + if not locales: + locales = sorted({default_locale, *(p.stem for p in overrides_dir.glob("*.json"))}) + registry = I18nRegistry(default_locale=default_locale, supported_locales=locales) + for mod in modules: + for namespace, locale_dir in mod.locale_dirs().items(): + registry.add_source(namespace, Path(locale_dir)) + for _, namespace, locale_dir in extra_sources: + registry.add_source(namespace, locale_dir) + registry.add_overrides(overrides_dir) + registry.load() + return registry.unknown_override_keys diff --git a/framework/core/simple_module_core/i18n.py b/framework/core/simple_module_core/i18n.py index b27a86ab..a9a709fb 100644 --- a/framework/core/simple_module_core/i18n.py +++ b/framework/core/simple_module_core/i18n.py @@ -130,6 +130,8 @@ def load(self) -> None: load_overrides(overrides_dir, locale), self._messages[locale], public_messages[locale], + default_messages=self._messages.get(self.default_locale), + default_public=public_messages.get(self.default_locale), ) if skipped: self.unknown_override_keys.setdefault(locale, []).extend(skipped) diff --git a/framework/core/simple_module_core/i18n_overrides.py b/framework/core/simple_module_core/i18n_overrides.py index 1edc83ec..215bc0d6 100644 --- a/framework/core/simple_module_core/i18n_overrides.py +++ b/framework/core/simple_module_core/i18n_overrides.py @@ -29,15 +29,26 @@ def load_overrides(overrides_dir: Path, locale: str) -> dict[str, str]: def apply_overrides( - overrides: dict[str, str], messages: dict[str, str], public: dict[str, str] + overrides: dict[str, str], + messages: dict[str, str], + public: dict[str, str], + *, + default_messages: dict[str, str] | None = None, + default_public: dict[str, str] | None = None, ) -> list[str]: - """Apply ``overrides`` in place; return the keys that matched nothing.""" + """Apply ``overrides`` in place; return the keys that matched nothing. + + A key exists if this locale or the default locale defines it, so a + translation of copy the locale has not translated yet still lands. The + public map only gains a key that is public here or in the default locale, + so an admin-only key never leaks. + """ unknown: list[str] = [] for key, value in overrides.items(): - if key not in messages: + if key not in messages and key not in (default_messages or {}): unknown.append(key) continue messages[key] = value - if key in public: + if key in public or key in (default_public or {}): public[key] = value return sorted(unknown) diff --git a/framework/core/tests/test_doctor_cli_overrides.py b/framework/core/tests/test_doctor_cli_overrides.py new file mode 100644 index 00000000..ea03dba2 --- /dev/null +++ b/framework/core/tests/test_doctor_cli_overrides.py @@ -0,0 +1,40 @@ +"""``python -m simple_module_core`` reports SM027 for unknown host overrides (#415).""" + +from __future__ import annotations + +import json + +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_core import __main__ as doctor + + +class _Mod(ModuleBase): + meta = ModuleMeta(name="users") + + def __init__(self, locales): + self._locales = locales + + def locale_dirs(self): + return {"users": self._locales} + + +def test_doctor_cli_reports_unknown_override_key(tmp_path, monkeypatch, capsys): + mod_dir = tmp_path / "mod" + mod_dir.mkdir() + (mod_dir / "en.json").write_text(json.dumps({"login": {"aside_heading": "X"}})) + ov = tmp_path / "host" / "locales" / "overrides" + ov.mkdir(parents=True) + (ov / "en.json").write_text( + json.dumps({"users.login.aside_heading": "ok", "users.login.typo": "bad"}) + ) + mod = _Mod(mod_dir) + monkeypatch.setattr(doctor, "discover_modules", lambda strict=True: [mod]) + monkeypatch.setattr(doctor, "find_env_file", lambda: tmp_path / ".env") + monkeypatch.setattr(doctor, "parse_dotenv", dict) + monkeypatch.delenv("SM_I18N_SUPPORTED_LOCALES", raising=False) + + assert doctor.main() == 0 + err = capsys.readouterr().err + assert "SM027" in err + assert "users.login.typo" in err + assert "aside_heading" not in err.split("SM027", 1)[1] diff --git a/framework/core/tests/test_i18n_overrides_default_layer.py b/framework/core/tests/test_i18n_overrides_default_layer.py new file mode 100644 index 00000000..e94584ab --- /dev/null +++ b/framework/core/tests/test_i18n_overrides_default_layer.py @@ -0,0 +1,50 @@ +"""Locale overrides of keys only the default locale defines (#415).""" + +from __future__ import annotations + +import json + +from simple_module_core.diagnostics import run_diagnostics +from simple_module_core.i18n import I18nRegistry + + +def _write(dir_, locale, data): + dir_.mkdir(parents=True, exist_ok=True) + (dir_ / f"{locale}.json").write_text(json.dumps(data), encoding="utf-8") + + +def _reg(tmp_path, ov, *, audience="public"): + _write(tmp_path / "m", "en", {"a": "A", "b": "B"}) + _write(tmp_path / "m", "es", {"a": "A-es"}) + _write(tmp_path / "ov", "es", ov) + reg = I18nRegistry(default_locale="en", supported_locales=["en", "es"]) + reg.add_source("m", tmp_path / "m", audience=audience) + reg.add_overrides(tmp_path / "ov") + reg.load() + return reg + + +def test_override_of_default_only_public_key_lands(tmp_path): + reg = _reg(tmp_path, {"m": {"b": "B-es"}}) + assert reg.unknown_override_keys == {} + assert reg.messages("es")["m.b"] == "B-es" + assert reg.messages_snapshot("es", include_admin=False)["m.b"] == "B-es" + assert reg.messages_snapshot("es", include_admin=True)["m.b"] == "B-es" + + +def test_override_of_default_only_admin_key_stays_private(tmp_path): + reg = _reg(tmp_path, {"m.b": "B-es"}, audience="admin") + assert reg.unknown_override_keys == {} + assert reg.messages("es")["m.b"] == "B-es" + assert reg.messages_snapshot("es", include_admin=True)["m.b"] == "B-es" + assert "m.b" not in reg.messages_snapshot("es", include_admin=False) + + +def test_key_absent_from_every_locale_is_still_unknown(tmp_path): + reg = _reg(tmp_path, {"m.zzz": "x"}) + assert reg.unknown_override_keys == {"es": ["m.zzz"]} + + +def test_run_diagnostics_reports_sm027_from_unknown_overrides(): + findings = run_diagnostics([], i18n_unknown_overrides={"en": ["users.nope"]}) + assert [f.code for f in findings if f.code == "SM027"] == ["SM027"] From 87f8a4c655490d9e9aef22c2c39f85e2c7c5d470 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:37:28 +0200 Subject: [PATCH 14/30] fix(hosting): emit @source per wheel-module subdirectory (#419) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_module_css_subdir_sources.py | 47 +++++++++++++++++++ .../hosting/simple_module_hosting/assets.py | 30 +++++++++++- 2 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 framework/cli/tests/test_module_css_subdir_sources.py diff --git a/framework/cli/tests/test_module_css_subdir_sources.py b/framework/cli/tests/test_module_css_subdir_sources.py new file mode 100644 index 00000000..eee00af4 --- /dev/null +++ b/framework/cli/tests/test_module_css_subdir_sources.py @@ -0,0 +1,47 @@ +"""uv's .venv/.gitignore hides subdirectories from Tailwind, so each gets its own @source (#419).""" + +from __future__ import annotations + +from simple_module_hosting.assets import ModuleAssets, render_modules_css + + +def _wheel(tmp_path, **overrides): + defaults = { + "name": "Pagebuilder", + "package_name": "pagebuilder", + "package_dir": tmp_path / "pagebuilder", + "pages_dir": None, + "theme_css": None, + "styles_css": None, + } + return ModuleAssets(**{**defaults, **overrides}) + + +def test_each_subdirectory_with_sources_gets_its_own_line(tmp_path): + components = tmp_path / "pagebuilder" / "components" + (components / "widgets" / "quote").mkdir(parents=True) + (components / "Root.tsx").write_text("") + (components / "widgets" / "Gallery.tsx").write_text("") + (components / "widgets" / "quote" / "Quote.tsx").write_text("") + (components / "empty").mkdir() + (components / "__pycache__").mkdir() + (components / "__pycache__" / "x.ts").write_text("") + + css = render_modules_css( + [_wheel(tmp_path, components_dir=components)], in_repo=lambda _p: False + ) + + lines = [line for line in css.splitlines() if line.startswith("@source")] + assert lines == [ + f'@source "{components.as_posix()}/**/*.{{ts,tsx}}";', + f'@source "{(components / "widgets").as_posix()}/**/*.{{ts,tsx}}";', + f'@source "{(components / "widgets" / "quote").as_posix()}/**/*.{{ts,tsx}}";', + ] + + +def test_in_repo_module_subdirectories_get_nothing(tmp_path): + components = tmp_path / "local" / "components" + (components / "widgets").mkdir(parents=True) + (components / "widgets" / "A.tsx").write_text("") + css = render_modules_css([_wheel(tmp_path, components_dir=components)], in_repo=lambda _p: True) + assert "@source" not in css diff --git a/framework/hosting/simple_module_hosting/assets.py b/framework/hosting/simple_module_hosting/assets.py index 7c3f1b28..0dff2b73 100644 --- a/framework/hosting/simple_module_hosting/assets.py +++ b/framework/hosting/simple_module_hosting/assets.py @@ -171,6 +171,29 @@ def framework_assets() -> ModuleAssets: */""" +_SKIP_DIRS = frozenset({"__pycache__", "node_modules"}) + + +def _source_dirs(base: Path) -> list[Path]: + """``base`` plus every descendant directory that directly holds a .ts/.tsx file. + + uv writes ``.venv/.gitignore`` = ``*`` and Tailwind's scanner honours ignore + files *below* an ``@source`` base, so a wheel module's base directory was + scanned but none of its subdirectories (#419). Each one becomes its own + base. A directory that doesn't exist yields just ``base``. + """ + found: set[Path] = set() + if base.is_dir(): + for path in base.rglob("*.ts*"): + if ( + path.suffix in {".ts", ".tsx"} + and path.parent != base + and not _SKIP_DIRS.intersection(path.relative_to(base).parts) + ): + found.add(path.parent) + return [base, *sorted(found)] + + def render_modules_css( assets: Sequence[ModuleAssets], *, @@ -193,14 +216,19 @@ def render_modules_css( every widget class on Windows. Emitting the resolved absolute path here works on both. See GH #258. + Each subdirectory holding sources gets its own ``@source`` too, because + uv's ``.venv/.gitignore`` hides everything below a base from Tailwind + (GH #419). + Every path is absolute, so nothing here depends on the host's ``vite.config.ts`` — see the module docstring for why that matters. """ source_lines = [ - f'@source "{d.as_posix()}/**/*.{{ts,tsx}}";' + f'@source "{s.as_posix()}/**/*.{{ts,tsx}}";' for e in assets for d in (e.pages_dir, e.components_dir) if d and not in_repo(d) + for s in _source_dirs(d) ] theme_lines = [f'@import "{e.theme_css.as_posix()}";' for e in assets if e.theme_css] style_lines = [ From 1813c0c282a76869d98d981af58ed21ff8662ece Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:40:17 +0200 Subject: [PATCH 15/30] fix(ui): brand foreground ink by WCAG contrast; raise aside footer contrast (#414, #420) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../ui/src/components/BrandingHead.test.tsx | 13 ++++++ packages/ui/src/components/BrandingHead.tsx | 4 +- packages/ui/src/components/BrandingMark.tsx | 4 +- packages/ui/src/layouts/sidebar-theme.ts | 2 +- packages/ui/src/lib/color.test.ts | 27 ++++++++++++- packages/ui/src/lib/color.ts | 40 ++++++++++++++++++- packages/ui/src/styles/globals.css | 3 +- 7 files changed, 87 insertions(+), 6 deletions(-) diff --git a/packages/ui/src/components/BrandingHead.test.tsx b/packages/ui/src/components/BrandingHead.test.tsx index 1330bbbc..603b5f97 100644 --- a/packages/ui/src/components/BrandingHead.test.tsx +++ b/packages/ui/src/components/BrandingHead.test.tsx @@ -16,6 +16,8 @@ beforeEach(() => { document.documentElement.style.removeProperty('--primary'); document.documentElement.style.removeProperty('--sidebar-primary'); document.documentElement.style.removeProperty('--color-primary-600'); + document.documentElement.style.removeProperty('--primary-foreground'); + document.documentElement.style.removeProperty('--sidebar-primary-foreground'); }); afterEach(() => cleanup()); @@ -47,6 +49,17 @@ describe('BrandingHead', () => { expect(document.documentElement.style.getPropertyValue('--color-primary-600')).toBe(''); }); + test('picks a dark foreground ink for a light brand and clears it on unmount (#420)', () => { + state.branding = { appName: 'Acme', primaryColor: '#62B8E2', logoUrl: null, faviconUrl: null }; + const { unmount } = render(); + const style = document.documentElement.style; + expect(style.getPropertyValue('--primary-foreground')).toBe('oklch(0.2 0.02 250)'); + expect(style.getPropertyValue('--sidebar-primary-foreground')).toBe('oklch(0.2 0.02 250)'); + unmount(); + expect(style.getPropertyValue('--primary-foreground')).toBe(''); + expect(style.getPropertyValue('--sidebar-primary-foreground')).toBe(''); + }); + test('keeps the server-rendered theme-color meta in sync, restoring on unmount', () => { const meta = document.createElement('meta'); meta.setAttribute('name', 'theme-color'); diff --git a/packages/ui/src/components/BrandingHead.tsx b/packages/ui/src/components/BrandingHead.tsx index 3f512b51..f7c422ca 100644 --- a/packages/ui/src/components/BrandingHead.tsx +++ b/packages/ui/src/components/BrandingHead.tsx @@ -9,7 +9,9 @@ import type { SharedProps } from '../types'; * * - the favicon `` when a custom favicon is set, * - the primary brand colour — derived into the full `--color-primary-*` ramp - * (plus base `--primary` / `--sidebar-primary`) and written as inline CSS + * (plus base `--primary` / `--sidebar-primary`, and the matching + * `--primary-foreground` / `--sidebar-primary-foreground` ink chosen by WCAG + * contrast) and written as inline CSS * variables on `:root`. Inline wins over the stylesheet's `:root`/`.dark` * rules, so every Tailwind `primary` utility — solid buttons *and* the * `primary-600/700/800` gradient tints used by the brand badge — follows the diff --git a/packages/ui/src/components/BrandingMark.tsx b/packages/ui/src/components/BrandingMark.tsx index b418501a..552fc6df 100644 --- a/packages/ui/src/components/BrandingMark.tsx +++ b/packages/ui/src/components/BrandingMark.tsx @@ -65,7 +65,9 @@ export function BrandingMark({ /> ) : (
- {initial} + + {initial} +
)} {caption ? ( diff --git a/packages/ui/src/layouts/sidebar-theme.ts b/packages/ui/src/layouts/sidebar-theme.ts index fe0e2098..d0a73d05 100644 --- a/packages/ui/src/layouts/sidebar-theme.ts +++ b/packages/ui/src/layouts/sidebar-theme.ts @@ -33,7 +33,7 @@ export const DEFAULT_SIDEBAR_THEME: Omit = { // Solid pill, per the deck: a tinted row with a left rule read as a hover // state next to the near-black surface, and lost the current page at a // glance on a phone. - activeClass: 'bg-primary text-white', + activeClass: 'bg-primary text-primary-foreground', inactiveClass: 'text-app-sidebar-text hover:bg-app-sidebar-hover hover:text-white', mutedTextClass: 'text-app-sidebar-text-muted', }; diff --git a/packages/ui/src/lib/color.test.ts b/packages/ui/src/lib/color.test.ts index b190bf06..c4931880 100644 --- a/packages/ui/src/lib/color.test.ts +++ b/packages/ui/src/lib/color.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from 'vitest'; -import { BASE_PRIMARY_RAMP, deriveBrandRamp, hexToOklch } from './color'; +import { BASE_PRIMARY_RAMP, contrastRatio, deriveBrandRamp, hexToOklch } from './color'; describe('hexToOklch', () => { test('white is light and (near) achromatic', () => { @@ -60,3 +60,28 @@ describe('deriveBrandRamp', () => { expect(deriveBrandRamp('not-a-color')).toBeNull(); }); }); + +const DARK_INK = 'oklch(0.2 0.02 250)'; +const WHITE = 'oklch(1 0 0)'; + +describe('brand foreground ink (#420)', () => { + test.each([ + ['#62B8E2', DARK_INK], + ['#9AD3EF', DARK_INK], + ['#2E6DB0', WHITE], + ['#16276E', WHITE], + ])('%s gets %s', (hex, ink) => { + const ramp = deriveBrandRamp(hex); + expect(ramp?.['--primary-foreground']).toBe(ink); + expect(ramp?.['--sidebar-primary-foreground']).toBe(ink); + }); + + test('white is rejected on light brands because it fails WCAG AA', () => { + expect(contrastRatio('#62B8E2', '#ffffff')).toBeLessThan(4.5); + }); + + test('contrastRatio is 21 for black on white and 1 for unparseable input', () => { + expect(contrastRatio('#000000', '#ffffff')).toBeCloseTo(21, 5); + expect(contrastRatio('nope', '#ffffff')).toBe(1); + }); +}); diff --git a/packages/ui/src/lib/color.ts b/packages/ui/src/lib/color.ts index 09b93140..c8fe497c 100644 --- a/packages/ui/src/lib/color.ts +++ b/packages/ui/src/lib/color.ts @@ -91,11 +91,44 @@ function oklchString({ l, c, h }: Oklch): string { return `oklch(${round(l, 4)} ${round(c, 4)} ${round(h, 2)})`; } +/** WCAG 2.x relative luminance of an `#rrggbb` colour (null if unparseable). */ +function relativeLuminance(hex: string): number | null { + const lin = parseHex(hex); + if (!lin) return null; + const [r, g, b] = lin; + return 0.2126 * r + 0.7152 * g + 0.0722 * b; +} + +/** WCAG contrast ratio between two `#rrggbb` colours (1-21; 1 when either is unparseable). */ +export function contrastRatio(a: string, b: string): number { + const la = relativeLuminance(a); + const lb = relativeLuminance(b); + if (la === null || lb === null) return 1; + const [hi, lo] = la > lb ? [la, lb] : [lb, la]; + return (hi + 0.05) / (lo + 0.05); +} + +const WHITE_INK = 'oklch(1 0 0)'; +const DARK_INK = 'oklch(0.2 0.02 250)'; +// DARK_INK converted to sRGB (OKLCH -> OKLab -> linear sRGB -> gamma); used only +// for the contrast ratio, never emitted as a colour. +const DARK_INK_HEX = '#0f171f'; +const AA_NORMAL_TEXT = 4.5; + +/** Text colour for a brand-coloured surface: white when it passes AA, else the higher-contrast ink. */ +function foregroundFor(hex: string): string { + const white = contrastRatio(hex, '#ffffff'); + if (white >= AA_NORMAL_TEXT) return WHITE_INK; + return contrastRatio(hex, DARK_INK_HEX) > white ? DARK_INK : WHITE_INK; +} + /** * Derive the CSS custom properties that re-theme the primary ramp to `hex`. * * Returns a map of `--color-primary-` → `oklch(…)` (plus the base - * `--primary` / `--sidebar-primary` set to the brand colour itself). Returns + * `--primary` / `--sidebar-primary` set to the brand colour itself, and + * `--primary-foreground` / `--sidebar-primary-foreground` set to white or a dark + * ink, whichever reads better on it). Returns * null for an unparseable colour so callers can leave the default theme intact. */ export function deriveBrandRamp(hex: string): Record | null { @@ -111,5 +144,10 @@ export function deriveBrandRamp(hex: string): Record | null { // precisely what the admin chose, while the ramp drives gradients/tints. vars['--primary'] = hex; vars['--sidebar-primary'] = hex; + // White text on a light brand colour fails AA (2.2:1 on #62B8E2, #420), so + // the ink follows the colour rather than staying the stylesheet's white. + const ink = foregroundFor(hex); + vars['--primary-foreground'] = ink; + vars['--sidebar-primary-foreground'] = ink; return vars; } diff --git a/packages/ui/src/styles/globals.css b/packages/ui/src/styles/globals.css index 163ddde3..1926275b 100644 --- a/packages/ui/src/styles/globals.css +++ b/packages/ui/src/styles/globals.css @@ -27,7 +27,8 @@ --color-landing-bg: oklch(0.13 0.02 250); --color-dark-text: oklch(0.9 0.005 250); --color-dark-text-muted: oklch(0.6 0.01 250); - --color-dark-text-subtle: oklch(0.45 0.01 250); + /* >=4.5:1 on the auth aside (#414); still a step lighter than muted. */ + --color-dark-text-subtle: oklch(0.62 0.01 250); --color-dark-border: oklch(0.35 0.01 250); --color-dark-border-hover: oklch(0.5 0.01 250); From 64b1f224f4f33937d6942ab1fe2dcf114ca398fb Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:43:00 +0200 Subject: [PATCH 16/30] fix(ui): keep the logo badge initial white on the ramp gradient (#420) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- packages/ui/src/components/BrandingMark.tsx | 5 ++--- packages/ui/src/styles/globals.css | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/ui/src/components/BrandingMark.tsx b/packages/ui/src/components/BrandingMark.tsx index 552fc6df..838b2511 100644 --- a/packages/ui/src/components/BrandingMark.tsx +++ b/packages/ui/src/components/BrandingMark.tsx @@ -65,9 +65,8 @@ export function BrandingMark({ /> ) : (
- - {initial} - + {/* Stays white: the gradient uses fixed-lightness ramp steps, so the brand-hex ink does not apply. */} + {initial}
)} {caption ? ( diff --git a/packages/ui/src/styles/globals.css b/packages/ui/src/styles/globals.css index 1926275b..8c3051e9 100644 --- a/packages/ui/src/styles/globals.css +++ b/packages/ui/src/styles/globals.css @@ -27,7 +27,7 @@ --color-landing-bg: oklch(0.13 0.02 250); --color-dark-text: oklch(0.9 0.005 250); --color-dark-text-muted: oklch(0.6 0.01 250); - /* >=4.5:1 on the auth aside (#414); still a step lighter than muted. */ + /* 5.5:1 on the auth aside (#03080f), above AA's 4.5 (#414). */ --color-dark-text-subtle: oklch(0.62 0.01 250); --color-dark-border: oklch(0.35 0.01 250); --color-dark-border-hover: oklch(0.5 0.01 250); From 49199d7c85b64b911cac1c34f98b9bb0e9af50c9 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:44:57 +0200 Subject: [PATCH 17/30] feat(ui): NativeSelect wrapperClassName so callers can size the control (#421) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../src/components/ui/native-select.test.tsx | 25 +++++++++++++++++++ .../ui/src/components/ui/native-select.tsx | 16 ++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) create mode 100644 packages/ui/src/components/ui/native-select.test.tsx diff --git a/packages/ui/src/components/ui/native-select.test.tsx b/packages/ui/src/components/ui/native-select.test.tsx new file mode 100644 index 00000000..959e6af2 --- /dev/null +++ b/packages/ui/src/components/ui/native-select.test.tsx @@ -0,0 +1,25 @@ +import { render } from '@testing-library/react'; +import { NativeSelect, NativeSelectOption } from './native-select'; + +describe('NativeSelect', () => { + it('puts wrapperClassName on the wrapper and className on the select (#421)', () => { + const { container } = render( + + {/* i18n-exempt: test fixture */} + A + , + ); + const wrapper = container.querySelector('[data-slot="native-select-wrapper"]'); + const select = container.querySelector('select'); + expect(wrapper?.className).toContain('w-full'); + expect(select?.className).toContain('text-xs'); + expect(select?.className).not.toContain('w-fit'); + }); + + it('keeps the wrapper w-fit by default', () => { + const { container } = render(); + expect(container.querySelector('[data-slot="native-select-wrapper"]')?.className).toContain( + 'w-fit', + ); + }); +}); diff --git a/packages/ui/src/components/ui/native-select.tsx b/packages/ui/src/components/ui/native-select.tsx index a7fed4a6..91922981 100644 --- a/packages/ui/src/components/ui/native-select.tsx +++ b/packages/ui/src/components/ui/native-select.tsx @@ -2,14 +2,26 @@ import { cn } from '@simple-module-py/ui/lib/utils'; import { ChevronDownIcon } from 'lucide-react'; import type * as React from 'react'; +/** + * `className` styles the ` Date: Fri, 9 Oct 2026 08:48:39 +0200 Subject: [PATCH 18/30] fix(admin): page titles, settings h1, admin errors inside the admin shell (#422) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- host/client_app/admin-pages-head.test.ts | 34 +++ host/client_app/pages/Error.test.tsx | 65 +++++- host/client_app/pages/Error.tsx | 6 + host/client_app/pages/ErrorShell.tsx | 35 +++ .../settings/settings/pages/ModulesEdit.tsx | 4 + .../settings/pages/components/ModuleForm.tsx | 4 +- .../settings/tests-js/ModulesEdit.test.tsx | 52 +++++ modules/users/users/pages/Users/AddPeople.tsx | 199 +++++++++--------- modules/users/users/pages/Users/Edit.tsx | 123 +++++------ modules/users/users/pages/Users/Index.tsx | 135 ++++++------ packages/ui/src/components/ErrorScreen.tsx | 11 +- tests/e2e/test_document_titles.py | 1 + tests/e2e/test_error_pages.py | 13 ++ 13 files changed, 454 insertions(+), 228 deletions(-) create mode 100644 host/client_app/admin-pages-head.test.ts create mode 100644 host/client_app/pages/ErrorShell.tsx create mode 100644 modules/settings/tests-js/ModulesEdit.test.tsx diff --git a/host/client_app/admin-pages-head.test.ts b/host/client_app/admin-pages-head.test.ts new file mode 100644 index 00000000..6aca0a53 --- /dev/null +++ b/host/client_app/admin-pages-head.test.ts @@ -0,0 +1,34 @@ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** Every page rendered in AdminLayout sets a document title (#422: /admin/users/ had none). */ +function pagesUnder(dir: string): string[] { + return readdirSync(dir).flatMap((name) => { + const path = join(dir, name); + if (statSync(path).isDirectory()) return pagesUnder(path); + return name.endsWith('.tsx') && !name.endsWith('.test.tsx') ? [path] : []; + }); +} + +const ROOT = join(__dirname, '..', '..'); +const moduleDirs = readdirSync(join(ROOT, 'modules')).map((m) => + join(ROOT, 'modules', m, m, 'pages'), +); +const files = moduleDirs + .filter((d) => { + try { + return statSync(d).isDirectory(); + } catch { + return false; + } + }) + .flatMap(pagesUnder) + .filter((f) => /\.layout\s*=\s*\[\s*AdminLayout/.test(readFileSync(f, 'utf8'))); + +describe('admin pages', () => { + it('found some', () => expect(files.length).toBeGreaterThan(5)); + it.each(files)('%s renders ', (file) => { + expect(readFileSync(file, 'utf8')).toMatch(/]*title=/); + }); +}); diff --git a/host/client_app/pages/Error.test.tsx b/host/client_app/pages/Error.test.tsx index 2d8bfc88..a3755420 100644 --- a/host/client_app/pages/Error.test.tsx +++ b/host/client_app/pages/Error.test.tsx @@ -13,6 +13,7 @@ import '@testing-library/jest-dom/vitest'; import { configureI18n } from '@simple-module-py/i18n'; import { render, screen } from '@testing-library/react'; +import type { ReactNode } from 'react'; import { describe, expect, test, vi } from 'vitest'; configureI18n({ @@ -38,21 +39,41 @@ configureI18n({ }, }); +const mocks = vi.hoisted(() => ({ + page: { url: '/', props: { auth: { user: null } } } as { + url: string; + props: Record; + }, +})); + vi.mock('@inertiajs/react', () => ({ Head: () => null, Link: ({ children, ...rest }: { children?: unknown }) => {children as never}, router: { visit: vi.fn(), reload: vi.fn() }, - // Only `auth` comes from the page props here — the rest are ordinary + // Only shared props come from the page here — the rest are ordinary // component props, which is what Inertia passes a page component. - usePage: () => ({ props: { auth: { user: null } } }), + usePage: () => mocks.page, +})); + +vi.mock('@simple-module-py/ui/layouts/AdminLayout', () => ({ + AdminLayout: ({ children }: { children?: ReactNode }) => ( +
{children}
+ ), })); const { default: ErrorPage } = await import('./Error'); function renderError(props: Record) { - render(); + const layout = (ErrorPage as unknown as { layout: (page: ReactNode) => ReactNode }).layout; + render(<>{layout()}); +} + +function setPage(url: string, props: Record) { + mocks.page = { url, props }; } +const ADMIN_MENUS = { menus: { adminSidebar: [{ label: 'Users', url: '/admin/users/' }] } }; + describe('a 403 from a permission guard', () => { test('it names the permission, so the reader knows what to ask for', () => { renderError({ status: 403, message: '', required_permission: 'settings.manage' }); @@ -105,3 +126,41 @@ describe('a 403 with no single permission to name', () => { expect(screen.getByText('This workspace is read-only.')).toBeInTheDocument(); }); }); + +describe('an error under /admin', () => { + test('renders inside AdminLayout for a signed-in admin', () => { + setPage('/admin/users/999999', { auth: { isAuthenticated: true }, ...ADMIN_MENUS }); + renderError({ status: 404, message: '' }); + + expect(screen.getByTestId('admin-layout')).toBeInTheDocument(); + expect(screen.getByText('That page is not here.')).toBeInTheDocument(); + }); + + test('renders bare for an anonymous 401', () => { + setPage('/admin/users/', { auth: { isAuthenticated: false }, ...ADMIN_MENUS }); + renderError({ status: 401, message: '' }); + + expect(screen.queryByTestId('admin-layout')).not.toBeInTheDocument(); + }); + + test('renders bare outside /admin, matching whole path segments', () => { + setPage('/administer', { auth: { isAuthenticated: true }, ...ADMIN_MENUS }); + renderError({ status: 404, message: '' }); + + expect(screen.queryByTestId('admin-layout')).not.toBeInTheDocument(); + }); + + test('renders bare when the admin menu is empty', () => { + setPage('/admin/users/', { auth: { isAuthenticated: true }, menus: { adminSidebar: [] } }); + renderError({ status: 403, message: '' }); + + expect(screen.queryByTestId('admin-layout')).not.toBeInTheDocument(); + }); + + test('renders bare, without throwing, when shared props never arrived', () => { + setPage('/admin/users/', {}); + renderError({ status: 500, message: '' }); + + expect(screen.queryByTestId('admin-layout')).not.toBeInTheDocument(); + }); +}); diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 013215d0..72f4b474 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -6,6 +6,7 @@ import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedTe import { Button } from '@simple-module-py/ui/components/ui/button'; import type { SharedProps } from '@simple-module-py/ui/types'; import type { ReactNode } from 'react'; +import { ErrorShell, useInAdminShell } from './ErrorShell'; interface Props { status: number; @@ -121,6 +122,7 @@ function ErrorPage({ const { t } = useT(); const { auth } = usePage<{ props: SharedProps }>().props as unknown as SharedProps; const copy = useStatusCopy(status, Boolean(maintenance)); + const inline = useInAdminShell(); // Three sources, most specific first. A named permission beats everything: // it is the only wording that tells the reader what to go and ask for, and @@ -160,6 +162,7 @@ function ErrorPage({ title={copy.title} description={description} accent={copy.accent} + inline={inline} details={ // Only where it is actionable. The id is the handle on a server // failure worth reporting; on a 403 or a 404 it is a hex string @@ -209,4 +212,7 @@ function ErrorPage({ ); } +// An admin who hits a 404/422 under /admin stays in the admin shell (#422). +ErrorPage.layout = (page: ReactNode) => {page}; + export default ErrorPage; diff --git a/host/client_app/pages/ErrorShell.tsx b/host/client_app/pages/ErrorShell.tsx new file mode 100644 index 00000000..1b533d81 --- /dev/null +++ b/host/client_app/pages/ErrorShell.tsx @@ -0,0 +1,35 @@ +import { usePage } from '@inertiajs/react'; +import { AdminLayout } from '@simple-module-py/ui/layouts/AdminLayout'; +import type { SharedProps } from '@simple-module-py/ui/types'; +import { createContext, type ReactNode, useContext } from 'react'; + +/** `/admin` or anything under `/admin/` — not `/administer`. */ +export function isAdminPath(url: string): boolean { + const path = url.split(/[?#]/, 1)[0]; + return path === '/admin' || path.startsWith('/admin/'); +} + +const InShell = createContext(false); +export const useInAdminShell = () => useContext(InShell); + +/** + * Keeps an admin inside the admin shell when a page under /admin fails (#422). + * Only for a signed-in viewer whose admin menu reached the page: an anonymous + * 401/419 has no admin to keep in context, and a 500 raised before the shared + * props were built has no menu to render. + */ +export function ErrorShell({ children }: { children: ReactNode }) { + const page = usePage(); + const props = page.props as unknown as Partial; + const inShell = + isAdminPath(page.url) && + Boolean(props.auth?.isAuthenticated) && + Array.isArray(props.menus?.adminSidebar) && + props.menus.adminSidebar.length > 0; + if (!inShell) return <>{children}; + return ( + + {children} + + ); +} diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 996d0c41..18968fc5 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -110,6 +110,10 @@ function ModulesEdit({ modules, testable = {} }: Props) { {/* `pb-10` rather than a fade: the fields list ends on a full row with breathing room under it, not half a row cut off by the pane edge. */}
+ {/* The page's heading for assistive tech. The visible heading is + the selected module's own, in ModuleForm, which is an h2 so this + screen has exactly one h1 (#422). */} +

{t(keys.settings.modules.title)}

{current?.manage_url ? ( {/* No second editor for these fields — the module's own page is diff --git a/modules/settings/settings/pages/components/ModuleForm.tsx b/modules/settings/settings/pages/components/ModuleForm.tsx index 6478b8c8..33697ef1 100644 --- a/modules/settings/settings/pages/components/ModuleForm.tsx +++ b/modules/settings/settings/pages/components/ModuleForm.tsx @@ -117,10 +117,10 @@ export function ModuleForm({ module: m, checks = [] }: Props) { card for the fields alone. */}
-

+

{m.package}{' '} {t(keys.settings.modules.heading_suffix)} -

+

{t(keys.settings.modules.description)}

diff --git a/modules/settings/tests-js/ModulesEdit.test.tsx b/modules/settings/tests-js/ModulesEdit.test.tsx new file mode 100644 index 00000000..1663887f --- /dev/null +++ b/modules/settings/tests-js/ModulesEdit.test.tsx @@ -0,0 +1,52 @@ +import '@testing-library/jest-dom/vitest'; +import { configureI18n } from '@simple-module-py/i18n'; +import { render, screen } from '@testing-library/react'; +import { describe, expect, test, vi } from 'vitest'; + +configureI18n({ + locale: 'en', + messages: { + 'settings.modules.title': 'Module Settings', + 'settings.modules.head_title': 'Module settings', + 'settings.modules.search_placeholder': 'Search modules…', + 'settings.modules.empty_title': 'No modules with settings', + 'settings.modules.managed_title': 'Managed on its own page', + }, +}); + +vi.mock('@inertiajs/react', () => ({ + Head: () => null, + Link: ({ children, ...rest }: { children?: unknown }) => {children as never}, + router: { reload: vi.fn() }, +})); + +vi.mock('@simple-module-py/ui/layouts/AdminLayout', () => ({ AdminLayout: () => null })); + +import ModulesEdit from '../settings/pages/ModulesEdit'; + +describe('ModulesEdit', () => { + test('has one level-1 heading naming the screen, even with no modules', () => { + render(); + + const h1 = screen.getAllByRole('heading', { level: 1 }); + expect(h1).toHaveLength(1); + expect(h1[0]).toHaveTextContent('Module Settings'); + }); + + test('keeps the single h1 when a module is managed elsewhere', () => { + render( + , + ); + + expect(screen.getAllByRole('heading', { level: 1 })).toHaveLength(1); + }); +}); diff --git a/modules/users/users/pages/Users/AddPeople.tsx b/modules/users/users/pages/Users/AddPeople.tsx index 398b968b..774931ec 100644 --- a/modules/users/users/pages/Users/AddPeople.tsx +++ b/modules/users/users/pages/Users/AddPeople.tsx @@ -1,4 +1,4 @@ -import { Link, router, usePage } from '@inertiajs/react'; +import { Head, Link, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { InlineBanner } from '@simple-module-py/ui/components/InlineBanner'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; @@ -180,110 +180,117 @@ function AddPeople() { : t(keys.users.add_people.submit_create); return ( - - { - setMode(next); - setError(null); - }} - aria-label={t(keys.users.add_people.tablist_label)} - options={[ - { value: 'invite', label: t(keys.users.add_people.mode_invite) }, - { value: 'create', label: t(keys.users.add_people.mode_create) }, - ]} - className="mb-4" - /> + <> + + + { + setMode(next); + setError(null); + }} + aria-label={t(keys.users.add_people.tablist_label)} + options={[ + { value: 'invite', label: t(keys.users.add_people.mode_invite) }, + { value: 'create', label: t(keys.users.add_people.mode_create) }, + ]} + className="mb-4" + /> - {!mailerDelivers && ( - // One sentence, as the deck has it — a title/body pair reads as two - // separate facts when it is really one: which mailer, and what that - // means for the links. The "Configure SMTP" link ends the sentence - // rather than sitting right-aligned away from what it refers to. - - {t(keys.users.add_people.mailer_banner_prefix)} {mailerName}{' '} - {t(keys.users.add_people.mailer_banner_suffix)}{' '} - {/* The 44px tap area is grown with a pseudo-element: an inline + {!mailerDelivers && ( + // One sentence, as the deck has it — a title/body pair reads as two + // separate facts when it is really one: which mailer, and what that + // means for the links. The "Configure SMTP" link ends the sentence + // rather than sitting right-aligned away from what it refers to. + + {t(keys.users.add_people.mailer_banner_prefix)} {mailerName}{' '} + {t(keys.users.add_people.mailer_banner_suffix)}{' '} + {/* The 44px tap area is grown with a pseudo-element: an inline link inside a sentence cannot take `min-h-11` without stretching the line box around it. */} - - {t(keys.users.add_people.configure_smtp)} - - - } - /> - )} + + {t(keys.users.add_people.configure_smtp)} + + + } + /> + )} - {/* "Last batch" belongs to the invite flow; creating one account with a + {/* "Last batch" belongs to the invite flow; creating one account with a password you set has no batch to report. */} -
- - -
- {mode === 'invite' ? ( - - ) : ( - <> - + + + + {mode === 'invite' ? ( + - - - )} + ) : ( + <> + + + + )} - {error &&

{error}

} + {error &&

{error}

} -
- - -
- - - +
+ + +
+ +
+ - {mode === 'invite' && ( - - )} -
- + {mode === 'invite' && ( + + )} +
+ + ); } diff --git a/modules/users/users/pages/Users/Edit.tsx b/modules/users/users/pages/Users/Edit.tsx index 4b826ef0..29503218 100644 --- a/modules/users/users/pages/Users/Edit.tsx +++ b/modules/users/users/pages/Users/Edit.tsx @@ -1,4 +1,4 @@ -import { usePage } from '@inertiajs/react'; +import { Head, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { useRelativeTime } from '@simple-module-py/ui/hooks/use-relative-time'; @@ -57,68 +57,71 @@ function Edit() { const actions = useUserActions(user.id, user.is_active, user.is_verified); return ( - - {initials(user.full_name, user.email)} - - } - badge={ - - } - description={t(keys.users.edit.subtitle, { - email: user.email, - joined: joinedMonth(user.created_at), - lastLogin: user.last_login_at ? ago(user.last_login_at) : t(keys.users.edit.never), - })} - actions={ - - } - > -
- form.setForm((prev) => ({ ...prev, email }))} - onFullNameChange={(fullName) => form.setForm((prev) => ({ ...prev, fullName }))} - roles={roles} - selectedRoles={form.form.roles} - onToggleRole={form.toggleRole} - userId={user.id} - hasPermissionsModule={has_permissions_module} - error={form.error} - /> + <> + + + {initials(user.full_name, user.email)} + + } + badge={ + + } + description={t(keys.users.edit.subtitle, { + email: user.email, + joined: joinedMonth(user.created_at), + lastLogin: user.last_login_at ? ago(user.last_login_at) : t(keys.users.edit.never), + })} + actions={ + + } + > +
+ form.setForm((prev) => ({ ...prev, email }))} + onFullNameChange={(fullName) => form.setForm((prev) => ({ ...prev, fullName }))} + roles={roles} + selectedRoles={form.form.roles} + onToggleRole={form.toggleRole} + userId={user.id} + hasPermissionsModule={has_permissions_module} + error={form.error} + /> - + - {/* Absent, not empty, when the deployment records nothing. */} - {recentActivity !== null && ( - - )} + {/* Absent, not empty, when the deployment records nothing. */} + {recentActivity !== null && ( + + )} - -
- + +
+ + ); } diff --git a/modules/users/users/pages/Users/Index.tsx b/modules/users/users/pages/Users/Index.tsx index 71147b70..ea364421 100644 --- a/modules/users/users/pages/Users/Index.tsx +++ b/modules/users/users/pages/Users/Index.tsx @@ -1,4 +1,4 @@ -import { Link, router, usePage } from '@inertiajs/react'; +import { Head, Link, router, usePage } from '@inertiajs/react'; import { keys, useT } from '@simple-module-py/i18n'; import { PageShell } from '@simple-module-py/ui/components/PageShell'; import { SegmentedControl } from '@simple-module-py/ui/components/SegmentedControl'; @@ -109,77 +109,80 @@ function Index() { }, [navigate]); return ( - - - - {t(keys.users.index.add_people)} - - - } - > - - -
- + + + + + {t(keys.users.index.add_people)} + + + } + > + - {view === 'users' && ( - <> -
- - setSearch(e.target.value)} - className="pl-9 max-lg:min-h-11" + +
+ + {view === 'users' && ( + <> +
+ + setSearch(e.target.value)} + className="pl-9 max-lg:min-h-11" + /> +
+ r.name)} + onChange={(next) => navigate(next)} /> -
- + )} +
+ + {view === 'users' ? ( + <> + {pagination.total === 1 && !isFiltered && } + r.name)} - onChange={(next) => navigate(next)} + page={pagination.page} + perPage={pagination.per_page} + total={pagination.total} + filtered={isFiltered} + onSort={toggleSort} + onPage={(page) => navigate({ page })} + onClearFilters={clearFilters} /> + ) : ( + )} -
- - {view === 'users' ? ( - <> - {pagination.total === 1 && !isFiltered && } - navigate({ page })} - onClearFilters={clearFilters} - /> - - ) : ( - - )} - + + ); } diff --git a/packages/ui/src/components/ErrorScreen.tsx b/packages/ui/src/components/ErrorScreen.tsx index 9bf8b750..33b3c889 100644 --- a/packages/ui/src/components/ErrorScreen.tsx +++ b/packages/ui/src/components/ErrorScreen.tsx @@ -9,6 +9,8 @@ interface Props { children: ReactNode; /** Visual accent for the numeral (403=warning, 404=primary, 5xx=destructive). */ accent?: 'primary' | 'warning' | 'destructive'; + /** Inside an app shell: no full-viewport frame or page background (#422). */ + inline?: boolean; } /** @@ -30,9 +32,16 @@ export function ErrorScreen({ details, children, accent = 'primary', + inline = false, }: Props) { return ( -
+

None: + """A missing admin record keeps the admin in the admin layout (#422).""" + _login(page, e2e_username, e2e_password) + response = page.goto("/admin/users/00000000-0000-0000-0000-000000000000") + assert response is not None + assert response.status == 404 + expect(page.get_by_role("heading", level=1)).to_be_visible(timeout=10_000) + # The admin panel badge links to /admin; the bare public error page has none. + expect(page.locator("a[href='/admin']").first).to_be_attached() From 9f145d6baa3318f0e2e5bc8cafe2e4ff4d43bcbe Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:51:05 +0200 Subject: [PATCH 19/30] fix(admin): visible settings heading; ErrorShell out of pages/ (#422) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- host/client_app/{pages => components}/ErrorShell.tsx | 0 host/client_app/pages/Error.tsx | 2 +- modules/settings/settings/pages/ModulesEdit.tsx | 9 +++++---- modules/settings/tests-js/ModulesEdit.test.tsx | 2 ++ 4 files changed, 8 insertions(+), 5 deletions(-) rename host/client_app/{pages => components}/ErrorShell.tsx (100%) diff --git a/host/client_app/pages/ErrorShell.tsx b/host/client_app/components/ErrorShell.tsx similarity index 100% rename from host/client_app/pages/ErrorShell.tsx rename to host/client_app/components/ErrorShell.tsx diff --git a/host/client_app/pages/Error.tsx b/host/client_app/pages/Error.tsx index 72f4b474..d2f12ccd 100644 --- a/host/client_app/pages/Error.tsx +++ b/host/client_app/pages/Error.tsx @@ -6,7 +6,7 @@ import { InterpolatedText } from '@simple-module-py/ui/components/InterpolatedTe import { Button } from '@simple-module-py/ui/components/ui/button'; import type { SharedProps } from '@simple-module-py/ui/types'; import type { ReactNode } from 'react'; -import { ErrorShell, useInAdminShell } from './ErrorShell'; +import { ErrorShell, useInAdminShell } from '../components/ErrorShell'; interface Props { status: number; diff --git a/modules/settings/settings/pages/ModulesEdit.tsx b/modules/settings/settings/pages/ModulesEdit.tsx index 18968fc5..23cdefc0 100644 --- a/modules/settings/settings/pages/ModulesEdit.tsx +++ b/modules/settings/settings/pages/ModulesEdit.tsx @@ -110,10 +110,11 @@ function ModulesEdit({ modules, testable = {} }: Props) { {/* `pb-10` rather than a fade: the fields list ends on a full row with breathing room under it, not half a row cut off by the pane edge. */}

- {/* The page's heading for assistive tech. The visible heading is - the selected module's own, in ModuleForm, which is an h2 so this - screen has exactly one h1 (#422). */} -

{t(keys.settings.modules.title)}

+ {/* One h1 per screen (#422); the selected module's own heading in + ModuleForm is an h2 beneath it. */} +

+ {t(keys.settings.modules.title)} +

{current?.manage_url ? ( {/* No second editor for these fields — the module's own page is diff --git a/modules/settings/tests-js/ModulesEdit.test.tsx b/modules/settings/tests-js/ModulesEdit.test.tsx index 1663887f..9efeec58 100644 --- a/modules/settings/tests-js/ModulesEdit.test.tsx +++ b/modules/settings/tests-js/ModulesEdit.test.tsx @@ -31,6 +31,8 @@ describe('ModulesEdit', () => { const h1 = screen.getAllByRole('heading', { level: 1 }); expect(h1).toHaveLength(1); expect(h1[0]).toHaveTextContent('Module Settings'); + // Visible, like the other admin pages' headings (#422), not screen-reader-only. + expect(h1[0]).not.toHaveClass('sr-only'); }); test('keeps the single h1 when a module is managed elsewhere', () => { From b5a5106f414c7b5d5c9c93c035e7529ee34fe25d Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 08:53:45 +0200 Subject: [PATCH 20/30] docs: ruff-format plan snippets Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../plans/2026-10-09-issue-batch-413-424.md | 108 ++++++++++++------ 1 file changed, 75 insertions(+), 33 deletions(-) diff --git a/docs/superpowers/plans/2026-10-09-issue-batch-413-424.md b/docs/superpowers/plans/2026-10-09-issue-batch-413-424.md index bfa7fa5b..91e511be 100644 --- a/docs/superpowers/plans/2026-10-09-issue-batch-413-424.md +++ b/docs/superpowers/plans/2026-10-09-issue-batch-413-424.md @@ -59,7 +59,11 @@ from simple_module_hosting.middleware import TenantMiddleware, TenantResolution def _scope(resolver=None, user=None): scope = { - "type": "http", "method": "GET", "path": "/", "headers": [], "state": {}, + "type": "http", + "method": "GET", + "path": "/", + "headers": [], + "state": {}, "app": SimpleNamespace(state=SimpleNamespace(tenant_resolver=resolver)), } if user is not None: @@ -75,10 +79,13 @@ async def _run(scope, vary_lines: list[bytes]): sent: list[dict] = [] async def inner(scope, receive, send): - await send({ - "type": "http.response.start", "status": 200, - "headers": [(b"vary", v) for v in vary_lines], - }) + await send( + { + "type": "http.response.start", + "status": 200, + "headers": [(b"vary", v) for v in vary_lines], + } + ) async def send(message): sent.append(message) @@ -90,6 +97,7 @@ async def _run(scope, vary_lines: list[bytes]): def _resolver(result): async def resolve(_request): return result + return resolve @@ -196,13 +204,11 @@ Replace the body of `_vary_sender.wrapped`: In `_resolve`, change the claim branch to: ```python - if user is not None: - claim = getattr(user, "tenant_id", None) - # The claim came from whichever credential authenticated the - # request, so the answer varies with it (#418). - return TenantResolution( - claim, "claim" if claim is not None else None, _CREDENTIAL_VARY - ) +if user is not None: + claim = getattr(user, "tenant_id", None) + # The claim came from whichever credential authenticated the + # request, so the answer varies with it (#418). + return TenantResolution(claim, "claim" if claim is not None else None, _CREDENTIAL_VARY) ``` Update the `TenantMiddleware` docstring sentence on `request.state` to mention `tenant_vary`. @@ -260,7 +266,11 @@ import pytest from fastapi import APIRouter, Depends, HTTPException, Request from simple_module_db import DEFAULT_TENANT_ID, current_tenant_id from simple_module_hosting.tenancy import ( - TenancyMode, require_tenant, single_tenant_id, tenancy_mode, tenant_vary, + TenancyMode, + require_tenant, + single_tenant_id, + tenancy_mode, + tenant_vary, ) @@ -394,8 +404,12 @@ from simple_module_core.tenancy import TenancyMode from simple_module_db import DEFAULT_TENANT_ID, is_valid_tenant_id, tenant_context __all__ = [ - "TenancyMode", "mode_for", "require_tenant", "single_tenant_id", - "tenancy_mode", "tenant_vary", + "TenancyMode", + "mode_for", + "require_tenant", + "single_tenant_id", + "tenancy_mode", + "tenant_vary", ] @@ -541,15 +555,18 @@ Use the fixture pattern from `test_query_filters.py` (`init_db("sqlite+aiosqlite COUNT_SHAPES = { "entity+count": lambda: ( select(_Tag, func.count(_TagLink.article_id)) - .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id).group_by(_Tag.id) + .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.id) ), "columns+count, select_from": lambda: ( - select(_Tag.name, func.count(_TagLink.article_id)).select_from(_Tag) - .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id).group_by(_Tag.name) + select(_Tag.name, func.count(_TagLink.article_id)) + .select_from(_Tag) + .outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.name) ), "entity+entity": lambda: select(_Tag, _TagLink).outerjoin(_TagLink, _TagLink.tag_id == _Tag.id), - "entity+column": lambda: ( - select(_Tag, _TagLink.article_id).outerjoin(_TagLink, _TagLink.tag_id == _Tag.id) + "entity+column": lambda: select(_Tag, _TagLink.article_id).outerjoin( + _TagLink, _TagLink.tag_id == _Tag.id ), } @@ -574,23 +591,34 @@ async def test_outer_join_entity_count_never_sees_other_tenants(seeded): async def test_inner_join_still_filters_the_child(seeded): - stmt = (select(_Tag.name, func.count(_TagLink.article_id)) - .join(_TagLink, _TagLink.tag_id == _Tag.id).group_by(_Tag.name)) + stmt = ( + select(_Tag.name, func.count(_TagLink.article_id)) + .join(_TagLink, _TagLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ) assert dict((await seeded.execute(stmt)).all()) == {"used": 1} async def test_raw_table_outer_join_stays_filtered(seeded): """A Core table target gets no ON criteria, so it keeps the WHERE predicate (no leak).""" link = _TagLink.__table__ - stmt = (select(_Tag.name, func.count(link.c.article_id)).select_from(_Tag) - .outerjoin(link, link.c.tag_id == _Tag.id).group_by(_Tag.name)) + stmt = ( + select(_Tag.name, func.count(link.c.article_id)) + .select_from(_Tag) + .outerjoin(link, link.c.tag_id == _Tag.id) + .group_by(_Tag.name) + ) rows = dict((await seeded.execute(stmt)).all()) assert rows.get("used") == 1 # never 4 (tenant B's links) async def test_soft_deleted_outer_child_keeps_the_parent(seeded): - stmt = (select(_Tag.name, func.count(_SoftLink.id)).select_from(_Tag) - .outerjoin(_SoftLink, _SoftLink.tag_id == _Tag.id).group_by(_Tag.name)) + stmt = ( + select(_Tag.name, func.count(_SoftLink.id)) + .select_from(_Tag) + .outerjoin(_SoftLink, _SoftLink.tag_id == _Tag.id) + .group_by(_Tag.name) + ) assert dict((await seeded.execute(stmt)).all()) == {"used": 1, "unused": 0} ``` @@ -671,7 +699,14 @@ from background_tasks.models import TaskExecution from users.models.refresh_token import RefreshToken _AWARE = { - TaskExecution: ("queued_at", "started_at", "finished_at", "heartbeat_at", "created_at", "updated_at"), + TaskExecution: ( + "queued_at", + "started_at", + "finished_at", + "heartbeat_at", + "created_at", + "updated_at", + ), RefreshToken: ("created_at", "expires_at", "revoked_at"), } @@ -735,13 +770,13 @@ When not recorded, the session is untouched and the provider gets `next_url=None import pytest CASES = [ - ({"Sec-Fetch-Dest": "image", "Sec-Fetch-Mode": "no-cors"}, None), # favicon + ({"Sec-Fetch-Dest": "image", "Sec-Fetch-Mode": "no-cors"}, None), # favicon ({"Sec-Fetch-Dest": "script", "Sec-Fetch-Mode": "no-cors"}, None), - ({"Sec-Fetch-Dest": "empty", "Sec-Fetch-Mode": "cors"}, None), # fetch() + ({"Sec-Fetch-Dest": "empty", "Sec-Fetch-Mode": "cors"}, None), # fetch() ({"Sec-Fetch-Dest": "document", "Sec-Fetch-Mode": "navigate"}, "/protected/page"), ({"Sec-Fetch-Mode": "navigate"}, "/protected/page"), ({"X-Inertia": "true", "Sec-Fetch-Dest": "empty", "Sec-Fetch-Mode": "cors"}, "/protected/page"), - ({}, "/protected/page"), # no metadata + ({}, "/protected/page"), # no metadata ] @@ -757,7 +792,9 @@ async def test_only_navigations_record_the_target(headers, expected): app = _build_app(_Recording()) app.add_middleware(AuthMiddleware) app.add_middleware(SessionMiddleware, secret_key=SECRET) - async with httpx.AsyncClient(transport=httpx.ASGITransport(app=app), base_url="http://test") as c: + async with httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url="http://test" + ) as c: resp = await c.get("/protected/page", headers=headers) assert resp.status_code == 302 assert seen == [expected] @@ -885,7 +922,10 @@ def _registry(tmp_path, overrides, *, audience="public", locales=("en",)): def test_nested_override_replaces_module_copy(tmp_path): reg = _registry(tmp_path, {"en": {"users": {"login": {"aside_heading": "Kuri for teams"}}}}) assert reg.messages("en")["users.login.aside_heading"] == "Kuri for teams" - assert reg.messages_snapshot("en", include_admin=False)["users.login.aside_heading"] == "Kuri for teams" + assert ( + reg.messages_snapshot("en", include_admin=False)["users.login.aside_heading"] + == "Kuri for teams" + ) def test_flat_dotted_override_works_too(tmp_path): @@ -1055,7 +1095,9 @@ def test_each_subdirectory_with_sources_gets_its_own_line(tmp_path): (components / "__pycache__").mkdir() (components / "__pycache__" / "x.ts").write_text("") - css = render_modules_css([_wheel(tmp_path, components_dir=components)], in_repo=lambda _p: False) + css = render_modules_css( + [_wheel(tmp_path, components_dir=components)], in_repo=lambda _p: False + ) lines = [line for line in css.splitlines() if line.startswith("@source")] assert lines == [ From 06dbbcc81e15129270c61d847bd81fa8aced35f5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 9 Oct 2026 09:04:37 +0200 Subject: [PATCH 21/30] =?UTF-8?q?docs:=20changelog=20for=20#413=E2=80=93#4?= =?UTF-8?q?24?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c80f5d4..8df3d958 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,19 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Added +- **Host override layer for module copy** (#415). A host ships + `host/locales/overrides/.json` (nested or flat dotted keys) and it is + applied after every module, framework and host catalog, so the server-side + `Translator`, menus and Inertia props all see it. An override only replaces an + existing key: an unknown key is skipped with a warning and reported by + `make doctor`. Admin-only keys stay admin-only. +- **Public tenancy API** (#418): `simple_module_hosting.tenancy` exposes + `tenancy_mode(app)`, `single_tenant_id(app)`, `require_tenant(...)` (binds + the single-tenant id, or the request's tenant else `on_missing`) and + `tenant_vary(request)`; `TenancyMode` lives in `simple_module_core.tenancy`. + Modules no longer need to read the middleware stack. +- `NativeSelect` takes a `wrapperClassName` for sizing the control (#421). + `className` still targets the ` onChange(e.target.value)} + className="h-9 w-9 shrink-0 cursor-pointer rounded-[9px] border bg-transparent" + /> + onChange(e.target.value)} + onBlur={() => { + const normal = normalizeHex(value); + if (normal && normal !== value) onChange(normal); + }} + className="min-w-0 flex-1 font-mono" + /> +
+ {valid ? null : ( + + )} +
+ ); +} diff --git a/modules/branding/branding/components/PreviewSurfaces.tsx b/modules/branding/branding/components/PreviewSurfaces.tsx index 538979c8..d353685f 100644 --- a/modules/branding/branding/components/PreviewSurfaces.tsx +++ b/modules/branding/branding/components/PreviewSurfaces.tsx @@ -1,5 +1,5 @@ import { keys, useT } from '@simple-module-py/i18n'; -import { type PreviewBrand, previewFooterLinks } from './BrandingPreview'; +import { type PreviewBrand, previewFooterLinks, previewInk } from './BrandingPreview'; function Logo({ brand }: { brand: PreviewBrand }) { return brand.logoUrl ? ( @@ -38,8 +38,8 @@ export function SignInPreview({ brand }: { brand: PreviewBrand }) {
{t(keys.branding.manage.preview_signin_action)}
@@ -63,11 +63,9 @@ export function EmailPreview({ brand }: { brand: PreviewBrand }) {
- - {brand.appName} - + {brand.appName}
@@ -76,8 +74,8 @@ export function EmailPreview({ brand }: { brand: PreviewBrand }) {
{t(keys.branding.manage.preview_email_action)}
diff --git a/modules/branding/branding/components/hex.ts b/modules/branding/branding/components/hex.ts new file mode 100644 index 00000000..33a10315 --- /dev/null +++ b/modules/branding/branding/components/hex.ts @@ -0,0 +1,19 @@ +const HEX = /^#?([0-9a-f]{3}|[0-9a-f]{6})$/i; + +/** + * Normalise what an admin typed into `#rrggbb`, or `null` when it is not a + * 3- or 6-digit hex colour. The leading `#` is optional; case is folded. + * The server only accepts `#rrggbb`, so 3-digit shorthand is expanded here. + */ +export function normalizeHex(raw: string): string | null { + const match = HEX.exec(raw.trim()); + if (!match) return null; + let digits = match[1].toLowerCase(); + if (digits.length === 3) digits = [...digits].map((d) => d + d).join(''); + return `#${digits}`; +} + +/** Empty means "use the theme default", which is a valid stored value. */ +export function isValidColor(raw: string): boolean { + return raw.trim() === '' || normalizeHex(raw) !== null; +} diff --git a/modules/branding/branding/locales/en.json b/modules/branding/branding/locales/en.json index de85cdc6..9762d25d 100644 --- a/modules/branding/branding/locales/en.json +++ b/modules/branding/branding/locales/en.json @@ -9,6 +9,7 @@ "publishing": "Publishing…", "app_name_label": "App name", "primary_color_label": "Primary colour", + "primary_color_invalid": "Enter a hex colour such as #0f766e or #0fe.", "design_pack_label": "Design pack", "design_pack_inline": "Design pack: {name}", "design_pack_none": "None (base tokens)", diff --git a/modules/branding/branding/pages/Manage.tsx b/modules/branding/branding/pages/Manage.tsx index 56f1aad0..26768f56 100644 --- a/modules/branding/branding/pages/Manage.tsx +++ b/modules/branding/branding/pages/Manage.tsx @@ -10,9 +10,11 @@ import type { SharedProps } from '@simple-module-py/ui/types'; import { useMemo, useState } from 'react'; import { toast } from 'sonner'; import { BannerField, type BannerSeverity } from '../components/BannerField'; +import { ColorField } from '../components/ColorField'; import { DesignPackField, type DesignPackOption } from '../components/DesignPackField'; import { type BrandingForm, countBrandingChanges } from '../components/dirty'; import { FooterField } from '../components/FooterField'; +import { isValidColor, normalizeHex } from '../components/hex'; import { ImageDropzones, type ImageKind } from '../components/ImageDropzones'; import { PresetField, type PresetOption } from '../components/PresetField'; import { PreviewTabs } from '../components/PreviewTabs'; @@ -65,6 +67,8 @@ function Manage() { const [busy, setBusy] = useState(false); const changes = countBrandingChanges(form, baseline); const locked = !canManage || busy; + const colorValid = isValidColor(form.color); + const accent = normalizeHex(form.color) ?? DEFAULT_SWATCH; const set = (patch: Partial) => setForm((current) => ({ ...current, ...patch })); @@ -90,7 +94,7 @@ function Manage() { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ app_name: form.appName, - primary_color: form.color, + primary_color: normalizeHex(form.color) ?? '', design_pack: form.designPack, banner_message: form.bannerMessage, banner_severity: form.bannerSeverity, @@ -144,7 +148,7 @@ function Manage() {
-
- -
- set({ color: e.target.value })} - className="h-9 w-9 shrink-0 cursor-pointer rounded-[9px] border bg-transparent" - /> - set({ color: e.target.value })} - className="min-w-0 flex-1 font-mono" - /> -
-
+ set({ color })} + />
{/* The *effective* colour, not the stored one: with nothing set @@ -204,7 +188,7 @@ function Manage() { chip is active claims the opposite. */} set({ color: swatch })} disabled={locked} /> @@ -251,7 +235,7 @@ function Manage() { ; +} + +const BRAND: PreviewBrand = { + appName: 'Acme', + accent: '#f5f5f5', + logoUrl: null, + logoDarkUrl: null, + bannerMessage: '', + footerText: '', + footerLinks: [], + menuLabels: [], +}; + +describe('normalizeHex', () => { + test('accepts 3/6 digits, optional hash, any case', () => { + expect(normalizeHex('F5F5F5')).toBe('#f5f5f5'); + expect(normalizeHex('#ABC')).toBe('#aabbcc'); + expect(normalizeHex(' #0f766e ')).toBe('#0f766e'); + }); + + test('rejects invalid values', () => { + for (const bad of ['#zzz', '12345', 'red', '#12345g', '#1234']) { + expect(normalizeHex(bad)).toBeNull(); + expect(isValidColor(bad)).toBe(false); + } + expect(isValidColor('')).toBe(true); + }); +}); + +describe('ColorField', () => { + test('flags an invalid value and keeps the swatch valid', () => { + const { container } = render(); + fireEvent.change(screen.getByRole('textbox'), { target: { value: '#zzz' } }); + expect(screen.getByRole('alert')).toHaveTextContent('Enter a hex colour'); + expect(container.querySelector('input[type="color"]')).toHaveValue('#0f766e'); + }); + + test('normalises a bare hex value on blur', () => { + render(); + const input = screen.getByRole('textbox'); + fireEvent.change(input, { target: { value: 'F5F5F5' } }); + expect(screen.queryByRole('alert')).toBeNull(); + fireEvent.blur(input); + expect(input).toHaveValue('#f5f5f5'); + }); +}); + +describe('preview ink', () => { + test('light brand colour gets dark ink, dark one gets white', () => { + expect(previewInk('#f5f5f5')).not.toBe(previewInk('#0f766e')); + expect(previewInk('#0f766e')).toBe('oklch(1 0 0)'); + }); + + test('sign-in button uses the derived ink, not hard-coded white', () => { + render(); + const button = screen.getByText('Sign in now'); + expect(button.className).not.toContain('text-white'); + expect(button.style.color).toBe(previewInk('#f5f5f5')); + }); +}); diff --git a/packages/i18n/src/generated-resources.ts b/packages/i18n/src/generated-resources.ts index b4f44ca8..60a46de2 100644 --- a/packages/i18n/src/generated-resources.ts +++ b/packages/i18n/src/generated-resources.ts @@ -222,6 +222,7 @@ export default { 'branding.manage.preview_tab_signin': '', 'branding.manage.preview_tabs_label': '', 'branding.manage.preview_title': '', + 'branding.manage.primary_color_invalid': '', 'branding.manage.primary_color_label': '', 'branding.manage.publish': '', 'branding.manage.publish_note': '', diff --git a/packages/i18n/src/keys.generated.ts b/packages/i18n/src/keys.generated.ts index f470994a..c06942c0 100644 --- a/packages/i18n/src/keys.generated.ts +++ b/packages/i18n/src/keys.generated.ts @@ -279,6 +279,7 @@ export const keys = { preview_tab_signin: 'branding.manage.preview_tab_signin', preview_tabs_label: 'branding.manage.preview_tabs_label', preview_title: 'branding.manage.preview_title', + primary_color_invalid: 'branding.manage.primary_color_invalid', primary_color_label: 'branding.manage.primary_color_label', publish: 'branding.manage.publish', publish_note: 'branding.manage.publish_note',