Add MCP tool for querying a service#42
Conversation
AGE-159 Tiger CLI: Database tool calls
I did not implement the Database Operations tool calls from the v0 Tool Priority section of the MCP spec in AGE-121. We should implement them.
|
| // Build connection string for testing with password (if available) | ||
| connectionString, err := password.BuildConnectionString(service, password.ConnectionStringOptions{ | ||
| Pooled: dbTestConnectionPooled, | ||
| Role: dbTestConnectionRole, | ||
| PasswordMode: password.PasswordOptional, | ||
| WarnWriter: cmd.ErrOrStderr(), | ||
| }) |
There was a problem hiding this comment.
This is the only place we use PasswordOptional. That causes BuildConnectionString to attempt to include the password if possible, but to return the connection string without the password if the password can't be found.
The previous code would build the connection string without the password, but then testDatabaseConnection would call buildConnectionConfig, which parsed the connection string into a config struct and attempted to add the password to the config, if possible. That seemed like a pretty roundabout way of doing it (building a connection string just to parse it back into a config), so I got rid of that logic and added the PasswordOptional option when building the connection string.
That being said, I'm not 100% convinced it's worth it to return the connection string without a password here. The old code in buildConnectionConfig had this comment:
// Note: If keyring password retrieval fails, we let pgx try without it
// This allows fallback to other authentication methods
But I'm not sure what "other authentication methods" it's talking about here, or if it's really worth worrying about it. If we don't need to support this, we could get rid of PasswordOptional and replace PasswordMode with a PasswordRequired boolean, which would probably make the code a little more straightforward/clear.
There was a problem hiding this comment.
The other method I originally had in mind was PGPASSWORD env variable being set in the shell or a .pgpassword file (even if not set by the cli). I still think its worth allowing this kind of fallback.
| // Build connection string for testing with password (if available) | ||
| connectionString, err := password.BuildConnectionString(service, password.ConnectionStringOptions{ | ||
| Pooled: dbTestConnectionPooled, | ||
| Role: dbTestConnectionRole, | ||
| PasswordMode: password.PasswordOptional, | ||
| WarnWriter: cmd.ErrOrStderr(), | ||
| }) |
There was a problem hiding this comment.
The other method I originally had in mind was PGPASSWORD env variable being set in the shell or a .pgpassword file (even if not set by the cli). I still think its worth allowing this kind of fallback.
| // registerDatabaseTools registers database operation tools with comprehensive schemas and descriptions | ||
| func (s *Server) registerDatabaseTools() { | ||
| mcp.AddTool(s.mcpServer, &mcp.Tool{ | ||
| Name: "db_execute_query", |
There was a problem hiding this comment.
as a followup/discussion point: should we have a separate tool for "read-only" that make sure you are using a read-only role or connection? Alternatively, we can have another input parameter "enforce read-only"
There was a problem hiding this comment.
Yeah, I think a separate input parameter is probably the way to go. I do think we should do that in a follow-up though.
| // DBExecuteQueryInput represents input for tiger_db_execute_query | ||
| type DBExecuteQueryInput struct { | ||
| ServiceID string `json:"service_id"` | ||
| Query string `json:"query"` |
There was a problem hiding this comment.
I think we should also take in query parameter array for $1, $2 substitution to help llm avoid sql injection
There was a problem hiding this comment.
I considered this too, but on further reflection, I'm not really sure what it would be guarding against. If the entire SQL query is crafted by the LLM anyways, what's the value of having it pass parameters separately? It isn't really like a normal backend, where you have predefined "safe" queries that you want to parameterize with "unsafe" user-provided input. In this case, both the queries and the parameters are coming from the same LLM.
That being said, I'm not opposed to supporting parameters, as a way of giving the LLM more flexibility. For example, if a user provided a query with placeholders, it would allow the LLM to just execute it directly with specific parameters without having to modify the query. So yeah, I can implement this (I just think it's worth pointing out that I don't really think it affects security in any meaningful way).
There was a problem hiding this comment.
Support for parameterized queries added here: 178a33d
| schema.Properties["role"].Examples = []any{"tsdbadmin", "readonly", "postgres"} | ||
|
|
||
| schema.Properties["pooled"].Description = "Use connection pooling (if available for the service)" | ||
| schema.Properties["pooled"].Default = util.Must(json.Marshal(false)) |
There was a problem hiding this comment.
I think default should be true?
There was a problem hiding this comment.
It defaults to false for tiger db connect. What's your rationale for the difference?
There was a problem hiding this comment.
I think thats wrong there too. create an issue https://linear.app/tigerdata/issue/AGE-200/make-pooled=true-the-default-in-climcp
| schema.Properties["role"].Examples = []any{"tsdbadmin", "readonly", "postgres"} | ||
|
|
||
| schema.Properties["pooled"].Description = "Use connection pooling (if available for the service)" | ||
| schema.Properties["pooled"].Default = util.Must(json.Marshal(false)) |
There was a problem hiding this comment.
I think thats wrong there too. create an issue https://linear.app/tigerdata/issue/AGE-200/make-pooled=true-the-default-in-climcp
2 participants
Adds a new
db_execute_querytool (I omitted thetiger_prefix, per the changes in #30) that is capable of executing a SQL query and returning the results.I made a handful of small changes compared to the original spec, such as:
timeouttotimeout_secondsfor clarity (similar changes were made in feat: comprehensive MCP service tool improvements #30).roleandpooledparameters for parity with thetiger db connectflags.SELECT), but also shows the number of rows modified forINSERT/UPDATE/DELETEqueries, making it useful in those scenarios as well.time.Durationoutput format, rather than in milliseconds. For long-running queries, this will hopefully make the duration easier for both humans and LLMs to parse and understand.I moved the logic for building connection strings from the
internal/tiger/cmdpackage intointernal/tiger/passwordso that it could be shared with the MCP tools. That seemed like a fairly reasonable place to put it, even if not entirely ideal. The code depends on theinternal/tiger/apipackage, so it can't go inutil, and I didn't want to make another package just for the connection string logic (I think having a bunch of very small packages is kind of an anti-pattern in Go). I think we may need to rethink our package organization at some point, but this seemed good enough for now 🤷♂️.Note that there was talk about also adding an MCP tool for executing a query from a file (see here). I did not do that in this PR, but it should be easy to do in a follow-up if we want it.
Closes AGE-159