Add service_start and service_stop MCP tools#104
Conversation
…reduce duplication
… full service details
| // NewSpinner creates and returns a new [Spinner] for displaying animated | ||
| // status messages. If output is a terminal, it uses bubbletea to dynamically | ||
| // update the spinner and message in place. If output is not a terminal, it | ||
| // prints each message on a new line without animation. | ||
| // status messages. If the output is nil or [io.Discard], it returns a no-op | ||
| // spinner. If output is a terminal, it uses bubbletea to dynamically update | ||
| // the spinner and message in place. If output is not a terminal, it prints | ||
| // each message on a new line without animation. | ||
| func NewSpinner(output io.Writer, message string) Spinner { | ||
| if output == nil || output == io.Discard { | ||
| return newNopSpinner() | ||
| } |
There was a problem hiding this comment.
This change makes it possible to use WaitForService in the MCP tools without worrying about the spinner (the MCP tools pass in a nil output parameter, which results in a no-op spinner).
| // Include password in ServiceDetail if requested | ||
| if input.WithPassword { | ||
| output.Service.Password = util.Deref(service.InitialPassword) | ||
| } | ||
|
|
||
| // Always include connection string in ServiceDetail | ||
| // Password is embedded in connection string only if with_password=true | ||
| if details, err := password.GetConnectionDetails(api.Service(service), password.ConnectionDetailsOptions{ | ||
| Role: "tsdbadmin", | ||
| WithPassword: input.WithPassword, | ||
| InitialPassword: util.Deref(service.InitialPassword), | ||
| }); err != nil { | ||
| logging.Debug("MCP: Failed to build connection string", zap.Error(err)) | ||
| } else { | ||
| if input.WithPassword && details.Password == "" { | ||
| // This should not happen since we have InitialPassword, but check just in case | ||
| logging.Error("MCP: Requested password but password not available") | ||
| } | ||
| output.Service.ConnectionString = details.String() | ||
| } |
There was a problem hiding this comment.
All of this connection string/password stuff was duplicated between multiple MCP tool implementations. I DRY'd things up by moving it into convertToServiceDetail.
| // Convert service to output format (after wait so status is accurate) | ||
| output := ServiceCreateOutput{ | ||
| Service: s.convertToServiceDetail(service, input.WithPassword), | ||
| Message: message, | ||
| PasswordStorage: passwordStorage, | ||
| } |
There was a problem hiding this comment.
I restructured things to avoid mutating the output struct, and instead to create it at the end before returning it. I think this makes things more clear and easy to follow. Same for the other tool handlers.
| - `wait` (boolean, optional): Wait for service to be fully started before returning - default: false | ||
| - `timeout_minutes` (number, optional): Timeout for waiting in minutes - default: 10 |
There was a problem hiding this comment.
Couldn't these be combined into one optional param? (where there's no wait if it's not specified, otherwise it waits the specified number of minutes)
There was a problem hiding this comment.
Yeah, that's a good point. I made a backlog ticket about rethinking these parameters, since they're present for a bunch of tool calls now (not just the ones I added in this PR), and I think we should look at them holistically and improve them across the board (which I think is out-of-scope for this PR): AGE-272.
Askir
left a comment
Askir
left a comment
There was a problem hiding this comment.
Generally looks good. I would on a more generic level question if we need the MCP tools to start and stop at all. I think these probably just confuse an LLM and don't need to be present all the time. And similarly to Smitty I am not sure about the timeout params, I'd remove those altogether to reduce mental load on the LLM if possible.
But the structural changes are definitely good.
| schema.Properties["wait"].Default = util.Must(json.Marshal(false)) | ||
| schema.Properties["wait"].Examples = []any{false, true} | ||
|
|
||
| schema.Properties["timeout_minutes"].Description = "Timeout in minutes when waiting for service to start. Only used when 'wait' is true." | ||
| schema.Properties["timeout_minutes"].Minimum = util.Ptr(0.0) | ||
| schema.Properties["timeout_minutes"].Default = util.Must(json.Marshal(10)) | ||
| schema.Properties["timeout_minutes"].Examples = []any{5, 10, 15} |
There was a problem hiding this comment.
This seems to be a pattern we follow in all tools but I think we'd ideally just use a good default for timeouts and not make it configurable by the LLM.
It saves tokens and simplifies the signature. I would also reconsider if wait false should be the default I think usually if you start a service you want to use it afterwards, so doing it in the background seems odd.
There was a problem hiding this comment.
This seems to be a pattern we follow in all tools but I think we'd ideally just use a good default for timeouts and not make it configurable by the LLM. It saves tokens and simplifies the signature.
Hmm, yeah, that seems reasonable. I've never seen the LLM choose a different timeout without being explicitly prompted to, so you're probably right that the parameter is just wasting tokens. Or alternatively, we could do what Smitty suggested above and combine wait and timeout_minutes into a single parameter that takes the number of minutes to wait (and doesn't wait if it's not provided). In either case, we could probably get rid of at least one of these two parameters.
I would also reconsider if wait false should be the default I think usually if you start a service you want to use it afterwards, so doing it in the background seems odd.
We made all of the MCP tools default to wait: false thus far, because some of these operations can take a long time (e.g. service creation/forking), and holding up the LLM for the entire time is really noticeable and breaks the flow. I guess we could make different tools have different defaults, but I'm worried about the potential confusion arising from the inconsistency.
I created AGE-272 about rethinking these parameters, because I think it's a bigger question that's out-of-scope for this particular PR (which is just following the existing convention).
3 participants
This PR adds two new MCP tools corresponding to the CLI commands added in #103:
service_start- Starts a stopped/paused databaseservice_stop- Stops a running databaseBoth tools accept
service_id,wait, andtimeout_minutesparameters, and return just the servicestatusand amessagefield describing the outcome. I opted not to return the full service details (though they're available and could be returned) to save tokens, and because the LLM can always callservice_getif it needs the full details (open to other thoughts/feedback on this, though).I also did some refactoring:
waitForServicefunction (and its dependencies, like the spinner and error codes) frominternal/tiger/cmdinto a newinternal/tiger/commonpackage, so that it can be reused in theinternal/tiger/mcppackage. Previously, we had duplicate versions of the wait logic for the CLI and MCP tools, and it was annoying to keep them in sync. Now there is just a single configurableWaitForServicefunction that's used everywhere./internal/tiger/passwordpackage, and moved its contents into the new/internal/tiger/commonpackage as well. Thepasswordpackage had been added in Add MCP install command and next steps after login #26, when we had to move some password-related stuff out ofinternal/tiger/utilsin order to resolve circular reference issues. However, now that we have more code to share between the CLI commands and MCP tools beyond just "password" related stuff, I figured a singlecommonpackage made more sense than a bunch of small shared packages.internal/tiger/utilsintointernal/tiger/commonas well. Particularly the functions that contained "business logic". I only left fairly simple, self-contained utilities functions (with few dependencies) in theutilspackage.convertToServiceDetailfunction.Finally, I updated a bunch of documentation to reflect the above changes (as well as the changes from #103, which I had previously forgotten to document).
Closes AGE-250
CC @billy-the-fish for docs update.