-
Notifications
You must be signed in to change notification settings - Fork 178
Add SQLite FTS5-backed ToolStore for optimizer search #3786
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
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.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3786 +/- ##
==========================================
+ Coverage 66.81% 66.86% +0.04%
==========================================
Files 438 439 +1
Lines 43321 43404 +83
==========================================
+ Hits 28946 29020 +74
- Misses 12128 12132 +4
- Partials 2247 2252 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jerm-dro
left a comment
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'll take a closer look at the sqlite_store_test when they're in a more condensed form. Everything else looks mostly good aside for some blocking comments which should be easy to address.
86a4829 to
27c5e5c
Compare
27c5e5c to
76c494b
Compare
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
jerm-dro
left a comment
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.
One minor blocker, but otherwise LGTM
Add an SQLite FTS5 ToolStore implementation as an alternative to the existing InMemoryToolStore, and wire it into the vMCP server when optimizer config is present. This adds: - pkg/vmcp/optimizer/fts5store: SQLiteToolStore implementation using modernc.org/sqlite (pure Go, no CGO) with FTS5 virtual tables - BM25 ranking for search results with LIKE-based fallback - FTS5 query sanitization for safe handling of special characters - Thread-safe concurrent access with sync.RWMutex - Close() method on the ToolStore interface for resource cleanup - OptimizerStoreCloser on server.Config for store lifecycle management - OptimizerConfig.FTSDBPath for configurable database location (defaults to in-memory, use emptyDir in Kubernetes) - Wire FTS5 store into vMCP serve command when optimizer is configured - Comprehensive tests for search, upsert, and concurrency scenarios Part 2 of the optimizer FTS5 migration (issue #3731). Co-Authored-By: Claude Opus 4.6 <[email protected]>
631f1d6 to
85a4c84
Compare
jerm-dro
left a comment
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.
feel free to address the nit in a later PR
Closes: #3731
Add an SQLite FTS5 ToolStore implementation as an alternative to the existing InMemoryToolStore, and wire it into the vMCP server when optimizer config is present.
This adds:
Part 2 of the optimizer FTS5 migration (issue #3731).
Large PR Justification