Add "docker model code" command#264
Conversation
First implemented with aider integration Signed-off-by: Eric Curtin <eric.curtin@docker.com>
Reviewer's GuideThis PR introduces a new Sequence diagram for docker model code command executionsequenceDiagram
actor User
participant CLI as "docker model code"
participant Git
participant Editor
participant ModelRunner
participant Docker
User->>CLI: Run 'docker model code [MODEL] [PROMPT]'
CLI->>Git: Check if in git repo
Git-->>CLI: Repo root path
alt Prompt provided
CLI->>CLI: Use provided prompt
else No prompt provided
CLI->>Editor: Open editor for prompt
Editor-->>CLI: User writes prompt
end
CLI->>ModelRunner: Validate backend/model
ModelRunner-->>CLI: Model info / pull if needed
CLI->>Docker: Run aider container with args
Docker-->>CLI: Aider runs, edits code
CLI-->>User: Output results
Class diagram for new code command and related functionsclassDiagram
class CodeCmd {
+backend : string
+aiderImage : string
+newCodeCmd()
+PreRunE()
+RunE()
+Args()
}
class EditorUtils {
+getPromptFromEditor() string
}
class ModelUtils {
+getModelRunnerURL() string
+validateBackend(backend)
+ensureStandaloneRunnerAvailable()
+pullModel()
+isDockerDesktop() bool
+removeElement(slice, element) []string
}
class DockerUtils {
+runAiderInContainer(cmd, aiderImage, repoRoot, model, prompt, modelRunnerURL) error
}
class DesktopClient {
+Inspect(model, bool)
}
CodeCmd --> EditorUtils : uses
CodeCmd --> ModelUtils : uses
CodeCmd --> DockerUtils : uses
CodeCmd --> DesktopClient : uses
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 introduces a new 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
|
ericcurtin
changed the title
![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:
Blocking 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:
- Refactor the large RunE function in newCodeCmd into smaller, well-defined helpers to improve readability and make the logic easier to test.
- In runAiderInContainer, avoid hardcoding the "openai/" model prefix—use the chosen backend flag to build the model reference dynamically.
- Instead of relying on global desktopClient and modelRunner variables, inject these dependencies into the command for better modularity and unit testing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Refactor the large RunE function in newCodeCmd into smaller, well-defined helpers to improve readability and make the logic easier to test.
- In runAiderInContainer, avoid hardcoding the "openai/" model prefix—use the chosen backend flag to build the model reference dynamically.
- Instead of relying on global desktopClient and modelRunner variables, inject these dependencies into the command for better modularity and unit testing.
## Individual Comments
### Comment 1
<location> `cmd/cli/commands/code_test.go:47` </location>
<code_context>
+ }
+}
+
+func TestGetPromptFromEditor(t *testing.T) {
+ // Save original environment
+ origEditor := os.Getenv("EDITOR")
</code_context>
<issue_to_address>
**suggestion (testing):** No test for editor failure or temp file creation error.
Add tests for editor launch failures and temp file creation errors to ensure getPromptFromEditor handles errors correctly.
Suggested implementation:
```golang
func TestGetPromptFromEditor(t *testing.T) {
// Save original environment
origEditor := os.Getenv("EDITOR")
origVisual := os.Getenv("VISUAL")
defer func() {
os.Setenv("EDITOR", origEditor)
os.Setenv("VISUAL", origVisual)
}()
// Test with a mock editor that writes to the temp file
tmpDir := t.TempDir()
mockEditor := filepath.Join(tmpDir, "mock-editor.sh")
mockScript := `#!/bin/bash
`
// ... existing test code for successful editor ...
// Test editor launch failure
t.Run("editor launch failure", func(t *testing.T) {
os.Setenv("EDITOR", "/nonexistent-editor")
_, err := getPromptFromEditor()
if err == nil {
t.Errorf("expected error when editor does not exist, got nil")
}
})
// Test temp file creation failure by monkey-patching os.CreateTemp
t.Run("temp file creation failure", func(t *testing.T) {
origCreateTemp := osCreateTemp
defer func() { osCreateTemp = origCreateTemp }()
osCreateTemp = func(dir, pattern string) (*os.File, error) {
return nil, fmt.Errorf("simulated temp file creation error")
}
_, err := getPromptFromEditor()
if err == nil {
t.Errorf("expected error when temp file creation fails, got nil")
}
})
```
You will need to refactor `getPromptFromEditor` to use a variable (e.g., `osCreateTemp`) for temp file creation, so it can be monkey-patched in the test. For example:
```go
var osCreateTemp = os.CreateTemp
```
And replace all `os.CreateTemp` calls in `getPromptFromEditor` with `osCreateTemp`.
Also, ensure `getPromptFromEditor` returns errors properly when editor launch or temp file creation fails.
</issue_to_address>
### Comment 2
<location> `cmd/cli/commands/code_test.go:26` </location>
<code_context>
+ }
+}
+
+func TestGetPromptFromEditorFiltersComments(t *testing.T) {
+ // Save original environment
+ origEditor := os.Getenv("EDITOR")
</code_context>
<issue_to_address>
**suggestion (testing):** No test for empty prompt after filtering comments.
Please add a test where only comment lines are provided, so the function's behavior with an empty prompt is verified.
```suggestion
}
func TestGetPromptFromEditorOnlyCommentsYieldsEmptyPrompt(t *testing.T) {
// Save original environment
origEditor := os.Getenv("EDITOR")
origVisual := os.Getenv("VISUAL")
defer func() {
os.Setenv("EDITOR", origEditor)
os.Setenv("VISUAL", origVisual)
}()
// Test with a mock editor that writes only comment lines to the temp file
tmpDir := t.TempDir()
mockEditor := filepath.Join(tmpDir, "mock-editor-comments.sh")
mockScript := `#!/bin/bash
echo "# This is a comment" > $1
echo "# Another comment line" >> $1
`
if err := os.WriteFile(mockEditor, []byte(mockScript), 0755); err != nil {
t.Fatalf("failed to write mock editor script: %v", err)
}
os.Setenv("EDITOR", mockEditor)
os.Setenv("VISUAL", "")
prompt, err := getPromptFromEditor()
if err != nil {
t.Fatalf("getPromptFromEditor() returned error: %v", err)
}
if prompt != "" {
t.Errorf("expected prompt to be empty after filtering comments, got: %q", prompt)
}
}
```
</issue_to_address>
### Comment 3
<location> `cmd/cli/commands/code_test.go:174` </location>
<code_context>
+ }
+}
+
+func TestGetModelRunnerURL(t *testing.T) {
+ // Save original environment
+ origHost := os.Getenv("MODEL_RUNNER_HOST")
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for Docker Desktop and modelRunner context.
Please add tests for cases where modelRunner is set with various EngineKind values and for Docker Desktop context to ensure full coverage of getModelRunnerURL.
Suggested implementation:
```golang
tests := []struct {
name string
context string
engineKind string
envHost string
expected string
```
```golang
tests := []struct {
name string
context string
engineKind string
envHost string
expected string
}{
{
name: "Default context, no env",
context: "",
engineKind: "",
envHost: "",
expected: "http://localhost:8080",
},
{
name: "MODEL_RUNNER_HOST set",
context: "",
engineKind: "",
envHost: "http://custom-host:9000",
expected: "http://custom-host:9000",
},
{
name: "modelRunner context, EngineKind=local",
context: "modelRunner",
engineKind: "local",
envHost: "",
expected: "http://localhost:8080",
},
{
name: "modelRunner context, EngineKind=remote",
context: "modelRunner",
engineKind: "remote",
envHost: "",
expected: "https://modelrunner.example.com",
},
{
name: "Docker Desktop context",
context: "docker-desktop",
engineKind: "",
envHost: "",
expected: "http://host.docker.internal:8080",
},
}
```
```golang
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if tt.envHost != "" {
os.Setenv("MODEL_RUNNER_HOST", tt.envHost)
} else {
os.Unsetenv("MODEL_RUNNER_HOST")
}
result := getModelRunnerURL(tt.context, tt.engineKind)
if result != tt.expected {
t.Errorf("expected %v, got %v", tt.expected, result)
}
})
}
}
```
1. You will need to update the signature of `getModelRunnerURL` to accept `context` and `engineKind` parameters, and update its implementation to handle these cases.
2. Ensure that the logic for Docker Desktop and modelRunner context with different EngineKind values is implemented in `getModelRunnerURL`.
3. If your codebase uses a different way to determine context/engineKind, adjust the test table and function calls accordingly.
</issue_to_address>
### Comment 4
<location> `cmd/cli/commands/code_test.go:10` </location>
<code_context>
+ "testing"
+)
+
+func TestCodeCommandFlags(t *testing.T) {
+ cmd := newCodeCmd()
+
</code_context>
<issue_to_address>
**suggestion (testing):** No test for flag value override via command line.
Please add a test that sets flags using command line arguments and checks that the override works as intended.
Suggested implementation:
```golang
func TestCodeCommandFlags(t *testing.T) {
cmd := newCodeCmd()
}
// Test that command line flag overrides work as intended
func TestCodeCommandFlagOverride(t *testing.T) {
cmd := newCodeCmd()
// Example: Assume the command has a --path flag with default value "default/path"
defaultPath := cmd.Flag("path").Value.String()
if defaultPath != "default/path" {
t.Fatalf("expected default path to be 'default/path', got '%s'", defaultPath)
}
// Simulate command line args to override the flag
args := []string{"--path=overridden/path"}
cmd.SetArgs(args)
// Parse flags
err := cmd.Execute()
if err != nil {
// If Execute() runs the command, use cmd.ParseFlags(args) or cmd.Flags().Parse(args) instead
// depending on your CLI library. Adjust as needed.
t.Fatalf("unexpected error executing command: %v", err)
}
// Check that the flag value was overridden
overriddenPath := cmd.Flag("path").Value.String()
if overriddenPath != "overridden/path" {
t.Errorf("expected path to be overridden to 'overridden/path', got '%s'", overriddenPath)
}
}
```
- You may need to adjust the flag name ("path") and default value ("default/path") to match your actual command's flags.
- If your CLI library does not support cmd.Flag(), use the appropriate method to access flag values.
- If cmd.Execute() runs the command logic, use cmd.ParseFlags(args) or cmd.Flags().Parse(args) to only parse flags.
- Ensure your command is set up to allow flag parsing without side effects during tests.
</issue_to_address>
### Comment 5
<location> `cmd/cli/commands/code.go:158` </location>
<code_context>
editorCmd := exec.Command(editor, tmpPath)
</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.
| if aiderImage != "paulgauthier/aider" { | ||
| t.Errorf("expected default aider-image to be 'paulgauthier/aider', got %q", aiderImage) | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (testing): No test for empty prompt after filtering comments.
Please add a test where only comment lines are provided, so the function's behavior with an empty prompt is verified.
| } | |
| } | |
| func TestGetPromptFromEditorOnlyCommentsYieldsEmptyPrompt(t *testing.T) { | |
| // Save original environment | |
| origEditor := os.Getenv("EDITOR") | |
| origVisual := os.Getenv("VISUAL") | |
| defer func() { | |
| os.Setenv("EDITOR", origEditor) | |
| os.Setenv("VISUAL", origVisual) | |
| }() | |
| // Test with a mock editor that writes only comment lines to the temp file | |
| tmpDir := t.TempDir() | |
| mockEditor := filepath.Join(tmpDir, "mock-editor-comments.sh") | |
| mockScript := `#!/bin/bash | |
| echo "# This is a comment" > $1 | |
| echo "# Another comment line" >> $1 | |
| ` | |
| if err := os.WriteFile(mockEditor, []byte(mockScript), 0755); err != nil { | |
| t.Fatalf("failed to write mock editor script: %v", err) | |
| } | |
| os.Setenv("EDITOR", mockEditor) | |
| os.Setenv("VISUAL", "") | |
| prompt, err := getPromptFromEditor() | |
| if err != nil { | |
| t.Fatalf("getPromptFromEditor() returned error: %v", err) | |
| } | |
| if prompt != "" { | |
| t.Errorf("expected prompt to be empty after filtering comments, got: %q", prompt) | |
| } | |
| } |
| tmpFile.Close() | ||
|
|
||
| // Open the editor | ||
| editorCmd := exec.Command(editor, tmpPath) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new docker model code command that integrates aider (AI-powered code editing tool) to run within a Docker container and interact with Docker Model Runner for AI-assisted code editing.
- Adds a new
codecommand to the CLI with support for model selection and prompt input - Implements integration with aider running in an ephemeral Docker container
- Provides editor-based prompt composition similar to git commit workflow
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| cmd/cli/commands/root.go | Registers the new newCodeCmd() command in the root command structure |
| cmd/cli/commands/code.go | Core implementation of the code command with aider integration and Docker container execution |
| cmd/cli/commands/code_test.go | Comprehensive test suite covering command validation, editor integration, and utility functions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if strings.TrimSpace(prompt) == "" { | ||
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") | ||
| return nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The inner condition on line 63 will never be true since line 62 already checks if prompt is empty. The nested check for strings.TrimSpace(prompt) == \"\" is unreachable.
| if strings.TrimSpace(prompt) == "" { | |
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") | |
| return nil | |
| } | |
| } | |
| } | |
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") | |
| return nil | |
| } | |
| } | |
| } |
| # Please enter the commit message for your changes. Lines starting | ||
| # with '#' will be ignored, and an empty message aborts the commit |
There was a problem hiding this comment.
The instruction text refers to 'commit message' but this is for composing a coding prompt. Should be 'Please enter the prompt for your coding task' or similar.
| # Please enter the commit message for your changes. Lines starting | |
| # with '#' will be ignored, and an empty message aborts the commit | |
| # Please enter the prompt for your coding task below. Lines starting | |
| # with '#' will be ignored, and an empty prompt will abort the operation. |
| return fmt.Errorf("failed to get prompt from editor: %w", err) | ||
| } | ||
| if strings.TrimSpace(prompt) == "" { | ||
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") |
There was a problem hiding this comment.
Error message refers to 'commit message' but should refer to 'prompt' since this is about coding task prompts, not git commits.
| func runAiderInContainer(cmd *cobra.Command, aiderImage, repoRoot, model, prompt, modelRunnerURL string) error { | ||
| model = "openai/" + model |
There was a problem hiding this comment.
Hard-coding the 'openai/' prefix for all models may not be appropriate. This should be conditional based on the backend or model configuration.
| func runAiderInContainer(cmd *cobra.Command, aiderImage, repoRoot, model, prompt, modelRunnerURL string) error { | |
| model = "openai/" + model | |
| func runAiderInContainer(cmd *cobra.Command, aiderImage, repoRoot, model, prompt, modelRunnerURL string, backend string) error { | |
| // Only prepend "openai/" if backend is openai | |
| if backend == "openai" && !strings.HasPrefix(model, "openai/") { | |
| model = "openai/" + model | |
| } |
| aiderArgs = removeElement(aiderArgs, "host") | ||
| } | ||
|
|
||
| cmd.Printf("Running aider with model %s %s...\n", model, aiderArgs) |
There was a problem hiding this comment.
The format string expects two parameters but aiderArgs is a slice. This will print the slice address rather than the arguments. Consider using strings.Join(aiderArgs, \" \") or a different format.
| cmd.Printf("Running aider with model %s %s...\n", model, aiderArgs) | |
| cmd.Printf("Running aider with model %s %s...\n", model, strings.Join(aiderArgs, " ")) |
![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 code command, which integrates with aider for AI-assisted coding. The implementation is solid and includes a good set of tests. My review focuses on a couple of areas for improvement: fixing a bug in the command-line prompt handling and refactoring the logic for manipulating command-line arguments to be more robust and efficient. These changes will improve the command's correctness and maintainability.
| if argsLen > 1 { | ||
| prompt = strings.Join(args[1:], " ") | ||
| if prompt == "" { | ||
| if strings.TrimSpace(prompt) == "" { | ||
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // If no prompt provided, open editor | ||
| if prompt == "" { | ||
| var err error | ||
| prompt, err = getPromptFromEditor() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get prompt from editor: %w", err) | ||
| } | ||
| if strings.TrimSpace(prompt) == "" { | ||
| fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n") | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling the prompt argument is a bit convoluted and has a bug. If a user provides a prompt that consists only of whitespace (e.g., docker model code mymodel " "), the command will proceed with the whitespace prompt instead of opening the editor as intended. The logic can be simplified to correctly handle all cases: no prompt, an empty prompt, or a whitespace-only prompt.
if argsLen > 1 {
prompt = strings.Join(args[1:], " ")
}
// If no prompt provided, or if it's all whitespace, open editor
if strings.TrimSpace(prompt) == "" {
var err error
prompt, err = getPromptFromEditor()
if err != nil {
return fmt.Errorf("failed to get prompt from editor: %w", err)
}
if strings.TrimSpace(prompt) == "" {
fmt.Fprintf(os.Stderr, "Aborting coding task due to empty commit message.\n")
return nil
}
}| if isDockerDesktop() { | ||
| // Remove --network host and use Docker Desktop's DNS | ||
| aiderArgs = removeElement(aiderArgs, "--network") | ||
| aiderArgs = removeElement(aiderArgs, "host") | ||
| } |
There was a problem hiding this comment.
The current method of removing the --network host arguments is fragile. It uses a generic removeElement function twice, which could unintentionally remove other arguments if they happen to be "host". A more robust approach is to specifically find and remove the --network host pair. This also allows removing the now-unnecessary removeElement helper function.
if isDockerDesktop() {
// Remove --network host and use Docker Desktop's DNS
newAiderArgs := make([]string, 0, len(aiderArgs))
for i := 0; i < len(aiderArgs); i++ {
if aiderArgs[i] == "--network" && i+1 < len(aiderArgs) && aiderArgs[i+1] == "host" {
i++ // Skip both "--network" and "host"
} else {
newAiderArgs = append(newAiderArgs, aiderArgs[i])
}
}
aiderArgs = newAiderArgs
}| // removeElement removes all occurrences of a string from a slice | ||
| func removeElement(slice []string, element string) []string { | ||
| result := []string{} | ||
| for _, item := range slice { | ||
| if item != element { | ||
| result = append(result, item) | ||
| } | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
With the suggested change to how command-line arguments are removed, this helper function is no longer needed and can be removed to simplify the code.
| func TestRemoveElement(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| slice []string | ||
| element string | ||
| expected []string | ||
| }{ | ||
| { | ||
| name: "remove single occurrence", | ||
| slice: []string{"a", "b", "c"}, | ||
| element: "b", | ||
| expected: []string{"a", "c"}, | ||
| }, | ||
| { | ||
| name: "remove multiple occurrences", | ||
| slice: []string{"a", "b", "c", "b"}, | ||
| element: "b", | ||
| expected: []string{"a", "c"}, | ||
| }, | ||
| { | ||
| name: "element not present", | ||
| slice: []string{"a", "b", "c"}, | ||
| element: "d", | ||
| expected: []string{"a", "b", "c"}, | ||
| }, | ||
| { | ||
| name: "empty slice", | ||
| slice: []string{}, | ||
| element: "a", | ||
| expected: []string{}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| result := removeElement(tt.slice, tt.element) | ||
| if len(result) != len(tt.expected) { | ||
| t.Errorf("expected length %d, got %d", len(tt.expected), len(result)) | ||
| return | ||
| } | ||
| for i := range result { | ||
| if result[i] != tt.expected[i] { | ||
| t.Errorf("expected %v, got %v", tt.expected, result) | ||
| return | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Since the removeElement function is being removed, this corresponding test function is no longer needed and should also be removed.
Do you plan to make it generic or stick to aider? |
First implemented with aider integration
Summary by Sourcery
Introduce a new 'code' command to the Docker CLI that launches the aider AI assistant in a container for interactive code editing and add associated helper functions and tests.
New Features:
docker model codecommand to run aider in an ephemeral Docker container for AI-driven code editsEnhancements:
Tests:
getModelRunnerURLURL resolution logicremoveElementslice utility