From f68138d85c89fd490648e3dbe2216781e00de2f8 Mon Sep 17 00:00:00 2001 From: Daniel Vejsada Date: Sun, 20 Sep 2026 23:13:39 +0200 Subject: [PATCH 1/3] admin: count master-YAML templates on a section's Overview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by reviewing the deployed instance, which has the shape that exposes it: 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 smlouva_o_poskytovani_sluzeb as Live. Server > Status counted 1 live Word tool. But the Word Overview said 0 live, the dashboard tile said 0, and the tool was missing entirely from the table headed "Tools the AI can call" — which it can. section_facts() built its counts and its tool list from store.list_specs(), the managed half only. template_table() walks the managed specs *and* unmanaged_master_specs(), which is why the Templates tab was the one page that was right. A hand-written template registers the same MCP tool as one made here, so it belongs in both. Both halves now feed the record. The origin on each ToolFact keeps them distinguishable — "From master YAML" rather than "From a template", because one is editable here and the other is not, and knowing which saves a trip to the Templates tab to find out why a row has no Edit button. The note above the table changes with it: it used to say master templates are shown read-only elsewhere, which read as "and are not counted here"; it now says they are counted. The regression tests register the template the way main.py does at startup. A fixture that only writes the YAML has a template the registry has never heard of, so "live 0" would be correct there and the test would pass against the bug — which is exactly what the first draft of it did. Co-Authored-By: Claude Opus 5 (1M context) --- admin/app.py | 33 +++++++---- admin/views/sections.py | 24 ++++++-- docs/development/admin-ui.md | 13 ++++ tests/test_admin_master_templates.py | 89 ++++++++++++++++++++++++++++ 4 files changed, 141 insertions(+), 18 deletions(-) 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/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") From 15be1f8e77883f42f844510f4815704e40e32798 Mon Sep 17 00:00:00 2001 From: Daniel Vejsada Date: Sun, 20 Sep 2026 23:13:52 +0200 Subject: [PATCH 2/3] pptx: a rejected slide field says what that type does take MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same review turned up a real failed call on the deployed server: Invalid slides: slide 0 -> body: Extra inputs are not permitted; slide 2 -> subtitle: Extra inputs are not permitted; slide 3 -> subtitle: … slide 7 -> body: Extra inputs are not permitted A seven-slide deck, lost whole — the schema is strict, so one field on the wrong type fails the entire call and nothing is generated. The caller had assumed what most people assume: that every slide takes a title, a subtitle and a body. It put 'body' on the title slide and 'subtitle' on the content slides. The published schema already says which types accept which field — 'subtitle' renders as "[title] … [closing] …". So the schema was not the gap. The gap is that the error names the mistake and not the fix, and by the time it arrives the caller has read the schema once and drawn the wrong conclusion. Repeating the rule where it failed is what turns a retry into a correction: 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, so a new slide type is covered without touching the error path, and a field no type declares says so rather than pointing nowhere. Only extra_forbidden is rewritten; every other error keeps pydantic's own wording, which already says something useful. The tool description gains one paragraph on the same point, including that the call fails whole — worth knowing before writing eight slides — and the reference page a matching line under its per-type field table. Also corrects coerce_slides()'s docstring, which claimed MCP callers never reach this path because FastMCP validates first. They always reach it: the parameter is declared Any carrying a hand-built schema, so nothing validates earlier. These strings are the tool's error message, not developer diagnostics, and the docstring now says so. Co-Authored-By: Claude Opus 5 (1M context) --- docs/development/tools/powerpoint.md | 21 +++++++++ docs/powerpoint-slides.md | 2 + main.py | 7 +++ pptx_tools/schema.py | 64 +++++++++++++++++++++++++--- tests/test_pptx_schema.py | 53 +++++++++++++++++++++++ 5 files changed, 142 insertions(+), 5 deletions(-) diff --git a/docs/development/tools/powerpoint.md b/docs/development/tools/powerpoint.md index 01072a0..3d560f4 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 fbc5478..d38120d 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. + **Sections.** Every `section` slide also starts a section in PowerPoint's outline pane and slide sorter, so the presenter sees the deck's structure rather than a flat list. Slides before the first section slide go in a "Default Section", as PowerPoint would name it. **Blank slides** are the escape hatch for the one layout no typed slide fits. Elements draw in order, so a later one sits on top; a `text` element takes the same inline markdown as a bullet, a `shape` is one of `rectangle`, `rounded_rectangle`, `ellipse`, `chevron`, `arrow` with an optional `fill` and centred `text`, and an `image` keeps its aspect ratio within its box. Anything that would run past the slide edge is shrunk to fit and reported in `warnings`; anything starting off the slide is skipped and reported. A `title` on a blank slide is kept: the blank layout has no title placeholder, so it is drawn as a text box where the template puts its titles, and elements you position draw over it. Element coordinates are absolute on the slide, and a blank layout usually still carries the template's logo, header rule and footer — `list_presentation_templates` with `include_layouts` reports each template's `content_area`, the band that stays clear of them, as percentages you can use directly. 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..8aabe34 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,30 @@ 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 it, because it is the slide's own type and the only thing that + # says which field set applied. index = loc[0] + slide_type = next((part for part in loc[1:] if part in SLIDE_TYPES), None) rest = [part for part in loc[1:] if part not in SLIDE_TYPES] 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_pptx_schema.py b/tests/test_pptx_schema.py index 5fc9ef0..3031153 100644 --- a/tests/test_pptx_schema.py +++ b/tests/test_pptx_schema.py @@ -101,6 +101,59 @@ 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_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"}]) From d8ceade9dd27f82167f58ee43049381eec43c71d Mon Sep 17 00:00:00 2001 From: Daniel Vejsada Date: Sun, 20 Sep 2026 23:30:24 +0200 Subject: [PATCH 3/3] pptx: stop the error path eating the 'title' field's name (#197) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review flagged as a latent risk that the slide-type/field disambiguation would misbehave "if a field is ever named the same as a slide type". It already is: `title` is a field on every slide and also a slide type. The path was built by dropping every segment that matched a type name, so the one field whose name collides was silently removed from it. An error about a slide's title rendered as slide 0: Input should be a valid string naming no field at all — on the field every single slide has. Whatever else a caller can be expected to work out, that one is unguessable. The discriminator tag is always at position 1 of a tagged union's path, so position is what identifies it. Exactly one leading segment is dropped, and a collision cannot recur however the field names change. The same change makes this commit's own slide_type lookup exact rather than first-match. slide 0 -> title: Input should be a valid string Pre-existing, not introduced here, but this PR builds on that filter, so it is this PR's to fix. Co-Authored-By: Claude Opus 5 (1M context) --- pptx_tools/schema.py | 15 +++++++++++---- tests/test_pptx_schema.py | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/pptx_tools/schema.py b/pptx_tools/schema.py index 8aabe34..0a0a64f 100644 --- a/pptx_tools/schema.py +++ b/pptx_tools/schema.py @@ -953,12 +953,19 @@ 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 — - # but keep it, because it is the slide's own type and the only thing that + # 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] - slide_type = next((part for part in loc[1:] if part in SLIDE_TYPES), None) - 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) diff --git a/tests/test_pptx_schema.py b/tests/test_pptx_schema.py index 3031153..3282e93 100644 --- a/tests/test_pptx_schema.py +++ b/tests/test_pptx_schema.py @@ -137,6 +137,29 @@ def test_a_field_no_type_has_says_so(self): 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