diff --git a/CHANGELOG.md b/CHANGELOG.md index e18c51d..0fb5438 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,32 @@ follow [SemVer](https://semver.org/). ## [Unreleased] +### Added +- Drift guards batch (architecture review findings F1/F6/F9, + 2026-08-20): + - `tests/test_grammar_completeness.py` — the F1 pin: every + `QuestionType` member must carry a row in the completeness tables + (widget collect mode + a wrong-shaped answer), and each of the + four surfaces must emit construct-specific output for it — a + construct wired into only three surfaces, or a new type added + without updating the tables, fails red instead of silently falling + through a default branch + - `tests/test_docs_drift.py` — the grammar's hand-maintained docs + tracked mechanically: README's spelled-out construct count and + per-construct coverage, SKILL.md's coverage of every question + type, and every MCP tool / `x_to_y` library function the skill + names must actually exist (the count had already rotted by hand + once, commit 543a7a0) + - `docs/adding-a-construct.md` — the ~19-touchpoint checklist for a + new construct, with the review's accept-and-pin ruling and the + rejected registry/base-class alternatives recorded + - Surface-decision authority stated where it was only implicit + (F9): `select_form_surface` docstring and README now say the + router is advisory in the shipped plugin — the agent's MCP tool + choice is the effective decision and the router runs after the + fact for telemetry agreement; binding only for library consumers + routing their own calls + ### Changed - 0.6.x cleanup batch (architecture review findings F4/F5/F7, 2026-08-20) — single-sourcing and schema hygiene, output diff --git a/README.md b/README.md index 7ddd9a6..57c4c0f 100644 --- a/README.md +++ b/README.md @@ -112,7 +112,11 @@ if select_form_surface(form) == "widget": `problems_to_markdown` re-asks exactly the fields that failed. - **Surface routing** — `select_form_surface` picks widget vs fallback; a keyboard-mode opt-out is persisted per project. The form degrades — - it never breaks. + it never breaks. Authority note: in the shipped plugin the router is + *advisory* — the agent's choice of MCP tool IS the surface decision, + guided by the skill's prose ladder, and the router runs after the + fact so telemetry can record agreement. Library consumers routing + their own calls (as above) are the path where its answer is binding. - **Validation** — `form_from_dict` refuses malformed definitions; `collect_form_response` refuses malformed answers (required fields, option membership) with field-level problems. diff --git a/docs/adding-a-construct.md b/docs/adding-a-construct.md new file mode 100644 index 0000000..d17517d --- /dev/null +++ b/docs/adding-a-construct.md @@ -0,0 +1,99 @@ +# Adding a construct: the touchpoint checklist + +A new construct type costs roughly 19 files and ~1,000 lines, about +half of it tests. The 2026-08-20 architecture review ruled that cost +the honest price of four surfaces — a construct *means something +different* on each one, so the per-surface branches are four genuine +translations, not duplication a registry could collapse (rejected +alternatives, recorded so they stay rejected: a `Construct` base class +with per-surface render methods; entry-point plugin discovery; codegen +from a spec table). What the review added instead is this explicit +checklist and the drift catchers that turn a forgotten touchpoint into +a red test (`tests/test_grammar_completeness.py`). + +Worked examples: `ranking` (PR #24, 20 files, +865) and +`assumption_review` (PR #25, 19 files, +1219). + +## The model (1–2) + +1. **`models.py` — `QuestionType`**: add the member, with the spec + prose as its comment block (the enum body is the grammar's + normative text). If the construct carries new extras, add them as + fields on the `FormQuestion` dataclass — additive columns, no + subclassing. +2. **`models.py` — `to_ask_user_format` / `to_ask_user_formats`**: how + the construct degrades on a plain question tool. An expanding + construct (one payload per item/slot) raises in the singular method + and expands in the plural one; iterate rows through + `expansion_items` / `suggested_pick` / `item_context` so the + surfaces cannot disagree. + +## The parser and validator (3–7) + +3. **`bridge.py` — `_parse__extras`**: a new parser returning + `(value(s), problems)`, called from `form_from_dict` and guarded + internally by `qtype is not QuestionType.X`. Reject what the + construct's rules forbid (e.g. `default` on a ranking, D2). +4. **`bridge.py` — `form_from_dict`**: call the parser, extend + `problems`, pass the new kwarg to the `FormQuestion(...)` + construction. +5. **`bridge.py` — the frozensets**: `_OPTIONS_REQUIRED_TYPES`, + `_WIDGET_ONLY_TYPES`, `_EXPANDING_TYPES`, `_NO_PORTABLE_CONTROL` — + add the type wherever its behavior matches. +6. **`bridge.py` — `_validate_` + `_ANSWER_VALIDATORS`**: the + answer-shape validator, registered in the dict. Six constructs + whose answer is one selected option just register + `_validate_membership`. +7. **`bridge.py` — `_fold_expanded_answers`**: only if the construct + expands to dotted keys on flat surfaces — how they fold back. + +## The four surfaces (8–14) + +8. **`elicitation_schema.py` — `_property_for` / + `form_to_elicitation_schema`**: the native-elicitation projection + (flat primitives; expanding constructs become dotted properties). +9. **`widget.py` — `_control__html` + `_CONTROL_RENDERERS`**: the + rich HTML control, registered in the dict. +10. **`widget.py` — `_COLLECT_MODES`**: how the submit script reads + the answer out of the DOM. A construct that answers like an + existing one reuses its mode and needs NO script edit; a genuinely + new answer shape needs a new script case AND a new case in the + gate-parity port (`tests/test_widget_roundtrip.py`). +11. **`widget.py` — `_families_for`** and **`theme.py` — `CSS_` + + `CSS_FAMILIES`**: the control's CSS family, so forms never ship + styles they don't use. +12. **`markdown_surface.py` — `_control_lines` + `_skeleton_value`**: + the portable-markdown rendering and the reply skeleton's + placeholder shape. +13. **`markdown_ingestion.py`**: how a typed shorthand line for the + construct parses back (`_known_keys`, `_coerce`, + `_resolve_line_key`, `markdown_to_answers`). +14. **`mcp_server.py` — `_field_schema`**: the type enum entry, a line + in the prose description, and any new extra-key property. The + schema drift test names what you forget. + +## The exemplar and exports (15–16) + +15. **`reference_form.py`**: one field for the new type in + `REFERENCE_FORM` plus a valid answer in `EXAMPLE_ANSWERS` — the + round-trip, CSS, and grammar-completeness suites all span it, and + `test_reference_form` fails until the field exists. +16. **`__init__.py`**: export any new public helper in `__all__`. + +## Tests and docs (17–19) + +17. **Tests**: a `tests/test__construct.py` file (definition rules, + answer validation, each surface's rendering), plus rows in the + completeness tables of `tests/test_grammar_completeness.py` + (collect mode + wrong-shaped answer) and gate-parity fixtures in + `tests/test_widget_roundtrip.py`. +18. **Docs**: README "The grammar" (bullet AND the spelled-out + construct count), `plugin/skills/forms/SKILL.md` (a `##` section: + extra keys, answer shape, flat-surface expansion), CHANGELOG. + `tests/test_docs_drift.py` enforces the count and the name + coverage. +19. **Sanity**: `python -m pytest` — the drift catchers + (`test_grammar_completeness`, `test_widget_roundtrip`, + `test_widget_css_families`, `test_docs_drift`, + `test_reference_form`, the `_field_schema` coverage test) are + designed to fail red on any touchpoint you missed above. diff --git a/src/attune_forms/bridge.py b/src/attune_forms/bridge.py index 9a80dcf..1d87355 100644 --- a/src/attune_forms/bridge.py +++ b/src/attune_forms/bridge.py @@ -1136,6 +1136,18 @@ def select_form_surface( input — the axis is how much of the option space the user can see at once, not how many tool calls it costs. + .. note:: + Authority (architecture review F9, 2026-08-20): in the shipped + plugin this router is **advisory** — the agent's choice of MCP + tool is the effective surface decision, made from the skill's + prose ladder, and the MCP handlers call this only *after the + fact* (passing ``chosen``) so telemetry records agreement vs + disagreement. Its return value is binding only for library + consumers who route their own render calls through it. The + markdown surface is outside its range entirely (it can return + only ``"widget"`` / ``"ask"``) — revisit when the markdown + surface gains an MCP tool. + Precedence, highest first: 1. **Capability floor** — a client that cannot render widgets gets diff --git a/tests/test_docs_drift.py b/tests/test_docs_drift.py new file mode 100644 index 0000000..0f7e5e0 --- /dev/null +++ b/tests/test_docs_drift.py @@ -0,0 +1,85 @@ +"""Docs drift catchers (architecture review finding F6, 2026-08-20). + +The grammar is documented by hand in three places — README's "The +grammar" section, the plugin skill, and the CHANGELOG — and the +construct COUNT has already rotted once (commit 543a7a0 hand-corrected +"six"). The code-level drift catchers (round-trip, CSS families, +version sync) had no docs-level counterpart, so the next construct's +documentation depended entirely on the author remembering. These tests +are that counterpart: they read the real files and fail red when the +grammar and its documentation disagree. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +from attune_forms import __all__ as _public_names +from attune_forms.mcp_server import tool_definitions +from attune_forms.models import QuestionType + +_ROOT = Path(__file__).resolve().parent.parent +_README = (_ROOT / "README.md").read_text(encoding="utf-8") +_SKILL = (_ROOT / "plugin" / "skills" / "forms" / "SKILL.md").read_text(encoding="utf-8") + +#: The plain controls; every other QuestionType member is a construct. +_CORE_TYPES = { + QuestionType.TEXT_INPUT, + QuestionType.SINGLE_SELECT, + QuestionType.MULTI_SELECT, + QuestionType.BOOLEAN, + QuestionType.NUMBER, + QuestionType.DATE, + QuestionType.TEXTAREA, +} +_CONSTRUCTS = [t for t in QuestionType if t not in _CORE_TYPES] + +_COUNT_WORDS = { + 5: "five", + 6: "six", + 7: "seven", + 8: "eight", + 9: "nine", + 10: "ten", + 11: "eleven", + 12: "twelve", +} + + +def test_readme_states_the_real_construct_count() -> None: + """ "eight constructs" must track the enum — the count is a + maintained invariant that has been hand-corrected before.""" + word = _COUNT_WORDS[len(_CONSTRUCTS)] + assert f"{word} constructs" in _README + + +def test_readme_describes_every_construct() -> None: + lower = _README.lower() + for qtype in _CONSTRUCTS: + name = qtype.value.replace("_", " ") + assert name in lower, f"README's grammar section is missing {qtype.value!r}" + + +def test_skill_describes_every_question_type() -> None: + lower = _SKILL.lower() + for qtype in QuestionType: + assert qtype.value in lower, f"SKILL.md is missing {qtype.value!r}" + + +def test_skill_names_only_real_mcp_tools() -> None: + real = {tool.name for tool in tool_definitions()} + named = set(re.findall(r"`(elicitation_[a-z_]+)`", _SKILL)) + ghosts = named - real + assert not ghosts, f"SKILL.md names MCP tool(s) that do not exist: {sorted(ghosts)}" + + +def test_skill_names_only_real_library_functions() -> None: + """A backticked transform name (`x_to_y` shape) in the skill must be + a real public export or a real MCP tool — the skill is the agent's + instruction sheet, and a renamed function leaves it instructing the + impossible.""" + real = set(_public_names) | {tool.name for tool in tool_definitions()} + named = {name for name in re.findall(r"`([a-z][a-z0-9_]*)\(?", _SKILL) if "_to_" in name} + ghosts = named - real + assert not ghosts, f"SKILL.md names library function(s) that do not exist: {sorted(ghosts)}" diff --git a/tests/test_grammar_completeness.py b/tests/test_grammar_completeness.py new file mode 100644 index 0000000..53f3bcf --- /dev/null +++ b/tests/test_grammar_completeness.py @@ -0,0 +1,175 @@ +"""Grammar-completeness drift catcher (architecture review F1 pin, 2026-08-20). + +Adding a construct touches ~19 places across four surfaces (see +``docs/adding-a-construct.md``). The review ruled that cost the honest +price of four genuinely different translations — no registry or base +class collapses it — so the protection is this completeness table +instead: every :class:`QuestionType` member must carry a row here, and +the rows pin the construct-specific output each surface must emit plus +a wrong-shaped answer the validator must reject. A construct wired +into only three of the four surfaces — or a new type added without +updating these tables — fails red instead of silently falling through +a default branch. + +The reference form is the fixture: it is drift-guarded elsewhere +(``test_reference_form``) to hold exactly one field per type. +""" + +from __future__ import annotations + +import json +import re +from typing import Any + +import pytest + +from attune_forms import ( + FormValidationError, + collect_form_response, + form_from_dict, + form_to_elicitation_schema, + form_to_markdown, + form_to_widget_html, +) +from attune_forms.models import QuestionType, expansion_items, ranking_slot_count +from attune_forms.reference_form import EXAMPLE_ANSWERS, REFERENCE_FORM + +#: How the widget's submit script reads each type's answer out of the +#: DOM. A deliberate second copy of ``widget._COLLECT_MODES``'s +#: *meaning* (not an import): a new type must state its mode here too, +#: which is the point of the table. +_WIDGET_COLLECT_MODES: dict[QuestionType, str] = { + QuestionType.TEXT_INPUT: "value", + QuestionType.SINGLE_SELECT: "value", + QuestionType.MULTI_SELECT: "checked-many", + QuestionType.BOOLEAN: "value", + QuestionType.NUMBER: "value", + QuestionType.DATE: "value", + QuestionType.TEXTAREA: "value", + QuestionType.DECISION: "checked-one", + QuestionType.PUSHBACK: "checked-one", + QuestionType.PROGRESS: "checked-one", + QuestionType.DELIBERATION: "checked-one", + QuestionType.TRIAGE: "rulings", + QuestionType.CONFIRM: "checked-one", + QuestionType.RANKING: "ranked", + QuestionType.ASSUMPTION_REVIEW: "rulings-with-text", +} + +#: A canonically WRONG-shaped answer per type — the validator must name +#: the field rather than accept it. +_WRONG_ANSWERS: dict[QuestionType, Any] = { + QuestionType.TEXT_INPUT: 42, + QuestionType.SINGLE_SELECT: "not-an-option", + QuestionType.MULTI_SELECT: "impl", # scalar where a list is required + QuestionType.BOOLEAN: "maybe", + QuestionType.NUMBER: "three", + QuestionType.DATE: "01/02/2026", + QuestionType.TEXTAREA: 42, + QuestionType.DECISION: "not-an-option", + QuestionType.PUSHBACK: "not-an-option", + QuestionType.PROGRESS: "not-an-option", + QuestionType.DELIBERATION: "not-an-option", + QuestionType.TRIAGE: ["fix now"], # list where a mapping is required + QuestionType.CONFIRM: "Maybe", + QuestionType.RANKING: ["staging", "staging", "canary"], # repeat + QuestionType.ASSUMPTION_REVIEW: {"py-floor": "maybe"}, +} + + +def _questions_by_type(): + form = form_from_dict(REFERENCE_FORM) + return form, {q.type: q for q in form.questions} + + +class TestTableCompleteness: + """The gate that makes the other tests a drift catcher: a new + QuestionType member without a row in BOTH tables fails here.""" + + def test_collect_mode_table_covers_every_type(self) -> None: + assert set(_WIDGET_COLLECT_MODES) == set(QuestionType) + + def test_wrong_answer_table_covers_every_type(self) -> None: + assert set(_WRONG_ANSWERS) == set(QuestionType) + + +class TestWidgetSurface: + def test_every_type_emits_its_pinned_collect_mode(self) -> None: + form, by_type = _questions_by_type() + html = form_to_widget_html(form, instance_id="grammar") + emitted = dict( + re.findall(r'data-fid="([^"]+)" data-ftype="[^"]+" data-collect="([^"]+)"', html) + ) + for qtype, question in by_type.items(): + assert emitted.get(question.id) == _WIDGET_COLLECT_MODES[qtype], qtype + + +class TestAskSurface: + def test_every_type_expands_to_the_expected_payload_count(self) -> None: + """TRIAGE expands per item, RANKING per slot, ASSUMPTION_REVIEW + per item plus its paired text lane; everything else is one + payload. Sizes derive from the shared helpers every surface is + required to iterate through.""" + _, by_type = _questions_by_type() + for qtype, question in by_type.items(): + payloads = question.to_ask_user_formats() + if qtype is QuestionType.TRIAGE: + expected = len(expansion_items(question)) + elif qtype is QuestionType.RANKING: + expected = ranking_slot_count(question) + elif qtype is QuestionType.ASSUMPTION_REVIEW: + expected = 2 * len(expansion_items(question)) + else: + expected = 1 + assert len(payloads) == expected, qtype + + +class TestElicitationSchemaSurface: + def test_every_type_projects_a_construct_shaped_property(self) -> None: + form, by_type = _questions_by_type() + props = form_to_elicitation_schema(form)["properties"] + for qtype, question in by_type.items(): + if qtype in (QuestionType.TRIAGE, QuestionType.ASSUMPTION_REVIEW): + for key, _item in expansion_items(question): + assert f"{question.id}.{key}" in props, qtype + if qtype is QuestionType.ASSUMPTION_REVIEW: + for key, _item in expansion_items(question): + assert f"{question.id}.{key}.text" in props + elif qtype is QuestionType.RANKING: + prop = props[question.id] + assert prop["type"] == "array" + assert prop["maxItems"] == ranking_slot_count(question) + else: + assert question.id in props, qtype + + +class TestMarkdownSurface: + def test_the_skeleton_carries_every_types_answer_shape(self) -> None: + """The reply skeleton is the markdown surface's contract: item- + keyed constructs must expose their per-item keys, a ranking its + ordered list, a multi-select a list — a type falling through to + a scalar placeholder is the silent-degradation this pins.""" + form, by_type = _questions_by_type() + md = form_to_markdown(form) + block = re.findall(r"```json\n(.*?)```", md, re.S)[-1] + skeleton = json.loads(block)["answers"] + for qtype, question in by_type.items(): + assert question.id in skeleton, qtype + value = skeleton[question.id] + if qtype in (QuestionType.TRIAGE, QuestionType.ASSUMPTION_REVIEW): + assert isinstance(value, dict), qtype + assert set(value) == {k for k, _ in expansion_items(question)} + elif qtype in (QuestionType.RANKING, QuestionType.MULTI_SELECT): + assert isinstance(value, list), qtype + + +class TestValidatorSurface: + @pytest.mark.parametrize("qtype", list(QuestionType), ids=lambda t: t.value) + def test_a_wrong_shaped_answer_is_rejected_by_name(self, qtype: QuestionType) -> None: + form, by_type = _questions_by_type() + question = by_type[qtype] + answers = dict(EXAMPLE_ANSWERS) + answers[question.id] = _WRONG_ANSWERS[qtype] + with pytest.raises(FormValidationError) as excinfo: + collect_form_response(form, answers) + assert question.id in str(excinfo.value)