diff --git a/admin/app.py b/admin/app.py index 5613d76..1522129 100644 --- a/admin/app.py +++ b/admin/app.py @@ -290,19 +290,27 @@ def section_facts(self, s) -> "views.SectionFacts": the live registries and the metrics counters — the three things a view is not allowed to know about. The view gets a plain record. """ - specs = self.store.list_specs(s.kind) if s.kind else [] + # Both halves of what the Templates tab lists. A template written by + # hand in the master YAML is as live as one made here — it registers + # the same tool — so counting only the managed specs made this page + # disagree with the Templates tab beside it and with the Server + # section's "live Word tools", and left a tool the AI can call out of + # the table headed "Tools the AI can call". + managed = self.store.list_specs(s.kind) if s.kind else [] + from_master = self.unmanaged_master_specs(s.kind) if s.kind else [] + specs = [(spec, "template") for spec in managed] + specs += [(spec, "master") for spec in from_master] live_names = set(self.live_names(s.kind)) if s.kind else set() - enabled = [spec for spec in specs if is_enabled(spec)] tools = [self._tool_fact(name, origin="static") for name in s.tools] # A docx or email template is a tool of its own and belongs in the # same list; a pptx template is not, so it is counted as a template # and nothing else. descriptor().has_args is the distinction. if s.kind and descriptor(s.kind).has_args: - for spec in specs: + for spec, origin in specs: name = spec.get("name") tools.append(self._tool_fact( - name, origin="template", live=name in live_names)) + name, origin=origin, live=name in live_names)) slots = [] for slot in s.slots: @@ -320,13 +328,11 @@ def section_facts(self, s) -> "views.SectionFacts": notes.append(( f"No {slot.label} file is installed. {slot.controls}", "warn")) - if s.kind: - unmanaged = self.unmanaged_master_specs(s.kind) - if unmanaged: - notes.append(( - f"{len(unmanaged)} template(s) come from the hand-written " - "master YAML and are shown read-only on the Templates tab.", - "info")) + if from_master: + notes.append(( + f"{len(from_master)} template(s) come from the hand-written " + "master YAML. They are counted here and shown read-only on " + "the Templates tab.", "info")) extra = [] if s.kind == KIND_PPTX: @@ -334,9 +340,10 @@ def section_facts(self, s) -> "views.SectionFacts": return views.SectionFacts( templates=len(specs), - live=len([spec for spec in specs + live=len([spec for spec, _origin in specs if spec.get("name") in live_names]), - disabled=len(specs) - len(enabled), + disabled=len([spec for spec, _origin in specs + if not is_enabled(spec)]), tools=tuple(tools), slots=tuple(slots), notes=tuple(notes), extra_stats=tuple(extra), ) diff --git a/admin/views/sections.py b/admin/views/sections.py index ce1f260..541eedd 100644 --- a/admin/views/sections.py +++ b/admin/views/sections.py @@ -38,7 +38,8 @@ class ToolFact: name: str live: bool = True - #: "static" for a tool always present, "template" for one a spec created. + #: "static" for a tool always present, "template" for one this UI manages, + #: "master" for one declared in the hand-written master YAML. origin: str = "static" calls: int = 0 errors: int = 0 @@ -111,11 +112,24 @@ def section_page(ctx, s: Section, tab: str, *content, # Overview # --------------------------------------------------------------------------- +#: How each origin reads in the Overview's tool table. +_ORIGIN_LABELS = { + "template": "From a template", + "master": "From master YAML", + "static": "Always available", +} + + def _origin_badge(t: ToolFact): - """Where a tool comes from — a classification, so no status dot.""" - if t.origin == "template": - return c.badge("From a template", "off", plain=True) - return c.badge("Always available", "off", plain=True) + """Where a tool comes from — a classification, so no status dot. + + A master-YAML template is called out rather than folded in with the rest: + it is live like any other, but it is not editable here, so knowing which + one it is saves a trip to the Templates tab to find out why it has no + Edit button. + """ + label = _ORIGIN_LABELS.get(t.origin, t.origin) + return c.badge(label, "off", plain=True) def _tool_row(t: ToolFact): diff --git a/docs/development/admin-ui.md b/docs/development/admin-ui.md index c7ad7eb..f753c24 100644 --- a/docs/development/admin-ui.md +++ b/docs/development/admin-ui.md @@ -65,6 +65,19 @@ Three consequences worth knowing: into a `views.SectionFacts`, and the view renders that. A tool no page in the admin UI mentions is a tool nobody can check on. + It walks **both** halves of what the Templates tab lists — the managed `*.d` + specs *and* `unmanaged_master_specs()`. A hand-written master-YAML template + registers the same tool as a managed one, so counting only the managed half + made this page disagree with the Templates tab beside it and with the Server + section's "live Word tools", and left a live tool out of the table headed + "Tools the AI can call". It surfaced on the deployed instance, which has + exactly that shape: one master-YAML Word template and none managed here. The + `origin` on each `ToolFact` keeps the two distinguishable — `From master + YAML` rather than `From a template`, since one is editable here and the other + is not. `tests/test_admin_master_templates.py` registers the template the way + startup does: a fixture that only writes the YAML has nothing registered, and + "live 0" would be correct there. + ### Section routes must not shadow the handlers below them A section slug occupies the same first path segment as a template kind, and diff --git a/docs/development/tools/powerpoint.md b/docs/development/tools/powerpoint.md index 8a28cac..cddfccc 100644 --- a/docs/development/tools/powerpoint.md +++ b/docs/development/tools/powerpoint.md @@ -122,6 +122,27 @@ read. The tool parameter declares that flat schema through `WithJsonSchema`; validation still runs against the union in `coerce_slides()`, on the worker thread, where the error can say `slide 2 -> rows.0: …`. +**`coerce_slides()`'s wording *is* the tool's error message.** Because the +parameter is declared `Any` with a hand-built schema, FastMCP passes the +payload straight through and every rejection is raised here — there is no +earlier validator whose message a caller might see instead. So these strings +are model-facing API, not developer diagnostics. + +That is why `extra_forbidden` is rewritten rather than passed through. A model +that assumes every slide takes a title, a subtitle and a body — the common +assumption — used to get only "Extra inputs are not permitted", which names +the mistake and not the fix; a production call lost a whole seven-slide deck +to exactly that. `_rejected_field()` now answers both halves: + + slide 1 -> subtitle: 'subtitle' is not a field of a 'section' slide + (it takes: title, notes, layout); 'subtitle' belongs to: title, closing. + +Both sides are derived from `_SLIDE_MODELS` (`_fields_by_type()` and +`_types_by_field()`), so a new slide type is covered without touching the +error path, and a field no type declares says so rather than pointing +nowhere. Every other error keeps pydantic's own wording — only the one that +said nothing actionable is replaced. + Two shims sit in front of validation. `migrate_legacy_slide()` renames the previous key spellings (`slide_type`, `slide_text`, `chart_data`, …) so old client prompts still work, logging once per call. `slide_from_text()` reads diff --git a/docs/powerpoint-slides.md b/docs/powerpoint-slides.md index 13e3ac8..c0ceb61 100644 --- a/docs/powerpoint-slides.md +++ b/docs/powerpoint-slides.md @@ -37,6 +37,8 @@ Every slide takes `type` plus optional `title`, `notes` (speaker notes) and `lay | `closing` | `subtitle?`, `contact?` | | `blank` | `elements` — positioned items, each `{kind: text\|image\|shape, x, y, w, h?}` with lengths in inches (`1.5`, `"1.5in"`) or as a share of the slide (`"40%"`) | +**Fields are per type.** A field not listed for a type is rejected, and the call fails without generating anything — there is no partial deck. `subtitle` exists only on `title` and `closing`; bullets go in `body`, which only `content`, `chart` and `image` take (`two_column` puts them in `left` and `right`); `section` carries a title and nothing else. A rejection names the slide, the field, what that type does take and which types the field belongs to, so it can be corrected in one pass. + **Colours come from your template.** Table headers use its `accent1` and zebra rows its `bg2`, and text the tool draws itself — KPI figures, timeline captions, quotes, positioned text, chart axis labels — takes the colour your template gives body text. A dark template gets readable text without you asking. Override any of it per slide with `header_color`, `fills` or a `fill` on a shape. **Empty placeholders are removed.** A layout usually reserves more boxes than a slide fills — a section layout's body, a comparison column you gave no heading, a third card. Anything left empty is dropped from the finished deck, so it does not open with "Click to add text" boxes in it. PowerPoint's Reset Slide brings them back from the layout if you want them. diff --git a/main.py b/main.py index b33ce9b..607380a 100644 --- a/main.py +++ b/main.py @@ -384,6 +384,13 @@ async def create_powerpoint_presentation( "([x, y] point pairs). Keep each series' 'values' the same length as 'categories'. Both " "'chart' and 'image' take an optional 'body' to put takeaways beside them.\n" "\n" + "FIELDS ARE PER TYPE: a field belonging to another type is rejected and the whole " + "call fails — nothing is generated. 'subtitle' is only on 'title' and 'closing'; " + "'body' only on 'content', 'chart' and 'image' ('two_column' puts its bullets in " + "'left' and 'right'); 'section' carries a title and nothing else. Every field's " + "description names the types that accept it, and a rejection names what the type " + "you used does take.\n" + "\n" "STRUCTURE: open with 'title', then 'agenda' (omit its 'items' and it is built from the " "deck's own section slides), divide with 'section', and end with 'closing'. Each 'section' " "slide also starts a section in PowerPoint's outline pane. Use 'kpi' for two to four " diff --git a/pptx_tools/schema.py b/pptx_tools/schema.py index 7b01fa1..0a0a64f 100644 --- a/pptx_tools/schema.py +++ b/pptx_tools/schema.py @@ -899,6 +899,52 @@ def flat_slide_schema() -> Dict[str, Any]: # Entry point for non-MCP callers # ============================================================================= +@lru_cache(maxsize=1) +def _fields_by_type() -> Dict[str, Tuple[str, ...]]: + """Every field name each slide type accepts, in declaration order.""" + return { + _slide_type_of(model): tuple( + name for name in model.model_fields if name != "type" + ) + for model in _SLIDE_MODELS + } + + +@lru_cache(maxsize=1) +def _types_by_field() -> Dict[str, Tuple[str, ...]]: + """Which slide types accept each field name.""" + owners: Dict[str, List[str]] = {} + for slide_type, fields in _fields_by_type().items(): + for name in fields: + owners.setdefault(name, []).append(slide_type) + return {name: tuple(types) for name, types in owners.items()} + + +def _rejected_field(slide_type: Optional[str], field: str) -> str: + """Why this field was refused, and what to do instead. + + "Extra inputs are not permitted" names the mistake and not the fix, so a + model that assumed every slide takes a title, a subtitle and a body — the + common assumption, and the one that cost a whole deck in production — gets + told only that it is wrong. The schema does say which types take which + field, but by then the caller has already read it once and drawn the wrong + conclusion; repeating it at the point of failure is what turns a retry + into a correction rather than a guess. + """ + detail = f"'{field}' is not a field of" + detail += f" a '{slide_type}' slide" if slide_type else " this slide type" + if slide_type: + accepted = _fields_by_type().get(slide_type, ()) + if accepted: + detail += f" (it takes: {', '.join(accepted)})" + owners = _types_by_field().get(field, ()) + if owners: + detail += f"; '{field}' belongs to: {', '.join(owners)}" + else: + detail += "; no slide type has that field" + return detail + "." + + def _describe_error(error: Dict[str, Any]) -> str: """Render one pydantic error as 'slide 2 -> rows.0: message'.""" loc = [str(part) for part in error.get("loc", ())] @@ -907,22 +953,37 @@ def _describe_error(error: Dict[str, Any]) -> str: # JSON) carry no path and already name the slide they are about. if not loc: return message.removeprefix("Value error, ") - # Drop the union-member tag pydantic injects so the path reads naturally. + # Drop the union-member tag pydantic injects so the path reads naturally, + # but keep hold of it: it is the slide's own type, and the only thing that + # says which field set applied. + # + # Exactly one leading segment, never "every segment that looks like a type + # name". `title` is both a field on every slide and a slide type, so + # filtering by membership silently ate it: an error about the title field + # rendered as "slide 0: Input should be a valid string", naming no field + # at all. The tag is always at position 1 of a tagged union's path, so + # position is what identifies it. index = loc[0] - rest = [part for part in loc[1:] if part not in SLIDE_TYPES] + rest = list(loc[1:]) + slide_type = rest.pop(0) if rest and rest[0] in SLIDE_TYPES else None where = f"slide {index}" if rest: where += " -> " + ".".join(rest) + if error.get("type") == "extra_forbidden" and len(rest) == 1: + message = _rejected_field(slide_type, rest[0]) return f"{where}: {message}" def coerce_slides(slides: Any) -> List[Any]: """Validate *slides* into typed models, raising a readable ``ValueError``. - MCP callers never reach the error path — FastMCP validates against the tool - signature first — but direct callers (tests, ``create_presentation``) do, - and a raw pydantic dump is poor feedback for a model trying to correct - itself. + This is the *only* validation an MCP caller meets, so its wording is the + tool's error message. The parameter is declared as ``Any`` carrying a + hand-built JSON schema (see :data:`SlideInput`), so FastMCP passes the + payload through untouched and every rejection is raised here — which is + why these messages name the slide, the field and, for a field the type + does not have, what it does take. A raw pydantic dump is poor feedback for + a model trying to correct itself, and worse when it is all the model gets. """ if isinstance(slides, list) and slides and all( isinstance(slide, SlideBase) for slide in slides diff --git a/tests/test_admin_master_templates.py b/tests/test_admin_master_templates.py index ff1b865..2f88231 100644 --- a/tests/test_admin_master_templates.py +++ b/tests/test_admin_master_templates.py @@ -460,3 +460,92 @@ def test_a_managed_templates_yaml_block_still_says_the_ui_wrote_it(admin_client) html = client.get("/admin/docx/legacy_letter/edit").text assert "what the UI wrote" in html + + +# --------------------------------------------------------------------------- +# The Overview tab counts it (#194 follow-up) +# --------------------------------------------------------------------------- + + +def _install_and_register(mcp, custom, cfg, spec=None): + """Write a master-YAML template and register it, as startup would. + + The admin app does not register dynamic tools — ``main.py`` does that once + at startup — so a test that only writes the YAML has a template the + registry has never heard of, and "live" is legitimately 0. The deployed + server has it registered, which is the shape this bug appeared in, so the + test has to do what startup does or it is measuring something else. + """ + from docx_tools.dynamic_docx_tools import register_docx_template_tools_from_yaml + + master = _write_master(cfg, spec or MASTER_SPEC) + (custom / "legacy_letter.docx").write_bytes(_docx_bytes()) + register_docx_template_tools_from_yaml(mcp, master) + return master + + +def test_the_overview_counts_a_master_yaml_template(admin_client): + """A hand-written template is as live as a managed one, and says so. + + Found on the deployed instance, which has exactly this shape: one Word + template declared in config/docx_templates.yaml and none managed by the + UI. Three pages then disagreed about the same fact — the Templates tab + showed it Live, Server > Status counted 1 live Word tool, and the Word + Overview said 0 with the tool missing from the table headed "Tools the AI + can call". The counts came from `store.list_specs()` alone, which is the + managed half only; `template_table()` walks both, which is why it was the + one page that was right. + """ + client, mcp, custom, cfg = admin_client + _install_and_register(mcp, custom, cfg) + + overview = client.get("/admin/word").text + assert "legacy_letter" in overview, ( + "a live tool is missing from the table of tools the AI can call") + assert "From master YAML" in overview, ( + "it is live but not editable here; the table should say which it is") + + # The dashboard tile counts it too — it reads from the same record. Match + # the tile, not the nav link of the same href, which carries no counts. + tile = re.search(r'.*?', + client.get("/admin/").text, re.S) + assert tile, "no Word tile on the dashboard" + assert "1live" in re.sub(r"\s+", "", tile.group(0)), ( + "the Word tile must count the master-YAML template as live") + + +def test_the_overview_and_the_server_status_agree_on_live_word_tools(admin_client): + """The two counts are of the same thing and must not contradict. + + Server > Status reads the registry through `live_names()`, the Overview + reads the specs. One counting a template the other does not is the shape + of the bug, whichever way round it happens. + """ + client, mcp, custom, cfg = admin_client + _install_and_register(mcp, custom, cfg) + + status = client.get("/admin/server/status").text + on_status = re.search( + r'
(\d+)
\s*
Live Word tools
', + status) + assert on_status, "the Status tab no longer reports live Word tools" + assert on_status.group(1) == "1", "the fixture did not register the tool" + + # +1: create_word_document is always live and is not a template. + overview = client.get("/admin/word").text + live_rows = len(re.findall(r"badge badge-live", overview)) + assert live_rows == int(on_status.group(1)) + 1, ( + "Overview and Status disagree about how many Word tools are live") + + +def test_a_disabled_master_template_is_counted_as_disabled(admin_client): + """`enabled: false` in the master YAML is a real state, not an absence.""" + client, mcp, custom, cfg = admin_client + _install_and_register(mcp, custom, cfg, dict(MASTER_SPEC, enabled=False)) + + overview = client.get("/admin/word").text + assert "legacy_letter" in overview + # One template, not live, so the Templates card's Disabled count is 1. + assert re.search(r'
1
\s*' + r'
Disabled
', overview), ( + "a disabled master-YAML template must show as disabled, not missing") diff --git a/tests/test_pptx_schema.py b/tests/test_pptx_schema.py index 5fc9ef0..3282e93 100644 --- a/tests/test_pptx_schema.py +++ b/tests/test_pptx_schema.py @@ -101,6 +101,82 @@ def test_error_names_the_slide_and_field(self): assert "slide 1" in message assert "series.0.values.0" in message + def test_a_field_from_another_type_says_what_this_type_takes(self): + """The production failure: every slide assumed to take title/subtitle/body. + + A real call lost a whole seven-slide deck to this — 'body' on a title + slide, 'subtitle' on the content slides — and the only feedback was + "Extra inputs are not permitted", which names the mistake and not the + fix. The schema does say which types take which field, but the caller + has already read it once and drawn the wrong conclusion; saying it + again at the point of failure is what makes the retry a correction. + """ + with pytest.raises(ValueError) as excinfo: + coerce_slides([ + {"type": "title", "title": "T", "subtitle": "S", "body": "- x"}, + {"type": "section", "title": "S", "subtitle": "sub"}, + {"type": "content", "title": "C", "subtitle": "s", "body": "- a"}, + ]) + message = str(excinfo.value) + # Which slide, which field — as before. + assert "slide 0 -> body" in message + assert "slide 1 -> subtitle" in message + # ...and now what that type does take, and where the field belongs. + assert "not a field of a 'title' slide" in message + assert "it takes: title, notes, layout, subtitle" in message + assert "'body' belongs to: content, chart, image" in message + assert "'subtitle' belongs to: title, closing" in message + assert "Extra inputs are not permitted" not in message, ( + "the pydantic wording says nothing actionable and is replaced") + + def test_a_field_no_type_has_says_so(self): + """A typo has no owner to point at, and must not claim one.""" + with pytest.raises(ValueError) as excinfo: + coerce_slides([{"type": "content", "title": "C", "bullets": ["a"]}]) + message = str(excinfo.value) + assert "no slide type has that field" in message + assert "it takes: title, notes, layout, body" in message + + def test_an_error_on_a_field_named_like_a_slide_type_keeps_its_name(self): + """'title' is both a field on every slide and a slide type. + + The path used to be built by dropping every segment that matched a + slide-type name, which ate the one field whose name collides — an + error about the title rendered as "slide 0: Input should be a valid + string", naming no field at all, on the field every single slide has. + The discriminator tag is always at position 1 of a tagged union's + path, so position identifies it and a collision cannot recur. + """ + with pytest.raises(ValueError) as excinfo: + coerce_slides([{"type": "content", "title": {"bad": 1}, + "body": "- a"}]) + assert "slide 0 -> title" in str(excinfo.value) + + def test_a_nested_path_survives_the_tag_being_dropped(self): + """Only the tag goes; everything under it is the path worth printing.""" + with pytest.raises(ValueError) as excinfo: + coerce_slides([{"type": "chart", "title": "c", "chart_type": "pie", + "categories": ["a"], + "series": [{"name": "s", "values": ["nope"]}]}]) + assert "slide 0 -> series.0.values.0" in str(excinfo.value) + + def test_every_type_can_describe_its_own_fields(self): + """The guidance is generated, so a new slide type is covered for free.""" + from pptx_tools.schema import _fields_by_type + + fields = _fields_by_type() + assert set(fields) == set(SLIDE_TYPES) + for slide_type, names in fields.items(): + assert names[:3] == ("title", "notes", "layout"), slide_type + assert "type" not in names + + def test_other_errors_keep_their_own_wording(self): + """Only extra_forbidden is rewritten; a bad value still says why.""" + with pytest.raises(ValueError) as excinfo: + coerce_slides([{"type": "table", "rows": [["a"]], + "header_color": "cornflower"}]) + assert "not a field of" not in str(excinfo.value) + def test_bad_colour_is_rejected_with_guidance(self): with pytest.raises(ValueError) as excinfo: coerce_slides([{"type": "table", "rows": [["a"]], "header_color": "cornflower"}])