Skip to content

Conversation

domdomegg
Copy link
Member

Summary

  • Clarified search_files tool description to explicitly state it searches by filename patterns, not file content
  • Standardized implementation to use glob matching exclusively instead of mixing glob and substring matching
  • Updated tests to reflect the new consistent glob-only behavior

Changes

  • Modified pattern matching in buildTree to use only minimatch glob patterns
  • Updated search_files to accept array of patterns instead of single pattern for better flexibility
  • Adjusted test cases to use proper glob patterns (e.g., **/node_modules for nested directories)
  • Improved tool descriptions in the API to be clearer about glob pattern usage

Fixes #896

Previously description was confusing/ambigious about whether this was doing text content matching or filename matching. This clarifies it's by filename. Also the implementation for globs was confusing - it would sometimes use glob and sometimes substring match. This makes it all globs. Fixes #896 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
@domdomegg domdomegg requested a review from olaservo August 23, 2025 06:51
@domdomegg
Copy link
Member Author

Credit: Some of the prompt change is inspired by the suggestions from @sebastien-rosset and others in #896

@domdomegg domdomegg added bug Something isn't working server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem labels Aug 23, 2025
Copy link
Member

@olaservo olaservo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could you also update the reference to the updated tool in the Readme?
  2. Do you think we should call out this being a breaking change in the Readme? With the calendar versioning we unfortunately can't signal breaking changes with the version number, so I am assuming that people/LLMs will discover this through trial and error (in instances where there is some reference to the old behavior)
@domdomegg
Copy link
Member Author

  1. @claude can you do this?
  2. Yeah, I worry about this being a breaking change a little. Although the previous format was so undocumented that I doubt too much has been built around it? I'm leaning to just pushing it out, but open to ideas on how we can better signal this to people.
Copy link
Contributor

claude bot commented Aug 25, 2025

Claude finished @domdomegg's task —— View job


Summary

Yes, I can help with the review feedback! I've updated the documentation and here's my guidance on the breaking change concerns.

**Completed Tasks & Recommendations**

✅ Completed Tasks

  • Updated filesystem README - Reflected the new search_files API with patterns array parameter
  • Added Breaking Changes section - Clear documentation of the parameter and behavior changes
  • Provided migration guidance - Examples showing how to update from old to new API
  • Built and tested changes - All tests pass, TypeScript compiles successfully
  • Committed and pushed - Changes are now in the PR branch

📋 Key Changes Made

Documentation Updates (src/filesystem/README.md):

  • Added "Breaking Changes" section documenting the search_files API change
  • Updated tool description: pattern (string) → patterns (string[])
  • Clarified "glob pattern matching exclusively" vs mixed glob/substring
  • Added migration example: {"pattern": "*.js"}{"patterns": ["*.js"]}

💡 Breaking Change Recommendations

Regarding your concern about the breaking change:

  1. 👍 Proceed with the change - You're right that the previous behavior was undocumented, which minimizes real-world impact

  2. 📢 Consider these signaling strategies:

    • Documentation (already done) - The Breaking Changes section provides clear migration path
    • Release notes - Highlight this in the next release/changelog
    • Version bump consideration - While calendar versioning limits options, consider if this warrants a minor version increment
    • Deprecation notice - Could add a temporary backward compatibility layer that warns about the old API
  3. 🔄 Migration support options:

    // Option 1: Backward compatibility (temporary) pattern?: string; // deprecated, warn users patterns: string[]; // Option 2: Auto-migration in server if (args.pattern && !args.patterns) { console.warn("search_files: 'pattern' parameter is deprecated, use 'patterns' array"); args.patterns = [args.pattern]; }
  4. 📊 Impact assessment:

    • Low impact likely due to undocumented previous behavior
    • Better long-term API consistency outweighs short-term friction
    • Clear documentation and examples help adoption

My recommendation: Proceed as planned. The benefits of API consistency and improved glob pattern matching outweigh the breaking change concerns, especially given the limited documentation of previous behavior.

The changes are now committed and ready. The filesystem README includes clear breaking change documentation with migration guidance for users.

--- | Branch: adamj/search-files-clarify-implementation

…_files docs - Add Breaking Changes section to README documenting search_files parameter change - Update search_files documentation to reflect new patterns array parameter - Clarify exclusive glob pattern matching behavior - Provide migration guidance from old pattern string to new patterns array 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: adam jones <domdomegg@users.noreply.github.com>
Co-authored-by: Ola Hungerford <olahungerford@gmail.com>
Co-authored-by: Ola Hungerford <olahungerford@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working server-filesystem Reference implementation for the Filesystem MCP server - src/filesystem

3 participants