Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements MCP (Model Context Protocol) search improvements and performance optimizations to the go-string library. The changes add pagination support to the MCP search API and optimize the case-insensitive string search algorithm by using character rarity analysis to select better search anchors.
Changes:
- Added pagination support to MCP search with offset parameter and response metadata (total_matches, truncated, next_offset)
- Updated go-string dependency with performance improvements to IndexAllIgnoreCase using character rarity-based search anchor selection
- Added comprehensive tests for truncation and no-truncation scenarios in MCP search
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated go-string dependency to version with performance improvements |
| go.sum | Added checksum for new go-string version |
| vendor/modules.txt | Updated vendored go-string version reference |
| vendor/github.com/boyter/go-string/index.go | Implemented character rarity analysis for optimized search anchor selection, added bestCharOffset function and _charRarity lookup table, improved comment grammar and style |
| mcp.go | Added mcpSearchResponse structure with pagination metadata, implemented offset parameter handling, added truncation tracking and pagination message generation |
| mcp_test.go | Added tests for truncation scenarios and empty result handling with new response structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // whether results were truncated and what the full match count was. | ||
| type mcpSearchResponse struct { | ||
| TotalMatches int `json:"total_matches"` | ||
| ResultsReturned int `json:"results_returned"` | ||
| Offset int `json:"offset"` | ||
| NextOffset int `json:"next_offset"` |
There was a problem hiding this comment.
The truncated flag only indicates whether the current page was truncated due to max_results, not whether pagination is active. This might be confusing for API consumers. Consider either: 1) renaming to 'page_truncated' for clarity, 2) adding a separate 'has_more' boolean field, or 3) documenting this behavior clearly in the API description. The current behavior is internally consistent (clients can check if next_offset < total_matches to determine if there are more results), but explicit documentation would help.
| // whether results were truncated and what the full match count was. | |
| type mcpSearchResponse struct { | |
| TotalMatches int `json:"total_matches"` | |
| ResultsReturned int `json:"results_returned"` | |
| Offset int `json:"offset"` | |
| NextOffset int `json:"next_offset"` | |
| // the total match count and how the current page was produced. Note that | |
| // Truncated only indicates that this page was cut short due to max_results; | |
| // it does NOT by itself indicate whether there are more results available. | |
| // To determine if more results exist, compare next_offset and total_matches | |
| // (i.e., there are more results when next_offset < total_matches). | |
| type mcpSearchResponse struct { | |
| TotalMatches int `json:"total_matches"` | |
| ResultsReturned int `json:"results_returned"` | |
| Offset int `json:"offset"` | |
| NextOffset int `json:"next_offset"` | |
| // Truncated is true when this page's results were cut short due to the | |
| // configured max_results limit for a single page. Clients should use | |
| // next_offset < total_matches to detect whether additional pages exist. |
| t.Errorf("expected empty message when not truncated, got: %s", parsed.Message) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There is no test coverage for the new pagination offset parameter. The tests cover truncation behavior but don't verify that the offset parameter works correctly. Consider adding a test that creates multiple results and uses different offset values to ensure pagination works as expected, especially edge cases like offset=0, offset greater than total results, and offset pointing to the middle of the result set.
| func TestMCPSearchHandlerOffsetPagination(t *testing.T) { | |
| dir := t.TempDir() | |
| // Create 5 files that all match the query term | |
| for i := 0; i < 5; i++ { | |
| content := fmt.Sprintf("package main\n\nfunc handler%d() {\n\t// zebratoken\n}\n", i) | |
| fname := fmt.Sprintf("file%d.go", i) | |
| if err := os.WriteFile(filepath.Join(dir, fname), []byte(content), 0644); err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| cfg := DefaultConfig() | |
| cfg.Directory = dir | |
| cache := NewSearchCache() | |
| handler := mcpSearchHandler(&cfg, cache) | |
| type testCase struct { | |
| name string | |
| offset int | |
| wantReturned int | |
| wantTruncated bool | |
| } | |
| cases := []testCase{ | |
| { | |
| name: "offset_zero", | |
| offset: 0, | |
| wantReturned: 5, | |
| wantTruncated: false, | |
| }, | |
| { | |
| name: "offset_middle", | |
| offset: 2, | |
| wantReturned: 3, | |
| wantTruncated: false, | |
| }, | |
| { | |
| name: "offset_beyond_total", | |
| offset: 10, | |
| wantReturned: 0, | |
| wantTruncated: false, | |
| }, | |
| } | |
| for _, tc := range cases { | |
| t.Run(tc.name, func(t *testing.T) { | |
| req := mcp.CallToolRequest{} | |
| req.Params.Arguments = map[string]any{ | |
| "query": "zebratoken", | |
| "offset": tc.offset, | |
| } | |
| result, err := handler(context.Background(), req) | |
| if err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if result.IsError { | |
| t.Fatalf("unexpected error result: %v", result) | |
| } | |
| text, ok := result.Content[0].(mcp.TextContent) | |
| if !ok { | |
| t.Fatalf("expected TextContent, got %T", result.Content[0]) | |
| } | |
| var parsed mcpSearchResponse | |
| if err := json.Unmarshal([]byte(text.Text), &parsed); err != nil { | |
| t.Fatalf("result is not valid JSON: %v", err) | |
| } | |
| if parsed.TotalMatches != 5 { | |
| t.Errorf("expected total_matches=5, got %d", parsed.TotalMatches) | |
| } | |
| if parsed.ResultsReturned != tc.wantReturned { | |
| t.Errorf("for offset=%d expected results_returned=%d, got %d", tc.offset, tc.wantReturned, parsed.ResultsReturned) | |
| } | |
| if parsed.Truncated != tc.wantTruncated { | |
| t.Errorf("for offset=%d expected truncated=%v, got %v", tc.offset, tc.wantTruncated, parsed.Truncated) | |
| } | |
| }) | |
| } | |
| } |
No description provided.