feat: CP-1037 New MCP tool to create revision from dependency list #3706
Conversation
| namespace, err := mcpRequest.RequireString("namespace") | ||
| if err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("an ingredient namespace is required: %s", errs.JoinMessage(err))), nil | ||
| } | ||
| name, err := mcpRequest.RequireString("name") | ||
| if err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("an ingredient name is required: %s", errs.JoinMessage(err))), nil | ||
| } | ||
| version, err := mcpRequest.RequireString("version") | ||
| if err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("an ingredient version is required: %s", errs.JoinMessage(err))), nil | ||
| } | ||
| dependencies, err := mcpRequest.RequireString("dependencies") | ||
| if err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("the ingredient dependencies are required: %s", errs.JoinMessage(err))), nil | ||
| } | ||
| comment, err := mcpRequest.RequireString("comment") | ||
| if err != nil { | ||
| return mcp.NewToolResultError(fmt.Sprintf("a comment for the ingredient is required: %s", errs.JoinMessage(err))), nil | ||
| } |
There was a problem hiding this comment.
Is this necessary? You're already specifying mcp.Required().
There was a problem hiding this comment.
Doc for mcp.Required() says "Required marks a property as required in the tool's input schema. Required properties must be provided when using the tool."
I think if the LLM does not provide these required args, the MCP server will complain, so we should be safe here. Do you think we should ignore the errors in mcpRequest.RequireString() then?
There was a problem hiding this comment.
"RequireString returns a string argument by key, or an error if not found or not a string"
Not sure the LLM would provide something that is not a string, in which case the MCP server is probably not checking on that for us.
There was a problem hiding this comment.
I'd try to verify that the library itself already handles it. Basically it should not even reach your RequireString logic if it was already required. If it does I'd consider that a pretty awkward design on the libraries part.
There was a problem hiding this comment.
Even in the tool's examples, it suggests checking for the error. e.g. here
Do you suggest I just ignore the errors here by setting them to _?
| var dependencies []inventory_models.Dependency | ||
| err := json.Unmarshal([]byte(params.dependencies), &dependencies) | ||
| if err != nil { | ||
| return fmt.Errorf("error unmarshaling dependencies, dependency JSON is in wrong format: %w", err) |
There was a problem hiding this comment.
Please use errs.Wrap(). Goes for all the changes in this PR.
| getParams := inventory_operations.NewGetIngredientVersionRevisionsParams() | ||
| getParams.SetIngredientID(*ingredient.IngredientID) | ||
| getParams.SetIngredientVersionID(*ingredient.IngredientVersionID) | ||
|
|
||
| client := inventory.Get(runner.auth) | ||
| revisions, err := client.GetIngredientVersionRevisions(getParams, runner.auth.ClientAuth()) | ||
| if err != nil { | ||
| return fmt.Errorf("error getting version revisions: %w", err) | ||
| } |
There was a problem hiding this comment.
Please make this a function inside the model package, just like the model.GetIngredientByNameAndVersion function above.
Goes for the other requests here also. Point is to keep the controller logic concerned with control flow, not with constructing API requests.
| buildScriptIDs = append(buildScriptIDs, *script.BuildScriptID) | ||
| } | ||
|
|
||
| // Replicate patches |
There was a problem hiding this comment.
Seeing a pattern here; seems these are all related to the single action of creating a new revision?
I'd suggest making that itself a function in the model package, which can itself reach out to other model functions. So you'd have something like model.CreateNewRevision(source *Ingredient, ...).
Logic here seems sound, just concerned about reusability and keeping things object oriented.
… to inventory model
| case "default": | ||
| default_optsets = append(default_optsets, *optset.IngredientOptionSetID) | ||
| case "override": | ||
| override_optsets = append(override_optsets, *optset.IngredientOptionSetID) |
There was a problem hiding this comment.
Suggest adding a default handler that errors out (eg. unrecognized type). Even if there are only 2, that should future proof us somewhat in the sense that if this change in the future it'll be immediately apparent that our code needs updating.
Uh oh!
There was an error while loading. Please reload this page.