Improve file navigation for code review tools - #10
Merged
Conversation
Outline symbols can now be classified as added, modified, or removed based on changed line ranges. This is the data model and algorithm layer; tool integration follows in phase 2.
The get_overview tool now accepts an optional changed_lines parameter that marks which outline symbols overlap with changed line ranges. When omitted, behavior is identical to before; when provided, each outline item includes a changes field and the response includes a changed_symbols count.
Update get_overview docstring so FastMCP auto-generates the correct schema for the new changed_lines parameter. Update API.md, design.md, and examples.md with parameter details, response fields, and a diff-aware code review workflow example.
Forward the changed_lines parameter from the MCP get_overview tool definition to the underlying tools.get_overview function, making diff-aware overview accessible to MCP clients.
Walks up the AST from a target line to find the nearest enclosing function, class, or method definition, with configurable depth for navigating nested scopes.
Wire up the tree-sitter enclosing definition lookup as a registered MCP tool with fallback to a centered context window for unsupported languages or top-level code.
Exercises the full tool path for enclosing definition lookup and fallback context window on non-code files.
Contributor
📝 WalkthroughWalkthroughAdds diff-awareness: Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant GetOverviewTool as get_overview
participant ChangeMarker as Change\ Marker
participant TreeParser as Tree\ Parser
Client->>Server: get_overview(file_path, changed_lines)
Server->>GetOverviewTool: get_overview(file_path, changed_lines)
GetOverviewTool->>TreeParser: build outline(file_path)
TreeParser-->>GetOverviewTool: outline items
GetOverviewTool->>ChangeMarker: parse_changed_lines(raw)
ChangeMarker-->>GetOverviewTool: changed_ranges
GetOverviewTool->>ChangeMarker: mark_outline(outline, changed_ranges)
ChangeMarker-->>GetOverviewTool: marked_outline, changed_symbols_count
GetOverviewTool-->>Server: {outline: marked_outline, changed_symbols: N}
Server-->>Client: response
sequenceDiagram
actor Client
participant Server
participant ReadEnclosing as read_enclosing
participant TreeParser as Tree\ Parser
Client->>Server: read_enclosing(file_path, line, depth)
Server->>ReadEnclosing: read_enclosing(file_path, line, depth)
ReadEnclosing->>TreeParser: find_enclosing_definition(file_path, content, line, depth)
alt definition found
TreeParser-->>ReadEnclosing: (text, start, end, label)
ReadEnclosing-->>Server: {mode: "enclosing", enclosing_symbol: label, content, start_line, end_line}
else not found
TreeParser-->>ReadEnclosing: None
ReadEnclosing-->>Server: {mode: "context_window", enclosing_symbol: null, content, start_line, end_line}
end
Server-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 89.86% 89.43% -0.44%
==========================================
Files 11 12 +1
Lines 1490 1675 +185
==========================================
+ Hits 1339 1498 +159
- Misses 151 177 +26
🚀 New features to boost your workflow:
|
![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: 2
🧹 Nitpick comments (4)
src/server.py (1)
194-207: Add upper bounds todepthandcontext_linesto prevent oversized requests.Line 194 and Line 201 currently allow unbounded values; adding
leconstraints improves resilience under malformed or extreme inputs.🛡️ Suggested guardrails
depth: Annotated[ int, Field( description="Nesting depth: 1 = innermost definition, 2 = parent (e.g., class containing a method)", ge=1, + le=20, ), ] = 1, context_lines: Annotated[ int, Field( description="Lines of context for fallback window when no enclosing definition is found", ge=1, + le=500, ), ] = 40,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.py` around lines 194 - 207, The depth and context_lines Annotated Field definitions currently lack upper bounds; update the Field(...) calls for the symbols depth and context_lines to include appropriate le (<=) constraints (e.g., set depth Field(..., ge=1, le=10) and context_lines Field(..., ge=1, le=200)) to prevent oversized requests, keeping the existing descriptions and defaults unchanged; modify the Annotated declarations for depth and context_lines accordingly so validation rejects extreme inputs at model parse time.docs/examples.md (1)
554-557: Filter changed symbols recursively, not only top-level items.Line 554 currently checks only
overview["outline"], which can skip nested changed symbols (e.g., methods inside classes) in this workflow example.♻️ Suggested doc snippet update
-# Step 3: Focus review on changed functions -changed = [s for s in overview["outline"] if s["changes"]] -for symbol in changed: +def flatten(items): + for item in items: + yield item + yield from flatten(item.get("children", [])) + +# Step 3: Focus review on changed symbols (including nested ones) +changed = [s for s in flatten(overview["outline"]) if s.get("changes")] +for symbol in changed: code = read_content("/path/to/src/auth.py", offset=symbol["line_number"], limit=symbol["line_count"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/examples.md` around lines 554 - 557, The example filters changed symbols only at the top level using overview["outline"], which misses nested items; update the logic that builds changed (the variable `changed`) to traverse `overview["outline"]` recursively (e.g., a small helper that walks nested children) so nested changed symbols (methods inside classes) are included before calling `read_content` with `symbol["line_number"]` and `symbol["line_count"]`; ensure the recursion returns the same symbol dicts used by `read_content` so existing calls to `read_content("/path/to/src/auth.py", offset=symbol["line_number"], limit=symbol["line_count"])` continue to work.src/tools.py (1)
568-570: Fallback window should backfill near EOF to honor requested size.At Line 568–Line 570, when the target is near file end,
lines_returnedcan be much smaller thancontext_lineseven if earlier lines are available. Consider shiftingstartbackward after clampingend.♻️ Suggested refactor
- start = max(1, line - context_lines // 2) - end = min(total_lines, start + context_lines - 1) + start = max(1, line - context_lines // 2) + end = min(total_lines, start + context_lines - 1) + # Backfill from above when we're clipped at EOF + if end - start + 1 < context_lines: + start = max(1, end - context_lines + 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools.py` around lines 568 - 570, The context window near EOF can be too small because only `start` is clamped downward; after computing `end = min(total_lines, start + context_lines - 1)` you should backfill by moving `start` backward when the window is short: if (end - start + 1) < context_lines then set start = max(1, end - context_lines + 1) before building `content_text`; update the `content_text = "".join(lines[start - 1 : end])` usage accordingly and ensure you keep the 1-based to 0-based conversion for slicing consistent with the `lines` list.tests/unit/test_tree_parser.py (1)
484-490: Make fixture loading path-robust.Line 484 and Line 485 rely on CWD-sensitive string paths (
"tests/test_data/..."). That can fail in some runners. Resolve from__file__instead to keep tests deterministic.♻️ Suggested refactor
-class TestFindEnclosingDefinition: +class TestFindEnclosingDefinition: """Tests for find_enclosing_definition().""" - PYTHON_FIXTURE = "tests/test_data/python/nested_class.py" - JS_FIXTURE = "tests/test_data/javascript/nested_arrow.js" + _TEST_DATA_DIR = Path(__file__).resolve().parents[1] / "test_data" + PYTHON_FIXTURE = _TEST_DATA_DIR / "python" / "nested_class.py" + JS_FIXTURE = _TEST_DATA_DIR / "javascript" / "nested_arrow.js" `@staticmethod` - def _read_fixture(path: str) -> str: - with open(path) as f: - return f.read() + def _read_fixture(path: Path) -> str: + return path.read_text(encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_tree_parser.py` around lines 484 - 490, PYTHON_FIXTURE and JS_FIXTURE use CWD-dependent string paths; update fixture loading so paths are resolved relative to the test file using __file__. Modify the _read_fixture function (and keep using PYTHON_FIXTURE/JS_FIXTURE) to resolve the incoming path against Path(__file__).parent (or the test module directory) before opening/reading, e.g., convert the path to a pathlib.Path and do parent_dir.joinpath(path).read_text(); ensure references to _read_fixture, PYTHON_FIXTURE, and JS_FIXTURE are preserved.
🤖 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/change_marker.py`:
- Around line 39-42: The current validation in changed_lines accepts booleans
because isinstance(True, int) is True; update the check in the function/method
that validates start and end (the block that currently does "if not
isinstance(start, int) or not isinstance(end, int):") to reject booleans
explicitly by requiring the exact int type (e.g., use type(start) is int and
type(end) is int or add explicit "isinstance(x, bool)" guards), and keep the
ValueError raised with a clear message referencing the actual types provided.
In `@tests/integration/test_get_overview_diff.py`:
- Around line 111-130: The test
test_get_overview_with_changed_lines_simple_outline can pass vacuously because
the .txt content may produce no outline items; update the test to guarantee at
least one simple-outline item and then assert its changes marking: modify the
tmp_path file content (used by get_overview) to include a header or marker that
your simple-outline parser recognizes (so outline is non-empty), call
get_overview(str(txt_file), changed_lines=[[1,2,"modified"]]) as before, assert
"changed_symbols" in result, assert result["outline"] is not empty, and then for
at least one specific item in result["outline"] verify the "changes" key exists
and reflects the modified lines; reference the test function name
test_get_overview_with_changed_lines_simple_outline and the get_overview/outline
keys when locating where to change the test.
---
Nitpick comments:
In `@docs/examples.md`:
- Around line 554-557: The example filters changed symbols only at the top level
using overview["outline"], which misses nested items; update the logic that
builds changed (the variable `changed`) to traverse `overview["outline"]`
recursively (e.g., a small helper that walks nested children) so nested changed
symbols (methods inside classes) are included before calling `read_content` with
`symbol["line_number"]` and `symbol["line_count"]`; ensure the recursion returns
the same symbol dicts used by `read_content` so existing calls to
`read_content("/path/to/src/auth.py", offset=symbol["line_number"],
limit=symbol["line_count"])` continue to work.
In `@src/server.py`:
- Around line 194-207: The depth and context_lines Annotated Field definitions
currently lack upper bounds; update the Field(...) calls for the symbols depth
and context_lines to include appropriate le (<=) constraints (e.g., set depth
Field(..., ge=1, le=10) and context_lines Field(..., ge=1, le=200)) to prevent
oversized requests, keeping the existing descriptions and defaults unchanged;
modify the Annotated declarations for depth and context_lines accordingly so
validation rejects extreme inputs at model parse time.
In `@src/tools.py`:
- Around line 568-570: The context window near EOF can be too small because only
`start` is clamped downward; after computing `end = min(total_lines, start +
context_lines - 1)` you should backfill by moving `start` backward when the
window is short: if (end - start + 1) < context_lines then set start = max(1,
end - context_lines + 1) before building `content_text`; update the
`content_text = "".join(lines[start - 1 : end])` usage accordingly and ensure
you keep the 1-based to 0-based conversion for slicing consistent with the
`lines` list.
In `@tests/unit/test_tree_parser.py`:
- Around line 484-490: PYTHON_FIXTURE and JS_FIXTURE use CWD-dependent string
paths; update fixture loading so paths are resolved relative to the test file
using __file__. Modify the _read_fixture function (and keep using
PYTHON_FIXTURE/JS_FIXTURE) to resolve the incoming path against
Path(__file__).parent (or the test module directory) before opening/reading,
e.g., convert the path to a pathlib.Path and do
parent_dir.joinpath(path).read_text(); ensure references to _read_fixture,
PYTHON_FIXTURE, and JS_FIXTURE are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0432fb1-d593-40bb-b44a-c6a67f560b5d
📒 Files selected for processing (17)
README.mddocs/API.mddocs/design.mddocs/examples.mdsrc/change_marker.pysrc/data_models.pysrc/server.pysrc/tools.pysrc/tree_parser.pytests/integration/test_get_overview_diff.pytests/integration/test_mcp_server.pytests/integration/test_read_enclosing.pytests/test_data/javascript/nested_arrow.jstests/test_data/python/nested_class.pytests/unit/test_change_marker.pytests/unit/test_tools.pytests/unit/test_tree_parser.py
Comment on lines
+39
to
+42
| if not isinstance(start, int) or not isinstance(end, int): | ||
| raise ValueError( | ||
| f"start and end must be integers, got {type(start).__name__} and {type(end).__name__}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Reject boolean line numbers in changed_lines validation.
At Line 39–Line 42, isinstance(..., int) accepts True/False. That can silently treat booleans as line numbers (1/0) and misclassify changes. Use strict int checks.
🛡️ Suggested fix
- if not isinstance(start, int) or not isinstance(end, int):
+ if type(start) is not int or type(end) is not int:
raise ValueError(
f"start and end must be integers, got {type(start).__name__} and {type(end).__name__}"
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/change_marker.py` around lines 39 - 42, The current validation in
changed_lines accepts booleans because isinstance(True, int) is True; update the
check in the function/method that validates start and end (the block that
currently does "if not isinstance(start, int) or not isinstance(end, int):") to
reject booleans explicitly by requiring the exact int type (e.g., use
type(start) is int and type(end) is int or add explicit "isinstance(x, bool)"
guards), and keep the ValueError raised with a clear message referencing the
actual types provided.
Fix vacuous simple-outline test, add EOF backfill for context window, and make test fixture paths CWD-independent.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/integration/test_get_overview_diff.py (1)
111-135:⚠️ Potential issue | 🟡 MinorMake the simple-outline assertion verify the intended item, not just “any changed”.
Line 135can still pass if a different outline item is incorrectly marked. Assert that theSection Onesymbol (lines 1–2) is specifically"modified".Proposed test tightening
- # At least one item should actually be marked as changed - assert any(item["changes"] is not None for item in result["outline"]) + section_one_items = [i for i in result["outline"] if "Section One" in i["name"]] + assert section_one_items, "Section One should appear in the simple outline" + assert section_one_items[0]["changes"] == "modified"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_get_overview_diff.py` around lines 111 - 135, The test test_get_overview_with_changed_lines_simple_outline currently only asserts that any outline item is marked changed; instead, locate the outline item that corresponds to "Section One" (created by get_overview when calling with changed_lines=[[1,2,"modified"]]) by matching its name/text or its start line/range in result["outline"], and assert that this specific item's "changes" field equals "modified" (while keeping the other existing assertions). Use the test's get_overview call and the outline items in result["outline"] to identify the correct symbol and verify its changes value.
🧹 Nitpick comments (1)
tests/integration/test_get_overview_diff.py (1)
141-163: Consider parameterizing invalid-input cases to reduce repetition.The three negative tests are solid; combining them with
pytest.mark.parametrizewould keep this suite easier to extend.Optional refactor sketch
- def test_get_overview_invalid_changed_lines_bad_type(self, tmp_path: Path): - ... - def test_get_overview_invalid_changed_lines_start_gt_end(self, tmp_path: Path): - ... - def test_get_overview_invalid_changed_lines_empty_inner(self, tmp_path: Path): - ... + `@pytest.mark.parametrize`( + "changed_lines", + [ + ["bad"], # type: ignore[list-item] + [[10, 5]], + [[]], # type: ignore[list-item] + ], + ) + def test_get_overview_invalid_changed_lines(self, tmp_path: Path, changed_lines): + py_file = tmp_path / "err.py" + py_file.write_text("x = 1\n") + with pytest.raises(ToolError, match="Invalid changed_lines"): + get_overview(str(py_file), changed_lines=changed_lines)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_get_overview_diff.py` around lines 141 - 163, Replace the three repetitive tests (test_get_overview_invalid_changed_lines_bad_type, test_get_overview_invalid_changed_lines_start_gt_end, test_get_overview_invalid_changed_lines_empty_inner) with a single parameterized test using pytest.mark.parametrize that iterates over the invalid changed_lines values (e.g., ["bad"], [[10, 5]], [[]]) and asserts with pytest.raises(ToolError, match="Invalid changed_lines") when calling get_overview(file_path, changed_lines=...); keep each case as a separate param entry (with optional ids) and ensure the tmp_path fixture still creates the file before calling get_overview.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/integration/test_get_overview_diff.py`:
- Around line 111-135: The test
test_get_overview_with_changed_lines_simple_outline currently only asserts that
any outline item is marked changed; instead, locate the outline item that
corresponds to "Section One" (created by get_overview when calling with
changed_lines=[[1,2,"modified"]]) by matching its name/text or its start
line/range in result["outline"], and assert that this specific item's "changes"
field equals "modified" (while keeping the other existing assertions). Use the
test's get_overview call and the outline items in result["outline"] to identify
the correct symbol and verify its changes value.
---
Nitpick comments:
In `@tests/integration/test_get_overview_diff.py`:
- Around line 141-163: Replace the three repetitive tests
(test_get_overview_invalid_changed_lines_bad_type,
test_get_overview_invalid_changed_lines_start_gt_end,
test_get_overview_invalid_changed_lines_empty_inner) with a single parameterized
test using pytest.mark.parametrize that iterates over the invalid changed_lines
values (e.g., ["bad"], [[10, 5]], [[]]) and asserts with
pytest.raises(ToolError, match="Invalid changed_lines") when calling
get_overview(file_path, changed_lines=...); keep each case as a separate param
entry (with optional ids) and ensure the tmp_path fixture still creates the file
before calling get_overview.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 453122b0-e54d-4959-b715-6688b75284f5
📒 Files selected for processing (4)
src/tools.pytests/integration/test_get_overview_diff.pytests/unit/test_tools.pytests/unit/test_tree_parser.py
✅ Files skipped from review due to trivial changes (1)
- tests/unit/test_tree_parser.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_tools.py
- src/tools.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.
Code review tools that use largefile mcp need to work around reading changed content. They currently have to call get_overview then manually correlate diff ranges with outline symbols, and estimate line ranges to read relevant definitions. This adds inbuilt capabilities for better navigation:
changed_linesaccepts diff ranges (for example as available from diffchunk) and classifies each symbol in the outline, so review tools can immediately focus on what changed without a second pass.read_enclosinguses tree-sitter to walk up the AST and return the complete enclosing definition for any line. Supports configurable nesting depth (e.g. depth=2 for the class containing a method) with a context-window fallback for unsupported languages.Summary by CodeRabbit