Implement mcp-hmr library and demo#23
Conversation
Co-authored-by: me <me@promplate.dev>
|
Cursor Agent can help with this pull request. Just |
|
@cursor uv sync --all-packages |
|
I refactored dependency management, integrating
Learn more about Cursor Agents |
Co-authored-by: me <me@promplate.dev>
|
@cursor Ruff is complaining a lot ... Details |
|
I refactored the MCP demo client and server, primarily by simplifying dictionary initializations and updating type hints for improved readability and modern Python practices. Additionally, I cleaned up the
Learn more about Cursor Agents |
This commit refactors the MCP client and server examples for better clarity and efficiency. It includes updates to the client's request handling, server's tool registration, and general code cleanup. Co-authored-by: me <me@promplate.dev>
|
@cursor Now pyright is complaining: |
|
I enhanced the project's development tooling by integrating PyRight for static type checking. This involved adding PyRight as a development dependency and introducing type assertions in the
Learn more about Cursor Agents |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: me <me@promplate.dev>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The CLI entrypoint in mcp_hmr/init.py is quite large and handles module resolution, file watching, and server lifecycle; consider refactoring it into smaller modules or helper classes (e.g., separate file‐watcher setup, server runner, and logging config) to improve readability and testability.
- The manual slug parsing and module resolution logic walks sys.path and filesystem directly; you could simplify and harden this by using importlib’s built-in finders and spec utilities rather than custom path checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CLI entrypoint in mcp_hmr/__init__.py is quite large and handles module resolution, file watching, and server lifecycle; consider refactoring it into smaller modules or helper classes (e.g., separate file‐watcher setup, server runner, and logging config) to improve readability and testability.
- The manual slug parsing and module resolution logic walks sys.path and filesystem directly; you could simplify and harden this by using importlib’s built-in finders and spec utilities rather than custom path checks.
## Individual Comments
### Comment 1
<location> `packages/mcp-hmr/mcp_hmr/__init__.py:53-57` </location>
<code_context>
+
+ file = file.resolve()
+
+ if module in sys.modules:
+ return secho(
+ f"It seems you've already imported `{module}` as a normal module. You should call `reactivity.hmr.core.patch_meta_path()` before it.",
+ fg="red",
+ )
+
+ from atexit import register
</code_context>
<issue_to_address>
**issue (bug_risk):** Early return after secho may not exit the process as intended.
If you intend to stop execution when the module is already imported, use exit(1) or raise an exception instead of just returning.
</issue_to_address>
### Comment 2
<location> `packages/mcp-hmr/mcp_hmr/__init__.py:138` </location>
<code_context>
+ if "." in module:
+ __import__(module.rsplit(".", 1)[0]) # ensure parent modules are imported
+
+ if __version__ >= "0.6.4":
+ from reactivity.hmr.core import _loader as loader
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** String comparison for version may not work as expected for semantic versions.
Lexicographical string comparison can misorder semantic versions. Use packaging.version.parse or an equivalent method for accurate version checks.
</issue_to_address>
### Comment 3
<location> `packages/mcp-hmr/mcp_hmr/__init__.py:189-198` </location>
<code_context>
+ ):
+ return super().start_watching()
+
+ logger = getLogger("mcp.hmr")
+ logger.setLevel("INFO")
+
+ # Set up console handler if not already present
+ if not logger.handlers:
+ import logging
+
+ handler = logging.StreamHandler()
+ formatter = logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s")
+ handler.setFormatter(formatter)
+ logger.addHandler(handler)
+
+ try:
</code_context>
<issue_to_address>
**nitpick:** Logger level is set to "INFO" as a string instead of logging.INFO.
Using a string for the logger level may cause unexpected behavior. Please use logging.INFO instead.
</issue_to_address>
### Comment 4
<location> `examples/mcp-demo/client.py:51-54` </location>
<code_context>
+
+ # Send request
+ request_json = json.dumps(request) + "\n"
+ self.process.stdin.write(request_json)
+ self.process.stdin.flush()
+
+ # Read response
</code_context>
<issue_to_address>
**suggestion:** No error handling for broken pipe when writing to stdin.
Add a try-except block to handle BrokenPipeError when writing to stdin, ensuring the client does not crash if the server process exits unexpectedly.
```suggestion
# Send request
request_json = json.dumps(request) + "\n"
try:
self.process.stdin.write(request_json)
self.process.stdin.flush()
except BrokenPipeError:
raise RuntimeError("Failed to write to server process stdin: Broken pipe. The server process may have exited unexpectedly.")
```
</issue_to_address>
### Comment 5
<location> `packages/mcp-hmr/mcp_hmr/__init__.py:13` </location>
<code_context>
+
+
+@app.command(no_args_is_help=True)
+def main(
+ slug: Annotated[str, Argument(help="MCP server module and function (e.g., 'server:main' or 'server:app')")] = "server:main",
+ reload_include: list[str] = [str(Path.cwd())], # noqa: B006, B008
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the code to separate CLI parsing, server lifecycle management, reloader logic, and logging into distinct modules for improved clarity and maintainability.
Here are a few focused refactorings that will flatten the nesting and split responsibilities without changing any behavior:
1. Extract CLI parsing into `hmr/cli.py`
```python
# hmr/cli.py
from typer import Typer, Argument, Option, secho
from .runner import run_hmr
app = Typer(help="Hot Module Replacement for MCP", add_completion=False)
@app.command(no_args_is_help=True)
def main(
slug: str = Argument("server:main", help="module:function"),
reload_include: list[str] = Option([str(Path.cwd())], "--reload-include"),
reload_exclude: list[str] = Option([".venv", "__pycache__", "*.pyc"], "--reload-exclude"),
transport: str = Option("stdio", "--transport"),
clear: bool = Option(False, "--clear"),
):
if ":" not in slug:
secho(f"Invalid slug `{slug}`, expected module:function", fg="red")
raise SystemExit(1)
run_hmr(slug, reload_include, reload_exclude, transport, clear)
```
2. Move server‐lifecycle into a `ServerManager` in `hmr/runner.py`
```python
# hmr/runner.py
import asyncio
import threading
from typing import Callable
class ServerManager:
def __init__(self, clear: bool):
self._clear = clear
self._task = None
self._event = threading.Event()
def start(self, server_func: Callable):
def _run():
try:
if asyncio.iscoroutinefunction(server_func):
loop = asyncio.new_event_loop()
loop.run_until_complete(server_func())
loop.close()
else:
server_func()
finally:
self._event.set()
thread = threading.Thread(target=_run, daemon=True)
thread.start()
def stop(self):
if self._task and not self._task.done():
self._task.cancel()
self._event.set()
def run_hmr(slug, inc, exc, transport, clear):
# module resolution + import machinery as before...
from .reloader import HmrReloader
manager = ServerManager(clear)
reloader = HmrReloader(module, file_path, attr, inc, exc, manager)
reloader.keep_watching_until_interrupt()
manager.stop()
```
3. Isolate the reloader in `hmr/reloader.py`
```python
# hmr/reloader.py
from watchfiles import Change
from reactivity.hmr.core import SyncReloader, ReactiveModule
from .runner import ServerManager
class HmrReloader(SyncReloader):
def __init__(self, module, filepath, attr, include, exclude, server_mgr: ServerManager):
super().__init__(str(filepath), include, exclude)
self.module = module
self.filepath = filepath
self.attr = attr
self.server_mgr = server_mgr
def run_entry_file(self):
self.server_mgr.stop()
mod = self._load_module() # factor out your cached_property logic
srv = getattr(mod, self.attr)
self.server_mgr.start(srv)
def on_events(self, events: set[Change]):
# same logic as before, then call super()
return super().on_events(events)
```
4. Delegate logging setup to `hmr/logging.py` or similar.
By splitting into `cli.py`, `runner.py`, `reloader.py` (and optional `logging.py`), you:
- eliminate deep nested closures,
- remove most `nonlocal` usage,
- keep each file under ~100 lines,
- and make each unit test‐friendly and self‐contained.
</issue_to_address>
### Comment 6
<location> `examples/mcp-demo/client.py:27` </location>
<code_context>
self.process = subprocess.Popen(command, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, bufsize=0) # noqa: ASYNC220
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location> `examples/mcp-demo/client.py:62-63` </location>
<code_context>
async def send_request(self, method: str, params: dict[str, Any] | None = None) -> dict[str, Any]:
"""Send a request to the MCP server."""
if not self.process:
raise RuntimeError("Server not started")
# Type assertion to help PyRight understand that self.process is not None
assert self.process is not None
assert self.process.stdin is not None
assert self.process.stdout is not None
request = {"jsonrpc": "2.0", "id": self.get_next_id(), "method": method}
if params:
request["params"] = params
# Send request
request_json = json.dumps(request) + "\n"
self.process.stdin.write(request_json)
self.process.stdin.flush()
# Read response
response_line = self.process.stdout.readline().strip()
if not response_line:
raise RuntimeError("No response from server")
try:
response = json.loads(response_line)
return response
except json.JSONDecodeError as e:
raise RuntimeError(f"Invalid JSON response: {e}") from e
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))
```suggestion
return json.loads(response_line)
```
</issue_to_address>
### Comment 8
<location> `examples/mcp-demo/server.py:51` </location>
<code_context>
async def handle_request(self, request: dict[str, Any]) -> dict[str, Any]:
"""Handle an incoming MCP request."""
method = request.get("method")
params = request.get("params", {})
request_id = request.get("id")
try:
if method == "initialize":
return {
"jsonrpc": "2.0",
"id": request_id,
"result": {
"protocolVersion": "2024-11-05",
"capabilities": {
"tools": {"listChanged": True},
"resources": {"subscribe": True, "listChanged": True},
},
"serverInfo": {"name": self.name, "version": self.version},
},
}
elif method == "tools/list":
tools_list = []
for _tool_name, tool_info in self.tools.items():
tools_list.append({"name": tool_info["name"], "description": tool_info["description"], "inputSchema": tool_info["inputSchema"]})
return {"jsonrpc": "2.0", "id": request_id, "result": {"tools": tools_list}}
elif method == "tools/call":
tool_name = params.get("name")
arguments = params.get("arguments", {})
if tool_name not in self.tools:
raise ValueError(f"Unknown tool: {tool_name}")
handler = self.tools[tool_name]["handler"]
result = await handler(arguments)
return {"jsonrpc": "2.0", "id": request_id, "result": {"content": result}}
elif method == "resources/list":
resources_list = []
for _uri, resource_info in self.resources.items():
resources_list.append({"uri": resource_info["uri"], "name": resource_info["name"], "description": resource_info["description"]})
return {"jsonrpc": "2.0", "id": request_id, "result": {"resources": resources_list}}
else:
raise ValueError(f"Unknown method: {method}")
except Exception as e:
return {"jsonrpc": "2.0", "id": request_id, "error": {"code": -32000, "message": str(e)}}
</code_context>
<issue_to_address>
**issue (code-quality):** Convert for loop into list comprehension [×2] ([`list-comprehension`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/list-comprehension/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if module in sys.modules: | ||
| return secho( | ||
| f"It seems you've already imported `{module}` as a normal module. You should call `reactivity.hmr.core.patch_meta_path()` before it.", | ||
| fg="red", | ||
| ) |
There was a problem hiding this comment.
issue (bug_risk): Early return after secho may not exit the process as intended.
If you intend to stop execution when the module is already imported, use exit(1) or raise an exception instead of just returning.
| if "." in module: | ||
| __import__(module.rsplit(".", 1)[0]) # ensure parent modules are imported | ||
|
|
||
| if __version__ >= "0.6.4": |
There was a problem hiding this comment.
issue (bug_risk): String comparison for version may not work as expected for semantic versions.
Lexicographical string comparison can misorder semantic versions. Use packaging.version.parse or an equivalent method for accurate version checks.
| logger = getLogger("mcp.hmr") | ||
| logger.setLevel("INFO") | ||
|
|
||
| # Set up console handler if not already present | ||
| if not logger.handlers: | ||
| import logging | ||
|
|
||
| handler = logging.StreamHandler() | ||
| formatter = logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s") | ||
| handler.setFormatter(formatter) |
There was a problem hiding this comment.
nitpick: Logger level is set to "INFO" as a string instead of logging.INFO.
Using a string for the logger level may cause unexpected behavior. Please use logging.INFO instead.
| # Send request | ||
| request_json = json.dumps(request) + "\n" | ||
| self.process.stdin.write(request_json) | ||
| self.process.stdin.flush() |
There was a problem hiding this comment.
suggestion: No error handling for broken pipe when writing to stdin.
Add a try-except block to handle BrokenPipeError when writing to stdin, ensuring the client does not crash if the server process exits unexpectedly.
| # Send request | |
| request_json = json.dumps(request) + "\n" | |
| self.process.stdin.write(request_json) | |
| self.process.stdin.flush() | |
| # Send request | |
| request_json = json.dumps(request) + "\n" | |
| try: | |
| self.process.stdin.write(request_json) | |
| self.process.stdin.flush() | |
| except BrokenPipeError: | |
| raise RuntimeError("Failed to write to server process stdin: Broken pipe. The server process may have exited unexpectedly.") |
| async def start_server(self, command: list[str]): | ||
| """Start the MCP server process.""" | ||
| print(f"🚀 Starting server: {' '.join(command)}") | ||
| self.process = subprocess.Popen(command, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, bufsize=0) # noqa: ASYNC220 |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
| response = json.loads(response_line) | ||
| return response |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| response = json.loads(response_line) | |
| return response | |
| return json.loads(response_line) |
| } | ||
|
|
||
| elif method == "tools/list": | ||
| tools_list = [] |
There was a problem hiding this comment.
issue (code-quality): Convert for loop into list comprehension [×2] (list-comprehension)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a new MCP-HMR package with a Typer-based CLI for hot-reloading MCP servers, adds an MCP demo server and client, updates workspace and root documentation, provides packaging and project configs, and includes an installation script. Implements file watching, dependency-aware reloads, and stdio transport for MCP JSON-RPC. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 5
🧹 Nitpick comments (14)
README.md (1)
100-100: Minor typographic correction needed.The apostrophe in "SolidJS's" should use a proper Unicode apostrophe for consistency.
-About fine-grained reactivity, I recommend reading [SolidJS's excellent explanation](https://docs.solidjs.com/advanced-concepts/fine-grained-reactivity). +About fine-grained reactivity, I recommend reading [SolidJS's excellent explanation](https://docs.solidjs.com/advanced-concepts/fine-grained-reactivity).packages/mcp-hmr/pyproject.toml (2)
28-30: Update placeholder URLs to actual repository.The GitHub URLs appear to be placeholders and should point to the actual repository location.
-Homepage = "https://github.com/mcp-hmr/mcp-hmr" -Repository = "https://github.com/mcp-hmr/mcp-hmr" +Homepage = "https://github.com/promplate/hmr" +Repository = "https://github.com/promplate/hmr"
8-8: Update placeholder email address.The email address appears to be a placeholder and should be updated to a real contact.
-authors = [{ name = "MCP HMR", email = "dev@mcp-hmr.dev" }] +authors = [{ name = "MCP HMR Contributors", email = "hmr@promplate.dev" }]IMPLEMENTATION-SUMMARY.md (1)
53-59: Add language specification for code block.The fenced code block should specify the language for better syntax highlighting.
-``` +```text mcp-hmr CLI ↓ File Watcher (watchfiles) → HMR Engine (reactivity.hmr) → MCP Server Functionpackages/mcp-hmr/README.md (2)
135-140: Inconsistent note on include/exclude “patterns” vs defaultsREADME says “patterns will be treated as literal paths,” yet defaults include
*.pyc, which is a glob. Align the wording (and behavior) to explicitly support globs.
149-152: “Preserves connection state” claim likely too strongGiven the server task is cancelled/restarted on reload, clients may see a brief disconnect depending on transport. Rephrase to “attempts to preserve” and document caveats.
examples/mcp-demo/client.py (3)
90-99: Avoid deprecated loop API; prefer asyncio.to_threadUse
asyncio.to_threadin both client and server to avoid deprecatedget_event_loop().run_in_executor(...).Apply:
- print("\n1. Initializing server...") - response = await client.send_request("initialize", {"protocolVersion": "2024-11-05", "capabilities": {}, "clientInfo": {"name": "test-client", "version": "0.1.0"}}) + print("\n1. Initializing server...") + response = await client.send_request( + "initialize", + { + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "test-client", "version": "0.1.0"}, + }, + )And in
stop_server:- await asyncio.wait_for(asyncio.get_event_loop().run_in_executor(None, process.wait), timeout=5.0) + await asyncio.wait_for(asyncio.to_thread(process.wait), timeout=5.0)
36-45: Optional member access: narrow once, use a localYour asserts help Pyright, but a local alias also simplifies and silences optional‑access warnings.
Apply:
- assert self.process is not None - assert self.process.stdin is not None - assert self.process.stdout is not None + assert self.process is not None + proc = self.process + assert proc.stdin is not None and proc.stdout is not NoneAnd below:
- self.process.stdin.write(request_json) - self.process.stdin.flush() + proc.stdin.write(request_json) + proc.stdin.flush() - response_line = self.process.stdout.readline().strip() + response_line = proc.stdout.readline().strip()
57-63: Avoid indefinite block on readlineConsider a timeout to prevent hangs if the server stops responding.
Apply:
- response_line = self.process.stdout.readline().strip() + response_line = await asyncio.wait_for( + asyncio.to_thread(self.process.stdout.readline), # type: ignore[union-attr] + timeout=10.0, + ) + response_line = response_line.strip()examples/mcp-demo/server.py (1)
90-101: Prefer asyncio.to_thread over get_event_loop().run_in_executorThis avoids deprecated APIs and is clearer.
Apply:
- line = await asyncio.get_event_loop().run_in_executor(None, sys.stdin.readline) + line = await asyncio.to_thread(sys.stdin.readline)packages/mcp-hmr/mcp_hmr/__init__.py (4)
118-121: Stop thread cleanly on reload to avoid leaksYou set
finish, but never join the thread. Join with a short timeout to reduce thread buildup across reloads.Apply:
- server_thread = Thread(target=run_server, daemon=True) + server_thread = Thread(target=run_server, daemon=True) server_thread.start() @@ - def stop_server(): + def stop_server(): nonlocal server_task if server_task and not server_task.done(): server_task.cancel() finish.set() + server_thread.join(timeout=2.0)
27-31: Prefer sys.exit or Typer’s Exit over built‑in exit
exit()depends on site; usesys.exit(1)orraise typer.Exit(1)for CLIs.Apply:
- exit(1) + sys.exit(1)
5-5: typing.override availability
typing.overriderequires Python 3.12+. If you support 3.11, usefrom typing_extensions import override.Apply:
-from typing import TYPE_CHECKING, Annotated, override +from typing import TYPE_CHECKING, Annotated +try: + from typing import override # py312+ +except ImportError: # py311 + from typing_extensions import override # type: ignore
85-87: Remove unused variable
server_processis defined but never used.- server_process = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
IMPLEMENTATION-SUMMARY.md(1 hunks)README-mcp-hmr.md(1 hunks)README.md(1 hunks)examples/mcp-demo/README.md(1 hunks)examples/mcp-demo/client.py(1 hunks)examples/mcp-demo/pyproject.toml(1 hunks)examples/mcp-demo/server.py(1 hunks)install-mcp-hmr.sh(1 hunks)packages/mcp-hmr/README.md(1 hunks)packages/mcp-hmr/mcp_hmr/__init__.py(1 hunks)packages/mcp-hmr/pyproject.toml(1 hunks)pyproject.toml(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/mcp-demo/server.py (1)
packages/mcp-hmr/mcp_hmr/__init__.py (1)
main(13-206)
packages/mcp-hmr/mcp_hmr/__init__.py (2)
examples/mcp-demo/server.py (1)
main(201-203)examples/mcp-demo/client.py (2)
stop_server(67-76)start_server(24-34)
🪛 markdownlint-cli2 (0.18.1)
IMPLEMENTATION-SUMMARY.md
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Link Check
🔇 Additional comments (14)
pyproject.toml (3)
21-21: LGTM on workspace source mapping.The workspace source configuration correctly maps
mcp-hmras a workspace dependency, consistent with the other packages.
56-59: LGTM on development dependencies.The
pyright>=1.1.405requirement is current and properly structured within the dependency groups.
9-9: Workspace config OK — mcp-hmr present & included.
packages/mcp-hmr contains pyproject.toml and the mcp_hmr module; root pyproject.toml lists [tool.uv.workspace] members = ["examples/","packages/"], so mcp-hmr is a workspace member and workspace resolution should succeed.examples/mcp-demo/pyproject.toml (1)
1-15: LGTM on demo project configuration.The project metadata is well-defined with appropriate dependencies for an MCP demo. The workspace reference to
mcp-hmraligns with the root configuration.README-mcp-hmr.md (1)
1-192: LGTM on comprehensive MCP-HMR documentation.The documentation is thorough and well-structured, covering features, installation, usage, architecture, and comparisons. The examples are clear and the workflow instructions are detailed.
install-mcp-hmr.sh (1)
1-55: LGTM on installation script with robust fallbacks.The script handles multiple installation scenarios gracefully with proper error checking and fallback strategies for different package managers.
README.md (1)
57-96: LGTM on workspace documentation updates.The additions clearly document the new MCP-HMR package and provide helpful getting started instructions that integrate well with the existing content.
packages/mcp-hmr/pyproject.toml (1)
1-34: Overall package configuration looks solid.The dependencies, entry point, and build system configuration are appropriate for the MCP-HMR package.
IMPLEMENTATION-SUMMARY.md (1)
1-123: LGTM on comprehensive implementation summary.The summary effectively documents all the key components, architecture, and success metrics of the MCP-HMR implementation.
examples/mcp-demo/README.md (1)
1-156: LGTM on detailed demo documentation.The README provides excellent step-by-step instructions for using the demo and demonstrates hot reloading capabilities clearly with practical examples.
examples/mcp-demo/client.py (1)
24-31: Subprocess safety: low risk here, but keep inputs staticCommand comes from code, not user input, so risk is low. Keep it a list (as you already do) and avoid shell=True. Consider draining stderr in a background task to prevent pipe back‑pressure if the server logs heavily.
packages/mcp-hmr/mcp_hmr/__init__.py (3)
201-206: Graceful shutdown path looks goodReloader loop exits cleanly and ensures
stop_server()is called.
1-19: CLI surface and defaults look solidGood Typer usage and sensible defaults for include/exclude; nice help text.
64-67: Incorrect: 'hmr = { workspace = true }' claim is wrong — mcp-hmr is covered by the workspaceNo 'hmr = { workspace = true }' exists in the repo. Top-level pyproject declares [tool.uv.workspace] members = ["examples/","packages/"] (pyproject.toml:14–16), and examples/mcp-demo/pyproject.toml declares mcp-hmr = { workspace = true } (examples/mcp-demo/pyproject.toml:14–15). Ignore the original workspace-warning.
Likely an incorrect or invalid review comment.
| await asyncio.wait_for(asyncio.get_event_loop().run_in_executor(None, process.wait), timeout=5.0) | ||
| except TimeoutError: | ||
| process.kill() |
There was a problem hiding this comment.
Catch the right timeout exception
asyncio.wait_for raises asyncio.TimeoutError, not the built‑in TimeoutError.
Apply:
- except TimeoutError:
+ except asyncio.TimeoutError:
process.kill()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await asyncio.wait_for(asyncio.get_event_loop().run_in_executor(None, process.wait), timeout=5.0) | |
| except TimeoutError: | |
| process.kill() | |
| await asyncio.wait_for(asyncio.get_event_loop().run_in_executor(None, process.wait), timeout=5.0) | |
| except asyncio.TimeoutError: | |
| process.kill() |
🤖 Prompt for AI Agents
In examples/mcp-demo/client.py around lines 73 to 75, the except block is
catching the built-in TimeoutError but asyncio.wait_for raises
asyncio.TimeoutError; update the except to catch asyncio.TimeoutError (or import
TimeoutError from asyncio and use that) so the timeout is handled correctly,
then keep the process.kill() logic as-is.
| transport: Annotated[str, Option("--transport", help="Transport method: 'stdio' or 'sse'")] = "stdio", # noqa: ARG001 | ||
| clear: Annotated[bool, Option("--clear", help="Clear the terminal before restarting the server")] = False, # noqa: FBT002 | ||
| ): |
There was a problem hiding this comment.
--transport option is unused
The code ignores the selected transport; README advertises SSE. Either implement or drop the option for now.
Suggested minimal stopgap:
- transport: Annotated[str, Option("--transport", help="Transport method: 'stdio' or 'sse'")] = "stdio", # noqa: ARG001
+ transport: Annotated[str, Option("--transport", help="Transport method: 'stdio' (default). 'sse' not yet implemented.")]= "stdio",And warn on non‑stdio:
- server_func = getattr(self.entry_module, attr)
+ server_func = getattr(self.entry_module, attr)
+ if transport != "stdio":
+ logger.warning("Transport '%s' is not implemented; falling back to 'stdio'.", transport)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/mcp-hmr/mcp_hmr/__init__.py around lines 17 to 19, the --transport
option is accepted but never used; either remove the option or handle non-stdio
selections. Minimal fix: keep the CLI option but detect if transport != "stdio",
emit a clear warning (or warning-level logger) that only "stdio" is implemented
and that the chosen transport will fall back to "stdio", then proceed using
stdio behavior; alternatively, if you prefer to drop the feature, remove the
transport CLI parameter and any docs referencing SSE.
| def start_server(server_func: "Callable"): | ||
| nonlocal stop_server | ||
|
|
||
| server_task = None | ||
| server_process = None | ||
| finish = Event() | ||
|
|
||
| def run_server(): | ||
| nonlocal server_task, server_process | ||
| watched_paths = [Path(p).resolve() for p in (file, *reload_include)] | ||
| ignored_paths = [Path(p).resolve() for p in reloader.excludes] | ||
| if all(is_relative_to_any(path, ignored_paths) or not is_relative_to_any(path, watched_paths) for path in ReactiveModule.instances): | ||
| logger.error("No files to watch for changes. The server will never reload.") | ||
|
|
||
| try: | ||
| if asyncio.iscoroutinefunction(server_func): | ||
| # If it's an async function, run it in an event loop | ||
| loop = asyncio.new_event_loop() | ||
| asyncio.set_event_loop(loop) | ||
| server_task = loop.create_task(server_func()) | ||
| try: | ||
| loop.run_until_complete(server_task) | ||
| except asyncio.CancelledError: | ||
| pass | ||
| finally: | ||
| loop.close() | ||
| else: | ||
| # If it's a sync function, just call it | ||
| server_func() | ||
| except KeyboardInterrupt: | ||
| pass | ||
| except Exception as e: | ||
| logger.error(f"Server error: {e}") | ||
| finally: | ||
| finish.set() | ||
|
|
||
| server_thread = Thread(target=run_server, daemon=True) | ||
| server_thread.start() | ||
|
|
||
| def stop_server(): |
There was a problem hiding this comment.
Logger used before definition can raise at runtime
run_server() logs before logger is defined (logger is initialized later). Move logger setup before start_server or capture it as a default argument.
Apply one of:
- from logging import getLogger
+ from logging import getLogger
@@
- def start_server(server_func: "Callable"):
+ logger = getLogger("mcp.hmr")
+ def start_server(server_func: "Callable"):Or:
- def run_server():
+ def run_server(_logger=logger):
@@
- logger.error("No files to watch for changes. The server will never reload.")
+ _logger.error("No files to watch for changes. The server will never reload.")
@@
- logger.error(f"Server error: {e}")
+ _logger.error(f"Server error: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def start_server(server_func: "Callable"): | |
| nonlocal stop_server | |
| server_task = None | |
| server_process = None | |
| finish = Event() | |
| def run_server(): | |
| nonlocal server_task, server_process | |
| watched_paths = [Path(p).resolve() for p in (file, *reload_include)] | |
| ignored_paths = [Path(p).resolve() for p in reloader.excludes] | |
| if all(is_relative_to_any(path, ignored_paths) or not is_relative_to_any(path, watched_paths) for path in ReactiveModule.instances): | |
| logger.error("No files to watch for changes. The server will never reload.") | |
| try: | |
| if asyncio.iscoroutinefunction(server_func): | |
| # If it's an async function, run it in an event loop | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| server_task = loop.create_task(server_func()) | |
| try: | |
| loop.run_until_complete(server_task) | |
| except asyncio.CancelledError: | |
| pass | |
| finally: | |
| loop.close() | |
| else: | |
| # If it's a sync function, just call it | |
| server_func() | |
| except KeyboardInterrupt: | |
| pass | |
| except Exception as e: | |
| logger.error(f"Server error: {e}") | |
| finally: | |
| finish.set() | |
| server_thread = Thread(target=run_server, daemon=True) | |
| server_thread.start() | |
| def stop_server(): | |
| logger = getLogger("mcp.hmr") | |
| def start_server(server_func: "Callable"): | |
| nonlocal stop_server | |
| server_task = None | |
| server_process = None | |
| finish = Event() | |
| def run_server(): | |
| nonlocal server_task, server_process | |
| watched_paths = [Path(p).resolve() for p in (file, *reload_include)] | |
| ignored_paths = [Path(p).resolve() for p in reloader.excludes] | |
| if all(is_relative_to_any(path, ignored_paths) or not is_relative_to_any(path, watched_paths) for path in ReactiveModule.instances): | |
| logger.error("No files to watch for changes. The server will never reload.") | |
| try: | |
| if asyncio.iscoroutinefunction(server_func): | |
| # If it's an async function, run it in an event loop | |
| loop = asyncio.new_event_loop() | |
| asyncio.set_event_loop(loop) | |
| server_task = loop.create_task(server_func()) | |
| try: | |
| loop.run_until_complete(server_task) | |
| except asyncio.CancelledError: | |
| pass | |
| finally: | |
| loop.close() | |
| else: | |
| # If it's a sync function, just call it | |
| server_func() | |
| except KeyboardInterrupt: | |
| pass | |
| except Exception as e: | |
| logger.error(f"Server error: {e}") | |
| finally: | |
| finish.set() | |
| server_thread = Thread(target=run_server, daemon=True) | |
| server_thread.start() | |
| def stop_server(): |
🤖 Prompt for AI Agents
In packages/mcp-hmr/mcp_hmr/__init__.py around lines 82 to 121, run_server()
uses logger before logger is initialized which can raise at runtime; fix by
ensuring logger is defined prior to start_server (move logger setup above the
start_server definition) or bind the logger into run_server as a default
parameter (e.g., def run_server(logger=logger):) so the closure captures an
already-initialized logger; update code to either initialize logger earlier or
change the function signature to capture logger, and remove any references to a
later logger initialization within start_server.
| if __version__ >= "0.6.4": | ||
| from reactivity.hmr.core import _loader as loader | ||
| else: | ||
| loader = ReactiveModuleLoader(file) # type: ignore |
There was a problem hiding this comment.
Bug: string version comparison is incorrect
"0.10.0" >= "0.6.4" is False lexicographically. Use proper version parsing.
Apply:
- if __version__ >= "0.6.4":
+ from packaging.version import Version
+ if Version(__version__) >= Version("0.6.4"):
from reactivity.hmr.core import _loader as loader
else:
loader = ReactiveModuleLoader(file) # type: ignore📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if __version__ >= "0.6.4": | |
| from reactivity.hmr.core import _loader as loader | |
| else: | |
| loader = ReactiveModuleLoader(file) # type: ignore | |
| from packaging.version import Version | |
| if Version(__version__) >= Version("0.6.4"): | |
| from reactivity.hmr.core import _loader as loader | |
| else: | |
| loader = ReactiveModuleLoader(file) # type: ignore |
🤖 Prompt for AI Agents
In packages/mcp-hmr/mcp_hmr/__init__.py around lines 138 to 141, the code
compares version strings lexicographically (e.g. "0.10.0" >= "0.6.4") which is
incorrect; change to parse versions before comparing by importing a version
parser (for example from packaging.version import parse) and replace the string
comparison with parse(__version__) >= parse("0.6.4"). Ensure the import is added
at top of the file and use the parsed Version objects for the conditional so
version ordering is correct.
| - `--transport`: Transport method - `stdio` or `sse` (defaults to `stdio`) | ||
| - `--clear`: Clear the terminal before each reload |
There was a problem hiding this comment.
Doc advertises --transport sse, but code doesn’t implement it
The CLI exposes --transport and README documents SSE, but the implementation doesn’t use this option at all. Please either wire it through (and implement SSE) or remove it from docs/CLI until supported.
CNSeniorious000
force-pushed
the
readme
branch
from
5a6f396 to
6dc27a0
Compare
|
CNSeniorious000
deleted the
cursor/implement-mcp-hmr-library-and-demo-2f44
branch
2 participants
Implement
mcp-hmrlibrary and a demo to provide hot module reloading for MCP servers, enhancing developer experience.This library enables developers to make changes to their MCP server code and see them applied instantly without restarting the entire server process, mirroring the functionality of
uvicorn-hmrfor ASGI servers.Summary by CodeRabbit
New Features
Documentation
Chores