fix: handle list_files for paths inside ignored directories #8047
Conversation
- Add checkPathForIgnoredSegments helper to detect if a path contains any ignored directory segments - Modify buildRecursiveArgs to treat paths inside ignored directories (e.g., node_modules/@types/) the same as explicitly targeting those directories - Apply same logic to buildNonRecursiveArgs for consistency - Add comprehensive tests for node_modules/@types scenarios This fixes the issue where list_files would return only directories when targeting paths inside ignored ancestors like node_modules/@types/stream-json. Fixes #4263
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const normalizedPath = path.normalize(dirPath) | ||
| // Split by separator and filter out empty parts | ||
| // This handles cases like trailing slashes, multiple separators, etc. | ||
| const pathParts = normalizedPath.split(path.sep).filter((part) => part.length > 0) |
There was a problem hiding this comment.
Is this handling Windows paths correctly? The use of path.sep should work cross-platform, but paths like C:\projects\node_modules\@types might benefit from additional testing. Consider adding a Windows-specific test case to ensure the normalization handles backslashes properly.
| * @param dirPath - The directory path to check | ||
| * @returns Object with flags indicating if path contains ignored segments | ||
| */ | ||
| function checkPathForIgnoredSegments(dirPath: string): { |
There was a problem hiding this comment.
Performance consideration: For deeply nested paths, this function could be called multiple times during a single operation. Would it be worth memoizing the result for the same path during a single listFiles call?
| const { hasHiddenSegment, ignoredSegment } = checkPathForIgnoredSegments(dirPath) | ||
|
|
||
| // If path contains ignored segments, use special handling | ||
| if (hasHiddenSegment || ignoredSegment) { |
There was a problem hiding this comment.
Could we extract this condition to a variable for better readability, similar to how it's done in buildRecursiveArgs()? For example:
| if (hasHiddenSegment || ignoredSegment) { | |
| const shouldApplySpecialHandling = hasHiddenSegment || ignoredSegment; | |
| if (shouldApplySpecialHandling) { | |
| args.push("--no-ignore-vcs") | |
| args.push("--no-ignore") | |
| } |
| } | ||
| }), | ||
| kill: vi.fn(), | ||
| } |
There was a problem hiding this comment.
Would it be valuable to add a test case for paths containing multiple different ignored directories? For example, node_modules/package/dist/bundle to ensure the logic correctly handles the first ignored segment and doesn't get confused by subsequent ones.
| .filter((f) => f.endsWith("/")) | ||
| .map((f) => { | ||
| const parts = f.split("/") | ||
| return parts[parts.length - 2] + "/" |
There was a problem hiding this comment.
This test name could be more specific. Consider: "should not apply node_modules exclusion when path contains node_modules as a segment" to better describe what's being tested.
| } | ||
|
|
||
| /** | ||
| * Check if a path contains any ignored directory segment |
There was a problem hiding this comment.
Documentation enhancement: This helper function could benefit from an example in its JSDoc comment showing what it returns for a path like node_modules/@types/node.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing this PR as the issue it addresses doesn't actually exist. Testing confirms that The issue title 'Bug: list_files tool only shows top-level empty directories in non-git workspaces' is incorrect. The actual behavior described in the issue (problems with paths inside Since the reported bug doesn't exist and the PR addresses a non-existent problem based on a misunderstanding of the issue, this PR should be closed. |
This PR addresses Issue #4263 where
list_fileswould return only directories when targeting paths inside ignored ancestors likenode_modules/@types/stream-json.Problem
When
list_filestargets a path that lives under a directory inDIRS_TO_IGNORE(e.g.,node_modules), ripgrep exclusions were applied globally, causing ripgrep to return no files. The fallback scanner only added directories, leading to "only directories" in results.Solution
The fix treats paths "inside an ignored ancestor" the same as explicitly targeting that ignored directory:
checkPathForIgnoredSegments()helper - Detects if any segment in the path is inDIRS_TO_IGNOREbuildRecursiveArgs()- Checks for ignored segments in the entire path (not just basename), skips exclusion patterns for directories that are part of the target path, and applies--no-ignore-vcsand--no-ignoreflags when inside ignored pathsbuildNonRecursiveArgs()- Applies the same logic for non-recursive operationsnode_modules, nested paths inside ignored directories, and paths with multiple ignored segmentsTesting
node_modules/@typesscenarios ✅References
This is my attempt to address the issue based on the detailed analysis provided. Feedback and guidance are welcome!
Important
Fixes
list_filesto handle paths inside ignored directories by adjusting ripgrep arguments and adding tests.list_filesto return files inside ignored directories by modifyingbuildRecursiveArgs()andbuildNonRecursiveArgs()to skip exclusion patterns for directories in the target path.checkPathForIgnoredSegments()to detect ignored segments in paths.node_modules, nested ignored directories, and paths with multiple ignored segments inlist-files.spec.ts.list-files.tsto apply--no-ignore-vcsand--no-ignoreflags when inside ignored paths.This description was created by
for f3cf443. You can customize this summary. It will automatically update as commits are pushed.