cli: hint when install-runner options are ignored#938
cli: hint when install-runner options are ignored#938YasharthPanwar-2003 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to provide users with a hint command when they attempt to install or start a Model Runner that is already active but with different configuration flags. It includes a new helper function to detect flag changes and comprehensive unit tests for the hint generation logic. Feedback was provided regarding a potential security and correctness issue where user-provided flag values are not properly escaped in the suggested command string, which could lead to issues if values contain spaces or malicious characters.
| if backendChanged && opts.backend != "" { | ||
| reinstallArgs = append(reinstallArgs, "--backend", opts.backend) | ||
| } | ||
| if gpuChanged && opts.gpuMode != "" { | ||
| reinstallArgs = append(reinstallArgs, "--gpu", opts.gpuMode) | ||
| } |
There was a problem hiding this comment.
Critical
- The suggested command string is constructed using unvalidated and unescaped user input from flags. This poses a security risk (command injection if a user copy-pastes a malicious string) and a correctness issue if values contain spaces. Values should be properly quoted/escaped for the shell.
Following the Security principle (Repository Style Guide, line 12), input must be handled safely when crossing boundaries, including when displayed back to the user as a suggested command.
| if backendChanged && opts.backend != "" { | |
| reinstallArgs = append(reinstallArgs, "--backend", opts.backend) | |
| } | |
| if gpuChanged && opts.gpuMode != "" { | |
| reinstallArgs = append(reinstallArgs, "--gpu", opts.gpuMode) | |
| } | |
| if backendChanged && opts.backend != "" { | |
| reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend)) | |
| } | |
| if gpuChanged && opts.gpuMode != "" { | |
| reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode)) | |
| } |
| @@ -0,0 +1,126 @@ | |||
| package commands | |||
There was a problem hiding this comment.
Better if you can put the tests in existing install-runner_test.go file
There was a problem hiding this comment.
Ok , I do that. May you please help me with if the gemini suggested changes to be done in code ?
There was a problem hiding this comment.
@YasharthPanwar-2003 if you check the code both of theopts.backend and the opts.gpuMode are validated by predefined sepcs. so it won't be a problem though but for defense in future its better to do the suggestion by gemini here like change to the below
if backendChanged && opts.backend != "" {
reinstallArgs = append(reinstallArgs, "--backend", fmt.Sprintf("%q", opts.backend))
}
if gpuChanged && opts.gpuMode != "" {
reinstallArgs = append(reinstallArgs, "--gpu", fmt.Sprintf("%q", opts.gpuMode))
}
Its always important to double check what these analyzers suggest because it sometimes can be wrong. Always try to check whether its the case
There was a problem hiding this comment.
and test this feature in local as well :)
There was a problem hiding this comment.
I dont have NVIDIA gpu cuda , but I tested it over wsl2 Ubuntu 22.04 for cpu only , after the new updation I will test again!
|
@YasharthPanwar-2003 thanks for looking at this, is the bot comment worth fixing? |
|
You will need to fix CI also |
Summary
install-runnerfinds an existing running Model Runner container--backendor--gpuFixes #907
What changed
commandFlagChangedto check whether a Cobra flag was explicitly changedexistingRunnerOptionsHintto build the reinstall command hintctrID != ""branch to print the hint after the current already-running messagecmd/cli/commands/install_runner_existing_options_hint_test.goWhat this does not change
reinstall-runnerbehaviorTests added
--backendand--gpuwere not explicitly passed--gpu nonehintValidation
go test ./cmd/cli/commands -run "TestInstallRunner|TestExistingRunnerOptionsHint|TestCommandFlagChanged" -vgo test ./...make validate-allValidation details
go.modtidy check passedgolangci-lint run ./...passed with 0 issuesgo test -race ./...passed.versionsis in sync with Dockerfile ARGs