Add host/port/url/url-alias flags to pull, push, run, and list commands #251
Conversation
Add external OpenAI endpoint support Add comprehensive tests for new server connection flags Signed-off-by: Eric Curtin <[email protected]>
Reviewer's GuideThis PR adds comprehensive support for custom server endpoints by introducing new connection flags (--host, --port, --url, --url-alias) across pull, push, run, and list commands. It centralizes endpoint resolution via resolveServerURL, conditionally overrides the model-runner context for local Docker or external OpenAI-compatible servers, enhances the listModels logic for varied OpenAI output formats, and updates the desktop client to route requests correctly. A full suite of tests validates flag registration, URL resolution, and output formatting. Sequence diagram for overriding model runner context based on connection flagssequenceDiagram
actor User
participant CLI
participant "resolveServerURL()"
participant "overrideModelRunnerContextFromURL()"
participant "overrideModelRunnerContext()"
User->>CLI: Run command with connection flags
CLI->>resolveServerURL(): Parse flags
resolveServerURL-->>CLI: Return serverURL, useOpenAI, apiKey
CLI->>overrideModelRunnerContextFromURL(): If serverURL set
overrideModelRunnerContextFromURL-->>CLI: Update context
CLI->>overrideModelRunnerContext(): Else if host/port set
overrideModelRunnerContext-->>CLI: Update context
Class diagram for updated ModelRunnerContext and ServerPreset typesclassDiagram
class ModelRunnerContext {
+kind ModelRunnerEngineKind
+urlPrefix url.URL
+client DockerHttpClient
+externalOpenAI bool
+IsExternalOpenAI() bool
}
class ServerPreset {
+Name string
+URL string
}
ModelRunnerContext -- DockerHttpClient
ModelRunnerContext -- url.URL
ServerPreset "*" -- "1" ValidBackends : presets
Class diagram for updated desktop.Client request routingclassDiagram
class Client {
+List()
+ListOpenAI(backend, apiKey)
+ChatWithContext(ctx, backend, model, prompt, apiKey)
-modelRunner ModelRunnerContext
}
Client -- ModelRunnerContext
Client : ListOpenAI() uses IsExternalOpenAI() to select route
Client : ChatWithContext() uses IsExternalOpenAI() to select route
Flow diagram for server endpoint resolution via resolveServerURLflowchart TD
A["User provides flags: --host, --port, --url, --url-alias"] --> B["resolveServerURL()"]
B --> C{"Which flags are set?"}
C -->|--url| D["Set serverURL = customURL, useOpenAI = true"]
C -->|--url-alias| E["Lookup preset, set serverURL, useOpenAI = true"]
C -->|--host/--port| F["Set serverURL = http://host:port, useOpenAI = false"]
D --> G["Return serverURL, useOpenAI, apiKey"]
E --> G
F --> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @ericcurtin, 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 significantly enhances the flexibility of the CLI by allowing users to specify custom server endpoints for interacting with model runners. It introduces new flags for host, port, and URL configuration, along with aliases for popular OpenAI-compatible services. This change streamlines the process of connecting to various local and remote inference servers, making the tool more adaptable to diverse deployment scenarios. Highlights
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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There’s a lot of repeated flag‐lookup code in the new server flag tests; consider extracting a helper to reduce duplication and improve readability.
- The resolveServerURL function is handling several distinct flows in one place—splitting out URL alias resolution, host/port defaults, and conflict checks into smaller helpers could make it easier to follow and maintain.
- overrideModelRunnerContext and overrideModelRunnerContextFromURL share very similar context‐switch logic; consolidating them (or sharing a common setup path) would cut down on duplicated code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repeated flag‐lookup code in the new server flag tests; consider extracting a helper to reduce duplication and improve readability.
- The resolveServerURL function is handling several distinct flows in one place—splitting out URL alias resolution, host/port defaults, and conflict checks into smaller helpers could make it easier to follow and maintain.
- overrideModelRunnerContext and overrideModelRunnerContextFromURL share very similar context‐switch logic; consolidating them (or sharing a common setup path) would cut down on duplicated code.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/backend_test.go:151` </location>
<code_context>
+ }
+}
+
+func TestValidateBackend(t *testing.T) {
+ tests := []struct {
+ name string
</code_context>
<issue_to_address>
**nitpick (testing):** Tests for validateBackend do not check error message content.
Add assertions to verify that error messages are clear and match expected content.
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/list_test.go:11-20` </location>
<code_context>
+func TestPrettyPrintOpenAIModels(t *testing.T) {
</code_context>
<issue_to_address>
**suggestion (testing):** Tests for prettyPrintOpenAIModels do not cover models with missing or zero Created timestamps.
Add a test for models with a zero or missing Created field to verify correct handling and output formatting.
Suggested implementation:
```golang
modelList := dmrm.OpenAIModelList{
Object: "list",
Data: []*dmrm.OpenAIModel{
{
ID: "llama3.2:3b",
Object: "model",
Created: time.Now().Unix() - 3600, // 1 hour ago
OwnedBy: "docker",
},
{
ID: "missing-created",
Object: "model",
Created: 0, // Zero timestamp
OwnedBy: "docker",
},
// If the struct allows omitting Created, you could add:
// {
// ID: "no-created-field",
// Object: "model",
// OwnedBy: "docker",
// },
```
```golang
output := prettyPrintOpenAIModels(modelList)
if !strings.Contains(output, "llama3.2:3b") {
t.Errorf("Expected output to contain model ID llama3.2:3b")
}
if !strings.Contains(output, "missing-created") {
t.Errorf("Expected output to contain model ID missing-created")
}
if !strings.Contains(output, "N/A") && !strings.Contains(output, "missing-created") {
t.Errorf("Expected output to show 'N/A' or similar for zero/missing Created timestamp")
}
// If you add a model with no Created field, check for its output as well.
```
If the `dmrm.OpenAIModel` struct does not allow omitting the `Created` field (i.e., it's not a pointer or omitempty), you can only test with zero value. If it does, add a model without the `Created` field and update the test accordingly.
You may need to adjust the output check depending on how `prettyPrintOpenAIModels` formats models with zero or missing timestamps (e.g., "N/A", "Unknown", or blank).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| func TestValidateBackend(t *testing.T) { |
There was a problem hiding this comment.
nitpick (testing): Tests for validateBackend do not check error message content.
Add assertions to verify that error messages are clear and match expected content.
| func TestPrettyPrintOpenAIModels(t *testing.T) { | ||
| modelList := dmrm.OpenAIModelList{ | ||
| Object: "list", | ||
| Data: []*dmrm.OpenAIModel{ | ||
| { | ||
| ID: "llama3.2:3b", | ||
| Object: "model", | ||
| Created: time.Now().Unix() - 3600, // 1 hour ago | ||
| OwnedBy: "docker", | ||
| }, |
There was a problem hiding this comment.
suggestion (testing): Tests for prettyPrintOpenAIModels do not cover models with missing or zero Created timestamps.
Add a test for models with a zero or missing Created field to verify correct handling and output formatting.
Suggested implementation:
modelList := dmrm.OpenAIModelList{
Object: "list",
Data: []*dmrm.OpenAIModel{
{
ID: "llama3.2:3b",
Object: "model",
Created: time.Now().Unix() - 3600, // 1 hour ago
OwnedBy: "docker",
},
{
ID: "missing-created",
Object: "model",
Created: 0, // Zero timestamp
OwnedBy: "docker",
},
// If the struct allows omitting Created, you could add:
// {
// ID: "no-created-field",
// Object: "model",
// OwnedBy: "docker",
// }, output := prettyPrintOpenAIModels(modelList)
if !strings.Contains(output, "llama3.2:3b") {
t.Errorf("Expected output to contain model ID llama3.2:3b")
}
if !strings.Contains(output, "missing-created") {
t.Errorf("Expected output to contain model ID missing-created")
}
if !strings.Contains(output, "N/A") && !strings.Contains(output, "missing-created") {
t.Errorf("Expected output to show 'N/A' or similar for zero/missing Created timestamp")
}
// If you add a model with no Created field, check for its output as well.If the dmrm.OpenAIModel struct does not allow omitting the Created field (i.e., it's not a pointer or omitempty), you can only test with zero value. If it does, add a model without the Created field and update the test accordingly.
You may need to adjust the output check depending on how prettyPrintOpenAIModels formats models with zero or missing timestamps (e.g., "N/A", "Unknown", or blank).
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for external OpenAI-compatible endpoints by introducing host/port/url/url-alias flags to pull, push, run, and list commands. The changes enable users to connect to custom model servers beyond the default Docker Model Runner.
Key Changes:
- Added server connection flags (--host, --port, --url, --url-alias) to CLI commands
- Implemented OpenAI endpoint detection and path routing logic
- Added comprehensive test coverage for new flag functionality
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/cli/desktop/desktop.go | Modified ListOpenAI and ChatWithContext to route requests differently for external OpenAI endpoints |
| cmd/cli/desktop/context.go | Added context creation functions for custom host/port/URL with external OpenAI support |
| cmd/cli/commands/utils.go | Added helper functions to override model runner context with custom connection parameters |
| cmd/cli/commands/run.go | Integrated server connection flags and URL resolution into run command |
| cmd/cli/commands/push.go | Added host/port flags to push command |
| cmd/cli/commands/pull.go | Added host/port flags to pull command |
| cmd/cli/commands/list.go | Added server connection flags and OpenAI model list formatting support |
| cmd/cli/commands/backend.go | Implemented URL resolution logic and server preset configurations |
| cmd/cli/commands/run_test.go | Added tests verifying presence and types of new server connection flags |
| cmd/cli/commands/list_test.go | Added tests for OpenAI model list formatting functionality |
| cmd/cli/commands/backend_test.go | Added comprehensive tests for URL resolution, backend validation, and API key handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return "", errors.New("OPENAI_API_KEY environment variable is required when using --backend=openai") | ||
| if apiKey != "" { | ||
| return apiKey, nil | ||
| } |
There was a problem hiding this comment.
The ensureAPIKey function no longer returns an error when the API key is missing for OpenAI backend. This is a breaking change from the original behavior where it returned an error message 'OPENAI_API_KEY environment variable is required when using --backend=openai'. This change may cause issues for code that expects an error to be returned when the API key is missing.
| } | |
| } | |
| return "", fmt.Errorf("OPENAI_API_KEY environment variable is required when using --backend=openai") |
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: llamacpp, ollama, openrouter", urlAlias) | ||
| } | ||
|
|
||
| apiKey = os.Getenv("OPENAI_API_KEY") | ||
| } else if hostPortSpecified { |
There was a problem hiding this comment.
Missing validation for required API key when using openrouter url-alias. The openrouter preset requires an API key to function, but there's no check to ensure it exists before returning. Consider adding validation similar to the original ensureAPIKey behavior for openrouter specifically.
| break | |
| } | |
| } | |
| if !found { | |
| return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: llamacpp, ollama, openrouter", urlAlias) | |
| } | |
| apiKey = os.Getenv("OPENAI_API_KEY") | |
| } else if hostPortSpecified { | |
| // Check for required API key for openrouter | |
| apiKey = os.Getenv("OPENAI_API_KEY") | |
| if preset.Name == "openrouter" && apiKey == "" { | |
| return "", false, "", fmt.Errorf("the openrouter preset requires an API key. Please set the OPENAI_API_KEY environment variable.") | |
| } | |
| break | |
| } | |
| } | |
| if !found { | |
| return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: llamacpp, ollama, openrouter", urlAlias) | |
| } |
![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 valuable functionality by adding flags to support external OpenAI-compatible endpoints for several commands. The implementation is well-structured and includes a comprehensive set of tests. My review highlights a critical issue with a failing test, a high-severity bug where a required API key is not enforced, and a few medium-severity suggestions to improve code maintainability and address a discrepancy between the code and the pull request description. Overall, these are great additions, and addressing the feedback will enhance the robustness of the new features.
| name: "openai backend without key", | ||
| backend: "openai", | ||
| wantErr: true, | ||
| }, |
There was a problem hiding this comment.
This test case expects ensureAPIKey to return an error when the openai backend is used without an API key. However, the implementation of ensureAPIKey in cmd/cli/commands/backend.go was changed to no longer return an error in this scenario (it returns "", nil). This test is now incorrect and will fail. The test should be updated to reflect the new behavior of ensureAPIKey.
| name: "openai backend without key", | |
| backend: "openai", | |
| wantErr: true, | |
| }, | |
| { | |
| name: "openai backend without key", | |
| backend: "openai", | |
| wantErr: false, | |
| }, |
| return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: llamacpp, ollama, openrouter", urlAlias) | ||
| } | ||
|
|
||
| apiKey = os.Getenv("OPENAI_API_KEY") |
There was a problem hiding this comment.
The logic to enforce OPENAI_API_KEY for the openrouter alias is missing. The function currently retrieves the API key but doesn't return an error if it's missing for openrouter, which is a required behavior. This will cause the openrouter url-alias without API key test in backend_test.go to fail.
apiKey = os.Getenv("OPENAI_API_KEY")
if urlAlias == "openrouter" && apiKey == "" {
return "", false, "", errors.New("OPENAI_API_KEY environment variable is required when using --url-alias=openrouter")
}| if !found { | ||
| return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: llamacpp, ollama, openrouter", urlAlias) | ||
| } |
There was a problem hiding this comment.
The error message for an invalid url-alias hardcodes the list of valid options. This can become out of sync if the ServerPresets slice is updated. For better maintainability, this list should be generated dynamically from ServerPresets.
if !found {
validAliases := make([]string, len(ServerPresets))
for i, p := range ServerPresets {
validAliases[i] = p.Name
}
return "", false, "", fmt.Errorf("invalid url-alias '%s'. Valid options are: %s", urlAlias, strings.Join(validAliases, ", "))
}| c.Flags().StringVar(&host, "host", "", "Host address to bind Docker Model Runner (default \"127.0.0.1\")") | ||
| c.Flags().IntVar(&port, "port", 0, "Docker container port for Docker Model Runner (default: 12434)") |
There was a problem hiding this comment.
The pull command (and push command) only receives the --host and --port flags. However, the pull request description states that --url and --url-alias flags should also be added to these commands. This is a discrepancy between the implementation and the description. If pull and push are only meant to work with the local model runner, then the PR description should be updated to reflect this.
| // Use HTTP default client for custom host/port | ||
| client := http.DefaultClient | ||
|
|
||
| if userAgent := os.Getenv("USER_AGENT"); userAgent != "" { | ||
| setUserAgent(client, userAgent) | ||
| } |
There was a problem hiding this comment.
The code for creating a default HTTP client and setting the user agent is duplicated here and in NewContextWithURLExternal (lines 216-221). This could be extracted into a private helper function to reduce duplication and improve maintainability.
For example:
func newHTTPClientWithUserAgent() DockerHttpClient {
client := http.DefaultClient
if userAgent := os.Getenv("USER_AGENT"); userAgent != "" {
setUserAgent(client, userAgent)
}
return client
}Then both NewContextWithHostPort and NewContextWithURLExternal can use client := newHTTPClientWithUserAgent().
Add external OpenAI endpoint support
Add comprehensive tests for new server connection flags
Summary by Sourcery
Add server connection flags and external OpenAI endpoint support across CLI commands, implement URL resolution and context override logic, update client routing for external endpoints, and include comprehensive tests for these new capabilities
New Features:
Enhancements:
Tests: