Implement new MCP protocol features

[jamiew]

a big modernization pass plus new mcp protocol features for the spotify server. trims the codebase to the fastmcp essentials, types the spotify boundary, bumps deps, then uses mcp 1.27.1 features the server wasn't touching. all backward compatible.

• annotate all 15 tools with readonly/destructive/idempotent hints + human titles + a spotify icon so clients can reason about safety
• give every tool a real output schema via pydantic result models instead of bare dict[str, Any] returns
• stream progress + log notifications from get_playlist_tracks via Context for large playlists
• confirm destructive playlist removals with ctx.elicit, falling through gracefully when the client doesn't support elicitation
• keep the SpotifyMCPError code + suggestion in the surfaced message instead of flattening it
• add spotify://track|playlist|artist|album/{id} resource templates
• strip dead code from the fastmcp rewrite, drop the unused audio-features/recommendations tools, and type the spotify boundary with TypedDicts
• bump deps to latest — mcp 1.27.1, spotipy 2.26, pytest 9, mypy 2.1, ruff 0.15
• tests now hit 99% on the server module (multi-batch pagination, elicitation, resource templates, error mappings)

to test:

• uv run pytest → 106 pass
• uv run mypy src/ → clean

[claude]

Code Review

PR: Implement new MCP protocol features
Author: @jamiew

---

Overview

This is a solid modernization pass that upgrades the server to MCP 1.27.1 features. The core changes — typed Pydantic output models, tool annotations, progress notifications, elicitation for destructive ops, and new resource templates — are well-motivated and cleanly executed. The diff is large but the structure is consistent throughout.

---

What's Working Well

• Elicitation safety model is correct. The three-way logic (unsupported client → skip prompt; supported client + error → propagate, don't delete; supported client + decline → cancel) handles all the cases that matter. The test suite covers each branch explicitly, including the subtle "accept with confirm=False" case.
• current_playback() fix is a real improvement — the old current_user_playing_track() didn't return device/volume/shuffle/repeat, so tools were silently returning null for those fields.
• log_tool_execution async/sync branching is clean. Using asyncio.iscoroutinefunction to select the right wrapper avoids any timing skew around coroutine creation.
• Null guard in search_tracks (if item before parsing) is defensive and correct — Spotify does occasionally return null items for removed content.
• snapshot_id surfacing in add_tracks_to_playlist and remove_tracks_from_playlist is a useful improvement for callers who want to verify playlist state.
• Pydantic output model refactor is consistent — no dict[str, Any] returns remain, and the models are concise and well-named.

---

Issues

🔴 Critical: Async tests may not be executing

The async test methods (e.g. TestRemoveTracksFromPlaylist.test_converts_ids_and_uris, all async methods in TestGetPlaylistTracks) don't have @pytest.mark.asyncio decorators:

class TestRemoveTracksFromPlaylist:
    async def test_converts_ids_and_uris(self, mock_spotify_api):
        ...

Without either @pytest.mark.asyncio on each method or asyncio_mode = "auto" in pyproject.toml (under [tool.pytest.ini_options]), pytest-asyncio won't await these coroutines — it collects them as passed tests without ever running the assertions inside. The "106 pass" count can be entirely correct while none of these tests actually exercise the code.

pyproject.toml isn't in this diff, so please confirm asyncio_mode = "auto" is already set there. If it isn't, add:

[tool.pytest.ini_options]
asyncio_mode = "auto"

or decorate every async test with @pytest.mark.asyncio. This is the most important thing to verify before merge.

---

🟡 Minor: Track.added_at mutated after construction

In get_saved_tracks:

track = parse_track(item["track"])
track.added_at = item.get("added_at")
tracks.append(track)

Mutating a Pydantic model's field post-construction works (V2 models are mutable by default), but parse_track could accept an optional added_at parameter and pass it to the constructor directly — that would keep construction immutable and avoid the implicit reliance on the model not being frozen.

---

🟡 Minor: SearchResults.limit typed as int, not int | None

class SearchResults(BaseModel):
    limit: int

But it's populated with result_section.get("limit", limit). If result_section is an empty dict and limit itself is somehow None, Pydantic will raise a validation error at runtime. PlaylistTracks.limit is already typed int | None, so the pattern is established — consider aligning SearchResults.limit for consistency.

---

🟡 Minor: Resource handlers catch bare Exception

@mcp.resource("spotify://track/{track_id}", icons=[SPOTIFY_ICON])
def track_resource(track_id: str) -> str:
    try:
        return parse_track(spotify_client.track(track_id)).model_dump_json()
    except Exception as e:
        return json.dumps({"error": str(e)})

This is consistent with the existing current_user and current_playback_resource handlers, so it's not a regression. But it does mean AttributeError, TypeError, and other programming mistakes surface as {"error": "..."} instead of crashing visibly. At minimum, it's worth logging e at logger.exception(...) level before returning the JSON error so server-side errors aren't completely silent.

---

🔵 Nit: PR description says "15 tools", CLAUDE.md says "11 tools"

The PR body mentions "annotate all 15 tools" but the codebase (and the updated CLAUDE.md) consistently says 11. Minor, but worth correcting in the PR description.

---

Summary

The code is well-structured and the protocol features are implemented correctly. The one thing that must be verified before merge is whether the async tests are actually running — if asyncio_mode = "auto" isn't already in pyproject.toml, those tests are silently no-ops. Everything else is minor polish.