Code Review Tools usage experience improvements#14
Merged
peteretelej merged 11 commits intomainfrom Apr 9, 2026
Merged
Conversation
Plumbs a format string parameter through the MCP and tools layers with raw as the default. Adds FormatMode enum, a formatter module with placeholder branches for annotated/compact, and validation that rejects unknown values.
Implements the annotated output mode that parses raw diff content into structured sections with file headers, line-numbered new hunks, and old hunks for removed code. Includes function context from @@ headers.
Compact mode shows only new hunks (context + added lines with new-file line numbers), omitting removed lines and old hunk sections entirely. Reuses the annotated format's hunk parsing infrastructure.
The load_diff response now includes a files_excluded count showing how many files were removed by exclude_patterns.
Strips excess context lines from diff hunks at load time, reducing token usage. Applied per-file before chunking via reduce_context() in the parser.
Integration and edge case tests covering format mode composition, context_lines reduction, and exclude_patterns interaction. Updated design.md and README with format options documentation.
list_chunks now returns per-chunk token_count and total_token_count, enabling consuming skills to budget context windows. Uses a chars/4 heuristic with no new dependencies.
Update all test files to handle the new list_chunks dict response shape, add unit and integration tests for token estimation, and document the token_count field in README and design docs.
📝 WalkthroughWalkthroughAdds per-chunk token estimation and a total token count, a context-line reduction option for diffs, and chunk formatting modes (raw/annotated/compact). Threads Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Tools as DiffChunkTools
participant Chunker as DiffChunker
participant Parser as DiffParser
participant Formatter
participant Session
Client->>Tools: load_diff(path, exclude_patterns, context_lines)
Tools->>Chunker: chunk_diff(content, context_lines=N)
Chunker->>Parser: reduce_context(file_diff, context_lines=N)
Parser-->>Chunker: reduced_diff
Chunker->>Session: create chunks from reduced_diff
Chunker->>Session: update stats (files_excluded)
Session-->>Tools: chunks metadata
Tools->>Session: estimate_tokens per chunk
Tools-->>Client: {chunks, total_token_count, files_excluded}
Client->>Tools: get_chunk(file, num, format="annotated")
Tools->>Session: fetch raw chunk
Session-->>Tools: raw chunk
rect rgba(200,150,100,0.5)
Tools->>Formatter: format_chunk(content, ANNOTATED, files)
Formatter->>Parser: parse file sections and hunks
Parser-->>Formatter: structured sections
Formatter-->>Tools: annotated output
end
Tools-->>Client: formatted chunk content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 Generate unit tests (beta)
Comment |
peteretelej
changed the title
![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)
src/chunker.py (1)
152-153: Consider settingfiles_excludedwithinupdate_stats()for better encapsulation.Currently
files_excludedis set afterupdate_stats()completes, which works but creates a subtle dependency on the call order. This is a minor cohesion concern - passing the count toupdate_stats()would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/chunker.py` around lines 152 - 153, The code sets session.stats.files_excluded after calling session.update_stats(), creating a call-order dependency; change update_stats to accept a files_excluded parameter (e.g., update_stats(self, files_excluded=0) or similar) and assign session.stats.files_excluded inside that method, then update the call site here (replace session.update_stats() with session.update_stats(files_excluded_count)) and any other callers to pass the count so the stats update is encapsulated within update_stats().src/parser.py (1)
340-344: Redundant if/else branch can be simplified.Both branches append the same
textvalue, making the conditional unnecessary.♻️ Suggested simplification
for lt, text, _, _ in sub: - if lt != "meta": - hunk_lines.append(text) - else: - hunk_lines.append(text) + hunk_lines.append(text)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/parser.py` around lines 340 - 344, The loop over "sub" contains a redundant conditional: both branches append the same "text" to "hunk_lines"; remove the if/else and always append "text" (i.e., replace the conditional block with a single hunk_lines.append(text)) to simplify the code in the loop that iterates "for lt, text, _, _ in sub".tests/test_integration.py (1)
308-311: Accessing internal members for testing is acceptable here but worth noting.The test accesses
tools._get_file_key()andtools.sessions[]which are implementation details. This is fine for integration testing, but be aware these tests will break if the internal structure changes. Consider adding a comment noting this is an intentional white-box test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_integration.py` around lines 308 - 311, This test intentionally reaches into internals by calling tools._get_file_key(react_diff_file) and indexing tools.sessions[file_key] to call session.get_chunk_info(1); add a short inline comment above these lines indicating this is a deliberate white-box/integration check that depends on internal structure (so future maintainers understand it may break if internals change) and keep the existing calls as-is; reference the symbols _get_file_key, sessions, get_chunk_info and react_diff_file in the comment to make intent explicit.
🤖 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/formatter.py`:
- Around line 79-83: The flush logic in _flush_file currently only appends
FileSection when current_hunks is non-empty, dropping rename-only/binary/no-hunk
file sections; update _flush_file to always append a FileSection (or a distinct
RawFileSection/passthrough marker) when current_path is set even if
current_hunks is empty so those files are carried through to output (preserve
current_path, current_hunks, in_hunk semantics), and make the same change to the
analogous flush/append branch later in the file that currently skips empty-hunk
files so list_chunks() and get_chunk(..., format="annotated"|"compact") remain
consistent.
---
Nitpick comments:
In `@src/chunker.py`:
- Around line 152-153: The code sets session.stats.files_excluded after calling
session.update_stats(), creating a call-order dependency; change update_stats to
accept a files_excluded parameter (e.g., update_stats(self, files_excluded=0) or
similar) and assign session.stats.files_excluded inside that method, then update
the call site here (replace session.update_stats() with
session.update_stats(files_excluded_count)) and any other callers to pass the
count so the stats update is encapsulated within update_stats().
In `@src/parser.py`:
- Around line 340-344: The loop over "sub" contains a redundant conditional:
both branches append the same "text" to "hunk_lines"; remove the if/else and
always append "text" (i.e., replace the conditional block with a single
hunk_lines.append(text)) to simplify the code in the loop that iterates "for lt,
text, _, _ in sub".
In `@tests/test_integration.py`:
- Around line 308-311: This test intentionally reaches into internals by calling
tools._get_file_key(react_diff_file) and indexing tools.sessions[file_key] to
call session.get_chunk_info(1); add a short inline comment above these lines
indicating this is a deliberate white-box/integration check that depends on
internal structure (so future maintainers understand it may break if internals
change) and keep the existing calls as-is; reference the symbols _get_file_key,
sessions, get_chunk_info and react_diff_file in the comment to make intent
explicit.
🪄 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: 091f245a-a6b8-459f-9eda-f80437658076
📒 Files selected for processing (13)
README.mddocs/design.mdsrc/chunker.pysrc/formatter.pysrc/models.pysrc/parser.pysrc/server.pysrc/tools.pytests/test_case_insensitive.pytests/test_filesystem_edge_cases.pytests/test_get_file_diff.pytests/test_integration.pytests/test_mcp_components.py
Rename ambiguous variable 'l' to 'ln' in test_mcp_components.py (E741). Align pre-push hook with CI by linting the full repo, not just src/.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
==========================================
+ Coverage 91.54% 94.07% +2.53%
==========================================
Files 7 8 +1
Lines 461 743 +282
==========================================
+ Hits 422 699 +277
- Misses 39 44 +5 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_mcp_components.py (2)
840-859: Consider deduplicating repeated fixtures across test classes.
test_data_dir,react_diff_file, andgo_diff_fileare repeated in multiple classes. A shared module-level fixture set would reduce duplication and keep skip behavior consistent.Also applies to: 1149-1165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_components.py` around lines 840 - 859, Move the duplicated pytest fixtures into a shared module-level fixture set and update tests to use them: extract the functions test_data_dir, go_diff_file, and react_diff_file into a top-level (module or conftest) fixture module so they are defined once (retain the same names and behavior, including the pytest.skip logic) and remove the repeated definitions from individual test classes; update imports/usage so tests reference the single fixtures (test_data_dir, go_diff_file, react_diff_file) to ensure consistent skip behavior across tests.
315-320: Avoid strict wall-clock thresholds in this unit test.
list_time < 2.0is CI-environment sensitive and can produce intermittent failures unrelated to correctness. Consider moving hard timing checks to perf/benchmark tests and keep this test focused on functional assertions.Proposed adjustment
- assert list_time < 2.0, f"List chunks took too long: {list_time}s" - assert len(list_result["chunks"]) == result["chunks"] + # Keep unit test deterministic: verify behavior, not machine-dependent timing. + assert len(list_result["chunks"]) == result["chunks"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_components.py` around lines 315 - 320, The test currently enforces a strict wall-clock assertion on list_chunks by computing list_time and asserting list_time < 2.0; remove or disable this timing assertion and keep the functional checks (i.e., retain the call to tools.list_chunks(go_diff_file) and the assert on len(list_result["chunks"]) == result["chunks"]). If you still need performance coverage, move the timing check into a separate perf/benchmark test (or change the assertion to a non-failing warning/log) so the unit test remains deterministic and CI-stable; target the symbols list_chunks, list_result, list_time and the timing assert for the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_mcp_components.py`:
- Around line 1083-1087: The test test_context_lines_negative_raises_valueerror
uses a non-existent path which can cause file-existence validation to trigger
before the context_lines validation; update the test to pass a valid diff file
to DiffChunkTools.load_diff (e.g., create a minimal valid diff via a fixture or
tmp_path and pass that path) so the call to DiffChunkTools.load_diff(...)
exercises the context_lines validation path with context_lines=-1; reference
DiffChunkTools and its load_diff method when making this change.
---
Nitpick comments:
In `@tests/test_mcp_components.py`:
- Around line 840-859: Move the duplicated pytest fixtures into a shared
module-level fixture set and update tests to use them: extract the functions
test_data_dir, go_diff_file, and react_diff_file into a top-level (module or
conftest) fixture module so they are defined once (retain the same names and
behavior, including the pytest.skip logic) and remove the repeated definitions
from individual test classes; update imports/usage so tests reference the single
fixtures (test_data_dir, go_diff_file, react_diff_file) to ensure consistent
skip behavior across tests.
- Around line 315-320: The test currently enforces a strict wall-clock assertion
on list_chunks by computing list_time and asserting list_time < 2.0; remove or
disable this timing assertion and keep the functional checks (i.e., retain the
call to tools.list_chunks(go_diff_file) and the assert on
len(list_result["chunks"]) == result["chunks"]). If you still need performance
coverage, move the timing check into a separate perf/benchmark test (or change
the assertion to a non-failing warning/log) so the unit test remains
deterministic and CI-stable; target the symbols list_chunks, list_result,
list_time and the timing assert for the change.
🪄 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: 46873719-9020-47da-938c-24c8e308a7d7
📒 Files selected for processing (1)
tests/test_mcp_components.py
Preserve no-hunk file sections (binary, rename) in formatted output, remove dead code in parser, and harden test assertions.
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 (1)
tests/test_mcp_components.py (1)
47-57:⚠️ Potential issue | 🟡 MinorAdd per-chunk
token_countassertions to cover the fulllist_chunkscontract.The test validates
total_token_countbut not each chunk’stoken_count, which is part of this PR’s budgeting feature surface.Suggested test hardening
assert "chunks" in result assert "total_token_count" in result assert isinstance(result["total_token_count"], int) chunks = result["chunks"] assert len(chunks) == total_chunks assert all("chunk" in chunk for chunk in chunks) + assert all("token_count" in chunk for chunk in chunks) + assert all( + isinstance(chunk["token_count"], int) and chunk["token_count"] >= 0 + for chunk in chunks + ) assert all("files" in chunk for chunk in chunks) assert all("lines" in chunk for chunk in chunks) assert all("summary" in chunk for chunk in chunks) + assert sum(chunk["token_count"] for chunk in chunks) == result["total_token_count"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_components.py` around lines 47 - 57, The test for tools.list_chunks is missing per-chunk token_count checks; update the test in tests/test_mcp_components.py after calling tools.list_chunks and obtaining chunks to assert each chunk contains a "token_count" key, that chunk["token_count"] is an int (or non-negative int), and that the sum of all chunk["token_count"] values equals result["total_token_count"]; use the existing variables result, chunks and total_chunks and keep the other chunk assertions intact.
🧹 Nitpick comments (2)
tests/test_mcp_components.py (2)
980-988:test_reduce_context_none_keeps_allis mislabeled for what it actually verifies.Line 987 calls
DiffParser.reduce_context(diff, 100), so this test checks “large context value keeps all lines,” not thecontext_lines=Nonepath inDiffChunkTools.load_diff(...).Minimal clarity fix
- def test_reduce_context_none_keeps_all(self): - """context_lines=None (not calling reduce_context) keeps all context.""" + def test_reduce_context_large_value_keeps_all(self): + """Large context_lines value preserves all available context lines."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_components.py` around lines 980 - 988, The test name/test intent mismatch: test_reduce_context_none_keeps_all actually calls DiffParser.reduce_context(diff, 100) which verifies that a large numeric context keeps lines, not the context_lines=None branch in DiffChunkTools.load_diff; either rename the test to reflect "large context keeps all" (e.g., test_reduce_context_large_keeps_all) or change the test to exercise the None path by invoking DiffChunkTools.load_diff(..., context_lines=None) (or calling the method that dispatches to that behavior) and asserting that all context lines are kept; update references to DiffParser.reduce_context and DiffChunkTools.load_diff accordingly so the test name and implementation match.
533-539: Line-number assertions are too permissive ("6" in ln/"22" in ln).These can pass on incorrect values like
16or122. Prefer exact leading-number checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mcp_components.py` around lines 533 - 539, The assertions that check line numbers are too permissive because they use substring checks ("6" in ln / "22" in ln); update the checks around add1_lines and add2_lines to assert the line number exactly at the start of the line (e.g., use ln.startswith("6") or a regex like re.match(r'^\s*6\b', ln) for add1_lines and similarly for 22 for add2_lines) so that "16" or "122" won't match; locate and modify the assertions referencing add1_lines, add2_lines and lines in the test to perform exact leading-number matching.
🤖 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 `@tests/test_mcp_components.py`:
- Around line 47-57: The test for tools.list_chunks is missing per-chunk
token_count checks; update the test in tests/test_mcp_components.py after
calling tools.list_chunks and obtaining chunks to assert each chunk contains a
"token_count" key, that chunk["token_count"] is an int (or non-negative int),
and that the sum of all chunk["token_count"] values equals
result["total_token_count"]; use the existing variables result, chunks and
total_chunks and keep the other chunk assertions intact.
---
Nitpick comments:
In `@tests/test_mcp_components.py`:
- Around line 980-988: The test name/test intent mismatch:
test_reduce_context_none_keeps_all actually calls
DiffParser.reduce_context(diff, 100) which verifies that a large numeric context
keeps lines, not the context_lines=None branch in DiffChunkTools.load_diff;
either rename the test to reflect "large context keeps all" (e.g.,
test_reduce_context_large_keeps_all) or change the test to exercise the None
path by invoking DiffChunkTools.load_diff(..., context_lines=None) (or calling
the method that dispatches to that behavior) and asserting that all context
lines are kept; update references to DiffParser.reduce_context and
DiffChunkTools.load_diff accordingly so the test name and implementation match.
- Around line 533-539: The assertions that check line numbers are too permissive
because they use substring checks ("6" in ln / "22" in ln); update the checks
around add1_lines and add2_lines to assert the line number exactly at the start
of the line (e.g., use ln.startswith("6") or a regex like re.match(r'^\s*6\b',
ln) for add1_lines and similarly for 22 for add2_lines) so that "16" or "122"
won't match; locate and modify the assertions referencing add1_lines, add2_lines
and lines in the test to perform exact leading-number matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 817e3d7e-9607-437d-9ebb-faa65ef3e7ae
📒 Files selected for processing (3)
src/formatter.pysrc/parser.pytests/test_mcp_components.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/formatter.py
- src/parser.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.
AI code review tools that consume diffchunk need to fit diffs into limited context windows. These changes give them three levers: choose a compact format that drops removed-code hunks, reduce context lines at load time, and see token estimates before deciding which chunks to fetch.
raw, annotated, compact) toget_chunkso review agents can request token-efficient or structured representations of diffscontext_linesparameter toload_difffor stripping excess context at load time, and surfacefiles_excludedcount so agents know what was filteredper-chunkandtotaltoken count estimates tolist_chunksso agents can budget context windows before fetching chunk contentSummary by CodeRabbit
New Features
Documentation
Tests
Chores