Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions admin/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -320,23 +328,22 @@ 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:
extra.append(("Registered designs", len(live_names)))

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),
)
Expand Down
24 changes: 19 additions & 5 deletions admin/views/sections.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
13 changes: 13 additions & 0 deletions docs/development/admin-ui.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
21 changes: 21 additions & 0 deletions docs/development/tools/powerpoint.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions docs/powerpoint-slides.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions main.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down
73 changes: 67 additions & 6 deletions pptx_tools/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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", ())]
Expand All @@ -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
Expand Down
89 changes: 89 additions & 0 deletions tests/test_admin_master_templates.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'<a href="/admin/word" class="tile">.*?</a>',
client.get("/admin/").text, re.S)
assert tile, "no Word tile on the dashboard"
assert "<b>1</b><span>live</span>" 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'<div class="num">(\d+)</div>\s*<div class="lbl">Live Word tools</div>',
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'<div class="num warn-text">1</div>\s*'
r'<div class="lbl">Disabled</div>', overview), (
"a disabled master-YAML template must show as disabled, not missing")
Loading
Loading