feat(genkit-tools/cli): Add config command to set/unset runtimeCommand#5111
feat(genkit-tools/cli): Add config command to set/unset runtimeCommand#5111ssbushi wants to merge 2 commits intosb/self-clifrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces project-specific configuration stored in .genkit/genkit.json and adds support for ephemeral runtimes to several CLI commands, allowing users to define a runtimeCommand via settings or the command line. Key feedback involves improving the robustness of command string parsing, ensuring consistent fallback to project settings across all commands, and adding safety checks when logging trace IDs to avoid undefined values.
I am having trouble creating individual review comments. Click here to see my feedback.
genkit-tools/cli/src/commands/eval-flow.ts (125)
The runtimeCommand is being split by a single space, which is a naive approach that will fail if the command contains arguments with spaces (e.g., node -e "console.log('hi')"). Consider using a library like shell-quote or a more robust regex to parse the command string into an array of arguments.
genkit-tools/cli/src/commands/eval-extract-data.ts (127-131)
This command is missing the logic to check for a configured runtimeCommand in the project settings when one is not provided via the CLI. This creates an inconsistency with other commands like eval:flow and flow:run.
References
- Ensure consistency between different API versions and command implementations to maintain a uniform behavior across the repository.
genkit-tools/cli/src/commands/flow-batch-run.ts (123-127)
This command is missing the logic to check for a configured runtimeCommand in the project settings when one is not provided via the CLI. For consistency across the CLI, it should fallback to the project configuration.
References
- Ensure consistency between different API versions and command implementations to maintain a uniform behavior across the repository.
genkit-tools/common/src/eval/evaluate.ts (197)
The Trace ID is logged without checking if it exists, which could result in logging Trace ID: undefined. It's better to wrap this in a conditional check.
if (response.telemetry?.traceId) {
logger.info(clc.cyan('Trace ID:') + ' ' + response.telemetry.traceId);
}
Add config command to save runtime command.
Set:
Unset:
Checklist (if applicable):