ldraney/basketball-api

Fork 0

feat: add MCP endpoint with get_schedule tool #455

Merged

forgejo_admin merged 1 commit from 454-mcp-hello-world into main

2026-04-12 19:35:53 +00:00

forgejo_admin commented

2026-04-12 19:02:44 +00:00

Contributor

Summary

Adds an MCP server mounted at /mcp on the FastAPI app using the streamable HTTP transport protocol. Exposes a get_schedule tool that returns practices and events from the database, enabling AI assistant integration (e.g., claude.ai Connectors).

Changes

pyproject.toml — added mcp>=1.0 dependency
src/basketball_api/mcp_server.py — new file: MCP server definition with get_schedule tool and create_mcp_app() factory for session manager lifecycle
src/basketball_api/main.py — imports create_mcp_app, initializes MCP session manager in lifespan, mounts ASGI app at /mcp
tests/test_mcp.py — 8 tests: server init, MCP protocol (initialize, tools/list, tools/call with empty and seeded data), existing endpoints unaffected

Test Plan

pytest tests/test_mcp.py -v — all 8 tests pass
pytest tests/test_mcp.py tests/test_health.py tests/test_public.py -v — 33 tests pass, existing endpoints unaffected
Manual: connect from claude.ai Connectors to https://basketball-api.tail5b443a.ts.net/mcp/ and ask "What's the schedule?"

Review Checklist

mcp package added to pyproject.toml dependencies
MCP server mounts at /mcp on the FastAPI app
get_schedule tool returns upcoming schedule entries from the database
MCP endpoint responds to the streamable HTTP transport protocol
Existing REST endpoints are unaffected
Health check still passes
Tests pass (pytest tests/ -k test_mcp)
No unrelated changes
ruff check and ruff format clean

Parent story: story-westside-basketball-ai-assistant (stage 1 of 4)

Closes #454

## Summary Adds an MCP server mounted at `/mcp` on the FastAPI app using the streamable HTTP transport protocol. Exposes a `get_schedule` tool that returns practices and events from the database, enabling AI assistant integration (e.g., claude.ai Connectors). ## Changes - `pyproject.toml` — added `mcp>=1.0` dependency - `src/basketball_api/mcp_server.py` — new file: MCP server definition with `get_schedule` tool and `create_mcp_app()` factory for session manager lifecycle - `src/basketball_api/main.py` — imports `create_mcp_app`, initializes MCP session manager in lifespan, mounts ASGI app at `/mcp` - `tests/test_mcp.py` — 8 tests: server init, MCP protocol (initialize, tools/list, tools/call with empty and seeded data), existing endpoints unaffected ## Test Plan - `pytest tests/test_mcp.py -v` — all 8 tests pass - `pytest tests/test_mcp.py tests/test_health.py tests/test_public.py -v` — 33 tests pass, existing endpoints unaffected - Manual: connect from claude.ai Connectors to `https://basketball-api.tail5b443a.ts.net/mcp/` and ask "What's the schedule?" ## Review Checklist - [x] `mcp` package added to `pyproject.toml` dependencies - [x] MCP server mounts at `/mcp` on the FastAPI app - [x] `get_schedule` tool returns upcoming schedule entries from the database - [x] MCP endpoint responds to the streamable HTTP transport protocol - [x] Existing REST endpoints are unaffected - [x] Health check still passes - [x] Tests pass (`pytest tests/ -k test_mcp`) - [x] No unrelated changes - [x] `ruff check` and `ruff format` clean ## Related Notes - Parent story: `story-westside-basketball-ai-assistant` (stage 1 of 4) ## Related Closes #454

forgejo_admin added 1 commit

2026-04-12 19:02:44 +00:00

