-
Notifications
You must be signed in to change notification settings - Fork 157
chore: update atlas tools output to json - MCP-264 #653
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
Changes from all commits
51ac3d3
db2cff0
eb2de0c
dcb153f
0bc90e6
a959af3
db381e1
148dcb2
b811a9c
dcfcad0
0e4fa72
7a50ba1
e208f7c
6c236e1
bba8d12
3c408fe
c9247e7
a0a6c36
377a1a3
f636ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,13 +25,17 @@ export class InspectClusterTool extends AtlasToolBase { | |
| } | ||
|
|
||
| private formatOutput(formattedCluster: Cluster): CallToolResult { | ||
| const clusterDetails = { | ||
| name: formattedCluster.name || "Unknown", | ||
| instanceType: formattedCluster.instanceType, | ||
| instanceSize: formattedCluster.instanceSize || "N/A", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure if this is supposed to be a number but if yes then you might wanna use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks for flagging but it's text |
||
| state: formattedCluster.state || "UNKNOWN", | ||
| mongoDBVersion: formattedCluster.mongoDBVersion || "N/A", | ||
| connectionStrings: formattedCluster.connectionStrings || {}, | ||
| }; | ||
|
|
||
| return { | ||
| content: formatUntrustedData( | ||
| "Cluster details:", | ||
| `Cluster Name | Cluster Type | Tier | State | MongoDB Version | Connection String | ||
| ----------------|----------------|----------------|----------------|----------------|---------------- | ||
| ${formattedCluster.name || "Unknown"} | ${formattedCluster.instanceType} | ${formattedCluster.instanceSize || "N/A"} | ${formattedCluster.state || "UNKNOWN"} | ${formattedCluster.mongoDBVersion || "N/A"} | ${formattedCluster.connectionStrings?.standardSrv || formattedCluster.connectionStrings?.standard || "N/A"}` | ||
| ), | ||
| content: formatUntrustedData("Cluster details:", JSON.stringify(clusterDetails)), | ||
| }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,22 +28,20 @@ export class ListAlertsTool extends AtlasToolBase { | |
| return { content: [{ type: "text", text: "No alerts found in your MongoDB Atlas project." }] }; | ||
| } | ||
|
|
||
| // Format alerts as a table | ||
| const output = | ||
| `Alert ID | Status | Created | Updated | Type | Comment | ||
| ----------|---------|----------|----------|------|-------- | ||
| ` + | ||
| data.results | ||
| .map((alert) => { | ||
| const created = alert.created ? new Date(alert.created).toLocaleString() : "N/A"; | ||
| const updated = alert.updated ? new Date(alert.updated).toLocaleString() : "N/A"; | ||
| const comment = alert.acknowledgementComment ?? "N/A"; | ||
| return `${alert.id} | ${alert.status} | ${created} | ${updated} | ${alert.eventTypeName} | ${comment}`; | ||
| }) | ||
| .join("\n"); | ||
| const alerts = data.results.map((alert) => ({ | ||
| id: alert.id, | ||
| status: alert.status, | ||
| created: alert.created ? new Date(alert.created).toISOString() : "N/A", | ||
| updated: alert.updated ? new Date(alert.updated).toISOString() : "N/A", | ||
| eventTypeName: alert.eventTypeName, | ||
| acknowledgementComment: alert.acknowledgementComment ?? "N/A", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same remark, can we avoid mixing |
||
| })); | ||
|
|
||
| return { | ||
| content: formatUntrustedData(`Found ${data.results.length} alerts in project ${projectId}`, output), | ||
| content: formatUntrustedData( | ||
| `Found ${data.results.length} alerts in project ${projectId}`, | ||
| JSON.stringify(alerts) | ||
| ), | ||
| }; | ||
| } | ||
| } | ||
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.
In other commands we return
null/undefined, could we do that for all tools?Uh oh!
There was an error while loading. Please reload this page.
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.
That's an interesting point. having these human-readable make it easier to parse from both human and LLM perspective I'd think. If there's both
nullandundefinedcases which mean different things then it's worth keeping or handling them differently but otherwise I don't have strong feelings about it.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.
ack, I'll keep "N/A" like the previous implementation everywhere then! thanks for flagging
Uh oh!
There was an error while loading. Please reload this page.
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.
for context on the initial atlas tools I've added N/A as it was a better fit for markdown tables, for json I personally think
nullwould make more sense (or omitting the field)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 think I can group this feedback and
structuredContentinto upcoming changes and get rid of the tables for now, so i'll merge