fix(mcp): code review fixes — type-aware dockit schema, error handling, resource notifications - #27
Merged
Merged
Conversation
Strengthen tool descriptions so AI agents reach for data_studio__* on any database task (query, schema, row counts) instead of local DB CLIs. get_status and list_connections are now framed as the entry point, every backend tool carries a note to prefer data-studio, and all descriptions ask agents to report results in the user's language (中文/English). Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Extend data-studio-mcp beyond plain tools: expose connections and per-connection schema as read-only MCP resources (with a resource template), add explore-database/inspect-table/debug-query prompts, and argument completions for connection_id/database. Every tool now carries a human-readable title and strict input schemas (additionalProperties: false). get_status gains a summary block (connection counts, types, tool distribution) so agents can pick the right connection without extra calls. execute_query results include structuredContent (columns/rows) for lossless consumption. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Add integration tests for the new primitives: resources list/read (connections + schema), resource templates, prompts (explore/inspect/debug), argument completions, and structuredContent on execute_query results. Extend the get_status test to assert the summary block. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…pe clients fetchSchemaViaListTables: dockit backends don't have get_schema — now uses list_tables + list_columns to build the schema snapshot. completeArgument: respects context.arguments.connection_id instead of always using the first connection. collectConnections: reuses this.clients instead of creating duplicate BackendClient instances on each poll. Updates R1 test stub to exercise the list_tables path. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…rce list notifications fetchSchemaViaListTables replaced with type-specific fetchers: ES uses es__cat_indices + es__get_mapping, MongoDB uses mongo__list_databases + mongo__list_collections, DynamoDB uses dynamo__list_tables + dynamo__describe_table. listResources now uses cached connections from registry snapshot instead of live HTTP calls. readResource and completeArgument wrapped in try-catch for consistent error handling. onToolsChanged now also sends sendResourceListChanged. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
…fetchers Resolved 3-file conflict: index.ts (keep cached connections + try-catch + sendResourceListChanged), resources.ts (keep type-aware ES/MongoDB/DynamoDB schema fetchers), tests (keep ES-specific stubs). All 63 tests pass.
…detection
fetchMongoSchema/fetchDynamoSchema: dockit bridge wraps responses in ApiResponse {status, message, data}. Added unwrapBridgePayload to handle the double-wrapping. MongoDB returns {databases: [string]} and {collections: [string]} — not arrays of objects. DynamoDB returns {tableNames: [string]} and {attributeDefinitions: [{attributeName, attributeType}]}. extractArray upgraded to handle nested wrappers. Removed non-standard isError field from ReadResourceResult. Registry now detects connection-only changes (connSignature) and fires onToolsChanged → sendResourceListChanged. Added R1b (MongoDB) and R1c (DynamoDB) test cases.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent)
Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to PR #26. Fixes 3 issues + 1 performance point found in code review.
Issue 1 (Medium): dockit schema resources called non-existent tools
fetchSchemaViaListTablescalledlist_tables+list_columns— sqlkit tool names that don't exist on the dockit bridge. Replaced with type-specific fetchers:es__cat_indices+es__get_mappingmongo__list_databases+mongo__list_collectionsdynamo__list_tables+dynamo__describe_tableIssue 2 (Low-Medium):
sendResourceListChanged()never calledServer advertised
resources: { listChanged: true }but never sent the notification. NowonToolsChangedcalls bothsendToolListChanged()andsendResourceListChanged().Issue 3 (Low): Resource/completion handlers don't catch errors
Tool handler wraps in try-catch with structured error results, but resource/completion handlers let exceptions propagate. Now:
readResourceerrors return{ isError: true }with error textcompleteArgumenterrors return{ completion: { values: [] } }Performance:
listResourcesuses cached connectionsWas making live HTTP calls (
safeListConnections) on everyListResourcesrequest. Now usessnapshot.connectionsfrom the registry poll cache — zero HTTP calls for resource listing.Verification
Ultraworked with Sisyphus