Modernize MCP server with structured output and 2025 protocol features

[jamiew]

consulted the latest MCP spec and pulled in the worthwhile 2025-06-18 features we were missing. the big one is structured tool output — every tool now returns a typed pydantic model so clients get a real outputSchema + machine-readable results (with a text fallback for older clients). also added the lighter-weight niceties: titles, resource templates, completions, and progress.

• all 19 tools return typed models → FastMCP advertises outputSchema and emits structured content, no more hand-built json strings
• human-friendly titles on every tool/resource/prompt
• new parameterized resource templates: accounts://{account_id}/holdings and accounts://{account_id}/history
• argument completions for the prompt category arg and template account_id, backed by live monarch data (best-effort, never raises)
• progress reporting via injected Context on the two batch tools (complete overview, spending patterns)
• fixed get_account_holdings to pass the required account_id — the underlying library always required it, so the no-arg version was a latent bug
• tests grew 180 → 202; deliberately did NOT add Sampling/Roots/elicitation since the draft spec deprecates them (we already log to stderr, the recommended path)

to test:

• uv run python scripts/ci.py → ruff + format + mypy + pytest all green
• uv run python server.py starts clean (logs to stderr, stdout stays empty)

[claude]

Claude finished @jamiew's task in 2m 58s —— View job

---

Code Review — mcp-upgrade PR #11

Overall: Approve with minor suggestions. This is a well-scoped, well-tested modernization. The structured output models are cleanly designed, the new features (titles, resource templates, completions, progress) follow the 2025-06-18 spec faithfully, and the get_account_holdings bug fix is correct. A few things worth addressing before merge.

View full review

---

What's good

• extract_list() neatly handles the Monarch API's inconsistent response shapes (bare list vs {"accounts": [...]} wrapped dict) across all tools — much cleaner than the ad-hoc unwrapping that was there before.
• MMModel(extra="allow") is the right call for passthrough API shapes that evolve independently of this wrapper. JsonValue (recursive JSON, not Any) keeps type safety without over-specifying the Monarch schema.
• Best-effort completions (_category_name_completions, _account_id_completions) correctly swallow all exceptions, which is exactly what the MCP spec requires for completion handlers.
• run() entry point fix (server:run instead of server:main) is an important correctness fix for the PyPI-published package — main is a coroutine and calling it as a console script entry point would have silently done nothing on many launchers.
• Test coverage for the new features is solid: output schema presence, structured content round-trip, resource templates, completions, progress reporting, and the optional-ctx path.

---

Issues

1. BudgetsResult.budgets double-nesting (confusing naming)

get_budgets returns BudgetsResult(budgets=budgets) where budgets is the raw API dict {"budgets": [...]}. That means callers must write result.budgets["budgets"][0] — the field named budgets contains a dict whose key is also budgets. The updated test in test_tool_coverage.py line 106 makes this visible:

result = await server.get_budgets()
assert result.budgets["budgets"][0]["category_id"] == "cat_1"  # ← double nesting

Every other tool that wraps a list (accounts, categories) unwraps it with extract_list() first. Budget should too:

# server.py, get_budgets tool (~line 1514)
budget_list = extract_list(convert_dates_to_strings(budgets), "budgets")
return BudgetsResult(budgets=budget_list)
# caller: result.budgets[0]["category_id"]  ← clean

Fix this →

---

2. Breaking key renames not surfaced in the PR description

_batch_metadata → batch_metadata (in get_complete_financial_overview) and _metadata → metadata (in analyze_spending_patterns) are necessary for Pydantic (which treats _ prefixed names as private), but these are quiet breaking changes for any client that was parsing the old JSON keys. The CHANGELOG has a bullet for the overall PR but doesn't call this out. Worth a specific note.

---

3. ctx: Context | None not in the docstring for batch tools

get_complete_financial_overview (line 2103) and analyze_spending_patterns (line 2221) gained a ctx parameter, but the docstring Args: section wasn't updated to document it. FastMCP typically auto-injects Context parameters and strips them from the advertised input schema, but if something changes in the library, this gap makes the parameter invisible to readers.

---

4. Progress reporting in get_complete_financial_overview jumps 0 → 5

All five API calls go into a single asyncio.gather(), so progress necessarily reports (0, 5, ...) at start and (5, 5, ...) at the end with no intermediate steps. This is honest, but the total=5 value is misleading since the client never sees steps 1–4. Using (0, 1, ...) / (1, 1, ...) would be more accurate and match the analyze_spending_patterns style (which uses total=2).

---

5. _account_id_completions returns opaque IDs

The completion for the account_id template argument returns bare IDs ("acc_123") rather than human-readable names. This makes autocompletion effectively useless for anyone who doesn't already know their account IDs. Consider returning "Checking (acc_123)" as the completion value, or at minimum filter/sort by account name rather than ID substring. This is a UX suggestion, not a bug.

---

6. test_structured_output.py tests internal FastMCP APIs

test_call_tool_returns_text_and_structured_content (line 48) calls mgr.call_tool(..., convert_result=True) and destructures the (content, structured) return. This is reaching into FastMCP's private tool manager implementation. If FastMCP changes its internal call_tool signature (which it has historically), this test silently breaks. The test intent (verify structured content round-trips) is valuable — just worth flagging the fragility.

---

Minor notes

• FinancialOverview.period: str has no default but all its sibling fields do. It's always set via results["period"] = period before model_validate, so this is safe — but a future code path that forgets to set it will produce a harder-to-debug ValidationError rather than None. Worth adding period: str = "" or keeping it required and confirming the comment in the code.

• SearchResult.model_validate({"search_metadata": metadata, "transactions": transactions}) where metadata is already a SearchMetadata instance works in Pydantic v2 (it accepts model instances during dict validation), but it's non-obvious. The comment (# model_validate (not the constructor) sidesteps mypy's list invariance) is correct and sufficient.

• The test_tool_count_is_nineteen test (line 25) is a good guard — but it will require a one-line update every time a tool is added. That's intentional and fine.

---

• Gather context and understand request
• Read server.py changes
• Read new test files
• Write and post full review