From 293a14361aa8c940f69dae234d3f35b177bcd7e0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 12:53:57 -0400 Subject: [PATCH 01/16] fix(db): bind datetimes, not strings, when filtering timestamptz columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1727 and #2257 — the same bug in two services written months apart. AppLog.created_at is `timestamp with time zone`. asyncpg binds a Python str as VARCHAR and Postgres has no `timestamptz >= text` operator, so both of these raised when Postgres planned the query: notifications.check_due_tasks AppLog.created_at >= today.isoformat() logging.get_logs AppLog.created_at >= #1727 was the worse of the two because a per-user `except Exception` swallowed it: reminder emails silently never sent, and the only outward trace was an hourly traceback in the Postgres log. It has been open since 2026-07-19 with the diagnosis written and the fix never applied. #2257 has no swallowing handler, so it merely breaks the admin log viewer's date filters outright. notifications: `utc_day_start(day)` returns midnight UTC as an AWARE datetime. Deliberately not the bare `date` the original diagnosis suggested — comparing timestamptz to date does work via an implicit cast, but Postgres resolves that cast in the SESSION's TimeZone, so the dedup window would drift with a server setting nobody remembers is load-bearing. logging: `parse_filter_datetime()` converts the query-string value to an aware UTC datetime; unparseable input returns None so the filter is skipped rather than 500ing the viewer. It also fixes a bug the naive fix would have introduced — `date_to=2026-07-30` parses to midnight, so `<=` would exclude the entire day the user asked for. Date-only upper bounds now run to 23:59:59.999999, while a value carrying an explicit time is left as given. The guard is the point. This class is invisible to ordinary testing: the failure happens when Postgres plans the query, not when Python builds it, so no unit test that doesn't execute SQL can see it. tests/test_timestamp_filters.py fails CI on two shapes — 1. a local bound to .isoformat() compared against a *_at column (#1727) 2. a str-ANNOTATED PARAMETER compared against a *_at column (#2257) Shape 2 is the one that matters. Nothing in logging.py looks date-ish, so a guard built only from #1727's shape finds nothing there — which is exactly how the second instance survived. Verified by replaying both checks against the pre-fix files out of git: shape 1 catches `today_str`, shape 2 catches `date_from`/`date_to`, and the current tree is clean. Found by grepping for siblings after fixing #1727 — the third instance today of "the second place nobody checked", after #2245. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- src/scribe/services/logging.py | 46 +++++++-- src/scribe/services/notifications.py | 26 ++++- tests/test_timestamp_filters.py | 145 +++++++++++++++++++++++++++ 3 files changed, 207 insertions(+), 10 deletions(-) create mode 100644 tests/test_timestamp_filters.py diff --git a/src/scribe/services/logging.py b/src/scribe/services/logging.py index 228e2ee..3123920 100644 --- a/src/scribe/services/logging.py +++ b/src/scribe/services/logging.py @@ -89,6 +89,38 @@ async def log_error( await session.commit() +def parse_filter_datetime(value: str | None, *, end_of_day: bool = False) -> datetime | None: + """An ISO date/datetime from a query string as an AWARE UTC datetime. + + The admin log filters arrive as raw `request.args` strings and used to be + compared straight against `AppLog.created_at`. asyncpg binds a str as + VARCHAR and Postgres has no `timestamptz >= text` operator, so supplying + either date filter raised — the same defect as #1727 in notifications, in a + second place (#2254). + + Returns None for anything unparseable: a malformed filter should narrow + nothing rather than 500 the log viewer. + + `end_of_day` matters for the upper bound. A bare "2026-07-30" parses to + midnight, so `created_at <= that` would exclude the whole of the day the + user asked for — the one day they most likely wanted. With this flag a + date-only value is pushed to the last microsecond of that day; a value that + already carries a time is left exactly as given. + """ + raw = (value or "").strip() + if not raw: + return None + try: + parsed = datetime.fromisoformat(raw) + except ValueError: + return None + if end_of_day and len(raw) == 10: # date-only, no time component + parsed = parsed.replace(hour=23, minute=59, second=59, microsecond=999999) + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed + + async def get_logs( category: str | None = None, user_id: int | None = None, @@ -118,12 +150,14 @@ async def get_logs( ) query = query.where(search_filter) count_query = count_query.where(search_filter) - if date_from: - query = query.where(AppLog.created_at >= date_from) - count_query = count_query.where(AppLog.created_at >= date_from) - if date_to: - query = query.where(AppLog.created_at <= date_to) - count_query = count_query.where(AppLog.created_at <= date_to) + start = parse_filter_datetime(date_from) + end = parse_filter_datetime(date_to, end_of_day=True) + if start: + query = query.where(AppLog.created_at >= start) + count_query = count_query.where(AppLog.created_at >= start) + if end: + query = query.where(AppLog.created_at <= end) + count_query = count_query.where(AppLog.created_at <= end) total = (await session.execute(count_query)).scalar() or 0 diff --git a/src/scribe/services/notifications.py b/src/scribe/services/notifications.py index ae84c9f..cd171d6 100644 --- a/src/scribe/services/notifications.py +++ b/src/scribe/services/notifications.py @@ -3,7 +3,7 @@ import asyncio import json import logging -from datetime import date, datetime, timezone +from datetime import date, datetime, time, timezone from sqlalchemy import func, select, text @@ -149,13 +149,25 @@ async def send_invitation_email(email: str, invite_url: str, invited_by_username await send_email(email, "Fabled Scribe - You're Invited!", _email_html("You're Invited!", body)) +def utc_day_start(day: date) -> datetime: + """Midnight UTC on `day`, as an AWARE datetime — the reminder dedup window. + + Deliberately a `datetime` and not the bare `date` (#1727). Comparing a + `timestamptz` column against a `date` does work, via an implicit cast — but + Postgres resolves that cast in the SESSION's TimeZone, so the window would + move with a server setting nobody remembers is load-bearing. An aware UTC + datetime says what it means and compares the same way everywhere. + """ + return datetime.combine(day, time.min, tzinfo=timezone.utc) + + async def check_due_tasks() -> None: """Check for tasks due today and send reminder emails.""" if not await is_smtp_configured(): return today = date.today() - today_str = today.isoformat() + window_start = utc_day_start(today) async with async_session() as session: # Find tasks due today or overdue, not done @@ -188,12 +200,18 @@ async def check_due_tasks() -> None: if not email: continue - # Dedup: check if we already sent a reminder today + # Dedup: check if we already sent a reminder today. + # `window_start` is an aware datetime, NOT `today.isoformat()`. + # asyncpg binds a str as VARCHAR and Postgres has no + # `timestamptz >= text` operator, so this raised on every run + # that got this far — swallowed by the per-user `except` below, + # which is why the only symptom was reminders silently never + # sending plus an hourly traceback in the DB log (#1727). dedup_result = await session.execute( select(func.count(AppLog.id)).where( AppLog.action == "task_reminder", AppLog.user_id == user_id, - AppLog.created_at >= today_str, + AppLog.created_at >= window_start, ) ) if (dedup_result.scalar() or 0) > 0: diff --git a/tests/test_timestamp_filters.py b/tests/test_timestamp_filters.py new file mode 100644 index 0000000..4f2af64 --- /dev/null +++ b/tests/test_timestamp_filters.py @@ -0,0 +1,145 @@ +"""Timestamp filters must bind datetimes, never strings (#1727, #2254). + +`AppLog.created_at` is `timestamp with time zone`. asyncpg binds a Python `str` +as VARCHAR, and Postgres has no `timestamptz >= text` operator — so comparing +the column against `date.today().isoformat()` or a raw `request.args` string +raises at query time, not at import time and not in any unit test that doesn't +touch a database. + +That is what made this class of bug survive: it is invisible to every test that +doesn't execute the SQL, and in the notifications case the exception was +swallowed by a per-user `except Exception`, so the only outward symptom was +reminder emails silently never sending. + +Found twice in the same afternoon, in two services written months apart, so the +last test here guards the CLASS rather than the two instances. +""" +import ast +import re +from datetime import date, datetime, timezone +from pathlib import Path + +SRC = Path(__file__).resolve().parents[1] / "src" / "scribe" + + +# --- #1727: the reminder dedup window ---------------------------------------- + +def test_utc_day_start_is_an_aware_datetime_not_a_string(): + from scribe.services.notifications import utc_day_start + + start = utc_day_start(date(2026, 7, 30)) + assert isinstance(start, datetime) + assert not isinstance(start, str) + # Aware, and pinned to UTC rather than whatever the DB session's TimeZone + # happens to be — a bare `date` would resolve midnight server-side. + assert start.tzinfo is not None + assert start.utcoffset() == timezone.utc.utcoffset(None) + assert (start.year, start.month, start.day) == (2026, 7, 30) + assert (start.hour, start.minute, start.second, start.microsecond) == (0, 0, 0, 0) + + +# --- #2254: the admin log viewer's date filters ------------------------------ + +def test_filter_datetime_parses_dates_and_datetimes_to_aware_utc(): + from scribe.services.logging import parse_filter_datetime + + d = parse_filter_datetime("2026-07-30") + assert isinstance(d, datetime) and d.tzinfo is not None + assert (d.hour, d.minute) == (0, 0) + + # An explicit time is honoured, not overwritten. + dt = parse_filter_datetime("2026-07-30T14:35:00") + assert (dt.hour, dt.minute) == (14, 35) + + # Already-aware input keeps its offset rather than being stamped UTC. + aware = parse_filter_datetime("2026-07-30T14:35:00+02:00") + assert aware.utcoffset().total_seconds() == 2 * 3600 + + +def test_filter_datetime_upper_bound_covers_the_whole_requested_day(): + """`date_to=2026-07-30` must INCLUDE that day. Parsed as bare midnight it + would exclude every event on the one day the user asked for.""" + from scribe.services.logging import parse_filter_datetime + + end = parse_filter_datetime("2026-07-30", end_of_day=True) + assert (end.hour, end.minute, end.second) == (23, 59, 59) + assert end.microsecond == 999999 + + # But a value that already carries a time is left alone — the caller meant it. + exact = parse_filter_datetime("2026-07-30T09:00:00", end_of_day=True) + assert (exact.hour, exact.minute, exact.second) == (9, 0, 0) + + +def test_filter_datetime_rejects_garbage_instead_of_raising(): + """A malformed filter should narrow nothing, not 500 the log viewer.""" + from scribe.services.logging import parse_filter_datetime + + for junk in ("", " ", None, "not-a-date", "2026-13-45", "yesterday"): + assert parse_filter_datetime(junk) is None + + +# --- the class-level guard --------------------------------------------------- + +def _timestamp_comparison_offenders() -> list[str]: + """Every place a `*_at` column is compared against a known-string name. + + TWO shapes, because the two real bugs had different ones and a guard built + from only the first would have missed the second: + + 1. a local bound to `.isoformat()` — #1727, `today_str = today.isoformat()` + 2. a parameter annotated `str` — #2254, `date_from: str | None`, straight + off `request.args`, never touching + isoformat at all + + Shape 2 is the nastier one: nothing in the file looks date-ish, so grepping + for isoformat finds nothing. It was caught by reading, not by tooling — + which is exactly why it belongs in tooling now. + + Deliberately narrow on both. A blanket "no strings near SQL" rule would fire + on every to_dict() in the codebase and get muted, and a muted guard is worse + than none. + """ + offenders: list[str] = [] + files = sorted((SRC / "services").rglob("*.py")) + sorted((SRC / "routes").rglob("*.py")) + + for path in files: + src = path.read_text() + rel = path.relative_to(SRC) + + suspect_names: dict[str, str] = {} + + # Shape 1 — locals bound to an isoformat() result. + for name in re.findall(r"^\s*(\w+)\s*=\s*[\w.()]+\.isoformat\(\)", src, re.M): + suspect_names[name] = "an isoformat() string" + + # Shape 2 — parameters annotated as str (incl. `str | None`, Optional[str]). + try: + tree = ast.parse(src) + except SyntaxError: # pragma: no cover - not our concern here + tree = None + if tree is not None: + for node in ast.walk(tree): + if not isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)): + continue + args = node.args + for arg in [*args.posonlyargs, *args.args, *args.kwonlyargs]: + if arg.annotation is None: + continue + ann = ast.unparse(arg.annotation) + if re.fullmatch(r"(?:Optional\[)?str\]?(?:\s*\|\s*None)?", ann): + suspect_names[arg.arg] = f"a str-annotated parameter ({ann})" + + for name, why in suspect_names.items(): + if re.search(rf"\w*_at\s*(?:>=|<=|>|<)\s*{re.escape(name)}\b", src): + offenders.append(f"{rel}: timestamp column compared against `{name}` — {why}") + + return offenders + + +def test_no_service_compares_a_timestamp_column_against_a_string(): + offenders = _timestamp_comparison_offenders() + assert not offenders, ( + "asyncpg binds a str as VARCHAR and Postgres has no `timestamptz >= text` " + "operator, so this raises at query time — invisible to any test that " + "doesn't execute the SQL:\n " + "\n ".join(offenders) + ) -- 2.54.0 From 61e6e3841963b07ca9874cbed4634e3e6d924eda Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 13:48:23 -0400 Subject: [PATCH 02/16] =?UTF-8?q?feat(design-explorer):=20token=20inventor?= =?UTF-8?q?y=20=E2=80=94=20what=20exists=20and=20what=20it=20resolves=20to?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #251 step 1 (#2258). Foundation for the gallery and the drift panel. Parses NAMES from theme.css and asks the BROWSER for every value. That split is deliberate. Extracting `--foo` is a trivial regex; extracting its value is not — theme.css has nested parens, commas inside rgba(), var() chains, multi-part shadows and gradients. getComputedStyle already resolves all of it, reports what actually won the cascade, and — the reason that matters here — reflects live overrides set on a container, which is exactly what the preview surface needs (#2261). Parsing values would report what the file says rather than what the user is looking at. It also keeps the error-prone half out of our code, which matters because the frontend has no test runner: `vue-tsc --noEmit` is the entire check. Logic that can't be unit-tested should be logic that can't be very wrong. readTokens(host) takes an element, so the same function reads app-wide values from :root and scoped values from inside a preview container. A BUG CAUGHT BEFORE SHIPPING, worth recording because the first version looked obviously right: the declaration regex originally required the match to follow `{` or `;`, to avoid matching var() uses. That silently dropped every declaration preceded by a COMMENT — including --color-bg, the first and most-used token in the file. 67 of 70 tokens found, no error, no warning. The anchor was never needed. A declaration is `--name:` and a reference is `var(--name)` or `var(--name,` — the colon alone discriminates. Comments are stripped first so commented-out declarations aren't counted. Verified against the real stylesheet: 70 unique tokens, 60 dark-overridden, 10 light-only, every group resolving, zero var()-only false positives. Also records a constraint discovered while building, which shapes step 6: light is declared on :root and dark on [data-theme="dark"], so an attribute selector can ADD dark to a subtree but nothing can add light back. Dark-inside-light previews work; light-inside-dark previews cannot, until a [data-theme="light"] block exists. readTokensForMode documents this rather than pretending otherwise. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- frontend/src/utils/designTokens.ts | 181 +++++++++++++++++++++++++++++ 1 file changed, 181 insertions(+) create mode 100644 frontend/src/utils/designTokens.ts diff --git a/frontend/src/utils/designTokens.ts b/frontend/src/utils/designTokens.ts new file mode 100644 index 0000000..029d475 --- /dev/null +++ b/frontend/src/utils/designTokens.ts @@ -0,0 +1,181 @@ +/** + * Design-token inventory — what tokens exist, and what they actually resolve to. + * + * Foundation for the design explorer (milestone #251): the gallery renders + * against these, and the drift panel compares them to the design rulebook. + * + * DESIGN NOTE — why this parses NAMES but never VALUES. + * Extracting `--foo` from a stylesheet is a trivial, robust regex. Extracting + * its VALUE is not: values contain nested parens, commas inside rgba(), + * `var()` references to other tokens, multi-part shadows, and gradients — and + * `theme.css` has all of those today. So we take the names from the source and + * ask the BROWSER for every value. + * + * That is not just easier, it is more correct. getComputedStyle reports what + * actually won the cascade, resolves `var()` chains, and — critically for this + * milestone — reflects live overrides set on a container, which is exactly what + * the preview surface needs (see #2261). Parsing the source would report what + * the file says rather than what the user is looking at. + * + * It also means this module needs no unit tests to be trustworthy: the only + * logic here is a name regex and a group lookup. The frontend has no test + * runner today (`vue-tsc --noEmit` is the whole check), so keeping the + * error-prone half in the browser rather than in our code is deliberate. + */ +import themeCss from "@/assets/theme.css?raw"; + +export type TokenGroup = + | "color" + | "radius" + | "gradient" + | "glow" + | "focus" + | "layout" + | "other"; + +export type ThemeMode = "light" | "dark"; + +export interface DesignToken { + /** Full custom-property name, including the leading `--`. */ + name: string; + /** Coarse family, derived from the name prefix. */ + group: TokenGroup; + /** Resolved value in the requested context, straight from the browser. */ + value: string; + /** True when the declaration appears inside the dark block in source. */ + overriddenInDark: boolean; +} + +/** + * Matches a custom-property DECLARATION, and never a `var(--name)` use. + * + * The discriminator is the COLON, not the preceding character. A declaration is + * `--name:`; a reference is `var(--name)` or `var(--name, fallback)` — followed + * by `)` or `,`, never by `:`. So no anchor is needed, and adding one is + * actively wrong: an earlier version required the match to follow `{` or `;`, + * which silently dropped every declaration that came after a comment — + * including `--color-bg`, the first and most-used token in the file. + */ +const DECLARATION = /(--[A-Za-z0-9_-]+)\s*:/g; + +/** Comments are stripped first so a commented-out declaration isn't counted. */ +const COMMENT = /\/\*[\s\S]*?\*\//g; + +/** The dark block's selector, as written in theme.css. */ +const DARK_SELECTOR = '[data-theme="dark"]'; + +const GROUP_PREFIXES: ReadonlyArray<[string, TokenGroup]> = [ + ["--color-", "color"], + ["--radius-", "radius"], + ["--gradient-", "gradient"], + ["--glow-", "glow"], + ["--focus-", "focus"], + ["--page-", "layout"], + ["--sidebar-", "layout"], + ["--chat-", "layout"], +]; + +export function groupFor(name: string): TokenGroup { + for (const [prefix, group] of GROUP_PREFIXES) { + if (name.startsWith(prefix)) return group; + } + return "other"; +} + +/** Every custom property declared anywhere in the stylesheet, in source order, deduped. */ +export function tokenNames(css: string = themeCss): string[] { + const seen = new Set(); + const out: string[] = []; + for (const match of css.replace(COMMENT, "").matchAll(DECLARATION)) { + const name = match[1]; + if (!seen.has(name)) { + seen.add(name); + out.push(name); + } + } + return out; +} + +/** The subset re-declared inside the dark block — i.e. tokens that change with mode. */ +export function darkOverriddenNames(css: string = themeCss): Set { + const bare = css.replace(COMMENT, ""); + const start = bare.indexOf(DARK_SELECTOR); + if (start === -1) return new Set(); + const open = bare.indexOf("{", start); + const close = bare.indexOf("}", open); + if (open === -1 || close === -1) return new Set(); + return new Set(tokenNames(bare.slice(open, close))); +} + +/** + * Read the resolved value of every token in `host`'s context. + * + * Pass a container to read the tokens as they apply INSIDE it — which is how + * the preview surface reads a scoped override without disturbing the page. + * Defaults to the document root, i.e. the app-wide values. + */ +export function readTokens(host: Element = document.documentElement): DesignToken[] { + const computed = getComputedStyle(host); + const dark = darkOverriddenNames(); + return tokenNames().map((name) => ({ + name, + group: groupFor(name), + value: computed.getPropertyValue(name).trim(), + overriddenInDark: dark.has(name), + })); +} + +/** + * Read tokens as they would resolve in a given mode, without touching the page. + * + * Uses an offscreen probe carrying the mode attribute, so the live UI is never + * mutated to take a reading. + * + * KNOWN LIMITATION, and it is a property of the stylesheet rather than of this + * function: light is declared on `:root` while dark is declared on + * `[data-theme="dark"]`. An attribute selector can ADD the dark values to a + * subtree, but there is no `[data-theme="light"]` block to add the light ones + * back. So reading "light" from inside a dark page returns the dark values — + * the probe has nothing to match. + * + * Concretely: dark-inside-light previews work, light-inside-dark previews do + * not. Introducing a `[data-theme="light"]` block alongside the dark-first flip + * (milestone #251 step 6) is what makes this symmetric, and until then callers + * should treat a cross-mode read as best-effort. + */ +export function readTokensForMode(mode: ThemeMode): DesignToken[] { + const probe = document.createElement("div"); + probe.setAttribute("data-theme", mode); + probe.style.display = "none"; + document.body.appendChild(probe); + try { + return readTokens(probe); + } finally { + probe.remove(); + } +} + +/** Tokens grouped by family, preserving source order within each group. */ +export function groupTokens(tokens: DesignToken[]): Map { + const out = new Map(); + for (const token of tokens) { + const bucket = out.get(token.group); + if (bucket) bucket.push(token); + else out.set(token.group, [token]); + } + return out; +} + +/** + * Tokens declared in the stylesheet that nothing references with `var()`. + * + * Dead tokens are drift too: `--chat-reading-width` and + * `--chat-context-sidebar-width` outlived the chat subsystem that was deleted + * in the MCP-first pivot, and nothing has referenced them since. Takes the + * corpus of source files to search as an argument so the caller decides what + * "used" means — this module has no opinion about the project layout. + */ +export function unreferencedTokens(tokens: DesignToken[], sources: string[]): DesignToken[] { + const haystack = sources.join("\n"); + return tokens.filter((token) => !haystack.includes(`var(${token.name}`)); +} -- 2.54.0 From 3c0192d7492a8b739eda3188c74411e2ea0b1d08 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 14:41:06 -0400 Subject: [PATCH 03/16] feat(design-explorer): the gallery, including what isn't there MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #251 step 3 (#2260). New /design view, reachable from a Palette icon beside Trash and Settings — a meta-surface like /rules, so an icon rather than a sixth primary nav link, but not hidden either, since somewhere the design system is visible is the entire point. Renders three things, and refuses to render a fourth: - REAL components, imported not recreated: StatusBadge, PriorityBadge, TagPill. - REAL tokens, read at runtime via readTokens() so the page shows the live cascade rather than what the stylesheet claims. Grouped, swatched where the value is a colour, flagged where the token is mode-aware. - Rule 65's four button variants and rule 60's type scale, listed as SPEC and marked missing. That last part is the point of the step rather than a shortfall of it. Step 3's premise was "render the real components, not copies — a gallery of look-alikes drifts from the app within a month and then lies." Buttons have no shared implementation to import: .btn-primary is defined four separate times in four -- 2.54.0 From d3ee24f239853d91675c83dc631a51311a8f2ba4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 15:25:36 -0400 Subject: [PATCH 04/16] =?UTF-8?q?feat(design-explorer):=20rulebook=20bindi?= =?UTF-8?q?ng=20+=20prose=E2=86=92claims=20extraction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #251 step 2 (#2259). The half of the drift panel that needed to be testable, which is why it is Python: the frontend has no test runner, so the fiddly extraction lives server-side and the browser only does set arithmetic over live token values. BINDING. A per-user setting `design_rulebook_id` names the rulebook that describes this install's design system. A setting rather than a column: no migration, discoverable in the Settings UI (rule #25), and honest about being a per-install choice rather than a property of the rulebook. No rulebook designated returns an empty set with rulebook_id: null — the NORMAL case for any install but the one that set it up (rule #115), which the client renders as an explanatory empty state rather than an error. The id comes back alongside the list so "not designated" and "designated but empty" stay distinguishable. EXTRACTION. No NLP. Rule statements are prose written for humans and should stay that way, so this takes only what is unambiguous in any prose — the hex colours and custom-property names a rule mentions. Anything subtler needs a rule author to opt into a structured form, deliberately left for when someone wants it. Three things earn their complexity: - SENTENCE-SCOPED NEGATION. A rule routinely states what the palette requires and what it forbids in consecutive sentences ("Parchment #E8E4D8 …, Vellum #C2BFB4 …. Pure white #FFFFFF is NEVER used."). Detecting negation across the whole statement would mark the required colours as forbidden — inverting the finding rather than missing it, which is worse. Per sentence, all four come out right. - HEX NORMALISATION is load-bearing, not tidiness. The rulebook writes #FFFFFF and components write #fff; if those don't compare equal the largest drift finding in the codebase — 67 hardcoded white text colours (#2275) — reads as zero. Alpha forms keep their alpha, since #fff and #ffff are different colours and collapsing them would manufacture equality. - SLASH SHORTHAND. Rulebooks write token families as --fs-radius-sm/md/lg/xl and --fs-obsidian/iron/slate/pewter. Both expand under one rule — prefix is everything up to and including the LAST hyphen of the first segment — which also handles --fs-dur-fast/base/slow. Verified against the real rule text: 18 tokens from three different shorthand shapes. how_to_apply is read alongside statement, because rulebooks routinely keep the statement declarative and put the concrete values in how_to_apply; ignoring it would miss the checkable half. Claims dedupe on (kind, value), first source winning, so a colour named by several rules is one expectation attributed to the rule that introduced it. Prose with nothing checkable yields nothing — most rules are judgement, not specification, and a panel that reported unparseable rules as problems would be unusable. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- src/scribe/app.py | 2 + src/scribe/routes/design.py | 29 ++++ src/scribe/services/design_system.py | 232 +++++++++++++++++++++++++++ tests/test_design_system.py | 161 +++++++++++++++++++ 4 files changed, 424 insertions(+) create mode 100644 src/scribe/routes/design.py create mode 100644 src/scribe/services/design_system.py create mode 100644 tests/test_design_system.py diff --git a/src/scribe/app.py b/src/scribe/app.py index abd7c2a..5f665ab 100644 --- a/src/scribe/app.py +++ b/src/scribe/app.py @@ -26,6 +26,7 @@ from scribe.routes.profile import profile_bp from scribe.routes.knowledge import knowledge_bp from scribe.routes.rulebooks import rulebooks_bp from scribe.routes.plugin import plugin_bp +from scribe.routes.design import design_bp from scribe.routes.trash import trash_bp from scribe.routes.dashboard import dashboard_bp from scribe.routes.systems import systems_bp @@ -89,6 +90,7 @@ def create_app() -> Quart: app.register_blueprint(knowledge_bp) app.register_blueprint(rulebooks_bp) app.register_blueprint(plugin_bp) + app.register_blueprint(design_bp) app.register_blueprint(trash_bp) app.register_blueprint(dashboard_bp) app.register_blueprint(systems_bp) diff --git a/src/scribe/routes/design.py b/src/scribe/routes/design.py new file mode 100644 index 0000000..33b3146 --- /dev/null +++ b/src/scribe/routes/design.py @@ -0,0 +1,29 @@ +"""Design-system surface — what the rulebook expects of the stylesheet. + +The client owns the other half of the comparison: it reads live token values from +the browser (see `utils/designTokens.ts`), which is the only place they exist +resolved. This endpoint supplies the claims to check them against. +""" +from quart import Blueprint, jsonify + +from scribe.auth import get_current_user_id, login_required +from scribe.services import design_system as design_svc + +design_bp = Blueprint("design", __name__, url_prefix="/api/design") + + +@design_bp.get("/expectations") +@login_required +async def get_expectations(): + """Checkable claims from the rulebook this install designated as its design system. + + Returns `{"rulebook_id": int|null, "expectations": [...]}`. + + `rulebook_id: null` is the NORMAL case, not an error — an install that has + not designated a design rulebook has nothing to compare against, and the + client shows an explanatory empty state (rule #115). Distinguishing it from + "designated but empty" is why the id is returned alongside the list. + """ + uid = get_current_user_id() + result = await design_svc.design_expectations(uid) + return jsonify(result.as_dict()) diff --git a/src/scribe/services/design_system.py b/src/scribe/services/design_system.py new file mode 100644 index 0000000..7f637fd --- /dev/null +++ b/src/scribe/services/design_system.py @@ -0,0 +1,232 @@ +"""Design-system expectations — turning rulebook prose into checkable claims. + +Milestone #251 step 2. The drift panel compares what the design rulebook SAYS +against what the stylesheet and components actually DO. This module owns the +first half: reading a rulebook's rules and extracting the claims that can be +mechanically checked. + +WHY THIS LIVES SERVER-SIDE. The frontend has no test runner — `vue-tsc --noEmit` +is the entire check — and this is the one genuinely fiddly piece of the feature. +Extraction happens here where pytest can assert on it; the comparison itself is +set arithmetic and stays in the browser, where the live token values are. + +WHY NOT NLP. Rule statements are prose written for humans, and they should stay +that way — they are read by people far more often than they are parsed. So this +extracts only what is unambiguous in ANY prose: the hex colours and CSS custom +property names a rule mentions. Everything subtler (padding scales, type ramps) +needs a rule author to opt into a structured form, which is deliberately left for +when someone wants it rather than invented up front. + +RULE #115. Nothing here assumes a design rulebook exists, or that it is this +operator's. An install designates one; an install that hasn't gets an empty +result and a panel that explains itself. +""" +from __future__ import annotations + +import logging +import re +from dataclasses import dataclass, field + +from scribe.models.rulebook import Rule +from scribe.services.settings import get_setting + +logger = logging.getLogger(__name__) + +# Which rulebook describes this install's design system. A plain setting rather +# than a column: no migration, discoverable in the Settings UI (rule #25), and +# honest about being a per-install choice rather than a property of the rulebook. +DESIGN_RULEBOOK_SETTING = "design_rulebook_id" + +# `#abc` and `#aabbcc`, plus the 4/8-digit alpha forms. +_HEX = re.compile(r"#([0-9a-fA-F]{3,8})\b") + +# A custom-property name as written in prose, including the slash shorthand the +# rulebook uses: `--fs-radius-sm/md/lg/xl`, `--fs-obsidian/iron/slate/pewter`. +_TOKEN = re.compile(r"(--[a-zA-Z][\w-]*(?:/[\w-]+)*)") + +# Sentence-ish split. Rules use semicolons as hard breaks as often as periods. +_SENTENCE_SPLIT = re.compile(r"(?<=[.;])\s+|\n+") + +# Negation markers. Checked PER SENTENCE, which is the whole trick — see +# _extract_from_sentence. +_NEGATIONS = ("never", "not ", "no ", "avoid", "don't", "must not", "excluded") + + +@dataclass +class Expectation: + """One mechanically-checkable claim a rule makes.""" + + kind: str # "token" | "color" | "prohibited_color" + value: str # "--fs-obsidian" | "#14171a" + rule_id: int + rule_title: str + context: str # the sentence it came from, for showing your work + + def as_dict(self) -> dict: + return { + "kind": self.kind, + "value": self.value, + "rule_id": self.rule_id, + "rule_title": self.rule_title, + "context": self.context, + } + + +@dataclass +class ExpectationSet: + rulebook_id: int | None = None + expectations: list[Expectation] = field(default_factory=list) + + def as_dict(self) -> dict: + return { + "rulebook_id": self.rulebook_id, + "expectations": [e.as_dict() for e in self.expectations], + } + + +def normalize_hex(value: str) -> str | None: + """Fold a hex colour to a comparable form, or None if it isn't one. + + Load-bearing for the whole comparison: the rulebook writes `#FFFFFF` and the + code writes `#fff`, and those must compare equal or the single largest drift + finding (#2275) reads as zero. Expands 3-digit shorthand and lowercases. + + Alpha forms (4 and 8 digit) keep their alpha — `#fff` and `#ffff` are not the + same colour, and silently dropping the alpha would invent equality. + """ + match = _HEX.fullmatch(value.strip()) or _HEX.match(value.strip()) + if not match: + return None + digits = match.group(1).lower() + if len(digits) in (3, 4): + digits = "".join(c * 2 for c in digits) + if len(digits) not in (6, 8): + return None + return f"#{digits}" + + +def expand_token_shorthand(raw: str) -> list[str]: + """`--fs-radius-sm/md/lg/xl` -> the four names it stands for. + + The rulebook writes token families in a slash shorthand, and both forms it + uses expand correctly under one rule: take everything up to and including the + LAST hyphen of the first segment as the prefix, then append each alternative. + + --fs-radius-sm/md/lg/xl prefix `--fs-radius-` -> sm, md, lg, xl + --fs-obsidian/iron/slate prefix `--fs-` -> obsidian, iron, slate + --fs-dur-fast/base/slow prefix `--fs-dur-` -> fast, base, slow + + A name with no slash is returned as-is. + """ + if "/" not in raw: + return [raw] + head, *rest = raw.split("/") + cut = head.rfind("-") + if cut <= 1: # no hyphen beyond the leading `--` + return [head, *rest] + prefix = head[: cut + 1] + return [head, *[f"{prefix}{part}" for part in rest if part]] + + +def _is_negated(sentence: str) -> bool: + return any(marker in sentence.lower() for marker in _NEGATIONS) + + +def _extract_from_sentence(sentence: str, rule: Rule) -> list[Expectation]: + """Claims in ONE sentence, with negation scoped to that sentence. + + Sentence scope is what makes the prohibition detection usable. Rule 52 reads: + + "Text tokens: Parchment #E8E4D8 …, Vellum #C2BFB4 …, Ash #9C9A92 …. + Pure white #FFFFFF is NEVER used as text color." + + Three colours the palette REQUIRES and one it FORBIDS, in one statement. + Detecting negation across the whole statement would mark all four as + forbidden; detecting it per sentence gets all four right. + """ + out: list[Expectation] = [] + negated = _is_negated(sentence) + + for match in _HEX.finditer(sentence): + value = normalize_hex(match.group(0)) + if not value: + continue + out.append(Expectation( + kind="prohibited_color" if negated else "color", + value=value, + rule_id=int(rule.id), + rule_title=rule.title, + context=sentence.strip(), + )) + + # Token names are not negated in practice — a rule says which tokens should + # exist, never which must not — so they are recorded as expectations + # regardless. If that ever changes, it needs its own kind rather than + # borrowing the colour one. + for match in _TOKEN.finditer(sentence): + for name in expand_token_shorthand(match.group(1)): + out.append(Expectation( + kind="token", + value=name, + rule_id=int(rule.id), + rule_title=rule.title, + context=sentence.strip(), + )) + + return out + + +def extract_expectations(rules: list[Rule]) -> list[Expectation]: + """Every checkable claim across a set of rules, deduped on (kind, value). + + First occurrence wins so the reported rule is the one that introduced the + claim, which is usually the most specific place to send a reader. + """ + seen: set[tuple[str, str]] = set() + out: list[Expectation] = [] + for rule in rules: + text = " ".join(filter(None, [rule.statement or "", rule.how_to_apply or ""])) + for sentence in _SENTENCE_SPLIT.split(text): + if not sentence.strip(): + continue + for expectation in _extract_from_sentence(sentence, rule): + key = (expectation.kind, expectation.value) + if key in seen: + continue + seen.add(key) + out.append(expectation) + return out + + +async def get_design_rulebook_id(user_id: int) -> int | None: + """The rulebook this install designated as its design system, if any.""" + raw = (await get_setting(user_id, DESIGN_RULEBOOK_SETTING, "")).strip() + if not raw: + return None + try: + value = int(raw) + except (TypeError, ValueError): + return None + return value if value > 0 else None + + +async def design_expectations(user_id: int) -> ExpectationSet: + """Checkable claims from the designated design rulebook. + + Returns an empty set when no rulebook is designated — the normal case for + any install but the one that set it up (rule #115). The caller shows an + explanatory empty state rather than treating this as an error. + """ + rulebook_id = await get_design_rulebook_id(user_id) + if rulebook_id is None: + return ExpectationSet() + + from scribe.services import rulebooks as rulebooks_svc + + try: + rules = await rulebooks_svc.list_rules(user_id, rulebook_id=rulebook_id) + except Exception: + logger.warning("Design rulebook %s could not be read", rulebook_id, exc_info=True) + return ExpectationSet(rulebook_id=rulebook_id) + + return ExpectationSet(rulebook_id=rulebook_id, expectations=extract_expectations(rules)) diff --git a/tests/test_design_system.py b/tests/test_design_system.py new file mode 100644 index 0000000..0b29af2 --- /dev/null +++ b/tests/test_design_system.py @@ -0,0 +1,161 @@ +"""Rulebook prose → checkable claims (milestone #251 step 2). + +This is the piece of the design explorer that most needed to be testable, which +is why it lives in Python at all: the frontend has no test runner, so the fiddly +extraction happens server-side and the browser only does set arithmetic over it. + +Rule text below is representative of a real design rulebook rather than copied +from this operator's — rule #115: the product must work for an install that has +none of their data, and a test that only passes against their exact wording would +be testing the instance, not the parser. +""" +from types import SimpleNamespace + +from scribe.services.design_system import ( + expand_token_shorthand, + extract_expectations, + normalize_hex, +) + + +def _rule(rule_id, title, statement, how_to_apply=None): + return SimpleNamespace( + id=rule_id, title=title, statement=statement, how_to_apply=how_to_apply + ) + + +# --- hex normalisation ------------------------------------------------------- + +def test_normalize_hex_makes_shorthand_and_case_comparable(): + """LOAD-BEARING. The rulebook writes `#FFFFFF` and components write `#fff`. + If those don't compare equal, the single largest drift finding — 67 hardcoded + white text colours (#2275) — reads as zero findings.""" + assert normalize_hex("#fff") == normalize_hex("#FFFFFF") == "#ffffff" + assert normalize_hex("#E8E4D8") == "#e8e4d8" + assert normalize_hex("#14171a") == "#14171a" + + +def test_normalize_hex_keeps_alpha_rather_than_inventing_equality(): + """`#fff` and `#ffff` are different colours. Dropping the alpha to make them + match would manufacture agreement that isn't there.""" + assert normalize_hex("#ffff") == "#ffffffff" + assert normalize_hex("#fff") != normalize_hex("#ffff") + + +def test_normalize_hex_rejects_non_colours(): + for junk in ("", " ", "not-a-colour", "#", "#gg", "#12345"): + assert normalize_hex(junk) is None + + +# --- the slash shorthand ----------------------------------------------------- + +def test_expand_token_shorthand_handles_every_form_a_rulebook_uses(): + """One rule expands all three shapes: take everything up to and including the + LAST hyphen of the first segment as the prefix.""" + assert expand_token_shorthand("--fs-radius-sm/md/lg/xl") == [ + "--fs-radius-sm", "--fs-radius-md", "--fs-radius-lg", "--fs-radius-xl", + ] + # Prefix is just `--fs-` here, and the same rule finds it. + assert expand_token_shorthand("--fs-obsidian/iron/slate/pewter") == [ + "--fs-obsidian", "--fs-iron", "--fs-slate", "--fs-pewter", + ] + assert expand_token_shorthand("--fs-dur-fast/base/slow") == [ + "--fs-dur-fast", "--fs-dur-base", "--fs-dur-slow", + ] + + +def test_expand_token_shorthand_passes_plain_names_through(): + assert expand_token_shorthand("--fs-ease") == ["--fs-ease"] + + +# --- extraction -------------------------------------------------------------- + +def test_negation_is_scoped_to_the_sentence_not_the_rule(): + """THE trick that makes prohibition detection usable. + + A single rule routinely states what the palette REQUIRES and what it FORBIDS + in consecutive sentences. Detecting negation across the whole statement would + mark the required colours as forbidden too — inverting the finding rather + than missing it, which is worse. + """ + rule = _rule( + 52, "Text palette", + "Text tokens: Parchment #E8E4D8 (primary), Vellum #C2BFB4 (secondary), " + "Ash #9C9A92 (tertiary). Pure white #FFFFFF is NEVER used as text color.", + ) + found = extract_expectations([rule]) + required = {e.value for e in found if e.kind == "color"} + forbidden = {e.value for e in found if e.kind == "prohibited_color"} + + assert required == {"#e8e4d8", "#c2bfb4", "#9c9a92"} + assert forbidden == {"#ffffff"} + assert not (required & forbidden) + + +def test_token_names_are_extracted_and_expanded(): + rule = _rule( + 72, "CSS custom properties", + "Expose the system as custom properties on :root — surfaces " + "(--fs-obsidian/iron/slate/pewter), radius (--fs-radius-sm/md/lg/xl), " + "and motion (--fs-ease).", + ) + names = {e.value for e in extract_expectations([rule]) if e.kind == "token"} + assert "--fs-obsidian" in names and "--fs-pewter" in names + assert "--fs-radius-xl" in names + assert "--fs-ease" in names + assert len(names) == 9 + + +def test_how_to_apply_is_read_as_well_as_the_statement(): + """Rulebooks routinely put the concrete values in how_to_apply and keep the + statement declarative, so ignoring it would miss the checkable half.""" + rule = _rule( + 56, "Per-app accent", "Each app owns exactly one accent.", + how_to_apply='[data-app="scribe"] #5B4A8A, [data-app="minstrel"] #4A6B5C.', + ) + colours = {e.value for e in extract_expectations([rule]) if e.kind == "color"} + assert colours == {"#5b4a8a", "#4a6b5c"} + + +def test_claims_are_deduped_across_rules_keeping_the_first_source(): + """A colour named by several rules is one expectation, attributed to the rule + that introduced it — usually the most specific place to send a reader.""" + rules = [ + _rule(51, "Surfaces", "Obsidian #14171A is the page background."), + _rule(99, "Elsewhere", "Obsidian #14171A again, mentioned in passing."), + ] + found = [e for e in extract_expectations(rules) if e.kind == "color"] + assert len(found) == 1 + assert found[0].rule_id == 51 + + +def test_prose_with_nothing_checkable_yields_nothing(): + """Most rules are judgement, not specification. They must contribute no + findings rather than a shrug — a panel that reports unparseable rules as + problems would be unusable.""" + rule = _rule( + 68, "Voice and tone", + "Voice is understated: plain language for anything functional, flavour " + "only where the user is waiting or failing. Be brief.", + ) + assert extract_expectations([rule]) == [] + + +def test_every_expectation_carries_the_sentence_it_came_from(): + """The panel has to show its working — "the rulebook says X" is only + actionable if you can see where, and in what context. + + Asserts the context is the SENTENCE, not the whole statement: a rule that + states a requirement and a prohibition in consecutive sentences would + otherwise attribute both to the same undifferentiated blob of prose. + """ + rule = _rule(63, "Radius", "Radius: Small 4px. Pure white #FFFFFF is never used.") + found = extract_expectations([rule]) + assert len(found) == 1 + + only = found[0] + assert only.kind == "prohibited_color" + assert only.rule_id == 63 + assert only.rule_title == "Radius" + assert only.context == "Pure white #FFFFFF is never used." + assert "Radius: Small 4px" not in only.context -- 2.54.0 From 4ca3ab02c432e460cf7508a8c0487614ee32f866 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 15:32:29 -0400 Subject: [PATCH 05/16] =?UTF-8?q?feat(design-explorer):=20the=20drift=20pa?= =?UTF-8?q?nel=20=E2=80=94=20rulebook=20says=20X,=20tokens=20say=20Y?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #251 step 5 (#2262), plus the Settings control that makes it reachable. The comparison is deliberately thin — set arithmetic over live token values, which is the one thing the browser knows and the server doesn't. The hard half (prose to claims) already lives in Python where pytest can assert on it. Three claim kinds, and the third inverts the test: a `token` claim asks whether a custom property of that name exists; a `color` claim asks whether any token resolves to that value; a `prohibited_color` claim FAILS when present. normalizeColour is the client-side twin of normalize_hex and has one job the server cannot do: getComputedStyle reports colours as rgb()/rgba() regardless of how they were authored. So one colour has three spellings in play — #FFFFFF in the rulebook, #fff in the stylesheet, rgb(255,255,255) from the browser — and a comparison that misses any of them under-reports silently rather than erroring. THE PANEL STATES ITS OWN BLIND SPOT, which matters more than it sounds. This compares the rulebook against TOKENS. A literal hardcoded in a component, where a token should have been referenced, is invisible to it — the drift isn't in the tokens at all (#2275: 67 hardcoded whites against a rule forbidding pure white). Reading those would mean bundling every SFC's source into the app; the check belongs in CI and is tracked at #2277. A drift report that silently omitted a whole category would invite the reader to conclude the category is clean, so the panel says so in the panel rather than in a comment nobody reads. Findings are ranked violated → missing → ok, and `ok` rows are hidden behind a toggle. Same principle the auto-inject menu is built on: a short list that gets read beats a complete one that doesn't. Settings gains a rulebook picker. "None" is a first-class choice, not an unset error — most installs have no rulebook describing their design system, and saving empty DELETES the setting rather than storing a zero. The panel's empty state points at Settings and Settings points back at the panel, so neither is a dead end. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UaYUaouG9jjhATyuxCKrQs --- frontend/src/api/design.ts | 9 ++ frontend/src/utils/designDrift.ts | 169 ++++++++++++++++++++++++++++ frontend/src/views/DesignView.vue | 144 +++++++++++++++++++++++- frontend/src/views/SettingsView.vue | 33 ++++++ 4 files changed, 354 insertions(+), 1 deletion(-) create mode 100644 frontend/src/api/design.ts create mode 100644 frontend/src/utils/designDrift.ts diff --git a/frontend/src/api/design.ts b/frontend/src/api/design.ts new file mode 100644 index 0000000..cbdfe09 --- /dev/null +++ b/frontend/src/api/design.ts @@ -0,0 +1,9 @@ +import { apiGet } from "@/api/client"; +import type { ExpectationResponse } from "@/utils/designDrift"; + +/** Checkable claims from the rulebook this install designated as its design system. + * + * `rulebook_id: null` means none has been designated — the normal state for a + * fresh install, not an error. The caller shows an explanatory empty state. */ +export const fetchDesignExpectations = () => + apiGet("/api/design/expectations"); diff --git a/frontend/src/utils/designDrift.ts b/frontend/src/utils/designDrift.ts new file mode 100644 index 0000000..453e84f --- /dev/null +++ b/frontend/src/utils/designDrift.ts @@ -0,0 +1,169 @@ +/** + * Drift comparison — what the rulebook claims vs what the stylesheet does. + * + * Milestone #251 step 5. Deliberately thin: the hard half (turning rulebook + * prose into claims) is server-side in `services/design_system.py`, where pytest + * can assert on it. What's left here is set arithmetic over live token values, + * which is the one thing the browser knows and the server doesn't. + * + * SCOPE, and it is a real limit rather than an omission. This compares the + * rulebook against the TOKENS. It cannot see the third category of drift — a + * literal hardcoded in a component where a token should be referenced (#2275, + * 67 occurrences of `color: #fff` against a rule that forbids pure white). That + * drift isn't in the tokens at all, so no amount of inspecting them finds it. + * + * Catching it needs the component sources, which would mean bundling every SFC + * into the app to read at runtime — a large cost for a panel. It belongs in CI, + * as a lint-shaped check, and is tracked there (#2277). Saying so in the panel + * matters: a drift report that silently omits a category invites the reader to + * conclude the category is clean. + */ +import type { DesignToken } from "@/utils/designTokens"; + +export type ExpectationKind = "token" | "color" | "prohibited_color"; + +export interface Expectation { + kind: ExpectationKind; + value: string; + rule_id: number; + rule_title: string; + context: string; +} + +export interface ExpectationResponse { + rulebook_id: number | null; + expectations: Expectation[]; +} + +export type FindingStatus = "ok" | "missing" | "violated"; + +export interface Finding { + expectation: Expectation; + status: FindingStatus; + /** Tokens that satisfy (or, for a prohibition, breach) the expectation. */ + matches: string[]; +} + +/** + * Normalise a colour for comparison — the client-side twin of + * `normalize_hex` in services/design_system.py. + * + * These two MUST agree. The rulebook writes `#FFFFFF`, `theme.css` writes + * `#fff`, and getComputedStyle hands back `rgb(255, 255, 255)` — three + * spellings of one colour, and a comparison that misses any of them under-reports + * rather than erroring. The rgb() case is browser-specific and therefore has no + * server-side counterpart, which is exactly why it is handled here. + */ +export function normalizeColour(value: string): string | null { + const raw = value.trim().toLowerCase(); + + const hex = /^#([0-9a-f]{3,8})$/.exec(raw); + if (hex) { + let digits = hex[1]; + if (digits.length === 3 || digits.length === 4) { + digits = digits.split("").map((c) => c + c).join(""); + } + return digits.length === 6 || digits.length === 8 ? `#${digits}` : null; + } + + // getComputedStyle always reports colours as rgb()/rgba(), never as authored. + const rgb = /^rgba?\(([^)]+)\)$/.exec(raw); + if (rgb) { + const parts = rgb[1].split(/[,\s/]+/).filter(Boolean); + if (parts.length < 3) return null; + const channels = parts.slice(0, 3).map((p) => Number(p)); + if (channels.some((n) => !Number.isFinite(n))) return null; + const hexOf = (n: number) => Math.round(n).toString(16).padStart(2, "0"); + const base = `#${channels.map(hexOf).join("")}`; + if (parts.length === 3) return base; + const alpha = Number(parts[3]); + if (!Number.isFinite(alpha) || alpha >= 1) return base; + return `${base}${hexOf(alpha * 255)}`; + } + + return null; +} + +/** Every distinct colour the stylesheet actually resolves to, mapped to its tokens. */ +export function colourIndex(tokens: DesignToken[]): Map { + const index = new Map(); + for (const token of tokens) { + const colour = normalizeColour(token.value); + if (!colour) continue; + const names = index.get(colour); + if (names) names.push(token.name); + else index.set(colour, [token.name]); + } + return index; +} + +/** + * Compare claims against the live tokens. + * + * A `token` claim asks whether a custom property of that name exists. + * A `color` claim asks whether any token resolves to that value. + * A `prohibited_color` claim INVERTS the test — present is the failure. + */ +export function compareToTokens( + expectations: Expectation[], + tokens: DesignToken[], +): Finding[] { + const names = new Set(tokens.map((t) => t.name)); + const colours = colourIndex(tokens); + + return expectations.map((expectation) => { + if (expectation.kind === "token") { + const present = names.has(expectation.value); + return { + expectation, + status: present ? "ok" : "missing", + matches: present ? [expectation.value] : [], + }; + } + + const matches = colours.get(expectation.value) ?? []; + if (expectation.kind === "prohibited_color") { + return { + expectation, + status: matches.length ? "violated" : "ok", + matches, + }; + } + return { + expectation, + status: matches.length ? "ok" : "missing", + matches, + }; + }); +} + +export interface DriftSummary { + ok: number; + missing: number; + violated: number; + total: number; +} + +export function summarise(findings: Finding[]): DriftSummary { + const summary: DriftSummary = { ok: 0, missing: 0, violated: 0, total: findings.length }; + for (const finding of findings) summary[finding.status] += 1; + return summary; +} + +/** + * Findings worth leading with. + * + * A panel that opens with every row gets closed and never reopened — the same + * principle the auto-inject menu is built on: a short list that gets read beats + * a complete one that doesn't. Violations first (something is actively wrong), + * then missing (something was never built), and `ok` rows are not "findings" at + * all — they belong behind an expansion. + */ +export function rankFindings(findings: Finding[]): Finding[] { + const order: Record = { violated: 0, missing: 1, ok: 2 }; + return [...findings].sort((a, b) => { + const byStatus = order[a.status] - order[b.status]; + if (byStatus !== 0) return byStatus; + return a.expectation.rule_id - b.expectation.rule_id; + }); +} diff --git a/frontend/src/views/DesignView.vue b/frontend/src/views/DesignView.vue index 39cfd05..0e86b8d 100644 --- a/frontend/src/views/DesignView.vue +++ b/frontend/src/views/DesignView.vue @@ -20,21 +20,52 @@ */ import { computed, onMounted, ref } from "vue"; +import { fetchDesignExpectations } from "@/api/design"; import PriorityBadge from "@/components/PriorityBadge.vue"; import StatusBadge from "@/components/StatusBadge.vue"; import TagPill from "@/components/TagPill.vue"; +import { + compareToTokens, + rankFindings, + summarise, + type Expectation, + type Finding, +} from "@/utils/designDrift"; import { groupTokens, readTokens, type DesignToken, type TokenGroup } from "@/utils/designTokens"; const tokens = ref([]); +const expectations = ref([]); +const designRulebookId = ref(null); +const driftLoaded = ref(false); +const showCleanRows = ref(false); /** * Read on mount, not at module scope: the values depend on the live cascade, * which needs the app's stylesheets applied and the theme attribute set. */ -onMounted(() => { +onMounted(async () => { tokens.value = readTokens(); + try { + const response = await fetchDesignExpectations(); + designRulebookId.value = response.rulebook_id; + expectations.value = response.expectations; + } catch { + // The gallery is useful without the panel, so a failed fetch degrades to + // "no drift data" rather than taking the page down with it. + designRulebookId.value = null; + } finally { + driftLoaded.value = true; + } }); +const findings = computed(() => + rankFindings(compareToTokens(expectations.value, tokens.value)), +); +const driftSummary = computed(() => summarise(findings.value)); +const visibleFindings = computed(() => + showCleanRows.value ? findings.value : findings.value.filter((f) => f.status !== "ok"), +); + const grouped = computed(() => groupTokens(tokens.value)); const GROUP_ORDER: TokenGroup[] = ["color", "radius", "glow", "gradient", "focus", "layout", "other"]; @@ -81,6 +112,82 @@ const TYPE_SPECIMENS = [

