-
Notifications
You must be signed in to change notification settings - Fork 7
Add tool support for document queries #17
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
rocktavious
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.
LGTM but i'd like @eapache-opslevel opinion too.
src/cmd/root.go
Outdated
|
|
||
| // Register all documents, filtered by search term | ||
| s.AddTool( | ||
| mcp.NewTool("listDocuments", |
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.
we don't have verbs in the other commands, so for consistency this should be documents
src/cmd/root.go
Outdated
| // Register all documents, filtered by search term | ||
| s.AddTool( | ||
| mcp.NewTool("listDocuments", | ||
| mcp.WithDescription("Get all the documents for the opslevel account. Documents are filterable by search term"), |
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.
Can we describe what kind of things might be in documents?
Something like
"Get all the documents for the documents for the opslevel account. Documents could be things like run books, getting started guides and other forms of documentation."
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.
If you do this copy-enriching here, similar verbage should happen on the other descriptions too.
src/cmd/root.go
Outdated
| // Register all documents, filtered by service id and search term | ||
| s.AddTool( | ||
| mcp.NewTool("documentsOnService", | ||
| mcp.WithDescription("Get all documents on a specified service for the opslevel account, specified by service id and filtered by search term. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), |
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.
| mcp.WithDescription("Get all documents on a specified service for the opslevel account, specified by service id and filtered by search term. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), | |
| mcp.WithDescription("Get all documents on a specified service for the opslevel account, specified by service id and filtered by search term. Documents could be things like runbooks, integration documentation, api documentation, readme's, or other forms of documentation."), |
src/cmd/root.go
Outdated
| // Register document by id | ||
| s.AddTool( | ||
| mcp.NewTool("document", | ||
| mcp.WithDescription("Get document contents for the opslevel account, specified by id. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), |
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.
| mcp.WithDescription("Get document contents for the opslevel account, specified by id. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), | |
| mcp.WithDescription("Get document contents for the opslevel account, specified by id. Documents could be things like runbooks, integration documentation, api documentation, readme's, or other forms of documentation."), |
src/cmd/root.go
Outdated
| // Register all documents, filtered by search term | ||
| s.AddTool( | ||
| mcp.NewTool("documents", | ||
| mcp.WithDescription("Get all the documents for the opslevel account. Documents are filterable by search term. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), |
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.
| mcp.WithDescription("Get all the documents for the opslevel account. Documents are filterable by search term. Documents could be things like runbooks, integration documentation, api documentatin, readmes, or other forms of documentation."), | |
| mcp.WithDescription("Get all the documents for the opslevel account. Documents are filterable by search term. Documents could be things like runbooks, integration documentation, api documentation, readme's, or other forms of documentation."), |
| func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| searchTerm := "" | ||
| if req.Params.Arguments["searchTerm"] != nil { | ||
| searchTerm = req.Params.Arguments["searchTerm"].(string) |
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'm curious what type this is coming in as, that it needs casting to string?
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.
Probably any and since searchTerm := "" is a string its needed
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 correct, it's any.
| variables := opslevel.PayloadVariables{ | ||
| "searchTerm": searchTerm, | ||
| "after": "", | ||
| "first": 100, |
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.
Do we need to specify first and after here? opslevel-go should fill those in and auto-paginate for us, right?
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.
It appears the pattern is only if you pass nil so since this is initialized it won't hit that logic. That would be a good improvement in opslevel-go though.
https://github.com/OpsLevel/opslevel-go/blob/main/document.go#L38
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'll put up an MR in opslevel-go to change this in documents (and the other spots where this pattern exists too)
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.
This is being held up by some ci issues in opslevel go. IMO we should move forwards for now without this change.
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.
agree
src/cmd/root.go
Outdated
| } | ||
|
|
||
| func getListDocumentPayloadVariables(searchTerm string) opslevel.PayloadVariables { | ||
| variables := opslevel.PayloadVariables{ |
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 believe you can just return the literal, you don't need to assign it to a variable and return the variable
Resolves #
Problem
The MCP server wasn't able to handle questions about retrieving documents.
Solution
This MR provides three tools to empower users to query for their documents within opslevel. They may query for their documents in bulk unconstrained, within the context of a service (both with filters by doc name) & may query for a document individually to get the contents.
There seems to be a generic problem with response length + claude that has now appeared due to the number of documents that you can fetch. I'll address this in a separate MR.
Checklist