fix: minor cleanup fixes for new tools - #8
Conversation
Only skip entries matching ignored_dir_patterns when they are actually directories, surface PermissionError at root depth instead of returning silent empty results, and narrow an overly broad except clause in search_directory to catch only FileAccessError. - Added minimum: 1 to max_depth and max_entries in the list_directory JSON schema
Corrected the "Java-specific nodes" comment to include TypeScript, replaced low-signal Java search hints (void, public, private) with structural tokens (enum, import, @interface), and removed a dead _make_tree helper from list_directory tests.
Added the missing list_directory detailed section to API.md. Fixed several inaccuracies in search_directory docs: `directory` key renamed to `path`, added `include_pattern` to return keys, removed non-existent `match_count` from per-file results. - Fixed double-pipe typo in API.md overview table - Added trailing newline to configuration.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
==========================================
+ Coverage 89.02% 89.03% +0.01%
==========================================
Files 12 12
Lines 1476 1478 +2
==========================================
+ Hits 1314 1316 +2
Misses 162 162
🚀 New features to boost your workflow:
|
|
@coderabbitai you asleep? |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR introduces documentation for a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_tools_list_directory.py (1)
309-313: Make nested PermissionError injection path-based instead of call-order-based.Using
call_count["n"] == 3is fragile and can break with harmless internal traversal changes.♻️ Suggested deterministic test tweak
- call_count = {"n": 0} - def patched_scandir(path: str) -> object: - call_count["n"] += 1 - if call_count["n"] == 3: # third call: recursing into restricted/ + if Path(path).name == "restricted": raise PermissionError("access denied") return real_scandir(path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_tools_list_directory.py` around lines 309 - 313, The patched_scandir stub currently raises PermissionError based on call_count["n"] == 3 which is brittle; change it to inspect the incoming path instead (e.g., check Path(path).name or whether "restricted" is in the path) and raise PermissionError only when the stub is asked to list that nested restricted directory, while still delegating to real_scandir(path) for other paths and optionally incrementing call_count for diagnostics. Locate the patched_scandir definition and replace the call-order condition with a deterministic path-based check (referencing patched_scandir, call_count, and real_scandir).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/API.md`:
- Line 415: Update the sentence to explicitly state that the ignore patterns
(LARGEFILE_IGNORED_DIR_PATTERNS) apply only to directory names (not files);
reword the line about ignored defaults to something like “__pycache__,
node_modules, and .git are ignored by default from directory traversal
(configurable via LARGEFILE_IGNORED_DIR_PATTERNS)” to match runtime behavior
where the code only skips names after confirming they are directories (the check
uses Path.is_dir / is_dir logic in the traversal implementation).
---
Nitpick comments:
In `@tests/unit/test_tools_list_directory.py`:
- Around line 309-313: The patched_scandir stub currently raises PermissionError
based on call_count["n"] == 3 which is brittle; change it to inspect the
incoming path instead (e.g., check Path(path).name or whether "restricted" is in
the path) and raise PermissionError only when the stub is asked to list that
nested restricted directory, while still delegating to real_scandir(path) for
other paths and optionally incrementing call_count for diagnostics. Locate the
patched_scandir definition and replace the call-order condition with a
deterministic path-based check (referencing patched_scandir, call_count, and
real_scandir).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21356dab-eee8-4ae6-aa55-591c2af03b04
📒 Files selected for processing (8)
docs/API.mddocs/configuration.mdpyproject.tomlsrc/mcp_schemas.pysrc/tools.pysrc/tree_parser.pytests/unit/test_tools_list_directory.pytests/unit/test_tools_search_directory.py
Summary by CodeRabbit
New Features
Improvements