Rename ui= to app= and consolidate ToolUI/ResourceUI into AppConfig#3117
Rename ui= to app= and consolidate ToolUI/ResourceUI into AppConfig#3117
Conversation
WalkthroughThis change replaces the public UI models ToolUI and ResourceUI with a single AppConfig model in fastmcp.server.apps, and renames the decorator parameter from Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/fastmcp/tools/function_tool.py (1)
76-76: Consider a narrower type annotation forapp.
Anyworks but is looser than the corresponding parameter inserver.py(AppConfig | dict[str, Any] | bool | None). If avoiding an import ofAppConfighere, aTYPE_CHECKINGguard would preserve the type information for static analysis without runtime coupling.That said, this is a metadata carrier on an internal dataclass, so
Anyis pragmatic.src/fastmcp/server/providers/local_provider/decorators/tools.py (1)
54-63: Duplicated app→meta merging logic across two sites.This merging logic (lines 54–63) is nearly identical to
server.pylines 1510–1516. Currently they don't conflict becauseserver.tool()merges first and passesapp=NonethroughToolMeta, while this path handlesToolMeta.appset directly. However, having two independent merge sites increases the risk of future divergence.Consider extracting a shared helper (e.g.,
merge_app_into_meta(meta, app)) to keep the logic in one place.src/fastmcp/server/server.py (1)
1642-1654: Resource validation is bypassed whenappis a rawdict.The
resource_uri/visibilityguards only fire forisinstance(app, AppConfig). A user passingapp={"resourceUri": "...", "visibility": ["app"]}would skip validation and silently produce incorrect wire metadata on a resource.This may be acceptable since raw dicts are the "expert escape hatch," but it's worth noting. If you want parity, you could also check for those keys in the dict path:
Proposed fix (optional)
if isinstance(app, AppConfig): if app.resource_uri is not None: raise ValueError( "resource_uri cannot be set on resources — " "the resource itself is the UI. " "Use resource_uri on tools to point to a UI resource." ) if app.visibility is not None: raise ValueError( "visibility cannot be set on resources — it only applies to tools." ) + elif isinstance(app, dict): + if "resourceUri" in app or "resource_uri" in app: + raise ValueError( + "resource_uri cannot be set on resources — " + "the resource itself is the UI. " + "Use resource_uri on tools to point to a UI resource." + ) + if "visibility" in app: + raise ValueError( + "visibility cannot be set on resources — it only applies to tools." + )docs/development/v3-notes/v3-features.mdx (1)
1-3: Frontmatter is missing thedescriptionfield.Per coding guidelines, every MDX documentation page must begin with YAML frontmatter containing both
titleanddescription. As per coding guidelines, "Every MDX documentation page must begin with YAML frontmatter containing title and description."
Test Failure AnalysisSummary: OpenAPI performance test failed on Ubuntu due to CI runner load - initialization took 215ms instead of expected <100ms. Root Cause: The test
This failure is unrelated to PR #3117 (ui→app rename), which only touches app configuration code and doesn't affect OpenAPIProvider initialization logic. Suggested Solution: Option 1: Increase the threshold (test_comprehensive.py:736) # Current:
assert initialization_time < 0.1 # Should be under 100ms
# Suggested:
assert initialization_time < 0.3 # Should be under 300msOption 2: Mark as flaky and allow retries @pytest.mark.flaky(reruns=2)
async def test_server_performance_no_latency(self, comprehensive_openapi_spec):Option 3: Remove hard threshold - Test that initialization happens without errors rather than enforcing a specific time limit, since the actual performance varies widely by environment. My recommendation: Option 1 (increase threshold to 300ms) is simplest and still catches genuine performance regressions while being more tolerant of CI variability. Detailed AnalysisTest Failure LogThe test times this code block: async with httpx.AsyncClient(base_url="https://api.example.com") as client:
provider = OpenAPIProvider(
openapi_spec=comprehensive_openapi_spec,
client=client,
)Why This Happened Now
Historical ContextRecent test runs on main branch show mixed results, with occasional failures suggesting timing sensitivity is a known issue. Related Files
🤖 Updated analysis for workflow run #21809237545 by Marvin |
The MCP Apps decorator parameter was
ui=ToolUI(...)/ui=ResourceUI(...), which didn't match the feature's name ("Apps") or the establishedtask=True/TaskConfigconvention. This renames the user-facing API toapp=True/app=AppConfig(...)and merges the two config classes into one.Resources now validate that tool-only fields (
resource_uri,visibility) aren't set onAppConfigwhen used with@mcp.resource(). Wire format (meta["ui"],_meta.ui) is unchanged — this is purely a user-facing API rename. Breaking change from beta 2, noted in v3-features tracking.