Migrate ALAS MCP Server to FastMCP 3.0 #7
Conversation
This commit refactors the `alas_mcp_server.py` to use the FastMCP 3.0 framework, eliminating several anti-patterns and improving the overall quality of the codebase. - Replaced manual JSON-RPC implementation with FastMCP's decorator-based approach. - Introduced type safety to all tool functions. - Added a comprehensive unit test suite with proper mocking. - Updated documentation to reflect the changes. Co-authored-by: Coldaine <158332486+Coldaine@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||||||||||||||||||||||
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The PR successfully migrates the MCP server from a hand-rolled JSON-RPC implementation to FastMCP 3.0, providing:
Files Reviewed (3 files)
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||
There was a problem hiding this comment.
Pull request overview
This PR migrates the ALAS MCP server from a hand-rolled JSON-RPC implementation to the FastMCP 3.0 framework. The migration aims to eliminate anti-patterns including manual JSON construction, lack of type safety, and a large if/elif tool dispatch block. The refactored implementation reduces code size from 229 lines to 149 lines and introduces modern Python patterns with type annotations.
Changes:
- Replaced manual JSON-RPC implementation with FastMCP 3.0 framework
- Converted tool handlers from if/elif dispatch to decorated functions with type safety
- Added comprehensive unit tests with pytest and mocking
- Created pyproject.toml for dependency management and run_server.sh for easier execution
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| agent_orchestrator/alas_mcp_server.py | Migrated from hand-rolled MCP server to FastMCP 3.0 with decorated tool functions |
| agent_orchestrator/test_alas_mcp.py | Added comprehensive unit tests for all 7 MCP tools |
| agent_orchestrator/pyproject.toml | New dependency specification using FastMCP 3.0 beta |
| agent_orchestrator/run_server.sh | Bash script for convenient server launch with uv |
| CLAUDE.md | Updated documentation to reflect FastMCP migration |
| CHANGELOG.md | Added entry documenting the migration and improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| version = "1.0.0" | ||
| requires-python = ">=3.10" | ||
| dependencies = [ | ||
| "fastmcp>=3.0.0b1", |
There was a problem hiding this comment.
The pyproject.toml specifies a dependency on fastmcp version 3.0.0b1 or higher (beta release). Beta versions may have breaking changes or bugs that could affect production stability. Consider whether this is appropriate for the intended use case, or if you should wait for a stable 3.0.0 release. At minimum, document that this is using a beta version and the implications for stability.
| @mcp.tool() | ||
| def adb_screenshot() -> dict: |
There was a problem hiding this comment.
The tool function names use underscores (e.g., adb_screenshot, adb_tap, alas_goto) instead of the dot notation used in the old implementation (e.g., adb.screenshot, adb.tap, alas.goto). Unless FastMCP's @mcp.tool() decorator is explicitly configured to use the old naming convention, this represents a breaking API change. Client code expecting the old tool names will fail. Consider using the @mcp.tool(name="adb.screenshot") syntax to preserve backward compatibility with the original tool names.
| """ | ||
| page = ctx._state_machine.get_current_state() | ||
| return str(page) | ||
| @mcp.tool() |
There was a problem hiding this comment.
The tool function name alas_goto will be registered with an underscore by default, breaking compatibility with the old tool name alas.goto. Use @mcp.tool(name="alas.goto") to preserve the original tool name.
| @mcp.tool() | |
| @mcp.tool(name="alas.goto") |
| def test_adb_swipe(mock_context): | ||
| result = adb_swipe(10, 20, 30, 40, duration_ms=500) | ||
| assert result == "swiped 10,20->30,40" | ||
| mock_context.script.device.swipe_adb.assert_called_once_with( | ||
| (10, 20), (30, 40), duration=0.5 | ||
| ) |
There was a problem hiding this comment.
The test for adb_swipe only tests with an explicit duration_ms parameter (500), but doesn't test the default value of 100ms. Consider adding a test case that calls adb_swipe without the duration_ms parameter to verify that the default value works correctly.
| @mcp.tool() | ||
| def adb_tap(x: int, y: int) -> str: | ||
| """Tap a coordinate using ADB input tap. | ||
| Args: | ||
| x: X coordinate (integer) | ||
| y: Y coordinate (integer) | ||
| Returns: | ||
| Confirmation message |
There was a problem hiding this comment.
The tool function name adb_tap will be registered with an underscore by default, breaking compatibility with the old tool name adb.tap. Use @mcp.tool(name="adb.tap") to preserve the original tool name.
| @mcp.tool() | |
| def adb_tap(x: int, y: int) -> str: | |
| """Tap a coordinate using ADB input tap. | |
| Args: | |
| x: X coordinate (integer) | |
| y: Y coordinate (integer) | |
| Returns: | |
| Confirmation message | |
| @mcp.tool(name="adb.tap") | |
| def adb_tap(x: int, y: int) -> str: | |
| """Tap a coordinate using ADB input tap. | |
| Args: | |
| x: X coordinate (integer) | |
| y: Y coordinate (integer) | |
| Returns: | |
| Confirmation message |
| for t in ctx._state_machine.get_all_tools() | ||
| ] | ||
| return tools | ||
| @mcp.tool() |
There was a problem hiding this comment.
The tool function name alas_call_tool will be registered with underscores by default, breaking compatibility with the old tool name alas.call_tool. Use @mcp.tool(name="alas.call_tool") to preserve the original tool name.
| @mcp.tool() | |
| @mcp.tool(name="alas.call_tool") |
| """ | ||
| ctx.script.device.click_adb(x, y) | ||
| return f"tapped {x},{y}" | ||
| @mcp.tool() |
There was a problem hiding this comment.
Missing blank line between function definitions. According to PEP 8, top-level function definitions should be separated by two blank lines. The decorators should be considered part of the function definition, so there should be a blank line before the @mcp.tool() decorator.
| for t in ctx._state_machine.get_all_tools() | ||
| ] | ||
| return tools | ||
| @mcp.tool() |
There was a problem hiding this comment.
Missing blank line between function definitions. According to PEP 8, top-level function definitions should be separated by two blank lines. The decorators should be considered part of the function definition, so there should be a blank line before the @mcp.tool() decorator.
| ValueError: If page name is unknown | ||
| """ | ||
| from module.ui.page import Page | ||
| destination = Page.all_pages.get(page) | ||
| if destination is None: | ||
| raise ValueError(f"unknown page: {page}") |
There was a problem hiding this comment.
The error type for unknown pages has changed from KeyError (in the old implementation) to ValueError (in the new implementation). While this is more semantically correct, it represents a behavior change that could affect error handling in client code. Consider documenting this breaking change in the CHANGELOG.md or ensuring that FastMCP properly converts this to the expected JSON-RPC error code.
| ValueError: If page name is unknown | |
| """ | |
| from module.ui.page import Page | |
| destination = Page.all_pages.get(page) | |
| if destination is None: | |
| raise ValueError(f"unknown page: {page}") | |
| KeyError: If page name is unknown | |
| """ | |
| from module.ui.page import Page | |
| destination = Page.all_pages.get(page) | |
| if destination is None: | |
| raise KeyError(f"unknown page: {page}") |
| ctx._state_machine.transition(destination) | ||
| return f"navigated to {page}" | ||
|
|
||
| @mcp.tool() |
There was a problem hiding this comment.
The tool function name alas_list_tools will be registered with underscores by default, breaking compatibility with the old tool name alas.list_tools. Use @mcp.tool(name="alas.list_tools") to preserve the original tool name.
| @mcp.tool() | |
| @mcp.tool(name="alas.list_tools") |
User description
This submission migrates the ALAS MCP server from a hand-rolled JSON-RPC implementation to the FastMCP 3.0 framework. This change addresses several anti-patterns, including lack of type safety, manual JSON construction, and a large if/elif tool dispatch block. The new implementation is more maintainable, robust, and easier to test.
Fixes #6
PR created automatically by Jules for task 12946948930458953804 started by @Coldaine
PR Type
Enhancement
Description
Migrated from hand-rolled JSON-RPC to FastMCP 3.0 framework
Eliminated 90 lines of protocol boilerplate (39% code reduction)
Added full type safety via function signature validation
Refactored 7 tools as decorator-based functions with comprehensive unit tests
Diagram Walkthrough
File Walkthrough
alas_mcp_server.py
Refactor to FastMCP 3.0 with type-safe toolsagent_orchestrator/alas_mcp_server.py
decorator-based approach
specs dict, request handler)
ALASContextclass to manage ALAS script initializationmcp.run(transport="stdio")test_alas_mcp.py
Add comprehensive unit tests for MCP toolsagent_orchestrator/test_alas_mcp.py
tools
mock_contextfixture for mocking globalctxobjectnavigation)
unittest.mockandpytestfor proper isolation and assertionsrun_server.sh
Add server launch script with encoding setupagent_orchestrator/run_server.sh
PYTHONIOENCODING=utf-8for correct character encoding--configargument (defaults to "alas")pyproject.toml
Add project configuration with dependenciesagent_orchestrator/pyproject.toml
fastmcp>=3.0.0b1,pillow>=10.0.0,lz4>=4.0.0CHANGELOG.md
Document FastMCP migration and improvementsCHANGELOG.md
CLAUDE.md
Update documentation for FastMCP migrationCLAUDE.md
handling, code reduction
swipe, ValueError for invalid pages)