Follow-up from code review comments left by Copilot on #4912 on `pkg/vmcp/cli/embedding_manager.go`.
Findings to address
1. Inaccurate comment for Kubernetes URL (lines 97–101)
The package-level comment says the Kubernetes URL is `http://:`, but the actual implementation builds the URL using the ClusterIP service name `mcp-`. The comment should be updated to reflect the real address.
2. Missing nil-check for `factory` in `NewEmbeddingServiceManager` (lines 131–132)
`NewEmbeddingServiceManager` validates `cfg.Model` but does not guard against a nil `ContainerFactory`. Passing `nil` will cause a panic in `Start`/`Stop` when `m.factory.Create` is called. A nil-check (and matching error return) should be added early in the constructor.
if factory == nil {
return nil, fmt.Errorf("container factory must not be nil")
}The doc comment on the function should be updated to mention the nil-factory case:
Returns an error when factory is nil or cfg.Model is empty.
3. `localhost` vs `127.0.0.1` inconsistency on Docker path (lines 170–176)
The Docker path binds the container port to `HostIP: 127.0.0.1`, but `urlFor` / `healthURLFor` construct URLs with `http://localhost:`. On hosts where `localhost` resolves to IPv6 (`::1`) first this can cause failed health checks or connection delays. Use `127.0.0.1` consistently in both closures (or bind to both addresses).
4. Whitespace-only model value passes validation (lines 133–135)
`cfg.Model == ""` passes for strings that are all whitespace (e.g. `" "`). Such a value will reach the container command as a blank `--model-id` flag and fail at runtime. Validate `strings.TrimSpace(cfg.Model)` instead, and consider storing the trimmed value back into the config.
References
Follow-up from code review comments left by Copilot on #4912 on `pkg/vmcp/cli/embedding_manager.go`.
Findings to address
1. Inaccurate comment for Kubernetes URL (lines 97–101)
The package-level comment says the Kubernetes URL is `http://:`, but the actual implementation builds the URL using the ClusterIP service name `mcp-`. The comment should be updated to reflect the real address.
2. Missing nil-check for `factory` in `NewEmbeddingServiceManager` (lines 131–132)
`NewEmbeddingServiceManager` validates `cfg.Model` but does not guard against a nil `ContainerFactory`. Passing `nil` will cause a panic in `Start`/`Stop` when `m.factory.Create` is called. A nil-check (and matching error return) should be added early in the constructor.
The doc comment on the function should be updated to mention the nil-factory case:
3. `localhost` vs `127.0.0.1` inconsistency on Docker path (lines 170–176)
The Docker path binds the container port to `HostIP: 127.0.0.1`, but `urlFor` / `healthURLFor` construct URLs with `http://localhost:`. On hosts where `localhost` resolves to IPv6 (`::1`) first this can cause failed health checks or connection delays. Use `127.0.0.1` consistently in both closures (or bind to both addresses).
4. Whitespace-only model value passes validation (lines 133–135)
`cfg.Model == ""` passes for strings that are all whitespace (e.g. `" "`). Such a value will reach the container command as a blank `--model-id` flag and fail at runtime. Validate `strings.TrimSpace(cfg.Model)` instead, and consider storing the trimmed value back into the config.
References