revert(design-systems): drop the rulebook import — a migration, not a feature
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 19s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 43s
CI & Build / Build & push image (push) Successful in 44s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / integration (push) Successful in 19s
CI & Build / TypeScript typecheck (push) Successful in 32s
CI & Build / Python tests (push) Successful in 43s
CI & Build / Build & push image (push) Successful in 44s
Operator's call, and it corrects a scope error rather than a bug: "this is a path for a user to go from a rulebook to a design system. we don't need to build this path in the app itself ... you should be the one that does the import ... going forward no one else should have to do such a migration." Right. Nobody starting from a design system will ever go rulebook -> system, so the whole path was permanent product code serving a single act on one install. Rule #22: remove it, don't flag it off. Gone from the service, the REST route, the MCP tool, the UI panel, the API client and its tests. There is a second consequence I had missed, and it is the better argument. The parser was WORSE at this than doing it by hand. `propose_tokens` leaves radius steps and type sizes valueless because "Small 4px" is not a hex and nothing here parses it — a limitation I documented carefully and shipped anyway. But that limitation only exists because the importer had to run unattended. Done as work rather than as a feature, those values are just read and written, and the result is a complete design system instead of one with a dozen blanks and a count explaining them. Scaffolding built around my own absence from the loop, when I am the loop. KEPT: `extract_expectations` and `design_expectations` in services/design_rulebook_import.py. The live drift panel still reads them until it is repointed at a resolved design system (#2295), and removing them now would take the /design page's only content with it. They go with that change, not this one.
This commit is contained in:
@@ -1,145 +0,0 @@
|
||||
"""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 == []
|
||||
@@ -24,7 +24,7 @@ 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",
|
||||
"import_design_system", "get_design_system_stylesheet",
|
||||
"get_design_system_stylesheet",
|
||||
"check_snippets_against_system",
|
||||
"list_design_tokens", "create_design_token",
|
||||
"update_design_token", "delete_design_token", "set_project_design_system",
|
||||
@@ -46,7 +46,6 @@ def test_every_endpoint_is_reachable_on_the_app():
|
||||
"/api/design-systems",
|
||||
"/api/design-systems/<int:design_system_id>",
|
||||
"/api/design-systems/<int:design_system_id>/resolved",
|
||||
"/api/design-systems/<int:design_system_id>/import",
|
||||
"/api/design-systems/<int:design_system_id>/stylesheet",
|
||||
"/api/design-systems/<int:design_system_id>/snippet-check",
|
||||
"/api/design-systems/<int:design_system_id>/tokens",
|
||||
@@ -62,7 +61,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", "import_from_rulebook",
|
||||
"set_project_design_system",
|
||||
"stylesheet_for_system", "check_snippets_against_system",
|
||||
):
|
||||
fn = getattr(svc, fn_name)
|
||||
@@ -91,11 +90,9 @@ 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)
|
||||
# The snippet check is the one verb whose handler names differ between the
|
||||
# surfaces (the tool says what it checks AGAINST; 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.check_snippets_against_design_system)
|
||||
assert callable(routes.check_snippets_against_system)
|
||||
|
||||
|
||||
@@ -276,105 +276,6 @@ async def test_create_token_records_the_literals_it_replaces():
|
||||
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": []}
|
||||
|
||||
|
||||
# --- the master sheet -------------------------------------------------------
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
||||
Reference in New Issue
Block a user