feat(cli): add support for direct tool invocation from CLI #2353
Conversation
averikitsch
left a comment
There was a problem hiding this comment.
@Yuan325 can you do a first pass of this code review?
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a call-tool subcommand, which is a great addition for testing and debugging tools directly from the CLI. The implementation is well-structured, with good test coverage and documentation updates.
I've identified a couple of areas for improvement:
- There's some code duplication for logger and telemetry setup that could be refactored for better maintainability.
- There's a minor bug in how a deprecated flag is marked.
Overall, this is a solid feature addition. Addressing these points will make the codebase even more robust.
| flags.StringVar(&cmd.tools_file, "tools_file", "", "File path specifying the tool configuration. Cannot be used with --tools-files, or --tools-folder.") | ||
| persistentFlags.StringVar(&cmd.tools_file, "tools_file", "", "File path specifying the tool configuration. Cannot be used with --tools-files, or --tools-folder.") | ||
| // deprecate tools_file | ||
| _ = flags.MarkDeprecated("tools_file", "please use --tools-file instead") |
There was a problem hiding this comment.
The tools_file flag is defined on the persistentFlags set, but MarkDeprecated is being called on the local flags set. This means the deprecation warning will not be shown as intended, because the flag doesn't exist in that set.
To fix this, MarkDeprecated should be called on persistentFlags.
| _ = flags.MarkDeprecated("tools_file", "please use --tools-file instead") | |
| _ = persistentFlags.MarkDeprecated("tools_file", "please use --tools-file instead") |
There was a problem hiding this comment.
@averikitsch Are we good that the new subcommands will not support the deprecated tools_file flag?
| // Initialize Logger | ||
| var logger log.Logger | ||
| var err error | ||
| switch strings.ToLower(cmd.cfg.LoggingFormat.String()) { | ||
| case "json": | ||
| logger, err = log.NewStructuredLogger(cmd.outStream, cmd.errStream, cmd.cfg.LogLevel.String()) | ||
| case "standard": | ||
| logger, err = log.NewStdLogger(cmd.outStream, cmd.errStream, cmd.cfg.LogLevel.String()) | ||
| default: | ||
| return fmt.Errorf("logging format invalid") | ||
| } | ||
| if err != nil { | ||
| return fmt.Errorf("unable to initialize logger: %w", err) | ||
| } | ||
| cmd.logger = logger | ||
| ctx = util.WithLogger(ctx, cmd.logger) | ||
|
|
||
| // Set up OpenTelemetry | ||
| otelShutdown, err := telemetry.SetupOTel(ctx, cmd.cfg.Version, cmd.cfg.TelemetryOTLP, cmd.cfg.TelemetryGCP, cmd.cfg.TelemetryServiceName) | ||
| if err != nil { | ||
| return fmt.Errorf("error setting up OpenTelemetry: %w", err) | ||
| } | ||
| defer func() { | ||
| _ = otelShutdown(ctx) | ||
| }() | ||
|
|
||
| instrumentation, err := telemetry.CreateTelemetryInstrumentation(versionString) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to create telemetry instrumentation: %w", err) | ||
| } | ||
| ctx = util.WithInstrumentation(ctx, instrumentation) |
There was a problem hiding this comment.
There is significant code duplication for logger and telemetry initialization between this function and run() in cmd/root.go. This can lead to maintenance issues where a change in one place might be missed in the other.
To improve this, consider extracting the common setup logic into a new method on the Command struct. This method could handle:
- Logger initialization.
- OpenTelemetry setup.
- Instrumentation creation.
This new method could then be called from both runCallTool and run, ensuring consistency and making future changes easier.
There was a problem hiding this comment.
Might be leaning towards following the modular layout of organizing the subcommands since we will be adding more subcommand during the implementation of advance control plane. (https://cobra.dev/docs/how-to-guides/working-with-commands/ check out the "How to Organize Commands in Packages" section)
| } | ||
|
|
||
| func runCallTool(cmd *Command, args []string) error { | ||
| ctx := cmd.Context() |
There was a problem hiding this comment.
| ctx := cmd.Context() | |
| ctx := context.WithCancel(cmd.Context()) | |
| defer cancel() |
| flags.StringVar(&cmd.tools_file, "tools_file", "", "File path specifying the tool configuration. Cannot be used with --tools-files, or --tools-folder.") | ||
| persistentFlags.StringVar(&cmd.tools_file, "tools_file", "", "File path specifying the tool configuration. Cannot be used with --tools-files, or --tools-folder.") | ||
| // deprecate tools_file | ||
| _ = flags.MarkDeprecated("tools_file", "please use --tools-file instead") |
There was a problem hiding this comment.
@averikitsch Are we good that the new subcommands will not support the deprecated tools_file flag?
| } | ||
|
|
||
| // Initialize Resources | ||
| sourcesMap, authServicesMap, embeddingModelsMap, toolsMap, toolsetsMap, promptsMap, promptsetsMap, err := server.InitializeConfigs(ctx, cmd.cfg) |
There was a problem hiding this comment.
With this implementation, it seems like Toolbox will initialize the whole server every time this subcommand it's called. Is this intended? Or do we want to use this subcommand as like a "HTTP Client" shim that will invoke the underlying endpoint of a deployed server?
|
|
||
| Executes a tool directly with the provided parameters. This is useful for testing tool configurations and parameters without needing a full client setup. | ||
|
|
||
| **Usage:** |
There was a problem hiding this comment.
| **Usage:** | |
| **Syntax** |
| | | `--user-agent-extra` | Appends additional metadata to the User-Agent. | | | ||
| | `-v` | `--version` | version for toolbox | | | ||
|
|
||
| ## Sub Commands |
There was a problem hiding this comment.
note: we can keep this for now, but eventually might want to move to a drop down of subcommand references when this increases.
Description
This PR introduces a new subcommand, call-tool, to the toolbox CLI. This feature allows developers to execute tools defined in their configuration directly from the command line.