servers icon indicating copy to clipboard operation
servers copied to clipboard

feat(search_files): clarify description and standardise implementation

Open domdomegg opened this issue 4 months ago • 3 comments

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

domdomegg avatar Aug 23 '25 06:08 domdomegg

Credit: Some of the prompt change is inspired by the suggestions from @sebastien-rosset and others in https://github.com/modelcontextprotocol/servers/issues/896

domdomegg avatar Aug 23 '25 07:08 domdomegg

  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.

domdomegg avatar Aug 25 '25 17:08 domdomegg

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

  • [x] Updated filesystem README - Reflected the new search_files API with patterns array parameter
  • [x] Added Breaking Changes section - Clear documentation of the parameter and behavior changes
  • [x] Provided migration guidance - Examples showing how to update from old to new API
  • [x] Built and tested changes - All tests pass, TypeScript compiles successfully
  • [x] 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

claude[bot] avatar Aug 25 '25 17:08 claude[bot]