feat: add structured content for response in mongodb tools - MCP-374#1249
feat: add structured content for response in mongodb tools - MCP-374#1249dudaschar wants to merge 27 commits into
Conversation
…d switchConnection
…unnecessary type assertions
|
|
||
| type StructuredToolResult<OutputSchema extends ZodRawShape> = { | ||
| content: { type: "text"; text: string }[]; | ||
| content: CallToolResult["content"]; |
There was a problem hiding this comment.
This change is allowing the tools to with a previous custom tool result to have a content other than only text + structured content without the need to custom types or casting the output type.
| * | ||
| * ObjectId becomes `{ "$oid": "..." }`, Long becomes `{ "$numberLong": "..." }`, etc. | ||
| */ | ||
| export function bsonToJson(value: Record<string, unknown>): Record<string, unknown>; |
There was a problem hiding this comment.
This is overloading is supporting dbStats tool, which is an object instead of a collection document, so no cast in the tool will be necessary (or any other similar).
…stringify and streamline structured content
…d update tests to directly assert structured content
dudaschar
requested review from
Luke-Sanderson and
lerouxb
and removed request for
a team
dudaschar
changed the title
| protected override async handleError( | ||
| error: unknown, | ||
| args: ToolArgs<typeof this.argsShape> | ||
| ): Promise<CallToolResult> { | ||
| const result = await super.handleError(error, args); | ||
| if (result.isError) { | ||
| return { ...result, structuredContent: { connected: false } }; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Another option here would be not handling the error, and structured content be undefined, instead of returning false. If we decide to return undefined, I'll update similar tools to do the same.
nirinchev
left a comment
nirinchev
left a comment
There was a problem hiding this comment.
Looks good - I have mostly stylistic comments/questions, that are not critical, but good to address. We should probably revert the export changes though as it's the kind of tool that doesn't benefit much (if at all) by adding structured content.
| documents: z.array(z.unknown()).describe("The documents returned by the aggregation pipeline"), | ||
| aggResultsCount: z | ||
| .number() | ||
| .optional() |
There was a problem hiding this comment.
Why is this optional? Wouldn't we always be able to supply the count?
There was a problem hiding this comment.
totalDocuments can be null, if something downstream fails in the countAggregationResultDocuments, the value will be undefined (not 0).
Not sure how often this can actually happen or if we wanna handle the response in a different way. I don't think we should return as 0 tho, because it could be misleading.
There was a problem hiding this comment.
Ah, I see - wonder if that will be clear enough for the LLM - we could also have it as union type of number and the const string indeterminate or something similar to be extra explicit?
There was a problem hiding this comment.
Since this is triggered by an error, I think "not_available" could be better, so it's also a hint to the llm to retry the tool if count is necessary. But not a hard opinion here, tho.
There was a problem hiding this comment.
I see the existing message already uses indeterminable number of documents, so I'll keep indeterminate for consistency now.
…DB tools and tests for improved serialization handling
…rectly using JSON.stringify
…dation in find tool tests
…ing content parameter and enhancing count handling
2 participants
This work enables the output to be provided in structuredContent format which caters for the prompt injection case, but removes the need for additional work on the users side to re-format the output to remove the tags.
It also add serialization safety when parsing BSON documents into JSON, to values type
longare now properly handled in the Tool output. A follow up ticket will be created to discuss Tool arguments and interpolation in the output text response.This PR apply the updates for the tools inside the mongodb package, a follow up PR will handle /atlas and /atlas-local
Checklist