-
Notifications
You must be signed in to change notification settings - Fork 3
Implement Tool Selection Framework for NLWeb June 2025 Release #46
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
Co-authored-by: jongalloway <[email protected]>
…imization and comprehensive tests Co-authored-by: jongalloway <[email protected]>
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.
Pull Request Overview
Implements the NLWeb tool selection framework with intent-based routing while preserving backward compatibility.
- Introduces
IToolSelectorinterface andToolSelectorservice for intent-driven query routing - Integrates tool selection in
QueryProcessorunder a newToolSelectionEnabledoption - Adds unit tests, performance tests, and sample development configuration
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/NLWebNet.Tests/TestLogger.cs | Added configurable minimum log level filter to TestLogger |
| tests/NLWebNet.Tests/Services/ToolSelectorTests.cs | New unit tests covering ToolSelector behavior |
| tests/NLWebNet.Tests/Services/ToolSelectorPerformanceTests.cs | Performance tests measuring tool selection overhead |
| tests/NLWebNet.Tests/Services/QueryProcessorTests.cs | Extended QueryProcessor tests to include tool selection scenarios |
| src/NLWebNet/Services/ToolSelector.cs | Implements keyword-based intent analysis and selection |
| src/NLWebNet/Services/QueryProcessor.cs | Invokes tool selection when enabled |
| src/NLWebNet/Services/IToolSelector.cs | Defines the IToolSelector contract |
| src/NLWebNet/Models/NLWebOptions.cs | Adds ToolSelectionEnabled configuration option |
| src/NLWebNet/Extensions/ServiceCollectionExtensions.cs | Registers IToolSelector in dependency injection |
| samples/Demo/appsettings.Development.json | Enables tool selection and debug logging in sample |
Comments suppressed due to low confidence (1)
tests/NLWebNet.Tests/Services/QueryProcessorTests.cs:147
- This test comments that tool selection should be invoked but doesn’t verify it. Use a mock or spy to assert that
SelectToolAsyncis actually called when tool selection is enabled.
// The tool selection should have been called but not affect the final result
| if (ContainsKeywords(queryLower, "search", "find", "look for", "locate")) | ||
| { | ||
| return Task.FromResult<string?>("search"); | ||
| } | ||
|
|
||
| if (ContainsKeywords(queryLower, "compare", "difference", "versus", "vs", "contrast")) | ||
| { | ||
| return Task.FromResult<string?>("compare"); | ||
| } | ||
|
|
||
| if (ContainsKeywords(queryLower, "details", "information about", "tell me about", "describe")) | ||
| { | ||
| return Task.FromResult<string?>("details"); | ||
| } | ||
|
|
||
| if (ContainsKeywords(queryLower, "recommend", "suggest", "what should", "ensemble", "set of")) | ||
| { | ||
| return Task.FromResult<string?>("ensemble"); | ||
| } | ||
|
|
||
| // Default to search tool for general queries | ||
| return Task.FromResult<string?>("search"); |
Copilot
AI
Jul 2, 2025
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.
[nitpick] Consider extracting these hard-coded keyword lists and the resulting tool names into named constants or an enum to avoid duplication and reduce typos.
| if (ContainsKeywords(queryLower, "search", "find", "look for", "locate")) | |
| { | |
| return Task.FromResult<string?>("search"); | |
| } | |
| if (ContainsKeywords(queryLower, "compare", "difference", "versus", "vs", "contrast")) | |
| { | |
| return Task.FromResult<string?>("compare"); | |
| } | |
| if (ContainsKeywords(queryLower, "details", "information about", "tell me about", "describe")) | |
| { | |
| return Task.FromResult<string?>("details"); | |
| } | |
| if (ContainsKeywords(queryLower, "recommend", "suggest", "what should", "ensemble", "set of")) | |
| { | |
| return Task.FromResult<string?>("ensemble"); | |
| } | |
| // Default to search tool for general queries | |
| return Task.FromResult<string?>("search"); | |
| if (ContainsKeywords(queryLower, SearchKeywords)) | |
| { | |
| return Task.FromResult<string?>(ToolSearch); | |
| } | |
| if (ContainsKeywords(queryLower, CompareKeywords)) | |
| { | |
| return Task.FromResult<string?>(ToolCompare); | |
| } | |
| if (ContainsKeywords(queryLower, DetailsKeywords)) | |
| { | |
| return Task.FromResult<string?>(ToolDetails); | |
| } | |
| if (ContainsKeywords(queryLower, EnsembleKeywords)) | |
| { | |
| return Task.FromResult<string?>(ToolEnsemble); | |
| } | |
| // Default to search tool for general queries | |
| return Task.FromResult<string?>(ToolSearch); |
| Assert.IsTrue(performanceImpactPercent < 1000, | ||
| "Performance impact should be reasonable for a test environment with debug overhead"); |
Copilot
AI
Jul 2, 2025
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.
[nitpick] The 1000% allowance for performance impact is quite high; consider tightening this threshold or making it configurable to better reflect realistic overhead expectations.
| Assert.IsTrue(performanceImpactPercent < 1000, | |
| "Performance impact should be reasonable for a test environment with debug overhead"); | |
| const double MaxPerformanceImpactPercent = 200; // Configurable threshold for test environment | |
| Assert.IsTrue(performanceImpactPercent < MaxPerformanceImpactPercent, | |
| $"Performance impact ({performanceImpactPercent:F2}%) exceeds the acceptable threshold ({MaxPerformanceImpactPercent}%) for a test environment."); |
|
@copilot from src/NLWebNet/Services/ToolSelector.cs Consider extracting these hard-coded keyword lists and the resulting tool names into named constants or an enum to avoid duplication and reduce typos. |
…n ToolSelector Co-authored-by: jongalloway <[email protected]>
I've extracted the hard-coded keyword lists and tool names into a nested |
Co-authored-by: Copilot <[email protected]>
This PR implements the Tool Selection Framework as outlined in the NLWeb June 2025 requirements, providing intelligent query routing to appropriate tools based on intent analysis while maintaining full backward compatibility.
Overview
The Tool Selection Framework introduces a new layer that analyzes incoming queries and routes them to appropriate tools (search, compare, details, ensemble) based on detected intent. This enables more sophisticated query handling while preserving existing behavior when disabled.
Key Features
🔧 Core Implementation
SelectToolAsyncandShouldSelectToolmethods⚙️ Configuration
NLWebOptions(defaults tofalse)🔄 Integration
Backward Compatibility
✅ 100% Backward Compatible
ToolSelectionEnabled = false)Performance
Task.FromResultfor synchronous operationsTesting
Comprehensive Test Coverage (15 new tests)
Test Results
Usage Examples
Enable Tool Selection
{ "NLWebNet": { "ToolSelectionEnabled": true } }Query Routing Examples
Future Enhancements
The framework provides a solid foundation for:
Code Quality
Fixes #35.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.