-
Notifications
You must be signed in to change notification settings - Fork 9.1k
Rename "search_files" to "search_files_by_name" #897
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
base: main
Are you sure you want to change the base?
Rename "search_files" to "search_files_by_name" #897
Conversation
8b48040 to
eaa92e1
Compare
|
Another typically test scenario is when refactoring code. I've noticed Claude typically tries to read a file if the user has provided the file name, as in the example below:
If the file is too large, then the LLM tries to search content within the file(s). Previously, it was invoking the
This makes it possible to have better discovery of a code base, e.g.: View result from list_allowed_directories from filesystem (local) |
eaa92e1 to
8277da8
Compare
|
I've noticed sometimes Claude invokes I improved the description of the |
8277da8 to
58595de
Compare
4b1f2bb to
cc4103f
Compare
cc4103f to
8c36bff
Compare
|
I've noticed that Claude AI often tries to invoke the search functions with a regex, most likely because the name of the input argument is "pattern". I've added a |
b8ea5dc to
63e1d59
Compare
ced817a to
f668adc
Compare
f668adc to
44e2d41
Compare
|
Thanks for this, I came here with the same issue that you've solved here. I was going to suggest |
af8476e to
3259d90
Compare
I've used this for over a month and it generally works well. I don't mind changing to "find_files" and "grep_files". I'm not sure if it would change the behavior. |
a940af0 to
12c56bd
Compare
12c56bd to
a7bb012
Compare
|
I tried your change and my LLM was confused.. so I asked it to figure out what was wrong and suggest a change.. there is what it suggested: The original instructions were a bit open-ended, which led to a minor confusion about parameter names. Here are a couple of improvements that could make it clearer:
By refining the prompt with these details, the assistant (or developer) would be less likely to misuse the functions and run into errors. |
9488896 to
9f55bd4
Compare
51e5b72 to
a0e94e8
Compare
|
@olaservo , is there anything that I need to do such that this PR can be considered for review? |
a0e94e8 to
df76986
Compare
|
The official MCP Servers cannot guarantee itself even for such simple tools, so better unit test them or consider implementing these basic ones by ourselves. |
fe9cb97 to
efc5368
Compare
|
I think this needs to be a tool in that server. |
efc5368 to
016740d
Compare
016740d to
63c4f3a
Compare
|
Not sure what's holding up this PR, but the tool failing to search for files by extension seems like it should be fixed ... mcp check results find /tmp/work -name '*.png' | wc -l
4 |
According to the CONTRIBUTING.md, improvements to existing servers like this filesystem enhancement are welcome, but I don't have visibility into the review timelines and I don't know if a repo maintainer will look at this PR. I'm happy to address any concerns or make adjustments. The fix has been working well in my testing environment for months, and I believe it would benefit the broader MCP community. |
becf720 to
fb5ad1d
Compare
|
Resolved merge conflicts after #543 was merged. |
|
Awesome, thanks for the contribution! Sorry we've been very slow getting to this - we're a small team, most of whom are volunteer contributors. The poor tool description definitely seems like a bug in the filesystem server, and it makes a lot of sense why models are confused by it. I think also the patterns was quite confusing for models previously. After #623 and #745, I think this should be quite a bit better. Additionally, I've taken a stab at fixing the problem with a more minor description change in #2606 - somewhat inspired by the suggestions in the issue you raised, so thank you. I think we're trying to keep these servers small and avoid adding net new functionality so I'm hesitant to accept a new tool However, I would be happy to approve and merge the parts around improving testing and nested folder traversal performance, if you'd be open to reworking this PR to target that / opening a new PR for that and tagging me? Possibly it'd be worth waiting until #2606 is merged, given I think that's a small one which can go in easily soon - and save you having to resolve yet another set of merge conflicts 😅 Thanks again for putting up with us here, and for thoughtfully raising the issue and PR! And sorry filesystem MCP has been in such a bad state this whole time. |
4d210d2 to
fc1841b
Compare
I've removed the
Hmm, the
Interesting, I would have thought it makes sense to extend the existing MCP tool rather than create a new one. This is what I had in mind:
Working on it by first resolving merge conflicts and removing the
|
src/filesystem/lib.ts
Outdated
| } | ||
|
|
||
| // Legacy function for backward compatibility | ||
| export async function searchFilesWithValidation( |
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.
Is this used anywhere? I think we can delete?
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.
You are right. This is a byproduct of repeatedly resolving merge conflicts.
I'm removing searchFilesWithValidation entirely and using searchFilesByName, which is less ambiguous. I.e. searchFilesWithValidation could mean search file/directory by name, OR it could mean file content.
This is in 56a4962 for now. I will squash everything when all feedback has been addressed.
| const caseSensitive = /[A-Z]/.test(pattern); // Check if pattern has uppercase characters | ||
|
|
||
| // Determine if the pattern is a glob pattern or a simple substring | ||
| const isGlobPattern = pattern.includes('*') || pattern.includes('?') || pattern.includes('[') || pattern.includes('{'); |
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.
Potentially breaking change (in the same way as discussed here)
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.
I think if we're going to make breaking changes, we should probably just simplify to only doing glob matching
- Add descriptions to help the LLM understand the fields. - Support single file search. - Support glob pattern for search by file name. - Rename 'pattern' argument to 'searchText'. Sometimes the name 'pattern' caused Claude to pass a regex even though it was processed as a simple substring match. - Add unit tests.
fc1841b to
c0478e9
Compare
Description
search_filesfunction description leads to misunderstanding of its purpose #896: "Search file by name" enhancementssearch_filesfunction tosearch_files_by_namesearch_file_contentsto search file content.Server Details
Motivation and Context
Both Claude and human users may misinterpret the
search_filesfunction as a content search tool (like grep) rather than a file/directory name matcher. The suggested description makes the distinction clear from the beginning and explicitly notes that it doesn't search file contents.How Has This Been Tested?
Tested with Claude Desktop with the following prompts, before and after the change.
/Users/my_userid/my_project/
/Users/my_userid/my_project/
/Users/my_userid/my_project using filesystem MCP.
/Users/my_userid/my_project using filesystem MCP.
For prompt 1, the LLM can easily infer it should be searching for file contents, not matching file names. Previously, the LLM mistakenly invoked the
search_filesfunction, which typically would return no matching results.For prompts 3 and 4, there is intentionally some prompt ambiguity to test how the LLM decide whether to use
search_files_by_nameorsearch_file_contentbased on the function names and descriptions.Breaking Changes
No, but I'm not sure I understand breaking changes. I'm unclear whether it's ok to rename a function, or if it's considered to be a breaking change.
Types of changes
Checklist
Additional context
Tests for the
search_file_contentsfunctionasync function \w+\(**/*.test.ts) correctly filter out matching files*config*) correctly filter content containing those strings