Fix router prefix ignored when set via include_router (#204)

[wakqasahmed]

Fixes #204

Root cause

When an APIRouter is created without a prefix on the constructor and is instead
given one at inclusion time via app.include_router(router, prefix="/x"), the prefix is
only reflected on self.fastapi's route table — the router object's own .prefix
attribute stays empty (""). mount_http()/mount_sse() used router.prefix directly
to compute where the MCP endpoint should live and how to re-include the router, so the
MCP endpoint ended up mounted at the app root instead of under the router's actual path.

The issue's second and third examples (passing an explicit mount_path="/other/route" /
"/other/route/mcp") are, per the issue text itself, not the intended usage of the
mount_path parameter — that parameter controls where the MCP endpoint sits relative to
the router, not the router's own external prefix — so this PR does not change that
behavior.

Update after review

A cold-start review caught that the original version of this fix (matching routes by
endpoint is router_endpoint identity and diffing paths against app.routes) had two
blocking correctness problems, both confirmed empirically:

1. Silent no-op on FastAPI >= 0.137. As of that version app.include_router() no
   longer flattens sub-routes into app.routes — it appends one opaque _IncludedRouter
   object with no .path/.endpoint. The old resolver found nothing and silently
   returned "", restoring the original bug. This repo declares fastapi>=0.100.0, so
   this was within the supported range, and the original test only passed because
   uv.lock pins fastapi 0.115.11 — it would not have caught the regression once the
   lockfile is bumped.
2. The issue's literal repro (a zero-route router used purely as a mount point) was
   still broken — no routes meant nothing to diff, so the resolver returned "" and
   the MCP endpoint still landed at /mcp. The original test only passed because it
   added a /ping route the issue's repro never had.
3. A router whose endpoint function is reused on a second router could resolve to the
   other router's prefix — silently mounting the MCP endpoint under the wrong prefix,
   which is worse than the original bug.

What changed

• _resolve_router_include_prefix() now tries an exact, identity-based resolution first:
  on FastAPI >= 0.137, app.routes contains one _IncludedRouter per include_router()
  call, carrying .original_router (the exact router instance) and
  .include_context.prefix (the prefix it was included under). Matching on
  original_router is router is exact — it works even with zero routes, and cannot be
  confused by two routers sharing an endpoint function.
