Merged
Conversation
Replace the class-based DiffChunkServer with module-level FastMCP wiring, removing the manual tool dispatcher and isError suppression. Tool annotations and structured_output=False are set on all tools.
Pass max_chunk_lines as a parameter instead of mutating shared state, add structured logging to stderr, and make error messages actionable for LLM self-correction.
Rewrite server-layer tests to exercise the real MCP protocol path, testing tool discovery, annotations, error propagation with isError, and resource access through create_connected_server_and_client_session.
Make pattern matching case-insensitive in find_chunks_for_files and get_file_diff, add .gitattributes for LF line endings in test data, and update docs/design.md to reflect FastMCP architecture.
The old test_handle_call_tool and test_server_error_handling files tested the manual dispatcher that was replaced by FastMCP. Equivalent coverage now exists in the protocol-level tests.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces class-based MCP server with FastMCP module-level tools/resources, adds per-call max_chunk_lines for chunking, makes file-pattern matching case-insensitive, adds logging/timing and improved error messages, tightens MCP dependency range, updates docs, and adjusts tests (removals and expectation changes). Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastMCP as mcp (FastMCP)
participant Tools as DiffChunkTools
participant FS as FileSystem
Client->>FastMCP: Call tool (e.g., load_diff / find_chunks_for_files)
FastMCP->>Tools: Invoke tool function with params (path, max_chunk_lines, patterns)
Tools->>FS: Read diff file / list files
FS-->>Tools: Return diff content / file list
Tools-->>FastMCP: Return JSON or raw string result
FastMCP-->>Client: Return tool response (success or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/test_case_insensitive.py (2)
49-56: Test doesn't verify uppercase directory behavior as intended.Both
all_chunksandupper_alluse the identical pattern"*". This test effectively verifies that the same pattern returns the same result, not that uppercase patterns work correctly.Consider using a directory-based pattern like
"SRC/*"vs"src/*"to actually test case-insensitive directory matching.♻️ Proposed fix
def test_find_chunks_uppercase_directory(self, tools, react_diff_file): """find_chunks_for_files with uppercase dir pattern matches.""" tools.load_diff(react_diff_file) - all_chunks = tools.find_chunks_for_files(react_diff_file, "*") - assert len(all_chunks) > 0 - # Uppercase wildcard should match the same set - upper_all = tools.find_chunks_for_files(react_diff_file, "*") - assert all_chunks == upper_all + # Test case-insensitive directory matching if applicable + lower_chunks = tools.find_chunks_for_files(react_diff_file, "packages/*") + upper_chunks = tools.find_chunks_for_files(react_diff_file, "PACKAGES/*") + assert lower_chunks == upper_chunks🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_case_insensitive.py` around lines 49 - 56, The test test_find_chunks_uppercase_directory currently calls tools.find_chunks_for_files twice with the same pattern "*", so it doesn't verify case-insensitive directory matching; update the second call to use an uppercase directory pattern (e.g., "SRC/*" or the uppercase variant of the directory in react_diff_file) when invoking tools.find_chunks_for_files so the assertion all_chunks == upper_all actually verifies that an uppercase directory pattern matches the same set of chunks as the lowercase pattern.
80-93: Potential test fragility if first file has no extension.The test assumes
chunks[0]["files"][0]contains a file with an extension. If the first file lacks an extension, the test silently skips the assertion without validating case-insensitive glob behavior.Additionally, if the uppercase pattern matches multiple files,
get_file_diffwill raise aValueErrorfor ambiguous matches.♻️ Proposed fix to make the test more robust
def test_get_file_diff_case_insensitive_glob(self, tools, react_diff_file): """get_file_diff glob matching is case-insensitive.""" tools.load_diff(react_diff_file) chunks = tools.list_chunks(react_diff_file) - # Find a file that has a unique extension to use as glob pattern - file_name = chunks[0]["files"][0] + # Find a file with an extension for the test + file_name = None + for chunk in chunks: + for f in chunk["files"]: + if "." in f: + file_name = f + break + if file_name: + break + + if not file_name: + pytest.skip("No file with extension found in diff") # If the file has an extension, try matching with different case - if "." in file_name: - base, ext = file_name.rsplit(".", 1) - upper_pattern = f"{base.upper()}.{ext.upper()}" - result = tools.get_file_diff(react_diff_file, upper_pattern) - assert "diff --git" in result + base, ext = file_name.rsplit(".", 1) + upper_pattern = f"{base.upper()}.{ext.upper()}" + result = tools.get_file_diff(react_diff_file, upper_pattern) + assert "diff --git" in result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_case_insensitive.py` around lines 80 - 93, The test assumes the first file has an extension and is unique; make it robust by scanning all files from tools.list_chunks(react_diff_file) (use chunks = tools.list_chunks(...) and iterate chunks[*]["files"]) to find a file_name that contains a "." and whose uppercase pattern is unique among all files when uppercased, then build upper_pattern = f"{base.upper()}.{ext.upper()}" and call tools.get_file_diff(react_diff_file, upper_pattern) and assert "diff --git" in the result; if no such uniquely-matchable file is found, explicitly fail the test (raise AssertionError) so the test doesn't silently skip.src/server.py (1)
53-53: Consider shortening the docstring.The docstring is quite lengthy (single line with ~500+ characters). While comprehensive guidance for LLMs is valuable, this might be better structured as a shorter docstring with key points, potentially moving detailed usage notes to the design documentation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.py` at line 53, Replace the overly long triple-quoted docstring that begins "Parse and load a diff file with custom chunking settings..." with a concise 1–2 sentence summary stating the function's purpose and the critical constraints (e.g., must use absolute path; use diffchunk tools for large diffs); move the detailed usage notes and examples into the project design/README docs and update the docstring to end with a short "See design docs for details" pointer so callers get the essentials quickly while preserving full guidance elsewhere.
🤖 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/server.py`:
- Line 30: Import ToolAnnotations from mcp.types and replace the five
occurrences of annotations={"readOnlyHint": True} (the annotations argument in
the `@mcp.tool` decorator usages in this module) with a ToolAnnotations instance,
e.g. annotations=ToolAnnotations(readOnlyHint=True), so the decorator argument
matches the declared type; ensure you add the import line (from mcp.types import
ToolAnnotations) and update each `@mcp.tool`(...) decorator accordingly.
---
Nitpick comments:
In `@src/server.py`:
- Line 53: Replace the overly long triple-quoted docstring that begins "Parse
and load a diff file with custom chunking settings..." with a concise 1–2
sentence summary stating the function's purpose and the critical constraints
(e.g., must use absolute path; use diffchunk tools for large diffs); move the
detailed usage notes and examples into the project design/README docs and update
the docstring to end with a short "See design docs for details" pointer so
callers get the essentials quickly while preserving full guidance elsewhere.
In `@tests/test_case_insensitive.py`:
- Around line 49-56: The test test_find_chunks_uppercase_directory currently
calls tools.find_chunks_for_files twice with the same pattern "*", so it doesn't
verify case-insensitive directory matching; update the second call to use an
uppercase directory pattern (e.g., "SRC/*" or the uppercase variant of the
directory in react_diff_file) when invoking tools.find_chunks_for_files so the
assertion all_chunks == upper_all actually verifies that an uppercase directory
pattern matches the same set of chunks as the lowercase pattern.
- Around line 80-93: The test assumes the first file has an extension and is
unique; make it robust by scanning all files from
tools.list_chunks(react_diff_file) (use chunks = tools.list_chunks(...) and
iterate chunks[*]["files"]) to find a file_name that contains a "." and whose
uppercase pattern is unique among all files when uppercased, then build
upper_pattern = f"{base.upper()}.{ext.upper()}" and call
tools.get_file_diff(react_diff_file, upper_pattern) and assert "diff --git" in
the result; if no such uniquely-matchable file is found, explicitly fail the
test (raise AssertionError) so the test doesn't silently skip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05e40a3c-7fa2-4922-909a-45d15c6a180a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.gitattributesdocs/design.mdpyproject.tomlsrc/chunker.pysrc/main.pysrc/models.pysrc/parser.pysrc/server.pysrc/tools.pytests/test_case_insensitive.pytests/test_filesystem_edge_cases.pytests/test_get_file_diff.pytests/test_handle_call_tool.pytests/test_integration.pytests/test_mcp_components.pytests/test_server_error_handling.py
💤 Files with no reviewable changes (3)
- tests/test_server_error_handling.py
- tests/test_mcp_components.py
- tests/test_handle_call_tool.py
Use ToolAnnotations model instead of plain dicts for the annotations parameter on @mcp.tool decorators.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/server.py`:
- Line 22: The global instantiation tools = DiffChunkTools() creates shared
mutable state across requests; instead instantiate DiffChunkTools per
client/session and remove the module-global tools. Locate where request handling
or session lifecycle is created (the server request handler or session
initializer) and create a new DiffChunkTools() there, pass that instance into
handlers/functions that currently reference the global tools (or attach it to
the request/session context), and ensure any code using current_overview/tool
calls reads from the per-session instance rather than the removed global
variable.
- Line 54: The docstrings that state "CRITICAL: You must use an absolute
directory path - relative paths will fail." should be changed to recommend
absolute paths instead of requiring them; update the occurrences matching the
shown docstring text (the long diff-file parsing description) and the other
docstrings at the same wording (lines referenced) to read something like
"absolute path recommended" or "an absolute directory path is recommended;
relative paths may still work." Keep the rest of the guidance intact and, if you
prefer stronger enforcement instead of wording changes, add an explicit runtime
check (os.path.isabs) in the relevant loader/parsing functions and raise a clear
ValueError to match documentation—otherwise only change the docstring text in
each affected docstring occurrence.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/chunker.py`:
- Around line 23-26: The current assignment "max_chunk_lines = max_chunk_lines
or self.max_chunk_lines" treats 0 as falsey and incorrectly falls back to
self.max_chunk_lines; change it to use an explicit None check (if
max_chunk_lines is None: max_chunk_lines = self.max_chunk_lines) and then
validate the resulting value (ensure it's an int and greater than 0, otherwise
raise a ValueError) for the method that accepts the parameter max_chunk_lines so
per-call 0 is not silently ignored.
In `@src/tools.py`:
- Around line 109-115: The logger.info call currently logs the full
absolute_file_path which can leak local paths; change the info-level message in
the function that calls logger.info (the block using absolute_file_path and
session.stats) to log a sanitized identifier instead (e.g.,
os.path.basename(absolute_file_path) or session.id) and move the full
absolute_file_path to a logger.debug message; update the info call text to
include basename/session id and keep session.stats.chunks_count,
session.stats.total_files and elapsed unchanged, and add a separate logger.debug
that logs the full absolute_file_path for troubleshooting.
- Around line 221-240: The current matching logic loses case-distinct exact
filenames; change the sequence so you first try a case-sensitive exact match
(compare f == file_path) and if that yields results use it, otherwise fall back
to the existing case-insensitive exact match (f.lower() == file_path.lower())
and then the case-insensitive glob (fnmatch.fnmatch(f.lower(),
file_path.lower())). Update the block that builds matches (the variables
matches, all_files, file_path and the fnmatch call) so a true exact
(case-sensitive) match is preserved before any case-insensitive fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25ddbbdb-6fec-4b2b-8bd0-2c500b6f6167
📒 Files selected for processing (3)
src/chunker.pysrc/tools.pytests/test_case_insensitive.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_case_insensitive.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
==========================================
- Coverage 94.11% 91.54% -2.58%
==========================================
Files 7 7
Lines 459 461 +2
==========================================
- Hits 432 422 -10
- Misses 27 39 +12 🚀 New features to boost your workflow:
|
…le matching - Use explicit None check for max_chunk_lines instead of truthy check that swallows 0 - Add case-sensitive exact match before case-insensitive fallback in get_file_diff
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/tools.py (1)
109-115:⚠️ Potential issue | 🟡 MinorAvoid logging full local paths at
INFOlevel.Line 111 logs the caller-supplied
absolute_file_pathat INFO level on every load. For an MCP service, this can expose usernames and workspace structure from local absolute paths. Consider logging a sanitized identifier (e.g., basename) at INFO and keeping the full path at DEBUG.Suggested fix
logger.info( "Parsed and chunked %s: %d chunks, %d files in %.2fs", - absolute_file_path, + os.path.basename(resolved_file_path), session.stats.chunks_count, session.stats.total_files, elapsed, ) + logger.debug("Full path: %s", resolved_file_path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.py` around lines 109 - 115, The info-level log in logger.info currently prints the caller-supplied absolute_file_path (and may leak local usernames/paths); change it to log a sanitized identifier (e.g., the basename) instead and move the full absolute_file_path to a debug-level log. Concretely, replace usage of absolute_file_path in the info message with a sanitized name derived from absolute_file_path (e.g., os.path.basename or Path(...).name) and add a separate logger.debug that includes the full absolute_file_path for troubleshooting; keep the existing session.stats and elapsed variables unchanged in the messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/tools.py`:
- Around line 109-115: The info-level log in logger.info currently prints the
caller-supplied absolute_file_path (and may leak local usernames/paths); change
it to log a sanitized identifier (e.g., the basename) instead and move the full
absolute_file_path to a debug-level log. Concretely, replace usage of
absolute_file_path in the info message with a sanitized name derived from
absolute_file_path (e.g., os.path.basename or Path(...).name) and add a separate
logger.debug that includes the full absolute_file_path for troubleshooting; keep
the existing session.stats and elapsed variables unchanged in the messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0e9fff8-7df1-49f0-b7e8-3725c25d65b1
📒 Files selected for processing (2)
src/chunker.pysrc/tools.py
Flush stderr prints before mcp.run() to ensure startup messages are captured on Windows. Increase test timeouts for slower CI environments. Log only basename at INFO level to avoid leaking user paths.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/server.py (1)
22-29:⚠️ Potential issue | 🟠 MajorProcess-global
DiffChunkToolsstill shares one cache across all clients.
current_overview()and every tool use the sameDiffChunkTools.sessionsmap, so loaded diffs can leak between MCP clients and concurrent requests mutate shared state. ScopeDiffChunkToolsto the connection/session unless this server is explicitly single-tenant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.py` around lines 22 - 29, The issue is that a module-level DiffChunkTools instance (tools) shares DiffChunkTools.sessions across all clients causing state leakage; remove the global "tools" and instead create or attach a DiffChunkTools per connection/session: either instantiate DiffChunkTools inside the handler (current_overview) or, better, create it during connection setup and store it on the MCP connection/context (use FastMCP connection/session hook or attach to the mcp object) and then reference that per-connection instance in current_overview and all resource handlers so DiffChunkTools.sessions is scoped to each client rather than shared globally.src/tools.py (1)
77-80:⚠️ Potential issue | 🟡 MinorDon't require absolute paths in the error text unless the loader enforces it.
Line 69 already normalizes relative inputs with
expanduser()andrealpath(), so this message currently tells users a supported path form is invalid. Either add an explicitos.path.isabs()check or soften the wording to “absolute path recommended.”Suggested wording
- f"Diff file not found: {absolute_file_path}. Verify the file exists and the path is absolute." + f"Diff file not found: {absolute_file_path}. Verify the file exists and the path is correct."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.py` around lines 77 - 80, The error message incorrectly demands an absolute path despite normalizing inputs with expanduser()/realpath(); update the ValueError raised where resolved_file_path and absolute_file_path are used to either (a) first check os.path.isabs(absolute_file_path) and raise a distinct, explicit error about requiring an absolute path, or (b) more simply soften the existing message to something like "Diff file not found: {absolute_file_path}. Verify the file exists (absolute path recommended)." Modify the ValueError text accordingly and keep the check on resolved_file_path the same.
🧹 Nitpick comments (1)
src/server.py (1)
15-19: Avoid configuring the root logger when this module is imported.
src.mainimportssrc.serverbefore parsing args, so every CLI path—and any embedding consumer—gets this global logging setup as a side effect. Move it into the CLI entry point, or only configure logging when no handlers are already attached.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.py` around lines 15 - 19, The module currently calls logging.basicConfig at import time which configures the root logger as a side effect; remove or guard that call in src.server so importing the module doesn't change global logging: either move the logging.basicConfig invocation into the CLI entry point (where args are parsed) or wrap it so it only runs when no handlers exist (e.g., check logging.getLogger().handlers or use if not logging.getLogger().hasHandlers()) before calling logging.basicConfig; update any tests or startup code that relied on the import-time configuration accordingly.
🤖 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/server.py`:
- Line 95: The file has unformatted long logger.info calls (e.g.,
logger.info("get_chunk: %s chunk=%d", os.path.basename(absolute_file_path),
chunk_number) and similar calls at the other locations) causing the ruff format
check to fail; run ruff format (or your project's formatter) on src/server.py
and rewrap/format the long logger.info calls so they conform to the project's
line-length/formatting rules (ensure the calls in the get_chunk logging and the
two other logger.info occurrences are line-broken or reformatted by the
formatter).
---
Duplicate comments:
In `@src/server.py`:
- Around line 22-29: The issue is that a module-level DiffChunkTools instance
(tools) shares DiffChunkTools.sessions across all clients causing state leakage;
remove the global "tools" and instead create or attach a DiffChunkTools per
connection/session: either instantiate DiffChunkTools inside the handler
(current_overview) or, better, create it during connection setup and store it on
the MCP connection/context (use FastMCP connection/session hook or attach to the
mcp object) and then reference that per-connection instance in current_overview
and all resource handlers so DiffChunkTools.sessions is scoped to each client
rather than shared globally.
In `@src/tools.py`:
- Around line 77-80: The error message incorrectly demands an absolute path
despite normalizing inputs with expanduser()/realpath(); update the ValueError
raised where resolved_file_path and absolute_file_path are used to either (a)
first check os.path.isabs(absolute_file_path) and raise a distinct, explicit
error about requiring an absolute path, or (b) more simply soften the existing
message to something like "Diff file not found: {absolute_file_path}. Verify the
file exists (absolute path recommended)." Modify the ValueError text accordingly
and keep the check on resolved_file_path the same.
---
Nitpick comments:
In `@src/server.py`:
- Around line 15-19: The module currently calls logging.basicConfig at import
time which configures the root logger as a side effect; remove or guard that
call in src.server so importing the module doesn't change global logging: either
move the logging.basicConfig invocation into the CLI entry point (where args are
parsed) or wrap it so it only runs when no handlers exist (e.g., check
logging.getLogger().handlers or use if not logging.getLogger().hasHandlers())
before calling logging.basicConfig; update any tests or startup code that relied
on the import-time configuration accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7f24515-331a-49e6-994b-0a71f3f929a3
📒 Files selected for processing (4)
src/main.pysrc/server.pysrc/tools.pytests/test_cli_integration.py
Replace bash script with minimal sh-compatible hook targeting src/ and tests/ directories.
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
Documentation
Chores
Tests