-
-
Notifications
You must be signed in to change notification settings - Fork 558
Add recursive depth dir listing #250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughImplements depth-limited recursive directory listing with optional depth parameter, updates schema to accept depth with default 2, passes depth through handler, and expands server tool description. Handler also records execution time and returns joined result text. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Client
participant H as filesys handler
participant S as Schema parser
participant T as listDirectory(depth)
participant FS as File System
U->>H: invoke list_directory { path, depth? }
H->>S: parse({ path, depth? })
S-->>H: { path, depth=2 }
rect rgba(220,240,255,0.5)
note over H: Start timer
H->>T: listDirectory(path, depth)
loop Depth-limited traversal (d > 0)
T->>FS: readdir(path)
alt readable
FS-->>T: entries
T-->>T: accumulate [DIR]/[FILE] relative paths
opt subdirectories and depth>1
T->>T: recurse(child, depth-1)
end
else permission denied
T-->>T: record "[DENIED] path"
end
end
end
T-->>H: string[] entries
H-->>H: resultText = entries.join("\n")
note over H: Stop timer
H-->>U: { content: resultText }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/handlers/filesystem-handlers.ts (1)
212-215: Remove unuseddurationvariable.The
durationvariable is calculated but never used in the response or for logging. This is dead code that should be removed to improve code clarity.Apply this diff to remove the unused variable:
export async function handleListDirectory(args: unknown): Promise<ServerResult> { try { - const startTime = Date.now(); const parsed = ListDirectoryArgsSchema.parse(args); const entries = await listDirectory(parsed.path, parsed.depth); - const duration = Date.now() - startTime; const resultText = entries.join('\n');Alternatively, if timing information is valuable for debugging or telemetry, consider logging it:
export async function handleListDirectory(args: unknown): Promise<ServerResult> { try { const startTime = Date.now(); const parsed = ListDirectoryArgsSchema.parse(args); const entries = await listDirectory(parsed.path, parsed.depth); const duration = Date.now() - startTime; + + // Log performance metrics for deep recursive listings + if (parsed.depth > 2 || entries.length > 100) { + console.log(`Directory listing took ${duration}ms (depth: ${parsed.depth}, entries: ${entries.length})`); + } const resultText = entries.join('\n');src/tools/filesystem.ts (1)
904-906: Consider clarifying the display path for root directory access errors.When the root directory itself cannot be read, the fallback uses
path.basename(currentPath), which may not provide enough context. For example, if listing/var/logfails due to permissions, it would show[DENIED] lograther than[DENIED] /var/log.Consider this refinement:
} catch (error) { // If we can't read this directory (permission denied), show as denied - const displayPath = relativePath || path.basename(currentPath); + const displayPath = relativePath || currentPath; results.push(`[DENIED] ${displayPath}`); return; }This provides fuller context when the initial directory is inaccessible, while still using relative paths for nested failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
src/handlers/filesystem-handlers.ts(1 hunks)src/server.ts(1 hunks)src/tools/filesystem.ts(1 hunks)src/tools/schemas.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/handlers/filesystem-handlers.ts (2)
src/tools/schemas.ts (1)
ListDirectoryArgsSchema(64-67)src/tools/filesystem.ts (1)
listDirectory(893-934)
src/tools/filesystem.ts (1)
test/test-home-directory.js (1)
entries(224-224)
🔇 Additional comments (6)
src/tools/schemas.ts (1)
66-66: LGTM!The depth parameter is properly defined with a sensible default value of 2, which aligns with the implementation in
src/tools/filesystem.ts.src/handlers/filesystem-handlers.ts (1)
214-220: LGTM!The handler correctly passes the depth parameter to
listDirectoryand properly formats the response using the joined entries.src/server.ts (1)
327-340: LGTM!The expanded documentation clearly explains the recursive listing behavior with the depth parameter, provides useful examples, and accurately describes the output format including the [DENIED] marker for inaccessible directories.
src/tools/filesystem.ts (3)
893-893: LGTM!The function signature correctly adds the depth parameter with a sensible default of 2, maintaining backward compatibility.
910-929: LGTM!The recursive logic is sound:
- Correctly constructs relative paths for display
- Adds entries in the right format ([DIR]/[FILE] prefix)
- Validates paths before recursing to prevent unauthorized access
- Properly handles recursion depth with
currentDepth - 1- Uses
continueon validation errors to avoid breaking the entire listing
897-908: Verify depth=0 behavior & add tests if neededRipgrep didn’t find any tests covering
listRecursivewithdepth=0. Please confirm whether zero-depth traversal should be allowed or if you’d prefer enforcing a minimum depth of 1 in your API schema, and add tests for this edge case.
Summary by CodeRabbit