admin: count master-YAML templates on Overview; pptx: say what a slide type takes - #197
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Both sides added a paragraph to the same place in docs/powerpoint-slides.md, under the per-type field table. Kept both, the field contract first because it is what the table directly above it is a list of. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@claude review |
|
Claude finished @dvejsada's task in 2m 4s —— View job Review complete
Both fixes are correct and well-targeted. Details below. 1. Overview counting fix (
|
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) <noreply@anthropic.com>
|
Thanks — no changes needed for the two fixes themselves, but the 'latent fragility' is worth reporting back on, because it is not latent.
It already has. No field named — on the field every single slide has. Fixed in d8ceade: the discriminator tag is always at position 1 of a tagged union's path, so position identifies it and exactly one leading segment is dropped. A collision cannot recur however the field names change, which is stronger than the comment you suggested. Two tests added: one for a field named like a slide type keeping its name, one for a nested path ( Re-ran locally: |
1 participant
Two fixes, both found by reviewing the deployed instance after #194 shipped. They are unrelated in code but came from the same look, so they are one PR with a commit each.
1. The Overview did not count hand-written templates
The deployment has the shape that exposes this: one Word template declared in
config/docx_templates.yaml, none managed by the UI. Three pages then disagreed about the same fact:smlouva_o_poskytovani_sluzeb— Live —master YAML✅create_word_document— the live tool absent ❌section_facts()built its counts and its tool list fromstore.list_specs()— the managed half only.template_table()walks the managed specs andunmanaged_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.
ToolFact.originkeeps 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 used to say master templates are shown read-only elsewhere, which read as "and are not counted here"; it now says they are counted.On the tests: they register the template the way
main.pydoes at startup. A fixture that only writes the YAML has a template the registry has never heard of, solive 0is correct there and the test passes against the bug — which is what my first draft did, and why it's worth calling out.2. A rejected slide field now says what that type does take
The same review turned up a real failed call on the server:
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 assumed what most people assume: that every slide takes a title, a subtitle and a body. It put
bodyon the title slide andsubtitleon the content slides.The published schema was not the gap. It already names the types per field —
subtitlerenders as[title] Subtitle: author, tagline, date… [closing] Line under the closing title.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: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. Onlyextra_forbiddenis rewritten — every other error keeps pydantic's own wording, which already says something useful, and there is a test pinning that.The tool description gains one paragraph on the same point, including that the call fails whole — worth knowing before writing eight slides — and
docs/powerpoint-slides.mda 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 isAnycarrying a hand-built schema, so nothing validates earlier. These strings are the tool's error message, not developer diagnostics.Testing
ruff check .clean. 2459 passed, 12 failed — the same pre-existing set verified onmasterby stashing (7test_pptx_text_metricsfont metrics, 3test_admin_source_filesWindows symlink/filename, 1test_admin_pptxencoding, 1test_admin_log_viewcapture-level).8 new tests. The three admin ones fail without the fix and pass with it (checked by stashing only the two source files). The pptx ones pin the exact production payload.
Docs updated in the same change:
docs/development/admin-ui.md,docs/development/tools/powerpoint.md,docs/powerpoint-slides.md, and thecreate_powerpoint_presentationdescription inmain.py.🤖 Generated with Claude Code