• Falls back to the previous path-diffing approach on FastAPI < 0.137 (where that
  metadata doesn't exist), but hardened: it now requires every route on the router to
  agree on the same candidate prefix (matching endpoint identity and HTTP methods)
  before accepting it. On disagreement (or zero matches), it warns and falls back to the
  safe empty prefix instead of guessing.
• Same treatment for a router include_router()-ed more than once under different
  prefixes: warns and falls back to the safe empty prefix rather than silently picking
  the first inclusion.
• mount_http()/mount_sse() now log the actually-resolved mount path instead of the
  pre-resolution relative mount_path.
• Documented that mount_http()/mount_sse() must be called after the router has
  already been include_router()-ed — resolution happens once at mount time and is not
  re-evaluated if routes/routers change afterwards.

Known, documented limitations (not fixed in this PR)

• On FastAPI < 0.137 only, a router with zero routes of its own still cannot be
  resolved — there's no metadata and nothing to diff against, so it safely falls back to
  the app root rather than guessing. test_mount_empty_router_with_include_router_prefix
  documents this explicitly and branches on the fastapi version.
• Re-including a router (the existing # HACK ... TODO in mount_http/mount_sse)
  re-registers its routes at the same paths, which can produce duplicate-operation-ID
  warnings from FastAPI's OpenAPI generation. Pre-existing, not introduced by this PR.
• Resolution happens once at mount time; mounting before include_router() still lands
  at the app root (now documented in the docstrings, not silently fixed).

Tests

Added to tests/test_basic_functionality.py:

• test_mount_router_with_include_router_prefix (existing test, now asserts via a real
  request through TestClient instead of walking app.routes, since FastAPI's internal
  route-table shape is not a stable contract across versions).
• test_mount_empty_router_with_include_router_prefix — the issue's literal zero-route
  repro; asserts full resolution on fastapi >= 0.137, and the documented safe fallback on
  fastapi < 0.137.
• test_resolve_prefix_with_endpoint_shared_across_routers — two routers registering the
  same endpoint function; asserts the resolver never returns the other router's prefix.
• test_resolve_prefix_with_router_included_at_multiple_prefixes — a router
  include_router()-ed under two different prefixes; asserts the safe empty-prefix
  fallback rather than "first one wins".

Verified against both fastapi==0.115.11 (this repo's uv.lock pin) and
fastapi==0.141.1 (current latest as of writing) — full test suite green on both:

$ pip install fastapi==0.115.11 && pytest tests/ -q --no-cov
79 passed
$ pip install fastapi==0.141.1  && pytest tests/ -q --no-cov
79 passed

Also ran ruff check . and mypy . with the exact versions pinned in uv.lock
(ruff 0.9.10, mypy 1.15.0) — clean aside from one pre-existing, unrelated mypy error in
fastapi_mcp/types.py that reproduces identically on unmodified main.

tests/test_http_real_transport.py has a handful of RuntimeError: Event loop is closed
teardown errors; this reproduces identically on unmodified main in this sandbox and is
unrelated to this change.

Honest scope assessment

Fixes #204 is now true across the fastapi version range this repo declares
(fastapi>=0.100.0), including the issue's literal zero-route repro, on fastapi >=
0.137. On fastapi < 0.137, the zero-route case remains a documented limitation (falls
back to the app root, same as before this PR existed) rather than a silent wrong-prefix
mount — there is no recoverable information in app.routes on those versions to do
better, short of tracking include_router() calls ourselves at a higher level, which
felt out of scope for this fix.

[wakqasahmed]

Cold-start review — request changes

Verified locally against a fresh clone of main + this PR head. The core idea (recover the include_router(prefix=...) prefix from the app's route table) is sound and the happy path works, and I confirmed the new test genuinely fails on the parent commit e5cad13 with exactly the issue's symptom (/mcp at app root) and passes on this head — so it is a real regression test, not a tautology.

However there are two blocking correctness problems and several smaller ones. Details are in the inline comments; summary:

Blocking

1. The fix is a silent no-op on FastAPI >= 0.137.0. As of 0.137.0 app.include_router() no longer flattens sub-routes into app.routes; it appends a single _IncludedRouter object with no .path / .endpoint. _resolve_router_include_prefix() therefore finds nothing and returns "", and the bug is back. This project declares fastapi>=0.100.0, so this is inside the supported range. The new test also fails on this PR head under 0.141.1 — it only passes because uv.lock pins fastapi 0.115.11, so CI will never catch it. Bisected: 0.136.0 flat, 0.137.0 _IncludedRouter.

2. The issue's literal repro is still not fixed. In #204 other_router = APIRouter() has zero routes — it is used purely as a mount point. With no routes there is nothing to match against, so the method returns "" and mcp.mount(other_router) still lands at the app root. The new test only passes because it adds a /ping route the issue never had. The empty-router path is handled gracefully (no crash — I verified), but it is silent, and it is the exact case the reporter filed.

Non-blocking but worth fixing

3. Endpoint-identity matching picks the wrong prefix when the same endpoint function is registered on more than one router (a legitimate FastAPI pattern) — first match in app.routes wins. Reproduced.
4. Same for a router included twice under different prefixes — first inclusion wins, with no way for the caller to select the intended one and no warning.
5. Resolution happens once at mount time and is never re-evaluated, so mount_http() before include_router() still mounts at the root (and then leaks a second MCP route at the later prefix).
6. logger.info(f"MCP HTTP/SSE server listening at {mount_path}") still logs /mcp, not the resolved /other/route/mcp — now actively misleading.

Scope check (passes): the PR does what it claims and no more — the mount_path semantics from the issue's 2nd/3rd examples (explicitly called "not the expected usage" by the reporter) are untouched. Good restraint. Also verified there is no double-counting when a router has both a constructor prefix and an include_router prefix (/outer/inner/mcp — correct), and that a router route at / resolves correctly.

[wakqasahmed]

Blocking (1/2): this whole method returns "" on FastAPI >= 0.137.0.

Since FastAPI 0.137.0, app.include_router() no longer copies the sub-routes into app.routes — it appends one _IncludedRouter object that has neither .path nor .endpoint. Both getattr(app_route, "endpoint", None) and getattr(app_route, "path", "") come back None/"", nothing matches, and the method falls through to return "" — i.e. the pre-fix behaviour.

Reproduced on a clean checkout of this PR head:

fastapi 0.135.0 -> [('APIRoute', '/x/p')]
fastapi 0.136.0 -> [('APIRoute', '/x/p')]
fastapi 0.137.0 -> [('_IncludedRouter', None)]
fastapi 0.141.1 -> [('_IncludedRouter', None)]

and the new test on this branch:

$ pip install fastapi==0.141.1
$ pytest tests/test_basic_functionality.py -q
FAILED test_mount_router_with_include_router_prefix -
  assert '/other/route/mcp' in [..., '/error', None, None]
$ pip install fastapi==0.115.11   # the uv.lock pin
4 passed

pyproject.toml declares fastapi>=0.100.0, so 0.137+ is in the supported range; CI passes only because uv.lock pins 0.115.11. Please either walk _IncludedRouter (its nested router/prefix are reachable off the object) or, better, resolve the prefix from self.fastapi.routes after a app.router.routes-flattening step that handles both shapes — and add a test matrix entry (or at least a min/max fastapi tox/CI job) so this can't silently rot again.

[wakqasahmed]

Blocking (2/2): the issue's actual repro has an empty router, and it is still broken.

In #204 the reporter's router is:

other_router = APIRouter()          # zero routes
app.include_router(other_router, prefix="/other/route")
mcp.mount(other_router)

There are no routes to match, so this loop never executes and we return "". I verified the empty-router case does not crash (good — the graceful fallback works), but the outcome is that the MCP endpoint still lands at /mcp on the app root, which is precisely the bug being closed:

--- empty router used purely as a mount point ---
routes: ['/root', '/mcp']       # expected '/empty/mcp'

The new test passes only because it adds a /ping route that the issue never had. A router with no routes of its own that exists purely as a mount point is a normal pattern and is the literal filed case, so Fixes #204 currently over-claims.

Two things needed here:

• handle the zero-route case (on modern FastAPI the _IncludedRouter object carries the router identity and prefix, which solves this and finding (1) together);
• if it genuinely can't be resolved, logger.warning(...) rather than silently returning "" — a silent fallback to the buggy behaviour is the worst outcome for a user hitting this.

Also a small robustness nit while you're here: the fallback is documented as "the router hasn't been included into the app yet", but it also fires for empty routers, for Mount-only routers, and (per finding 1) for every modern FastAPI — the docstring should not imply a single cause.

[wakqasahmed]

Wrong prefix when the same endpoint function is reused across routers. Registering one handler on two routers is legitimate FastAPI; endpoint is router_endpoint cannot tell the two registrations apart, and the first hit in self.fastapi.routes wins. Reproduced on this branch:

async def ping(): ...
ra = APIRouter(); ra.add_api_route("/ping", ping, methods=["GET"])
rb = APIRouter(); rb.add_api_route("/ping", ping, methods=["GET"])
app.include_router(ra, prefix="/alpha")
app.include_router(rb, prefix="/beta")

mcp._resolve_router_include_prefix(rb)  # -> '/alpha'   (expected '/beta')
mcp.mount_http(rb)
# app paths: ['/alpha/ping', '/beta/ping', '/alpha/ping', '/alpha/mcp']

So rb gets its routes re-registered under /alpha and the MCP endpoint lands at /alpha/mcp. Silently mounting a user's MCP server under an unrelated router's prefix is worse than the original bug.

Cheap hardening that fixes this without changing the approach: don't return on the first hit. Collect the candidate prefix implied by every route in router.routes and only accept a prefix p for which all of the router's routes have a counterpart at p + route.path with the same endpoint and matching methods; if zero or more than one candidate survives, warn and fall back. In the example above only /beta satisfies all-routes agreement.

(The and app_path.endswith(router_path) guard doesn't help here — both candidates end with /ping. I checked the guard does correctly reject unrelated suffixes like /xping.)

[wakqasahmed]

Ambiguity when a router is included more than once, and no re-evaluation after mount time.

Multiple inclusions — unusual but legal, and the method silently picks whichever inclusion appears first in app.routes:

app.include_router(rc, prefix="/one")
app.include_router(rc, prefix="/two")
mcp._resolve_router_include_prefix(rc)  # -> '/one'
# after mount_http(rc): ['/one/x', '/two/x', '/one/x', '/one/mcp']

There is no correct answer here from mount_http()'s signature alone — but "first wins, silently" is the wrong default. Either logger.warning on multiple distinct candidate prefixes, or let the caller disambiguate explicitly.

Ordering — this is computed once, here, at mount time and never revisited. Mounting before including still mounts at the root, and then the later include_router re-registers the MCP route a second time:

mcp.mount_http(rf)                       # rf not yet included
app.include_router(rf, prefix="/late")
# app paths: ['/z', '/mcp', '/late/z', '/late/mcp']

Two MCP endpoints, one of them at the root. Worth at least documenting the ordering requirement in the mount_http/mount_sse docstrings ("the router must already be included into the app"), and ideally warning when the router isn't found in the app at all.

[wakqasahmed]

Nit, but now actively misleading: this logs MCP HTTP server listening at /mcp even when the endpoint was actually registered at /other/route/mcp. Since the point of the PR is that the effective path differs from mount_path, please log the resolved path (include_prefix + router.prefix + mount_path). Same at the mount_sse equivalent below.

[wakqasahmed]

Confirmed correct — no double-counting when a router has both a constructor prefix and an include_router prefix, because router.routes paths already contain router.prefix, so the diff yields only the include-time prefix and include_router(..., prefix=include_prefix) re-applies router.prefix itself:

rd = APIRouter(prefix="/inner"); app.include_router(rd, prefix="/outer")
resolved='/outer', router.prefix='/inner'  ->  '/outer/inner/mcp'   # correct

Also verified mount_sse's include_prefix + router.prefix + mount_path produces the right messages_path (/sse-pfx/sse/messages/).

One pre-existing wart this change makes more visible: re-including the router now lands its own routes on exactly the same paths as the original inclusion, so every route is registered twice at the same path and app.openapi() emits UserWarning: Duplicate Operation ID .... Before this PR the duplicates landed at different (root) paths, so it was equally duplicated but less obvious. Not introduced here, and the # HACK ... TODO: Find a better way to do this comment already flags it — but if you're touching this line anyway, re-including only the newly added MCP route instead of the whole router would kill both the duplication and most of the edge cases above.

[wakqasahmed]

This is the crux of the scope gap: the issue's other_router = APIRouter() has no routes at all, and the fix depends entirely on there being at least one route to diff against. Adding /ping here makes the test pass but moves it off the reported case.

Please keep this test and add one with a genuinely empty router asserting /empty/mcp (it currently yields /mcp), plus a case where the same endpoint function is shared by two routers. Those two are where the implementation actually breaks.

[wakqasahmed]

Good assertion — I verified this test is a real regression test, not a tautology. On the parent commit e5cad13 with only this test file applied:

FAILED test_mount_router_with_include_router_prefix -
  assert '/other/route/mcp' in ['/openapi.json', ..., '/items/{item_id}', '/error', None, None]
3 passed, 1 failed

and 4 passed on this head (with the lockfile's fastapi 0.115.11). So the claim in the PR description holds.

One caveat: because it asserts on the pinned fastapi, it gives false confidence — see the fastapi >= 0.137 comment on server.py. Consider asserting via app.openapi()["paths"] or a TestClient request to /other/route/mcp instead of walking app.routes, which would be resilient to FastAPI's internal route-table representation changing (and would have caught the 0.137 breakage).

[wakqasahmed]

Thanks for the thorough review — all three concerns confirmed and addressed in the new commit.

What changed:

1. FastAPI >= 0.137 silent no-op (blocking): _resolve_router_include_prefix now tries an exact resolution first, using FastAPI's own include-time metadata. Since 0.137, app.routes carries one _IncludedRouter per include_router() call with .original_router (exact router instance) and .include_context.prefix. Matching on original_router is router identity is exact — no diffing needed, and it works even with zero routes on the router. Falls back to the old path-diffing approach only on fastapi < 0.137, where that metadata doesn't exist.

2. Zero-route repro (blocking): with the metadata-based resolution above, the issue's literal repro (other_router = APIRouter() with no routes, included purely as a mount point) now resolves correctly on fastapi >= 0.137 — verified with a new test that also exercises the actual endpoint via TestClient rather than walking app.routes. On fastapi < 0.137 there's genuinely no data in app.routes to recover the prefix from (no metadata, no routes to diff), so it falls back safely to the app root — documented as a known limitation, not silently claimed as fixed.

3. Wrong-prefix on shared endpoint function (non-blocking, but fixed anyway): the fallback diffing path now requires every route on the router to agree on the same candidate prefix (matching endpoint identity and HTTP methods) before accepting it. On disagreement it warns and falls back to the safe empty prefix instead of picking a (possibly wrong) candidate. New test reproduces the exact ra/rb scenario from your comment and asserts the resolver never returns the other router's prefix.

Also addressed:

• Multiple-inclusion ambiguity (router included under 2+ different prefixes) — now warns and falls back to the safe empty prefix, sharing the same disambiguation logic.
• logger.info in both mount_http and mount_sse now logs the actually-resolved mount path instead of the pre-resolution relative mount_path.
• Docstrings on mount_http/mount_sse now state the ordering requirement (router must be include_router()-ed before mounting).

Left as documented, not silently glossed over:

• Zero-route resolution on fastapi < 0.137 (no recoverable data, explained above).
• The pre-existing route-duplication wart when re-including a router (already flagged by a # HACK ... TODO in the code) — untouched, out of scope for this fix.
• Resolution is still computed once at mount time and not re-evaluated later — now explicitly documented rather than silently assumed.

Verified the full suite green against both fastapi==0.115.11 (this repo's uv.lock pin) and fastapi==0.141.1 (current latest), plus ruff==0.9.10/mypy==1.15.0 (the exact versions pinned in uv.lock) clean aside from one pre-existing, unrelated mypy error in types.py that reproduces on unmodified main.

PR description updated to reflect the actual final scope.