+ +
+

Rulebook drift

+ +

Checking against the design rulebook…

+ +
+ No design rulebook designated. +

+ This install hasn't said which rulebook describes its design system, so + there is nothing to check the tokens against. Designate one in + Settings and this panel will + compare every colour and token the rulebook names against what the + stylesheet actually resolves to. +

+
+ + +
+

Components

@@ -307,6 +414,41 @@ const TYPE_SPECIMENS = [ color: var(--color-priority-medium); } +.spec-status.violated { + background: var(--color-priority-high-bg); + color: var(--color-priority-high); +} + +.spec-status.ok { + background: var(--color-status-done-bg); + color: var(--color-status-done); +} + +.spec-name .swatch { + vertical-align: middle; + margin-right: 0.4rem; +} + +.matches { + color: var(--color-text-muted); +} + +.reveal-toggle { + margin-top: 0.75rem; + padding: 0.35rem 0.75rem; + background: transparent; + color: var(--color-text-secondary); + border: 1px solid var(--color-border); + border-radius: var(--radius-sm); + cursor: pointer; + font-family: inherit; + font-size: 0.8rem; +} + +.reveal-toggle:hover { + border-color: var(--color-text-muted); +} + /* Tokens ----------------------------------------------------------------- */ .token-list { diff --git a/frontend/src/views/SettingsView.vue b/frontend/src/views/SettingsView.vue index 4c7c8d8..fbcf524 100644 --- a/frontend/src/views/SettingsView.vue +++ b/frontend/src/views/SettingsView.vue @@ -4,6 +4,7 @@ import { useSettingsStore } from "@/stores/settings"; import { useAuthStore } from "@/stores/auth"; import { useToastStore } from "@/stores/toast"; import { apiGet, apiPost, apiPut, apiDelete, listGroups, createGroup, deleteGroup, listGroupMembers, addGroupMember, removeGroupMember, searchUsers, listApiKeys, createApiKey as apiCreateApiKey, revokeApiKey as apiRevokeApiKey, getProfile, updateProfile, type ApiKeyEntry, type GroupEntry, type GroupMember, type UserSearchResult, type UserProfile } from "@/api/client"; +import { listRulebooks } from "@/api/rulebooks"; import type { User } from "@/types/auth"; import PaginationBar from "@/components/PaginationBar.vue"; import TagInput from "@/components/TagInput.vue"; @@ -31,6 +32,11 @@ const kbWritePathThreshold = ref("0.68"); // gate: that one BLOCKS a create and must be unforgiving of noise, this one only // suggests a merge the operator reviews (services/dedup.py). const kbDuplicateThreshold = ref("0.82"); +// Which rulebook describes this install's design system, for the /design drift +// panel. Empty = none designated, which is the normal state for a fresh install +// rather than a misconfiguration — the panel explains itself when unset. +const designRulebookId = ref(""); +const designRulebooks = ref<{ id: number; title: string }[]>([]); const savingKbInject = ref(false); const kbInjectSaved = ref(false); @@ -100,6 +106,9 @@ async function saveKbInject() { kb_writepath_enabled: kbWritePathEnabled.value ? 'true' : 'false', kb_writepath_threshold: String(wpT), kb_duplicate_threshold: String(dupT), + // Empty string DELETES the setting (see routes/settings.py), which is + // exactly right for "no design rulebook" — absent rather than zero. + design_rulebook_id: designRulebookId.value, }); kbInjectSaved.value = true; setTimeout(() => (kbInjectSaved.value = false), 2000); @@ -490,6 +499,14 @@ onMounted(async () => { if (allSettings.kb_duplicate_threshold !== undefined) { kbDuplicateThreshold.value = allSettings.kb_duplicate_threshold; } + designRulebookId.value = allSettings.design_rulebook_id ?? ""; + // Best-effort: the picker degrades to "none available" rather than blocking + // the whole settings page if rulebooks can't be listed. + try { + designRulebooks.value = (await listRulebooks()).map((r) => ({ id: r.id, title: r.title })); + } catch { + designRulebooks.value = []; + } if (allSettings.notify_task_reminders !== undefined) { notifyTaskReminders.value = allSettings.notify_task_reminders !== "false"; } @@ -1260,6 +1277,22 @@ function formatUserDate(iso: string): string { location, not by resemblance.

+
+ + +

+ Which rulebook describes how this app should look. Once set, the + Design page compares every colour + and token your rules name against what the stylesheet actually resolves + to, and reports where they disagree. Leave it as None if your rules + don't describe a design system — nothing else depends on this. +

+
Date: Thu, 30 Jul 2026 16:54:41 -0400 Subject: [PATCH 06/16] feat(design-systems): the model, the parent chain, and the guard on it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #254 step 1 (#2286). A design system becomes a record Scribe holds rather than prose in a rulebook: a named set of tokens with an OPTIONAL parent, so a family system carries the house style and an app system carries only what it changes. Answering "what does this app alter?" is then `list its tokens` — nothing to compute. `parent_id` is the whole model. It replaces both an `always_on` flag (a family system is one with no parent) and a subscription join table (a project points at ONE system; the chain supplies the rest) — less schema than the rulebook shape it mirrors. Two decisions the task left open, settled here: - **Token values are JSONB keyed by mode**, not `value_light`/`value_dark` columns. The deciding argument was not flexibility, it was ambiguity: in a child system an unset mode means "inherit", in a root it means "not mode-dependent", and as columns both are NULL and the resolver cannot tell them apart. As a map, resolution is `{**parent, **child}` at every level with no special case for roots. Against it: queryability — but nothing filters tokens by value in SQL, so that buys a query no caller makes. - **`group_name` is free text, no CHECK enum.** Groupings are each design system's own vocabulary; a whitelist would bake one install's kit into the schema. No CHECK is introduced anywhere, so rule #36 does not fire. The cascade lives in `services/design_cascade.py` as pure functions over a `{id: parent_id}` map, importing nothing — which is what lets both the service and `access.py` use it without a cycle, and lets a test state a whole hierarchy in one literal. Cycles are refused on WRITE by walking up from the proposed parent (the cheap direction), and survived on READ by a visited-set, because a loop from a direct DB edit must truncate rather than hang. ACL (rule #78) is deliberately asymmetric: owning a system grants write, reaching one through a project you can see grants READ ONLY. An editor on a shared project must not be able to rewrite the family system every other project in that family resolves through. Also renames `services/design_system.py` -> `design_rulebook_import.py`. It is the #251 prose extractor, whose role is already scheduled to become a one-shot importer (#2288), and leaving it one character away from the new `design_systems.py` was a trap for every later session. Rule #115 throughout: nothing seeds a system or implies a default. An install with zero design systems is ordinary, not degraded. --- alembic/versions/0072_design_systems.py | 138 +++++++++ src/scribe/models/__init__.py | 3 + src/scribe/models/design_system.py | 130 +++++++++ src/scribe/models/project.py | 9 +- src/scribe/routes/design.py | 2 +- src/scribe/services/access.py | 84 ++++++ src/scribe/services/design_cascade.py | 61 ++++ ...gn_system.py => design_rulebook_import.py} | 0 src/scribe/services/design_systems.py | 271 ++++++++++++++++++ tests/test_design_cascade.py | 94 ++++++ ...stem.py => test_design_rulebook_import.py} | 2 +- tests/test_services_access_visibility.py | 37 +++ tests/test_services_design_systems.py | 194 +++++++++++++ 13 files changed, 1022 insertions(+), 3 deletions(-) create mode 100644 alembic/versions/0072_design_systems.py create mode 100644 src/scribe/models/design_system.py create mode 100644 src/scribe/services/design_cascade.py rename src/scribe/services/{design_system.py => design_rulebook_import.py} (100%) create mode 100644 src/scribe/services/design_systems.py create mode 100644 tests/test_design_cascade.py rename tests/{test_design_system.py => test_design_rulebook_import.py} (99%) create mode 100644 tests/test_services_design_systems.py diff --git a/alembic/versions/0072_design_systems.py b/alembic/versions/0072_design_systems.py new file mode 100644 index 0000000..80e467c --- /dev/null +++ b/alembic/versions/0072_design_systems.py @@ -0,0 +1,138 @@ +"""design systems + tokens, and the project pointer + +Revision ID: 0072 +Revises: 0071 +Create Date: 2026-07-30 + +Makes the design system a first-class record instead of prose in a rulebook: a +named set of tokens with an optional parent, so a family system holds the house +style and an app system holds only what it changes. + +`design_systems.parent_id` is the whole model. It replaces both an `always_on` +flag (a family system is one with no parent) and a subscription join table (a +project points at ONE system, and the chain supplies the rest), which is less +schema than the rulebook shape it mirrors. + +Two deliberate choices worth stating here rather than leaving to be re-derived: + + - **`design_tokens.value_by_mode` is JSONB keyed by mode**, not `value_light` + + `value_dark` columns. In a child system an unset mode means "inherit"; in a + root it would mean "not mode-dependent", and as columns both are NULL and + indistinguishable. As a map, resolution is a dict merge at every level with + no special case for roots — and a third mode (high-contrast, print) is data + rather than a schema change. The cost is that a typo'd mode key is not + rejected by the database. Nothing filters tokens by value in SQL, so the + queryability the columns would have bought is for a query no caller makes. + (Named `value_by_mode` rather than `values`, which is reserved in SQL.) + - **`group_name` is free text, not a CHECK enum.** Groupings are each design + system's own vocabulary; a whitelist would bake one install's kit into the + schema. No CHECK is introduced anywhere in this migration. + +`parent_id` and `projects.design_system_id` are both ON DELETE SET NULL. Deleting +a family system must orphan its children into roots that still hold their own +overrides, not cascade away every app system that inherited from it; deleting a +system a project points at must unstyle that project, not delete it. + +Downgrade drops both tables and the column. Any design system defined this way +is lost — this is the migration that introduces the concept, so there is no +earlier representation to fall back to. +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects.postgresql import JSONB + + +revision = "0072" +down_revision = "0071" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "design_systems", + sa.Column("id", sa.BigInteger(), primary_key=True), + sa.Column( + "owner_user_id", sa.BigInteger(), + sa.ForeignKey("users.id", ondelete="CASCADE"), nullable=False, + ), + sa.Column("title", sa.Text(), nullable=False), + sa.Column("description", sa.Text(), nullable=True), + sa.Column( + "parent_id", sa.BigInteger(), + sa.ForeignKey("design_systems.id", ondelete="SET NULL"), nullable=True, + ), + sa.Column( + "created_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + sa.Column( + "updated_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("deleted_batch_id", sa.Text(), nullable=True), + ) + op.create_index( + "ix_design_systems_owner_user_id", "design_systems", ["owner_user_id"] + ) + op.create_index("ix_design_systems_parent_id", "design_systems", ["parent_id"]) + + op.create_table( + "design_tokens", + sa.Column("id", sa.BigInteger(), primary_key=True), + sa.Column( + "design_system_id", sa.BigInteger(), + sa.ForeignKey("design_systems.id", ondelete="CASCADE"), nullable=False, + ), + sa.Column("name", sa.Text(), nullable=False), + # NOT NULL with a '{}' default: a nullable JSONB column has two empty + # states (SQL NULL and JSON null) and every reader has to test for both. + sa.Column( + "value_by_mode", JSONB, + nullable=False, server_default=sa.text("'{}'::jsonb"), + ), + sa.Column("group_name", sa.Text(), nullable=True), + sa.Column("purpose", sa.Text(), nullable=True), + sa.Column("order_index", sa.Integer(), nullable=False, server_default="0"), + sa.Column( + "created_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + sa.Column( + "updated_at", sa.DateTime(timezone=True), nullable=False, + server_default=sa.text("now()"), + ), + sa.Column("deleted_at", sa.DateTime(timezone=True), nullable=True), + sa.Column("deleted_batch_id", sa.Text(), nullable=True), + ) + op.create_index( + "ix_design_tokens_design_system_id", "design_tokens", ["design_system_id"] + ) + # Partial unique: a name is unique among LIVE tokens in a system. Two live + # rows with the same name are a duplicate definition and the cascade would + # pick between them arbitrarily; a trashed row must not block reusing its + # name. + op.create_index( + "uq_token_per_design_system", "design_tokens", ["design_system_id", "name"], + unique=True, postgresql_where=sa.text("deleted_at IS NULL"), + ) + + op.add_column( + "projects", sa.Column("design_system_id", sa.BigInteger(), nullable=True) + ) + op.create_foreign_key( + "fk_projects_design_system_id", "projects", "design_systems", + ["design_system_id"], ["id"], ondelete="SET NULL", + ) + + +def downgrade() -> None: + op.drop_constraint("fk_projects_design_system_id", "projects", type_="foreignkey") + op.drop_column("projects", "design_system_id") + op.drop_index("uq_token_per_design_system", table_name="design_tokens") + op.drop_index("ix_design_tokens_design_system_id", table_name="design_tokens") + op.drop_table("design_tokens") + op.drop_index("ix_design_systems_parent_id", table_name="design_systems") + op.drop_index("ix_design_systems_owner_user_id", table_name="design_systems") + op.drop_table("design_systems") diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index 2297cdd..f4528a8 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -43,3 +43,6 @@ from scribe.models.rulebook import ( # noqa: E402, F401 ) from scribe.models.repo_binding import RepoBinding # noqa: E402, F401 from scribe.models.system import System, RecordSystem # noqa: E402, F401 +from scribe.models.design_system import ( # noqa: E402, F401 + BASE_MODE, DesignSystem, DesignToken, +) diff --git a/src/scribe/models/design_system.py b/src/scribe/models/design_system.py new file mode 100644 index 0000000..7c91a02 --- /dev/null +++ b/src/scribe/models/design_system.py @@ -0,0 +1,130 @@ +"""Design systems — a stylesheet held as records, inherited family -> app. + +A DesignSystem is a named set of design tokens with an OPTIONAL parent, and that +single self-FK is the whole model. A system with no parent is a family system; a +system WITH one holds only what it changes. "What does this app alter?" is +therefore `list its tokens` — nothing to compute, nothing to diff — which is why +inheritance won over a flat family-plus-loose-overrides shape. + +Resolution walks the chain and lets the deepest system win by token name. That +is the CSS cascade rather than an analogy to it, which is why the storage model +and the stylesheet model come out the same shape. + +`parent_id` also replaces two things the rulebook model needs to express the same +idea: an `always_on` flag (a family system is simply one with no parent) and a +subscription join table (a project points at ONE system, and the chain supplies +the rest). Less schema for more structure. +""" +from sqlalchemy import BigInteger, ForeignKey, Index, Integer, Text, text +from sqlalchemy.dialects.postgresql import JSONB +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base +from scribe.models.base import SoftDeleteMixin, TimestampMixin + +# The mode key that applies when no more specific one does. Emitted CSS puts it +# on the base selector and every other key on a mode selector, mirroring how a +# stylesheet is actually written: light on `:root`, dark layered over it. +BASE_MODE = "base" + + +class DesignSystem(Base, TimestampMixin, SoftDeleteMixin): + __tablename__ = "design_systems" + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + owner_user_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("users.id", ondelete="CASCADE"), index=True + ) + title: Mapped[str] = mapped_column(Text) + description: Mapped[str | None] = mapped_column(Text, nullable=True) + # SET NULL, not CASCADE: deleting a family system must not delete every app + # system that inherited from it. Orphaning turns each child into a root that + # still holds its own overrides — recoverable. A cascade would destroy data + # the operator never asked to touch. + parent_id: Mapped[int | None] = mapped_column( + BigInteger, + ForeignKey("design_systems.id", ondelete="SET NULL"), + nullable=True, + index=True, + ) + + def to_dict(self) -> dict: + return { + "id": self.id, + "owner_user_id": self.owner_user_id, + "title": self.title, + "description": self.description or "", + "parent_id": self.parent_id, + "created_at": self.created_at.isoformat() if self.created_at else None, + "updated_at": self.updated_at.isoformat() if self.updated_at else None, + } + + +class DesignToken(Base, TimestampMixin, SoftDeleteMixin): + """One custom property in one system: its name, and its value per mode. + + `value_by_mode` is a JSONB map of mode -> value, e.g. + `{"base": "#f7f5ef", "dark": "#14171a"}`. Three reasons it beats a pair of + `value_light` / `value_dark` columns here: + + - **Absence means one thing.** In a child system an unset mode means + "inherit"; in a root it would have to mean "not mode-dependent". With + columns those are both NULL and the resolver cannot tell them apart. With + a map, resolution is `{**parent_map, **child_map}` at every level — + one rule, no special case for roots. + - **Per-mode overrides are already real.** A palette rule in this operator's + own kit deepens one accent on light backgrounds for contrast while + leaving the dark value alone. Mode is a second override axis, not a + second column. + - **Nothing filters tokens by value in SQL.** Drift comparison resolves the + set first and compares in the client; the importer diffs in Python. The + queryability columns would buy is for a query no caller makes. + + The cost is real — a third mode is data rather than schema, so the DB will + not reject a typo'd mode key. That is the trade taken. + + NOT NULL with a `{}` default deliberately: a JSONB column otherwise has two + empty states (SQL NULL and JSON null) and code has to test for both. + """ + __tablename__ = "design_tokens" + __table_args__ = ( + # Partial unique: a name is unique among LIVE tokens in a system, so a + # trashed token doesn't block recreating the same name. Two live rows + # named `--fs-obsidian` in one system is a duplicate definition, and the + # cascade would pick between them arbitrarily. + Index( + "uq_token_per_design_system", "design_system_id", "name", + unique=True, postgresql_where=text("deleted_at IS NULL"), + ), + ) + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + design_system_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("design_systems.id", ondelete="CASCADE"), index=True + ) + name: Mapped[str] = mapped_column(Text) + # Named for what it holds rather than the bare word `values`, which is + # reserved in SQL — the same reason `group_name` is not `group`. + value_by_mode: Mapped[dict] = mapped_column( + JSONB, nullable=False, default=dict, server_default=text("'{}'::jsonb") + ) + # `group` is a reserved word in SQL; `group_name` throughout — model, column + # and payload — so no layer has to remember which spelling it is on. + # Free text, not a CHECK enum: groupings are the design system's own + # vocabulary, and a whitelist would bake one install's kit into the schema. + group_name: Mapped[str | None] = mapped_column(Text, nullable=True) + purpose: Mapped[str | None] = mapped_column(Text, nullable=True) + order_index: Mapped[int] = mapped_column(Integer, default=0, server_default="0") + + def to_dict(self) -> dict: + return { + "id": self.id, + "design_system_id": self.design_system_id, + "name": self.name, + "value_by_mode": self.value_by_mode or {}, + "group_name": self.group_name, + "purpose": self.purpose, + "order_index": self.order_index, + "created_at": self.created_at.isoformat() if self.created_at else None, + "updated_at": self.updated_at.isoformat() if self.updated_at else None, + } diff --git a/src/scribe/models/project.py b/src/scribe/models/project.py index 8ccacc5..d05a165 100644 --- a/src/scribe/models/project.py +++ b/src/scribe/models/project.py @@ -1,5 +1,5 @@ import enum -from sqlalchemy import ForeignKey, Integer, Text +from sqlalchemy import BigInteger, ForeignKey, Integer, Text from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base from scribe.models.base import TimestampMixin, SoftDeleteMixin @@ -21,6 +21,12 @@ class Project(Base, TimestampMixin, SoftDeleteMixin): goal: Mapped[str] = mapped_column(Text, default="") status: Mapped[str] = mapped_column(Text, default="active") color: Mapped[str | None] = mapped_column(Text, nullable=True) # hex color + # The design system this project's UI is built from, or NULL. NULL is the + # ordinary state, not a degraded one — most installs have no design system + # at all and nothing may assume one exists. + design_system_id: Mapped[int | None] = mapped_column( + BigInteger, ForeignKey("design_systems.id", ondelete="SET NULL"), nullable=True + ) def to_dict(self) -> dict: return { @@ -31,6 +37,7 @@ class Project(Base, TimestampMixin, SoftDeleteMixin): "goal": self.goal, "status": self.status, "color": self.color, + "design_system_id": self.design_system_id, "created_at": self.created_at.isoformat(), "updated_at": self.updated_at.isoformat(), } diff --git a/src/scribe/routes/design.py b/src/scribe/routes/design.py index 33b3146..630e710 100644 --- a/src/scribe/routes/design.py +++ b/src/scribe/routes/design.py @@ -7,7 +7,7 @@ resolved. This endpoint supplies the claims to check them against. from quart import Blueprint, jsonify from scribe.auth import get_current_user_id, login_required -from scribe.services import design_system as design_svc +from scribe.services import design_rulebook_import as design_svc design_bp = Blueprint("design", __name__, url_prefix="/api/design") diff --git a/src/scribe/services/access.py b/src/scribe/services/access.py index bb39d61..2a43374 100644 --- a/src/scribe/services/access.py +++ b/src/scribe/services/access.py @@ -12,11 +12,13 @@ import logging from sqlalchemy import or_, select from scribe.models import async_session +from scribe.models.design_system import DesignSystem from scribe.models.group import GroupMembership from scribe.models.note import Note from scribe.models.project import Project from scribe.models.share import NoteShare, ProjectShare from scribe.models.user import User +from scribe.services.design_cascade import ancestry logger = logging.getLogger(__name__) @@ -164,6 +166,88 @@ async def can_write_note(user_id: int, note_id: int) -> bool: return perm in ("editor", "admin", "owner") +# --------------------------------------------------------------------------- +# Design-system permissions +# --------------------------------------------------------------------------- + +async def get_design_system_permission( + user_id: int, design_system_id: int +) -> str | None: + """Effective permission on a design system, or None. + + Two ways in, and the asymmetry between them is the point: + + - **Owning it** grants "owner" — full read and write. + - **Reaching it through a project you can see** grants "viewer", and only + ever "viewer". Being an editor on a shared project must NOT confer the + right to rewrite the family system that project inherits from: one + project's collaborator would be editing tokens every other project in + the family resolves through. Editing a design system stays the owner's + act, and it is the same reasoning that keeps a rulebook owner-scoped. + + Reachability follows the parent chain UPWARD. Rendering a project's UI means + resolving its whole chain, so read access to a system implies read access to + its ancestors — otherwise a shared project would resolve to a truncated + cascade and silently render with the wrong values. + """ + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return None + if system.owner_user_id == user_id: + return "owner" + + shared_project_ids = select(ProjectShare.project_id).where( + or_( + ProjectShare.shared_with_user_id == user_id, + ProjectShare.shared_with_group_id.in_(_my_group_ids(user_id)), + ) + ) + entry_points = set( + ( + await session.execute( + select(Project.design_system_id).where( + Project.design_system_id.is_not(None), + Project.deleted_at.is_(None), + or_( + Project.user_id == user_id, + Project.id.in_(shared_project_ids), + ), + ) + ) + ).scalars().all() + ) + if not entry_points: + return None + + # One narrow query for the whole forest's shape. Design systems are a + # handful of rows per install — a family and one per app — so walking + # from each entry point in memory beats a recursive CTE per check. + parents = dict( + ( + await session.execute( + select(DesignSystem.id, DesignSystem.parent_id).where( + DesignSystem.deleted_at.is_(None) + ) + ) + ).all() + ) + + for entry in entry_points: + if design_system_id in ancestry(entry, parents): + return "viewer" + return None + + +async def can_read_design_system(user_id: int, design_system_id: int) -> bool: + return (await get_design_system_permission(user_id, design_system_id)) is not None + + +async def can_write_design_system(user_id: int, design_system_id: int) -> bool: + perm = await get_design_system_permission(user_id, design_system_id) + return perm in ("editor", "admin", "owner") + + # --------------------------------------------------------------------------- # Set-based visibility (for LIST queries) # diff --git a/src/scribe/services/design_cascade.py b/src/scribe/services/design_cascade.py new file mode 100644 index 0000000..8d7f187 --- /dev/null +++ b/src/scribe/services/design_cascade.py @@ -0,0 +1,61 @@ +"""The design-system cascade, as pure functions over already-loaded rows. + +Deliberately free of every database and service import — including +`services/access.py`, which needs `ancestry` to answer "can this caller read +this system?" and would otherwise form an import cycle with the service that +needs `access` back. A module that imports nothing can be imported by both. + +Being pure is also what makes the cascade rule testable without a database: +these take a plain `{id: parent_id}` map, so a test states the shape of a +hierarchy in one literal instead of building one. +""" +from collections.abc import Mapping + + +def ancestry(system_id: int, parents: Mapping[int, int | None]) -> list[int]: + """The inheritance chain from `system_id` up to its root, nearest first. + + Includes `system_id` itself at index 0, because resolution wants + deepest-to-shallowest and the system being resolved is the deepest link. + + A system missing from `parents` terminates the chain rather than raising: a + parent whose row was soft-deleted or filtered out is a truncated chain, not + a failed request. + + The visited-set is defensive, not the primary guard — writes already refuse + to create a cycle (`would_cycle`). It is here because a loop introduced by a + direct DB edit or a future bug must degrade to a truncated chain instead of + spinning forever. Truncation shows up in the result; a hang shows up as an + outage. + """ + chain: list[int] = [] + seen: set[int] = set() + current: int | None = system_id + while current is not None and current not in seen: + seen.add(current) + chain.append(current) + current = parents.get(current) + return chain + + +def would_cycle( + system_id: int, + proposed_parent_id: int | None, + parents: Mapping[int, int | None], +) -> bool: + """Would making `proposed_parent_id` the parent of `system_id` close a loop? + + True when the proposed parent IS the system, or already inherits from it. + + The walk goes UP from the proposed parent, which is the cheap direction — + each system has at most one parent, so the chain is a line. Asking the + equivalent downward question ("is the proposed parent among my + descendants?") would mean searching a whole forest for the same answer. + + `proposed_parent_id=None` clears the parent and can never cycle. + """ + if proposed_parent_id is None: + return False + if proposed_parent_id == system_id: + return True + return system_id in ancestry(proposed_parent_id, parents) diff --git a/src/scribe/services/design_system.py b/src/scribe/services/design_rulebook_import.py similarity index 100% rename from src/scribe/services/design_system.py rename to src/scribe/services/design_rulebook_import.py diff --git a/src/scribe/services/design_systems.py b/src/scribe/services/design_systems.py new file mode 100644 index 0000000..4588d5c --- /dev/null +++ b/src/scribe/services/design_systems.py @@ -0,0 +1,271 @@ +"""Design system + token persistence, and the guard on the parent chain. + +Access goes through `services/access.py` (rule #78), where reaching a system via +a project you can see grants READ but never write — see +`get_design_system_permission` for why that asymmetry exists. + +The cascade itself lives in `services/design_cascade.py` as pure functions. This +module is the part that needs a database: loading the shape of the hierarchy, +refusing writes that would break it, and storing tokens. + +Nothing here seeds or implies a default system. An install with no design +systems is an ordinary install, and every caller must handle an empty list as +the normal case rather than a missing prerequisite. +""" +import logging +from datetime import datetime, timezone + +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.design_system import DesignSystem, DesignToken +from scribe.models.project import Project +from scribe.services import access +from scribe.services.design_cascade import would_cycle + +logger = logging.getLogger(__name__) + + +class DesignSystemCycle(ValueError): + """A parent assignment that would close an inheritance loop. + + Raised rather than returned as None because the two outcomes need different + answers: None already means "not found, or not yours", and a caller that + conflated them would show "no such design system" for what is really "that + parent is one of its own descendants". Routes map this to a 400. + """ + + +async def _parent_map(session, user_id: int) -> dict[int, int | None]: + """`{id: parent_id}` for the caller's live systems — the hierarchy's shape. + + Owner-scoped, which is also the constraint on parenting: you can only build + a chain out of systems you own. A borrowed link would let someone else's + delete or re-parent silently restyle your app. + """ + rows = ( + await session.execute( + select(DesignSystem.id, DesignSystem.parent_id).where( + DesignSystem.owner_user_id == user_id, + DesignSystem.deleted_at.is_(None), + ) + ) + ).all() + return dict(rows) + + +# --- design systems --------------------------------------------------------- + +async def create_design_system( + user_id: int, + title: str, + description: str | None = None, + parent_id: int | None = None, +) -> DesignSystem | None: + """Create a system, with or without a parent. + + Returns None when `parent_id` names a system the caller may not write — + which, per the ACL, means one they do not own. + """ + if parent_id is not None and not await access.can_write_design_system( + user_id, parent_id + ): + return None + async with async_session() as session: + system = DesignSystem( + owner_user_id=user_id, + title=title.strip(), + description=description, + parent_id=parent_id, + ) + session.add(system) + await session.commit() + await session.refresh(system) + return system + + +async def get_design_system(user_id: int, design_system_id: int) -> DesignSystem | None: + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return None + if not await access.can_read_design_system(user_id, design_system_id): + return None + return system + + +async def list_design_systems(user_id: int) -> list[DesignSystem]: + """The caller's own systems, ordered by title. Empty is normal.""" + async with async_session() as session: + rows = await session.execute( + select(DesignSystem) + .where( + DesignSystem.owner_user_id == user_id, + DesignSystem.deleted_at.is_(None), + ) + .order_by(DesignSystem.title) + ) + return list(rows.scalars().all()) + + +async def update_design_system( + user_id: int, design_system_id: int, **fields: object +) -> DesignSystem | None: + """Partial update. Raises DesignSystemCycle if `parent_id` would loop. + + `parent_id` is handled apart from the other fields because None is a + meaningful value for it — "make this a root" — where for every other field + None means "leave alone". Callers signal it by passing the key at all. + """ + if not await access.can_write_design_system(user_id, design_system_id): + return None + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return None + + if "parent_id" in fields: + parent_id = fields.pop("parent_id") + if parent_id is not None: + parent_id = int(parent_id) + if not await access.can_write_design_system(user_id, parent_id): + return None + if would_cycle( + design_system_id, parent_id, await _parent_map(session, user_id) + ): + raise DesignSystemCycle( + f"Design system {design_system_id} cannot inherit from " + f"{parent_id}: that system already inherits from it." + ) + system.parent_id = parent_id + + for key, value in fields.items(): + if key in ("title", "description") and value is not None: + setattr(system, key, value) + system.updated_at = datetime.now(timezone.utc) + await session.commit() + await session.refresh(system) + return system + + +async def delete_design_system(user_id: int, design_system_id: int) -> bool: + """Soft-delete a system. Children survive as roots (the FK is SET NULL).""" + if not await access.can_write_design_system(user_id, design_system_id): + return False + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return False + system.deleted_at = datetime.now(timezone.utc) + await session.commit() + return True + + +# --- tokens ----------------------------------------------------------------- + +async def create_token( + user_id: int, + design_system_id: int, + name: str, + value_by_mode: dict | None = None, + group_name: str | None = None, + purpose: str | None = None, + order_index: int = 0, +) -> DesignToken | None: + if not await access.can_write_design_system(user_id, design_system_id): + return None + async with async_session() as session: + token = DesignToken( + design_system_id=design_system_id, + name=name.strip(), + # `or {}` and not the argument as given: the column is NOT NULL so + # that absence has exactly one spelling. Passing None here would + # otherwise store JSON null and reintroduce the second empty state. + value_by_mode=value_by_mode or {}, + group_name=group_name, + purpose=purpose, + order_index=order_index, + ) + session.add(token) + await session.commit() + await session.refresh(token) + return token + + +async def list_tokens(user_id: int, design_system_id: int) -> list[DesignToken]: + """One system's OWN tokens — its override set, not its effective set. + + Resolving the chain is step 2's job; this deliberately answers the narrower + question ("what does this system change?") that the model exists to make + free. + """ + if not await access.can_read_design_system(user_id, design_system_id): + return [] + async with async_session() as session: + rows = await session.execute( + select(DesignToken) + .where( + DesignToken.design_system_id == design_system_id, + DesignToken.deleted_at.is_(None), + ) + .order_by(DesignToken.order_index.asc(), DesignToken.name.asc()) + ) + return list(rows.scalars().all()) + + +async def update_token( + user_id: int, token_id: int, **fields: object +) -> DesignToken | None: + allowed = {"name", "value_by_mode", "group_name", "purpose", "order_index"} + async with async_session() as session: + token = await session.get(DesignToken, token_id) + if token is None or token.deleted_at is not None: + return None + if not await access.can_write_design_system(user_id, token.design_system_id): + return None + for key, value in fields.items(): + if key in allowed and value is not None: + setattr(token, key, value) + token.updated_at = datetime.now(timezone.utc) + await session.commit() + await session.refresh(token) + return token + + +async def delete_token(user_id: int, token_id: int) -> bool: + async with async_session() as session: + token = await session.get(DesignToken, token_id) + if token is None or token.deleted_at is not None: + return False + if not await access.can_write_design_system(user_id, token.design_system_id): + return False + token.deleted_at = datetime.now(timezone.utc) + await session.commit() + return True + + +# --- the project pointer ---------------------------------------------------- + +async def set_project_design_system( + user_id: int, project_id: int, design_system_id: int | None +) -> bool: + """Point a project at a design system, or at nothing (None clears it). + + Needs write on the project and READ on the system: pointing at a system is + consuming it, not changing it, so a system shared with you through another + project is a legitimate choice here. + """ + if not await access.can_write_project(user_id, project_id): + return False + if design_system_id is not None and not await access.can_read_design_system( + user_id, design_system_id + ): + return False + async with async_session() as session: + project = await session.get(Project, project_id) + if project is None or project.deleted_at is not None: + return False + project.design_system_id = design_system_id + project.updated_at = datetime.now(timezone.utc) + await session.commit() + return True diff --git a/tests/test_design_cascade.py b/tests/test_design_cascade.py new file mode 100644 index 0000000..b00936c --- /dev/null +++ b/tests/test_design_cascade.py @@ -0,0 +1,94 @@ +"""The parent chain: walking it, and refusing to close it (milestone #254 step 1). + +Pure functions over a `{id: parent_id}` literal, so a whole hierarchy is one line +of setup and no database is involved. That is the reason the cascade lives in its +own import-free module — see services/design_cascade.py. +""" +from scribe.services.design_cascade import ancestry, would_cycle + + +# --- ancestry --------------------------------------------------------------- + +def test_ancestry_returns_the_chain_deepest_first(): + """Deepest first because that is the order resolution consumes it in: the + system being resolved wins over its parent, which wins over the root.""" + parents = {1: None, 2: 1, 3: 2} + assert ancestry(3, parents) == [3, 2, 1] + + +def test_a_root_is_its_own_whole_chain(): + """A family system has no parent, and that is an ordinary state — not an + incomplete one. It must resolve to exactly itself.""" + assert ancestry(1, {1: None}) == [1] + + +def test_a_missing_parent_truncates_rather_than_raising(): + """A parent that was soft-deleted (or filtered out of the caller's scope) is + a shorter chain, not a failed request. The alternative — raising — would make + one deleted system break every descendant's rendering.""" + assert ancestry(3, {3: 2}) == [3, 2] + + +def test_a_system_absent_from_the_map_still_yields_itself(): + assert ancestry(9, {}) == [9] + + +def test_ancestry_terminates_on_a_cycle_instead_of_hanging(): + """LOAD-BEARING, and the reason a visited-set exists even though writes are + guarded. A loop introduced by a direct DB edit or a future bug must degrade + to a truncated chain: truncation shows up in the result, a hang shows up as + an outage. Every id appears exactly once.""" + parents = {1: 3, 2: 1, 3: 2} + chain = ancestry(1, parents) + assert chain == [1, 3, 2] + assert len(chain) == len(set(chain)) + + +def test_ancestry_terminates_on_a_self_parent(): + assert ancestry(1, {1: 1}) == [1] + + +# --- would_cycle ------------------------------------------------------------ + +def test_clearing_the_parent_never_cycles(): + """None means "make this a root", which is always safe.""" + assert would_cycle(2, None, {1: None, 2: 1}) is False + + +def test_a_system_cannot_be_its_own_parent(): + assert would_cycle(1, 1, {1: None}) is True + + +def test_an_ordinary_reparent_is_allowed(): + """family <- app is the shape the whole model exists for; it must not trip + the guard.""" + assert would_cycle(2, 1, {1: None, 2: None}) is False + + +def test_a_direct_swap_is_refused(): + """A -> B, then B -> A. The two-system case, and the one a UI produces first + because both systems are on screen together.""" + parents = {1: None, 2: 1} + assert would_cycle(1, 2, parents) is True + + +def test_an_indirect_loop_is_refused(): + """Three deep: root <- mid <- leaf, then root's parent set to leaf. Catching + this is what makes the check a chain walk rather than a parent comparison.""" + parents = {1: None, 2: 1, 3: 2} + assert would_cycle(1, 3, parents) is True + + +def test_reparenting_onto_a_sibling_subtree_is_allowed(): + """Two branches off one root. Moving one under the other is legitimate — the + guard must refuse loops, not reorganisation.""" + parents = {1: None, 2: 1, 3: 1} + assert would_cycle(3, 2, parents) is False + + +def test_the_guard_survives_a_hierarchy_that_is_already_corrupt(): + """If a cycle somehow already exists, the guard still has to answer rather + than spin — the write path is exactly where such a hierarchy gets repaired.""" + parents = {1: 2, 2: 1, 3: None} + assert would_cycle(3, 1, parents) is False + assert would_cycle(1, 2, parents) is True diff --git a/tests/test_design_system.py b/tests/test_design_rulebook_import.py similarity index 99% rename from tests/test_design_system.py rename to tests/test_design_rulebook_import.py index 0b29af2..9a0c942 100644 --- a/tests/test_design_system.py +++ b/tests/test_design_rulebook_import.py @@ -11,7 +11,7 @@ be testing the instance, not the parser. """ from types import SimpleNamespace -from scribe.services.design_system import ( +from scribe.services.design_rulebook_import import ( expand_token_shorthand, extract_expectations, normalize_hex, diff --git a/tests/test_services_access_visibility.py b/tests/test_services_access_visibility.py index 1b9a6d0..e036fad 100644 --- a/tests/test_services_access_visibility.py +++ b/tests/test_services_access_visibility.py @@ -132,3 +132,40 @@ def test_clauses_are_pure_builders(clause_fn): import inspect assert not inspect.iscoroutinefunction(clause_fn) assert _sql(clause_fn(7)) # builds without touching a database + + +# --- design systems ---------------------------------------------------------- +# +# A design system is reachable two ways: you own it, or you can see a project +# that inherits from it. The gap between what those two grant is the invariant +# worth guarding — see get_design_system_permission. + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "permission, readable, writable", + [ + ("owner", True, True), + # Project-derived. Being an EDITOR on a shared project must not confer + # the right to rewrite the family system that project inherits from — + # that would let one project's collaborator restyle every other project + # in the family. + ("viewer", True, False), + (None, False, False), + ], +) +async def test_reaching_a_design_system_via_a_project_reads_but_never_writes( + permission, readable, writable +): + from unittest.mock import AsyncMock, patch + + from scribe.services.access import ( + can_read_design_system, + can_write_design_system, + ) + + with patch( + "scribe.services.access.get_design_system_permission", + AsyncMock(return_value=permission), + ): + assert await can_read_design_system(1, 3) is readable + assert await can_write_design_system(1, 3) is writable diff --git a/tests/test_services_design_systems.py b/tests/test_services_design_systems.py new file mode 100644 index 0000000..9d8bdc5 --- /dev/null +++ b/tests/test_services_design_systems.py @@ -0,0 +1,194 @@ +"""ACL gating and field handling for services/design_systems.py (unit, mocked). + +Mirrors tests/test_services_systems.py — same mocked-session shape, because the +question is the same one: does the service refuse before it touches a row? +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.services.design_systems import DesignSystemCycle + + +def _make_mock_session(): + s = AsyncMock() + s.__aenter__ = AsyncMock(return_value=s) + s.__aexit__ = AsyncMock(return_value=False) + s.add = MagicMock() + s.commit = AsyncMock() + s.refresh = AsyncMock() + return s + + +# --- creating --------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_create_with_a_parent_is_denied_without_write_on_that_parent(): + """Parenting to someone else's system would let their delete or re-parent + silently restyle your app, so the chain may only be built from systems you + can write — which, per the ACL, means ones you own.""" + with patch("scribe.services.design_systems.access") as acc: + acc.can_write_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import create_design_system + result = await create_design_system(user_id=1, title="Scribe", parent_id=7) + assert result is None + + +@pytest.mark.asyncio +async def test_create_without_a_parent_never_consults_the_parent_acl(): + """A family system has no parent, and creating one must not be gated on a + permission check for a system that does not exist.""" + mock_session = _make_mock_session() + captured = {} + mock_session.add = MagicMock( + side_effect=lambda obj: captured.update( + title=obj.title, parent_id=obj.parent_id + ) + ) + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc: + acc.can_write_design_system = AsyncMock(return_value=False) + mock_cls.return_value = mock_session + from scribe.services.design_systems import create_design_system + await create_design_system(user_id=1, title=" FabledSword ") + assert captured["title"] == "FabledSword" # stripped + assert captured["parent_id"] is None + acc.can_write_design_system.assert_not_awaited() + + +# --- the cycle guard, through the service ----------------------------------- + +@pytest.mark.asyncio +async def test_reparenting_into_a_loop_raises_rather_than_returning_none(): + """None already means "not found, or not yours". A caller that conflated the + two would report "no such design system" for what is really "that parent is + one of its own descendants", so the cycle gets its own exception type.""" + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=MagicMock(deleted_at=None, parent_id=None)) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.design_systems._parent_map", + AsyncMock(return_value={1: None, 2: 1})): + acc.can_write_design_system = AsyncMock(return_value=True) + mock_cls.return_value = mock_session + from scribe.services.design_systems import update_design_system + with pytest.raises(DesignSystemCycle): + await update_design_system(user_id=1, design_system_id=1, parent_id=2) + + mock_session.commit.assert_not_awaited() # refused BEFORE the write + + +@pytest.mark.asyncio +async def test_clearing_the_parent_is_allowed_and_is_not_read_as_no_change(): + """`parent_id=None` means "make this a root" — the one field where None is a + value rather than "leave alone", which is why it is handled apart from the + others.""" + system = MagicMock(deleted_at=None, parent_id=5) + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=system) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.design_systems._parent_map", + AsyncMock(return_value={1: 5, 5: None})): + acc.can_write_design_system = AsyncMock(return_value=True) + mock_cls.return_value = mock_session + from scribe.services.design_systems import update_design_system + await update_design_system(user_id=1, design_system_id=1, parent_id=None) + + assert system.parent_id is None + + +# --- tokens ----------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_create_token_denied_without_write_on_the_system(): + with patch("scribe.services.design_systems.access") as acc: + acc.can_write_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import create_token + result = await create_token(user_id=1, design_system_id=3, name="--fs-obsidian") + assert result is None + + +@pytest.mark.asyncio +async def test_token_values_default_to_an_empty_map_not_json_null(): + """The column is NOT NULL so that absence has exactly ONE spelling. Passing + None straight through would store JSON null and hand every reader back the + second empty state the schema was shaped to remove.""" + mock_session = _make_mock_session() + captured = {} + mock_session.add = MagicMock( + side_effect=lambda obj: captured.update(value_by_mode=obj.value_by_mode) + ) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc: + acc.can_write_design_system = AsyncMock(return_value=True) + mock_cls.return_value = mock_session + from scribe.services.design_systems import create_token + await create_token( + user_id=1, design_system_id=3, name="--fs-radius-md", value_by_mode=None + ) + + assert captured["value_by_mode"] == {} + + +@pytest.mark.asyncio +async def test_list_tokens_denied_returns_empty(): + with patch("scribe.services.design_systems.access") as acc: + acc.can_read_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import list_tokens + assert await list_tokens(user_id=1, design_system_id=3) == [] + + +# --- the project pointer ---------------------------------------------------- + +@pytest.mark.asyncio +async def test_pointing_a_project_at_a_system_needs_write_on_the_project(): + with patch("scribe.services.design_systems.access") as acc: + acc.can_write_project = AsyncMock(return_value=False) + acc.can_read_design_system = AsyncMock(return_value=True) + from scribe.services.design_systems import set_project_design_system + assert await set_project_design_system(1, project_id=5, design_system_id=3) is False + + +@pytest.mark.asyncio +async def test_pointing_a_project_needs_only_READ_on_the_system(): + """Consuming a design system is not changing it, so a system reachable + through another project is a legitimate choice here. Requiring write would + make a shared family style unusable by the people it was shared with.""" + project = MagicMock(deleted_at=None, design_system_id=None) + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=project) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc: + acc.can_write_project = AsyncMock(return_value=True) + acc.can_read_design_system = AsyncMock(return_value=True) + acc.can_write_design_system = AsyncMock(return_value=False) + mock_cls.return_value = mock_session + from scribe.services.design_systems import set_project_design_system + assert await set_project_design_system(1, project_id=5, design_system_id=3) is True + + assert project.design_system_id == 3 + + +@pytest.mark.asyncio +async def test_clearing_a_projects_design_system_skips_the_system_acl(): + """Un-styling a project must not require permission on the system it is + letting go of — including one that has since been deleted.""" + project = MagicMock(deleted_at=None, design_system_id=3) + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=project) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems.access") as acc: + acc.can_write_project = AsyncMock(return_value=True) + acc.can_read_design_system = AsyncMock(return_value=False) + mock_cls.return_value = mock_session + from scribe.services.design_systems import set_project_design_system + assert await set_project_design_system(1, project_id=5, design_system_id=None) is True + + assert project.design_system_id is None + acc.can_read_design_system.assert_not_awaited() -- 2.54.0 From 839d6902ad8d8b89b20d5d794c70ba39209ffd4f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 17:02:31 -0400 Subject: [PATCH 07/16] feat(design-systems): resolve the chain, and keep the argument not the verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #254 step 2 (#2287). `resolve_tokens` flattens a system's inheritance chain into its effective token set — walk to the root, deepest wins by token name. Pure and duck-typed, so a test states a whole hierarchy in literals and the service hands the same function ORM rows. **Provenance is stored as the contest, not the winner.** A ResolvedToken carries every system that offered a value, per mode, deepest first — `[0]` won and `[1:]` are what it shadowed. "Which system supplied this?" and "what did it override?" are then two reads of one list and cannot disagree, where a winner plus a separate provenance field would be two things to keep in step. **Merging is per (name, MODE), and that is the storage decision paying off.** A system that deepens one accent for light backgrounds while leaving dark alone owns `base` and still inherits `dark`. A token-level "overridden here" flag would have to lie about one of them, and the two-column shape could not have represented it at all. Metadata cascades separately by the same deepest-wins rule, with one exception: `order_index` treats 0 as UNSTATED rather than "first", because 0 is the column default. Reading it as a real value would let a colour-only override drag its token to the top of its group — a visible reshuffle in return for a change that touched nothing structural. One fix to step 1 while wiring this up: `_parent_map` is now scoped to the SYSTEM'S OWNER rather than the caller. A caller reading through a shared project owns no link in the chain, so the caller-scoped version would have handed them an empty forest and truncated the cascade to a single system — a page rendering with plausible wrong values and no error anywhere. The ACL already grants read along the whole chain; this is the loading side keeping that promise, and it now has a test naming the shared-project case. `BASE_MODE` moves from the model to the cascade module, where it belongs: it is a resolution rule, not a storage fact, and design_cascade.py deliberately imports nothing so both access.py and the service can depend on it. --- src/scribe/models/__init__.py | 4 +- src/scribe/models/design_system.py | 5 - src/scribe/services/design_cascade.py | 151 +++++++++++++++++- src/scribe/services/design_systems.py | 70 +++++++- tests/test_design_cascade.py | 220 +++++++++++++++++++++++++- tests/test_services_design_systems.py | 44 ++++++ 6 files changed, 476 insertions(+), 18 deletions(-) diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index f4528a8..0135af2 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -43,6 +43,4 @@ from scribe.models.rulebook import ( # noqa: E402, F401 ) from scribe.models.repo_binding import RepoBinding # noqa: E402, F401 from scribe.models.system import System, RecordSystem # noqa: E402, F401 -from scribe.models.design_system import ( # noqa: E402, F401 - BASE_MODE, DesignSystem, DesignToken, -) +from scribe.models.design_system import DesignSystem, DesignToken # noqa: E402, F401 diff --git a/src/scribe/models/design_system.py b/src/scribe/models/design_system.py index 7c91a02..6df3680 100644 --- a/src/scribe/models/design_system.py +++ b/src/scribe/models/design_system.py @@ -22,11 +22,6 @@ from sqlalchemy.orm import Mapped, mapped_column from scribe.models import Base from scribe.models.base import SoftDeleteMixin, TimestampMixin -# The mode key that applies when no more specific one does. Emitted CSS puts it -# on the base selector and every other key on a mode selector, mirroring how a -# stylesheet is actually written: light on `:root`, dark layered over it. -BASE_MODE = "base" - class DesignSystem(Base, TimestampMixin, SoftDeleteMixin): __tablename__ = "design_systems" diff --git a/src/scribe/services/design_cascade.py b/src/scribe/services/design_cascade.py index 8d7f187..fd016f0 100644 --- a/src/scribe/services/design_cascade.py +++ b/src/scribe/services/design_cascade.py @@ -9,7 +9,13 @@ Being pure is also what makes the cascade rule testable without a database: these take a plain `{id: parent_id}` map, so a test states the shape of a hierarchy in one literal instead of building one. """ -from collections.abc import Mapping +from collections.abc import Mapping, Sequence +from dataclasses import dataclass + +# The mode key that applies when no more specific one does. Emitted CSS puts it +# on the base selector and every other key on a mode selector, mirroring how a +# stylesheet is actually written: light on `:root`, dark layered over it. +BASE_MODE = "base" def ancestry(system_id: int, parents: Mapping[int, int | None]) -> list[int]: @@ -59,3 +65,146 @@ def would_cycle( if proposed_parent_id == system_id: return True return system_id in ancestry(proposed_parent_id, parents) + + +# --------------------------------------------------------------------------- +# Resolution — flattening a chain into an effective token set +# --------------------------------------------------------------------------- + +@dataclass(frozen=True) +class Contribution: + """One system's offer for one token in one mode.""" + system_id: int + value: str + + +@dataclass(frozen=True) +class ResolvedToken: + """A token after the cascade, carrying the whole argument rather than the verdict. + + `contributions` holds every system that supplied a value, per mode, DEEPEST + FIRST — so `[0]` is the winner and `[1:]` are what it shadowed. Storing the + contest rather than a winner plus a separate provenance field means there is + nothing to keep in sync: "which system supplied this?" and "what did it + override?" are both reads of the same list, and they cannot disagree. + + Provenance is per MODE, not per token, because overriding is. A system that + deepens one accent for light backgrounds while leaving dark alone owns the + base value and inherits the dark one, and a token-level "overridden here" + flag would have to lie about one of them. + """ + name: str + contributions: dict[str, tuple[Contribution, ...]] + group_name: str | None + purpose: str | None + order_index: int + + @property + def value_by_mode(self) -> dict[str, str]: + """The effective value for each mode — the winner of each contest.""" + return {mode: entries[0].value for mode, entries in self.contributions.items()} + + @property + def origin_by_mode(self) -> dict[str, int]: + """Which system supplied each mode's effective value.""" + return {mode: entries[0].system_id for mode, entries in self.contributions.items()} + + def value_for(self, mode: str) -> str | None: + """The value to render in `mode`, falling back to the base mode. + + This is the read rule the storage shape implies: a token that is not + mode-dependent carries only `base`, and asking it for "dark" must yield + the base value rather than nothing. + """ + entries = self.contributions.get(mode) or self.contributions.get(BASE_MODE) + return entries[0].value if entries else None + + def is_overridden_in(self, system_id: int) -> bool: + """Does `system_id` win any mode of this token AND shadow something? + + The distinction the UI needs: a token this system introduced is not an + override, and a token it merely inherits is not either. + """ + return any( + len(entries) > 1 and entries[0].system_id == system_id + for entries in self.contributions.values() + ) + + +def _sort_key(token: ResolvedToken) -> tuple: + # Ungrouped tokens sort last rather than first: a design system that has + # started grouping should read as its groups, with the not-yet-filed + # remainder at the end. + return (token.group_name is None, token.group_name or "", token.order_index, token.name) + + +def resolve_tokens( + system_id: int, + parents: Mapping[int, int | None], + tokens_by_system: Mapping[int, Sequence], +) -> list[ResolvedToken]: + """Flatten a system's inheritance chain into its effective token set. + + Walks from `system_id` up to the root and applies tokens by name, deepest + winning. That is the CSS cascade — precedence by name along a parent chain — + rather than an analogy to it, which is why the storage model and the + stylesheet model came out the same shape. + + `tokens_by_system` maps a system id to its own token rows. Any object with + `.name`, `.value_by_mode`, `.group_name`, `.purpose` and `.order_index` will + do, so a test can state a hierarchy in literals and the service can pass ORM + rows to the same function. + + The result includes tokens the system never mentions — inheriting one is + what puts it in the effective set. A system with no tokens of its own + resolves to its parent's set entire, which is the correct answer for an app + that has not departed from the family yet. + + Merging is per (name, MODE): a child that supplies only a dark value + overrides only dark and keeps inheriting base. Metadata (`group_name`, + `purpose`, `order_index`) cascades separately by the same deepest-wins rule, + since a child overriding a value routinely leaves the family's description + of what the token is FOR untouched — and inheriting it beats blanking it. + """ + chain = ancestry(system_id, parents) + + contributions: dict[str, dict[str, list[Contribution]]] = {} + metadata: dict[str, dict[str, object]] = {} + + # Deepest first, so the first contribution seen for a (name, mode) wins and + # every later one is a shadowed ancestor appended behind it. + for depth_system_id in chain: + for token in tokens_by_system.get(depth_system_id) or (): + per_mode = contributions.setdefault(token.name, {}) + for mode, value in (token.value_by_mode or {}).items(): + per_mode.setdefault(mode, []).append( + Contribution(system_id=depth_system_id, value=value) + ) + + meta = metadata.setdefault( + token.name, {"group_name": None, "purpose": None, "order_index": None} + ) + for field in ("group_name", "purpose"): + if meta[field] is None: + meta[field] = getattr(token, field, None) + # order_index alone treats 0 as UNSTATED rather than "first", + # because 0 is the column default. Reading it as a real value would + # let any child override drag its token to the top of the group and + # lose the family's ordering — a visible reshuffle in return for a + # change that only touched a colour. + if not meta["order_index"]: + meta["order_index"] = getattr(token, "order_index", 0) or None + + resolved = [ + ResolvedToken( + name=name, + contributions={ + mode: tuple(entries) for mode, entries in per_mode.items() + }, + group_name=metadata[name]["group_name"], + purpose=metadata[name]["purpose"], + order_index=metadata[name]["order_index"] or 0, + ) + for name, per_mode in contributions.items() + ] + return sorted(resolved, key=_sort_key) diff --git a/src/scribe/services/design_systems.py b/src/scribe/services/design_systems.py index 4588d5c..2aafbd3 100644 --- a/src/scribe/services/design_systems.py +++ b/src/scribe/services/design_systems.py @@ -21,7 +21,12 @@ from scribe.models import async_session from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.project import Project from scribe.services import access -from scribe.services.design_cascade import would_cycle +from scribe.services.design_cascade import ( + ResolvedToken, + ancestry, + resolve_tokens, + would_cycle, +) logger = logging.getLogger(__name__) @@ -36,17 +41,23 @@ class DesignSystemCycle(ValueError): """ -async def _parent_map(session, user_id: int) -> dict[int, int | None]: - """`{id: parent_id}` for the caller's live systems — the hierarchy's shape. +async def _parent_map(session, owner_user_id: int) -> dict[int, int | None]: + """`{id: parent_id}` for one owner's live systems — the hierarchy's shape. - Owner-scoped, which is also the constraint on parenting: you can only build - a chain out of systems you own. A borrowed link would let someone else's - delete or re-parent silently restyle your app. + Scoped to the OWNER of the systems, not the caller, and the distinction is + load-bearing on the read path: a caller reading through a shared project + does not own any link in the chain, and a caller-scoped map would hand them + an empty forest and a cascade truncated to one system. They would get a page + that renders with plausible wrong values and no error anywhere. + + Owner-scoping is also the constraint on parenting: a chain may only be built + from systems its owner controls, or someone else's delete or re-parent would + silently restyle your app. """ rows = ( await session.execute( select(DesignSystem.id, DesignSystem.parent_id).where( - DesignSystem.owner_user_id == user_id, + DesignSystem.owner_user_id == owner_user_id, DesignSystem.deleted_at.is_(None), ) ) @@ -131,7 +142,9 @@ async def update_design_system( if not await access.can_write_design_system(user_id, parent_id): return None if would_cycle( - design_system_id, parent_id, await _parent_map(session, user_id) + design_system_id, + parent_id, + await _parent_map(session, system.owner_user_id), ): raise DesignSystemCycle( f"Design system {design_system_id} cannot inherit from " @@ -213,6 +226,47 @@ async def list_tokens(user_id: int, design_system_id: int) -> list[DesignToken]: return list(rows.scalars().all()) +async def resolve_design_system( + user_id: int, design_system_id: int +) -> list[ResolvedToken] | None: + """A system's EFFECTIVE token set — everything it inherits, with its own on top. + + None when the caller may not read the system; an empty list when the chain + genuinely holds no tokens, which is an ordinary state for a system that has + just been created. + + Two queries regardless of how deep the chain runs: one for the hierarchy's + shape, one for every token in it. The flattening itself is + `design_cascade.resolve_tokens` — pure, so the cascade rule is tested + without a database and this function is only the loading. + """ + if not await access.can_read_design_system(user_id, design_system_id): + return None + async with async_session() as session: + system = await session.get(DesignSystem, design_system_id) + if system is None or system.deleted_at is not None: + return None + + parents = await _parent_map(session, system.owner_user_id) + chain = ancestry(design_system_id, parents) + + rows = ( + await session.execute( + select(DesignToken) + .where( + DesignToken.design_system_id.in_(chain), + DesignToken.deleted_at.is_(None), + ) + .order_by(DesignToken.order_index.asc(), DesignToken.name.asc()) + ) + ).scalars().all() + + tokens_by_system: dict[int, list[DesignToken]] = {} + for token in rows: + tokens_by_system.setdefault(token.design_system_id, []).append(token) + return resolve_tokens(design_system_id, parents, tokens_by_system) + + async def update_token( user_id: int, token_id: int, **fields: object ) -> DesignToken | None: diff --git a/tests/test_design_cascade.py b/tests/test_design_cascade.py index b00936c..99e6169 100644 --- a/tests/test_design_cascade.py +++ b/tests/test_design_cascade.py @@ -4,7 +4,9 @@ Pure functions over a `{id: parent_id}` literal, so a whole hierarchy is one lin of setup and no database is involved. That is the reason the cascade lives in its own import-free module — see services/design_cascade.py. """ -from scribe.services.design_cascade import ancestry, would_cycle +from types import SimpleNamespace + +from scribe.services.design_cascade import ancestry, resolve_tokens, would_cycle # --- ancestry --------------------------------------------------------------- @@ -92,3 +94,219 @@ def test_the_guard_survives_a_hierarchy_that_is_already_corrupt(): parents = {1: 2, 2: 1, 3: None} assert would_cycle(3, 1, parents) is False assert would_cycle(1, 2, parents) is True + + +# --- resolution ------------------------------------------------------------- +# +# The cascade proper (milestone #254 step 2). Hierarchies are stated as literals +# because resolve_tokens is pure and duck-typed — which is the whole reason it +# lives here rather than inside the service. + +def _token(name, value_by_mode, group_name=None, purpose=None, order_index=0): + return SimpleNamespace( + name=name, value_by_mode=value_by_mode, group_name=group_name, + purpose=purpose, order_index=order_index, + ) + + +# A family (1) and an app inheriting from it (2) — the shape the model exists for. +FAMILY, APP = 1, 2 +PARENTS = {FAMILY: None, APP: FAMILY} + + +def _by_name(tokens): + return {t.name: t for t in tokens} + + +def test_a_system_with_no_tokens_of_its_own_inherits_the_whole_family_set(): + """The correct answer for an app that has not departed from the family yet — + and the state every app system starts in.""" + resolved = resolve_tokens( + APP, PARENTS, + {FAMILY: [_token("--fs-obsidian", {"base": "#14171a"})], APP: []}, + ) + assert [t.name for t in resolved] == ["--fs-obsidian"] + assert resolved[0].value_by_mode == {"base": "#14171a"} + assert resolved[0].origin_by_mode == {"base": FAMILY} + + +def test_the_deepest_system_wins_and_says_what_it_overrode(): + """Provenance is the point of the whole model: the effective set is just a + list without it, and "where does this app depart from the family?" becomes a + diff someone has to compute.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.value_by_mode == {"base": "#5b4a8a"} + assert accent.origin_by_mode == {"base": APP} + # The shadowed ancestor is still there, behind the winner. + assert [c.system_id for c in accent.contributions["base"]] == [APP, FAMILY] + assert accent.contributions["base"][1].value == "#6b2118" + + +def test_overriding_one_mode_leaves_the_others_inherited(): + """LOAD-BEARING, and the argument that decided the storage shape. An accent + deepened for light backgrounds while dark is left alone must own `base` and + still inherit `dark` — which is why merging is per (name, MODE) and why the + value column is a map rather than a pair of columns.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#34a877", "dark": "#34a877"})], + APP: [_token("--fs-accent", {"base": "#15803d"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.value_by_mode == {"base": "#15803d", "dark": "#34a877"} + assert accent.origin_by_mode == {"base": APP, "dark": FAMILY} + + +def test_a_token_only_the_app_defines_is_not_an_override(): + """Introducing a token and overriding one are different acts, and a UI that + labelled both "overridden here" would misdescribe the first.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + {FAMILY: [], APP: [_token("--fs-editor-caret", {"base": "#5b4a8a"})]}, + )) + caret = resolved["--fs-editor-caret"] + assert caret.origin_by_mode == {"base": APP} + assert caret.is_overridden_in(APP) is False + + +def test_is_overridden_in_is_true_only_for_the_system_that_shadowed(): + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + accent = resolved["--fs-accent"] + assert accent.is_overridden_in(APP) is True + assert accent.is_overridden_in(FAMILY) is False + + +def test_three_levels_stack_nearest_first(): + """A chain deeper than family->app, to prove the walk isn't a single hop.""" + parents = {1: None, 2: 1, 3: 2} + resolved = _by_name(resolve_tokens( + 3, parents, + { + 1: [_token("--fs-bg", {"base": "a"})], + 2: [_token("--fs-bg", {"base": "b"})], + 3: [_token("--fs-bg", {"base": "c"})], + }, + )) + bg = resolved["--fs-bg"] + assert bg.value_by_mode == {"base": "c"} + assert [c.value for c in bg.contributions["base"]] == ["c", "b", "a"] + + +def test_resolving_the_family_itself_ignores_its_children(): + """Inheritance runs one way. A family system resolved on its own must not + pick up an app's overrides — otherwise every app would restyle the house.""" + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, + { + FAMILY: [_token("--fs-accent", {"base": "#6b2118"})], + APP: [_token("--fs-accent", {"base": "#5b4a8a"})], + }, + )) + assert resolved["--fs-accent"].value_by_mode == {"base": "#6b2118"} + + +def test_value_for_falls_back_to_the_base_mode(): + """A token that isn't mode-dependent carries only `base`, and asking it for + "dark" must yield that rather than nothing — the read rule the storage shape + implies.""" + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, {FAMILY: [_token("--fs-radius-md", {"base": "8px"})]}, + )) + radius = resolved["--fs-radius-md"] + assert radius.value_for("dark") == "8px" + assert radius.value_for("base") == "8px" + + +def test_value_for_prefers_an_explicit_mode_over_the_fallback(): + resolved = _by_name(resolve_tokens( + FAMILY, PARENTS, + {FAMILY: [_token("--fs-bg", {"base": "#f7f5ef", "dark": "#14171a"})]}, + )) + assert resolved["--fs-bg"].value_for("dark") == "#14171a" + + +def test_metadata_is_inherited_when_the_override_leaves_it_blank(): + """A child overriding a colour routinely says nothing about what the token is + FOR. Inheriting the family's description beats blanking it — the override was + about the value, not the meaning.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token( + "--fs-obsidian", {"base": "#14171a"}, + group_name="surface", purpose="page bg, deepest surface", + )], + APP: [_token("--fs-obsidian", {"base": "#101317"})], + }, + )) + obsidian = resolved["--fs-obsidian"] + assert obsidian.group_name == "surface" + assert obsidian.purpose == "page bg, deepest surface" + assert obsidian.value_by_mode == {"base": "#101317"} # the value still won + + +def test_an_override_that_states_metadata_wins_it_too(): + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-x", {"base": "a"}, purpose="family says")], + APP: [_token("--fs-x", {"base": "b"}, purpose="app says")], + }, + )) + assert resolved["--fs-x"].purpose == "app says" + + +def test_an_override_at_default_order_keeps_the_familys_position(): + """order_index 0 is the column DEFAULT, so it reads as unstated. Treating it + as "first" would let a colour-only override drag its token to the top of the + group — a visible reshuffle nobody asked for.""" + resolved = _by_name(resolve_tokens( + APP, PARENTS, + { + FAMILY: [_token("--fs-x", {"base": "a"}, order_index=7)], + APP: [_token("--fs-x", {"base": "b"})], + }, + )) + assert resolved["--fs-x"].order_index == 7 + + +def test_the_effective_set_is_ordered_by_group_then_position_with_ungrouped_last(): + resolved = resolve_tokens( + FAMILY, PARENTS, + {FAMILY: [ + _token("--fs-z", {"base": "1"}), # ungrouped + _token("--fs-b", {"base": "2"}, group_name="text", order_index=1), + _token("--fs-a", {"base": "3"}, group_name="surface", order_index=2), + _token("--fs-c", {"base": "4"}, group_name="surface", order_index=1), + ]}, + ) + assert [t.name for t in resolved] == ["--fs-c", "--fs-a", "--fs-b", "--fs-z"] + + +def test_resolution_terminates_on_a_corrupt_hierarchy(): + """Resolution inherits ancestry's visited-set. A loop reaching this function + must produce a truncated set, not a hung request — the defensive half of the + cycle guard, exercised end to end.""" + parents = {1: 2, 2: 1} + resolved = _by_name(resolve_tokens( + 1, parents, + {1: [_token("--fs-a", {"base": "one"})], 2: [_token("--fs-b", {"base": "two"})]}, + )) + assert set(resolved) == {"--fs-a", "--fs-b"} + # Each system contributes exactly once, not endlessly. + assert len(resolved["--fs-a"].contributions["base"]) == 1 diff --git a/tests/test_services_design_systems.py b/tests/test_services_design_systems.py index 9d8bdc5..ebbc94d 100644 --- a/tests/test_services_design_systems.py +++ b/tests/test_services_design_systems.py @@ -192,3 +192,47 @@ async def test_clearing_a_projects_design_system_skips_the_system_acl(): assert project.design_system_id is None acc.can_read_design_system.assert_not_awaited() + + +# --- resolution (step 2) ---------------------------------------------------- + +@pytest.mark.asyncio +async def test_resolve_denied_returns_none_not_an_empty_set(): + """None and [] mean different things here: "you may not see this" versus + "this chain genuinely holds no tokens". A caller that got [] for a denial + would render an empty design system as though it were real.""" + with patch("scribe.services.design_systems.access") as acc: + acc.can_read_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import resolve_design_system + assert await resolve_design_system(user_id=1, design_system_id=3) is None + + +@pytest.mark.asyncio +async def test_resolve_scopes_the_hierarchy_to_the_systems_OWNER_not_the_caller(): + """LOAD-BEARING for shared projects. A caller reading through a project + shared with them owns no link in the chain, so a caller-scoped parent map + returns an empty forest and the cascade truncates to one system — a page + that renders with plausible wrong values and no error anywhere. + + The ACL already grants read on the whole chain (reachability propagates + upward); this is the loading side keeping that promise. + """ + owner, caller = 42, 7 + system = MagicMock(deleted_at=None, owner_user_id=owner) + mock_session = _make_mock_session() + mock_session.get = AsyncMock(return_value=system) + mock_session.execute = AsyncMock( + return_value=MagicMock(scalars=MagicMock(return_value=MagicMock(all=lambda: []))) + ) + parent_map = AsyncMock(return_value={3: None}) + + with patch("scribe.services.design_systems.async_session") as mock_cls, \ + patch("scribe.services.design_systems._parent_map", parent_map), \ + patch("scribe.services.design_systems.access") as acc: + acc.can_read_design_system = AsyncMock(return_value=True) + mock_cls.return_value = mock_session + from scribe.services.design_systems import resolve_design_system + result = await resolve_design_system(user_id=caller, design_system_id=3) + + assert result == [] # empty chain, not None + assert parent_map.await_args.args[1] == owner # NOT `caller` -- 2.54.0 From 143b968c5d87f77b9c7113a3df3b3138778bc83b Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 17:09:25 -0400 Subject: [PATCH 08/16] feat(design-systems): REST + MCP surfaces, at parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #254 step 4 (#2290). Eleven capabilities, both surfaces, one service. Design systems are owner-scoped top-level records rather than project-scoped ones, so these do not nest under /api/projects/ the way systems do — routes/rulebooks.py was the closer shape. The one exception is the project pointer, which is genuinely about a project: PUT /api/projects//design-system, PUT rather than PATCH because clearing it is a first-class outcome and not an omission. `/resolved` and `/tokens` are deliberately separate endpoints. One answers "what does this system CHANGE", the other "what does it end up BEING", and a system that overrides nothing has an empty token list and a full resolved set. Shipping only one would have made the other a client-side computation of exactly the kind the record model exists to remove. ResolvedToken.to_dict carries the SHADOWED contributions, not just the winner. Dropping them at the serialisation boundary would have discarded the one thing step 2 was built to preserve, and it would have been invisible — the payload still looks complete. Three sentinel translations on the MCP side, each tested, because an agent cannot omit an argument and a wrong mapping here is silent: - parent_id: 0 = unchanged, -1 = clear (become a family system), positive = set. Renaming a system must not silently re-root it. - order_index: -1 = unchanged, since 0 is a valid position. - value_by_mode: guarded on `is not None`, not truthiness, so `{}` can strip every mode from a token instead of being unreachable. DesignSystemCycle maps to 400 on REST and to a ValueError carrying the message on MCP — kept apart from 404 throughout. An agent told "not found" retries the same call; one told what the loop is can fix it. Two structural guards beyond the parity list: every endpoint must be reachable on the app (catching a decorator copied without its path, where the second handler silently never runs), and every public coroutine in the tools module must be registered (a tool written but never registered is invisible to an agent, and nothing else would notice). --- src/scribe/app.py | 2 + src/scribe/mcp/tools/__init__.py | 4 +- src/scribe/mcp/tools/design_systems.py | 266 +++++++++++++++++++++++++ src/scribe/routes/design_systems.py | 189 ++++++++++++++++++ src/scribe/services/design_cascade.py | 24 +++ tests/test_mcp_tool_design_systems.py | 191 ++++++++++++++++++ tests/test_routes_design_systems.py | 106 ++++++++++ 7 files changed, 781 insertions(+), 1 deletion(-) create mode 100644 src/scribe/mcp/tools/design_systems.py create mode 100644 src/scribe/routes/design_systems.py create mode 100644 tests/test_mcp_tool_design_systems.py create mode 100644 tests/test_routes_design_systems.py diff --git a/src/scribe/app.py b/src/scribe/app.py index 5f665ab..c5ac59a 100644 --- a/src/scribe/app.py +++ b/src/scribe/app.py @@ -27,6 +27,7 @@ from scribe.routes.knowledge import knowledge_bp from scribe.routes.rulebooks import rulebooks_bp from scribe.routes.plugin import plugin_bp from scribe.routes.design import design_bp +from scribe.routes.design_systems import design_systems_bp from scribe.routes.trash import trash_bp from scribe.routes.dashboard import dashboard_bp from scribe.routes.systems import systems_bp @@ -91,6 +92,7 @@ def create_app() -> Quart: app.register_blueprint(rulebooks_bp) app.register_blueprint(plugin_bp) app.register_blueprint(design_bp) + app.register_blueprint(design_systems_bp) app.register_blueprint(trash_bp) app.register_blueprint(dashboard_bp) app.register_blueprint(systems_bp) diff --git a/src/scribe/mcp/tools/__init__.py b/src/scribe/mcp/tools/__init__.py index b9b8dc3..f53e73f 100644 --- a/src/scribe/mcp/tools/__init__.py +++ b/src/scribe/mcp/tools/__init__.py @@ -5,7 +5,8 @@ to a FastMCP instance. `register_all(mcp)` is the single entry point called from `mcp.server.build_mcp_server`. """ from scribe.mcp.tools import ( - milestones, notes, processes, projects, recent, repos, rulebooks, search, snippets, systems, tags, tasks, trash, + design_systems, milestones, notes, processes, projects, recent, repos, rulebooks, search, snippets, + systems, tags, tasks, trash, ) @@ -17,6 +18,7 @@ def register_all(mcp) -> None: projects.register(mcp) milestones.register(mcp) systems.register(mcp) + design_systems.register(mcp) tags.register(mcp) recent.register(mcp) repos.register(mcp) diff --git a/src/scribe/mcp/tools/design_systems.py b/src/scribe/mcp/tools/design_systems.py new file mode 100644 index 0000000..25a948b --- /dev/null +++ b/src/scribe/mcp/tools/design_systems.py @@ -0,0 +1,266 @@ +"""Design system + token MCP tools — wrappers over services/design_systems.py. + +A design system is a stylesheet held as records: a named set of tokens with an +optional parent, so a family system carries the house style and an app system +carries only what it changes. Precedence by name along the parent chain IS the +CSS cascade, which is why "what does this app alter?" is a plain list rather +than a diff. + +Parity with the REST surface is a rule, not a nicety (see +`tests/test_routes_design_systems.py`): an agent and a browser are two callers +of one service. + +Sentinels, matching the milestone/task tool conventions: + - title="" / description="" / etc. -> "leave unchanged" on update + - parent_id / design_system_id: 0 = leave unchanged, -1 = clear, positive = set + (three states, because clearing a parent is a real operation and not the + same as omitting the argument) + - order_index=-1 -> "leave unchanged" (0 is a valid order_index) +""" +from __future__ import annotations + +from scribe.mcp._context import current_user_id +from scribe.services import design_systems as ds_svc +from scribe.services.design_systems import DesignSystemCycle + + +async def create_design_system( + title: str, + description: str = "", + parent_id: int = 0, +) -> dict: + """Create a design system, optionally inheriting from another. + + Args: + title: What this system is, e.g. "FabledSword" or "Scribe" (required). + description: What it covers and when it applies. + parent_id: Inherit from this system — it holds the defaults this one + overrides. Omit (0) for a top-level "family" system, which is what + a first design system usually is. + """ + uid = current_user_id() + system = await ds_svc.create_design_system( + uid, + title=title, + description=description or None, + parent_id=parent_id or None, + ) + if system is None: + raise ValueError(f"parent design system {parent_id} not found or not writable") + return system.to_dict() + + +async def list_design_systems() -> dict: + """List your design systems. An empty list is normal — most installs have none.""" + uid = current_user_id() + rows = await ds_svc.list_design_systems(uid) + return {"design_systems": [s.to_dict() for s in rows]} + + +async def get_design_system(design_system_id: int) -> dict: + """Fetch a design system plus its OWN tokens — i.e. what it changes. + + For what it actually resolves to once inheritance is applied, use + `resolve_design_system`. The two answer different questions and a system + that overrides nothing has an empty token list but a full resolved set. + """ + uid = current_user_id() + system = await ds_svc.get_design_system(uid, design_system_id) + if system is None: + raise ValueError(f"design system {design_system_id} not found") + tokens = await ds_svc.list_tokens(uid, design_system_id) + return { + "design_system": system.to_dict(), + "tokens": [t.to_dict() for t in tokens], + } + + +async def resolve_design_system(design_system_id: int) -> dict: + """The EFFECTIVE token set — everything inherited, with this system's on top. + + Each token carries `origin_by_mode` (which system supplied each mode's + value) and `contributions` (every system that offered one, deepest first — + so entry 0 won and the rest were shadowed). Reach for this when you need to + know what a value actually IS; reach for `get_design_system` when you need + to know what this system CHANGES. + """ + uid = current_user_id() + resolved = await ds_svc.resolve_design_system(uid, design_system_id) + if resolved is None: + raise ValueError(f"design system {design_system_id} not found") + return { + "design_system_id": design_system_id, + "tokens": [t.to_dict() for t in resolved], + } + + +async def update_design_system( + design_system_id: int, + title: str = "", + description: str = "", + parent_id: int = 0, +) -> dict: + """Update a design system. + + Args: + design_system_id: The system to update. + title: New title, or "" to leave unchanged. + description: New description, or "" to leave unchanged. + parent_id: 0 = leave unchanged, -1 = clear (make this a top-level + family system), positive = inherit from that system. A parent that + already inherits from this system is refused — that would be a loop. + """ + uid = current_user_id() + fields: dict = {} + if title: + fields["title"] = title + if description: + fields["description"] = description + if parent_id: + fields["parent_id"] = None if parent_id == -1 else parent_id + try: + system = await ds_svc.update_design_system(uid, design_system_id, **fields) + except DesignSystemCycle as exc: + raise ValueError(str(exc)) from exc + if system is None: + raise ValueError(f"design system {design_system_id} not found or not writable") + return system.to_dict() + + +async def delete_design_system(design_system_id: int) -> dict: + """Soft-delete a design system (recoverable). + + Systems that inherited from it become top-level systems keeping their own + tokens — deleting a family does not delete the apps under it. + """ + uid = current_user_id() + if not await ds_svc.delete_design_system(uid, design_system_id): + raise ValueError(f"design system {design_system_id} not found or not writable") + return {"message": f"Design system {design_system_id} deleted."} + + +# ── Tokens ────────────────────────────────────────────────────────────── + +async def create_design_token( + design_system_id: int, + name: str, + value_by_mode: dict | None = None, + group_name: str = "", + purpose: str = "", + order_index: int = 0, +) -> dict: + """Add a token to a design system. + + Args: + design_system_id: The system that owns this token. + name: The custom-property name, e.g. "--fs-obsidian" (required). + value_by_mode: Values keyed by mode, e.g. + {"base": "#f7f5ef", "dark": "#14171a"}. Use "base" for the value + that applies when no mode is more specific; a token that is not + mode-dependent needs only "base". In a system WITH a parent, an + omitted mode is inherited rather than blanked. + group_name: Free-text grouping — "surface", "text", "radius", whatever + this system's own vocabulary is. + purpose: What the token is for, e.g. "page bg, deepest surface". + order_index: Display position within its group. + """ + uid = current_user_id() + token = await ds_svc.create_token( + uid, + design_system_id=design_system_id, + name=name, + value_by_mode=value_by_mode, + group_name=group_name or None, + purpose=purpose or None, + order_index=order_index, + ) + if token is None: + raise ValueError(f"design system {design_system_id} not found or not writable") + return token.to_dict() + + +async def list_design_tokens(design_system_id: int) -> dict: + """A design system's OWN tokens — its override set, not its effective set.""" + uid = current_user_id() + rows = await ds_svc.list_tokens(uid, design_system_id) + return {"tokens": [t.to_dict() for t in rows]} + + +async def update_design_token( + token_id: int, + name: str = "", + value_by_mode: dict | None = None, + group_name: str = "", + purpose: str = "", + order_index: int = -1, +) -> dict: + """Update a token. Empty/None args leave a field unchanged. + + `value_by_mode` REPLACES the whole map rather than merging into it, so send + every mode you want the token to keep. + """ + uid = current_user_id() + fields: dict = {} + if name: + fields["name"] = name + if value_by_mode is not None: + fields["value_by_mode"] = value_by_mode + if group_name: + fields["group_name"] = group_name + if purpose: + fields["purpose"] = purpose + if order_index >= 0: + fields["order_index"] = order_index + token = await ds_svc.update_token(uid, token_id, **fields) + if token is None: + raise ValueError(f"design token {token_id} not found or not writable") + return token.to_dict() + + +async def delete_design_token(token_id: int) -> dict: + """Soft-delete a token (recoverable). + + In a system with a parent this restores inheritance: the token stops being + overridden here and resolves to the parent's value again. + """ + uid = current_user_id() + if not await ds_svc.delete_token(uid, token_id): + raise ValueError(f"design token {token_id} not found or not writable") + return {"message": f"Design token {token_id} deleted."} + + +async def set_project_design_system(project_id: int, design_system_id: int = 0) -> dict: + """Point a project at a design system. + + Args: + project_id: The project to style. + design_system_id: The system it uses, or -1 to clear it. Pointing at a + system only requires READ access to it — consuming a design system + is not changing it. + """ + uid = current_user_id() + target = None if design_system_id == -1 else design_system_id + ok = await ds_svc.set_project_design_system(uid, project_id, target) + if not ok: + raise ValueError( + f"project {project_id} not writable, or design system " + f"{design_system_id} not found" + ) + return {"project_id": project_id, "design_system_id": target} + + +def register(mcp) -> None: + for fn in ( + create_design_system, + list_design_systems, + get_design_system, + resolve_design_system, + update_design_system, + delete_design_system, + create_design_token, + list_design_tokens, + update_design_token, + delete_design_token, + set_project_design_system, + ): + mcp.tool(name=fn.__name__)(fn) diff --git a/src/scribe/routes/design_systems.py b/src/scribe/routes/design_systems.py new file mode 100644 index 0000000..6787290 --- /dev/null +++ b/src/scribe/routes/design_systems.py @@ -0,0 +1,189 @@ +"""Design system + token REST endpoints (milestone #254 step 4). + +Wraps `services/design_systems.py`, which owns the ACL and the cycle guard. +Design systems are owner-scoped top-level records rather than project-scoped +ones, so these do NOT nest under `/api/projects/` — `routes/rulebooks.py` is the +closer shape. + +Two failures this layer has to keep apart, which is why the service raises for +one and returns None for the other: + + - `DesignSystemCycle` -> 400 with its message. "That parent already inherits + from this system" is a correctable mistake and the caller needs to be told + which one they made. + - None -> 404, covering both "no such system" and "not yours". Conflating + those two IS the intent: distinguishing them would confirm the existence of + records the caller may not see. +""" +from quart import Blueprint, g, jsonify, request + +from scribe.auth import login_required +from scribe.services import design_systems as ds_svc +from scribe.services.design_systems import DesignSystemCycle + +design_systems_bp = Blueprint("design_systems", __name__, url_prefix="/api") + + +def _uid() -> int: + return g.user.id + + +def _not_found(what: str = "design system"): + return jsonify({"error": f"{what} not found"}), 404 + + +# ── Design systems ────────────────────────────────────────────────────── + +@design_systems_bp.get("/design-systems") +@login_required +async def list_design_systems(): + """The caller's design systems. An empty list is the ordinary state for an + install that has never made one, not an error.""" + rows = await ds_svc.list_design_systems(_uid()) + return jsonify({"design_systems": [s.to_dict() for s in rows]}) + + +@design_systems_bp.post("/design-systems") +@login_required +async def create_design_system(): + data = await request.get_json() or {} + title = (data.get("title") or "").strip() + if not title: + return jsonify({"error": "title is required"}), 400 + system = await ds_svc.create_design_system( + user_id=_uid(), + title=title, + description=data.get("description") or None, + parent_id=data.get("parent_id"), + ) + if system is None: + return jsonify({"error": "parent design system not found"}), 404 + return jsonify(system.to_dict()), 201 + + +@design_systems_bp.get("/design-systems/") +@login_required +async def get_design_system(design_system_id: int): + system = await ds_svc.get_design_system(_uid(), design_system_id) + if system is None: + return _not_found() + return jsonify(system.to_dict()) + + +@design_systems_bp.patch("/design-systems/") +@login_required +async def update_design_system(design_system_id: int): + data = await request.get_json() or {} + fields = {k: v for k, v in data.items() if k in ("title", "description")} + # Presence, not truthiness: `{"parent_id": null}` means "make this a root", + # which a `if data.get("parent_id")` filter would silently drop. + if "parent_id" in data: + fields["parent_id"] = data["parent_id"] + try: + system = await ds_svc.update_design_system(_uid(), design_system_id, **fields) + except DesignSystemCycle as exc: + return jsonify({"error": str(exc)}), 400 + if system is None: + return _not_found() + return jsonify(system.to_dict()) + + +@design_systems_bp.delete("/design-systems/") +@login_required +async def delete_design_system(design_system_id: int): + if not await ds_svc.delete_design_system(_uid(), design_system_id): + return _not_found() + return "", 204 + + +@design_systems_bp.get("/design-systems//resolved") +@login_required +async def resolve_design_system(design_system_id: int): + """The EFFECTIVE token set — everything inherited, with this system's on top. + + Distinct from `/tokens` on purpose: that returns what this system CHANGES, + this returns what it ends up being. Both are real questions and answering + only one would make the other a client-side computation. + """ + resolved = await ds_svc.resolve_design_system(_uid(), design_system_id) + if resolved is None: + return _not_found() + return jsonify({ + "design_system_id": design_system_id, + "tokens": [t.to_dict() for t in resolved], + }) + + +# ── Tokens ────────────────────────────────────────────────────────────── + +@design_systems_bp.get("/design-systems//tokens") +@login_required +async def list_design_tokens(design_system_id: int): + """This system's OWN tokens — its override set, not its effective set.""" + if await ds_svc.get_design_system(_uid(), design_system_id) is None: + return _not_found() + rows = await ds_svc.list_tokens(_uid(), design_system_id) + return jsonify({"tokens": [t.to_dict() for t in rows]}) + + +@design_systems_bp.post("/design-systems//tokens") +@login_required +async def create_design_token(design_system_id: int): + data = await request.get_json() or {} + name = (data.get("name") or "").strip() + if not name: + return jsonify({"error": "name is required"}), 400 + token = await ds_svc.create_token( + user_id=_uid(), + design_system_id=design_system_id, + name=name, + value_by_mode=data.get("value_by_mode"), + group_name=data.get("group_name") or None, + purpose=data.get("purpose") or None, + order_index=data.get("order_index") or 0, + ) + if token is None: + return _not_found() + return jsonify(token.to_dict()), 201 + + +@design_systems_bp.patch("/design-tokens/") +@login_required +async def update_design_token(token_id: int): + data = await request.get_json() or {} + fields = { + k: v for k, v in data.items() + if k in ("name", "value_by_mode", "group_name", "purpose", "order_index") + } + token = await ds_svc.update_token(_uid(), token_id, **fields) + if token is None: + return _not_found("design token") + return jsonify(token.to_dict()) + + +@design_systems_bp.delete("/design-tokens/") +@login_required +async def delete_design_token(token_id: int): + if not await ds_svc.delete_token(_uid(), token_id): + return _not_found("design token") + return "", 204 + + +# ── The project pointer ───────────────────────────────────────────────── + +@design_systems_bp.put("/projects//design-system") +@login_required +async def set_project_design_system(project_id: int): + """Point a project at a design system. `{"design_system_id": null}` clears it. + + PUT rather than PATCH: this sets one field to exactly what is sent, and + clearing it is a first-class outcome rather than an omission. + """ + data = await request.get_json() or {} + ok = await ds_svc.set_project_design_system( + _uid(), project_id, data.get("design_system_id") + ) + if not ok: + return _not_found("project or design system") + return jsonify({"project_id": project_id, + "design_system_id": data.get("design_system_id")}) diff --git a/src/scribe/services/design_cascade.py b/src/scribe/services/design_cascade.py index fd016f0..02f4ceb 100644 --- a/src/scribe/services/design_cascade.py +++ b/src/scribe/services/design_cascade.py @@ -119,6 +119,30 @@ class ResolvedToken: entries = self.contributions.get(mode) or self.contributions.get(BASE_MODE) return entries[0].value if entries else None + def to_dict(self) -> dict: + """Payload shape for both surfaces — and it carries the SHADOWED entries. + + Serialising only the winner would throw away the provenance at the last + step, which is the one thing this type exists to preserve. `contributions` + is the audit trail; `value_by_mode` / `origin_by_mode` are alongside it so + a client renders without re-deriving anything, and cannot derive it + differently. + """ + return { + "name": self.name, + "group_name": self.group_name, + "purpose": self.purpose, + "order_index": self.order_index, + "value_by_mode": self.value_by_mode, + "origin_by_mode": self.origin_by_mode, + "contributions": { + mode: [ + {"system_id": c.system_id, "value": c.value} for c in entries + ] + for mode, entries in self.contributions.items() + }, + } + def is_overridden_in(self, system_id: int) -> bool: """Does `system_id` win any mode of this token AND shadow something? diff --git a/tests/test_mcp_tool_design_systems.py b/tests/test_mcp_tool_design_systems.py new file mode 100644 index 0000000..85c2cc9 --- /dev/null +++ b/tests/test_mcp_tool_design_systems.py @@ -0,0 +1,191 @@ +"""MCP design-system tools — the sentinel translations, mostly. + +The tools are thin wrappers, so the only logic worth testing is where the MCP +calling convention meets the service's: an agent cannot omit an argument, so +"leave unchanged", "clear" and "set" have to be encoded in the value. Getting +that mapping wrong is silent — the call succeeds and changes the wrong thing. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from scribe.mcp._context import _user_id_ctx +from scribe.services.design_systems import DesignSystemCycle + + +@pytest.fixture(autouse=True) +def _bind_user(): + token = _user_id_ctx.set(7) + yield + _user_id_ctx.reset(token) + + +def _fake_system(): + s = MagicMock() + s.to_dict.return_value = {"id": 1, "title": "FabledSword", "parent_id": None} + return s + + +def _fake_token(): + t = MagicMock() + t.to_dict.return_value = {"id": 9, "name": "--fs-obsidian"} + return t + + +# --- create ----------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_creating_without_a_parent_passes_none_not_zero(): + """0 is the "omitted" sentinel, and it must not reach the service as a + system id — there is no system 0, so the create would fail an ACL check for + a record that cannot exist.""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.create_design_system = AsyncMock(return_value=_fake_system()) + from scribe.mcp.tools.design_systems import create_design_system + await create_design_system(title="FabledSword") + assert svc.create_design_system.await_args.kwargs["parent_id"] is None + + +@pytest.mark.asyncio +async def test_creating_with_a_parent_passes_it_through(): + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.create_design_system = AsyncMock(return_value=_fake_system()) + from scribe.mcp.tools.design_systems import create_design_system + await create_design_system(title="Scribe", parent_id=4) + assert svc.create_design_system.await_args.kwargs["parent_id"] == 4 + + +@pytest.mark.asyncio +async def test_create_raises_when_the_parent_is_not_writable(): + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.create_design_system = AsyncMock(return_value=None) + from scribe.mcp.tools.design_systems import create_design_system + with pytest.raises(ValueError): + await create_design_system(title="Scribe", parent_id=4) + + +# --- the three-state parent ------------------------------------------------- + +@pytest.mark.asyncio +async def test_update_with_parent_id_zero_leaves_the_parent_alone(): + """The common case — renaming a system must not silently re-root it.""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_design_system = AsyncMock(return_value=_fake_system()) + from scribe.mcp.tools.design_systems import update_design_system + await update_design_system(design_system_id=1, title="Renamed") + fields = svc.update_design_system.await_args.kwargs + assert "parent_id" not in fields + assert fields["title"] == "Renamed" + + +@pytest.mark.asyncio +async def test_update_with_parent_id_minus_one_clears_it(): + """-1 means "make this a family system". It has to arrive at the service as + None, which is the value the service reads as "become a root" — where + omitting the key means "leave alone".""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_design_system = AsyncMock(return_value=_fake_system()) + from scribe.mcp.tools.design_systems import update_design_system + await update_design_system(design_system_id=1, parent_id=-1) + assert svc.update_design_system.await_args.kwargs["parent_id"] is None + + +@pytest.mark.asyncio +async def test_update_with_a_positive_parent_id_sets_it(): + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_design_system = AsyncMock(return_value=_fake_system()) + from scribe.mcp.tools.design_systems import update_design_system + await update_design_system(design_system_id=1, parent_id=4) + assert svc.update_design_system.await_args.kwargs["parent_id"] == 4 + + +@pytest.mark.asyncio +async def test_a_cycle_surfaces_as_a_usable_error_not_a_not_found(): + """The service raises so this layer can keep the two apart. An agent told + "not found" would retry the same call; one told what the loop is can fix it.""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_design_system = AsyncMock( + side_effect=DesignSystemCycle("2 already inherits from 1") + ) + from scribe.mcp.tools.design_systems import update_design_system + with pytest.raises(ValueError, match="already inherits"): + await update_design_system(design_system_id=1, parent_id=2) + + +# --- tokens ----------------------------------------------------------------- + +@pytest.mark.asyncio +async def test_update_token_treats_order_index_minus_one_as_unchanged(): + """0 is a VALID order_index, so it cannot double as the omitted sentinel — + the same reason the systems tools use -1.""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_token = AsyncMock(return_value=_fake_token()) + from scribe.mcp.tools.design_systems import update_design_token + await update_design_token(token_id=9, purpose="page bg") + fields = svc.update_token.await_args.kwargs + assert "order_index" not in fields + assert fields["purpose"] == "page bg" + + +@pytest.mark.asyncio +async def test_update_token_accepts_order_index_zero(): + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_token = AsyncMock(return_value=_fake_token()) + from scribe.mcp.tools.design_systems import update_design_token + await update_design_token(token_id=9, order_index=0) + assert svc.update_token.await_args.kwargs["order_index"] == 0 + + +@pytest.mark.asyncio +async def test_update_token_can_set_an_empty_value_map(): + """`value_by_mode={}` is meaningful — it strips every mode from a token. The + guard is `is not None`, not truthiness, or that edit would be unreachable.""" + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.update_token = AsyncMock(return_value=_fake_token()) + from scribe.mcp.tools.design_systems import update_design_token + await update_design_token(token_id=9, value_by_mode={}) + assert svc.update_token.await_args.kwargs["value_by_mode"] == {} + + +# --- the project pointer ---------------------------------------------------- + +@pytest.mark.asyncio +async def test_clearing_a_projects_design_system_passes_none(): + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.set_project_design_system = AsyncMock(return_value=True) + from scribe.mcp.tools.design_systems import set_project_design_system + result = await set_project_design_system(project_id=2, design_system_id=-1) + assert svc.set_project_design_system.await_args.args[2] is None + assert result["design_system_id"] is None + + +@pytest.mark.asyncio +async def test_resolve_returns_serialised_tokens_with_their_provenance(): + """The payload has to carry the shadowed entries, not just the winner — + dropping them at the serialisation boundary would discard the one thing + resolution was built to preserve.""" + from scribe.services.design_cascade import resolve_tokens + + class _T: + def __init__(self, name, value_by_mode): + self.name, self.value_by_mode = name, value_by_mode + self.group_name = self.purpose = None + self.order_index = 0 + + resolved = resolve_tokens( + 2, {1: None, 2: 1}, + {1: [_T("--fs-accent", {"base": "#6b2118"})], + 2: [_T("--fs-accent", {"base": "#5b4a8a"})]}, + ) + with patch("scribe.mcp.tools.design_systems.ds_svc") as svc: + svc.resolve_design_system = AsyncMock(return_value=resolved) + from scribe.mcp.tools.design_systems import resolve_design_system + payload = await resolve_design_system(design_system_id=2) + + token = payload["tokens"][0] + assert token["value_by_mode"] == {"base": "#5b4a8a"} + assert token["origin_by_mode"] == {"base": 2} + assert token["contributions"]["base"] == [ + {"system_id": 2, "value": "#5b4a8a"}, + {"system_id": 1, "value": "#6b2118"}, + ] diff --git a/tests/test_routes_design_systems.py b/tests/test_routes_design_systems.py new file mode 100644 index 0000000..f52d4f3 --- /dev/null +++ b/tests/test_routes_design_systems.py @@ -0,0 +1,106 @@ +"""Structural tests for the design-systems blueprint + the parity guard. + +Modelled on tests/test_routes_snippets.py. The enumerations below are +deliberate: relaxing one to a pattern would stop it catching the thing it was +written for, which is a capability landing on one surface and not the other. +""" +import inspect + + +def test_design_systems_blueprint_registered(): + from scribe.routes.design_systems import design_systems_bp + assert design_systems_bp.name == "design_systems" + assert design_systems_bp.url_prefix == "/api" + + +def test_design_systems_blueprint_registered_in_app(): + from scribe.app import create_app + app = create_app() + assert "design_systems" in app.blueprints + + +def test_route_handlers_callable(): + from scribe.routes import design_systems as routes + for name in ( + "list_design_systems", "create_design_system", "get_design_system", + "update_design_system", "delete_design_system", "resolve_design_system", + "list_design_tokens", "create_design_token", "update_design_token", + "delete_design_token", "set_project_design_system", + ): + assert callable(getattr(routes, name)) + + +def test_every_endpoint_is_reachable_on_the_app(): + """The handlers existing is not the same as them being routed. This catches + a decorator that was copied without its path changing — two handlers on one + rule, where the second silently never runs.""" + from scribe.app import create_app + app = create_app() + rules = { + str(r.rule) for r in app.url_map.iter_rules() + if r.endpoint.startswith("design_systems.") + } + assert rules == { + "/api/design-systems", + "/api/design-systems/", + "/api/design-systems//resolved", + "/api/design-systems//tokens", + "/api/design-tokens/", + "/api/projects//design-system", + } + + +def test_service_functions_take_user_id(): + """Routes must call the services with user_id — verify the contract (#33).""" + from scribe.services import design_systems as svc + for fn_name in ( + "create_design_system", "list_design_systems", "get_design_system", + "update_design_system", "delete_design_system", "resolve_design_system", + "create_token", "list_tokens", "update_token", "delete_token", + "set_project_design_system", + ): + fn = getattr(svc, fn_name) + assert callable(fn) + assert "user_id" in inspect.signature(fn).parameters + + +def test_agent_and_web_surfaces_stay_at_parity(): + """The MCP tools and the REST routes are two callers of one service; a + capability on one has to exist on the other (rule #33). + + This guard exists because the snippet surfaces drifted apart once — MCP had + no delete, the web side had no near-duplicate gate — and neither failed + anything until someone went looking. + """ + from scribe.mcp.tools import design_systems as tools + from scribe.routes import design_systems as routes + + for name in ( + "create_design_system", "list_design_systems", "get_design_system", + "resolve_design_system", "update_design_system", "delete_design_system", + "create_design_token", "list_design_tokens", "update_design_token", + "delete_design_token", "set_project_design_system", + ): + assert callable(getattr(tools, name)), f"MCP tool missing: {name}" + assert callable(getattr(routes, name)), f"REST route missing: {name}" + + +def test_every_mcp_tool_in_the_module_is_registered(): + """A tool written but never registered is invisible to an agent, and nothing + else in the codebase would notice.""" + from scribe.mcp.tools import design_systems as tools + + registered = [] + + class _Recorder: + def tool(self, name): + registered.append(name) + return lambda fn: fn + + tools.register(_Recorder()) + + public = { + name for name, obj in vars(tools).items() + if inspect.iscoroutinefunction(obj) and not name.startswith("_") + } + assert set(registered) == public -- 2.54.0 From 0937b1761ec45398e7c91701f788a0f12dc0b343 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 17:17:43 -0400 Subject: [PATCH 09/16] =?UTF-8?q?feat(design-systems):=20the=20editing=20s?= =?UTF-8?q?urface=20=E2=80=94=20overrides,=20effective=20set,=20provenance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #254 step 5 (#2294). /design-systems is the editable half of the surface /design already showed: that page is what the browser renders, this one is the record that ought to decide it. Each links to the other. The layout follows the model rather than decorating it. Two token lists, and they are deliberately different questions: Overrides — the system's own rows. Short by design, and EMPTY is the correct state for an app that hasn't departed from its family yet, so that empty state says so rather than looking unfinished. Effective — what it resolves to with inheritance applied, each row labelled with where its value came from. Provenance renders PER MODE when the modes disagree. A system can own `base` and inherit `dark` at once — that is the case the value column is a map for — and a single badge per row would have to lie about one of them. Rows whose modes agree (the common case) keep the single badge. "Defined here" and "overridden here" are distinct labels. Introducing a token and shadowing an ancestor's are different acts, and `is_overridden_in` is already false for the first. The parent picker filters out the selected system's descendants. The server refuses those anyway with a message naming the loop — but a refusal you cannot trigger beats a refusal explained well. Cycles that arrive some other way still render a truncated chain rather than freezing the tab: the client keeps the same defensive visited-set the server has. Three drift bugs caught while writing the styles, all of the shape this milestone exists to surface: - `--color-accent` does not exist. I had used it for every focus ring and active border; it would have rendered as nothing at all, silently. The brand token is `--color-primary`. - focus rings are ALREADY global in theme.css (`button:focus-visible` et al). My per-element rules would have overridden the house ring with a different one — the exact "bypassed abstraction" shape from #253. - every existing `.btn-primary` copy uses `color: #fff`, which is rule 52's prohibition and 67 live violations (#2275). This one uses Parchment and says why in a comment, rather than becoming the 68th. Also wires the project pointer into ProjectView's details panel, hidden entirely when no design systems exist (rule #115 — that is the ordinary state, not a degraded one) and saved through its own PUT, since clearing it is a real outcome rather than an omission. --- frontend/src/api/designSystems.ts | 116 +++ frontend/src/components/AppHeader.vue | 1 + frontend/src/router/index.ts | 7 + frontend/src/views/DesignSystemsView.vue | 1141 ++++++++++++++++++++++ frontend/src/views/ProjectView.vue | 53 +- 5 files changed, 1313 insertions(+), 5 deletions(-) create mode 100644 frontend/src/api/designSystems.ts create mode 100644 frontend/src/views/DesignSystemsView.vue diff --git a/frontend/src/api/designSystems.ts b/frontend/src/api/designSystems.ts new file mode 100644 index 0000000..af057f4 --- /dev/null +++ b/frontend/src/api/designSystems.ts @@ -0,0 +1,116 @@ +/** + * Design systems — the stylesheet held as records (milestone #254). + * + * A design system is a named set of tokens with an optional parent. A system + * with no parent is a "family"; one with a parent holds ONLY what it changes, + * so "what does this app alter?" is a plain list rather than a diff. + */ +import { apiDelete, apiGet, apiPatch, apiPost, apiPut } from "@/api/client"; + +export interface DesignSystem { + id: number; + owner_user_id: number; + title: string; + description: string; + parent_id: number | null; + created_at: string | null; + updated_at: string | null; +} + +/** A token as STORED — one system's own row for it. */ +export interface DesignToken { + id: number; + design_system_id: number; + name: string; + /** Values keyed by mode. `base` applies when no mode is more specific. */ + value_by_mode: Record; + group_name: string | null; + purpose: string | null; + order_index: number; +} + +export interface Contribution { + system_id: number; + value: string; +} + +/** + * A token after the cascade. + * + * `contributions` is every system that offered a value, per mode, DEEPEST + * FIRST — entry 0 won and the rest were shadowed. `value_by_mode` and + * `origin_by_mode` are the winners, provided so the client never has to derive + * them (and so it cannot derive them differently). + * + * Provenance is per MODE because overriding is: a system can own `base` and + * inherit `dark` at the same time. + */ +export interface ResolvedToken { + name: string; + group_name: string | null; + purpose: string | null; + order_index: number; + value_by_mode: Record; + origin_by_mode: Record; + contributions: Record; +} + +export const fetchDesignSystems = () => + apiGet<{ design_systems: DesignSystem[] }>("/api/design-systems"); + +export const fetchDesignSystem = (id: number) => + apiGet(`/api/design-systems/${id}`); + +export const createDesignSystem = (body: { + title: string; + description?: string; + parent_id?: number | null; +}) => apiPost("/api/design-systems", body); + +/** Omit `parent_id` to leave it alone; send `null` to make the system a family. */ +export const updateDesignSystem = ( + id: number, + body: { title?: string; description?: string; parent_id?: number | null }, +) => apiPatch(`/api/design-systems/${id}`, body); + +export const deleteDesignSystem = (id: number) => + apiDelete(`/api/design-systems/${id}`); + +/** The EFFECTIVE set: everything inherited, with this system's on top. */ +export const fetchResolvedTokens = (id: number) => + apiGet<{ design_system_id: number; tokens: ResolvedToken[] }>( + `/api/design-systems/${id}/resolved`, + ); + +/** This system's OWN tokens — its override set. */ +export const fetchDesignTokens = (id: number) => + apiGet<{ tokens: DesignToken[] }>(`/api/design-systems/${id}/tokens`); + +export const createDesignToken = ( + designSystemId: number, + body: { + name: string; + value_by_mode?: Record; + group_name?: string | null; + purpose?: string | null; + order_index?: number; + }, +) => apiPost(`/api/design-systems/${designSystemId}/tokens`, body); + +export const updateDesignToken = ( + tokenId: number, + body: Partial>, +) => apiPatch(`/api/design-tokens/${tokenId}`, body); + +export const deleteDesignToken = (tokenId: number) => + apiDelete(`/api/design-tokens/${tokenId}`); + +/** Point a project at a design system. `null` clears it. */ +export const setProjectDesignSystem = ( + projectId: number, + designSystemId: number | null, +) => + apiPut<{ project_id: number; design_system_id: number | null }>( + `/api/projects/${projectId}/design-system`, + { design_system_id: designSystemId }, + ); diff --git a/frontend/src/components/AppHeader.vue b/frontend/src/components/AppHeader.vue index 2af5cbc..9afa991 100644 --- a/frontend/src/components/AppHeader.vue +++ b/frontend/src/components/AppHeader.vue @@ -106,6 +106,7 @@ router.afterEach(() => { Shared
Design + Design systems Trash Settings
diff --git a/frontend/src/router/index.ts b/frontend/src/router/index.ts index bedd06c..5e9bf81 100644 --- a/frontend/src/router/index.ts +++ b/frontend/src/router/index.ts @@ -116,6 +116,13 @@ const router = createRouter({ name: "design", component: () => import("@/views/DesignView.vue"), }, + { + // The editable half of the same surface: /design is what the browser + // renders, /design-systems is the record that ought to decide it. + path: "/design-systems", + name: "design-systems", + component: () => import("@/views/DesignSystemsView.vue"), + }, { path: "/tasks", redirect: "/", diff --git a/frontend/src/views/DesignSystemsView.vue b/frontend/src/views/DesignSystemsView.vue new file mode 100644 index 0000000..0d02177 --- /dev/null +++ b/frontend/src/views/DesignSystemsView.vue @@ -0,0 +1,1141 @@ + + + + + diff --git a/frontend/src/views/ProjectView.vue b/frontend/src/views/ProjectView.vue index 4e2d9ea..14aeca2 100644 --- a/frontend/src/views/ProjectView.vue +++ b/frontend/src/views/ProjectView.vue @@ -9,6 +9,11 @@ import { renderMarkdown } from "@/utils/markdown"; import ShareDialog from "@/components/ShareDialog.vue"; import ProjectRulesTab from "@/components/rules/ProjectRulesTab.vue"; import SystemsSection from "@/components/SystemsSection.vue"; +import { + fetchDesignSystems, + setProjectDesignSystem, + type DesignSystem, +} from "@/api/designSystems"; import { LayoutGrid, Clock, @@ -41,6 +46,7 @@ interface Project { goal: string | null; status: "active" | "paused" | "completed" | "archived"; color: string | null; + design_system_id: number | null; permission?: string; created_at: string; updated_at: string; @@ -69,6 +75,12 @@ const toast = useToastStore(); const tasksStore = useTasksStore(); const project = ref(null); + +// Design system the project is styled from. Loaded separately because an +// install with none is the ordinary case (rule #115) and the picker simply +// doesn't render — a failed fetch must not take the project page with it. +const designSystems = ref([]); +const editDesignSystemId = ref(null); const loading = ref(false); const showStartPlanning = ref(false); @@ -175,6 +187,7 @@ async function loadProject() { editDescription.value = data.description ?? ""; editGoal.value = data.goal ?? ""; editStatus.value = data.status; + editDesignSystemId.value = data.design_system_id ?? null; editDirty.value = false; milestones.value = data.summary?.milestone_summary ?? []; autoCollapseCompleted(milestones.value); @@ -336,8 +349,20 @@ onMounted(async () => { await loadProject(); loadTasks(); loadNotes(); + loadDesignSystems(); }); +/** Populate the design-system picker. Swallows failure on purpose: with no + * design systems the picker doesn't render at all, which is the ordinary state + * for most installs — so this must never be able to break the project page. */ +async function loadDesignSystems() { + try { + designSystems.value = (await fetchDesignSystems()).design_systems; + } catch { + designSystems.value = []; + } +} + watch(projectId, async () => { await loadProject(); loadTasks(); @@ -345,28 +370,39 @@ watch(projectId, async () => { }); watch( - () => [editTitle.value, editDescription.value, editGoal.value, editStatus.value], + () => [editTitle.value, editDescription.value, editGoal.value, editStatus.value, editDesignSystemId.value], () => { if (!project.value) return; editDirty.value = editTitle.value !== project.value.title || editDescription.value !== (project.value.description ?? "") || editGoal.value !== (project.value.goal ?? "") || - editStatus.value !== project.value.status; + editStatus.value !== project.value.status || + editDesignSystemId.value !== (project.value.design_system_id ?? null); } ); async function saveProject() { - if (!project.value || saving.value) return; + // Bound once rather than re-read: the checks below straddle two awaits, and + // `project.value` is a ref whose narrowing doesn't survive them. + const current = project.value; + if (!current || saving.value) return; saving.value = true; try { - const updated = await apiPatch(`/api/projects/${project.value.id}`, { + const updated = await apiPatch(`/api/projects/${current.id}`, { title: editTitle.value.trim(), description: editDescription.value.trim() || null, goal: editGoal.value.trim() || null, status: editStatus.value, }); - project.value = { ...project.value, ...updated }; + // The design-system pointer is its own endpoint (PUT, because clearing it + // is a real outcome rather than an omission), so it saves separately — + // only when it actually changed, to keep the common save at one request. + if (editDesignSystemId.value !== (current.design_system_id ?? null)) { + await setProjectDesignSystem(current.id, editDesignSystemId.value); + updated.design_system_id = editDesignSystemId.value; + } + project.value = { ...current, ...updated }; editDirty.value = false; toast.show("Project saved"); } catch { @@ -505,6 +541,13 @@ async function confirmDelete() {
+
+ + +
-- 2.54.0 From 78489308b8879540cf8f41bf2707d915b4f765ae Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 17:17:59 -0400 Subject: [PATCH 10/16] feat(design-systems): link /design to its editable half MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two pages are halves of one surface — what the browser renders and the record that should decide it — and only one direction was linked. Missed in 0937b17 because the patch that added it silently didn't apply; the commit went out without it. --- frontend/src/views/DesignView.vue | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/frontend/src/views/DesignView.vue b/frontend/src/views/DesignView.vue index 0e86b8d..5d137f4 100644 --- a/frontend/src/views/DesignView.vue +++ b/frontend/src/views/DesignView.vue @@ -110,6 +110,11 @@ const TYPE_SPECIMENS = [ the system has no shared implementation, it is marked missing rather than mocked up.

+

+ For the other half — the design system as a record you can edit, with + family → app inheritance — see + design systems. +

-- 2.54.0 From 3da40abcb8b1f675dda17426e5e8cef55f2461a2 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 21:16:45 -0400 Subject: [PATCH 11/16] feat(design-systems): declare what to write instead, rather than what not to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Milestone #254 step 6, first half (#2295) — and this reframes the task rather than answering it. The operator's call: "in this case we should declare what should be used in place of pure white, it's not a prohibition it's what should be used in its place." None of the three options on the table (a constraints record / the panel reads both sources / negative token rows) was right, because all three kept the prohibition as a KIND OF THING. It isn't one. "Pure white is never text" is the shadow cast by a positive fact — text is Parchment — and a design system that stores what things ARE has no row for a ban because it never needed one. So `design_tokens` gains `supersedes`: the literal values this token should be written instead of. `--color-text-on-action` supersedes `#fff` / `#ffffff`. Same fact as the rule, stated forwards, and now actionable — a finding can say what to write rather than only objecting. It has to be DECLARED, not derived, and that is the crux: `#fff` and Parchment `#E8E4D8` are different colours, so no value-matching check could ever have connected them. That mismatch is precisely why the prohibition looked unrepresentable until it was turned around. `supersedes` cascades on EMPTINESS rather than on None. A child overriding a colour says nothing about which literals it replaces, and blanking the family's declaration there would silently disarm the check for every app that customises the token — while a child that states its own list replaces it wholesale. Two things this deliberately does NOT do: - It does not feed the drift panel. Superseded literals live in component CSS, which `designDrift.ts` cannot see and already documents as a blind spot. This is input for the source lint (#2277). Declaring it with nothing consuming it yet is honest; wiring it to a panel that cannot check it would not be. - It does not remove the panel's `prohibited_color` arm yet — that happens when the panel is repointed at a resolved system, which needs #2288 first. The declaration also exposes a missing token: most of the 67 hardcoded `color: #fff` (#2275) are text on a coloured action button, and the system has no token for that role at all. Every view hardcodes it. Declaring the token that was never there is the first real output of the operator's framing. --- .../versions/0073_design_token_supersedes.py | 55 +++++++++++++ frontend/src/api/designSystems.ts | 9 +++ frontend/src/views/DesignSystemsView.vue | 40 ++++++++- src/scribe/mcp/tools/design_systems.py | 18 ++++- src/scribe/models/design_system.py | 19 +++++ src/scribe/routes/design_systems.py | 6 +- src/scribe/services/design_cascade.py | 16 +++- src/scribe/services/design_systems.py | 10 ++- tests/test_design_cascade.py | 81 ++++++++++++++++++- tests/test_mcp_tool_design_systems.py | 21 +++++ tests/test_services_design_systems.py | 38 +++++++++ 11 files changed, 305 insertions(+), 8 deletions(-) create mode 100644 alembic/versions/0073_design_token_supersedes.py diff --git a/alembic/versions/0073_design_token_supersedes.py b/alembic/versions/0073_design_token_supersedes.py new file mode 100644 index 0000000..5596bb2 --- /dev/null +++ b/alembic/versions/0073_design_token_supersedes.py @@ -0,0 +1,55 @@ +"""design_tokens.supersedes — the literals a token should be used instead of + +Revision ID: 0073 +Revises: 0072 +Create Date: 2026-07-30 + +Records what a prohibition was actually trying to say. + +A design rulebook writes "pure white #FFFFFF is NEVER used as text color". That +sentence has no row in a table of tokens, because a design system stores what +things ARE — which looked like a gap in the model and was really a sentence +written backwards. The positive fact is "text is Parchment", and the useful +record is the mapping from the literal someone would otherwise write to the +token they should write instead. + +So `supersedes` is a JSONB array of literal values, e.g. `["#fff", "#ffffff"]` +on a text-on-action token. A finding built from it can say what to write, not +merely what not to. + +It must be DECLARED rather than derived. `#fff` and Parchment `#E8E4D8` are +different colours, so no value-matching rule could ever have connected them — +which is exactly why the prohibition felt unrepresentable until it was turned +around. + +Consumed by the source lint that reads component CSS, not by the drift panel: +these literals are in the components, which the panel cannot see. + +NOT NULL with a `'[]'` default, matching `value_by_mode` — a nullable JSONB +column has two empty states and every reader has to test for both. + +Downgrade drops the column; the declarations are lost, which costs the lint its +input and nothing else. +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects.postgresql import JSONB + + +revision = "0073" +down_revision = "0072" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.add_column( + "design_tokens", + sa.Column( + "supersedes", JSONB, nullable=False, server_default=sa.text("'[]'::jsonb") + ), + ) + + +def downgrade() -> None: + op.drop_column("design_tokens", "supersedes") diff --git a/frontend/src/api/designSystems.ts b/frontend/src/api/designSystems.ts index af057f4..64830ba 100644 --- a/frontend/src/api/designSystems.ts +++ b/frontend/src/api/designSystems.ts @@ -26,6 +26,13 @@ export interface DesignToken { value_by_mode: Record; group_name: string | null; purpose: string | null; + /** Literal values this token should be used INSTEAD OF, e.g. ["#fff"]. + * + * How a design system records what a prohibition was trying to say: not + * "white is banned" but "write this token instead". Declared rather than + * inferred, because a superseded literal and the token's own value are + * usually different values and nothing could connect them by matching. */ + supersedes: string[]; order_index: number; } @@ -49,6 +56,7 @@ export interface ResolvedToken { name: string; group_name: string | null; purpose: string | null; + supersedes: string[]; order_index: number; value_by_mode: Record; origin_by_mode: Record; @@ -93,6 +101,7 @@ export const createDesignToken = ( value_by_mode?: Record; group_name?: string | null; purpose?: string | null; + supersedes?: string[]; order_index?: number; }, ) => apiPost(`/api/design-systems/${designSystemId}/tokens`, body); diff --git a/frontend/src/views/DesignSystemsView.vue b/frontend/src/views/DesignSystemsView.vue index 0d02177..5c1aeee 100644 --- a/frontend/src/views/DesignSystemsView.vue +++ b/frontend/src/views/DesignSystemsView.vue @@ -245,6 +245,10 @@ const tokenGroup = ref(""); const tokenPurpose = ref(""); /** Mode/value pairs, edited as a list so a token can carry any number of modes. */ const tokenModes = ref<{ mode: string; value: string }[]>([{ mode: "base", value: "" }]); +/** Literals this token should be written instead of, comma-separated in the + * form. Kept as one text field rather than a repeater: it is a short list of + * literals, and a row-per-value UI would cost more than it explains. */ +const tokenSupersedes = ref(""); const savingToken = ref(false); const editingTokenId = ref(null); @@ -253,6 +257,7 @@ function resetTokenForm() { tokenGroup.value = ""; tokenPurpose.value = ""; tokenModes.value = [{ mode: "base", value: "" }]; + tokenSupersedes.value = ""; editingTokenId.value = null; showAddToken.value = false; } @@ -262,6 +267,7 @@ function startEditToken(token: DesignToken) { tokenName.value = token.name; tokenGroup.value = token.group_name ?? ""; tokenPurpose.value = token.purpose ?? ""; + tokenSupersedes.value = (token.supersedes ?? []).join(", "); const entries = Object.entries(token.value_by_mode); tokenModes.value = entries.length ? entries.map(([mode, value]) => ({ mode, value })) @@ -288,6 +294,10 @@ async function submitToken() { value_by_mode: collectModes(), group_name: tokenGroup.value.trim() || null, purpose: tokenPurpose.value.trim() || null, + supersedes: tokenSupersedes.value + .split(",") + .map((v) => v.trim()) + .filter(Boolean), }; if (editingTokenId.value !== null) { await updateDesignToken(editingTokenId.value, body); @@ -555,6 +565,20 @@ function isColourish(value: string): boolean { +
+ + +

+ Literal values that should be written as this token instead — + comma separated. This is where a rule like "pure white is never + text" belongs: not as a prohibition, but as the thing to write + in its place. Checked by the source lint, not by this page. +

+
+
Values by mode

@@ -613,7 +637,12 @@ function isColourish(value: string): boolean { {{ value }} - {{ token.purpose || token.group_name || "" }} + + {{ token.purpose || token.group_name || "" }} + + instead of {{ token.supersedes.join(", ") }} + +

+ +
+
+

Import from a rulebook

+ +
+ + +
+
@@ -1066,6 +1219,10 @@ function isColourish(value: string): boolean { gap: 0.35rem; } +.import-summary { + margin-top: 0.75rem; +} + .supersedes { font-size: 0.75rem; color: var(--color-text-muted); diff --git a/src/scribe/mcp/tools/design_systems.py b/src/scribe/mcp/tools/design_systems.py index 805b118..91f014d 100644 --- a/src/scribe/mcp/tools/design_systems.py +++ b/src/scribe/mcp/tools/design_systems.py @@ -139,6 +139,42 @@ async def delete_design_system(design_system_id: int) -> dict: return {"message": f"Design system {design_system_id} deleted."} +async def import_design_system_from_rulebook( + design_system_id: int, + rulebook_id: int, + apply: bool = False, +) -> dict: + """Seed a design system from a rulebook that describes one in prose. + + Reads the rulebook's colour and token declarations and proposes the tokens + they add up to — joining "Obsidian #14171A" in one rule to `--fs-obsidian` + in another, since neither alone is a token. + + Defaults to a PREVIEW. An import is a proposal: rulebooks are written + aspirationally and some of what they describe was never built, so read + `proposed` before setting apply=True. Every entry carries the rule and + sentence it came from so the claim can be checked rather than trusted. + + Existing token names are never overwritten — a second run fills gaps and + lists the rest under `skipped`, so it is safe to repeat. + + Args: + design_system_id: The system to seed. + rulebook_id: The rulebook to read. + apply: False (default) previews; True writes the tokens. + """ + uid = current_user_id() + report = await ds_svc.import_from_rulebook( + uid, design_system_id, rulebook_id, apply=apply, + ) + if report is None: + raise ValueError( + f"design system {design_system_id} not writable, or rulebook " + f"{rulebook_id} not readable" + ) + return report + + # ── Tokens ────────────────────────────────────────────────────────────── async def create_design_token( @@ -271,6 +307,7 @@ def register(mcp) -> None: resolve_design_system, update_design_system, delete_design_system, + import_design_system_from_rulebook, create_design_token, list_design_tokens, update_design_token, diff --git a/src/scribe/routes/design_systems.py b/src/scribe/routes/design_systems.py index a6ffe6a..c4cd9ea 100644 --- a/src/scribe/routes/design_systems.py +++ b/src/scribe/routes/design_systems.py @@ -114,6 +114,30 @@ async def resolve_design_system(design_system_id: int): }) +@design_systems_bp.post("/design-systems//import") +@login_required +async def import_design_system(design_system_id: int): + """Seed a design system from a rulebook's prose. + + `{"rulebook_id": N}` previews; add `"apply": true` to write. Preview is the + default because an import is a PROPOSAL — rulebooks are written + aspirationally and some of what they describe was never built. + + Existing token names are never overwritten, so a second run fills gaps and + reports the rest rather than undoing corrections. + """ + data = await request.get_json() or {} + rulebook_id = data.get("rulebook_id") + if not isinstance(rulebook_id, int) or rulebook_id <= 0: + return jsonify({"error": "rulebook_id is required"}), 400 + report = await ds_svc.import_from_rulebook( + _uid(), design_system_id, rulebook_id, apply=bool(data.get("apply")), + ) + if report is None: + return _not_found("design system or rulebook") + return jsonify(report) + + # ── Tokens ────────────────────────────────────────────────────────────── @design_systems_bp.get("/design-systems//tokens") diff --git a/src/scribe/services/design_rulebook_import.py b/src/scribe/services/design_rulebook_import.py index 7f637fd..34573c5 100644 --- a/src/scribe/services/design_rulebook_import.py +++ b/src/scribe/services/design_rulebook_import.py @@ -230,3 +230,175 @@ async def design_expectations(user_id: int) -> ExpectationSet: return ExpectationSet(rulebook_id=rulebook_id) return ExpectationSet(rulebook_id=rulebook_id, expectations=extract_expectations(rules)) + + +# --------------------------------------------------------------------------- +# Import — turning a rulebook into a PROPOSED design system (milestone #254 step 3) +# --------------------------------------------------------------------------- +# +# The extraction above answers "what claims does this rulebook make?", which is +# what a drift panel needs. Seeding a design system needs a different shape: +# tokens with names AND values, which the rulebook states in two separate +# places. Rule 51 names the colours ("Obsidian #14171A (page bg, deepest +# surface)"); rule 72 names the custom properties (`--fs-obsidian/iron/...`). +# Neither alone is a token. +# +# So the import joins them on the WORD: `--fs-obsidian` ends with `obsidian`, +# and a colour called Obsidian was declared elsewhere. That join is mechanical +# and it is the only reason an import produces something usable rather than 70 +# empty names. +# +# AN IMPORT IS A PROPOSAL, NOT A TRUTH. Rulebooks are written aspirationally and +# some of what they describe was never built. Every proposed token therefore +# carries the rule and sentence it came from, so a reviewer can check the claim +# rather than trust it. + +# "Obsidian #14171A (page bg, deepest surface)" — a capitalised name, a hex, and +# an optional parenthetical saying what it is for. +_NAMED_COLOUR = re.compile( + r"\b([A-Z][A-Za-z]*(?:\s+[A-Z][A-Za-z]*)?)\s+(#[0-9a-fA-F]{3,8})\b" + r"(?:\s*\(([^)]{0,80})\))?" +) + + +@dataclass +class ProposedToken: + """One token an import suggests, with the evidence for it. + + `value_by_mode` is empty when the rulebook names the token but states no + value this can read — radius steps, type sizes and durations are prose + (`Small 4px`), not hex, and inventing a parse for each would be guessing. + An empty value is the honest output: the name is real, the value needs a + human. Reporting how many landed that way is part of the result. + """ + + name: str + value_by_mode: dict[str, str] = field(default_factory=dict) + group_name: str | None = None + purpose: str | None = None + supersedes: list[str] = field(default_factory=list) + source_rule_id: int | None = None + source_rule_title: str = "" + source_context: str = "" + + def as_dict(self) -> dict: + return { + "name": self.name, + "value_by_mode": self.value_by_mode, + "group_name": self.group_name, + "purpose": self.purpose, + "supersedes": self.supersedes, + "source_rule_id": self.source_rule_id, + "source_rule_title": self.source_rule_title, + "source_context": self.source_context, + } + + +@dataclass +class _NamedColour: + value: str + purpose: str | None + rule_id: int + rule_title: str + context: str + + +def _group_from_name(name: str) -> str | None: + """`--fs-radius-sm` -> "radius"; `--fs-obsidian` -> None. + + A family name has a middle segment; a flat one does not. Structural rather + than a lookup table, so it works on a naming scheme this code has never + seen — which rule #115 requires, since the prefix is each install's own. + """ + parts = [p for p in name.lstrip("-").split("-") if p] + return parts[1] if len(parts) >= 3 else None + + +def _named_colours(rules: list[Rule]) -> dict[str, _NamedColour]: + """Every `Name #hex (purpose)` a rulebook declares, keyed by lowercased name. + + First declaration wins, matching `extract_expectations` — the rule that + introduces a colour is the one worth citing. + """ + out: dict[str, _NamedColour] = {} + for rule in rules: + text = " ".join(filter(None, [rule.statement or "", rule.how_to_apply or ""])) + for sentence in _SENTENCE_SPLIT.split(text): + if not sentence.strip() or _is_negated(sentence): + continue + for match in _NAMED_COLOUR.finditer(sentence): + label, raw_hex, purpose = match.groups() + value = normalize_hex(raw_hex) + key = label.strip().lower() + if not value or key in out: + continue + out[key] = _NamedColour( + value=value, + purpose=(purpose or "").strip() or None, + rule_id=int(rule.id), + rule_title=rule.title, + context=sentence.strip(), + ) + return out + + +def _prohibitions_by_rule(rules: list[Rule]) -> dict[int, list[str]]: + """Forbidden colours, grouped by the rule that forbids them.""" + out: dict[int, list[str]] = {} + for expectation in extract_expectations(rules): + if expectation.kind == "prohibited_color": + out.setdefault(expectation.rule_id, []).append(expectation.value) + return out + + +def propose_tokens(rules: list[Rule]) -> list[ProposedToken]: + """Turn a rulebook into the design system it is describing. + + One proposal per custom-property NAME the rulebook declares, valued from the + named colour whose word matches the token's last segment. + + Prohibitions attach as `supersedes` on the first token drawn from the SAME + rule that forbids them. Rule 52 declares Parchment/Vellum/Ash and forbids + pure white in one breath, so pure white becomes "write --fs-parchment + instead" — the positive form of what the rule was saying. Guessing which + token inherits the prohibition is acceptable precisely because this is a + proposal a human reviews; guessing silently would not be, which is why every + entry carries its source sentence. + """ + colours = _named_colours(rules) + prohibited = _prohibitions_by_rule(rules) + claimed_prohibitions: set[int] = set() + + proposals: list[ProposedToken] = [] + seen: set[str] = set() + + for expectation in extract_expectations(rules): + if expectation.kind != "token" or expectation.value in seen: + continue + seen.add(expectation.value) + + suffix = expectation.value.rsplit("-", 1)[-1].lower() + colour = colours.get(suffix) + + proposal = ProposedToken( + name=expectation.value, + value_by_mode={"base": colour.value} if colour else {}, + group_name=_group_from_name(expectation.value), + purpose=colour.purpose if colour else None, + source_rule_id=colour.rule_id if colour else expectation.rule_id, + source_rule_title=colour.rule_title if colour else expectation.rule_title, + source_context=colour.context if colour else expectation.context, + ) + + # The prohibition rides on the first token that rule supplied a value + # for — its primary. Attaching it to every token of that rule would + # claim the rulebook said something it didn't. + if colour and colour.rule_id not in claimed_prohibitions: + forbidden = prohibited.get(colour.rule_id) + if forbidden: + proposal.supersedes = list(forbidden) + claimed_prohibitions.add(colour.rule_id) + + proposals.append(proposal) + + return proposals diff --git a/src/scribe/services/design_systems.py b/src/scribe/services/design_systems.py index ea8b478..be8ea78 100644 --- a/src/scribe/services/design_systems.py +++ b/src/scribe/services/design_systems.py @@ -331,3 +331,74 @@ async def set_project_design_system( project.updated_at = datetime.now(timezone.utc) await session.commit() return True + + +# --- import from a rulebook ------------------------------------------------- + +async def import_from_rulebook( + user_id: int, + design_system_id: int, + rulebook_id: int, + apply: bool = False, +) -> dict | None: + """Propose (and optionally create) tokens for a system from a rulebook. + + Returns None when the caller may not write the system or read the rulebook. + Otherwise a report with three lists, and the split between them is the whole + point of running it with `apply=False` first: + + proposed — everything the rulebook describes, each entry carrying the + rule and sentence it came from + created — what was actually written (empty unless `apply`) + skipped — proposals whose name the system already defines + + **Existing tokens are never overwritten.** An import is a proposal built by + reading prose; a value already in the record was put there deliberately, and + a re-run must not undo an operator's correction. That also makes the whole + operation safe to repeat — it fills gaps and reports the rest. + + Tokens with no value are still created when `apply` is set. The rulebook + names them, so their absence from the system is itself a finding, and a + named token with a blank value says "this exists and needs deciding" where + silence says nothing at all. + """ + if not await access.can_write_design_system(user_id, design_system_id): + return None + + from scribe.services import rulebooks as rulebooks_svc + from scribe.services.design_rulebook_import import propose_tokens + + rules = await rulebooks_svc.list_rules(user_id, rulebook_id=rulebook_id) + if not rules: + return {"rulebook_id": rulebook_id, "proposed": [], "created": [], "skipped": []} + + proposals = propose_tokens(rules) + existing = {t.name for t in await list_tokens(user_id, design_system_id)} + + created: list[dict] = [] + skipped: list[str] = [] + for index, proposal in enumerate(proposals): + if proposal.name in existing: + skipped.append(proposal.name) + continue + if not apply: + continue + token = await create_token( + user_id, + design_system_id=design_system_id, + name=proposal.name, + value_by_mode=proposal.value_by_mode, + group_name=proposal.group_name, + purpose=proposal.purpose, + supersedes=proposal.supersedes, + order_index=index, + ) + if token is not None: + created.append(token.to_dict()) + + return { + "rulebook_id": rulebook_id, + "proposed": [p.as_dict() for p in proposals], + "created": created, + "skipped": skipped, + } diff --git a/tests/test_design_rulebook_propose.py b/tests/test_design_rulebook_propose.py new file mode 100644 index 0000000..4197f33 --- /dev/null +++ b/tests/test_design_rulebook_propose.py @@ -0,0 +1,145 @@ +"""Rulebook prose -> a PROPOSED design system (milestone #254 step 3). + +The extraction tested in test_design_rulebook_import.py answers "what claims does +this rulebook make". This answers a harder question — "what design system is it +describing" — which needs the two halves joined: one rule names the colours, +another names the custom properties, and neither alone is a token. + +Rule text is representative rather than copied from this operator's rulebook +(rule #115): a test that only passes against their exact wording would be +testing the instance. +""" +from types import SimpleNamespace + +from scribe.services.design_rulebook_import import propose_tokens + + +def _rule(rule_id, title, statement, how_to_apply=None): + return SimpleNamespace( + id=rule_id, title=title, statement=statement, how_to_apply=how_to_apply + ) + + +SURFACES = _rule( + 51, "Universal surfaces", + "Obsidian #14171A (page bg, deepest surface), Iron #1E2228 (cards), " + "Slate #2C313A (hovered surfaces).", +) +TEXT = _rule( + 52, "Text palette", + "Text tokens: Parchment #E8E4D8 (primary text), Vellum #C2BFB4 (secondary). " + "Pure white #FFFFFF is NEVER used as text color.", +) +PROPERTIES = _rule( + 72, "CSS custom properties", + "Expose the system as custom properties: surfaces " + "(--fs-obsidian/iron/slate), text (--fs-parchment/vellum), and radius " + "(--fs-radius-sm/md/lg).", +) + + +def _by_name(proposals): + return {p.name: p for p in proposals} + + +# --- the join --------------------------------------------------------------- + +def test_a_token_takes_its_value_from_the_colour_of_the_same_name(): + """THE mechanism. `--fs-obsidian` and "Obsidian #14171A" are declared in + different rules and neither is a token on its own. Joining them on the word + is the only reason an import produces something usable instead of a list of + empty names.""" + proposals = _by_name(propose_tokens([SURFACES, PROPERTIES])) + assert proposals["--fs-obsidian"].value_by_mode == {"base": "#14171a"} + assert proposals["--fs-iron"].value_by_mode == {"base": "#1e2228"} + + +def test_the_parenthetical_becomes_the_tokens_purpose(): + """Rulebooks say what a colour is FOR right beside its value, and that is + the field a bare hex can never carry.""" + proposals = _by_name(propose_tokens([SURFACES, PROPERTIES])) + assert proposals["--fs-obsidian"].purpose == "page bg, deepest surface" + + +def test_a_token_with_no_matching_colour_is_proposed_with_no_value(): + """HONEST OUTPUT, not a failure. The rulebook states radius steps as prose + ("Small 4px"), which nothing here parses. The name is real and the value + needs a human — proposing the name with an empty value says exactly that, + where dropping it would hide a token the rulebook asked for.""" + proposals = _by_name(propose_tokens([SURFACES, PROPERTIES])) + assert proposals["--fs-radius-sm"].value_by_mode == {} + assert "--fs-radius-lg" in proposals + + +def test_every_proposal_carries_the_rule_and_sentence_it_came_from(): + """An import is a proposal a human reviews, and a claim you cannot trace is + a claim you have to take on faith.""" + obsidian = _by_name(propose_tokens([SURFACES, PROPERTIES]))["--fs-obsidian"] + assert obsidian.source_rule_id == 51 + assert obsidian.source_rule_title == "Universal surfaces" + assert "Obsidian #14171A" in obsidian.source_context + + +# --- prohibitions become replacements --------------------------------------- + +def test_a_prohibition_becomes_supersedes_on_that_rules_primary_token(): + """The reframe, end to end. Rule 52 declares Parchment and forbids pure + white in one breath; the import turns that into "write --fs-parchment + instead of #ffffff" — the same fact, stated forwards, and actionable.""" + proposals = _by_name(propose_tokens([TEXT, PROPERTIES])) + assert proposals["--fs-parchment"].supersedes == ["#ffffff"] + + +def test_a_prohibition_attaches_to_one_token_not_every_token_of_its_rule(): + """Rule 52 declares two colours. Attaching the prohibition to both would + claim the rulebook said something it didn't — that Vellum is also the + replacement for white.""" + proposals = _by_name(propose_tokens([TEXT, PROPERTIES])) + assert proposals["--fs-vellum"].supersedes == [] + + +def test_a_forbidden_colour_never_becomes_a_token_value(): + """Sentence-scoped negation carried through to the import: #FFFFFF appears + in rule 52 as a hex, and a naive read would make it Parchment's value.""" + proposals = propose_tokens([TEXT, PROPERTIES]) + for proposal in proposals: + assert proposal.value_by_mode.get("base") != "#ffffff" + + +# --- grouping --------------------------------------------------------------- + +def test_a_family_token_is_grouped_by_its_middle_segment(): + """`--fs-radius-sm` -> "radius". Structural, so it works on a naming scheme + this code has never seen — the prefix is each install's own (rule #115).""" + proposals = _by_name(propose_tokens([SURFACES, PROPERTIES])) + assert proposals["--fs-radius-sm"].group_name == "radius" + + +def test_a_flat_token_is_left_ungrouped_rather_than_guessed_at(): + proposals = _by_name(propose_tokens([SURFACES, PROPERTIES])) + assert proposals["--fs-obsidian"].group_name is None + + +# --- shape ------------------------------------------------------------------ + +def test_each_token_name_is_proposed_exactly_once(): + """The slash shorthand expands and rules repeat colours; neither may produce + a duplicate, since two live rows with one name is the duplicate-definition + bug the unique index exists to refuse.""" + names = [p.name for p in propose_tokens([SURFACES, TEXT, PROPERTIES])] + assert len(names) == len(set(names)) + + +def test_a_rulebook_that_names_no_tokens_proposes_nothing(): + """Most rulebooks are not design rulebooks. That has to be an empty result + rather than an error — an install can point this at anything.""" + unrelated = _rule(1, "Branching", "Work happens on the dev branch.") + assert propose_tokens([unrelated]) == [] + + +def test_colours_declared_without_a_token_name_are_not_invented_into_tokens(): + """A rulebook naming a colour it never exposes as a custom property has not + asked for a token, and inventing a name for it would put a token in the + record that no rule sanctions.""" + proposals = propose_tokens([SURFACES]) + assert proposals == [] diff --git a/tests/test_routes_design_systems.py b/tests/test_routes_design_systems.py index f52d4f3..723167f 100644 --- a/tests/test_routes_design_systems.py +++ b/tests/test_routes_design_systems.py @@ -24,8 +24,8 @@ def test_route_handlers_callable(): for name in ( "list_design_systems", "create_design_system", "get_design_system", "update_design_system", "delete_design_system", "resolve_design_system", - "list_design_tokens", "create_design_token", "update_design_token", - "delete_design_token", "set_project_design_system", + "import_design_system", "list_design_tokens", "create_design_token", + "update_design_token", "delete_design_token", "set_project_design_system", ): assert callable(getattr(routes, name)) @@ -44,6 +44,7 @@ def test_every_endpoint_is_reachable_on_the_app(): "/api/design-systems", "/api/design-systems/", "/api/design-systems//resolved", + "/api/design-systems//import", "/api/design-systems//tokens", "/api/design-tokens/", "/api/projects//design-system", @@ -57,7 +58,7 @@ def test_service_functions_take_user_id(): "create_design_system", "list_design_systems", "get_design_system", "update_design_system", "delete_design_system", "resolve_design_system", "create_token", "list_tokens", "update_token", "delete_token", - "set_project_design_system", + "set_project_design_system", "import_from_rulebook", ): fn = getattr(svc, fn_name) assert callable(fn) @@ -84,6 +85,12 @@ def test_agent_and_web_surfaces_stay_at_parity(): assert callable(getattr(tools, name)), f"MCP tool missing: {name}" assert callable(getattr(routes, name)), f"REST route missing: {name}" + # Import is the one verb whose handler names differ between the surfaces + # (the tool says what it reads FROM; the route is already under the system), + # so the loop above can't pair it. It still has to exist on both. + assert callable(tools.import_design_system_from_rulebook) + assert callable(routes.import_design_system) + def test_every_mcp_tool_in_the_module_is_registered(): """A tool written but never registered is invisible to an agent, and nothing diff --git a/tests/test_services_design_systems.py b/tests/test_services_design_systems.py index 6f8aa88..9619866 100644 --- a/tests/test_services_design_systems.py +++ b/tests/test_services_design_systems.py @@ -274,3 +274,102 @@ async def test_create_token_records_the_literals_it_replaces(): value_by_mode={"base": "#e8e4d8"}, supersedes=["#fff", "#ffffff"], ) assert captured["supersedes"] == ["#fff", "#ffffff"] + + +# --- import from a rulebook (step 3) ---------------------------------------- + +def _proposal(name, value=None): + from scribe.services.design_rulebook_import import ProposedToken + return ProposedToken(name=name, value_by_mode={"base": value} if value else {}) + + +@pytest.mark.asyncio +async def test_import_denied_without_write_on_the_system(): + with patch("scribe.services.design_systems.access") as acc: + acc.can_write_design_system = AsyncMock(return_value=False) + from scribe.services.design_systems import import_from_rulebook + assert await import_from_rulebook(1, 3, 9) is None + + +@pytest.mark.asyncio +async def test_preview_proposes_without_creating_anything(): + """apply=False is the default because an import is a PROPOSAL. A preview + that quietly wrote would make the review step decorative.""" + created = AsyncMock() + with patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.design_systems.create_token", created), \ + patch("scribe.services.design_systems.list_tokens", AsyncMock(return_value=[])), \ + patch("scribe.services.rulebooks.list_rules", AsyncMock(return_value=[object()])), \ + patch("scribe.services.design_rulebook_import.propose_tokens", + MagicMock(return_value=[_proposal("--fs-obsidian", "#14171a")])): + acc.can_write_design_system = AsyncMock(return_value=True) + from scribe.services.design_systems import import_from_rulebook + report = await import_from_rulebook(1, 3, 9, apply=False) + + assert [p["name"] for p in report["proposed"]] == ["--fs-obsidian"] + assert report["created"] == [] + created.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_import_never_overwrites_a_token_the_system_already_defines(): + """LOAD-BEARING for re-running it. A value already in the record was put + there deliberately — very likely correcting this importer — and a second run + must fill gaps rather than undo the correction.""" + existing = MagicMock() + existing.name = "--fs-obsidian" + created_token = MagicMock() + created_token.to_dict.return_value = {"name": "--fs-iron"} + + with patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.design_systems.create_token", + AsyncMock(return_value=created_token)) as create, \ + patch("scribe.services.design_systems.list_tokens", + AsyncMock(return_value=[existing])), \ + patch("scribe.services.rulebooks.list_rules", AsyncMock(return_value=[object()])), \ + patch("scribe.services.design_rulebook_import.propose_tokens", + MagicMock(return_value=[ + _proposal("--fs-obsidian", "#000000"), + _proposal("--fs-iron", "#1e2228"), + ])): + acc.can_write_design_system = AsyncMock(return_value=True) + from scribe.services.design_systems import import_from_rulebook + report = await import_from_rulebook(1, 3, 9, apply=True) + + assert report["skipped"] == ["--fs-obsidian"] + assert [c["name"] for c in report["created"]] == ["--fs-iron"] + assert create.await_count == 1 + + +@pytest.mark.asyncio +async def test_a_valueless_proposal_is_still_created(): + """The rulebook names it, so its absence from the system is itself a + finding. A named token with a blank value says "this exists and needs + deciding"; silence says nothing at all.""" + token = MagicMock() + token.to_dict.return_value = {"name": "--fs-radius-sm"} + with patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.design_systems.create_token", + AsyncMock(return_value=token)) as create, \ + patch("scribe.services.design_systems.list_tokens", AsyncMock(return_value=[])), \ + patch("scribe.services.rulebooks.list_rules", AsyncMock(return_value=[object()])), \ + patch("scribe.services.design_rulebook_import.propose_tokens", + MagicMock(return_value=[_proposal("--fs-radius-sm")])): + acc.can_write_design_system = AsyncMock(return_value=True) + from scribe.services.design_systems import import_from_rulebook + report = await import_from_rulebook(1, 3, 9, apply=True) + + assert len(report["created"]) == 1 + assert create.await_args.kwargs["value_by_mode"] == {} + + +@pytest.mark.asyncio +async def test_an_unreadable_or_empty_rulebook_reports_nothing_rather_than_failing(): + """An install can point this at any rulebook, and most rulebooks are not + design rulebooks (rule #115).""" + with patch("scribe.services.design_systems.access") as acc, \ + patch("scribe.services.rulebooks.list_rules", AsyncMock(return_value=[])): + acc.can_write_design_system = AsyncMock(return_value=True) + from scribe.services.design_systems import import_from_rulebook + report = await import_from_rulebook(1, 3, 9, apply=True) + assert report == {"rulebook_id": 9, "proposed": [], "created": [], "skipped": []} -- 2.54.0 From b0a7d9e89b61c0670068ab4533f4b6358c02cb2d Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Thu, 30 Jul 2026 21:42:08 -0400 Subject: [PATCH 13/16] =?UTF-8?q?feat(design-systems):=20the=20master=20sh?= =?UTF-8?q?eet=20=E2=80=94=20purpose=20tokens,=20not=20per-element=20value?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator's new requirement (#2299, architecture in #2296): a design system does not just hold tokens, it generates and manages a master CSS sheet. That settles the milestone's open "authority mechanism" question — the record is authoritative because the stylesheet comes out of it. **The sheet is shaped by purpose and styles no elements.** It declares custom properties, grouped by what they mean, and contains no `.btn-primary`, no `table`, no `input`. That is the design, not a shortcut: a sheet that styled elements would restate the same handful of values once per element and grow with the UI, where purpose-named values are stated once and reused. Components live as SNIPPETS that reference these names — a surface that already exists and already carries prose, locations, drift checks, merge and write-path recall. A token named after an element (`--fs-button-bg`) is the smell that the two have been mixed; a purpose name (`--fs-action-primary`) is reused across all of them. Alongside the CSS the endpoint returns what the text cannot say for itself: which tokens are still valueless, and which VALUES are declared under more than one name. The second is the operator's "reuse consistent values" constraint made checkable — and it reports rather than refuses, because a design system legitimately aligns colours on purpose ("Success = Moss, by design") and only a human knows which case it is. Mode maps to selector the way the codebase already does it: base on the root selector, every other mode layered on `[data-theme="…"]`. The root selector is a PARAMETER — #251 recorded that a container-scoped preview cannot use `:root`, so hardcoding it would have made the generator useless to the preview surface. A token the rulebook names but states no value for is emitted as a commented-out declaration IN ITS GROUP rather than dropped. Its absence is the finding, and a comment puts that finding where the reader already is. Values are validated, not escaped, and this is a real boundary rather than tidiness: design systems are shareable records (rule #47), so `red; } body { display: none` in a system shared with you would otherwise inject CSS into your page. A value containing `{ } ; @ < >`, a comment delimiter or a newline is REFUSED and rendered as a comment saying so — rejecting beats stripping, since a partially-sanitised value is one the operator never wrote and the sheet's whole claim is that it is the record. Not in scope, and deliberately: serving this as the app's actual stylesheet. Generating and exposing a sheet is reversible; swapping theme.css for a generated one is not, and it should be an explicit call rather than a side effect. --- frontend/src/api/designSystems.ts | 19 ++ frontend/src/views/DesignSystemsView.vue | 139 ++++++++++++++ src/scribe/mcp/tools/design_systems.py | 29 +++ src/scribe/routes/design_systems.py | 21 +++ src/scribe/services/design_stylesheet.py | 213 +++++++++++++++++++++ src/scribe/services/design_systems.py | 35 ++++ tests/test_design_stylesheet.py | 230 +++++++++++++++++++++++ tests/test_routes_design_systems.py | 6 +- tests/test_services_design_systems.py | 46 +++++ 9 files changed, 737 insertions(+), 1 deletion(-) create mode 100644 src/scribe/services/design_stylesheet.py create mode 100644 tests/test_design_stylesheet.py diff --git a/frontend/src/api/designSystems.ts b/frontend/src/api/designSystems.ts index ffae9f2..0576ce1 100644 --- a/frontend/src/api/designSystems.ts +++ b/frontend/src/api/designSystems.ts @@ -159,3 +159,22 @@ export const importFromRulebook = ( rulebook_id: rulebookId, apply, }); + +export interface StylesheetResult { + design_system_id: number; + /** The master sheet: purpose tokens only, no element or class rules. */ + css: string; + token_count: number; + /** Tokens the system names but has no value for yet. */ + valueless: string[]; + /** Values declared under more than one name — alias, or one idea twice. */ + duplicates: Record; +} + +/** The master CSS sheet a design system generates. + * + * Purpose tokens only. Components (buttons, tables, input schemes) are + * snippets that reference these names, so a value is stated once and reused + * rather than restated per element. */ +export const fetchStylesheet = (id: number) => + apiGet(`/api/design-systems/${id}/stylesheet`); diff --git a/frontend/src/views/DesignSystemsView.vue b/frontend/src/views/DesignSystemsView.vue index 2688b13..85c491f 100644 --- a/frontend/src/views/DesignSystemsView.vue +++ b/frontend/src/views/DesignSystemsView.vue @@ -29,6 +29,7 @@ import { fetchDesignSystems, fetchDesignTokens, fetchResolvedTokens, + fetchStylesheet, importFromRulebook, updateDesignSystem, updateDesignToken, @@ -36,6 +37,7 @@ import { type DesignToken, type ImportReport, type ResolvedToken, + type StylesheetResult, } from "@/api/designSystems"; import { listRulebooks, type Rulebook } from "@/api/rulebooks"; import { ApiError } from "@/api/client"; @@ -385,6 +387,52 @@ const overrideCount = computed( () => resolved.value.filter((t) => Object.values(t.origin_by_mode).some((id) => id === selectedId.value)).length, ); +// --- the master sheet ------------------------------------------------------- + +const sheet = ref(null); +const sheetLoading = ref(false); +const showSheet = ref(false); + +async function loadSheet() { + if (selectedId.value === null) return; + sheetLoading.value = true; + try { + sheet.value = await fetchStylesheet(selectedId.value); + } catch { + sheet.value = null; + toast.show("Failed to render the stylesheet", "error"); + } finally { + sheetLoading.value = false; + } +} + +/** Toggle, and render on first open rather than on mount — the sheet is a + * derived view most visits don't need, and rendering it unasked would cost a + * round trip per system selection. */ +function toggleSheet() { + showSheet.value = !showSheet.value; + if (showSheet.value && !sheet.value) loadSheet(); +} + +async function copySheet() { + if (!sheet.value) return; + try { + await navigator.clipboard.writeText(sheet.value.css); + toast.show("Stylesheet copied"); + } catch { + toast.show("Couldn't copy — select the text instead", "error"); + } +} + +/** Re-render whenever the tokens behind it change, so the sheet on screen is + * never a stale answer to a question the operator has already moved past. */ +watch([selectedId, ownTokens], () => { + sheet.value = null; + if (showSheet.value) loadSheet(); +}); + +const duplicateEntries = computed(() => Object.entries(sheet.value?.duplicates ?? {})); + // --- import from a rulebook ------------------------------------------------- const rulebooks = ref([]); @@ -582,6 +630,69 @@ function isColourish(value: string): boolean {
+ +
+
+

Master stylesheet

+ +
+ + +
+
@@ -1219,6 +1330,34 @@ function isColourish(value: string): boolean { gap: 0.35rem; } +.sheet { + background: var(--color-bg); + border: 1px solid var(--color-border); + border-radius: var(--radius-sm); + padding: 0.75rem 1rem; + margin-top: 0.75rem; + overflow-x: auto; + font-family: 'JetBrains Mono', ui-monospace, Menlo, Consolas, monospace; + font-size: 0.8rem; + line-height: 1.6; + max-height: 30rem; + overflow-y: auto; +} + +.dupe-list { + list-style: none; + margin: 0.5rem 0 0; + padding: 0; + font-size: 0.85rem; +} + +.dupe-list li { + display: flex; + align-items: center; + gap: 0.4rem; + padding: 0.15rem 0; +} + .import-summary { margin-top: 0.75rem; } diff --git a/src/scribe/mcp/tools/design_systems.py b/src/scribe/mcp/tools/design_systems.py index 91f014d..b6a1122 100644 --- a/src/scribe/mcp/tools/design_systems.py +++ b/src/scribe/mcp/tools/design_systems.py @@ -139,6 +139,34 @@ async def delete_design_system(design_system_id: int) -> dict: return {"message": f"Design system {design_system_id} deleted."} +async def get_design_system_stylesheet( + design_system_id: int, + root_selector: str = ":root", +) -> dict: + """The master CSS sheet a design system generates. + + Purpose tokens only — this sheet declares what values MEAN and styles no + elements. Components (buttons, tables, input schemes) are SNIPPETS that + reference these names, so a value is stated once and reused rather than + restated per element. Reach for this when you need the tokens a snippet is + allowed to use. + + Also returns `valueless` (tokens the system names but has no value for) and + `duplicates` (values declared under more than one name — a deliberate alias, + or one idea recorded twice). + + Args: + design_system_id: The system to render. + root_selector: Selector for the base layer. Defaults to `:root`; pass a + container selector to scope the sheet to a preview region. + """ + uid = current_user_id() + result = await ds_svc.stylesheet_for_system(uid, design_system_id, root_selector) + if result is None: + raise ValueError(f"design system {design_system_id} not found") + return result + + async def import_design_system_from_rulebook( design_system_id: int, rulebook_id: int, @@ -307,6 +335,7 @@ def register(mcp) -> None: resolve_design_system, update_design_system, delete_design_system, + get_design_system_stylesheet, import_design_system_from_rulebook, create_design_token, list_design_tokens, diff --git a/src/scribe/routes/design_systems.py b/src/scribe/routes/design_systems.py index c4cd9ea..48706ea 100644 --- a/src/scribe/routes/design_systems.py +++ b/src/scribe/routes/design_systems.py @@ -114,6 +114,27 @@ async def resolve_design_system(design_system_id: int): }) +@design_systems_bp.get("/design-systems//stylesheet") +@login_required +async def get_design_system_stylesheet(design_system_id: int): + """The master CSS sheet this design system generates. + + JSON by default (the UI wants the reuse report alongside the CSS); add + `?format=css` for the raw stylesheet as `text/css`, which is what a build + step or a `curl` wants. + + `?root=` overrides the base selector — a container-scoped preview cannot use + `:root`, so the generator takes it as a parameter. + """ + root = (request.args.get("root") or ":root").strip() or ":root" + result = await ds_svc.stylesheet_for_system(_uid(), design_system_id, root) + if result is None: + return _not_found() + if request.args.get("format") == "css": + return result["css"], 200, {"Content-Type": "text/css; charset=utf-8"} + return jsonify(result) + + @design_systems_bp.post("/design-systems//import") @login_required async def import_design_system(design_system_id: int): diff --git a/src/scribe/services/design_stylesheet.py b/src/scribe/services/design_stylesheet.py new file mode 100644 index 0000000..b6f4e5a --- /dev/null +++ b/src/scribe/services/design_stylesheet.py @@ -0,0 +1,213 @@ +"""Render a design system's resolved tokens as its master CSS sheet. + +Pure, like `design_cascade` and for the same reasons: no database import, so the +rendering rule is testable without one and the preview surface can reuse it. + +WHAT THIS SHEET IS, AND DELIBERATELY IS NOT +------------------------------------------- +It declares **purpose tokens only** — custom properties, grouped by what they +mean. It contains no rules for elements or classes: no `.btn-primary`, no +`table`, no `input`. + +That is the point rather than a limitation. A sheet that styled every element +would restate the same handful of values once per element and grow with the UI; +a sheet of purpose-named values states each once and is reused. Components — +buttons, tables, input schemes — live as SNIPPETS that reference these names and +carry prose about the idea, which is a surface that already exists and already +has recall, locations, drift checks and merge. + +So the division is: this sheet says what the values MEAN; snippets say what +things LOOK LIKE, in terms of those values. A token named after an element +(`--fs-button-bg`) is the smell that the two have been mixed — it multiplies +with every new element, where a purpose name (`--fs-action-primary`) is reused. + +SAFETY +------ +Values are interpolated into a stylesheet, and design systems are shareable +records (rule #47). A value of `red; } body { display: none` in a system someone +shared with you would otherwise inject CSS into your page. Everything rendered +here is validated or dropped — never escaped-and-hoped. +""" +from __future__ import annotations + +import re +from collections.abc import Sequence + +BASE_MODE = "base" + +# A custom property name, strictly. Anything else is dropped rather than +# sanitised: a name is an identifier, and a "cleaned up" identifier is a +# different token than the one the operator recorded. +_VALID_NAME = re.compile(r"^--[A-Za-z0-9_-]+$") + +# Characters that would end the declaration, open a block, start an at-rule, or +# begin a tag. A value containing any of them is not a value. +_UNSAFE_VALUE = re.compile(r"[{};@<>]|\*/|/\*|\n|\r") + + +def is_valid_token_name(name: str) -> bool: + return bool(_VALID_NAME.match((name or "").strip())) + + +def safe_value(value: str) -> str | None: + """A CSS value that cannot escape its declaration, or None. + + Rejects rather than strips. A partially-sanitised value is a value the + operator did not write, and silently rendering a different colour than the + record holds is worse than rendering none — the sheet's whole claim is that + it IS the record. + """ + candidate = (value or "").strip() + if not candidate or _UNSAFE_VALUE.search(candidate): + return None + return candidate + + +def safe_comment(text: str) -> str: + """Comment text that cannot close its comment or break the line.""" + return re.sub(r"\*/|/\*|[\n\r]", " ", (text or "")).strip() + + +def selector_for_mode(mode: str, root_selector: str = ":root") -> str: + """Which selector a mode's declarations belong under. + + `base` gets the caller's root selector; every other mode gets a bare + attribute selector, matching the convention already in the codebase. + + The root selector is a PARAMETER because a container-scoped preview cannot + use `:root` — #251 recorded that mode scoping is one-way (light lives on + `:root`, dark layers over it), so a generator that hardcoded `:root` could + not serve a preview at all. + """ + if mode == BASE_MODE: + return root_selector + safe_mode = re.sub(r"[^A-Za-z0-9_-]", "", mode) + return f'[data-theme="{safe_mode}"]' if safe_mode else root_selector + + +def _grouped(tokens: Sequence) -> list[tuple[str | None, list]]: + """Tokens by group, preserving the order they arrive in (already sorted).""" + groups: dict[str | None, list] = {} + for token in tokens: + groups.setdefault(getattr(token, "group_name", None), []).append(token) + return list(groups.items()) + + +def _modes_present(tokens: Sequence) -> list[str]: + """Every mode any token declares, base first then the rest alphabetically.""" + modes = { + mode + for token in tokens + for mode in (getattr(token, "value_by_mode", None) or {}) + } + rest = sorted(modes - {BASE_MODE}) + return ([BASE_MODE] if BASE_MODE in modes else []) + rest + + +def render_stylesheet( + tokens: Sequence, + *, + root_selector: str = ":root", + title: str = "", + design_system_id: int | None = None, +) -> str: + """The master sheet for a resolved token set. + + One block per mode. Within a block, only the tokens that declare a value for + that mode — so a mode block is an override layer, exactly as the storage + model has it. + + Tokens with no value at all are emitted as COMMENTED-OUT declarations in + their group, not dropped. The rulebook named them, so their absence is a + finding, and a commented line puts that finding where the reader is already + looking. + """ + header = [ + "/*", + f" * {safe_comment(title) or 'Design system'} — generated stylesheet", + ] + if design_system_id is not None: + header.append(f" * Source: design system {int(design_system_id)}.") + header += [ + " *", + " * Purpose tokens only. This sheet declares what values MEAN; it styles", + " * no elements. Buttons, tables and input schemes live as snippets that", + " * reference these names, so each value is stated once and reused rather", + " * than restated per element.", + " *", + " * Generated — edit the design system, not this file.", + " */", + "", + ] + + lines = list(header) + valueless = [ + t for t in tokens + if is_valid_token_name(getattr(t, "name", "")) + and not (getattr(t, "value_by_mode", None) or {}) + ] + + for mode in _modes_present(tokens): + block: list[str] = [] + for group, group_tokens in _grouped(tokens): + entries: list[str] = [] + for token in group_tokens: + name = (getattr(token, "name", "") or "").strip() + if not is_valid_token_name(name): + continue + raw = (getattr(token, "value_by_mode", None) or {}).get(mode) + if raw is None: + continue + value = safe_value(str(raw)) + if value is None: + entries.append( + f" /* {name}: value rejected — not a safe CSS value */" + ) + continue + purpose = safe_comment(getattr(token, "purpose", "") or "") + comment = f" /* {purpose} */" if purpose and mode == BASE_MODE else "" + entries.append(f" {name}: {value};{comment}") + if entries: + if group: + block.append(f" /* {safe_comment(group)} */") + block.extend(entries) + block.append("") + + # Declared-but-valueless tokens belong to the base layer: they have no + # mode to sit under, and repeating them per mode would triple the noise. + if mode == BASE_MODE and valueless: + block.append(" /* Declared by the design system, no value set yet: */") + block.extend(f" /* {t.name}: ; */" for t in valueless) + block.append("") + + if not block: + continue + lines.append(f"{selector_for_mode(mode, root_selector)} {{") + lines.extend(block[:-1] if block[-1] == "" else block) + lines.append("}") + lines.append("") + + return "\n".join(lines).rstrip() + "\n" + + +def duplicate_values(tokens: Sequence) -> dict[str, list[str]]: + """Values declared by more than one token, mapped to the names declaring them. + + Two names for one value are either a deliberate alias or the same idea + recorded twice — the token-level form of the duplicated-definition shape. + Reported rather than refused: a design system legitimately aligns colours on + purpose ("Success = Moss, by design"), and a generator that rejected that + would be wrong about the operator's intent. + + Compares the BASE value only. Two tokens agreeing in one mode and diverging + in another are not duplicates of each other — they are a near-miss, which is + a different and less interesting finding. + """ + by_value: dict[str, list[str]] = {} + for token in tokens: + name = getattr(token, "name", "") + base = (getattr(token, "value_by_mode", None) or {}).get(BASE_MODE) + if not name or not base: + continue + by_value.setdefault(str(base).strip().lower(), []).append(name) + return {value: names for value, names in by_value.items() if len(names) > 1} diff --git a/src/scribe/services/design_systems.py b/src/scribe/services/design_systems.py index be8ea78..bcb2757 100644 --- a/src/scribe/services/design_systems.py +++ b/src/scribe/services/design_systems.py @@ -21,6 +21,7 @@ from scribe.models import async_session from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.project import Project from scribe.services import access +from scribe.services.design_stylesheet import duplicate_values, render_stylesheet from scribe.services.design_cascade import ( ResolvedToken, ancestry, @@ -402,3 +403,37 @@ async def import_from_rulebook( "created": created, "skipped": skipped, } + + +# --- the master sheet ------------------------------------------------------- + +async def stylesheet_for_system( + user_id: int, design_system_id: int, root_selector: str = ":root" +) -> dict | None: + """The master CSS sheet a design system generates, plus its reuse report. + + Purpose tokens only — the sheet styles no elements. See + `services/design_stylesheet.py` for why that split is the design rather than + a shortcut. + + Returns None if the caller may not read the system. The `duplicates` half is + advisory: two tokens sharing a value are either a deliberate alias or one + idea recorded twice, and only the operator knows which. + """ + resolved = await resolve_design_system(user_id, design_system_id) + if resolved is None: + return None + system = await get_design_system(user_id, design_system_id) + + return { + "design_system_id": design_system_id, + "css": render_stylesheet( + resolved, + root_selector=root_selector, + title=system.title if system else "", + design_system_id=design_system_id, + ), + "token_count": len(resolved), + "valueless": [t.name for t in resolved if not t.value_by_mode], + "duplicates": duplicate_values(resolved), + } diff --git a/tests/test_design_stylesheet.py b/tests/test_design_stylesheet.py new file mode 100644 index 0000000..df18171 --- /dev/null +++ b/tests/test_design_stylesheet.py @@ -0,0 +1,230 @@ +"""Rendering a design system as its master CSS sheet. + +The generator is pure, so a whole sheet is one literal of tokens in and a string +out. Two things here are load-bearing rather than cosmetic: what the sheet +deliberately does NOT contain, and the fact that a value cannot escape its +declaration. +""" +from types import SimpleNamespace + +from scribe.services.design_stylesheet import ( + duplicate_values, + is_valid_token_name, + render_stylesheet, + safe_comment, + safe_value, + selector_for_mode, +) + + +def _token(name, value_by_mode, group_name=None, purpose=None): + return SimpleNamespace( + name=name, value_by_mode=value_by_mode, + group_name=group_name, purpose=purpose, + ) + + +# --- safety ----------------------------------------------------------------- +# +# Design systems are shareable records. A value that can close its declaration +# can inject arbitrary CSS into the page of anyone the system was shared with, +# which makes this a real boundary rather than tidiness. + +def test_a_value_that_would_escape_its_declaration_is_refused(): + """`red; } body { display: none` is the whole attack: end the declaration, + close the block, open your own.""" + assert safe_value("red; } body { display: none") is None + assert safe_value("#fff}") is None + assert safe_value("#fff;") is None + + +def test_at_rules_and_tags_are_refused(): + assert safe_value("@import url(evil.css)") is None + assert safe_value("