fix(mcp-hmr): watch target directory by default #58
Conversation
Summary of ChangesHello @verveguy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider handling the case where
module_or_pathis a directory (not a file and not an importable module) explicitly, so that a user-supplied package directory is watched directly instead of silently falling back toPath.cwd(). - In
_resolve_watch_path,next(iter(spec.submodule_search_locations))will raise if the iterable is empty; it may be safer to guard this with a default or an explicit check on its length/content before callingnext().
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider handling the case where `module_or_path` is a directory (not a file and not an importable module) explicitly, so that a user-supplied package directory is watched directly instead of silently falling back to `Path.cwd()`.
- In `_resolve_watch_path`, `next(iter(spec.submodule_search_locations))` will raise if the iterable is empty; it may be safer to guard this with a default or an explicit check on its length/content before calling `next()`.
## Individual Comments
### Comment 1
<location> `packages/mcp-hmr/mcp_hmr.py:33-35` </location>
<code_context>
+ # module:attr target
+ spec = find_spec(module_or_path)
+ if spec is not None:
+ if spec.origin:
+ # For packages, origin points at __init__.py; for modules, origin is the .py file.
+ return str(Path(spec.origin).resolve().parent)
+ if spec.submodule_search_locations:
+ # Namespace packages may have no origin; use their search location.
</code_context>
<issue_to_address>
**issue:** Guard against non-filesystem `spec.origin` values (e.g. built-in or frozen modules).
For built-in / frozen / namespace-like modules, `spec.origin` can be non-path strings (e.g. `'built-in'`), so `Path(spec.origin).resolve()` may yield bogus directories like `<cwd>/built-in` and make the reloader watch unintended paths. It would be safer to first verify that `spec.origin` is an existing file (e.g. `Path(spec.origin).is_file()`), and otherwise fall back to `submodule_search_locations` or the final fallback logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the hot-reloader would fail silently by not watching any files. The introduction of the _resolve_watch_path function is a good solution to correctly determine the directory to watch. The implementation is solid, but I have one suggestion to improve its robustness when handling different types of Python modules.
packages/mcp-hmr/mcp_hmr.py
Outdated
| if spec.origin: | ||
| # For packages, origin points at __init__.py; for modules, origin is the .py file. | ||
| return str(Path(spec.origin).resolve().parent) |
There was a problem hiding this comment.
The check if spec.origin: is not fully robust. For built-in modules (e.g., sys), spec.origin is the string 'built-in', which is truthy. This causes Path('built-in').resolve().parent to be evaluated, which will likely resolve to the current working directory. While this may be the intended fallback behavior, the code is not explicit and relies on a side effect.
A more robust approach is to use spec.has_location, which is specifically designed to check if a module is associated with a file path. This makes the intent clearer and avoids ambiguity with built-in or other non-file-based modules.
| if spec.origin: | |
| # For packages, origin points at __init__.py; for modules, origin is the .py file. | |
| return str(Path(spec.origin).resolve().parent) | |
| if spec.has_location: | |
| # For packages, origin points at __init__.py; for modules, origin is the .py file. | |
| return str(Path(spec.origin).resolve().parent) |
|
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. 📝 Walkthroughfix(mcp-hmr): watch target directory by defaultAdds Changes:
Impact: File watching is now stable across module/path targets and platforms, restoring reliable hot reload behavior without manual workarounds. WalkthroughA new helper Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ 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). (1)
🔇 Additional comments (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization 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 (1)
packages/mcp-hmr/mcp_hmr.py
⏰ 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). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (1)
packages/mcp-hmr/mcp_hmr.py (1)
114-114: Proper fix for the empty watch path issue.The change from an empty string to dynamically resolved path correctly addresses the silent failure issue described in the PR objectives.
|
Pushed an update to address review feedback: guard spec.origin via spec.has_location + Path(...).is_file(), wrap find_spec() in try/except, avoid next(iter(...)) on empty submodule_search_locations, and handle directory targets explicitly. Commit: 53aee10. |
|
Update: addressed the review feedback in commit
This should prevent “watch nothing” / “watch a bogus path” edge cases while preserving the intended behavior for both |
|
Thanks @verveguy! I'll take a look later today. |
2 participants
Summary
mcp-hmrinitializingAsyncReloaderwith an empty watch path ("").Why
Watching "" can result in no files being watched (or watcher errors), causing hot reload to silently not work.
Notes
path:attrtargets watch the parent directory of the file.module:attrtargets watch the directory containing the module/package on disk viafind_spec().Test plan
mcp-hmr <target>:<attr>