-
-
Notifications
You must be signed in to change notification settings - Fork 180
Mcp fixes #154
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.
Pull Request Overview
This PR refactors the MCP pipeline to add a new FilterItems tool, propagate a database schema through the router, and clean up test and config utilities.
- Introduces
FilterItemsinToolArgsand implements it inMcpTools - Adds a
schema_resourceMCP endpoint and wiresschemainto the router and storage - Moves
StyledStringutility into a shared module and comments out HNSW bench tests
Reviewed Changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| helixdb/src/helix_gateway/mcp/tools.rs | Added FilterItems to ToolArgs and its implementation |
| helixdb/src/helix_gateway/mcp/mcp.rs | Removed init payload parsing, added schema_resource |
| helixdb/src/helix_gateway/router/router.rs | Passed schema into MCP tool input |
| helixdb/src/helix_engine/storage_core/storage_core.rs | Stored schema string on HelixGraphStorage |
| helixdb/src/helix_engine/graph_core/config.rs | Renamed and extended config loader to include schema |
| helixdb/src/helix_engine/vector_core/hnsw_tests.rs | Bench tests commented out |
| helix-container/src/queries.rs | Updated query handlers (get_patients_visits) outputs |
Comments suppressed due to low confidence (7)
helixdb/src/helix_gateway/mcp/tools.rs:134
- The parameter order for
filter_itemswas changed (movedtxnto the end), breaking the trait signature. Restore the original parameter order (txnfirst) or update all callers to match.
fn filter_items(
helixdb/src/helix_engine/vector_core/hnsw_tests.rs:27
- The HNSW benchmark tests have been entirely commented out, disabling coverage for vector recall testing. Consider converting these into
#[ignore]tests or parameterized CI benchmarks rather than fully removing them.
/*
helixdb/src/helix_gateway/mcp/mcp.rs:165
- The
inithandler no longer parses the expectedInitRequestfields (e.g.,connection_addrandconnection_port), breaking clients that send those. Reintroduce request deserialization or update the API contract and clients accordingly.
pub fn init<'a>(input: &'a mut MCPToolInput, response: &mut Response) -> Result<(), GraphError> {
helixdb/src/helix_engine/graph_core/config.rs:46
- Renaming
from_config_filetofrom_filesand changing its signature is a breaking API change. Either preserve the old method or clearly document the new signature and migrate callers.
pub fn from_files(config_path: PathBuf, schema_path:PathBuf) -> Result<Self, GraphError> {
helixdb/src/helix_gateway/mcp/tools.rs:83
- By removing the wildcard match arm, any unsupported
ToolArgsvariant will now cause a compile‐time pattern‐match error. Reintroduce a fallback case to return a clearTool not founderror for unknown variants.
ToolArgs::FilterItems { properties, filter_traversals } => self.filter_items(connection, properties, filter_traversals, txn),
helix-container/src/queries.rs:239
- The handler for
get_patients_visits_in_previous_monthnow only insertsvisitsin the response and omits thepatientfield, which likely breaks existing clients expecting both. Re-add the missingpatientinsertion or document the new response shape.
return_vals.insert("visits".to_string(), ReturnValue::from_traversal_value_array_with_mixin(visits.clone(), remapping_vals.borrow_mut()));
helixdb/src/helix_gateway/mcp/tools.rs:359
- [nitpick] Leaving debug
println!statements in production code can clutter logs and leak internal state. Remove or replace with a proper logging macro at debug level.
println!("properties: {:?}", properties);
|
|
||
| /// Same, but returns an owned clone for convenience. | ||
| fn cloned_base(&self) -> Type { | ||
| fn cloned_base(&self) -> Type { // TODO: never used? |
Copilot
AI
Jun 15, 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 cloned_base method is marked as never used—either remove it if obsolete or implement its intended use. Leaving dead code with TODO hurts long-term maintainability.
xav-db
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.
all looks good
description
filter_itemsmcp tool endpointissues
closes #103