feat: add 'docker model launch' cmd #618
Conversation
Summary of ChangesHello @denyszhak, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the Docker Model Runner CLI by adding a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new docker model launch command, which is a convenient way to launch applications configured to use Docker Model Runner. The implementation is well-structured and includes comprehensive tests. I have a couple of suggestions to improve the command-line user experience and maintainability.
|
Created draft for an early feedback and potential Gemini fixes, doing some manual testing atm |
ericcurtin
left a comment
ericcurtin
left a comment
There was a problem hiding this comment.
This looks great to me. I think we should merge this soon. We can even allow breaking changes for a few weeks if needs be.
denyszhak
force-pushed
the
feature/docker-model-launch-cmd
branch
from
42e26dc to
2e82981
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code. (link)
General comments:
- For unconfigurable host apps like
clawdbot,launchHostAppstill requires the binary to be present (exec.LookPath) before printing configuration guidance; consider skipping theLookPathcheck (or handling failures differently) whenenvFnis nil so users can at least get setup instructions even if the app is not installed yet. resolveBaseEndpointsis documented as returning base URLs “without path”, but for the Desktop engine it currently returnsmodelRunner.URL(""), which appears to already includeExperimentalEndpointsPrefix; combined withopenaiEnvappendingopenaiPathSuffixthis risks double-suffixing the path—either normalizeresolveBaseEndpointsto always return a true root URL or makeopenaiEnvrobust to already-suffixed base URLs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For unconfigurable host apps like `clawdbot`, `launchHostApp` still requires the binary to be present (`exec.LookPath`) before printing configuration guidance; consider skipping the `LookPath` check (or handling failures differently) when `envFn` is nil so users can at least get setup instructions even if the app is not installed yet.
- `resolveBaseEndpoints` is documented as returning base URLs “without path”, but for the Desktop engine it currently returns `modelRunner.URL("")`, which appears to already include `ExperimentalEndpointsPrefix`; combined with `openaiEnv` appending `openaiPathSuffix` this risks double-suffixing the path—either normalize `resolveBaseEndpoints` to always return a true root URL or make `openaiEnv` robust to already-suffixed base URLs.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/launch.go:90-99` </location>
<code_context>
+ Use: "launch APP [-- APP_ARGS...]",
</code_context>
<issue_to_address>
**issue (bug_risk):** Container apps ignore APP_ARGS despite the usage string advertising them.
`newLaunchCmd` reads `appArgs := args[1:]`, but `launchContainerApp` never receives or passes these to `docker run`, so args after `APP` are ignored for containerized apps. Either propagate `appArgs` into `launchContainerApp` and append them after the image name in `dockerArgs`, or update the usage string to clarify that extra args only apply to host apps.
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/launch.go:127-136` </location>
<code_context>
+
+// resolveBaseEndpoints resolves the base URLs (without path) for both
+// container and host client locations.
+func resolveBaseEndpoints(runner *standaloneRunner) (engineEndpoints, error) {
+ kind := modelRunner.EngineKind()
+ switch kind {
+ case types.ModelRunnerEngineKindDesktop:
+ return engineEndpoints{
+ container: "http://model-runner.docker.internal",
+ host: strings.TrimRight(modelRunner.URL(""), "/"),
+ }, nil
+ case types.ModelRunnerEngineKindMobyManual:
+ ep := strings.TrimRight(modelRunner.URL(""), "/")
+ containerEP := strings.NewReplacer(
+ "localhost", "host.docker.internal",
+ "127.0.0.1", "host.docker.internal",
+ ).Replace(ep)
+ return engineEndpoints{container: containerEP, host: ep}, nil
+ case types.ModelRunnerEngineKindCloud, types.ModelRunnerEngineKindMoby:
+ if runner == nil {
+ return engineEndpoints{}, errors.New("unable to determine standalone runner endpoint")
</code_context>
<issue_to_address>
**issue:** Cloud/Moby runners with only `hostPort` set produce an empty container endpoint, which may break container apps.
In the Cloud/Moby branch, when `runner.gatewayIP`/`gatewayPort` are unset but `runner.hostPort != 0`, `engineEndpoints.container` remains empty while `host` is set. `launchContainerApp` then uses `ep.container` to build env vars, so container apps get a base URL with no endpoint. Consider either falling back to the host URL for `container` in this case, or failing fast (e.g., return an error) if `container` is empty when launching a container app.
</issue_to_address>
### Comment 3
<location> `cmd/cli/commands/launch.go:298` </location>
<code_context>
c := exec.Command(prog, progArgs...)
</code_context>
<issue_to_address>
**security (go.lang.security.audit.dangerous-exec-command):** Detected non-static command inside Command. Audit the input to 'exec.Command'. If unverified user data can reach this call site, this is a code injection vulnerability. A malicious actor can inject a malicious script to execute arbitrary code.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
I tested e2e with the model: Codex, Claude, anythingllm, openwebui for now, moltbot referes in the documentation to moltbot but in reality it's clawdbot and now it's open-claw?) anyway this one can be tested once they set on the final name opencode connects successfully but I didn't test custom model stuff with it Overall, each of the providers requires some small tweaks here and there so it makes perfect sense to test/add support one by one as you pointed in your comment under the original issue. I'm good to merge it in it's current state so your call:) |
|
|
||
| // resolveBaseEndpoints resolves the base URLs (without path) for both | ||
| // container and host client locations. | ||
| func resolveBaseEndpoints(runner *standaloneRunner) (engineEndpoints, error) { |
There was a problem hiding this comment.
nice, let me take a look
There was a problem hiding this comment.
I used status for reference but still there are some differences between the two. maybe those differences only because I tried to make this running on my docker desktop and in reality jsonStatus would suffice or alternatively I could review it for the refactor.
Are you saying jsonStatus could be dropin replacement of this one to resolve endpoints?
There was a problem hiding this comment.
@doringeman
jsonLaunch is informational and it uses fallbacks without errors where if we do permissive fallbacks in launch it can lead to side effects so let me know what do you think is best here based on the context you know better
Co-authored-by: Dorin-Andrei Geman <doringeman@gmail.com>
Co-authored-by: Dorin-Andrei Geman <doringeman@gmail.com>
We should go with openclaw now, but it's not a blocker and be in a future PR if needs be. |
3 participants
Change:
Add "docker model launch" convenience command
Closes #598
Testing: