- Notifications
You must be signed in to change notification settings - Fork924
chore(codersdk/toolsdk): improve static analyzability of toolsdk.Tools#17562
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: moved this check to the handler instead.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The other option would be to attempt to fill out theArg via reflection, taking amap[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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The other option would be to attempt to fill out theArg via reflection, taking amap[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.
Uh oh!
There was an error while loading.Please reload this page.
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 personally still in favor of the context...
We can still use*Deps as part of the context so notanything 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 certainlymuch improved.
2acf0ad intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.