Add context support#808
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a context management system for the Docker Model Runner CLI, enabling users to manage and switch between multiple backend configurations via a new context subcommand. Key additions include a persistent JSON-based context store with file locking and updates to the backend detection logic. Review feedback identifies a critical race condition in the custom file locking implementation, recommends robust error handling for home directory retrieval, and suggests surfacing errors when the context store is unreadable to improve user transparency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a context management system for the Docker Model Runner, allowing users to define, switch, and inspect multiple backend configurations through the new docker model context command suite. The implementation includes a persistent JSON-based storage layer with cross-platform file locking and integrates with the existing context detection logic to support host and TLS settings. Feedback was provided regarding a security vulnerability in the file persistence logic where an ignored error during random byte generation could lead to predictable temporary filenames and potential symlink attacks.
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new global
globalOptionsvariable is never initialized inNewRootCmd, soinitDockerCLIwill dereference a nil*flags.ClientOptionswhenplugin.RunningStandalone()is true; consider constructing and assigningglobalOptionsinNewRootCmdas was done previously with the local variable. - On Windows,
lockFile/unlockFilecreate a newOverlappedstruct for each call and let it go out of scope immediately; it would be safer to store a singleOverlappedassociated with the file (e.g., via a small wrapper type) to ensure its memory remains valid for the lifetime of the lock per theLockFileEx/UnlockFileExAPI expectations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new global `globalOptions` variable is never initialized in `NewRootCmd`, so `initDockerCLI` will dereference a nil `*flags.ClientOptions` when `plugin.RunningStandalone()` is true; consider constructing and assigning `globalOptions` in `NewRootCmd` as was done previously with the local variable.
- On Windows, `lockFile`/`unlockFile` create a new `Overlapped` struct for each call and let it go out of scope immediately; it would be safer to store a single `Overlapped` associated with the file (e.g., via a small wrapper type) to ensure its memory remains valid for the lifetime of the lock per the `LockFileEx`/`UnlockFileEx` API expectations.
## Individual Comments
### Comment 1
<location path="cmd/cli/commands/context.go" line_range="86-92" />
<code_context>
+ Use: "create NAME",
+ Short: "Create a named Model Runner context",
+ Args: cobra.ExactArgs(1),
+ RunE: func(cmd *cobra.Command, args []string) error {
+ name := args[0]
+
+ // Validate and normalise the host URL.
+ if host == "" {
+ return fmt.Errorf("--host is required")
+ }
+ if _, err := url.Parse(host); err != nil {
+ return fmt.Errorf("invalid --host URL: %w", err)
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Tighten host URL validation to avoid accepting malformed endpoints
`url.Parse` accepts many non-network strings (e.g. `foo`, `localhost:1234`) as paths, so invalid hosts can pass validation and then fail later at use.
Please either:
- Use `url.ParseRequestURI`, or
- Parse and then require `u.Scheme != ""` and `u.Host != ""`, optionally restricting schemes to `http`/`https` to match how the endpoint is used.
This will surface invalid endpoints early with a clear error instead of deferring to a runtime failure.
```suggestion
// Validate and normalise the host URL.
if host == "" {
return fmt.Errorf("--host is required")
}
u, err := url.ParseRequestURI(host)
if err != nil {
return fmt.Errorf("invalid --host URL: %w", err)
}
if u.Scheme == "" || u.Host == "" {
return fmt.Errorf("invalid --host URL: must include scheme and host, e.g. https://example.com")
}
if u.Scheme != "http" && u.Scheme != "https" {
return fmt.Errorf("invalid --host URL: unsupported scheme %q (must be http or https)", u.Scheme)
}
// Normalise the host string.
host = u.String()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
doringeman
left a comment
There was a problem hiding this comment.
LGTM!
I wonder if we should add some details about the auto-detected default context, too.
As discussed in Slack, I'll add more details in a follow up PR |
This pull request introduces a new "context" command group to the Docker Model Runner CLI, enabling users to manage named Model Runner contexts (similar to Docker contexts). The changes include the implementation of subcommands for creating, listing, using, removing, and inspecting contexts, as well as a comprehensive suite of tests for these commands. The root command initialization is refactored to support context management, and integration points are added to ensure the correct context is used throughout the CLI.