diff --git a/CHANGELOG.md b/CHANGELOG.md index 2721c88..e18c51d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,31 @@ follow [SemVer](https://semver.org/). ## [Unreleased] +### Changed +- 0.6.x cleanup batch (architecture review findings F4/F5/F7, + 2026-08-20) — single-sourcing and schema hygiene, output + byte-identical (pinned by the characterization suite): + - The last policy duplications moved into `models`: + `BOOLEAN_OPTIONS` (was defined independently in bridge and + widget), `recommended_first()` (was implemented three times — + widget, markdown surface, and inline in `to_ask_user_format`), + `RATIONALE_HEADERS` and `PROGRESS_STATUS_ICONS` (each surface + carried its own copy with a "matches the widget" comment nothing + enforced). New `tests/test_single_sourcing.py` pins the + single-sourcing per surface + - `bridge._CONFIRM_DEFAULT_OPTIONS` deleted — the bridge now + consumes `models.CONFIRM_DEFAULT_OPTIONS`, making the 0.5.0 + changelog's single-sourcing claim true + - The MCP `_field_schema` types its object-array extras + (`progress_items`, `triage_items`, `consequences`, `assumptions`) + and gains a drift test: every `QuestionType` value must appear in + the schema's type enum and every `FormQuestion` field in its + properties (the prose description stays hand-written on purpose) + - Stale docstrings corrected: `keyboard_mode_enabled` and the + package overview now document `ATTUNE_FORMS_KEYBOARD_MODE` as the + preferred override with `ATTUNE_KEYBOARD_MODE` as the legacy + fallback, matching what the code reads + ### Fixed - **Directly-built D2 gate with a `default` can no longer pass unanswered** (checkpoint-2 promoted item, 2026-08-20 — empirically diff --git a/src/attune_forms/__init__.py b/src/attune_forms/__init__.py index 52baae7..865a1fb 100644 --- a/src/attune_forms/__init__.py +++ b/src/attune_forms/__init__.py @@ -23,7 +23,8 @@ than a question, and is never silently skipped. - :func:`keyboard_mode_enabled` — the user's terse/keyboard opt-out (D17), persisted per project in ``attune.config.json`` with - ``ATTUNE_KEYBOARD_MODE`` as a session override. + ``ATTUNE_FORMS_KEYBOARD_MODE`` (legacy fallback: + ``ATTUNE_KEYBOARD_MODE``) as a session override. - :func:`set_keyboard_mode` — persist that preference (what ``attune config set keyboard_mode`` calls). - :func:`needs_widget` — low-level *controls* check: True iff a form diff --git a/src/attune_forms/bridge.py b/src/attune_forms/bridge.py index 8f8392e..9a80dcf 100644 --- a/src/attune_forms/bridge.py +++ b/src/attune_forms/bridge.py @@ -24,6 +24,8 @@ from attune_forms.models import ( ASSUMPTION_RULINGS, ASSUMPTION_TEXT_SUFFIX, + BOOLEAN_OPTIONS, + CONFIRM_DEFAULT_OPTIONS, FormQuestion, FormResponse, FormSchema, @@ -34,10 +36,6 @@ triage_item_key, ) -#: The answer values accepted for a BOOLEAN question (its -#: ``to_ask_user_format`` renders as a Yes/No single-select). -_BOOLEAN_OPTIONS = ("Yes", "No") - #: ISO-8601 calendar-date format used by DATE questions. _DATE_FORMAT = "%Y-%m-%d" @@ -492,11 +490,6 @@ def _parse_suggested( return suggested, [] -#: The options a CONFIRM carries when the author names none. Exactly -#: two, always — the gate is two-way by ruling (confirm-construct D1). -_CONFIRM_DEFAULT_OPTIONS = ["Approve", "Abort"] - - #: The D2 constructs forbid a ``default`` outright — a pre-selected #: approval, a pre-filled order, or a pre-marked ruling defeats the #: two-way gate; approving/ordering/ruling must be an explicit act. @@ -522,9 +515,9 @@ def _parse_confirm_extras( """Parse the v7 CONFIRM extras and enforce its gate rules. Returns ``(consequences, options, problems)`` — options come back - defaulted to :data:`_CONFIRM_DEFAULT_OPTIONS` when the author named - none, and any count other than two is a definition error (D1: - the gate is two-way, always). + defaulted to :data:`~attune_forms.models.CONFIRM_DEFAULT_OPTIONS` + when the author named none, and any count other than two is a + definition error (D1: the gate is two-way, always). ``consequences`` is required for CONFIRM (a confirm with nothing to preview is a bare boolean and should be one) and invalid elsewhere: @@ -547,7 +540,7 @@ def _parse_confirm_extras( problems: list[str] = [] if not options: - options = list(_CONFIRM_DEFAULT_OPTIONS) + options = list(CONFIRM_DEFAULT_OPTIONS) elif len(options) != 2: problems.append(f"{where} type confirm requires exactly 2 options (got {len(options)})") @@ -1258,10 +1251,12 @@ def keyboard_mode_enabled(project_root: Path | None = None) -> bool: lives under ``keyboard_mode`` in the project-local ``attune.config.json``. - ``ATTUNE_KEYBOARD_MODE`` remains a session-scoped override in both - directions: set it truthy to force terse mode for one shell, or - falsey to force rich forms even where the project opted out. Unset - (or unrecognised) defers to the project file. + ``ATTUNE_FORMS_KEYBOARD_MODE`` (legacy fallback: + ``ATTUNE_KEYBOARD_MODE``, consulted only when the preferred name is + unset) remains a session-scoped override in both directions: set it + truthy to force terse mode for one shell, or falsey to force rich + forms even where the project opted out. Unset (or unrecognised) + defers to the project file. Args: project_root: Directory holding ``attune.config.json``. Defaults @@ -1510,8 +1505,8 @@ def _validate_assumption_review(question: FormQuestion, value: Any) -> str | Non def _validate_boolean(question: FormQuestion, value: Any) -> str | None: - """BOOLEAN: value must be exactly one of ``_BOOLEAN_OPTIONS``.""" - if value not in _BOOLEAN_OPTIONS: + """BOOLEAN: value must be exactly one of ``BOOLEAN_OPTIONS``.""" + if value not in BOOLEAN_OPTIONS: return f"{question.id!r} boolean value {value!r} must be 'Yes' or 'No'" return None diff --git a/src/attune_forms/markdown_surface.py b/src/attune_forms/markdown_surface.py index 28bb376..8613bad 100644 --- a/src/attune_forms/markdown_surface.py +++ b/src/attune_forms/markdown_surface.py @@ -23,26 +23,18 @@ from attune_forms.models import ( ASSUMPTION_RULINGS, + PROGRESS_STATUS_ICONS, + RATIONALE_HEADERS, FormQuestion, FormSchema, QuestionType, expansion_items, ranking_slot_count, + recommended_first, suggested_pick, ) from attune_forms.widget import WIDGET_RESPONSE_MARKER -#: Status icon per default-style progress status (matches the widget). -_PROGRESS_ICONS = {"done": "✓", "in_flight": "◐", "blocked": "✕"} - - -def _ordered_recommended_first(q: FormQuestion) -> list[str]: - """``q.options`` with ``q.recommended`` first, when it names one.""" - ordered = list(q.options) - if q.recommended and q.recommended in ordered: - ordered = [q.recommended] + [o for o in ordered if o != q.recommended] - return ordered - def _option_lines(q: FormQuestion, *, badge_for: dict[str, str] | None = None) -> list[str]: """Bullet (or numbered) lines for a select-like question's options. @@ -53,7 +45,7 @@ def _option_lines(q: FormQuestion, *, badge_for: dict[str, str] | None = None) - """ badges = badge_for or {} notes = q.option_notes or {} - ordered = _ordered_recommended_first(q) if badges else list(q.options) + ordered = recommended_first(q) if badges else list(q.options) lines = [] for idx, opt in enumerate(ordered): marker = f"{idx + 1}." if q.list_style == "ordered" else "-" @@ -85,7 +77,7 @@ def _progress_lines(q: FormQuestion) -> list[str]: if q.progress_style == "report": lines.append(f"- `{status}` {label}{detail}") else: - icon = _PROGRESS_ICONS.get(status, "•") + icon = PROGRESS_STATUS_ICONS.get(status, "•") lines.append(f"- {icon} {label}{detail}") if q.options: head = ( @@ -174,7 +166,7 @@ def _control_lines(q: FormQuestion) -> list[str]: return [ line + _endorsement_suffix(q, opt) for line, opt in zip( - lines, _ordered_recommended_first(q) if badges else list(q.options), strict=False + lines, recommended_first(q) if badges else list(q.options), strict=False ) ] if q.type == QuestionType.PROGRESS: @@ -201,14 +193,6 @@ def _control_lines(q: FormQuestion) -> list[str]: return lines -#: Rationale callout header per construct (matches the widget's). -_RATIONALE_HEADERS = { - QuestionType.PUSHBACK: "Why I'd push back", - QuestionType.PROGRESS: "Summary", - QuestionType.DELIBERATION: "Synthesis", -} - - def _field_lines(q: FormQuestion) -> list[str]: """All markdown lines for one question.""" req = "" if q.required else " *(optional)*" @@ -221,7 +205,7 @@ def _field_lines(q: FormQuestion) -> list[str]: lines.append(f"> guessed: `{q.default}` — {q.inferred_from}") lines.extend(_control_lines(q)) if q.rationale: - header = _RATIONALE_HEADERS.get(q.type, "Why") + header = RATIONALE_HEADERS.get(q.type, "Why") lines.append(f"> **{header}:** {q.rationale}") return lines diff --git a/src/attune_forms/mcp_server.py b/src/attune_forms/mcp_server.py index 2900d47..462ee52 100644 --- a/src/attune_forms/mcp_server.py +++ b/src/attune_forms/mcp_server.py @@ -117,7 +117,11 @@ def _field_schema() -> dict[str, Any]: "recommended": {"type": "string"}, "option_notes": {"type": "object"}, "user_position": {"type": "string"}, - "progress_items": {"type": "array"}, + "progress_items": { + "type": "array", + "items": {"type": "object"}, + "description": "progress: [{label, status, detail?}, ...]", + }, "progress_style": {"type": "string", "enum": ["report"]}, "endorsements": { "type": "object", @@ -125,6 +129,7 @@ def _field_schema() -> dict[str, Any]: }, "triage_items": { "type": "array", + "items": {"type": "object"}, "description": "triage: [{label, id?, detail?, tag?}, ...]", }, "dispositions": { @@ -143,6 +148,7 @@ def _field_schema() -> dict[str, Any]: }, "assumptions": { "type": "array", + "items": {"type": "object"}, "description": ( "assumption_review: [{label, id?, detail?, source?}, ...] — " "the inferred assumptions; source = where it was inferred from" @@ -154,6 +160,7 @@ def _field_schema() -> dict[str, Any]: }, "consequences": { "type": "array", + "items": {"type": "object"}, "description": "confirm: [{label, severity?, detail?}, ...]", }, "list_style": {"type": "string", "enum": ["ordered", "unordered"]}, diff --git a/src/attune_forms/models.py b/src/attune_forms/models.py index d527514..ed43999 100644 --- a/src/attune_forms/models.py +++ b/src/attune_forms/models.py @@ -174,6 +174,44 @@ def triage_item_key(item: dict[str, str]) -> str: #: :class:`FormQuestion` default identically (0.5.0 cleanup batch). CONFIRM_DEFAULT_OPTIONS = ("Approve", "Abort") +#: The Yes/No vocabulary of a BOOLEAN question — the values every +#: surface renders its control with and the only answers the validator +#: accepts. Single-sourced here so the widget's select, the flat-surface +#: options, and ``collect_form_response`` can never disagree (0.6.x +#: cleanup batch: it was defined independently in bridge and widget). +BOOLEAN_OPTIONS = ("Yes", "No") + +#: Status icon per PROGRESS status — the widget rows and the markdown +#: surface render the same three glyphs (0.6.x cleanup batch: each +#: surface carried its own copy with a "matches the widget" comment +#: nothing enforced). +PROGRESS_STATUS_ICONS = {"done": "✓", "in_flight": "◐", "blocked": "✕"} + +#: Rationale callout header per construct — the widget and markdown +#: surfaces show the same words above ``rationale`` (0.6.x cleanup +#: batch: previously duplicated per surface). Types absent here head +#: the callout "Why". +RATIONALE_HEADERS: dict["QuestionType", str] = { + QuestionType.PUSHBACK: "Why I'd push back", + QuestionType.PROGRESS: "Summary", + QuestionType.DELIBERATION: "Synthesis", +} + + +def recommended_first(question: "FormQuestion") -> list[str]: + """``question.options`` with ``question.recommended`` moved to the + front, when it names one of them. + + The recommended-first ordering is construct policy (the proposal + leads on every surface), so the widget's cards, the markdown option + lines, and the AskUserQuestion fallback all order through this one + function (0.6.x cleanup batch: it was implemented three times). + """ + ordered = list(question.options) + if question.recommended and question.recommended in ordered: + ordered = [question.recommended] + [o for o in ordered if o != question.recommended] + return ordered + def expansion_items(question: "FormQuestion") -> list[tuple[str, dict[str, str]]]: """The reviewed rows of an item-keyed construct as ``(key, item)`` @@ -410,9 +448,7 @@ def to_ask_user_format(self) -> dict[str, Any]: QuestionType.PROGRESS, QuestionType.DELIBERATION, ): - opts = list(self.options) - if self.recommended and self.recommended in opts: - opts = [self.recommended] + [o for o in opts if o != self.recommended] + opts = recommended_first(self) # DELIBERATION: the fallback surface has no chip row, so the # per-option endorsements fold into help_text — otherwise the # 2-1 split (the construct's whole payload) would be invisible. diff --git a/src/attune_forms/widget.py b/src/attune_forms/widget.py index 6191e43..e81095d 100644 --- a/src/attune_forms/widget.py +++ b/src/attune_forms/widget.py @@ -28,11 +28,15 @@ from attune_forms.bridge import is_fully_inferred from attune_forms.models import ( ASSUMPTION_RULINGS, + BOOLEAN_OPTIONS, + PROGRESS_STATUS_ICONS, + RATIONALE_HEADERS, FormQuestion, FormSchema, QuestionType, expansion_items, ranking_slot_count, + recommended_first, suggested_pick, ) from attune_forms.theme import CSS_BASE as _CSS_BASE @@ -43,10 +47,6 @@ #: ``collect_form_response``. Kept in sync with the ``elicit`` skill. WIDGET_RESPONSE_MARKER = "__elicitation_response__" -#: The Yes/No values a BOOLEAN control posts (``collect_form_response`` -#: validates a boolean answer against exactly these). -_BOOLEAN_OPTIONS = ("Yes", "No") - def _esc(value: object) -> str: """HTML-escape a value for safe use in text or a quoted attribute.""" @@ -85,22 +85,11 @@ def _list_html(q: FormQuestion, *, multi: bool) -> str: return f'<{tag} class="ae-list"{role}>{items}' -def _ordered_options_recommended_first(q: FormQuestion) -> list[str]: - """Return ``q.options`` with ``q.recommended`` moved to the front, - when it names one of them. Shared by the three card-based renders - (DECISION, PUSHBACK, PROGRESS report style). - """ - ordered = list(q.options) - if q.recommended and q.recommended in ordered: - ordered = [q.recommended] + [o for o in ordered if o != q.recommended] - return ordered - - def _control_decision_html(q: FormQuestion) -> str: """Render a DECISION control: recommended-first cards with notes.""" notes = q.option_notes or {} cards = "" - for opt in _ordered_options_recommended_first(q): + for opt in recommended_first(q): is_rec = opt == q.recommended badge = 'Recommended' if is_rec else "" note = f'{_esc(notes[opt])}' if opt in notes else "" @@ -125,7 +114,7 @@ def _control_pushback_html(q: FormQuestion) -> str: """ notes = q.option_notes or {} cards = "" - for opt in _ordered_options_recommended_first(q): + for opt in recommended_first(q): is_rec = opt == q.recommended is_user = opt == q.user_position badge = 'I'd suggest instead' if is_rec else "" @@ -167,7 +156,7 @@ def _control_progress_report_html(q: FormQuestion) -> str: f"{detail}" ) cards = "" - for opt in _ordered_options_recommended_first(q): + for opt in recommended_first(q): it = by_label.get(opt, {}) is_rec = opt == q.recommended badge = 'suggested next' if is_rec else "" @@ -208,7 +197,11 @@ def _control_progress_html(q: FormQuestion) -> str: if st in by_status: by_status[st].append(it) rows = "" - for status_key, icon, sr in (("done", "✓", "done"), ("in_flight", "◐", "in progress")): + status_rows = ( + ("done", PROGRESS_STATUS_ICONS["done"], "done"), + ("in_flight", PROGRESS_STATUS_ICONS["in_flight"], "in progress"), + ) + for status_key, icon, sr in status_rows: for it in by_status[status_key]: detail = ( f'{_esc(it["detail"])}' @@ -223,7 +216,7 @@ def _control_progress_html(q: FormQuestion) -> str: ) detail_by_label = {it.get("label"): it.get("detail") for it in by_status["blocked"]} cards = "" - for opt in _ordered_options_recommended_first(q): + for opt in recommended_first(q): is_rec = opt == q.recommended badge = 'suggested next' if is_rec else "" note_text = notes.get(opt) or detail_by_label.get(opt) @@ -234,7 +227,8 @@ def _control_progress_html(q: FormQuestion) -> str: f'' ) picker = ( @@ -259,7 +253,7 @@ def _control_deliberation_html(q: FormQuestion) -> str: notes = q.option_notes or {} endorse = q.endorsements or {} cards = "" - for opt in _ordered_options_recommended_first(q): + for opt in recommended_first(q): is_rec = opt == q.recommended badge = 'Synthesis pick' if is_rec else "" chips = "".join( @@ -485,7 +479,7 @@ def _control_single_select_html(q: FormQuestion) -> str: def _control_boolean_html(q: FormQuestion) -> str: """Render BOOLEAN as a Yes/No {opts}' @@ -625,12 +619,7 @@ def _field_html(q: FormQuestion) -> str: """ req = '*' if q.required else "" help_html = f'
{_esc(q.help_text)}
' if q.help_text else "" - rationale_headers = { - QuestionType.PUSHBACK: "Why I'd push back", - QuestionType.PROGRESS: "Summary", - QuestionType.DELIBERATION: "Synthesis", - } - rationale_h = rationale_headers.get(q.type, "Why") + rationale_h = _esc(RATIONALE_HEADERS.get(q.type, "Why")) rationale_html = ( f'
{rationale_h}' f"{_esc(q.rationale)}
" diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 8eb83e7..fd4c4eb 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -190,3 +190,23 @@ def test_schema_accepts_legal_triage_object_default(): form_from_dict(form) # legal by the library's own validator tools = {t.name: t for t in tool_definitions()} jsonschema.validate({"form": form}, tools["elicitation_render_form"].inputSchema) + + +def test_field_schema_covers_the_whole_grammar(): + """Drift catcher (architecture review F5, 2026-08-20): the + hand-maintained tool schema must keep up with the grammar — every + QuestionType value in its type enum, every FormQuestion field + (except the internal timestamp-free ones the schema derives) in its + properties. The prose description stays hand-written on purpose; do + NOT generate this schema from models.""" + from dataclasses import fields as dc_fields + + from attune_forms.mcp_server import _field_schema + from attune_forms.models import FormQuestion, QuestionType + + schema = _field_schema() + assert set(schema["properties"]["type"]["enum"]) == {t.value for t in QuestionType} + schema_props = set(schema["properties"]) + question_fields = {f.name for f in dc_fields(FormQuestion)} + missing = question_fields - schema_props + assert not missing, f"FormQuestion field(s) absent from _field_schema: {sorted(missing)}" diff --git a/tests/test_single_sourcing.py b/tests/test_single_sourcing.py new file mode 100644 index 0000000..b896bc3 --- /dev/null +++ b/tests/test_single_sourcing.py @@ -0,0 +1,127 @@ +"""Single-sourcing pins for cross-surface policy constants. + +The 0.6.x cleanup batch moved the last policy duplications into +:mod:`attune_forms.models`: the Yes/No boolean vocabulary, the confirm +gate's default options, the recommended-first option ordering, the +rationale callout headers, and the progress status icons. Each was +previously defined per surface with a "matches the widget" comment +nothing enforced. These tests pin the single-sourcing so a +reintroduced local copy (or a surface that stops consuming the shared +one) fails red instead of silently drifting. +""" + +from __future__ import annotations + +from attune_forms import bridge, form_from_dict, widget +from attune_forms.markdown_surface import form_to_markdown +from attune_forms.models import ( + BOOLEAN_OPTIONS, + CONFIRM_DEFAULT_OPTIONS, + PROGRESS_STATUS_ICONS, + RATIONALE_HEADERS, + QuestionType, + recommended_first, +) +from attune_forms.widget import form_to_widget_html + + +def _form(field: dict) -> dict: + return {"title": "Single sourcing", "fields": [field]} + + +class TestNoLocalCopies: + """The bridge and widget modules must consume the models constants, + not shadow them — the CHANGELOG's single-sourcing claim was false + once already (architecture review finding F4a, 2026-08-20).""" + + def test_bridge_has_no_local_confirm_default_options(self) -> None: + assert not hasattr(bridge, "_CONFIRM_DEFAULT_OPTIONS") + + def test_bridge_boolean_options_is_the_models_constant(self) -> None: + assert not hasattr(bridge, "_BOOLEAN_OPTIONS") + assert bridge.BOOLEAN_OPTIONS is BOOLEAN_OPTIONS + + def test_widget_boolean_options_is_the_models_constant(self) -> None: + assert not hasattr(widget, "_BOOLEAN_OPTIONS") + assert widget.BOOLEAN_OPTIONS is BOOLEAN_OPTIONS + + def test_confirm_defaults_to_the_shared_options(self) -> None: + form = form_from_dict( + _form( + {"id": "gate", "type": "confirm", "text": "Go?", "consequences": [{"label": "x"}]} + ) + ) + assert tuple(form.questions[0].options) == CONFIRM_DEFAULT_OPTIONS + + +class TestRecommendedFirstOrdering: + """All three surfaces order options through + :func:`~attune_forms.models.recommended_first`.""" + + _DECISION = { + "id": "route", + "type": "decision", + "text": "Which route?", + "options": ["a", "b", "c"], + "recommended": "b", + "rationale": "because", + } + + def test_helper_moves_the_recommendation_first(self) -> None: + form = form_from_dict(_form(self._DECISION)) + assert recommended_first(form.questions[0]) == ["b", "a", "c"] + + def test_widget_cards_render_in_helper_order(self) -> None: + form = form_from_dict(_form(self._DECISION)) + html = form_to_widget_html(form, instance_id="ss") + positions = [html.index(f'value="{o}"') for o in recommended_first(form.questions[0])] + assert positions == sorted(positions) + + def test_markdown_options_render_in_helper_order(self) -> None: + form = form_from_dict(_form(self._DECISION)) + md = form_to_markdown(form) + positions = [md.index(f"- {o}") for o in recommended_first(form.questions[0])] + assert positions == sorted(positions) + + def test_ask_fallback_options_are_the_helper_order(self) -> None: + form = form_from_dict(_form(self._DECISION)) + payload = form.questions[0].to_ask_user_format() + assert payload["options"] == recommended_first(form.questions[0]) + + +class TestSharedPresentationConstants: + def test_widget_and_markdown_show_the_same_rationale_header(self) -> None: + field = { + "id": "push", + "type": "pushback", + "text": "You proposed X.", + "options": ["X", "Y"], + "user_position": "X", + "recommended": "Y", + "rationale": "Y is safer.", + } + form = form_from_dict(_form(field)) + header = RATIONALE_HEADERS[QuestionType.PUSHBACK] + html = form_to_widget_html(form, instance_id="ss") + assert widget._esc(header) in html + assert f"**{header}:**" in form_to_markdown(form) + + def test_widget_and_markdown_show_the_same_progress_icons(self) -> None: + field = { + "id": "prog", + "type": "progress", + "text": "Status.", + "options": ["Blocked one"], + "progress_items": [ + {"label": "Done one", "status": "done"}, + {"label": "Rolling one", "status": "in_flight"}, + {"label": "Blocked one", "status": "blocked"}, + ], + } + form = form_from_dict(_form(field)) + html = form_to_widget_html(form, instance_id="ss") + md = form_to_markdown(form) + for status in ("done", "in_flight", "blocked"): + assert PROGRESS_STATUS_ICONS[status] in html + for status in ("done", "in_flight"): + assert PROGRESS_STATUS_ICONS[status] in md