chore(codersdk/toolsdk): improve static analyzability of toolsdk.Tools #17562
Conversation
coderd/workspaceagents.go
Outdated
| if len(req.Message) > 160 { | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Message is too long.", | ||
| Detail: "Message must be less than 160 characters.", | ||
| Validations: []codersdk.ValidationError{ | ||
| {Field: "message", Detail: "Message must be less than 160 characters."}, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| switch req.State { | ||
| case codersdk.WorkspaceAppStatusStateComplete, codersdk.WorkspaceAppStatusStateFailure, codersdk.WorkspaceAppStatusStateWorking: // valid states | ||
| default: | ||
| httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ | ||
| Message: "Invalid state provided.", | ||
| Detail: fmt.Sprintf("invalid state: %q", req.State), | ||
| Validations: []codersdk.ValidationError{ | ||
| {Field: "state", Detail: "State must be one of: complete, failure, working."}, | ||
| }, | ||
| }) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
review: moved this check to the handler instead.
6c97ac4 to
4a9ab00
Compare
codersdk/toolsdk/toolsdk.go
Outdated
| // Generic returns a type-erased version of a TypedTool where the arguments and | ||
| // return values are converted to/from json.RawMessage. | ||
| // This allows the tool to be referenced without knowing the concrete arguments | ||
| // or return values. The original TypedHandlerFunc is wrapped to handle type | ||
| // conversion. | ||
| func (t Tool[Arg, Ret]) Generic() GenericTool { | ||
| return GenericTool{ | ||
| Tool: t.Tool, | ||
| Handler: func(ctx context.Context, args map[string]any) (any, error) { | ||
| return t.Handler(ctx, args) | ||
| }, | ||
| Handler: wrap(func(ctx context.Context, tb Deps, args json.RawMessage) (json.RawMessage, error) { | ||
| var typedArgs Arg | ||
| if err := json.Unmarshal(args, &typedArgs); err != nil { | ||
| return nil, xerrors.Errorf("failed to unmarshal args: %w", err) | ||
| } | ||
| ret, err := t.Handler(ctx, tb, typedArgs) | ||
| var buf bytes.Buffer | ||
| if err := json.NewEncoder(&buf).Encode(ret); err != nil { | ||
| return json.RawMessage{}, err | ||
| } | ||
| return buf.Bytes(), err | ||
| }, WithCleanContext, WithRecover), |
There was a problem hiding this comment.
review: this was the best way I could find to maintain type safety while allowing external callers to treat all the tools equivalently.
There was a problem hiding this comment.
The other option would be to attempt to fill out the Arg via reflection, taking a map[string]any as "raw" input. Would definitely be more code, but might be a performance optimization in future for the MCP server which already parses the JSON of the tool invocation.
codersdk/toolsdk/toolsdk.go
Outdated
| var ( | ||
| // All is a list of all tools that can be used in the Coder CLI. | ||
| // When you add a new tool, be sure to include it here! | ||
| All = []Tool[any]{ | ||
| CreateTemplateVersion.Generic(), | ||
| All = []GenericTool{ |
There was a problem hiding this comment.
It would be useful to have different lists depending on whether you have an agent client and slug.
codersdk/toolsdk/toolsdk.go
Outdated
| // Generic returns a type-erased version of a TypedTool where the arguments and | ||
| // return values are converted to/from json.RawMessage. | ||
| // This allows the tool to be referenced without knowing the concrete arguments | ||
| // or return values. The original TypedHandlerFunc is wrapped to handle type | ||
| // conversion. | ||
| func (t Tool[Arg, Ret]) Generic() GenericTool { | ||
| return GenericTool{ | ||
| Tool: t.Tool, | ||
| Handler: func(ctx context.Context, args map[string]any) (any, error) { | ||
| return t.Handler(ctx, args) | ||
| }, | ||
| Handler: wrap(func(ctx context.Context, tb Deps, args json.RawMessage) (json.RawMessage, error) { | ||
| var typedArgs Arg | ||
| if err := json.Unmarshal(args, &typedArgs); err != nil { | ||
| return nil, xerrors.Errorf("failed to unmarshal args: %w", err) | ||
| } | ||
| ret, err := t.Handler(ctx, tb, typedArgs) | ||
| var buf bytes.Buffer | ||
| if err := json.NewEncoder(&buf).Encode(ret); err != nil { | ||
| return json.RawMessage{}, err | ||
| } | ||
| return buf.Bytes(), err | ||
| }, WithCleanContext, WithRecover), |
There was a problem hiding this comment.
The other option would be to attempt to fill out the Arg via reflection, taking a map[string]any as "raw" input. Would definitely be more code, but might be a performance optimization in future for the MCP server which already parses the JSON of the tool invocation.
3a98bbb to
2462f76
Compare
There was a problem hiding this comment.
I'm personally still in favor of the context...
We can still use *Deps as part of the context so not anything can be extracted, but all we're really doing is that, but with a layer of abstraction over all tools.
I don't want to cause Cian more rework so we can merge, but I don't think this accomplishes increased clarity or safety over doing the same thing, but in a context.
I really love how Cian did the args though - that is certainly much improved.
map[string]anyargument in favour of typed args structs.context.Contextin favour of a tool dependencies struct.GenericToolimplementation to allow keepingtoolsdk.Allwith uniform type signature while maintaining type information in handlers.patchWorkspaceAgentAppStatushandler.