Improved MCP server with init instructions and server list in query error#3896
Improved MCP server with init instructions and server list in query error#3896ExtReMLapin wants to merge 3 commits intoArcadeData:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request enhances the MCP implementation by adding specific instructions for LLMs and introducing a MCPToolUtils.resolveDatabase utility to provide a list of authorized databases when a requested database is not found. Feedback suggests improving the database filtering logic in the utility for better robustness and centralizing the security authorization check. Currently, manual security checks in the tool classes are performed before the resolution logic, which prevents restricted users from receiving the helpful list of available databases when a typo occurs.
| final Set<String> installed = new TreeSet<>(server.getDatabaseNames()); | ||
| final Set<String> allowed = user.getAuthorizedDatabases(); | ||
| if (!allowed.contains("*")) | ||
| installed.retainAll(allowed); |
There was a problem hiding this comment.
Using user.canAccessToDatabase(db) to filter the list is more robust than manually checking for * and using retainAll. This approach correctly leverages the existing security logic in the ServerSecurityUser class, which may include complex rules or wildcards beyond a simple * check.
| final Set<String> installed = new TreeSet<>(server.getDatabaseNames()); | |
| final Set<String> allowed = user.getAuthorizedDatabases(); | |
| if (!allowed.contains("*")) | |
| installed.retainAll(allowed); | |
| final Set<String> installed = new TreeSet<>(server.getDatabaseNames()); | |
| installed.removeIf(db -> !user.canAccessToDatabase(db)); |
| "Database '" + databaseName + "' does not exist. Available databases: " + installed | ||
| + ". Use one of these names or call list_databases to refresh the list."); | ||
| } | ||
| return server.getDatabase(databaseName); |
There was a problem hiding this comment.
Consider centralizing the database authorization check here. By moving the canAccessToDatabase check into this utility method, you ensure that all tools using resolveDatabase are consistently protected. Furthermore, this allows the 'Available databases' error message to be reached even when a restricted user makes a typo, which is currently blocked by the manual checks in the tool classes.
if (!user.canAccessToDatabase(databaseName))
throw new SecurityException("User '" + user.getName() + "' is not authorized to access database '" + databaseName + "'");
return server.getDatabase(databaseName);| throw new SecurityException("User '" + user.getName() + "' is not authorized to access database '" + databaseName + "'"); | ||
|
|
||
| final Database database = server.getDatabase(databaseName); | ||
| final Database database = MCPToolUtils.resolveDatabase(server, user, databaseName); |
There was a problem hiding this comment.
The security check on lines 76-77 (context) is currently executed before resolveDatabase. For restricted users, this means a typo in the database name will trigger a SecurityException instead of the helpful error message containing the list of available databases. Consider moving the authorization check inside resolveDatabase or after it to ensure the 'Available databases' feature works for all users.
| throw new SecurityException("User '" + user.getName() + "' is not authorized to access database '" + databaseName + "'"); | ||
|
|
||
| final Database database = server.getDatabase(databaseName); | ||
| final Database database = MCPToolUtils.resolveDatabase(server, user, databaseName); |
There was a problem hiding this comment.
The manual security check on lines 75-76 (context) prevents restricted users from seeing the list of available databases when they provide an incorrect database name. If the authorization check is moved inside or after resolveDatabase, the LLM will be able to self-correct using the provided list.
| throw new SecurityException("User '" + user.getName() + "' is not authorized to access database '" + databaseName + "'"); | ||
|
|
||
| final Database database = server.getDatabase(databaseName); | ||
| final Database database = MCPToolUtils.resolveDatabase(server, user, databaseName); |
There was a problem hiding this comment.
Similar to other tools, the preceding security check (lines 60-61) blocks the helpful error message for restricted users. Centralizing this check within resolveDatabase would improve the user experience and maintainability.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes. Give us feedback
|
@ExtReMLapin can you rebase and push again? thanks |
|
Done ! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3896 +/- ##
==========================================
- Coverage 64.46% 63.78% -0.69%
==========================================
Files 1587 1588 +1
Lines 118612 118620 +8
Branches 25194 25195 +1
==========================================
- Hits 76468 75659 -809
- Misses 31548 32483 +935
+ Partials 10596 10478 -118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
supersede by #3966 Thank you for your contribution! |
I was running tests with Qwen code, and it was tasked to run cypher queries on both neo4j and arcadedb.
Goofy qwen didn't bother getting the list of the databases first.
Now, it has instructions to do it first, and even if it fails to do so and go for an non existing base it will get the list of the bases in the error