feat: add MCP endpoint with get_schedule tool (#454 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

4929584ea5

Mount an MCP server at /mcp on the FastAPI app using the streamable HTTP
transport protocol. Exposes a get_schedule tool that returns practices
and events from the database for AI assistant integration.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-04-12 19:03:34 +00:00

Author

Contributor

QA Review -- PR #455

Scope Check

4 files changed (1 new module, 1 new test file, 2 modified) -- matches issue #454 file targets
No unrelated changes detected
Branch naming 454-mcp-hello-world follows convention

Positive

Clean create_mcp_app() factory pattern correctly handles the StreamableHTTPSessionManager.run() single-use constraint by creating fresh instances per lifespan cycle
DB session management in the MCP tool uses manual SessionLocal() with try/finally/close -- correct since MCP tools run outside FastAPI dependency injection
stateless_http=True and json_response=True are appropriate for hello-world scope
8 tests cover init, protocol (initialize, tools/list, tools/call empty + seeded), and existing endpoint regression
Test fixture uses with TestClient(app) as client to trigger lifespan -- required for MCP session manager initialization

Nits (non-blocking)

test_mcp_tool_registered is a duplicate of test_mcp_server_imports_without_error -- both just assert mcp is not None. Could verify the tool is actually in the registry.
_mcp_request posts to /mcp which 307-redirects to /mcp/. Posting to /mcp/ directly would skip the redirect hop.
mcp._mcp_server is private attribute access in create_mcp_app(). Fragile if SDK internals change, but no public API exists for this. Acceptable for hello-world.

Acceptance Criteria

mcp package added to pyproject.toml
MCP server mounts at /mcp
get_schedule tool returns schedule entries from DB
Streamable HTTP transport protocol
Existing REST endpoints unaffected
Health check passes
ruff clean

VERDICT: APPROVED

All acceptance criteria met. Nits are non-blocking and can be addressed in follow-up tickets.

## QA Review -- PR #455 ### Scope Check - 4 files changed (1 new module, 1 new test file, 2 modified) -- matches issue #454 file targets - No unrelated changes detected - Branch naming `454-mcp-hello-world` follows convention ### Positive - Clean `create_mcp_app()` factory pattern correctly handles the `StreamableHTTPSessionManager.run()` single-use constraint by creating fresh instances per lifespan cycle - DB session management in the MCP tool uses manual `SessionLocal()` with try/finally/close -- correct since MCP tools run outside FastAPI dependency injection - `stateless_http=True` and `json_response=True` are appropriate for hello-world scope - 8 tests cover init, protocol (initialize, tools/list, tools/call empty + seeded), and existing endpoint regression - Test fixture uses `with TestClient(app) as client` to trigger lifespan -- required for MCP session manager initialization ### Nits (non-blocking) 1. `test_mcp_tool_registered` is a duplicate of `test_mcp_server_imports_without_error` -- both just assert `mcp is not None`. Could verify the tool is actually in the registry. 2. `_mcp_request` posts to `/mcp` which 307-redirects to `/mcp/`. Posting to `/mcp/` directly would skip the redirect hop. 3. `mcp._mcp_server` is private attribute access in `create_mcp_app()`. Fragile if SDK internals change, but no public API exists for this. Acceptable for hello-world. ### Acceptance Criteria - [x] `mcp` package added to `pyproject.toml` - [x] MCP server mounts at `/mcp` - [x] `get_schedule` tool returns schedule entries from DB - [x] Streamable HTTP transport protocol - [x] Existing REST endpoints unaffected - [x] Health check passes - [x] ruff clean **VERDICT: APPROVED** All acceptance criteria met. Nits are non-blocking and can be addressed in follow-up tickets.

forgejo_admin commented

2026-04-12 19:10:49 +00:00

Author

Contributor

PR #455 Review

DOMAIN REVIEW

Stack: Python / FastAPI / SQLAlchemy / MCP SDK

Architecture -- double create_mcp_app() call
The module-level call _, mcp_asgi_app = create_mcp_app() creates a session manager that is immediately discarded (never started via sm.run()). Then the lifespan creates a second session manager and monkey-patches it onto the first ASGI app via mcp_asgi_app.session_manager = sm. This works but is fragile -- it relies on StreamableHTTPASGIApp allowing post-construction session manager replacement. The comment explains the rationale well, but this is a pattern that could break on MCP SDK upgrades.

Session management -- SessionLocal() bypass
mcp_server.py:get_schedule() calls SessionLocal() directly instead of using get_db(). Every other endpoint in basketball-api uses FastAPI's Depends(get_db) pattern. The MCP tool operates outside FastAPI's dependency injection, so Depends cannot be used directly, but this creates:

A DRY concern: two independent session lifecycle patterns in the same app
A testability concern: tests that override get_db do not affect the MCP tool's DB access

In practice, tests pass because both SessionLocal() and the test engine connect to the same basketball_test database. The data seeded by the db fixture is committed (db.commit()) before the MCP tool runs, so SessionLocal() sees it. This is correct but coincidental -- if tests ever switch to rollback-based isolation, the MCP tool will silently return stale data.

Recommendation: Accept for now since the data is read-only and publicly available. File a follow-up ticket to refactor the MCP tool to accept a session factory or use a shared helper, aligning with the rest of the codebase.

Auth -- no authentication on /mcp
The /mcp endpoint has no authentication. This is acceptable because:

The data exposed (get_schedule) is identical to /public/schedule which is also unauthenticated
No PII or admin data is returned
The stateless_http=True and stateless=True settings are correct for a public tool

If future MCP tools expose protected data, auth must be added at that point. This should be documented as a constraint.

MCP protocol correctness

stateless_http=True on FastMCP and stateless=True on StreamableHTTPSessionManager are consistent
json_response=True is appropriate for claude.ai Connector integration
The tool returns plain text (not JSON), which is fine for LLM consumption

BLOCKERS

None. The new functionality has 8 tests covering: server init, MCP protocol handshake, tool listing, empty schedule, seeded schedule, and regression checks on existing endpoints.

NITS

SessionLocal() direct usage (file: src/basketball_api/mcp_server.py, line ~63): As discussed above, this diverges from the get_db() pattern used everywhere else. Not a blocker for read-only public data, but should be a follow-up ticket.
Hardcoded tenant_id == 1 (file: src/basketball_api/mcp_server.py, lines ~71, ~78): This matches the existing pattern in routes/public.py:192-210 (also hardcoded to tenant 1), so it is consistent. Just noting the inherited tech debt.
_DAY_NAMES dict vs list: A tuple ("Monday", "Tuesday", ...) indexed by int would be simpler than a dict keyed 0-6, but this is purely cosmetic.
test_mcp_tool_registered is a no-op: The test body only asserts mcp is not None, which the previous test already covers. It does not actually verify get_schedule is registered (e.g., by inspecting mcp._mcp_server internals). Low value.
Missing joinedload on public endpoint: The MCP tool adds joinedload(PracticeSchedule.team) which the REST /public/schedule endpoint does not use. This is actually better (avoids N+1), but creates a minor inconsistency between the two code paths returning the same data.

SOP COMPLIANCE

Branch named after issue: 454-mcp-hello-world matches issue #454
PR body follows template: Summary, Changes, Test Plan, Related all present
Related references story slug: story-westside-basketball-ai-assistant referenced (no plan slug -- parent story is sufficient)
No secrets committed
No unrelated changes (4 files, all MCP-related)
Tests exist and cover happy path + empty state + protocol handshake + regression

PROCESS OBSERVATIONS

Clean first MCP integration. The scope is appropriately narrow (one read-only tool).
The SessionLocal() pattern should be tracked as tech debt before more tools are added. A second tool using direct sessions doubles the divergence.
Future tools that expose player/parent data MUST add auth. Consider documenting this constraint in the module docstring or a follow-up ticket.

VERDICT: APPROVED

## PR #455 Review ### DOMAIN REVIEW **Stack**: Python / FastAPI / SQLAlchemy / MCP SDK **Architecture -- double `create_mcp_app()` call** The module-level call `_, mcp_asgi_app = create_mcp_app()` creates a session manager that is immediately discarded (never started via `sm.run()`). Then the lifespan creates a *second* session manager and monkey-patches it onto the first ASGI app via `mcp_asgi_app.session_manager = sm`. This works but is fragile -- it relies on `StreamableHTTPASGIApp` allowing post-construction session manager replacement. The comment explains the rationale well, but this is a pattern that could break on MCP SDK upgrades. **Session management -- `SessionLocal()` bypass** `mcp_server.py:get_schedule()` calls `SessionLocal()` directly instead of using `get_db()`. Every other endpoint in basketball-api uses FastAPI's `Depends(get_db)` pattern. The MCP tool operates outside FastAPI's dependency injection, so `Depends` cannot be used directly, but this creates: - A DRY concern: two independent session lifecycle patterns in the same app - A testability concern: tests that override `get_db` do not affect the MCP tool's DB access In practice, tests pass because both `SessionLocal()` and the test engine connect to the same `basketball_test` database. The data seeded by the `db` fixture is committed (`db.commit()`) before the MCP tool runs, so `SessionLocal()` sees it. This is correct but coincidental -- if tests ever switch to rollback-based isolation, the MCP tool will silently return stale data. **Recommendation**: Accept for now since the data is read-only and publicly available. File a follow-up ticket to refactor the MCP tool to accept a session factory or use a shared helper, aligning with the rest of the codebase. **Auth -- no authentication on `/mcp`** The `/mcp` endpoint has no authentication. This is acceptable because: - The data exposed (`get_schedule`) is identical to `/public/schedule` which is also unauthenticated - No PII or admin data is returned - The `stateless_http=True` and `stateless=True` settings are correct for a public tool If future MCP tools expose protected data, auth must be added at that point. This should be documented as a constraint. **MCP protocol correctness** - `stateless_http=True` on `FastMCP` and `stateless=True` on `StreamableHTTPSessionManager` are consistent - `json_response=True` is appropriate for claude.ai Connector integration - The tool returns plain text (not JSON), which is fine for LLM consumption ### BLOCKERS None. The new functionality has 8 tests covering: server init, MCP protocol handshake, tool listing, empty schedule, seeded schedule, and regression checks on existing endpoints. ### NITS 1. **`SessionLocal()` direct usage** (file: `src/basketball_api/mcp_server.py`, line ~63): As discussed above, this diverges from the `get_db()` pattern used everywhere else. Not a blocker for read-only public data, but should be a follow-up ticket. 2. **Hardcoded `tenant_id == 1`** (file: `src/basketball_api/mcp_server.py`, lines ~71, ~78): This matches the existing pattern in `routes/public.py:192-210` (also hardcoded to tenant 1), so it is consistent. Just noting the inherited tech debt. 3. **`_DAY_NAMES` dict vs list**: A tuple `("Monday", "Tuesday", ...)` indexed by int would be simpler than a dict keyed 0-6, but this is purely cosmetic. 4. **`test_mcp_tool_registered` is a no-op**: The test body only asserts `mcp is not None`, which the previous test already covers. It does not actually verify `get_schedule` is registered (e.g., by inspecting `mcp._mcp_server` internals). Low value. 5. **Missing `joinedload` on public endpoint**: The MCP tool adds `joinedload(PracticeSchedule.team)` which the REST `/public/schedule` endpoint does not use. This is actually better (avoids N+1), but creates a minor inconsistency between the two code paths returning the same data. ### SOP COMPLIANCE - [x] Branch named after issue: `454-mcp-hello-world` matches issue #454 - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [x] Related references story slug: `story-westside-basketball-ai-assistant` referenced (no plan slug -- parent story is sufficient) - [x] No secrets committed - [x] No unrelated changes (4 files, all MCP-related) - [x] Tests exist and cover happy path + empty state + protocol handshake + regression ### PROCESS OBSERVATIONS - Clean first MCP integration. The scope is appropriately narrow (one read-only tool). - The `SessionLocal()` pattern should be tracked as tech debt before more tools are added. A second tool using direct sessions doubles the divergence. - Future tools that expose player/parent data MUST add auth. Consider documenting this constraint in the module docstring or a follow-up ticket. ### VERDICT: APPROVED