From 42d8f468de78d74602f2abe9c5544a646d510759 Mon Sep 17 00:00:00 2001 From: Eric Curtin Date: Wed, 20 May 2026 13:10:19 +0100 Subject: [PATCH] refactor: deduplicate shared driver and provider constants Move default_sandbox_image() and SUPERVISOR_IMAGE_BINARY_PATH into openshell-core so all compute drivers share a single authoritative definition. Eliminates four identical copies of default_sandbox_image() and two identical copies of SUPERVISOR_IMAGE_BINARY_PATH. Implement ProviderPlugin for ProviderDiscoverySpec to remove the repeated three-method delegation boilerplate from eight simple provider modules. Each now declares only a SPEC constant; the blanket impl handles id(), discover_existing(), and credential_env_vars(). --- crates/openshell-core/src/driver_utils.rs | 7 ++++ crates/openshell-core/src/image.rs | 9 +++++ crates/openshell-driver-docker/src/lib.rs | 14 ++------ .../openshell-driver-kubernetes/src/config.rs | 9 +---- .../openshell-driver-kubernetes/src/driver.rs | 8 +---- crates/openshell-driver-podman/src/config.rs | 9 +---- crates/openshell-providers/src/lib.rs | 35 ++++++++++++++----- .../src/providers/anthropic.rs | 20 +---------- .../src/providers/claude.rs | 20 +---------- .../src/providers/codex.rs | 20 +---------- .../src/providers/copilot.rs | 20 +---------- .../src/providers/github.rs | 20 +---------- .../src/providers/gitlab.rs | 20 +---------- .../src/providers/nvidia.rs | 20 +---------- .../src/providers/openai.rs | 20 +---------- crates/openshell-server/src/compute/vm.rs | 9 +---- 16 files changed, 57 insertions(+), 203 deletions(-) diff --git a/crates/openshell-core/src/driver_utils.rs b/crates/openshell-core/src/driver_utils.rs index c5e6f7e87..d961eec86 100644 --- a/crates/openshell-core/src/driver_utils.rs +++ b/crates/openshell-core/src/driver_utils.rs @@ -5,6 +5,13 @@ use crate::proto::compute::v1::DriverSandbox; +/// Path to the sandbox supervisor binary inside the container image. +/// +/// All compute drivers must launch this binary as the container entrypoint to +/// start the sandboxed environment. The value must be kept in sync with the +/// path used when building the `openshell-sandbox` image layer. +pub const SUPERVISOR_IMAGE_BINARY_PATH: &str = "/openshell-sandbox"; + /// Return the effective log level for a sandbox. /// /// Uses the level from the sandbox spec when non-empty, falling back to diff --git a/crates/openshell-core/src/image.rs b/crates/openshell-core/src/image.rs index 6a628e2a9..e804afd60 100644 --- a/crates/openshell-core/src/image.rs +++ b/crates/openshell-core/src/image.rs @@ -13,6 +13,15 @@ /// Override at runtime with the `OPENSHELL_COMMUNITY_REGISTRY` env var. pub const DEFAULT_COMMUNITY_REGISTRY: &str = "ghcr.io/nvidia/openshell-community/sandboxes"; +/// Return the default sandbox image reference (`{registry}/base:latest`). +/// +/// Used by all compute drivers as the fallback image when none is specified in +/// the sandbox spec. +#[must_use] +pub fn default_sandbox_image() -> String { + format!("{DEFAULT_COMMUNITY_REGISTRY}/base:latest") +} + /// Resolve a user-supplied image string into a fully-qualified reference. /// /// Resolution rules (applied in order): diff --git a/crates/openshell-driver-docker/src/lib.rs b/crates/openshell-driver-docker/src/lib.rs index 3a0772217..ef9ff80dc 100644 --- a/crates/openshell-driver-docker/src/lib.rs +++ b/crates/openshell-driver-docker/src/lib.rs @@ -19,6 +19,7 @@ use bollard::query_parameters::{ use bytes::Bytes; use futures::{Stream, StreamExt}; use openshell_core::config::{DEFAULT_DOCKER_NETWORK_NAME, DEFAULT_STOP_TIMEOUT_SECS}; +use openshell_core::driver_utils::SUPERVISOR_IMAGE_BINARY_PATH; use openshell_core::gpu::cdi_gpu_device_ids; use openshell_core::proto::compute::v1::{ CreateSandboxRequest, CreateSandboxResponse, DeleteSandboxRequest, DeleteSandboxResponse, @@ -68,10 +69,6 @@ const DOCKER_NETWORK_DRIVER: &str = "bridge"; /// explicit `supervisor_bin` override or local build is available. const DEFAULT_DOCKER_SUPERVISOR_IMAGE_REPO: &str = "ghcr.io/nvidia/openshell/supervisor"; -/// Path to the supervisor binary inside the `openshell/supervisor` image -/// (a `FROM scratch` image containing only the binary). -const SUPERVISOR_IMAGE_BINARY_PATH: &str = "/openshell-sandbox"; - /// Return the default `ghcr.io/nvidia/openshell/supervisor:` reference /// used when no supervisor binary override is provided. pub fn default_docker_supervisor_image() -> String { @@ -173,7 +170,7 @@ pub struct DockerComputeConfig { impl Default for DockerComputeConfig { fn default() -> Self { Self { - default_image: default_sandbox_image(), + default_image: openshell_core::image::default_sandbox_image(), image_pull_policy: String::new(), sandbox_namespace: "default".to_string(), grpc_endpoint: String::new(), @@ -189,13 +186,6 @@ impl Default for DockerComputeConfig { } } -fn default_sandbox_image() -> String { - format!( - "{}/base:latest", - openshell_core::image::DEFAULT_COMMUNITY_REGISTRY - ) -} - #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct DockerGuestTlsPaths { pub(crate) ca: PathBuf, diff --git a/crates/openshell-driver-kubernetes/src/config.rs b/crates/openshell-driver-kubernetes/src/config.rs index 3f7888af6..f6e178e88 100644 --- a/crates/openshell-driver-kubernetes/src/config.rs +++ b/crates/openshell-driver-kubernetes/src/config.rs @@ -74,7 +74,7 @@ impl Default for KubernetesComputeConfig { fn default() -> Self { Self { namespace: DEFAULT_K8S_NAMESPACE.to_string(), - default_image: default_sandbox_image(), + default_image: openshell_core::image::default_sandbox_image(), // Default empty so the gateway omits `imagePullPolicy` from pod // specs and Kubernetes applies its own default (Always for `latest`, // IfNotPresent otherwise). `DEFAULT_IMAGE_PULL_POLICY` ("missing") @@ -93,13 +93,6 @@ impl Default for KubernetesComputeConfig { } } -fn default_sandbox_image() -> String { - format!( - "{}/base:latest", - openshell_core::image::DEFAULT_COMMUNITY_REGISTRY - ) -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/openshell-driver-kubernetes/src/driver.rs b/crates/openshell-driver-kubernetes/src/driver.rs index a624f787e..1becb07ce 100644 --- a/crates/openshell-driver-kubernetes/src/driver.rs +++ b/crates/openshell-driver-kubernetes/src/driver.rs @@ -13,6 +13,7 @@ use kube::core::gvk::GroupVersionKind; use kube::core::{DynamicObject, ObjectMeta}; use kube::runtime::watcher::{self, Event}; use kube::{Client, Error as KubeError}; +use openshell_core::driver_utils::SUPERVISOR_IMAGE_BINARY_PATH; use openshell_core::progress::{ PROGRESS_STEP_PULLING_IMAGE, PROGRESS_STEP_REQUESTING_SANDBOX, PROGRESS_STEP_STARTING_SANDBOX, mark_progress_active, mark_progress_complete, mark_progress_detail, @@ -767,13 +768,6 @@ fn supervisor_volume_mount() -> serde_json::Value { }) } -/// Path of the supervisor binary inside the supervisor image. -/// -/// The supervisor image places the binary at the filesystem root. We invoke -/// it directly so the init path does not depend on shell utilities or PATH -/// resolution inside the image. -const SUPERVISOR_IMAGE_BINARY_PATH: &str = "/openshell-sandbox"; - /// Build an image volume that mounts the supervisor OCI image directly. /// /// Requires Kubernetes >= v1.33 (`ImageVolume` beta) or >= v1.36 (GA). diff --git a/crates/openshell-driver-podman/src/config.rs b/crates/openshell-driver-podman/src/config.rs index c78c2b12f..3496a077c 100644 --- a/crates/openshell-driver-podman/src/config.rs +++ b/crates/openshell-driver-podman/src/config.rs @@ -180,7 +180,7 @@ impl Default for PodmanComputeConfig { fn default() -> Self { Self { socket_path: Self::default_socket_path(), - default_image: default_sandbox_image(), + default_image: openshell_core::image::default_sandbox_image(), image_pull_policy: ImagePullPolicy::default(), grpc_endpoint: String::new(), gateway_port: openshell_core::config::DEFAULT_SERVER_PORT, @@ -195,13 +195,6 @@ impl Default for PodmanComputeConfig { } } -fn default_sandbox_image() -> String { - format!( - "{}/base:latest", - openshell_core::image::DEFAULT_COMMUNITY_REGISTRY - ) -} - impl std::fmt::Debug for PodmanComputeConfig { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("PodmanComputeConfig") diff --git a/crates/openshell-providers/src/lib.rs b/crates/openshell-providers/src/lib.rs index 6fa44d147..afdf796e5 100644 --- a/crates/openshell-providers/src/lib.rs +++ b/crates/openshell-providers/src/lib.rs @@ -73,6 +73,25 @@ pub trait ProviderPlugin: Send + Sync { } } +/// Blanket implementation of [`ProviderPlugin`] for [`ProviderDiscoverySpec`]. +/// +/// Providers that only need standard env-var discovery can register their +/// `SPEC` constant directly, instead of defining a dedicated struct and +/// repeating the same three-method delegation. +impl ProviderPlugin for ProviderDiscoverySpec { + fn id(&self) -> &'static str { + self.id + } + + fn discover_existing(&self) -> Result, ProviderError> { + discover_with_spec(self, &RealDiscoveryContext) + } + + fn credential_env_vars(&self) -> &'static [&'static str] { + self.credential_env_vars + } +} + #[derive(Default)] pub struct ProviderRegistry { plugins: HashMap<&'static str, Box>, @@ -82,16 +101,16 @@ impl ProviderRegistry { #[must_use] pub fn new() -> Self { let mut registry = Self::default(); - registry.register(providers::claude::ClaudeProvider); - registry.register(providers::codex::CodexProvider); - registry.register(providers::copilot::CopilotProvider); + registry.register(providers::claude::SPEC); + registry.register(providers::codex::SPEC); + registry.register(providers::copilot::SPEC); registry.register(providers::opencode::OpencodeProvider); registry.register(providers::generic::GenericProvider); - registry.register(providers::openai::OpenaiProvider); - registry.register(providers::anthropic::AnthropicProvider); - registry.register(providers::nvidia::NvidiaProvider); - registry.register(providers::gitlab::GitlabProvider); - registry.register(providers::github::GithubProvider); + registry.register(providers::openai::SPEC); + registry.register(providers::anthropic::SPEC); + registry.register(providers::nvidia::SPEC); + registry.register(providers::gitlab::SPEC); + registry.register(providers::github::SPEC); registry.register(providers::outlook::OutlookProvider); registry } diff --git a/crates/openshell-providers/src/providers/anthropic.rs b/crates/openshell-providers/src/providers/anthropic.rs index f4851dad4..bbc33c558 100644 --- a/crates/openshell-providers/src/providers/anthropic.rs +++ b/crates/openshell-providers/src/providers/anthropic.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct AnthropicProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "anthropic", credential_env_vars: &["ANTHROPIC_API_KEY"], }; -impl ProviderPlugin for AnthropicProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/claude.rs b/crates/openshell-providers/src/providers/claude.rs index 64944fc9e..0bb1a7d6f 100644 --- a/crates/openshell-providers/src/providers/claude.rs +++ b/crates/openshell-providers/src/providers/claude.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct ClaudeProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "claude-code", credential_env_vars: &["ANTHROPIC_API_KEY", "CLAUDE_API_KEY"], }; -impl ProviderPlugin for ClaudeProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/codex.rs b/crates/openshell-providers/src/providers/codex.rs index d9d43264f..757282f60 100644 --- a/crates/openshell-providers/src/providers/codex.rs +++ b/crates/openshell-providers/src/providers/codex.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct CodexProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "codex", credential_env_vars: &["OPENAI_API_KEY"], }; -impl ProviderPlugin for CodexProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/copilot.rs b/crates/openshell-providers/src/providers/copilot.rs index fff74cc3b..986bfef8b 100644 --- a/crates/openshell-providers/src/providers/copilot.rs +++ b/crates/openshell-providers/src/providers/copilot.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct CopilotProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "copilot", credential_env_vars: &["COPILOT_GITHUB_TOKEN", "GH_TOKEN", "GITHUB_TOKEN"], }; -impl ProviderPlugin for CopilotProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/github.rs b/crates/openshell-providers/src/providers/github.rs index 4ca25d6d2..9da1e471f 100644 --- a/crates/openshell-providers/src/providers/github.rs +++ b/crates/openshell-providers/src/providers/github.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct GithubProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "github", credential_env_vars: &["GITHUB_TOKEN", "GH_TOKEN"], }; -impl ProviderPlugin for GithubProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/gitlab.rs b/crates/openshell-providers/src/providers/gitlab.rs index 0f944e09f..c0b55427e 100644 --- a/crates/openshell-providers/src/providers/gitlab.rs +++ b/crates/openshell-providers/src/providers/gitlab.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct GitlabProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "gitlab", credential_env_vars: &["GITLAB_TOKEN", "GLAB_TOKEN", "CI_JOB_TOKEN"], }; -impl ProviderPlugin for GitlabProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/nvidia.rs b/crates/openshell-providers/src/providers/nvidia.rs index 62985c1dd..49983701e 100644 --- a/crates/openshell-providers/src/providers/nvidia.rs +++ b/crates/openshell-providers/src/providers/nvidia.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct NvidiaProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "nvidia", credential_env_vars: &["NVIDIA_API_KEY"], }; -impl ProviderPlugin for NvidiaProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-providers/src/providers/openai.rs b/crates/openshell-providers/src/providers/openai.rs index 0dbe39414..70a66d29f 100644 --- a/crates/openshell-providers/src/providers/openai.rs +++ b/crates/openshell-providers/src/providers/openai.rs @@ -1,31 +1,13 @@ // SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -use crate::{ - ProviderDiscoverySpec, ProviderError, ProviderPlugin, RealDiscoveryContext, discover_with_spec, -}; - -pub struct OpenaiProvider; +use crate::ProviderDiscoverySpec; pub const SPEC: ProviderDiscoverySpec = ProviderDiscoverySpec { id: "openai", credential_env_vars: &["OPENAI_API_KEY"], }; -impl ProviderPlugin for OpenaiProvider { - fn id(&self) -> &'static str { - SPEC.id - } - - fn discover_existing(&self) -> Result, ProviderError> { - discover_with_spec(&SPEC, &RealDiscoveryContext) - } - - fn credential_env_vars(&self) -> &'static [&'static str] { - SPEC.credential_env_vars - } -} - #[cfg(test)] mod tests { use super::SPEC; diff --git a/crates/openshell-server/src/compute/vm.rs b/crates/openshell-server/src/compute/vm.rs index 82b9cb3fd..f6cba21d2 100644 --- a/crates/openshell-server/src/compute/vm.rs +++ b/crates/openshell-server/src/compute/vm.rs @@ -153,7 +153,7 @@ impl Default for VmComputeConfig { Self { state_dir: Self::default_state_dir(), driver_dir: None, - default_image: default_sandbox_image(), + default_image: openshell_core::image::default_sandbox_image(), grpc_endpoint: String::new(), bootstrap_image: String::new(), krun_log_level: Self::default_krun_log_level(), @@ -167,13 +167,6 @@ impl Default for VmComputeConfig { } } -fn default_sandbox_image() -> String { - format!( - "{}/base:latest", - openshell_core::image::DEFAULT_COMMUNITY_REGISTRY - ) -} - #[cfg(unix)] #[derive(Debug, Clone, PartialEq, Eq)] pub struct VmGuestTlsPaths {