Modernize MCP Server - #9
Merged
Merged
Conversation
Widen MCP SDK to >=1.7.0,<2.0.0 for FastMCP support and add Python logging to stderr so tool errors produce stack traces instead of being silently swallowed.
Replace manual schema definitions and dispatch table with FastMCP-decorated functions, fix parameter defaults (case_sensitive, context_lines), and suppress outputSchema generation.
Domain exceptions now raise ToolError instead of returning error dicts, so MCP clients can distinguish errors from successes. Updated all tests accordingly.
Verify tool registration, descriptions, annotations, and outputSchema suppression. Update CLAUDE.md for the new FastMCP workflow.
Batch edits are now all-or-nothing (file unchanged if any change fails). Added Field constraints to FastMCP wrappers and explicit PermissionError handling in write operations.
No longer imported anywhere; all schemas now live as FastMCP decorator annotations in server.py.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMigrates the MCP server to FastMCP, exposes seven mcp.tool endpoints (overview, search, read, edit, revert, list, search_directory), centralizes logging/config, switches tools to raise ToolError, removes src/mcp_schemas.py, adds atomic batch-edit guard and explicit PermissionError handling in file writes. Changes
Sequence Diagram(s)mermaid Client->>FastMCP: invoke tool (e.g., edit_content) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/file_access.py (1)
15-16: Logger defined but unused.The logger is created but not used in the visible code. Consider either adding log statements for important operations (e.g., file writes, backup creation) or removing the unused import and variable if logging is not planned for this module.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/file_access.py` around lines 15 - 16, The module currently defines logger = logging.getLogger(__name__) but never uses it; either remove the logging import and the logger variable or add appropriate log statements (e.g., info/debug/error) in key functions such as any file write/backup-related functions (use logger.info(...) on successful writes, logger.error(...) on exceptions, and logger.debug(...) for detailed state) so the logger symbol is actually referenced; update imports accordingly and ensure exception handlers call logger.exception where applicable.tests/unit/test_file_access.py (1)
1124-1126: Usepytest.skip()instead of barereturn.Using
returnsilently exits the test without reporting it was skipped. Usingpytest.skip()properly marks the test as skipped in test reports.♻️ Proposed fix
# Skip on Windows where chmod doesn't restrict owner writes reliably if sys.platform == "win32": - return + pytest.skip("chmod doesn't restrict owner writes reliably on Windows")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_file_access.py` around lines 1124 - 1126, Replace the bare early return used to skip the test on Windows with pytest.skip so the test run records a skipped test; in tests/unit/test_file_access.py update the sys.platform == "win32" branch to call pytest.skip("Skipping on Windows where chmod doesn't restrict owner writes reliably") and ensure pytest is imported in the file (add import pytest if missing), leaving the rest of the test intact.src/editor.py (1)
13-14: Logger defined but unused.Similar to
src/file_access.py, the logger is created but not used in the visible code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/editor.py` around lines 13 - 14, The module-level logger variable created by logging.getLogger(__name__) is defined but never used; either remove the unused logger declaration or use it for relevant runtime messages—e.g., replace silent exceptions/prints in functions in this module with logger.debug/info/warning/error calls or add a short initialization log like logger.debug("editor module loaded")—update the logger variable (named logger) accordingly throughout the module or delete the unused declaration.tests/integration/test_documentation_workflows.py (1)
156-164: Cover the negative-limit case promised by the test docstring.Line 157 says “zero or negative limit,” but the test only asserts
limit=0. Addlimit=-1(parametrized) to keep intent and coverage aligned.♻️ Suggested test tweak
- def test_read_content_tail_mode_requires_positive_limit(self): - """Raises ToolError for zero or negative limit.""" + `@pytest.mark.parametrize`("bad_limit", [0, -1]) + def test_read_content_tail_mode_requires_positive_limit(self, bad_limit): + """Raises ToolError for zero or negative limit.""" doc = self.test_data_dir / "markdown" / "fastapi-docs.md" @@ - with pytest.raises(ToolError, match="limit must be >= 1"): - read_content(str(doc), limit=0, mode="tail") + with pytest.raises(ToolError, match="limit must be >= 1"): + read_content(str(doc), limit=bad_limit, mode="tail")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_documentation_workflows.py` around lines 156 - 164, Update the test_read_content_tail_mode_requires_positive_limit to assert both zero and negative limits: parametrize the test (or loop) so it runs with limit=0 and limit=-1 and in each case call read_content(str(doc), limit=limit, mode="tail") inside pytest.raises(ToolError, match="limit must be >= 1"); ensure the test still skips early if the doc file doesn't exist and retains the same error match and mode to cover the negative-limit case promised by the docstring.tests/unit/test_search_engine.py (1)
281-289: Strengthencount_onlyexpectation with a deterministic count assertion.This test currently only checks that
countexists. Since the fixture content is fixed, asserting the expected value will catch search regressions earlier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_search_engine.py` around lines 281 - 289, The test_count_only_mode currently only asserts that "count" exists; update it to assert the deterministic expected integer count for the fixed temp_file fixture when calling search_content(temp_file, "error", fuzzy=False, count_only=True). Replace or add to the existing assertion with assert result["count"] == <expected_count> (the known number of matches in the temp_file fixture) while keeping the other boolean assertions (fuzzy_enabled, regex_enabled, case_sensitive, inverted) unchanged; this ensures test_count_only_mode fails if search_content's counting logic regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tools.py`:
- Around line 371-379: The code currently validates offset/limit/mode by raising
ToolError but still returns a dict with an "error" key when pattern is missing;
change that behavior to raise a ToolError instead of returning an error dict.
Locate the pattern-check branches (the one near the shown validation block and
the similar branch referenced at lines 416-425) and replace the dict-return path
with raise ToolError(f"pattern must be provided") or a descriptive ToolError
including the invalid/missing pattern; keep existing ToolError usage consistent
with the offset/limit/mode checks (use ToolError and the same message style).
- Around line 591-597: In revert_edit, failure handling is inconsistent: an
unknown backup_id currently results in success=False and raw filesystem
exceptions can bubble; change both to use the same public-tool failure path by
raising ToolError with descriptive messages. Specifically, in the revert_edit
function, after calling list_backups(file_path) and when validating backup_id,
raise ToolError("Unknown backup id: {backup_id}") instead of returning a
success=False dict, and wrap any filesystem operations (open/read/write/remove)
in try/except to catch OSError/IOError and re-raise as ToolError with context
(include file_path and backup_id where relevant). Ensure all early checks (file
existence, empty backups) already raise ToolError and follow the same pattern so
revert_edit consistently surfaces failures via ToolError.
In `@tests/integration/test_mcp_server.py`:
- Around line 41-42: Replace direct access to the private tool registry
(mcp._tool_manager._tools) with the public FastMCP API: call mcp.get_tools() to
get the list/iterable of tool metadata and build tool_names from that, and use
mcp.get_tool(name) when the test needs to inspect a specific tool's metadata
(name, description, output_schema, annotations). Update the assertions at the
locations that read _tool_manager._tools (lines referenced) to use set(t.name
for t in mcp.get_tools()) or similar, and replace any direct lookups into _tools
with mcp.get_tool(tool_name) to fetch metadata synchronously via the public
methods.
---
Nitpick comments:
In `@src/editor.py`:
- Around line 13-14: The module-level logger variable created by
logging.getLogger(__name__) is defined but never used; either remove the unused
logger declaration or use it for relevant runtime messages—e.g., replace silent
exceptions/prints in functions in this module with
logger.debug/info/warning/error calls or add a short initialization log like
logger.debug("editor module loaded")—update the logger variable (named logger)
accordingly throughout the module or delete the unused declaration.
In `@src/file_access.py`:
- Around line 15-16: The module currently defines logger =
logging.getLogger(__name__) but never uses it; either remove the logging import
and the logger variable or add appropriate log statements (e.g.,
info/debug/error) in key functions such as any file write/backup-related
functions (use logger.info(...) on successful writes, logger.error(...) on
exceptions, and logger.debug(...) for detailed state) so the logger symbol is
actually referenced; update imports accordingly and ensure exception handlers
call logger.exception where applicable.
In `@tests/integration/test_documentation_workflows.py`:
- Around line 156-164: Update the
test_read_content_tail_mode_requires_positive_limit to assert both zero and
negative limits: parametrize the test (or loop) so it runs with limit=0 and
limit=-1 and in each case call read_content(str(doc), limit=limit, mode="tail")
inside pytest.raises(ToolError, match="limit must be >= 1"); ensure the test
still skips early if the doc file doesn't exist and retains the same error match
and mode to cover the negative-limit case promised by the docstring.
In `@tests/unit/test_file_access.py`:
- Around line 1124-1126: Replace the bare early return used to skip the test on
Windows with pytest.skip so the test run records a skipped test; in
tests/unit/test_file_access.py update the sys.platform == "win32" branch to call
pytest.skip("Skipping on Windows where chmod doesn't restrict owner writes
reliably") and ensure pytest is imported in the file (add import pytest if
missing), leaving the rest of the test intact.
In `@tests/unit/test_search_engine.py`:
- Around line 281-289: The test_count_only_mode currently only asserts that
"count" exists; update it to assert the deterministic expected integer count for
the fixed temp_file fixture when calling search_content(temp_file, "error",
fuzzy=False, count_only=True). Replace or add to the existing assertion with
assert result["count"] == <expected_count> (the known number of matches in the
temp_file fixture) while keeping the other boolean assertions (fuzzy_enabled,
regex_enabled, case_sensitive, inverted) unchanged; this ensures
test_count_only_mode fails if search_content's counting logic regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77008338-f1b8-43af-b2ad-cefb4737cf9c
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
pyproject.tomlsrc/config.pysrc/editor.pysrc/file_access.pysrc/main.pysrc/mcp_schemas.pysrc/server.pysrc/tools.pytests/integration/test_directory_workflows.pytests/integration/test_documentation_workflows.pytests/integration/test_mcp_server.pytests/integration/test_search_directory_workflows.pytests/unit/test_editor.pytests/unit/test_file_access.pytests/unit/test_revert_edit.pytests/unit/test_search_engine.pytests/unit/test_tools.pytests/unit/test_tools_list_directory.pytests/unit/test_tools_search_directory.py
💤 Files with no reviewable changes (1)
- src/mcp_schemas.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
==========================================
+ Coverage 89.03% 89.86% +0.82%
==========================================
Files 12 11 -1
Lines 1478 1490 +12
==========================================
+ Hits 1316 1339 +23
+ Misses 162 151 -11
🚀 New features to boost your workflow:
|
…I in tests - read_content pattern-not-found now raises ToolError instead of returning error dict - revert_edit unknown backup_id now raises ToolError instead of returning success=False - revert_edit filesystem operations wrapped in try/except for PermissionError/OSError - MCP server tests use list_tools()/get_tool() instead of private _tools dict
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools.py (2)
416-423:⚠️ Potential issue | 🟡 MinorThe retry hint is misleading in this branch.
Line 418 already does a fuzzy search, so telling callers to switch to
search_content“for fuzzy matching” points them at a capability they already used.✏️ Suggested patch
if not matches: raise ToolError( f"Pattern '{pattern}' not found in file. " - f"Try a different search term or use search_content for fuzzy matching." + "Try a different search term, or use search_content to explore the file first." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.py` around lines 416 - 423, The error message when no matches are found after calling search_file(canonical_path, pattern, fuzzy=True) is misleading; update the ToolError raised in that branch to remove the suggestion to use search_content for fuzzy matching and instead provide an accurate hint (e.g., suggest a different search term or turning off fuzzy matching) so callers aren’t directed to a capability already used by search_file; locate the block that calls search_file(...) and raises ToolError and replace the message text accordingly.
215-223:⚠️ Potential issue | 🟠 MajorPreserve the existing search defaults or treat this as a breaking API change.
These public tool signatures now default to case-insensitive matching, and
search_content()also returns less context by default. Any caller that omits those fields will start getting different results after what otherwise looks like an MCP SDK refactor.↩️ Suggested patch
def search_content( absolute_file_path: str, pattern: str, max_results: int = 20, - context_lines: int = 2, + context_lines: int = 3, fuzzy: bool = True, regex: bool = False, - case_sensitive: bool = False, + case_sensitive: bool = True, invert: bool = False, count_only: bool = False, ) -> dict: @@ - - case_sensitive: Case sensitive search (default False) + - case_sensitive: Case sensitive search (default True) @@ - if context_lines != 2: + if context_lines != 3: warnings.append("context_lines ignored in count_only mode") @@ def search_directory( @@ - case_sensitive: bool = False, + case_sensitive: bool = True, @@ - case_sensitive: Case-sensitive search (default False). + case_sensitive: Case-sensitive search (default True).Also applies to: 239-240, 251-255, 864-873, 895-896
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.py` around lines 215 - 223, search_content() and other public tool function signatures were changed to default to case-insensitive and reduced context, which is a breaking change; revert the defaults to the original behavior by setting search_content's parameters back to the previous defaults (restore case_sensitive=True and context_lines to the previous value), and apply the same restoration to the other public tool signatures referenced in the review (the functions around the other noted ranges) so callers receive the same results as before. Ensure you only change the default parameter values (e.g., in search_content) and do not alter the function signatures or behavior otherwise.
🧹 Nitpick comments (1)
tests/unit/test_revert_edit.py (1)
126-176: Add coverage for the new revert-copy failure branch.These tests cover the early
ToolErrorcases, but they never hit the newPermissionError/OSErrorhandling around the actual restore insrc/tools.py. A regression there would still leak a raw filesystem exception unnoticed.🧪 Suggested patch
class TestRevertErrors: @@ def test_revert_nonexistent_file(self): """Raises ToolError for missing file.""" with tempfile.TemporaryDirectory() as backup_dir: with patch("src.file_access.config") as mock_config: mock_config.backup_dir = backup_dir with patch("src.tools.config", mock_config): with pytest.raises(ToolError, match="does not exist"): revert_edit("/nonexistent/path/file.txt") + + def test_revert_copy_failure_raises_tool_error(self): + """Raises ToolError when restoring the backup fails.""" + with tempfile.NamedTemporaryFile(mode="w", delete=False, suffix=".txt") as f: + f.write("content") + temp_path = f.name + + with tempfile.TemporaryDirectory() as backup_dir: + with patch("src.file_access.config") as mock_config: + mock_config.backup_dir = backup_dir + mock_config.max_backups = 10 + mock_config.memory_threshold = 50 * 1024 * 1024 + + create_backup(temp_path) + Path(temp_path).write_text("modified") + + with patch("src.tools.config", mock_config), patch( + "src.tools.shutil.copy2", side_effect=PermissionError("denied") + ): + with pytest.raises(ToolError, match="Failed to revert file"): + revert_edit(temp_path) + + Path(temp_path).unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_revert_edit.py` around lines 126 - 176, Add a test that exercises the restore I/O failure branch by creating a real backup (use create_backup(temp_path)), then patching the filesystem copy/restore routine used by revert_edit (e.g., patch "shutil.copyfile" or "pathlib.Path.replace" depending on which src/tools.py calls) to raise PermissionError and/or OSError, call revert_edit(target_path) and assert it raises a ToolError (not the raw PermissionError/OSError) with an appropriate failure message; ensure you still set up mock_config.backup_dir/max_backups as in other tests and clean up the temp file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/tools.py`:
- Around line 416-423: The error message when no matches are found after calling
search_file(canonical_path, pattern, fuzzy=True) is misleading; update the
ToolError raised in that branch to remove the suggestion to use search_content
for fuzzy matching and instead provide an accurate hint (e.g., suggest a
different search term or turning off fuzzy matching) so callers aren’t directed
to a capability already used by search_file; locate the block that calls
search_file(...) and raises ToolError and replace the message text accordingly.
- Around line 215-223: search_content() and other public tool function
signatures were changed to default to case-insensitive and reduced context,
which is a breaking change; revert the defaults to the original behavior by
setting search_content's parameters back to the previous defaults (restore
case_sensitive=True and context_lines to the previous value), and apply the same
restoration to the other public tool signatures referenced in the review (the
functions around the other noted ranges) so callers receive the same results as
before. Ensure you only change the default parameter values (e.g., in
search_content) and do not alter the function signatures or behavior otherwise.
---
Nitpick comments:
In `@tests/unit/test_revert_edit.py`:
- Around line 126-176: Add a test that exercises the restore I/O failure branch
by creating a real backup (use create_backup(temp_path)), then patching the
filesystem copy/restore routine used by revert_edit (e.g., patch
"shutil.copyfile" or "pathlib.Path.replace" depending on which src/tools.py
calls) to raise PermissionError and/or OSError, call revert_edit(target_path)
and assert it raises a ToolError (not the raw PermissionError/OSError) with an
appropriate failure message; ensure you still set up
mock_config.backup_dir/max_backups as in other tests and clean up the temp file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b242477-7010-4eef-8887-1337edbef6e1
📒 Files selected for processing (4)
src/tools.pytests/integration/test_mcp_server.pytests/unit/test_revert_edit.pytests/unit/test_tools.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_tools.py
- tests/integration/test_mcp_server.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Python MCP SDK has evolved significantly since the MCP server was implemented. The addition of FastMCP into the SDK simplifies a lot of complexity previously manually implemented server. This change updates the MCP server to adopt the newer SDK features.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests